From b547049b20b6eeb69a85402a3dbac7185e9ac928 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 2 Nov 2025 20:55:21 +0100 Subject: [PATCH 1/3] Conditions: Change API 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. --- promotions/MIGRATING.md | 14 +++- ...e_item_applicable_order_level_condition.rb | 6 +- .../models/solidus_promotions/condition.rb | 53 +++++-------- .../conditions/first_order.rb | 2 +- .../conditions/first_repeat_purchase_since.rb | 2 +- .../conditions/item_total.rb | 2 +- .../conditions/line_item_option_value.rb | 2 +- .../conditions/line_item_product.rb | 2 +- .../conditions/line_item_taxon.rb | 2 +- .../conditions/minimum_quantity.rb | 2 +- .../conditions/nth_order.rb | 2 +- .../conditions/one_use_per_user.rb | 2 +- .../conditions/option_value.rb | 4 +- .../solidus_promotions/conditions/product.rb | 2 +- .../conditions/shipping_method.rb | 7 +- .../solidus_promotions/conditions/store.rb | 2 +- .../solidus_promotions/conditions/taxon.rb | 4 +- .../solidus_promotions/conditions/user.rb | 2 +- .../conditions/user_logged_in.rb | 4 +- .../conditions/user_role.rb | 4 +- .../solidus_promotions/condition_spec.rb | 79 ++++++++++++++++++- .../conditions/discounted_item_total_spec.rb | 6 +- .../conditions/item_total_spec.rb | 2 +- .../conditions/one_use_per_user_spec.rb | 4 +- 24 files changed, 132 insertions(+), 79 deletions(-) diff --git a/promotions/MIGRATING.md b/promotions/MIGRATING.md index 84ca018c385..b83ceff1654 100644 --- a/promotions/MIGRATING.md +++ b/promotions/MIGRATING.md @@ -96,10 +96,14 @@ If you have custom promotion rules or actions, you need to create new conditions In our experience, using the three actions can do almost all the things necessary, since they are customizable using calculators. -Rules share a lot of the previous API. If you make use of `#actionable?`, you might want to migrate your rule to be a line-item level rule: +Rules share a lot of the previous API. If you make use of `#actionable?`, you might want to migrate your rule to be a combined order and line-item level rule: ```rb class MyRule < Spree::PromotionRule + def eligible?(order) + order.total > 100 + end + def actionable?(promotable) promotable.quantity > 3 end @@ -110,10 +114,12 @@ would become: ```rb class MyCondition < SolidusPromotions::Condition - include LineItemLevelCondition + def order_eligible?(order, _options = {}) + order.total > 100 + end - def eligible?(promotable) - promotable.quantity > 3 + def line_item_eligible?(line_item, _options = {}) + line_item.quantity > 3 end end ``` diff --git a/promotions/app/models/concerns/solidus_promotions/conditions/line_item_applicable_order_level_condition.rb b/promotions/app/models/concerns/solidus_promotions/conditions/line_item_applicable_order_level_condition.rb index 229a3a73d83..ab5f1954438 100644 --- a/promotions/app/models/concerns/solidus_promotions/conditions/line_item_applicable_order_level_condition.rb +++ b/promotions/app/models/concerns/solidus_promotions/conditions/line_item_applicable_order_level_condition.rb @@ -8,11 +8,7 @@ def self.included(klass) end def applicable?(promotable) - promotable.is_a?(Spree::Order) || preferred_line_item_applicable && promotable.is_a?(Spree::LineItem) - end - - def eligible?(promotable) - send(:"#{promotable.class.name.demodulize.underscore}_eligible?", promotable) + promotable.is_a?(Spree::LineItem) ? preferred_line_item_applicable && super : super end def level diff --git a/promotions/app/models/solidus_promotions/condition.rb b/promotions/app/models/solidus_promotions/condition.rb index b1803de1605..feccdb93de7 100644 --- a/promotions/app/models/solidus_promotions/condition.rb +++ b/promotions/app/models/solidus_promotions/condition.rb @@ -71,58 +71,34 @@ def preload_relations # Determines if this condition can be applied to a given promotable object. # - # This method is typically implemented by including one of the level modules - # (OrderLevelCondition, LineItemLevelCondition, or LineItemApplicableOrderLevelCondition) - # rather than being overridden directly. - # # @param _promotable [Object] The object to check (e.g., Spree::Order, Spree::LineItem) # # @return [Boolean] true if this condition applies to the promotable type # - # @raise [NotImplementedError] if not implemented in subclass - # - # @example Order-level condition applicability + # @example Condition applicability # condition.applicable?(order) # => true # condition.applicable?(line_item) # => false - # - # @see OrderLevelCondition - # @see LineItemLevelCondition - # @see LineItemApplicableOrderLevelCondition - def applicable?(_promotable) - raise NotImplementedError, "applicable? should be implemented in a sub-class of SolidusPromotions::Condition" + def applicable?(promotable) + respond_to?(eligible_method_for(promotable)) end # Determines if the promotable object meets this condition's eligibility requirements. # - # This is the core method that implements the condition's logic. When the promotable - # is not eligible, this method should add errors to {#eligibility_errors} explaining why. + # This typically dispatches to a specific eligibility method defined on a subclass, such as + # `#order_eligible?` or `line_item_eligible?`. # # @param _promotable [Object] The object to evaluate (e.g., Spree::Order, Spree::LineItem) # @param _options [Hash] Additional options for eligibility checking # # @return [Boolean] true if the promotable meets the condition, false otherwise # - # @raise [NotImplementedError] if not implemented in subclass - # - # @example Order total condition - # def eligible?(order, _options = {}) - # if order.item_total < preferred_minimum - # eligibility_errors.add(:base, "Order total too low") - # end - # eligibility_errors.empty? - # end - # - # @example First order condition - # def eligible?(order, _options = {}) - # if order.user.orders.complete.count > 1 - # eligibility_errors.add(:base, "Not first order") - # end - # eligibility_errors.empty? - # end - # # @see #eligibility_errors - def eligible?(_promotable, _options = {}) - raise NotImplementedError, "eligible? should be implemented in a sub-class of SolidusPromotions::Condition" + def eligible?(promotable, ...) + if applicable?(promotable) + send(eligible_method_for(promotable), promotable, ...) + else + raise NotImplementedError, "Please implement #{eligible_method_for(promotable)} in your condition" + end end def level @@ -170,6 +146,13 @@ def updateable? private + # Generates the eligibility method name for a promotable + # + # @return [Symbol] the method name + def eligible_method_for(promotable) + :"#{promotable.class.name.demodulize.underscore}_eligible?" + end + # Validates that only one instance of this condition type exists per benefit. # # Prevents duplicate conditions of the same type from being added to a single benefit. diff --git a/promotions/app/models/solidus_promotions/conditions/first_order.rb b/promotions/app/models/solidus_promotions/conditions/first_order.rb index 1894606c1fd..79864929ac7 100644 --- a/promotions/app/models/solidus_promotions/conditions/first_order.rb +++ b/promotions/app/models/solidus_promotions/conditions/first_order.rb @@ -6,7 +6,7 @@ class FirstOrder < Condition include OrderLevelCondition attr_reader :user, :email - def eligible?(order, options = {}) + def order_eligible?(order, options = {}) @user = order.try(:user) || options[:user] @email = order.email diff --git a/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb b/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb index d561c7671d9..86da2c3bdcf 100644 --- a/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb +++ b/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb @@ -12,7 +12,7 @@ class FirstRepeatPurchaseSince < Condition # # This is eligible if the user's most recently completed order is more than the preferred days ago # @param order [Spree::Order] - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) return false unless order.user last_order = last_completed_order(order.user) diff --git a/promotions/app/models/solidus_promotions/conditions/item_total.rb b/promotions/app/models/solidus_promotions/conditions/item_total.rb index c9d4ea6aee5..7af3d8d284f 100644 --- a/promotions/app/models/solidus_promotions/conditions/item_total.rb +++ b/promotions/app/models/solidus_promotions/conditions/item_total.rb @@ -28,7 +28,7 @@ def self.operator_options end end - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) return false unless order.currency == preferred_currency unless total_for_order(order).send(operator, threshold) diff --git a/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb b/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb index c548b8b5311..2bd44d17713 100644 --- a/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb +++ b/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb @@ -7,7 +7,7 @@ class LineItemOptionValue < Condition preference :eligible_values, :hash - def eligible?(line_item, _options = {}) + def line_item_eligible?(line_item, _options = {}) pid = line_item.product.id ovids = line_item.variant.option_values.pluck(:id) diff --git a/promotions/app/models/solidus_promotions/conditions/line_item_product.rb b/promotions/app/models/solidus_promotions/conditions/line_item_product.rb index 6ad44d3e606..e746abf5a01 100644 --- a/promotions/app/models/solidus_promotions/conditions/line_item_product.rb +++ b/promotions/app/models/solidus_promotions/conditions/line_item_product.rb @@ -23,7 +23,7 @@ def preload_relations [:products] end - def eligible?(line_item, _options = {}) + def line_item_eligible?(line_item, _options = {}) order_includes_product = product_ids.include?(line_item.variant.product_id) success = inverse? ? !order_includes_product : order_includes_product diff --git a/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb b/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb index 3956512060f..9cc5ba5ea63 100644 --- a/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb +++ b/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb @@ -22,7 +22,7 @@ def preload_relations [:taxons] end - def eligible?(line_item, _options = {}) + def line_item_eligible?(line_item, _options = {}) found = Spree::Classification.where( product_id: line_item.variant.product_id, taxon_id: condition_taxon_ids_with_children diff --git a/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb b/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb index 06a3ddcc694..7ebe1d744c3 100644 --- a/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb +++ b/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb @@ -26,7 +26,7 @@ class MinimumQuantity < Condition # # @param order [Spree::Order] the order we want to check eligibility on # @return [Boolean] true if promotion is eligible, false otherwise - def eligible?(order) + def order_eligible?(order, _options = {}) if benefit.applicable_line_items(order).sum(&:quantity) < preferred_minimum_quantity eligibility_errors.add( :base, diff --git a/promotions/app/models/solidus_promotions/conditions/nth_order.rb b/promotions/app/models/solidus_promotions/conditions/nth_order.rb index de4f8927343..45703bfaa27 100644 --- a/promotions/app/models/solidus_promotions/conditions/nth_order.rb +++ b/promotions/app/models/solidus_promotions/conditions/nth_order.rb @@ -14,7 +14,7 @@ class NthOrder < Condition # # Use the first order condition if you want a promotion to be applied to the first order for a user. # @param order [Spree::Order] - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) return false unless order.user nth_order?(order) diff --git a/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb b/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb index 43a78b30162..13238289cae 100644 --- a/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb +++ b/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb @@ -5,7 +5,7 @@ module Conditions class OneUsePerUser < Condition include OrderLevelCondition - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) if order.user.present? if promotion.used_by?(order.user, [order]) eligibility_errors.add( diff --git a/promotions/app/models/solidus_promotions/conditions/option_value.rb b/promotions/app/models/solidus_promotions/conditions/option_value.rb index 085cbfaa556..52b4c1bd19e 100644 --- a/promotions/app/models/solidus_promotions/conditions/option_value.rb +++ b/promotions/app/models/solidus_promotions/conditions/option_value.rb @@ -7,11 +7,11 @@ class OptionValue < Condition preference :eligible_values, :hash - def order_eligible?(order) + def order_eligible?(order, _options = {}) order.line_items.any? { |item| line_item_eligible?(item) } end - def line_item_eligible?(line_item) + def line_item_eligible?(line_item, _options = {}) LineItemOptionValue.new(preferred_eligible_values: preferred_eligible_values).eligible?(line_item) end diff --git a/promotions/app/models/solidus_promotions/conditions/product.rb b/promotions/app/models/solidus_promotions/conditions/product.rb index ac796cd972b..9879c3b916d 100644 --- a/promotions/app/models/solidus_promotions/conditions/product.rb +++ b/promotions/app/models/solidus_promotions/conditions/product.rb @@ -31,7 +31,7 @@ def eligible_products products end - def order_eligible?(order) + def order_eligible?(order, _options = {}) return true if eligible_products.empty? case preferred_match_policy diff --git a/promotions/app/models/solidus_promotions/conditions/shipping_method.rb b/promotions/app/models/solidus_promotions/conditions/shipping_method.rb index 08430d64ae4..b755cdf39d3 100644 --- a/promotions/app/models/solidus_promotions/conditions/shipping_method.rb +++ b/promotions/app/models/solidus_promotions/conditions/shipping_method.rb @@ -7,13 +7,10 @@ class ShippingMethod < Condition preference :shipping_method_ids, type: :array, default: [] - def applicable?(promotable) - promotable.is_a?(Spree::Shipment) || promotable.is_a?(Spree::ShippingRate) - end - - def eligible?(promotable) + def shipment_eligible?(promotable, _options = {}) promotable.shipping_method&.id&.in?(preferred_shipping_method_ids.map(&:to_i)) end + alias_method :shipping_rate_eligible?, :shipment_eligible? end end end diff --git a/promotions/app/models/solidus_promotions/conditions/store.rb b/promotions/app/models/solidus_promotions/conditions/store.rb index 8dfbc9309ed..b3ff389c4c7 100644 --- a/promotions/app/models/solidus_promotions/conditions/store.rb +++ b/promotions/app/models/solidus_promotions/conditions/store.rb @@ -16,7 +16,7 @@ def preload_relations [:stores] end - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) stores.none? || stores.include?(order.store) end diff --git a/promotions/app/models/solidus_promotions/conditions/taxon.rb b/promotions/app/models/solidus_promotions/conditions/taxon.rb index 4f3fb4690a6..de1e4654590 100644 --- a/promotions/app/models/solidus_promotions/conditions/taxon.rb +++ b/promotions/app/models/solidus_promotions/conditions/taxon.rb @@ -22,7 +22,7 @@ def preload_relations preference :match_policy, :string, default: MATCH_POLICIES.first - def order_eligible?(order) + def order_eligible?(order, _options = {}) order_taxons = taxons_in_order(order) case preferred_match_policy @@ -57,7 +57,7 @@ def order_eligible?(order) eligibility_errors.empty? end - def line_item_eligible?(line_item) + def line_item_eligible?(line_item, _options = {}) # The order level eligibility check happens first, and if none of the taxons # are in the order, then no line items should be available to check. raise "This should not happen" if preferred_match_policy == "none" diff --git a/promotions/app/models/solidus_promotions/conditions/user.rb b/promotions/app/models/solidus_promotions/conditions/user.rb index c9b26bbd61f..ab2022630e9 100644 --- a/promotions/app/models/solidus_promotions/conditions/user.rb +++ b/promotions/app/models/solidus_promotions/conditions/user.rb @@ -16,7 +16,7 @@ def preload_relations [:users] end - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) users.include?(order.user) end diff --git a/promotions/app/models/solidus_promotions/conditions/user_logged_in.rb b/promotions/app/models/solidus_promotions/conditions/user_logged_in.rb index 45db92487cc..591487259db 100644 --- a/promotions/app/models/solidus_promotions/conditions/user_logged_in.rb +++ b/promotions/app/models/solidus_promotions/conditions/user_logged_in.rb @@ -3,9 +3,7 @@ module SolidusPromotions module Conditions class UserLoggedIn < Condition - include OrderLevelCondition - - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) if order.user.blank? eligibility_errors.add(:base, eligibility_error_message(:no_user_specified), error_code: :no_user_specified) end diff --git a/promotions/app/models/solidus_promotions/conditions/user_role.rb b/promotions/app/models/solidus_promotions/conditions/user_role.rb index d4c816d0429..174e75ef0e2 100644 --- a/promotions/app/models/solidus_promotions/conditions/user_role.rb +++ b/promotions/app/models/solidus_promotions/conditions/user_role.rb @@ -3,14 +3,12 @@ module SolidusPromotions module Conditions class UserRole < Condition - include OrderLevelCondition - preference :role_ids, :array, default: [] MATCH_POLICIES = %w[any all].freeze preference :match_policy, default: MATCH_POLICIES.first - def eligible?(order, _options = {}) + def order_eligible?(order, _options = {}) return false unless order.user if all_match_policy? diff --git a/promotions/spec/models/solidus_promotions/condition_spec.rb b/promotions/spec/models/solidus_promotions/condition_spec.rb index d600c8bb244..5e5e003de43 100644 --- a/promotions/spec/models/solidus_promotions/condition_spec.rb +++ b/promotions/spec/models/solidus_promotions/condition_spec.rb @@ -11,7 +11,7 @@ def self.model_name ActiveModel::Name.new(self, nil, "test_condition") end - def eligible?(_promotable, _options = {}) + def order_eligible?(_order, _options = {}) true end @@ -40,9 +40,80 @@ def level it { is_expected.to be_empty } end - it "forces developer to implement eligible? method" do - expect { bad_test_condition_class.new.eligible?("promotable") }.to raise_error NotImplementedError - expect { test_condition_class.new.eligible?("promotable") }.not_to raise_error + describe "#eligible?" do + let(:order_condition) do + Class.new(described_class) do + def order_eligible?(_order) = true + end + end + let(:line_item_condition) do + Class.new(described_class) do + def line_item_eligible?(_order) = true + end + end + + subject { condition.new.eligible?(promotable) } + + context "promotable is order" do + let(:promotable) { Spree::Order.new } + + context "if condition implements order_eligible?" do + let(:condition) { order_condition } + + it { is_expected.to be true } + end + + context "if condition does not implement order_eligible?" do + context "if condition implements order_eligible?" do + let(:condition) { line_item_condition } + + it "raises NotImplementedError" do + expect { subject }.to raise_error(NotImplementedError) + end + end + end + end + + describe "passing on options to a condition" do + let(:price_condition) do + Class.new(described_class) do + def price_eligible?(_price, options = {}) + options[:order].present? && options[:quantity] > 1 + end + end + end + + let(:order) { Spree::Order.new } + let(:price) { Spree::Price.new } + + subject { price_condition.new.eligible?(price, order:, quantity:) } + + context "with quantity 1" do + let(:quantity) { 1 } + + it { is_expected.to be false } + end + + context "with quantity 2" do + let(:quantity) { 2 } + + it { is_expected.to be true } + end + + context "if condition does not care about order" do + let(:price_condition) do + Class.new(described_class) do + def price_eligible?(_price, options = {}) + options[:quantity] > 1 + end + end + end + + let(:quantity) { 2 } + + it { is_expected.to be true } + end + end end it "forces developer to implement #applicable?" do diff --git a/promotions/spec/models/solidus_promotions/conditions/discounted_item_total_spec.rb b/promotions/spec/models/solidus_promotions/conditions/discounted_item_total_spec.rb index 7746a52b9ce..2fc3fa81953 100644 --- a/promotions/spec/models/solidus_promotions/conditions/discounted_item_total_spec.rb +++ b/promotions/spec/models/solidus_promotions/conditions/discounted_item_total_spec.rb @@ -9,10 +9,14 @@ preferred_operator: preferred_operator ) end - let(:order) { instance_double("Spree::Order", discountable_item_total: item_total, currency: order_currency) } + let(:order) { Spree::Order.new(currency: order_currency) } let(:preferred_amount) { 50 } let(:order_currency) { "USD" } let(:preferred_operator) { "gt" } + let(:item_total) { 0 } + before do + allow(order).to receive(:discountable_item_total).and_return(item_total) + end context "preferred operator set to gt" do context "item total is greater than preferred amount" do diff --git a/promotions/spec/models/solidus_promotions/conditions/item_total_spec.rb b/promotions/spec/models/solidus_promotions/conditions/item_total_spec.rb index ab4c3a6217e..16fc1465f8e 100644 --- a/promotions/spec/models/solidus_promotions/conditions/item_total_spec.rb +++ b/promotions/spec/models/solidus_promotions/conditions/item_total_spec.rb @@ -9,7 +9,7 @@ preferred_operator: preferred_operator ) end - let(:order) { instance_double("Spree::Order", item_total: item_total, currency: order_currency) } + let(:order) { Spree::Order.new(item_total: item_total, currency: order_currency) } let(:preferred_amount) { 50 } let(:order_currency) { "USD" } diff --git a/promotions/spec/models/solidus_promotions/conditions/one_use_per_user_spec.rb b/promotions/spec/models/solidus_promotions/conditions/one_use_per_user_spec.rb index 20fe909ff4a..798b5d56882 100644 --- a/promotions/spec/models/solidus_promotions/conditions/one_use_per_user_spec.rb +++ b/promotions/spec/models/solidus_promotions/conditions/one_use_per_user_spec.rb @@ -8,8 +8,8 @@ describe "#eligible?(order)" do subject { condition.eligible?(order) } - let(:order) { instance_double("Spree::Order", user: user) } - let(:user) { instance_double("Spree::LegacyUser") } + let(:order) { Spree::Order.new(user:) } + let(:user) { Spree::LegacyUser.new } let(:promotion) { stub_model SolidusPromotions::Promotion, used_by?: used_by } let(:used_by) { false } From fa6948034babaa57db86a123819cf72d3dcbfbab Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 2 Nov 2025 21:51:53 +0100 Subject: [PATCH 2/3] Deprecate `Level` concerns 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 #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. --- .../conditions/line_item_level_condition.rb | 16 +++++++- .../conditions/order_level_condition.rb | 16 +++++++- .../conditions/shipment_level_condition.rb | 16 +++++++- .../conditions/first_order.rb | 2 + .../conditions/first_repeat_purchase_since.rb | 1 + .../conditions/item_total.rb | 1 + .../conditions/line_item_option_value.rb | 1 + .../conditions/line_item_product.rb | 1 + .../conditions/line_item_taxon.rb | 1 + .../conditions/minimum_quantity.rb | 1 + .../conditions/nth_order.rb | 1 + .../conditions/one_use_per_user.rb | 1 + .../conditions/shipping_method.rb | 1 + .../solidus_promotions/conditions/store.rb | 1 + .../solidus_promotions/conditions/user.rb | 1 + .../line_item_level_condition_spec.rb | 37 +++++++++++++++++++ .../conditions/order_level_condition_spec.rb | 37 +++++++++++++++++++ .../shipment_level_condition_spec.rb | 37 +++++++++++++++++++ 18 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 promotions/spec/models/concerns/conditions/line_item_level_condition_spec.rb create mode 100644 promotions/spec/models/concerns/conditions/order_level_condition_spec.rb create mode 100644 promotions/spec/models/concerns/conditions/shipment_level_condition_spec.rb diff --git a/promotions/app/models/concerns/solidus_promotions/conditions/line_item_level_condition.rb b/promotions/app/models/concerns/solidus_promotions/conditions/line_item_level_condition.rb index de5f4063910..d23bda506f9 100644 --- a/promotions/app/models/concerns/solidus_promotions/conditions/line_item_level_condition.rb +++ b/promotions/app/models/concerns/solidus_promotions/conditions/line_item_level_condition.rb @@ -3,8 +3,20 @@ module SolidusPromotions module Conditions module LineItemLevelCondition - def applicable?(promotable) - promotable.is_a?(Spree::LineItem) + def self.included(base) + def base.method_added(method) + if method == :eligible? + Spree.deprecator.warn <<~MSG + Defining `eligible?` on a promotion along with including the `LineItemLevelCondition` module is deprecated. + Rename `eligible?` to `line_item_eligible?` and stop including the `LineItemLevelCondition` module. + MSG + define_method(:applicable?) do |promotable| + promotable.is_a?(Spree::LineItem) + end + end + + super + end end def level diff --git a/promotions/app/models/concerns/solidus_promotions/conditions/order_level_condition.rb b/promotions/app/models/concerns/solidus_promotions/conditions/order_level_condition.rb index 7f84cd874a7..7766dc3b582 100644 --- a/promotions/app/models/concerns/solidus_promotions/conditions/order_level_condition.rb +++ b/promotions/app/models/concerns/solidus_promotions/conditions/order_level_condition.rb @@ -3,8 +3,20 @@ module SolidusPromotions module Conditions module OrderLevelCondition - def applicable?(promotable) - promotable.is_a?(Spree::Order) + def self.included(base) + def base.method_added(method) + if method == :eligible? + Spree.deprecator.warn <<~MSG + Defining `eligible?` on a promotion along with including the `OrderLevelCondition` module is deprecated. + Rename `eligible?` to `order_eligible?` and stop including the `OrderLevelCondition` module. + MSG + define_method(:applicable?) do |promotable| + promotable.is_a?(Spree::Order) + end + end + + super + end end def level diff --git a/promotions/app/models/concerns/solidus_promotions/conditions/shipment_level_condition.rb b/promotions/app/models/concerns/solidus_promotions/conditions/shipment_level_condition.rb index 87a7dc049fb..a4c58c4b8e8 100644 --- a/promotions/app/models/concerns/solidus_promotions/conditions/shipment_level_condition.rb +++ b/promotions/app/models/concerns/solidus_promotions/conditions/shipment_level_condition.rb @@ -3,8 +3,20 @@ module SolidusPromotions module Conditions module ShipmentLevelCondition - def applicable?(promotable) - promotable.is_a?(Spree::Shipment) + def self.included(base) + def base.method_added(method) + if method == :eligible? + Spree.deprecator.warn <<~MSG + Defining `eligible?` on a promotion along with including the `ShipmentLevelCondition` module is deprecated. + Rename `eligible?` to `shipment_eligible?` and stop including the `ShipmentLevelCondition` module. + MSG + define_method(:applicable?) do |promotable| + promotable.is_a?(Spree::Shipment) + end + end + + super + end end def level diff --git a/promotions/app/models/solidus_promotions/conditions/first_order.rb b/promotions/app/models/solidus_promotions/conditions/first_order.rb index 79864929ac7..5a676217d13 100644 --- a/promotions/app/models/solidus_promotions/conditions/first_order.rb +++ b/promotions/app/models/solidus_promotions/conditions/first_order.rb @@ -3,7 +3,9 @@ module SolidusPromotions module Conditions class FirstOrder < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition + attr_reader :user, :email def order_eligible?(order, options = {}) diff --git a/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb b/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb index 86da2c3bdcf..888c4197153 100644 --- a/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb +++ b/promotions/app/models/solidus_promotions/conditions/first_repeat_purchase_since.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class FirstRepeatPurchaseSince < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition preference :days_ago, :integer, default: 365 diff --git a/promotions/app/models/solidus_promotions/conditions/item_total.rb b/promotions/app/models/solidus_promotions/conditions/item_total.rb index 7af3d8d284f..a9e7c51ceec 100644 --- a/promotions/app/models/solidus_promotions/conditions/item_total.rb +++ b/promotions/app/models/solidus_promotions/conditions/item_total.rb @@ -8,6 +8,7 @@ module Conditions # To add extra operators please override `self.operators_map` or any other helper method. # To customize the error message you can also override `ineligible_message`. class ItemTotal < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition preference :amount, :decimal, default: 100.00 diff --git a/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb b/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb index 2bd44d17713..a03e39e6c4b 100644 --- a/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb +++ b/promotions/app/models/solidus_promotions/conditions/line_item_option_value.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class LineItemOptionValue < Condition + # TODO: Remove in Solidus 5 include LineItemLevelCondition preference :eligible_values, :hash diff --git a/promotions/app/models/solidus_promotions/conditions/line_item_product.rb b/promotions/app/models/solidus_promotions/conditions/line_item_product.rb index e746abf5a01..01de2ff8e3f 100644 --- a/promotions/app/models/solidus_promotions/conditions/line_item_product.rb +++ b/promotions/app/models/solidus_promotions/conditions/line_item_product.rb @@ -4,6 +4,7 @@ module SolidusPromotions module Conditions # A condition to apply a promotion only to line items with or without a chosen product class LineItemProduct < Condition + # TODO: Remove in Solidus 5 include LineItemLevelCondition MATCH_POLICIES = %w[include exclude].freeze diff --git a/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb b/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb index 9cc5ba5ea63..108dd2b9f37 100644 --- a/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb +++ b/promotions/app/models/solidus_promotions/conditions/line_item_taxon.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class LineItemTaxon < Condition + # TODO: Remove in Solidus 5 include LineItemLevelCondition has_many :condition_taxons, diff --git a/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb b/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb index 7ebe1d744c3..208d8fd3e25 100644 --- a/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb +++ b/promotions/app/models/solidus_promotions/conditions/minimum_quantity.rb @@ -10,6 +10,7 @@ module Conditions # it to a simple quantity check across the entire order which would be # better served by an item total condition. class MinimumQuantity < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition validates :preferred_minimum_quantity, numericality: { only_integer: true, greater_than: 0 } diff --git a/promotions/app/models/solidus_promotions/conditions/nth_order.rb b/promotions/app/models/solidus_promotions/conditions/nth_order.rb index 45703bfaa27..dc6b595af69 100644 --- a/promotions/app/models/solidus_promotions/conditions/nth_order.rb +++ b/promotions/app/models/solidus_promotions/conditions/nth_order.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class NthOrder < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition preference :nth_order, :integer, default: 2 diff --git a/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb b/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb index 13238289cae..a1cbd661fc6 100644 --- a/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb +++ b/promotions/app/models/solidus_promotions/conditions/one_use_per_user.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class OneUsePerUser < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition def order_eligible?(order, _options = {}) diff --git a/promotions/app/models/solidus_promotions/conditions/shipping_method.rb b/promotions/app/models/solidus_promotions/conditions/shipping_method.rb index b755cdf39d3..a924a4a33c1 100644 --- a/promotions/app/models/solidus_promotions/conditions/shipping_method.rb +++ b/promotions/app/models/solidus_promotions/conditions/shipping_method.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class ShippingMethod < Condition + # TODO: Remove in Solidus 5 include ShipmentLevelCondition preference :shipping_method_ids, type: :array, default: [] diff --git a/promotions/app/models/solidus_promotions/conditions/store.rb b/promotions/app/models/solidus_promotions/conditions/store.rb index b3ff389c4c7..bd6fd3fd795 100644 --- a/promotions/app/models/solidus_promotions/conditions/store.rb +++ b/promotions/app/models/solidus_promotions/conditions/store.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class Store < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition has_many :condition_stores, diff --git a/promotions/app/models/solidus_promotions/conditions/user.rb b/promotions/app/models/solidus_promotions/conditions/user.rb index ab2022630e9..6b4cc870899 100644 --- a/promotions/app/models/solidus_promotions/conditions/user.rb +++ b/promotions/app/models/solidus_promotions/conditions/user.rb @@ -3,6 +3,7 @@ module SolidusPromotions module Conditions class User < Condition + # TODO: Remove in Solidus 5 include OrderLevelCondition has_many :condition_users, diff --git a/promotions/spec/models/concerns/conditions/line_item_level_condition_spec.rb b/promotions/spec/models/concerns/conditions/line_item_level_condition_spec.rb new file mode 100644 index 00000000000..645afcee3cf --- /dev/null +++ b/promotions/spec/models/concerns/conditions/line_item_level_condition_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SolidusPromotions::Conditions::LineItemLevelCondition do + let(:legacy_condition) do + Class.new(SolidusPromotions::Condition) do + include SolidusPromotions::Conditions::LineItemLevelCondition + + def eligible?(_line_item) + true + end + end + end + + it "emits a warning telling the user to rename eligible? to line_item_eligible?" do + expect(Spree.deprecator).to receive(:warn).with <<~MSG + Defining `eligible?` on a promotion along with including the `LineItemLevelCondition` module is deprecated. + Rename `eligible?` to `line_item_eligible?` and stop including the `LineItemLevelCondition` module. + MSG + legacy_condition + end + + it "responds to eligible?", :silence_deprecations do + expect(legacy_condition.new.eligible?(Spree::LineItem.new)).to be true + end + + describe "#applicable?", :silence_deprecations do + it "is applicable for line items" do + expect(legacy_condition.new.applicable?(Spree::LineItem.new)).to be true + end + + it "is not applicable for orders" do + expect(legacy_condition.new.applicable?(Spree::Order.new)).to be false + end + end +end diff --git a/promotions/spec/models/concerns/conditions/order_level_condition_spec.rb b/promotions/spec/models/concerns/conditions/order_level_condition_spec.rb new file mode 100644 index 00000000000..a76450c0f88 --- /dev/null +++ b/promotions/spec/models/concerns/conditions/order_level_condition_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SolidusPromotions::Conditions::OrderLevelCondition do + let(:legacy_condition) do + Class.new(SolidusPromotions::Condition) do + include SolidusPromotions::Conditions::OrderLevelCondition + + def eligible?(_line_item) + true + end + end + end + + it "emits a warning telling the user to rename eligible? to line_item_eligible?" do + expect(Spree.deprecator).to receive(:warn).with <<~MSG + Defining `eligible?` on a promotion along with including the `OrderLevelCondition` module is deprecated. + Rename `eligible?` to `order_eligible?` and stop including the `OrderLevelCondition` module. + MSG + legacy_condition + end + + it "responds to eligible?", :silence_deprecations do + expect(legacy_condition.new.eligible?(Spree::Order.new)).to be true + end + + describe "#applicable?", :silence_deprecations do + it "is applicable for orders" do + expect(legacy_condition.new.applicable?(Spree::Order.new)).to be true + end + + it "is not applicable for line items" do + expect(legacy_condition.new.applicable?(Spree::LineItem.new)).to be false + end + end +end diff --git a/promotions/spec/models/concerns/conditions/shipment_level_condition_spec.rb b/promotions/spec/models/concerns/conditions/shipment_level_condition_spec.rb new file mode 100644 index 00000000000..2cb336bd005 --- /dev/null +++ b/promotions/spec/models/concerns/conditions/shipment_level_condition_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SolidusPromotions::Conditions::ShipmentLevelCondition do + let(:legacy_condition) do + Class.new(SolidusPromotions::Condition) do + include SolidusPromotions::Conditions::ShipmentLevelCondition + + def eligible?(_line_item) + true + end + end + end + + it "emits a warning telling the user to rename eligible? to line_item_eligible?" do + expect(Spree.deprecator).to receive(:warn).with <<~MSG + Defining `eligible?` on a promotion along with including the `ShipmentLevelCondition` module is deprecated. + Rename `eligible?` to `shipment_eligible?` and stop including the `ShipmentLevelCondition` module. + MSG + legacy_condition + end + + it "responds to eligible?", :silence_deprecations do + expect(legacy_condition.new.eligible?(Spree::Shipment.new)).to be true + end + + describe "#applicable?", :silence_deprecations do + it "is applicable for shipments" do + expect(legacy_condition.new.applicable?(Spree::Shipment.new)).to be true + end + + it "is not applicable for line items" do + expect(legacy_condition.new.applicable?(Spree::LineItem.new)).to be false + end + end +end From e21a13f96c10897c881a63df5cda900c6ab352b3 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 2 Nov 2025 23:09:26 +0100 Subject: [PATCH 3/3] Add deprecation warning for defining `eligible?` in conditions This deprecation warning is chonky, but it tells the implementer in detail what to do and is easy to fix. --- .../models/solidus_promotions/condition.rb | 31 ++++++++++ .../solidus_promotions/condition_spec.rb | 62 ++++++++++++++++--- 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/promotions/app/models/solidus_promotions/condition.rb b/promotions/app/models/solidus_promotions/condition.rb index feccdb93de7..62bd77830a3 100644 --- a/promotions/app/models/solidus_promotions/condition.rb +++ b/promotions/app/models/solidus_promotions/condition.rb @@ -101,6 +101,37 @@ def eligible?(promotable, ...) end end + def self.inherited(klass) + def klass.method_added(method_added) + if method_added == :eligible? + Spree.deprecator.warn <<~MSG + Please refactor `#{name}`. You're defining `eligible?`. Instead, define method for each type of promotable + that your condition can be applied to. For example: + ``` + class MyCondition < SolidusPromotions::Condition + def applicable?(promotable) + promotable.is_a?(Spree::Order) + end + + def eligible?(order) + order.total > 20 + end + ``` + can now become + ``` + class MyCondition < SolidusPromotions::Condition + def order_eligible?(order) + order.total > 20 + end + end + ``` + MSG + end + super + end + super + end + def level raise NotImplementedError, "level should be implemented in a sub-class of SolidusPromotions::Condition" end diff --git a/promotions/spec/models/solidus_promotions/condition_spec.rb b/promotions/spec/models/solidus_promotions/condition_spec.rb index 5e5e003de43..b5a835b3520 100644 --- a/promotions/spec/models/solidus_promotions/condition_spec.rb +++ b/promotions/spec/models/solidus_promotions/condition_spec.rb @@ -116,16 +116,62 @@ def price_eligible?(_price, options = {}) end end - it "forces developer to implement #applicable?" do - expect { bad_test_condition_class.new.applicable?("promotable") }.to raise_error NotImplementedError - expect { test_condition_class.new.applicable?("promotable") }.not_to raise_error - end + describe "inherited hook" do + context "for a well-formed condition" do + subject(:condition) do + Class.new(described_class) do + def line_item_eligible?(_line_item, _options = {}) + true + end + end + end - it "forces developer to implement #level", :silence_deprecations do - expect { bad_test_condition_class.new.level }.to raise_error NotImplementedError - expect { test_condition_class.new.level }.not_to raise_error - end + it "does not emit a deprecation warning" do + expect(Spree.deprecator).not_to receive(:warn) + condition + end + end + + context "for a legacy condition" do + subject(:condition) do + Class.new(described_class) do + def self.name + "LegacyCondition" + end + def eligible?(_line_item, _options = {}) + true + end + end + end + + it "emits a deprecation warning" do + expect(Spree.deprecator).to receive(:warn).with(<<~MSG) + Please refactor `LegacyCondition`. You're defining `eligible?`. Instead, define method for each type of promotable + that your condition can be applied to. For example: + ``` + class MyCondition < SolidusPromotions::Condition + def applicable?(promotable) + promotable.is_a?(Spree::Order) + end + + def eligible?(order) + order.total > 20 + end + ``` + can now become + ``` + class MyCondition < SolidusPromotions::Condition + def order_eligible?(order) + order.total > 20 + end + end + ``` + MSG + condition + end + end + end it "validates unique conditions for a promotion benefit" do # Because of Rails' STI, we can't use the anonymous class here promotion = create(:solidus_promotion, :with_adjustable_benefit)