-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow promotion conditions to work with in-memory line items, improve performance #6383
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6383 +/- ##
==========================================
- Coverage 89.46% 89.46% -0.01%
==========================================
Files 977 977
Lines 20386 20394 +8
==========================================
+ Hits 18238 18245 +7
- Misses 2148 2149 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
309d917 to
19592f0
Compare
tvdeyen
left a comment
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.
Great stuff. I think the funky SQL is worth the benefit.
promotions/app/models/concerns/solidus_promotions/conditions/taxon_condition.rb
Show resolved
Hide resolved
promotions/app/models/concerns/solidus_promotions/conditions/taxon_condition.rb
Show resolved
Hide resolved
In-memory line items don't have an ID, but we can just use the line item itself as the key.
This refactors the taxon conditions to rely on in-memory properties of line items rather than on the current database state of the order. The database state of the conditions is still necessary - we're not changing conditions while calculating promotions. The change to the order condition will result in a lot of queries to line item product classifications - but since it's an order level condition, that is only going to happen once. For line item level condition, we're actually removing an n+1 here, because we're instacaching the list of taxons and descendants on the condition instance.
The taxon condition instantiates new in-memory conditions for checking at the order level and at the line item level. The array of arrays of taxons ids with their respective descendants is instacached on the instance. In order to prevent loading this array n times (once for each line item, and once for the order), we hydrate the instance caches on the in-memory conditions.
Promotion calculation should be fast, so let's optimize the slowest parts. This gets the array of taxon ids with children in a single SQL query, rather than with a query per taxon.
If we don't use pluck, we can preload.
19592f0 to
18fd749
Compare
Summary
This PR does four things:
DistributedAmountsHandlerto not require line items IDs, so it can work with in-memory line itemspluckand rather take advantage of loaded option values.The taxon condition is likely one of our most-used conditions, so it makes sense to spend a bit of time here making sure it's not too slow.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: