From edee341914f497ac34aaa2bdb0fc31c11f926ef5 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 09:16:18 +0100 Subject: [PATCH 1/7] Create PromotionEligibilityChecker This new service object checks whether a promotion is eligible. This takes the specs from the `DiscountOrder` object with the `dry_run_promotion` set to `true`, but applies it to just the single promotion we need to run it for. This simple thing is possible because using `PromotionLane.set(current_lane:) {}` we can check promotion availability with existing adjustments. This opens up removing a lot of the verbose error handling from the promotion object, and moving it into the checker class, but refactoring comes in the next commits. --- .../promotion_eligibility_checker.rb | 35 ++++ .../order_adjuster/discount_order_spec.rb | 174 ----------------- .../promotion_eligibility_checker_spec.rb | 178 ++++++++++++++++++ 3 files changed, 213 insertions(+), 174 deletions(-) create mode 100644 promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb create mode 100644 promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb diff --git a/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb b/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb new file mode 100644 index 0000000000..b9ff6e75e0 --- /dev/null +++ b/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module SolidusPromotions + class PromotionEligibilityChecker + attr_reader :promotion + + def initialize(order:, promotion:) + @order = order + @promotion = promotion + end + + def call + SolidusPromotions::PromotionLane.set(current_lane: promotion.lane) do + promotion.benefits.each do |benefit| + benefit.eligible_by_applicable_conditions?(order, dry_run: true) + order.line_items.each do |line_item| + check_item(line_item, benefit) + end + order.shipments.each do |shipment| + check_item(shipment, benefit) + end + end + end + end + + private + + attr_reader :order + + def check_item(item, benefit) + benefit.can_discount?(item) && + benefit.eligible_by_applicable_conditions?(item, dry_run: true) + end + end +end diff --git a/promotions/spec/models/solidus_promotions/order_adjuster/discount_order_spec.rb b/promotions/spec/models/solidus_promotions/order_adjuster/discount_order_spec.rb index 611b34debb..e1820fcd24 100644 --- a/promotions/spec/models/solidus_promotions/order_adjuster/discount_order_spec.rb +++ b/promotions/spec/models/solidus_promotions/order_adjuster/discount_order_spec.rb @@ -32,178 +32,4 @@ expect { subject }.not_to raise_exception end end - - describe "collecting eligibility results in a dry run" do - let(:shirt) { create(:product, name: "Shirt") } - let(:order) { create(:order_with_line_items, line_items_attributes: [{variant: shirt.master, quantity: 1}]) } - let(:conditions) { [product_condition] } - let!(:promotion) { create(:solidus_promotion, :with_adjustable_benefit, conditions: conditions, name: "20% off Shirts", apply_automatically: true) } - let(:product_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt]) } - let(:promotions) { [promotion] } - let(:discounter) { described_class.new(order, promotions, dry_run: true) } - - subject { discounter.call } - - it "will collect eligibility results" do - subject - - expect(promotion.eligibility_results.first.success).to be true - expect(promotion.eligibility_results.first.code).to be nil - expect(promotion.eligibility_results.first.condition).to eq(product_condition) - expect(promotion.eligibility_results.first.message).to be nil - expect(promotion.eligibility_results.first.item).to eq(order) - end - - it "can tell us about success" do - subject - expect(promotion.eligibility_results.success?).to be true - end - - context "with two conditions" do - let(:conditions) { [product_condition, item_total_condition] } - let(:item_total_condition) { SolidusPromotions::Conditions::ItemTotal.new(preferred_amount: 2000) } - - it "will collect eligibility results" do - subject - - expect(promotion.eligibility_results.first.success).to be true - expect(promotion.eligibility_results.first.code).to be nil - expect(promotion.eligibility_results.first.condition).to eq(product_condition) - expect(promotion.eligibility_results.first.message).to be nil - expect(promotion.eligibility_results.first.item).to eq(order) - expect(promotion.eligibility_results.last.success).to be false - expect(promotion.eligibility_results.last.condition).to eq(item_total_condition) - expect(promotion.eligibility_results.last.code).to eq :item_total_less_than_or_equal - expect(promotion.eligibility_results.last.message).to eq "This coupon code can't be applied to orders less than or equal to $2,000.00." - expect(promotion.eligibility_results.last.item).to eq(order) - end - - it "can tell us about success" do - subject - expect(promotion.eligibility_results.success?).to be false - end - - it "has errors for this promo" do - subject - expect(promotion.eligibility_results.error_messages).to eq([ - "This coupon code can't be applied to orders less than or equal to $2,000.00." - ]) - end - end - - context "with an order with multiple line items and an item-level condition" do - let(:pants) { create(:product, name: "Pants") } - let(:order) do - create( - :order_with_line_items, - line_items_attributes: [{variant: shirt.master, quantity: 1}, {variant: pants.master, quantity: 1}] - ) - end - - let(:shirt_product_condition) { SolidusPromotions::Conditions::LineItemProduct.new(products: [shirt]) } - let(:conditions) { [shirt_product_condition] } - - it "can tell us about success" do - subject - # This is successful, because one of the line item conditions matches - expect(promotion.eligibility_results.success?).to be true - end - - it "has no errors for this promo" do - subject - expect(promotion.eligibility_results.error_messages).to be_empty - end - - context "with a second line item level condition" do - let(:hats) { create(:taxon, name: "Hats", products: [hat]) } - let(:hat) { create(:product) } - let(:hat_product_condition) { SolidusPromotions::Conditions::LineItemTaxon.new(taxons: [hats]) } - let(:conditions) { [shirt_product_condition, hat_product_condition] } - - it "can tell us about success" do - subject - expect(promotion.eligibility_results.success?).to be false - end - - it "has errors for this promo" do - subject - expect(promotion.eligibility_results.error_messages).to eq([ - "This coupon code could not be applied to the cart at this time." - ]) - end - end - end - - context "when the order must not contain a shirt" do - let(:no_shirt_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt], preferred_match_policy: "none") } - let(:conditions) { [no_shirt_condition] } - - it "can tell us about success" do - subject - # This is successful, because the order has a shirt - expect(promotion.eligibility_results.success?).to be false - end - end - - context "where one benefit succeeds and another errors" do - let(:usps) { create(:shipping_method) } - let(:ups_ground) { create(:shipping_method) } - let(:order) { create(:order_with_line_items, line_items_attributes: [{variant: shirt.master, quantity: 1}], shipping_method: ups_ground) } - let(:product_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt]) } - let(:shipping_method_condition) { SolidusPromotions::Conditions::ShippingMethod.new(preferred_shipping_method_ids: [usps.id]) } - let(:ten_off_items) { SolidusPromotions::Calculators::Percent.create!(preferred_percent: 10) } - let(:ten_off_shipping) { SolidusPromotions::Calculators::Percent.create!(preferred_percent: 10) } - let(:shipping_benefit) { SolidusPromotions::Benefits::AdjustShipment.new(calculator: ten_off_shipping) } - let(:line_item_benefit) { SolidusPromotions::Benefits::AdjustLineItem.new(calculator: ten_off_items) } - let(:benefits) { [shipping_benefit, line_item_benefit] } - let(:conditions) { [product_condition, shipping_method_condition] } - let!(:promotion) { create(:solidus_promotion, benefits: benefits, name: "10% off Shirts and USPS Shipping", apply_automatically: true) } - - before do - shipping_benefit.conditions << shipping_method_condition - line_item_benefit.conditions << product_condition - end - - it "can tell us about success" do - subject - expect(promotion.eligibility_results.success?).to be true - end - - it "can tell us about errors" do - subject - expect(promotion.eligibility_results.error_messages).to eq(["This coupon code could not be applied to the cart at this time."]) - end - end - - context "with no conditions" do - let(:conditions) { [] } - - it "has no errors for this promo" do - subject - expect(promotion.eligibility_results.error_messages).to be_empty - end - end - - context "with an ineligible order-level condition" do - let(:mug) { create(:product) } - let(:order_condition) { SolidusPromotions::Conditions::NthOrder.new(preferred_nth_order: 2) } - let(:line_item_condition) { SolidusPromotions::Conditions::LineItemProduct.new(products: [mug]) } - let(:conditions) { [order_condition, line_item_condition] } - - it "can tell us about success" do - subject - expect(promotion.eligibility_results.success?).to be false - end - - it "can tell us about all the errors", :pending do - subject - expect(promotion.eligibility_results.error_messages).to eq( - [ - "This coupon code could not be applied to the cart at this time.", - "You need to add an applicable product before applying this coupon code." - ] - ) - end - end - end end diff --git a/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb b/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb new file mode 100644 index 0000000000..2d67acf0aa --- /dev/null +++ b/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb @@ -0,0 +1,178 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SolidusPromotions::PromotionEligibilityChecker do + describe "collecting eligibility results in a dry run" do + let(:shirt) { create(:product, name: "Shirt") } + let(:order) { create(:order_with_line_items, line_items_attributes: [{variant: shirt.master, quantity: 1}]) } + let(:conditions) { [product_condition] } + let!(:promotion) { create(:solidus_promotion, :with_adjustable_benefit, conditions: conditions, name: "20% off Shirts", apply_automatically: true) } + let(:product_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt]) } + let(:checker) { described_class.new(order:, promotion:) } + + subject { checker.call } + + it "will collect eligibility results" do + subject + + expect(promotion.eligibility_results.first.success).to be true + expect(promotion.eligibility_results.first.code).to be nil + expect(promotion.eligibility_results.first.condition).to eq(product_condition) + expect(promotion.eligibility_results.first.message).to be nil + expect(promotion.eligibility_results.first.item).to eq(order) + end + + it "can tell us about success" do + subject + expect(promotion.eligibility_results.success?).to be true + end + + context "with two conditions" do + let(:conditions) { [product_condition, item_total_condition] } + let(:item_total_condition) { SolidusPromotions::Conditions::ItemTotal.new(preferred_amount: 2000) } + + it "will collect eligibility results" do + subject + + expect(promotion.eligibility_results.first.success).to be true + expect(promotion.eligibility_results.first.code).to be nil + expect(promotion.eligibility_results.first.condition).to eq(product_condition) + expect(promotion.eligibility_results.first.message).to be nil + expect(promotion.eligibility_results.first.item).to eq(order) + expect(promotion.eligibility_results.last.success).to be false + expect(promotion.eligibility_results.last.condition).to eq(item_total_condition) + expect(promotion.eligibility_results.last.code).to eq :item_total_less_than_or_equal + expect(promotion.eligibility_results.last.message).to eq "This coupon code can't be applied to orders less than or equal to $2,000.00." + expect(promotion.eligibility_results.last.item).to eq(order) + end + + it "can tell us about success" do + subject + expect(promotion.eligibility_results.success?).to be false + end + + it "has errors for this promo" do + subject + expect(promotion.eligibility_results.error_messages).to eq([ + "This coupon code can't be applied to orders less than or equal to $2,000.00." + ]) + end + end + + context "with an order with multiple line items and an item-level condition" do + let(:pants) { create(:product, name: "Pants") } + let(:order) do + create( + :order_with_line_items, + line_items_attributes: [{variant: shirt.master, quantity: 1}, {variant: pants.master, quantity: 1}] + ) + end + + let(:shirt_product_condition) { SolidusPromotions::Conditions::LineItemProduct.new(products: [shirt]) } + let(:conditions) { [shirt_product_condition] } + + it "can tell us about success" do + subject + # This is successful, because one of the line item conditions matches + expect(promotion.eligibility_results.success?).to be true + end + + it "has no errors for this promo" do + subject + expect(promotion.eligibility_results.error_messages).to be_empty + end + + context "with a second line item level condition" do + let(:hats) { create(:taxon, name: "Hats", products: [hat]) } + let(:hat) { create(:product) } + let(:hat_product_condition) { SolidusPromotions::Conditions::LineItemTaxon.new(taxons: [hats]) } + let(:conditions) { [shirt_product_condition, hat_product_condition] } + + it "can tell us about success" do + subject + expect(promotion.eligibility_results.success?).to be false + end + + it "has errors for this promo" do + subject + expect(promotion.eligibility_results.error_messages).to eq([ + "This coupon code could not be applied to the cart at this time." + ]) + end + end + end + + context "when the order must not contain a shirt" do + let(:no_shirt_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt], preferred_match_policy: "none") } + let(:conditions) { [no_shirt_condition] } + + it "can tell us about success" do + subject + # This is successful, because the order has a shirt + expect(promotion.eligibility_results.success?).to be false + end + end + + context "where one benefit succeeds and another errors" do + let(:usps) { create(:shipping_method) } + let(:ups_ground) { create(:shipping_method) } + let(:order) { create(:order_with_line_items, line_items_attributes: [{variant: shirt.master, quantity: 1}], shipping_method: ups_ground) } + let(:product_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt]) } + let(:shipping_method_condition) { SolidusPromotions::Conditions::ShippingMethod.new(preferred_shipping_method_ids: [usps.id]) } + let(:ten_off_items) { SolidusPromotions::Calculators::Percent.create!(preferred_percent: 10) } + let(:ten_off_shipping) { SolidusPromotions::Calculators::Percent.create!(preferred_percent: 10) } + let(:shipping_benefit) { SolidusPromotions::Benefits::AdjustShipment.new(calculator: ten_off_shipping) } + let(:line_item_benefit) { SolidusPromotions::Benefits::AdjustLineItem.new(calculator: ten_off_items) } + let(:benefits) { [shipping_benefit, line_item_benefit] } + let(:conditions) { [product_condition, shipping_method_condition] } + let!(:promotion) { create(:solidus_promotion, benefits: benefits, name: "10% off Shirts and USPS Shipping", apply_automatically: true) } + + before do + shipping_benefit.conditions << shipping_method_condition + line_item_benefit.conditions << product_condition + end + + it "can tell us about success" do + subject + expect(promotion.eligibility_results.success?).to be true + end + + it "can tell us about errors" do + subject + expect(promotion.eligibility_results.error_messages).to eq(["This coupon code could not be applied to the cart at this time."]) + end + end + + context "with no conditions" do + let(:conditions) { [] } + + it "has no errors for this promo" do + subject + expect(promotion.eligibility_results.error_messages).to be_empty + end + end + + context "with an ineligible order-level condition" do + let(:mug) { create(:product) } + let(:order_condition) { SolidusPromotions::Conditions::NthOrder.new(preferred_nth_order: 2) } + let(:line_item_condition) { SolidusPromotions::Conditions::LineItemProduct.new(products: [mug]) } + let(:conditions) { [order_condition, line_item_condition] } + + it "can tell us about success" do + subject + expect(promotion.eligibility_results.success?).to be false + end + + it "can tell us about all the errors" do + subject + expect(promotion.eligibility_results.error_messages).to eq( + [ + "This coupon code could not be applied to the cart at this time.", + "You need to add an applicable product before applying this coupon code." + ] + ) + end + end + end +end From 43f4b243e4760791a156c0caf7969547a8a0f760 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 09:24:58 +0100 Subject: [PATCH 2/7] Add a configuration option for the eligibility checker People might have other implementations of an order adjuster, and that means a possible different implementation of the eligibility checker. Let's make this configurable so it can be overridden. --- promotions/lib/solidus_promotions/configuration.rb | 5 +++++ .../spec/lib/solidus_promotions/configuration_spec.rb | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/promotions/lib/solidus_promotions/configuration.rb b/promotions/lib/solidus_promotions/configuration.rb index 06a0773065..ae36f82d1c 100644 --- a/promotions/lib/solidus_promotions/configuration.rb +++ b/promotions/lib/solidus_promotions/configuration.rb @@ -8,6 +8,11 @@ class Configuration < Spree::Preferences::Configuration class_name_attribute :order_adjuster_class, default: "SolidusPromotions::OrderAdjuster" + # Service object that checks whether a promotion is eligible for an order. + # @!attribute [rw] eligibility_checker_class + # @return [String] Defaults to "SolidusPromotions::PromotionEligibilityChecker". + class_name_attribute :eligibility_checker_class, default: "SolidusPromotions::PromotionEligibilityChecker" + class_name_attribute :coupon_code_handler_class, default: "SolidusPromotions::PromotionHandler::Coupon" # The class used to normalize coupon codes before saving or lookup. diff --git a/promotions/spec/lib/solidus_promotions/configuration_spec.rb b/promotions/spec/lib/solidus_promotions/configuration_spec.rb index 7e10c306f2..cf7826c3d3 100644 --- a/promotions/spec/lib/solidus_promotions/configuration_spec.rb +++ b/promotions/spec/lib/solidus_promotions/configuration_spec.rb @@ -19,6 +19,12 @@ end end + describe ".eligibility_checker_class" do + it "is the standard eligibility checker" do + expect(subject.eligibility_checker_class).to eq(SolidusPromotions::PromotionEligibilityChecker) + end + end + describe ".advertiser_class" do it "is the standard advertiser" do expect(subject.advertiser_class).to eq(SolidusPromotions::PromotionAdvertiser) From c71ba12573da9d90dd4c5291ecf2d95546a92022 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 09:27:13 +0100 Subject: [PATCH 3/7] Use configurable eligibility checker in promotion handlers --- .../app/models/solidus_promotions/promotion_handler/coupon.rb | 4 ++-- .../app/models/solidus_promotions/promotion_handler/page.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb b/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb index 098b6ae1b9..8afc33eed5 100644 --- a/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb +++ b/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb @@ -77,8 +77,8 @@ def handle_present_promotion return promotion_usage_limit_exceeded if promotion.usage_limit_exceeded? || promotion_code.usage_limit_exceeded? return promotion_applied if promotion_exists_on_order?(order, promotion) - # Try applying this promotion, with no effects - Spree::Config.promotions.order_adjuster_class.new(order, dry_run_promotion: promotion).call + # Check promotion eligibility + Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call if promotion.eligibility_results.success? order.solidus_order_promotions.create!( diff --git a/promotions/app/models/solidus_promotions/promotion_handler/page.rb b/promotions/app/models/solidus_promotions/promotion_handler/page.rb index 1855aa4988..e5491209c5 100644 --- a/promotions/app/models/solidus_promotions/promotion_handler/page.rb +++ b/promotions/app/models/solidus_promotions/promotion_handler/page.rb @@ -12,7 +12,7 @@ def initialize(order, path) def activate if promotion - Spree::Config.promotions.order_adjuster_class.new(order, dry_run_promotion: promotion).call + Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call if promotion.eligibility_results.success? order.solidus_promotions << promotion order.recalculate From 212bb57b4918f1def9cbff9eeaef041d5daec52e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 09:40:52 +0100 Subject: [PATCH 4/7] Deprecate passing dry_run promotions to the OrderAdjuster Now that we have a specialized object for checking promotion eligibility with error messages, we don't need this argument in the order adjuster any longer. --- .../solidus_promotions/load_promotions.rb | 6 ++ .../solidus_promotions/order_adjuster.rb | 6 ++ .../order_adjuster/discount_order.rb | 6 ++ .../promotion_eligibility_checker.rb | 47 +++++++++--- .../promotion_handler/coupon.rb | 12 +-- .../promotion_handler/page.rb | 14 ++-- .../solidus_promotions/order_adjuster_spec.rb | 2 +- .../promotion_eligibility_checker_spec.rb | 76 +++++++++++-------- .../promotion_handler/coupon_spec.rb | 2 +- 9 files changed, 115 insertions(+), 56 deletions(-) diff --git a/promotions/app/models/solidus_promotions/load_promotions.rb b/promotions/app/models/solidus_promotions/load_promotions.rb index 58fe641b29..ca385d9c95 100644 --- a/promotions/app/models/solidus_promotions/load_promotions.rb +++ b/promotions/app/models/solidus_promotions/load_promotions.rb @@ -3,6 +3,12 @@ module SolidusPromotions class LoadPromotions def initialize(order:, dry_run_promotion: nil) + if dry_run_promotion + Spree.deprecator.warn <<~MSG + Passing `dry_run_promotion` to `SolidusPromotions::LoadPromotions` is deprecated. + Use `Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call` instead. + MSG + end @order = order @dry_run_promotion = dry_run_promotion end diff --git a/promotions/app/models/solidus_promotions/order_adjuster.rb b/promotions/app/models/solidus_promotions/order_adjuster.rb index 9a1fae8b72..d9b1f53fa3 100644 --- a/promotions/app/models/solidus_promotions/order_adjuster.rb +++ b/promotions/app/models/solidus_promotions/order_adjuster.rb @@ -5,6 +5,12 @@ class OrderAdjuster attr_reader :order, :promotions, :dry_run def initialize(order, dry_run_promotion: nil) + if dry_run_promotion + Spree.deprecator.warn <<~MSG + Passing `dry_run_promotion` to `SolidusPromotions::OrderAdjuster` is deprecated. + Use `Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call` instead. + MSG + end @order = order @dry_run = !!dry_run_promotion @promotions = SolidusPromotions::LoadPromotions.new( diff --git a/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb b/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb index 441c80df1e..40d88bf3c5 100644 --- a/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb +++ b/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb @@ -6,6 +6,12 @@ class DiscountOrder attr_reader :order, :promotions, :dry_run def initialize(order, promotions, dry_run: false) + if dry_run + Spree.deprecator.warn <<~MSG + Passing `dry_run` to `SolidusPromotions::OrderAdjuster::DiscountOrder` is deprecated. + Use `Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call` instead. + MSG + end @order = order @promotions = promotions @dry_run = dry_run diff --git a/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb b/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb index b9ff6e75e0..750a08ea09 100644 --- a/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb +++ b/promotions/app/models/solidus_promotions/promotion_eligibility_checker.rb @@ -2,34 +2,59 @@ module SolidusPromotions class PromotionEligibilityChecker - attr_reader :promotion + attr_reader :results def initialize(order:, promotion:) @order = order @promotion = promotion + @results = SolidusPromotions::EligibilityResults.new(promotion) end def call SolidusPromotions::PromotionLane.set(current_lane: promotion.lane) do - promotion.benefits.each do |benefit| - benefit.eligible_by_applicable_conditions?(order, dry_run: true) - order.line_items.each do |line_item| - check_item(line_item, benefit) - end - order.shipments.each do |shipment| - check_item(shipment, benefit) - end + promotion.benefits.any? do |benefit| + # We're running this first and storing the result so the following + # block does not short-circuit on ineligible items, and we get all errors. + order_eligible = applicable_conditions_eligible?(order, benefit) + ( + order.line_items.any? do |line_item| + check_item(line_item, benefit) + end || order.shipments.any? do |shipment| + check_item(shipment, benefit) + end + ) && order_eligible end end end private - attr_reader :order + attr_reader :order, :promotion def check_item(item, benefit) benefit.can_discount?(item) && - benefit.eligible_by_applicable_conditions?(item, dry_run: true) + applicable_conditions_eligible?(item, benefit) + end + + def applicable_conditions_eligible?(item, benefit) + benefit.conditions.map do |condition| + next unless condition.applicable?(item) + eligible = !!condition.eligible?(item) + + if condition.eligibility_errors.details[:base].first + code = condition.eligibility_errors.details[:base].first[:error_code] + message = condition.eligibility_errors.full_messages.first + end + results.add( + item: item, + condition: condition, + success: eligible, + code: eligible ? nil : (code || :coupon_code_unknown_error), + message: eligible ? nil : (message || I18n.t(:coupon_code_unknown_error, scope: [:solidus_promotions, :eligibility_errors])) + ) + + eligible + end.compact.all? end end end diff --git a/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb b/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb index 8afc33eed5..7c4f60c874 100644 --- a/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb +++ b/promotions/app/models/solidus_promotions/promotion_handler/coupon.rb @@ -78,9 +78,9 @@ def handle_present_promotion return promotion_applied if promotion_exists_on_order?(order, promotion) # Check promotion eligibility - Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call + checker = Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion) - if promotion.eligibility_results.success? + if checker.call order.solidus_order_promotions.create!( promotion: promotion, promotion_code: promotion_code @@ -88,13 +88,13 @@ def handle_present_promotion order.recalculate set_success_code :coupon_code_applied else - set_promotion_eligibility_error(promotion) + set_promotion_eligibility_error(checker.results) end end - def set_promotion_eligibility_error(promotion) - eligibility_error = promotion.eligibility_results.detect { |result| !result.success } - set_error_code(eligibility_error.code, error: eligibility_error.message, errors: promotion.eligibility_results.error_messages) + def set_promotion_eligibility_error(results) + eligibility_error = results.detect { |result| !result.success } + set_error_code(eligibility_error.code, error: eligibility_error.message, errors: results.error_messages) end def promotion_usage_limit_exceeded diff --git a/promotions/app/models/solidus_promotions/promotion_handler/page.rb b/promotions/app/models/solidus_promotions/promotion_handler/page.rb index e5491209c5..80a1ff4f38 100644 --- a/promotions/app/models/solidus_promotions/promotion_handler/page.rb +++ b/promotions/app/models/solidus_promotions/promotion_handler/page.rb @@ -3,20 +3,20 @@ module SolidusPromotions module PromotionHandler class Page - attr_reader :order, :path + attr_reader :order, :path, :checker def initialize(order, path) @order = order @path = path.delete_prefix("/") + @checker = Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion) end + delegate :results, to: :checker + def activate - if promotion - Spree::Config.promotions.eligibility_checker_class.new(order: order, promotion: promotion).call - if promotion.eligibility_results.success? - order.solidus_promotions << promotion - order.recalculate - end + if promotion && checker.call + order.solidus_promotions << promotion + order.recalculate end end diff --git a/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb b/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb index 7e44093367..8429aec10f 100644 --- a/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb +++ b/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb @@ -39,7 +39,7 @@ end end - context "when on a dry run" do + context "when on a dry run", :silence_deprecations do let(:dry_run_promotion) { promotion } subject do diff --git a/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb b/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb index 2d67acf0aa..3bd1ecb6c9 100644 --- a/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb +++ b/promotions/spec/models/solidus_promotions/promotion_eligibility_checker_spec.rb @@ -13,48 +13,52 @@ subject { checker.call } + it { is_expected.to be true } + it "will collect eligibility results" do subject - expect(promotion.eligibility_results.first.success).to be true - expect(promotion.eligibility_results.first.code).to be nil - expect(promotion.eligibility_results.first.condition).to eq(product_condition) - expect(promotion.eligibility_results.first.message).to be nil - expect(promotion.eligibility_results.first.item).to eq(order) + expect(checker.results.first.success).to be true + expect(checker.results.first.code).to be nil + expect(checker.results.first.condition).to eq(product_condition) + expect(checker.results.first.message).to be nil + expect(checker.results.first.item).to eq(order) end it "can tell us about success" do subject - expect(promotion.eligibility_results.success?).to be true + expect(checker.results.success?).to be true end context "with two conditions" do let(:conditions) { [product_condition, item_total_condition] } let(:item_total_condition) { SolidusPromotions::Conditions::ItemTotal.new(preferred_amount: 2000) } + it { is_expected.to be false } + it "will collect eligibility results" do subject - expect(promotion.eligibility_results.first.success).to be true - expect(promotion.eligibility_results.first.code).to be nil - expect(promotion.eligibility_results.first.condition).to eq(product_condition) - expect(promotion.eligibility_results.first.message).to be nil - expect(promotion.eligibility_results.first.item).to eq(order) - expect(promotion.eligibility_results.last.success).to be false - expect(promotion.eligibility_results.last.condition).to eq(item_total_condition) - expect(promotion.eligibility_results.last.code).to eq :item_total_less_than_or_equal - expect(promotion.eligibility_results.last.message).to eq "This coupon code can't be applied to orders less than or equal to $2,000.00." - expect(promotion.eligibility_results.last.item).to eq(order) + expect(checker.results.first.success).to be true + expect(checker.results.first.code).to be nil + expect(checker.results.first.condition).to eq(product_condition) + expect(checker.results.first.message).to be nil + expect(checker.results.first.item).to eq(order) + expect(checker.results.last.success).to be false + expect(checker.results.last.condition).to eq(item_total_condition) + expect(checker.results.last.code).to eq :item_total_less_than_or_equal + expect(checker.results.last.message).to eq "This coupon code can't be applied to orders less than or equal to $2,000.00." + expect(checker.results.last.item).to eq(order) end it "can tell us about success" do subject - expect(promotion.eligibility_results.success?).to be false + expect(checker.results.success?).to be false end it "has errors for this promo" do subject - expect(promotion.eligibility_results.error_messages).to eq([ + expect(checker.results.error_messages).to eq([ "This coupon code can't be applied to orders less than or equal to $2,000.00." ]) end @@ -72,15 +76,17 @@ let(:shirt_product_condition) { SolidusPromotions::Conditions::LineItemProduct.new(products: [shirt]) } let(:conditions) { [shirt_product_condition] } + # This is successful, because one of the line item conditions matches + it { is_expected.to be true } + it "can tell us about success" do subject - # This is successful, because one of the line item conditions matches - expect(promotion.eligibility_results.success?).to be true + expect(checker.results.success?).to be true end it "has no errors for this promo" do subject - expect(promotion.eligibility_results.error_messages).to be_empty + expect(checker.results.error_messages).to be_empty end context "with a second line item level condition" do @@ -89,14 +95,16 @@ let(:hat_product_condition) { SolidusPromotions::Conditions::LineItemTaxon.new(taxons: [hats]) } let(:conditions) { [shirt_product_condition, hat_product_condition] } + it { is_expected.to be false } + it "can tell us about success" do subject - expect(promotion.eligibility_results.success?).to be false + expect(checker.results.success?).to be false end it "has errors for this promo" do subject - expect(promotion.eligibility_results.error_messages).to eq([ + expect(checker.results.error_messages).to eq([ "This coupon code could not be applied to the cart at this time." ]) end @@ -107,10 +115,12 @@ let(:no_shirt_condition) { SolidusPromotions::Conditions::OrderProduct.new(products: [shirt], preferred_match_policy: "none") } let(:conditions) { [no_shirt_condition] } + # This is unsuccessful, because the order has a shirt + it { is_expected.to be false } + it "can tell us about success" do subject - # This is successful, because the order has a shirt - expect(promotion.eligibility_results.success?).to be false + expect(checker.results.success?).to be false end end @@ -133,23 +143,27 @@ line_item_benefit.conditions << product_condition end + it { is_expected.to be true } + it "can tell us about success" do subject - expect(promotion.eligibility_results.success?).to be true + expect(checker.results.success?).to be true end it "can tell us about errors" do subject - expect(promotion.eligibility_results.error_messages).to eq(["This coupon code could not be applied to the cart at this time."]) + expect(checker.results.error_messages).to eq(["This coupon code could not be applied to the cart at this time."]) end end context "with no conditions" do let(:conditions) { [] } + it { is_expected.to be true } + it "has no errors for this promo" do subject - expect(promotion.eligibility_results.error_messages).to be_empty + expect(checker.results.error_messages).to be_empty end end @@ -159,14 +173,16 @@ let(:line_item_condition) { SolidusPromotions::Conditions::LineItemProduct.new(products: [mug]) } let(:conditions) { [order_condition, line_item_condition] } + it { is_expected.to be false } + it "can tell us about success" do subject - expect(promotion.eligibility_results.success?).to be false + expect(checker.results.success?).to be false end it "can tell us about all the errors" do subject - expect(promotion.eligibility_results.error_messages).to eq( + expect(checker.results.error_messages).to eq( [ "This coupon code could not be applied to the cart at this time.", "You need to add an applicable product before applying this coupon code." diff --git a/promotions/spec/models/solidus_promotions/promotion_handler/coupon_spec.rb b/promotions/spec/models/solidus_promotions/promotion_handler/coupon_spec.rb index 647efb74b3..1927c8876c 100644 --- a/promotions/spec/models/solidus_promotions/promotion_handler/coupon_spec.rb +++ b/promotions/spec/models/solidus_promotions/promotion_handler/coupon_spec.rb @@ -308,7 +308,7 @@ def self.call(value) context "with a whole-order adjustment benefit" do let!(:benefit) { SolidusPromotions::Benefits::AdjustLineItem.create(promotion: promotion, calculator: calculator) } context "right coupon given" do - let(:order) { create(:order) } + let(:order) { create(:order_with_line_items) } let(:calculator) { SolidusPromotions::Calculators::DistributedAmount.new(preferred_amount: 10) } before do From db106bf7f7f1e5dd8f7453daa965693243bfb6c4 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 10:44:45 +0100 Subject: [PATCH 5/7] DRY up DiscountOrder Less LOC, same functionality --- .../order_adjuster/discount_order.rb | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb b/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb index 40d88bf3c5..5cc1006bb9 100644 --- a/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb +++ b/promotions/app/models/solidus_promotions/order_adjuster/discount_order.rb @@ -52,27 +52,20 @@ def perform_order_benefits(lane_benefits, lane) def adjust_line_items(benefits) order.discountable_line_items.filter_map do |line_item| next unless line_item.variant.product.promotionable? - - discounts = generate_discounts(benefits, line_item) - chosen_discounts = SolidusPromotions.config.discount_chooser_class.new(discounts).call - (line_item.current_lane_discounts - chosen_discounts).each(&:mark_for_destruction) + generate_discounts(benefits, line_item) end end def adjust_shipments(benefits) order.shipments.map do |shipment| - discounts = generate_discounts(benefits, shipment) - chosen_discounts = SolidusPromotions.config.discount_chooser_class.new(discounts).call - (shipment.current_lane_discounts - chosen_discounts).each(&:mark_for_destruction) + generate_discounts(benefits, shipment) end end def adjust_shipping_rates(benefits) order.shipments.flat_map(&:shipping_rates).filter_map do |rate| next unless rate.cost - discounts = generate_discounts(benefits, rate) - chosen_discounts = SolidusPromotions.config.discount_chooser_class.new(discounts).call - (rate.current_lane_discounts - chosen_discounts).each(&:mark_for_destruction) + generate_discounts(benefits, rate) end end @@ -84,9 +77,11 @@ def eligible_benefits_for_promotable(possible_benefits, promotable) def generate_discounts(possible_benefits, item) eligible_benefits = eligible_benefits_for_promotable(possible_benefits, item) - eligible_benefits.filter_map do |benefit| + discounts = eligible_benefits.filter_map do |benefit| benefit.can_discount?(item) && benefit.discount(item) end + chosen_discounts = SolidusPromotions.config.discount_chooser_class.new(discounts).call + (item.current_lane_discounts - chosen_discounts).each(&:mark_for_destruction) end end end From 88264f9f514d7875366ffedca5054e2d78e49572 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 13:13:03 +0100 Subject: [PATCH 6/7] Deprecate passing `dry_run: true` in eligible_by_applicable_conditions We can now do the error message collection in the PromotionEligibilityChecker object, where it belongs. --- promotions/app/models/solidus_promotions/benefit.rb | 6 ++++++ promotions/spec/models/solidus_promotions/benefit_spec.rb | 8 ++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/promotions/app/models/solidus_promotions/benefit.rb b/promotions/app/models/solidus_promotions/benefit.rb index cbe7c60509..5128a500ea 100644 --- a/promotions/app/models/solidus_promotions/benefit.rb +++ b/promotions/app/models/solidus_promotions/benefit.rb @@ -270,6 +270,12 @@ def available_calculators # @param dry_run [Boolean] whether to collect detailed eligibility information # @return [Boolean] true when all applicable conditions are eligible def eligible_by_applicable_conditions?(promotable, dry_run: false) + if dry_run + Spree.deprecator.warn <<~MSG + Passing `dry_run` to `#eligible_by_applicable_conditions` is deprecated. If you want to check promotion + eligibility, use the `SolidusPromotions::PromotionEligibilityChecker` service object instead. + MSG + end conditions.map do |condition| next unless condition.applicable?(promotable) eligible = condition.eligible?(promotable) diff --git a/promotions/spec/models/solidus_promotions/benefit_spec.rb b/promotions/spec/models/solidus_promotions/benefit_spec.rb index 96a0b09027..4da07d2a74 100644 --- a/promotions/spec/models/solidus_promotions/benefit_spec.rb +++ b/promotions/spec/models/solidus_promotions/benefit_spec.rb @@ -353,7 +353,7 @@ def line_item_eligible?(_) it { is_expected.to be true } - context "with dry_run true" do + context "with dry_run true", :silence_deprecations do let(:dry_run) { true } it { is_expected.to be true } @@ -373,7 +373,7 @@ def line_item_eligible?(_) let(:promotable) { order.line_items.first } it { is_expected.to be true } - context "with dry_run true" do + context "with dry_run true", :silence_deprecations do let(:dry_run) { true } it { is_expected.to be true } @@ -391,7 +391,7 @@ def line_item_eligible?(_) it { is_expected.to be false } - context "with dry_run true" do + context "with dry_run true", :silence_deprecations do let(:dry_run) { true } it { is_expected.to be false } @@ -432,7 +432,7 @@ def order_eligible?(_) expect(promotion.eligibility_results.error_messages).to be_empty end - context "if dry_run is true" do + context "if dry_run is true", :silence_deprecations do let(:dry_run) { true } it { is_expected.to be false } From ec53d6f541beb5617e283a1d0f4dff9047138c2b Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 26 Mar 2026 13:16:17 +0100 Subject: [PATCH 7/7] Deprecate Promotion#eligibility_results These are now in the PromotionEligibility checker, where they have a much better place. --- promotions/app/models/solidus_promotions/promotion.rb | 1 + promotions/spec/models/solidus_promotions/benefit_spec.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/promotions/app/models/solidus_promotions/promotion.rb b/promotions/app/models/solidus_promotions/promotion.rb index 36a019b4ab..a80bf66333 100644 --- a/promotions/app/models/solidus_promotions/promotion.rb +++ b/promotions/app/models/solidus_promotions/promotion.rb @@ -152,6 +152,7 @@ def products def eligibility_results @eligibility_results ||= SolidusPromotions::EligibilityResults.new(self) end + deprecate eligibility_results: "Please use the `SolidusPromotions::PromotionEligibilityChecker` service object instead", deprecator: Spree.deprecator private diff --git a/promotions/spec/models/solidus_promotions/benefit_spec.rb b/promotions/spec/models/solidus_promotions/benefit_spec.rb index 4da07d2a74..ededd39a4c 100644 --- a/promotions/spec/models/solidus_promotions/benefit_spec.rb +++ b/promotions/spec/models/solidus_promotions/benefit_spec.rb @@ -425,7 +425,7 @@ def order_eligible?(_) it { is_expected.to be false } - it "only asks the first condition and does not collect eligibility errors" do + it "only asks the first condition and does not collect eligibility errors", :silence_deprecations do expect(taxon_condition).to receive(:order_eligible?).and_call_original expect(product_condition).not_to receive(:order_eligible?) subject