Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix n plus one #228

Merged
merged 3 commits into from
Mar 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ group :development, :test do

# Added rails controller to use render_template
gem "rails-controller-testing", "~> 1.0", ">= 1.0.5"

# help to kill N+1 queries and unused eager loading. https://github.com/flyerhzm/bullet
gem "bullet"
end

group :development do
Expand Down
6 changes: 6 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ GEM
bootsnap (1.9.4)
msgpack (~> 1.0)
builder (3.2.4)
bullet (7.0.1)
activesupport (>= 3.0.0)
uniform_notifier (~> 1.11)
capybara (3.36.0)
addressable
matrix
Expand Down Expand Up @@ -423,6 +426,7 @@ GEM
tzinfo (2.0.4)
concurrent-ruby (~> 1.0)
unicode-display_width (2.1.0)
uniform_notifier (1.16.0)
warden (1.2.9)
rack (>= 2.0.9)
web-console (4.2.0)
Expand All @@ -448,6 +452,7 @@ GEM

PLATFORMS
arm64-darwin-21
x86_64-darwin-19
x86_64-darwin-20
x86_64-darwin-21
x86_64-linux
Expand All @@ -456,6 +461,7 @@ DEPENDENCIES
annotate
aws-sdk-s3
bootsnap (>= 1.4.4)
bullet
capybara (>= 3.26)
countries
data_migrate (~> 8.0.0.rc2)
Expand Down
6 changes: 4 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,15 @@ cp .env.example .env
10. Update `APP_BASE_URL` in `.env` to `localhost:3000`
11. Run `bin/rails db:create RAILS_ENV=development` to create the database
12. Run `bin/rails db:migrate RAILS_ENV=development` for migrations
13. Run app in local env
13. Run `bin/rails db:seed` for populating the database with initial data

14. Run app in local env

```
foreman start -f Procfile.dev
```

14. Navigate to [http://0.0.0.0:3000](http://0.0.0.0:3000)
15. Navigate to [http://0.0.0.0:3000](http://0.0.0.0:3000)

### To receive the emails in non-production apps.

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/concerns/current_company_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ module CurrentCompanyConcern
def current_company
return if current_user.nil?

@_current_company ||= current_user&.current_workspace || current_user.companies.first
@_current_company ||= current_user&.current_workspace || current_user.companies.includes(:logo_attachment).first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put current_user.companies.includes(:logo_attachment).first inside current_workspace method? @alkesh26

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keshavbiswa I am not fully aware of the current_workspace context here and the impact if we change the method to companies.includes(:logo_attachment).first. We will discuss this tomorrow. For now I will merge this PR.

end
end
2 changes: 1 addition & 1 deletion app/controllers/internal_api/v1/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def index
private

def entries
current_company.timesheet_entries.map do |timesheet_entry|
current_company.timesheet_entries.includes([:user, :project]).map do |timesheet_entry|
timesheet_entry.formatted_entry.transform_keys { |key| key.to_s.camelize(:lower) }
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/team_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class TeamController < ApplicationController
after_action :assign_role, only: [:update]

def index
query = current_company.users.ransack(params[:q])
query = current_company.users.includes([:avatar_attachment, :roles]).ransack(params[:q])
teams = query.result(distinct: true)
render :index, locals: { query:, teams: }
end
Expand Down
12 changes: 8 additions & 4 deletions app/controllers/time_tracking_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,14 @@ def index
projects = {}
clients.map { |client| projects[client.name] = client.projects }

timesheet_entries = current_user.timesheet_entries.in_workspace(current_company).during(
Date.today.beginning_of_week,
Date.today.end_of_week
)
timesheet_entries = current_user
.timesheet_entries
.includes([:project, :user])
.in_workspace(current_company)
.during(
Date.today.beginning_of_week,
Date.today.end_of_week
)
entries = formatted_entries_by_date(timesheet_entries)

render :index, locals: { is_admin:, clients:, projects:, entries: }
Expand Down
9 changes: 8 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ class User < ApplicationRecord
include Discard::Model

# Associations
belongs_to :current_workspace, class_name: "Company", foreign_key: :current_workspace_id, optional: true
has_many :company_users, dependent: :destroy
has_many :companies, through: :company_users
has_many :project_members, dependent: :destroy
Expand Down Expand Up @@ -94,6 +93,14 @@ def has_owner_or_admin_role?(company)
self.has_cached_role?(:owner, company) || self.has_cached_role?(:admin, company)
end

def current_workspace(load_associations: [:logo_attachment])
@_current_workspace ||= Company.includes(load_associations).find_by(id: current_workspace_id)
end

def current_workspace=(workspace)
write_attribute(:current_workspace_id, workspace&.id)
end

private

def discard_project_members
Expand Down
2 changes: 1 addition & 1 deletion app/views/partial/_navbar.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@

<div x-show="isOpen" @click.away="isOpen = false">
<div class="flex flex-col absolute right-0 mt-2 w-64 rounded-md shadow-lg bg-white ring-1 ring-black ring-opacity-5 divide-y divide-gray-100 focus:outline-none" role="menu" aria-orientation="vertical" aria-labelledby="menu-button" tabindex="-1">
<% current_user.companies.each do |company| %>
<% current_user.companies.includes(:logo_attachment).each do |company| %>
<div class="<%= current_user.current_workspace == company ? 'bg-miru-gray-100' : '' %> items-center px-4 py-4 hover:bg-miru-gray-100 text-miru-dark-purple-1000 cursor-pointer rounded-sm" role="menuitem" tabindex="-1" id="menu-item-0">
<%= link_to workspace_path(company), method: :patch, class: "flex" do %>
<%= image_tag company_logo(company), alt: "company_logo", class: "mr-3 h-5 w-5 mt-1" %>
Expand Down
9 changes: 9 additions & 0 deletions config/environments/development.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,13 @@

config.action_mailer.delivery_method = ENV.fetch("MAILER_DELIVERY_METHOD", "letter_opener").to_sym
config.action_mailer.perform_deliveries = true

Rails.application.config.after_initialize do
Bullet.enable = true
Bullet.bullet_logger = true
Bullet.console = true
Bullet.rails_logger = true
Bullet.add_footer = true
Bullet.unused_eager_loading_enable = false
end
end
1 change: 0 additions & 1 deletion spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
it { is_expected.to have_many(:project_members).dependent(:destroy) }
it { is_expected.to have_many(:timesheet_entries) }
it { is_expected.to have_one_attached(:avatar) }
it { is_expected.to belong_to(:current_workspace).optional }
end

describe "Validations" do
Expand Down