-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improve promotion condition API #6360
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
Improve promotion condition API #6360
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6360 +/- ##
==========================================
+ Coverage 89.35% 89.36% +0.01%
==========================================
Files 961 961
Lines 20173 20193 +20
==========================================
+ Hits 18026 18046 +20
Misses 2147 2147 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
I really like this API. Much better and more flexible for future conditions.
Since this is compatible and adds a bunch of useful deprecation warnings, I think this is fine to be shipped with next minor.
401122a to
017b2f8
Compare
Prior to this, we had a bit of cumbersome way of definining a condition. We first had to define whether the condition is applicable by implementing a method, and then we had to define the actual, polymorphic, eligibility check. This commit changes the matter such that a condition can simply implement ```rb def order_eligible? = order.foo? ``` and the promotion system knows that this condition is applicable for order checks. Neat! This - along with the deprecation of `Condition#level` - allows us to also deprecate the `OrderLevelCondition`, `LineItemLevelCondition`, and `ShipmentLevelCondition` modules that do nothing but a type check. Next commit will have the deprecation warnings. For the upcoming price promotions (strikethrough prices) we need to be able to test whether a price is eligible for a promotion given a current order and a quantity. This adds a test for that API.
The `LineItemLevelCondition`, `OrderLevelCondition`, and `ShipmentLevelCondition` concerns are awkwardly named, and there's no adequate thing for conditions that target shipments and shipping rates with this system (let alone something for prices). With the previous commit in place and solidusio#6357, we can fully deprecate these modules for Solidus 5. What this does is: If a condition includes any of the modules and defines the `eligible?` method, we emit a deprecation warning telling the implementer to stop including the module and rename their method. That way the `respond_to?` logic from the base class kicks in, and we can remove the awkward API.
This deprecation warning is chonky, but it tells the implementer in detail what to do and is easy to fix.
017b2f8 to
e21a13f
Compare
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now. Also fixes some AI slop in the docs.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now. Also fixes some AI slop in the docs.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now. Also fixes some AI slop in the docs.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now. Also fixes some AI slop in the docs.
Similar to the work in solidusio#6360, this improves the DSL for creating benefits. Rather than having to define one method that cares for applicability to a discountable, we now simply define that a benefit `#can_discount?` an object if its public interface has `#discount_{discountable_type}` defined. This does not change the implementation of the different methods, but that will come when we refactor the promotion system to use a single system of record for discounts. Then, different objects will need different discount records (line items and shipment use adjustments, but shipping rates use shipping rate discounts etc.). I'm not doing the work for adding deprecation warnings here, as this code was never meant to be overridden up to now. Also fixes some AI slop in the docs.
Summary
This changes the DSL for defining promotion conditions. I know we don't love changing APIs, but hear me out - it becomes simpler and way more straightforward, and the refactor needed is tiny. Also, together with #6357 , we can remove three smelly modules.
The API changes from (this is a real-world example):
To:
The new API is more concise, and allows us easily to adapt conditions to the upcoming strikethrough prices mechanism, in which we need to discount
Spree::Priceobjects (and also check eligibility of these objects). For checking the eligibility of a price, we need to take into account the current order and the desired quantity of the price's variant, so that can be accomplished with this API - without having to redefineapplicable?for a whole lot of conditions.This is what a strikethrough-price compatible condition would look like:
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: