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

Optimise and Refactor Org.with_template_and_user_counts #3431

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

aaronskiba
Copy link
Contributor

Changes proposed in this PR:

  • app/models/org.rb

    • Updated the query logic for scope :with_template_and_user_counts. It is now significantly faster (see table at bottom of description).
  • app/views/paginable/orgs/_index.html.erb

    • Updated the code to correspond with the changes made in app/models/org.rb.
    • The code updated in this file is for determining whether or not a listed Org should have the delete option enabled. These PR changes should result in the same exact Orgs having the delete option enabled as before.
  • Refactoring:

  • To reflect these changes, scope: Org.with_template_and_user_counts has been renamed to scope: Org.with_template_count_and_associations_check throughout the codebase.

  • In app/models/org.rb, joins('LEFT OUTER JOIN templates ON orgs.id = templates.org_id') has been replaced with the equivalent left_outer_joins(:templates)

  • The following table compares the development branch to aaron/optimize-scope-with_template_and_user_counts by making requests to the various paths affected by this PR. The benchmarking was performed via ab - Apache HTTP server benchmarking tool alongside a recent (May 2024) db dump from the production environment of DMP Assistant. In both cases, 100 consecutive requests were performed to each path and the recorded result is the mean request time (ms).

Path mean request time (ms) before mean request time (ms) now % change
/super_admin/orgs 1090.878 61.887 -94.33%
/paginable/orgs/index/ALL 3322.034 328.263 -90.12%

`app/views/paginable/orgs/_index.html.erb` was performing the following check:
`<% unless org.user_count > 0 || org.template_count > 0 || org.contributors.length > 0 || org.plans.length > 0 %>`
- However, `org.template_count` is the only value that is actually used (line 19 in `app/views/paginable/orgs/_index.html.erb` `<td><%= org.template_count %></td>`)
- Rather than unnecessarily computing the counts of the other values (`org.user_count`, `org.contributors.length`, and `org.plans.length`), we only need to know if at least one record exists for each. Thus, `scope :with_template_and_user_counts` in `app/models/org.rb` is modified to perform these EXISTS queries.
- Adding the EXISTS queries for `contributors` and `plans` to `scope :with_template_and_user_counts` also resolves a `USE eager loading detected` warning from the Bullet gem.
- This is a small addition to commit 6708697.
- Rather than using all of the virtual attributes (org.has_users, org.has_contributors, org.has_plans), we 'combine' them into org.has_associations.
- Basically, has_associations == (org.has_users || org.has_contributors || org.has_plans)
- The corresponding changes are also made in app/views/paginable/orgs/_index.html.erb
- To reflect the behaviour changes to  Org.with_template_and_user_counts (commits 6708697 and d335d4d), the scope has been renamed to Org.with_template_count_and_associations_check).
- joins('LEFT OUTER JOIN templates ON orgs.id = templates.org_id') has been replaced with left_outer_joins(:templates)
Copy link

</tr>
1 Error
🚫

Please include a CHANGELOG entry.

You can find it at [CHANGELOG.md](https://github.com/DMPRoadmap/roadmap/blob/main/CHANGELOG.md).

Generated by 🚫 Danger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant