-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduce a null promotion configuration #5667
Conversation
a7f58db
to
dcc8f86
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5667 +/- ##
==========================================
+ Coverage 88.62% 88.64% +0.02%
==========================================
Files 688 691 +3
Lines 16493 16523 +30
==========================================
+ Hits 14617 14647 +30
Misses 1876 1876 ☔ View full report in Codecov by Sentry. |
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.
Very nice work!
# frozen_string_literal: true | ||
|
||
module Spree | ||
class NullAdjuster |
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.
Maybe a more specific name would help to understand the purpose of the object.
What about?
Spree::NullPromotionAdjuster
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.
I second this question, unless you see a reason to keep it more generic. In that case, can you please help us understand where it is?
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.
My observation was that the class doesn't actually do anything promotion-specific, and the interface (#call
) also doesn't necessarily do anything. I was suspecting (but wrongly) that also tax adjuster might share that interface.
Whatever: It's completely fine to call it NullPromotionAdjuster
, and I've amended the PR to reflect that.
@mamhoff the PR title is a bit confusing, though. Mind to adjust it? |
Done, thank you! |
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.
Well done, thanks Martin, left a comment.
# frozen_string_literal: true | ||
|
||
module Spree | ||
class NullAdjuster |
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.
I second this question, unless you see a reason to keep it more generic. In that case, can you please help us understand where it is?
Spree::Core::PromotionConfiguration will have to live in the solidus_legacy_promotions gem. We will need a stand-in! But for that stand-in, we also need an extension point.
This allows us to have a stand-in for a promotion system when, in fact, we don't have one.
This is a stand-in for all the promotion handlers we have. It implements all the methods from the promotion handler "coupon".
We need a promotion configuration when solidus_legacy_promotions is out of core. This provides it.
dcc8f86
to
e29983f
Compare
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 work. 👏
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.
👍 Thanks Martin, you rock!
Summary
This adds a "null" promotion configuration, consisting of only those extension points for the promotion system that are used from within core (rather than from within the promotion system).
This is a nice exercise as it also tells us what extension points we need for any promo system: A class to help recalculate orders, and one to add coupon codes to orders.
This includes the commits from #5635 . I'll rebase once that is merged.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: