Use configurable promo adjuster in callback #5498
Conversation
`Spree::Order#ensure_promotions_eligible` uses the `Spree::Adjustment#calculate_eligibility` in order to determine whether the order's promo total might change. `Spree::Adjustment` should probably not know too much about promotion eligibility, and the error message we add to the order also does only talks about the promo total changing.
The default order promotion adjuster duplicates this logic, and `calculate_eligibility` should now not be called from anywhere in the codebase during normal operation.
| !adjustment.calculate_eligibility | ||
| end | ||
| if adjustment_changed | ||
| Spree::Config.promotion_adjuster_class.new(self).call |
There was a problem hiding this comment.
What this class does seems pretty different from the calculate_eligibility method's content. Can you help me understand in which way they are equivalent in this context?
There was a problem hiding this comment.
It does check the eligibility of each adjustment in recalculate(adjustment) and especially calculate_eligibility(adjustment). The class does a little bit more - it calculates not only eligibility, but also checks whether the adjustment's sum of amount is still correct. So it actually checks the promo total that the error message indicates it should have checked.
There was a problem hiding this comment.
I got it; I am a bit worried about degrading the performance of this method with the call to the promotion_adjuster_class because I guess (I didn't check) it will hit the database many more times. Considering that it only happens on the order completion, it might be a good trade-off. If you or others are not concerned about that, I'm okay with merging this change.
There was a problem hiding this comment.
There's a slight performance degradation for sure. From my experience with the promotion system though, the expensive part is checking the rules for eligibility, and not (re)calculating the discounts.
Also: I fully agree that it's a tradeoff, and that it's okay because it only happens on order completion.
Codecov Report
@@ Coverage Diff @@
## main #5498 +/- ##
=======================================
Coverage 88.93% 88.93%
=======================================
Files 622 622
Lines 14909 14909
=======================================
Hits 13259 13259
Misses 1650 1650
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
Summary
This deprecates
Spree::Adjustment#calculate_eligibilityby refactoringSpree::Order#ensure_promotions_eligibleto use the configurable promotion adjuster rather thanSpree::Adjustment#calculate_eligibility.Checklist
The following are not always needed: