-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add #discounted_amount by adjustments
#6379
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6379 +/- ##
==========================================
+ Coverage 89.46% 89.48% +0.02%
==========================================
Files 977 980 +3
Lines 20386 20427 +41
==========================================
+ Hits 18238 18279 +41
Misses 2148 2148 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a101831 to
3be73ac
Compare
#discounted_amount by adjustments
3be73ac to
536b0ce
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.
This is very clever code, but honestly I am not very happy with this approach.
I would like to not mix in even more modules into already large objects (LineItem, Shipment, etc.) just for storing an in memory calculation.
That we now have a raise condition in a module so we are sure that we never call that public methods outside of the promotion run is a smell imo.
Maybe we can use some kind of object that holds the current state during calculation instead of the objects itself?
Would like to have some thoughts from other @solidusio/core-team members as well .
promotions/app/models/concerns/solidus_promotions/adjustment_discounts.rb
Show resolved
Hide resolved
I've tried - hard, over many months - to use a specialized order object just for promotion calculation. However, in order to discount an order, it needs to be mutated, and I have not found a way to expose all of the order to calculators and conditions (because that's what the calling code expects) from an object that is another class than The goal of this work is to actually go closer to the real order object. To use adjustments/shipping rate discounts to store discounts, rather than the double bookkeeping we have now with discounts while calculating promotions, and adjustments outside of it. A side requirement I have is to require as little change to conditions and calculators as possible, as many stores have many custom conditions and calculators. |
00fd257 to
ac0f616
Compare
This work allows us to remove the The idea in #6371 is to store the results of all calculations on objects that we will ultimately store in the database. |
e43adaf to
6a789c8
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.
Nice. Some suggestions by @manleyac and me
promotions/app/patches/models/solidus_promotions/line_item_patch.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/not_calculating_promotions.rb
Outdated
Show resolved
Hide resolved
This singleton records the currently calculated promotion lane. It has a helper to give us all the lanes before the current lane (or all the lanes if there is no current lane). We overwrite the setter for the lane so that the lane is always a string.
This will replace `#discountable_amount`, and operates on in-memory adjustments.
This is the same code as in Spree::LineItem. We'll move it to a module soon.
Again, this has some code duplication that we'll get rid of in the next commits.
Promotable#discounted_amount is implemented in exactly the same way for all promotables we have: Line items, shipments, and shipping rates. Let's move it to a module.
Line items and shipments use adjustments to record discounts, and their `#discounts_by_lanes` method is identical. Let's DRY this up and extract the method into a module.
When we refactor the promotion system to use in-memory adjustments/discounts to store discounts, we need to be able to ask which discounts are in the current promotion lane. This allows that.
6a789c8 to
ff58cdd
Compare
Summary
This adds the
DiscountedAmountmodule to the promotion system. It is preparatory work to be able to calculate thediscounted_amountfor each discountable item depending on the current promotion lane, without taking into account adjustments/discounts from later lanes or discounts that are market for destruction.This just adds methods, their use will be done in a subsequent PR.
Extracted from #6371 .
This introduces an Object to hold the current promotion lane while calculating, and adds a couple of methods in order to replace a couple of older methods.
New:
discountable.discounted_amount. Will replacediscountable.discountable_amount.Rationale: When called from within the promotion system, these mean the same thing (we can only discount from what is still available to discount, and if there's previous discounts, the discountable amount is equal to the discounted amount). Also:
discounted_amountis calculated from real adjustments, so it can be called from outside the promotion system, and the return value will make sense (it will be equal tototal_before_taxunless someone adds manual adjustments somewhere. That's why this should stay public.New:
discountable.previous_lanes_discounts. Will replacediscountable.current_discountsRationale: We need a different name for this as it returns an array of adjustments, rather than an array of
SolidusPromotion::ItemDiscountobjects. I could make this private, too, as it's only used fromdiscounted_amountabove.New:
discountable.current_lane_discounts. Replaces a local variable in the order discounter.Rationale: We need to know which discounts are being currently added in the current lane so that we can mark those for destruction that we don't want. Here's where it will be used (draft PR): ea4cd30#diff-828a39d27bc67194ff2509c675bf0148420849b1fcafabe848605f4e2d81f085R69.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: