[v3.2] Fix variant price performance regressions #4657
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tvdeyen
approved these changes
Oct 4, 2022
mamhoff
force-pushed
the
variant-prices-3.2
branch
from
1b1b581 to
9bc9193
Compare
When we soft-delete a variant, we also soft-delete the variant's prices. In my opinion, this is not necessary, as the association between prices and variants includes discarded prices, and harmful, as it leads to price inconsistencies between a variant before discarding and after undiscarding (if the variant has a newer, but discarded, price, we have no way of knowing that that price should not be un-discarded along with the variant).
Using the `currently_valid` scope in this instance method leads to an "n+1" issue when getting prices for more than one variant. By sorting these in memory rather than in the database, we can save a lot of database round-trips.
This uses the redefined public method from the previous commit to detect the default price. Note that through using `reverse_each.detect` rather than `min` with a block we should be able to save some iterations.
This was the previous behavior.
We don't need to be using the weirdly named `currently_valid_prices` method. As this is the only spot in core where we use that method, we can now deprecate it.
mamhoff
force-pushed
the
variant-prices-3.2
branch
from
9bc9193 to
a920d3f
Compare
This uses the price selector in order to find the default price rather than having to re-implement the default price selector's logic.
Previous Solidus behavior included soft-deleted prices when looking at the default price. The intention of this behavior was that a a variant that is soft-deleted, and has its prices soft-deleted as a consequence, still has a `default_price` record. However, for a `kept` variant, we do not want to see a discarded `default_price` - otherwise what sense does it make to even add the delete action to the price? Especially given that deletion touches the variant and will then move it forward in the array of sorted prices to be considered for a given set of pricing attributes. This commit changes the `prices` association to include discarded prices, but then filters out discarded prices for non-discarded variants in the price selector.
|
Recreated from changes merged in main branch. |
|
Closing in favor of #4690 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Backport of #4639 to the v3.2 branch.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):I have attached screenshots to demo visual changes.I have opened a PR to update the guides.I have updated the readme to account for my changes.