-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Check if stock_items are loaded before doing a SUM() #5687
base: main
Are you sure you want to change the base?
Check if stock_items are loaded before doing a SUM() #5687
Conversation
@BenMorganIO failure seems actually related to the change, can you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A rebase with latest main should fix the unrelated spec failures also I think we should make it simpler.
280048c
to
93f2419
Compare
93f2419
to
51df6ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenMorganIO thanks. Do you mind to fix the specs and rebase with latest main?
it 'returns the loaded count_on_hand instead of doing SUM(count_on_hand)' do | ||
product.stock_items.first.set_count_on_hand(5) | ||
|
||
product = described_class.includes(:stock_items).find(product.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this override works properly since it is failing on CircleCI. Also spec setup should be in a before
block.
Summary
This PR is about trying to reduce n+1's in stores. If you have a product listing page that also displays the
total_on_hand
then you can get an n+1 with:Now, if you've done
Spree::Product.includes(:stock_items)
you won't have to worry about this.Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: