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

Fix n plus one #228

merged 3 commits into from
Mar 30, 2022

Conversation

alkesh26
Copy link
Contributor

Notion card

https://saeloun.notion.site/Fix-N-1-query-f7836df2aff240ed8e1b51683c95d920

Summary

  1. Added bullet gem.
  2. Fixed N+1 queries across multiple places

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have manually tested all workflows
  • I have performed a self-review of my own code
  • I have added automated tests for my code

@alkesh26 alkesh26 self-assigned this Mar 30, 2022
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-228 March 30, 2022 08:27 Inactive
@github-actions
Copy link

Current Code Coverage Percent of this PR:

78.65 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/concerns/error_handler.rb 69.44 %
/app/controllers/projects_controller.rb 46.15 %
/app/controllers/concerns/timesheet.rb 75.0 %
/app/controllers/invoices_controller.rb 66.67 %
/app/controllers/workspaces_controller.rb 77.78 %
/app/controllers/clients_controller.rb 94.44 %
/app/models/company.rb 90.91 %
/app/models/timesheet_entry.rb 76.92 %
/app/models/project.rb 89.66 %
/app/policies/timesheet_entry_policy.rb 50.0 %
/app/policies/project_policy.rb 85.71 %
/app/policies/client_policy.rb 80.0 %
/app/controllers/users/passwords_controller.rb 41.67 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/users/sessions_controller.rb 25.0 %
/app/controllers/users/registrations_controller.rb 40.0 %
/app/controllers/internal_api/v1/projects_controller.rb 75.0 %
/app/controllers/internal_api/v1/reports_controller.rb 88.89 %
/app/controllers/internal_api/v1/timesheet_entry_controller.rb 40.0 %
/app/controllers/internal_api/v1/workspaces_controller.rb 85.71 %
/app/controllers/internal_api/v1/clients_controller.rb 80.77 %
/lib/authentication/google.rb 0.0 %

@alkesh26 alkesh26 requested a review from rohitjoshixyz March 30, 2022 08:31
Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rohitjoshixyz rohitjoshixyz left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@@ -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.

Copy link
Contributor

@keshavbiswa keshavbiswa left a comment

Choose a reason for hiding this comment

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

LGTM!

@alkesh26 alkesh26 merged commit 7ee624a into develop Mar 30, 2022
@alkesh26 alkesh26 deleted the fix-n-plus-one branch March 30, 2022 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants