-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Discountable amounts by adjustments #6371
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?
Discountable amounts by adjustments #6371
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6371 +/- ##
==========================================
+ Coverage 89.48% 89.49% +0.01%
==========================================
Files 980 981 +1
Lines 20438 20471 +33
==========================================
+ Hits 18289 18321 +32
- Misses 2149 2150 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
promotions/spec/models/solidus_promotions/order_adjuster_spec.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/concerns/solidus_promotions/discountable.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/concerns/solidus_promotions/discountable.rb
Outdated
Show resolved
Hide resolved
5f2c828 to
fc5b38b
Compare
fc5b38b to
bd24d6b
Compare
67487b3 to
ea30703
Compare
Prior to this, the `adjustment_label` method on
`SolidusPromotions::Benefit` could only output a relatively static
string. This commit allows calculators to define one of these methods:
- `#line_item_adjustment_label(line_item, options = {})`
- `#shipment_adjustment_label(shipment, options = {})`
- `#shipping_rate_adjustment_label(shipping_rate, options = {})`
- `#price_adjustment_label(price, options = {})`
These have access to the inner workings of the calculator, so they can
provide dynamic labels (to reflect e.g. which tier a promotion chooses,
making that transparent to the customer.
If a promotion calculator implements a dynamic
`#{adjustable}_adjustment_label` method, we need to update the
adjustment label when it changes. This makes sure that is the case.
96fab42 to
00fec51
Compare
00fec51 to
e3c0bbf
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 a really great improvement. I have some nits about naming and overall API design, because I would like to make sure that the introduced classes are not used publicly.
promotions/app/models/solidus_promotions/order_adjuster/nullify_order_discounts.rb
Outdated
Show resolved
Hide resolved
promotions/spec/models/solidus_promotions/benefits/create_discounted_item_spec.rb
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/clean_discounted_order.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/recalculate_promo_totals.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/clean_discounted_order.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb
Outdated
Show resolved
Hide resolved
b3e5d37 to
7872e08
Compare
mamhoff
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.
Added some comments after rebasing
promotions/spec/models/solidus_promotions/benefits/create_discounted_item_spec.rb
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/nullify_order_discounts.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/clean_discounted_order.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/recalculate_promo_totals.rb
Outdated
Show resolved
Hide resolved
promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb
Outdated
Show resolved
Hide resolved
7872e08 to
5377e72
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.
Reads extremely well and is a great change
|
|
||
| module SolidusPromotions | ||
| class OrderAdjuster | ||
| class SetDiscountsToZero |
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 could now be a module, no?
This module recalculates all promo totals after promotion calculation has finished, and marks any adjustments with an amount of zero for destruction.
Now that the promotion order adjuster does not persist things to the database any longer, we can calculate promo totals even in dry runs (and get better results). The `PersistDiscountedOrder` class is slated for deletion, but that is the next commit.
This service object sets all discounts on an order - adjustments on shipments and line items, and shipping rate discounts on shipping rates - to an amount of zero. This service module is intended to run within the order adjuster, before promotion calculation. Promotion calculation will then re-calculate the amounts, with the effect that adjustments that need to change will be changed, while adjustments that do not need to change will not have changes (and will therefore also not be saved).
We want the individual benefits to issue exactly the kind of object that will discount the adjustable object, so we need to generate the right kind of discount object for each adjustable type. This allows us to remove the `PersistDiscountedOrder` class, and it makes the promotion system more understandable and more in line with the rest of Solidus. Specifically, we can now check for adjustments when calculating promotion discounts (this is pretty big!), and `promotion.discounted_amount` can be called from outside the promotion calculation loop.
5377e72 to
ac699f3
Compare
Summary
This PR changes the new promotion system to calculate discounts entirely via adjustments rather than using in-memory
ItemDiscountobjects.The advantage of this approach is that it allows to a) calculate discountable amounts for line items outside of the
DiscountOrderloop. This is useful for calculating discounted prices, a feature I have on the roadmap. It specifically covers needs like the one addressed in this commit. This way, we have one system of record, not two.The other advantage is that we can remove the pretty complex
PersistDiscountedOrdercode, and just rely onorder.save!to do the right thing.It also dovetails nicely with the work on the in-memory order updater.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: