Configurable promotion adjuster #4460
Merged
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
waiting-for-dev
added
type:enhancement
kennyadsl
requested changes
Aug 29, 2022
There was a problem hiding this comment.
Love it! I left a couple of things I think should be changed, though.
Also, can you please provide a real-world example of when someone could use a custom version of this? Not because I don't think there will be, but it will be super useful when writing a how-to for this preference based on a real need.
Thanks Martin! 🙏
mamhoff
force-pushed
the
configurable-promotion-adjuster
branch
from
0c2e655 to
1c2d9bb
Compare
The Order Updater does many things, and one thing it does only semi-well is recalculating promotion adjustments. Promotion adjustments have an action as a source, but when the action asks its promotion whether it's eligible for a particular promotable (the promotion adjustment's adjustable), the promotion will only ask those promotion rules that are of the `applicable` to the promotable's type. This is not necessarily true for line item promotion adjustments, for example. What this commit does is a first step: Extract the current promotion-related behavior from the order updater. It also extracts the relevant specs from the order updater.
mamhoff
force-pushed
the
configurable-promotion-adjuster
branch
2 times, most recently
from
426e078 to
e8b14d4
Compare
adammathys
approved these changes
Sep 6, 2022
kennyadsl
approved these changes
Sep 7, 2022
This provides an extension point for adjusting promotions.
The `Spree::Adjustment#recalculate` method is used only for unit cancels and promotions at this point, and it's preferable that this logic is handled inside the Spree::Promotion::OrderAdjustmentsRecalculator. I cannot deprecate `Spree::Adjustment#recalculate` and friends yet, because the unit cancel code still depends on it.
We want to make the contract between the order updater and the promotion adjuster simpler. What this does is move the promo total calculation for both items and the order into the OrderAdjustmentsRecalculator. This restricts the surface area of the contract between order updater and promotion adjuster to JUST the order's and line item's promotion total, which is everything the taxation code needs.
mamhoff
force-pushed
the
configurable-promotion-adjuster
branch
from
e8b14d4 to
4b5d9f1
Compare
jarednorman
approved these changes
Sep 7, 2022
waiting-for-dev
added a commit
to nebulab/solidus
that referenced
this pull request
Jan 16, 2023
We introduced an extension point for promotion adjustments in solidusio#4460 [1]. We change the expected interface from `#adjust!` to `#call` for consistency, as that's the new standard we want to follow (see solidusio#4677 [2] for an example). That's going to ease the adoption of a service layer if we eventually introduce them. [1] - solidusio#4460 [2] - solidusio#4677
waiting-for-dev
added a commit
to nebulab/solidus
that referenced
this pull request
Jan 16, 2023
We introduced an extension point for promotion adjustments in solidusio#4460 [1]. We change the expected interface from `#adjust!` to `#call` for consistency, as that's the new standard we want to follow (see solidusio#4677 [2] for an example). That's going to ease the adoption of a service layer if we eventually introduce it. [1] - solidusio#4460 [2] - solidusio#4677
3 tasks
waiting-for-dev
added a commit
to nebulab/solidus
that referenced
this pull request
Jan 16, 2023
We introduced an extension point for promotion adjustments in solidusio#4460 [1]. We change the expected interface from `#adjust!` to `#call` for consistency, as that's the new standard we want to follow (see solidusio#4677 [2] for an example). That's going to ease the adoption of a service layer if we eventually introduce it. [1] - solidusio#4460 [2] - solidusio#4677
kennyadsl
reviewed
Jan 27, 2023
| @@ -163,6 +162,7 @@ def update_adjustment_total | |||
| order.included_tax_total = all_items.sum(&:included_tax_total) + order_tax_adjustments.select(&:included?).sum(&:amount) | |||
| order.additional_tax_total = all_items.sum(&:additional_tax_total) + order_tax_adjustments.reject(&:included?).sum(&:amount) | |||
|
|
|||
| # TODO: Delete this line in Solidus 4.0, when it is run in `update_promotions`. | |||
There was a problem hiding this comment.
@mamhoff while taking a look at this PR again, I notice this line. Is this comment still valid? I think we are already doing this in the update_promotion method so we can probably already remove this line?
There was a problem hiding this comment.
I discussed with @mamhoff about this, and it is actually valid. People might be calling update_item_promotions in their own store, which doesn't include this promo_total calculation.
8 tasks
3 tasks
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
This adds an extension point for a configurable promotion adjuster to the app configuration, and uses that extension point to point to a new
Spree::Promotion::LegacyAdjuster. This LegacyAdjuster does all the things that the order updater in conjunction with the Spree::Adjustment class was doing.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.