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

Refactor Stock Quantifier to use Enumerable #4291

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Mar 3, 2022

This allows a developer to preload stock items and stock location,
removing many, many n+1 queries to the database.

Checklist:

@mamhoff mamhoff force-pushed the remove-n+1-from-stock-quantifier branch from 5fefd82 to 9ec786c Compare March 3, 2022 18:03
@mamhoff mamhoff force-pushed the remove-n+1-from-stock-quantifier branch from 9ec786c to ff93926 Compare March 4, 2022 10:51
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Very nice work! Thank for all this optimization work. 🙏🏻

I have one tiny non-blocking nit.

core/lib/spree/testing_support/order_walkthrough.rb Outdated Show resolved Hide resolved
core/lib/spree/testing_support/order_walkthrough.rb Outdated Show resolved Hide resolved
@mamhoff mamhoff force-pushed the remove-n+1-from-stock-quantifier branch from ff93926 to e0c6c91 Compare March 4, 2022 11:39
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Hey @mamhoff! I like this change but I don't get who is passing the id of the stock location to the initialize of the quantifier. Is that something you need to change in your own app or something we already support in core?

core/app/models/spree/stock/quantifier.rb Outdated Show resolved Hide resolved
This allows a developer to preload stock items and stock location,
removing many, many n+1 queries to the database.
@mamhoff mamhoff force-pushed the remove-n+1-from-stock-quantifier branch from e0c6c91 to 1b2cf01 Compare March 16, 2022 10:26
We need to reload the variant's stock items in these before blocks now,
otherwise the code under test doesn't see that the count on hand has
changed.
@mamhoff mamhoff force-pushed the remove-n+1-from-stock-quantifier branch from 1b2cf01 to c8cc04b Compare March 30, 2022 12:45
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍

@tvdeyen tvdeyen merged commit 8f4d633 into solidusio:master Apr 11, 2022
kennyadsl added a commit to solidusio/solidus_starter_frontend that referenced this pull request Apr 22, 2022
As done in the main repository, there are some changes
needed in the specs after some changes happened in the
StockQuantifier.

See solidusio/solidus#4291 for more information.
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.

4 participants