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

Add ForUserScope concern to DRY & simplify code #955

Merged
merged 5 commits into from
Feb 17, 2024

Conversation

reid-rigo
Copy link
Contributor

@reid-rigo reid-rigo commented Feb 16, 2024

Description

Most of these scopes are the same and only the for_user combined scope is used

Comment on lines -60 to -65
scope :for_user, ->(user) {
joins('
LEFT OUTER JOIN teams_members on teams_members.team_id = teams.id
LEFT OUTER JOIN users on users.id = teams_members.member_id
').where('`teams_members`.`member_id` = ?', user.id)
}
Copy link
Contributor Author

@reid-rigo reid-rigo Feb 16, 2024

Choose a reason for hiding this comment

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

Team.for_user appears to be unused

Copy link
Member

Choose a reason for hiding this comment

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

Could be!

@epugh
Copy link
Member

epugh commented Feb 16, 2024

This is very exciting! Will this reduce the number of queries we run? I believe right now we run three, one for the directly owned, one for the teams, then we pluck the id's, and then do a query for that!

@epugh
Copy link
Member

epugh commented Feb 16, 2024

the test #create#test_alerts_when_a_scorer_associated_with_a_book_does_not_exist failing is my bad!

Copy link
Member

Choose a reason for hiding this comment

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

i notice only a test on case_test.rb is this becasue it would be basically the same darn test and reducndent for book and scorer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah actually let me move these cases into a test file just for this concern, for clarity. Also, I'll add test cases for Scorer.for_user

module ForUserScope
extend ActiveSupport::Concern

included do
Copy link
Member

Choose a reason for hiding this comment

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

This is so pretty!

@epugh epugh merged commit 2609c5d into o19s:main Feb 17, 2024
3 of 4 checks passed
@epugh
Copy link
Member

epugh commented Feb 17, 2024

THank you fo rthis, I've pushed it out to app.quepid.com... It "feels" a bit faster... The homepage on app.quepid.com still struggles to load quickly sigh.

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.

2 participants