From dd6d2459be2eed4cb367e4f767a427b06f5556f6 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sun, 27 Mar 2022 09:16:14 +0200 Subject: [PATCH 01/38] Add LineItemProduct Rule This rule is like "Product", but only applies to line items. --- .../rules/_line_item_product.html.erb | 18 +++++++ .../promotion/rules/line_item_product.rb | 48 +++++++++++++++++++ core/config/locales/en.yml | 9 ++++ core/lib/spree/app_configuration.rb | 1 + .../promotion/rules/line_item_product_spec.rb | 47 ++++++++++++++++++ 5 files changed, 123 insertions(+) create mode 100644 backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb create mode 100644 core/app/models/spree/promotion/rules/line_item_product.rb create mode 100644 core/spec/models/spree/promotion/rules/line_item_product_spec.rb diff --git a/backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb b/backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb new file mode 100644 index 00000000000..972486873b6 --- /dev/null +++ b/backend/app/views/spree/admin/promotions/rules/_line_item_product.html.erb @@ -0,0 +1,18 @@ +
+
+
+ <%= label_tag "#{param_prefix}_product_ids_string", t('spree.product_rule.choose_products') %> + <%= hidden_field_tag "#{param_prefix}[product_ids_string]", promotion_rule.product_ids.join(","), class: "product_picker fullwidth" %> +
+
+
+
+ <%= label_tag("#{param_prefix}_preferred_match_policy", promotion_rule.class.human_attribute_name(:preferred_match_policy)) %> + <%= select_tag( + "#{param_prefix}[preferred_match_policy]", + options_for_select(I18n.t("spree.promotion_rules.line_item_product.match_policies").to_a.map(&:reverse), promotion_rule.preferred_match_policy), + class: "custom-select fullwidth" + ) %> +
+
+
diff --git a/core/app/models/spree/promotion/rules/line_item_product.rb b/core/app/models/spree/promotion/rules/line_item_product.rb new file mode 100644 index 00000000000..daafcfb5096 --- /dev/null +++ b/core/app/models/spree/promotion/rules/line_item_product.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Spree + class Promotion < Spree::Base + module Rules + # A rule to apply a promotion only to line items with or without a chosen product + class LineItemProduct < Spree::PromotionRule + MATCH_POLICIES = %w(normal inverse) + + has_many :product_promotion_rules, + dependent: :destroy, + foreign_key: :promotion_rule_id, + class_name: "Spree::ProductPromotionRule" + has_many :products, + class_name: "Spree::Product", + through: :product_promotion_rules + + preference :match_policy, :string, default: MATCH_POLICIES.first + + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + + def eligible?(line_item, _options = {}) + if inverse? + !product_ids.include?(line_item.variant.product_id) + else + product_ids.include?(line_item.variant.product_id) + end + end + + def product_ids_string + product_ids.join(",") + end + + def product_ids_string=(product_ids) + self.product_ids = product_ids.to_s.split(",").map(&:strip) + end + + private + + def inverse? + preferred_match_policy == "inverse" + end + end + end + end +end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 3e1cec69b05..d8cec6db988 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -228,6 +228,9 @@ en: description: Order includes specified product(s) with matching option value(s) spree/promotion/rules/product: description: Order includes specified product(s) + spree/promotion/rules/line_item_product: + description: Line item matches specified product(s) + preferred_match_policy: Match Policy spree/promotion/rules/store: description: Available only to the specified stores spree/promotion/rules/taxon: @@ -656,6 +659,7 @@ en: spree/promotion/rules/one_use_per_user: One Use Per User spree/promotion/rules/option_value: Option Value(s) spree/promotion/rules/product: Product(s) + spree/promotion/rules/line_item_product: Line Item Product(s) spree/promotion/rules/taxon: Taxon(s) spree/promotion/rules/user: User spree/promotion/rules/user_logged_in: User Logged In @@ -1867,6 +1871,11 @@ en: all: Match all of these rules any: Match any of these rules promotion_rule: Promotion Rule + promotion_rules: + line_item_product: + match_policies: + normal: Line item's product is one of the chosen products + inverse: Line item's product is NOT one of the chosen products promotion_successfully_created: Promotion has been successfully created! promotion_total_changed_before_complete: One or more of the promotions on your order have become ineligible and were removed. Please check the new order amounts diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index d167d0c83f2..7e200f57241 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -623,6 +623,7 @@ def environment Spree::Promotion::Rules::User Spree::Promotion::Rules::FirstOrder Spree::Promotion::Rules::UserLoggedIn + Spree::Promotion::Rules::LineItemProduct Spree::Promotion::Rules::OneUsePerUser Spree::Promotion::Rules::Taxon Spree::Promotion::Rules::NthOrder diff --git a/core/spec/models/spree/promotion/rules/line_item_product_spec.rb b/core/spec/models/spree/promotion/rules/line_item_product_spec.rb new file mode 100644 index 00000000000..de68cbe93b4 --- /dev/null +++ b/core/spec/models/spree/promotion/rules/line_item_product_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::Promotion::Rules::LineItemProduct, type: :model do + let(:rule) { described_class.new(rule_options) } + let(:rule_options) { {} } + + context "#eligible?(line_item)" do + let(:rule_line_item) { Spree::LineItem.new(product: rule_product) } + let(:other_line_item) { Spree::LineItem.new(product: other_product) } + + let(:rule_options) { super().merge(products: [rule_product]) } + let(:rule_product) { mock_model(Spree::Product) } + let(:other_product) { mock_model(Spree::Product) } + + it "should be eligible if there are no products" do + expect(rule).to be_eligible(rule_line_item) + end + + subject { rule.eligible?(line_item, {}) } + + context "for product in rule" do + let(:line_item) { rule_line_item } + it { is_expected.to be_truthy } + end + + context "for product not in rule" do + let(:line_item) { other_line_item } + it { is_expected.to be_falsey } + end + + context "if match policy is inverse" do + let(:rule_options) { super().merge(preferred_match_policy: "inverse") } + + context "for product in rule" do + let(:line_item) { rule_line_item } + it { is_expected.to be_falsey } + end + + context "for product not in rule" do + let(:line_item) { other_line_item } + it { is_expected.to be_truthy } + end + end + end +end From 44920c9690ec9aa9ec7c834bcb57c9bb9c4e5f9d Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 30 Mar 2022 14:53:03 +0200 Subject: [PATCH 02/38] Rename Product rule This rule applies to both an order and a line item, and the naming should reflect that. --- core/config/locales/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index d8cec6db988..b121a730911 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -658,7 +658,7 @@ en: spree/promotion/rules/nth_order: Nth Order spree/promotion/rules/one_use_per_user: One Use Per User spree/promotion/rules/option_value: Option Value(s) - spree/promotion/rules/product: Product(s) + spree/promotion/rules/product: Order Product(s) spree/promotion/rules/line_item_product: Line Item Product(s) spree/promotion/rules/taxon: Taxon(s) spree/promotion/rules/user: User From e0ad236056b58c86e5c36896e25e73f50ea62344 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 30 Mar 2022 17:47:27 +0200 Subject: [PATCH 03/38] Use "Order Product(s)" rule in promotion admin feature spec --- backend/spec/features/admin/promotions/product_rule_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/spec/features/admin/promotions/product_rule_spec.rb b/backend/spec/features/admin/promotions/product_rule_spec.rb index 2b6d909c352..11b1ec081fa 100644 --- a/backend/spec/features/admin/promotions/product_rule_spec.rb +++ b/backend/spec/features/admin/promotions/product_rule_spec.rb @@ -18,7 +18,7 @@ def add_promotion_rule_of_type(type) background do visit spree.edit_admin_promotion_path(promotion) - add_promotion_rule_of_type("Product(s)") + add_promotion_rule_of_type("Order Product(s)") end it "can select by product sku" do From 5a828ee957d7365271a94b5e305a150ec7f28009 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 30 Mar 2022 17:58:27 +0200 Subject: [PATCH 04/38] Fix promotion adjustments spec to not use Product rule --- backend/spec/features/admin/promotion_adjustments_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/spec/features/admin/promotion_adjustments_spec.rb b/backend/spec/features/admin/promotion_adjustments_spec.rb index 383a4a70f29..342143bcd77 100644 --- a/backend/spec/features/admin/promotion_adjustments_spec.rb +++ b/backend/spec/features/admin/promotion_adjustments_spec.rb @@ -118,7 +118,7 @@ click_button "Create" expect(page).to have_title("SAVE SAVE SAVE - Promotions") - select "Product(s)", from: "Discount Rules" + select "Line Item Product(s)", from: "Discount Rules" within("#rule_fields") { click_button "Add" } select2_search "RoR Mug", from: "Choose products" within('#rule_fields') { click_button "Update" } @@ -136,7 +136,7 @@ expect(promotion.codes.first).to be_nil first_rule = promotion.rules.first - expect(first_rule.class).to eq(Spree::Promotion::Rules::Product) + expect(first_rule.class).to eq(Spree::Promotion::Rules::LineItemProduct) expect(first_rule.products.map(&:name)).to include("RoR Mug") first_action = promotion.actions.first From 0aeaf81b461154a692b14c42eb533e3490545c14 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 31 Mar 2022 10:15:29 +0200 Subject: [PATCH 05/38] Add promotion integration spec This spec allows me to show some of the weird things in the promotion system. --- .../spree/promotion/integration_spec.rb | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 core/spec/models/spree/promotion/integration_spec.rb diff --git a/core/spec/models/spree/promotion/integration_spec.rb b/core/spec/models/spree/promotion/integration_spec.rb new file mode 100644 index 00000000000..1e4175c6fd6 --- /dev/null +++ b/core/spec/models/spree/promotion/integration_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "Promotion System" do + context "A promotion that creates line item adjustments" do + let(:shirt) { create(:product) } + let(:pants) { create(:product) } + let(:promotion) { create(:promotion, name: "20% off Shirts", apply_automatically: true) } + let(:order) { create(:order) } + + before do + promotion.rules << rule + promotion.actions << action + order.contents.add(shirt.master, 1) + order.contents.add(pants.master, 1) + end + + context "with an order-level rule" do + let(:rule) { Spree::Promotion::Rules::Product.new(products: [shirt]) } + + context "with an order level action" do + let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateAdjustment.new(calculator: calculator) } + + it "creates one order-level adjustment" do + expect(order.adjustments.length).to eq(1) + expect(order.total).to eq(31.98) + expect(order.item_total).to eq(39.98) + # This is wrong! But order level adjustments can't work any other way + expect(order.item_total_before_tax).to eq(39.98) + expect(order.line_items.flat_map(&:adjustments)).to be_empty + end + end + + context "with an line item level action" do + let(:calculator) { Spree::Calculator::PercentOnLineItem.new(preferred_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateItemAdjustments.new(calculator: calculator) } + + it "creates one order-level adjustment" do + expect(order.adjustments).to be_empty + expect(order.total).to eq(31.98) + expect(order.item_total).to eq(39.98) + expect(order.item_total_before_tax).to eq(31.98) + expect(order.line_items.flat_map(&:adjustments).length).to eq(2) + end + end + end + + context "with a line-item level rule" do + let(:rule) { Spree::Promotion::Rules::LineItemProduct.new(products: [shirt]) } + + context "with an order level action" do + let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateAdjustment.new(calculator: calculator) } + + it "creates one order-level adjustment" do + # Whoops - this works because line item level rules don't affect order-level actions :( + expect(order.adjustments.length).to eq(1) + expect(order.total).to eq(31.98) + expect(order.item_total).to eq(39.98) + # This is wrong! But order level adjustments can't work any other way + expect(order.item_total_before_tax).to eq(39.98) + expect(order.line_items.flat_map(&:adjustments)).to be_empty + end + end + + context "with an line item level action" do + let(:calculator) { Spree::Calculator::PercentOnLineItem.new(preferred_percent: 20) } + let(:action) { Spree::Promotion::Actions::CreateItemAdjustments.new(calculator: calculator) } + + it "creates one line item level adjustment" do + expect(order.adjustments).to be_empty + expect(order.total).to eq(35.98) + expect(order.item_total).to eq(39.98) + expect(order.item_total_before_tax).to eq(35.98) + pending "There's a bug here" + expect(order.line_items.flat_map(&:adjustments).length).to eq(1) + end + end + end + end +end From 3f3d202d79ac917bd82247410bcad57323f3fabf Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 31 Mar 2022 11:22:16 +0200 Subject: [PATCH 06/38] Prevent ineligible line item adjustments When we have a promotion with a line item rule that evaluates to ineligible for a line item, it will still call "actionable?" on that rule, which will evaluate to `true`, and create an adjustment on line items that will always be ineligible. --- core/app/models/spree/promotion.rb | 6 ++- .../spree/promotion/integration_spec.rb | 1 - core/spec/models/spree/promotion_spec.rb | 50 +++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index e48589dadbe..d41dae981e5 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -211,7 +211,7 @@ def usage_count(excluded_orders: []) end def line_item_actionable?(order, line_item, promotion_code: nil) - return false if blacklisted?(line_item) + return false unless line_item_eligible?(line_item) if eligible?(order, promotion_code: promotion_code) rules = eligible_rules(order) @@ -227,6 +227,10 @@ def line_item_actionable?(order, line_item, promotion_code: nil) end end + def line_item_eligible?(line_item) + !blacklisted?(line_item) && !!eligible_rules(line_item) + end + def used_by?(user, excluded_orders = []) discounted_orders. complete. diff --git a/core/spec/models/spree/promotion/integration_spec.rb b/core/spec/models/spree/promotion/integration_spec.rb index 1e4175c6fd6..275b0270d94 100644 --- a/core/spec/models/spree/promotion/integration_spec.rb +++ b/core/spec/models/spree/promotion/integration_spec.rb @@ -74,7 +74,6 @@ expect(order.total).to eq(35.98) expect(order.item_total).to eq(39.98) expect(order.item_total_before_tax).to eq(35.98) - pending "There's a bug here" expect(order.line_items.flat_map(&:adjustments).length).to eq(1) end end diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 2b2294d572b..ddde53c75ae 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -880,6 +880,14 @@ let(:line_item) { build(:line_item) { |li| li.variant.product.promotionable = false } } it { is_expected.not_to be } end + + context "if the promotion has ineligible line item rules" do + before do + expect(promotion).to receive(:line_item_eligible?) { false } + end + + it { is_expected.to be false } + end end end @@ -903,6 +911,48 @@ end end + describe "#line_item_eligible?" do + let(:line_item) { build(:line_item) } + let(:promotion) { build(:promotion) } + let(:rules) { [] } + + subject { promotion.line_item_eligible?(line_item) } + + before do + promotion.promotion_rules = rules + end + + it { is_expected.to be true } + + context "if the line item's variant is not safelisted" do + let(:product) { build(:product, promotionable: false) } + let(:line_item) { build(:line_item, variant: product.master) } + + it { is_expected.to be false } + end + + context "if the promotion has inapplicable rules" do + let(:rule) { stub_model Spree::PromotionRule, eligible?: false, applicable?: false } + let(:rules) { [rule] } + + it { is_expected.to be true } + end + + context "if the promotion has applicable rules" do + let(:rule) { stub_model Spree::PromotionRule, eligible?: false, applicable?: true } + let(:rules) { [rule] } + + it { is_expected.to be false } + end + + context "if the promotion has applicable and eligible rules" do + let(:rule) { stub_model Spree::PromotionRule, eligible?: true, applicable?: true } + let(:rules) { [rule] } + + it { is_expected.to be true } + end + end + # regression for https://github.com/spree/spree/issues/4059 # admin form posts the code and path as empty string describe "normalize blank values for path" do From 47f4297eab4d37b2e30fad74bea6c6a9587797fe Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 31 Mar 2022 11:28:24 +0200 Subject: [PATCH 07/38] Simplify Promotion Rule Option Value There was only one option for the match policy, so we can get rid of it. --- core/app/models/spree/promotion/rules/option_value.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/core/app/models/spree/promotion/rules/option_value.rb b/core/app/models/spree/promotion/rules/option_value.rb index 04de936957a..6ee3856eebc 100644 --- a/core/app/models/spree/promotion/rules/option_value.rb +++ b/core/app/models/spree/promotion/rules/option_value.rb @@ -4,8 +4,6 @@ module Spree class Promotion < Spree::Base module Rules class OptionValue < PromotionRule - MATCH_POLICIES = %w(any) - preference :match_policy, :string, default: MATCH_POLICIES.first preference :eligible_values, :hash def applicable?(promotable) @@ -13,10 +11,7 @@ def applicable?(promotable) end def eligible?(promotable, _options = {}) - case preferred_match_policy - when 'any' - promotable.line_items.any? { |item| actionable?(item) } - end + promotable.line_items.any? { |item| actionable?(item) } end def actionable?(line_item) From 9192a7dbb99b501fcdd98c7b7df5a771a55d342a Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 31 Mar 2022 13:32:42 +0200 Subject: [PATCH 08/38] Add Promotion Rule LineItemOptionValue This promotion rule shall replace the use of `actionable` on the `OptionValue` promotion rule. --- .../rules/_line_item_option_value.html.erb | 12 ++++++ .../promotion/rules/line_item_option_value.rb | 41 +++++++++++++++++++ core/config/locales/en.yml | 3 ++ core/lib/spree/app_configuration.rb | 1 + 4 files changed, 57 insertions(+) create mode 100644 backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb create mode 100644 core/app/models/spree/promotion/rules/line_item_option_value.rb diff --git a/backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb b/backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb new file mode 100644 index 00000000000..99ca90d34af --- /dev/null +++ b/backend/app/views/spree/admin/promotions/rules/_line_item_option_value.html.erb @@ -0,0 +1,12 @@ +
+ +
+
<%= label_tag nil, Spree::Product.model_name.human %>
+
<%= label_tag nil, plural_resource_name(Spree::OptionValue) %>
+
+ + <%= content_tag :div, nil, class: "js-promo-rule-option-values", + data: { :'original-option-values' => promotion_rule.preferred_eligible_values } %> + + +
diff --git a/core/app/models/spree/promotion/rules/line_item_option_value.rb b/core/app/models/spree/promotion/rules/line_item_option_value.rb new file mode 100644 index 00000000000..b639c804f5c --- /dev/null +++ b/core/app/models/spree/promotion/rules/line_item_option_value.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Spree + class Promotion < Spree::Base + module Rules + class LineItemOptionValue < PromotionRule + preference :eligible_values, :hash + + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + + def eligible?(line_item, _options = {}) + pid = line_item.product.id + ovids = line_item.variant.option_values.pluck(:id) + + product_ids.include?(pid) && (value_ids(pid) & ovids).present? + end + + def preferred_eligible_values + values = preferences[:eligible_values] || {} + Hash[values.keys.map(&:to_i).zip( + values.values.map do |value| + (value.is_a?(Array) ? value : value.split(",")).map(&:to_i) + end + )] + end + + private + + def product_ids + preferred_eligible_values.keys + end + + def value_ids(product_id) + preferred_eligible_values[product_id] + end + end + end + end +end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index b121a730911..3b687db3767 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -226,6 +226,8 @@ en: description: Only one use per user spree/promotion/rules/option_value: description: Order includes specified product(s) with matching option value(s) + spree/promotion/rules/line_item_option_value: + description: Line Item has specified product with matching option value spree/promotion/rules/product: description: Order includes specified product(s) spree/promotion/rules/line_item_product: @@ -658,6 +660,7 @@ en: spree/promotion/rules/nth_order: Nth Order spree/promotion/rules/one_use_per_user: One Use Per User spree/promotion/rules/option_value: Option Value(s) + spree/promotion/rules/line_item_option_value: Line Item Option Value(s) spree/promotion/rules/product: Order Product(s) spree/promotion/rules/line_item_product: Line Item Product(s) spree/promotion/rules/taxon: Taxon(s) diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 7e200f57241..8649205d936 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -628,6 +628,7 @@ def environment Spree::Promotion::Rules::Taxon Spree::Promotion::Rules::NthOrder Spree::Promotion::Rules::OptionValue + Spree::Promotion::Rules::LineItemOptionValue Spree::Promotion::Rules::FirstRepeatPurchaseSince Spree::Promotion::Rules::UserRole Spree::Promotion::Rules::Store From 908bbbc687af3b4939e8a8bd877bded4a4ca9392 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 10:43:48 +0200 Subject: [PATCH 09/38] Add Promotion Rule "LineItemTaxon" This Rule can replace the use of `actionable?` on the taxon promotion rule. --- .../rules/_line_item_taxon.html.erb | 14 +++ .../spree/promotion/rules/line_item_taxon.rb | 54 +++++++++++ core/config/locales/en.yml | 10 +- core/lib/spree/app_configuration.rb | 1 + .../promotion/rules/line_item_taxon_spec.rb | 92 +++++++++++++++++++ 5 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb create mode 100644 core/app/models/spree/promotion/rules/line_item_taxon.rb create mode 100644 core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb diff --git a/backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb b/backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb new file mode 100644 index 00000000000..c06d03c79d8 --- /dev/null +++ b/backend/app/views/spree/admin/promotions/rules/_line_item_taxon.html.erb @@ -0,0 +1,14 @@ +
+ <%= label_tag "#{param_prefix}_taxon_ids_string", t("spree.taxon_rule.choose_taxons") %> + <%= hidden_field_tag "#{param_prefix}[taxon_ids_string]", promotion_rule.taxon_ids.join(","), class: "taxon_picker fullwidth", id: "product_taxon_ids" %> +
+
+
+ <%= label_tag("#{param_prefix}_preferred_match_policy", promotion_rule.class.human_attribute_name(:preferred_match_policy)) %> + <%= select_tag( + "#{param_prefix}[preferred_match_policy]", + options_for_select(I18n.t("spree.promotion_rules.line_item_taxon.match_policies").to_a.map(&:reverse), promotion_rule.preferred_match_policy), + class: "custom-select fullwidth", + ) %> +
+
diff --git a/core/app/models/spree/promotion/rules/line_item_taxon.rb b/core/app/models/spree/promotion/rules/line_item_taxon.rb new file mode 100644 index 00000000000..5f5f9075034 --- /dev/null +++ b/core/app/models/spree/promotion/rules/line_item_taxon.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Spree + class Promotion < Spree::Base + module Rules + class LineItemTaxon < PromotionRule + has_many :promotion_rule_taxons, class_name: 'Spree::PromotionRuleTaxon', foreign_key: :promotion_rule_id, + dependent: :destroy + has_many :taxons, through: :promotion_rule_taxons, class_name: 'Spree::Taxon' + + MATCH_POLICIES = %w(normal inverse) + + validates_inclusion_of :preferred_match_policy, in: MATCH_POLICIES + + preference :match_policy, :string, default: MATCH_POLICIES.first + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + + def eligible?(line_item, _options = {}) + found = Spree::Classification.where( + product_id: line_item.variant.product_id, + taxon_id: rule_taxon_ids_with_children + ).exists? + + case preferred_match_policy + when 'normal' + found + when 'inverse' + !found + else + raise "unexpected match policy: #{preferred_match_policy.inspect}" + end + end + + def taxon_ids_string + taxons.pluck(:id).join(',') + end + + def taxon_ids_string=(taxon_ids) + taxon_ids = taxon_ids.to_s.split(',').map(&:strip) + self.taxons = Spree::Taxon.find(taxon_ids) + end + + private + + # ids of taxons rules and taxons rules children + def rule_taxon_ids_with_children + taxons.flat_map { |taxon| taxon.self_and_descendants.ids }.uniq + end + end + end + end +end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 3b687db3767..086bd574944 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -237,6 +237,9 @@ en: description: Available only to the specified stores spree/promotion/rules/taxon: description: Order includes products with specified taxon(s) + spree/promotion/rules/line_item_taxon: + description: Line Item has product with specified taxon(s) + preferred_match_policy: Match Policy spree/promotion/rules/user: description: Available only to the specified users spree/promotion/rules/user_logged_in: @@ -663,7 +666,8 @@ en: spree/promotion/rules/line_item_option_value: Line Item Option Value(s) spree/promotion/rules/product: Order Product(s) spree/promotion/rules/line_item_product: Line Item Product(s) - spree/promotion/rules/taxon: Taxon(s) + spree/promotion/rules/taxon: Order Taxon(s) + spree/promotion/rules/line_item_taxon: Line Item Taxon(s) spree/promotion/rules/user: User spree/promotion/rules/user_logged_in: User Logged In spree/promotion/rules/user_role: User Role(s) @@ -1879,6 +1883,10 @@ en: match_policies: normal: Line item's product is one of the chosen products inverse: Line item's product is NOT one of the chosen products + line_item_taxon: + match_policies: + normal: Line item's product has one of the chosen taxons + inverse: Line item's product does not have one of the chosen taxons promotion_successfully_created: Promotion has been successfully created! promotion_total_changed_before_complete: One or more of the promotions on your order have become ineligible and were removed. Please check the new order amounts diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 8649205d936..5802fff22f7 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -626,6 +626,7 @@ def environment Spree::Promotion::Rules::LineItemProduct Spree::Promotion::Rules::OneUsePerUser Spree::Promotion::Rules::Taxon + Spree::Promotion::Rules::LineItemTaxon Spree::Promotion::Rules::NthOrder Spree::Promotion::Rules::OptionValue Spree::Promotion::Rules::LineItemOptionValue diff --git a/core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb b/core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb new file mode 100644 index 00000000000..25cc37625b8 --- /dev/null +++ b/core/spec/models/spree/promotion/rules/line_item_taxon_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::Promotion::Rules::LineItemTaxon, type: :model do + let(:taxon) { create :taxon, name: 'first' } + let(:taxon2) { create :taxon, name: 'second' } + let(:order) { create :order_with_line_items } + let(:product) { order.products.first } + + let(:rule) do + described_class.create!(promotion: create(:promotion)) + end + + describe '#eligible?' do + let(:line_item) { order.line_items.first! } + let(:order) { create :order_with_line_items } + let(:taxon) { create :taxon, name: 'first' } + + context 'with an invalid match policy' do + before do + rule.preferred_match_policy = 'invalid' + rule.save!(validate: false) + line_item.product.taxons << taxon + rule.taxons << taxon + end + + it 'raises' do + expect { + rule.eligible?(line_item) + }.to raise_error('unexpected match policy: "invalid"') + end + end + + context 'when a product has a taxon of a taxon rule' do + before do + product.taxons << taxon + rule.taxons << taxon + rule.save! + end + + it 'is eligible' do + expect(rule).to be_eligible(line_item) + end + end + + context 'when a product has a taxon child of a taxon rule' do + before do + taxon.children << taxon2 + product.taxons << taxon2 + rule.taxons << taxon + rule.save! + end + + it 'is eligible' do + expect(rule).to be_eligible(line_item) + end + + context "with inverse match policy" do + before do + rule.update(preferred_match_policy: :inverse) + end + + it "is not eligible" do + expect(rule).not_to be_eligible(line_item) + end + end + end + + context 'when a product does not have taxon or child taxon of a taxon rule' do + before do + product.taxons << taxon2 + rule.taxons << taxon + rule.save! + end + + it 'is not eligible' do + expect(rule).not_to be_eligible(line_item) + end + + context "with inverse match policy" do + before do + rule.update(preferred_match_policy: :inverse) + end + + it "is not eligible" do + expect(rule).to be_eligible(line_item) + end + end + end + end +end From 80dbd5c6911ed2776bd249861f7d74f659119c44 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 11:06:15 +0200 Subject: [PATCH 10/38] Add migration to add necessary line item rules to existing promotions Now that the Order, Taxon, and OptionValue rules only operate on orders, we need to add the respective line item rules to the promotions that need them. --- ..._promotion_rules_to_existing_promotions.rb | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb diff --git a/core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb b/core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb new file mode 100644 index 00000000000..89d483775af --- /dev/null +++ b/core/db/migrate/20220401085754_add_line_item_promotion_rules_to_existing_promotions.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class AddLineItemPromotionRulesToExistingPromotions < ActiveRecord::Migration[5.2] + def up + Spree::Promotion::Rules::Product.all.each do |promotion_rule| + match_policy = promotion_rule.preferred_match_policy == "none" ? "inverse" : "normal" + promotion_rule.promotion.rules << Spree::Promotion::Rules::LineItemProduct.new( + preferred_match_policy: match_policy, + products: promotion_rule.products + ) + end + Spree::Promotion::Rules::Taxon.all.each do |promotion_rule| + match_policy = promotion_rule.preferred_match_policy == "none" ? "inverse" : "normal" + promotion_rule.promotion.rules << Spree::Promotion::Rules::LineItemTaxon.new( + preferred_match_policy: match_policy, + taxons: promotion_rule.taxons + ) + end + Spree::Promotion::Rules::OptionValue.all.each do |promotion_rule| + promotion_rule.promotion.rules << Spree::Promotion::Rules::LineItemOptionValue.new( + preferred_eligible_values: promotion_rule.preferred_eligible_values + ) + end + end + + def down + Spree::Promotion::Rules::LineItemOptionValue.destroy_all + Spree::Promotion::Rules::LineItemTaxon.destroy_all + Spree::Promotion::Rules::LineItemProduct.destroy_all + end +end From 77b01d21021a9ae05aef4c5bba4ef5d4afd1b0f6 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 11:09:17 +0200 Subject: [PATCH 11/38] Remove "actionable?" from Product rule --- .../models/spree/promotion/rules/product.rb | 11 ---- .../spree/promotion/rules/product_spec.rb | 66 ------------------- 2 files changed, 77 deletions(-) diff --git a/core/app/models/spree/promotion/rules/product.rb b/core/app/models/spree/promotion/rules/product.rb index c8888bc71ad..8b0294b8eec 100644 --- a/core/app/models/spree/promotion/rules/product.rb +++ b/core/app/models/spree/promotion/rules/product.rb @@ -56,17 +56,6 @@ def eligible?(order, _options = {}) eligibility_errors.empty? end - def actionable?(line_item) - case preferred_match_policy - when 'any', 'all' - product_ids.include? line_item.variant.product_id - when 'none' - product_ids.exclude? line_item.variant.product_id - else - raise "unexpected match policy: #{preferred_match_policy.inspect}" - end - end - def product_ids_string product_ids.join(',') end diff --git a/core/spec/models/spree/promotion/rules/product_spec.rb b/core/spec/models/spree/promotion/rules/product_spec.rb index afe73223aa0..7e400baa287 100644 --- a/core/spec/models/spree/promotion/rules/product_spec.rb +++ b/core/spec/models/spree/promotion/rules/product_spec.rb @@ -124,70 +124,4 @@ end end end - - describe '#actionable?' do - subject do - rule.actionable?(line_item) - end - - let(:rule_line_item) { Spree::LineItem.new(product: rule_product) } - let(:other_line_item) { Spree::LineItem.new(product: other_product) } - - let(:rule_options) { super().merge(products: [rule_product]) } - let(:rule_product) { mock_model(Spree::Product) } - let(:other_product) { mock_model(Spree::Product) } - - context "with 'any' match policy" do - let(:rule_options) { super().merge(preferred_match_policy: 'any') } - - context 'for product in rule' do - let(:line_item) { rule_line_item } - it { is_expected.to be_truthy } - end - - context 'for product not in rule' do - let(:line_item) { other_line_item } - it { is_expected.to be_falsey } - end - end - - context "with 'all' match policy" do - let(:rule_options) { super().merge(preferred_match_policy: 'all') } - - context 'for product in rule' do - let(:line_item) { rule_line_item } - it { is_expected.to be_truthy } - end - - context 'for product not in rule' do - let(:line_item) { other_line_item } - it { is_expected.to be_falsey } - end - end - - context "with 'none' match policy" do - let(:rule_options) { super().merge(preferred_match_policy: 'none') } - - context 'for product in rule' do - let(:line_item) { rule_line_item } - it { is_expected.to be_falsey } - end - - context 'for product not in rule' do - let(:line_item) { other_line_item } - it { is_expected.to be_truthy } - end - end - - context 'with an invalid match policy' do - let(:rule_options) { super().merge(preferred_match_policy: 'invalid') } - let(:line_item) { rule_line_item } - - it 'raises' do - expect { - rule.actionable?(line_item) - }.to raise_error('unexpected match policy: "invalid"') - end - end - end end From f50ba529e88f48994f6e77a8ea4dd7366d5dd47b Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 11:11:23 +0200 Subject: [PATCH 12/38] Option Value rule: Remove #actionable? This can now be accomplished using the LineItemActionable rule. --- .../spree/promotion/rules/option_value.rb | 21 ++------ .../promotion/rules/option_value_spec.rb | 48 ------------------- 2 files changed, 3 insertions(+), 66 deletions(-) diff --git a/core/app/models/spree/promotion/rules/option_value.rb b/core/app/models/spree/promotion/rules/option_value.rb index 6ee3856eebc..030151b2076 100644 --- a/core/app/models/spree/promotion/rules/option_value.rb +++ b/core/app/models/spree/promotion/rules/option_value.rb @@ -11,14 +11,9 @@ def applicable?(promotable) end def eligible?(promotable, _options = {}) - promotable.line_items.any? { |item| actionable?(item) } - end - - def actionable?(line_item) - pid = line_item.product.id - ovids = line_item.variant.option_values.pluck(:id) - - product_ids.include?(pid) && (value_ids(pid) & ovids).present? + promotable.line_items.any? do |item| + LineItemOptionValue.new(preferred_eligible_values: preferred_eligible_values).eligible?(item) + end end def preferred_eligible_values @@ -29,16 +24,6 @@ def preferred_eligible_values end )] end - - private - - def product_ids - preferred_eligible_values.keys - end - - def value_ids(product_id) - preferred_eligible_values[product_id] - end end end end diff --git a/core/spec/models/spree/promotion/rules/option_value_spec.rb b/core/spec/models/spree/promotion/rules/option_value_spec.rb index 26eef3780db..db01051c8fb 100644 --- a/core/spec/models/spree/promotion/rules/option_value_spec.rb +++ b/core/spec/models/spree/promotion/rules/option_value_spec.rb @@ -45,52 +45,4 @@ it { is_expected.to be false } end end - - describe "#actionable?" do - let(:line_item) { create :line_item } - let(:option_value_blue) do - create( - :option_value, - name: 'Blue', - presentation: 'Blue', - option_type: create( - :option_type, - name: 'foo-colour', - presentation: 'Colour' - ) - ) - end - let(:option_value_medium) do - create( - :option_value, - name: 'Medium', - presentation: 'M' - ) - end - before do - line_item.variant.option_values << option_value_blue - rule.preferred_eligible_values = Hash[product_id => option_value_ids] - end - subject { rule.actionable?(line_item) } - context "when the line item has the correct product" do - let(:product_id) { line_item.product.id } - context "when all of the option values match" do - let(:option_value_ids) { [option_value_blue.id] } - it { is_expected.to be true } - end - context "when any of the option values match" do - let(:option_value_ids) { [option_value_blue.id, option_value_medium.id] } - it { is_expected.to be true } - end - context "when none of the option values match" do - let(:option_value_ids) { [option_value_medium.id] } - it { is_expected.to be false } - end - end - context "when the line item's product doesn't match" do - let(:product_id) { 99 } - let(:option_value_ids) { [99] } - it { is_expected.to be false } - end - end end From 53659f1a78ab9bf0d9ebe1ab0b8de34bef71ec0f Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 11:13:34 +0200 Subject: [PATCH 13/38] Remove Rules::Taxon#actionable? This can be done with the LineItemTaxon rule now. --- .../app/models/spree/promotion/rules/taxon.rb | 16 ---- .../spree/promotion/rules/taxon_spec.rb | 74 ------------------- 2 files changed, 90 deletions(-) diff --git a/core/app/models/spree/promotion/rules/taxon.rb b/core/app/models/spree/promotion/rules/taxon.rb index 16b459636d5..e88c0fbd657 100644 --- a/core/app/models/spree/promotion/rules/taxon.rb +++ b/core/app/models/spree/promotion/rules/taxon.rb @@ -48,22 +48,6 @@ def eligible?(order, _options = {}) eligibility_errors.empty? end - def actionable?(line_item) - found = Spree::Classification.where( - product_id: line_item.variant.product_id, - taxon_id: rule_taxon_ids_with_children - ).exists? - - case preferred_match_policy - when 'any', 'all' - found - when 'none' - !found - else - raise "unexpected match policy: #{preferred_match_policy.inspect}" - end - end - def taxon_ids_string taxons.pluck(:id).join(',') end diff --git a/core/spec/models/spree/promotion/rules/taxon_spec.rb b/core/spec/models/spree/promotion/rules/taxon_spec.rb index 322be092b4e..4c866595076 100644 --- a/core/spec/models/spree/promotion/rules/taxon_spec.rb +++ b/core/spec/models/spree/promotion/rules/taxon_spec.rb @@ -24,22 +24,6 @@ expect(rule).to be_eligible(order) end - context 'when order contains items from different taxons' do - before do - product.taxons << taxon - rule.taxons << taxon - end - - it 'should act on a product within the eligible taxon' do - expect(rule).to be_actionable(order.line_items.last) - end - - it 'should not act on a product in another taxon' do - order.line_items << create(:line_item, product: create(:product, taxons: [taxon2])) - expect(rule).not_to be_actionable(order.line_items.last) - end - end - context "when order does not have any prefered taxon" do before { rule.taxons << taxon2 } it { expect(rule).not_to be_eligible(order) } @@ -157,62 +141,4 @@ end end end - - describe '#actionable?' do - let(:line_item) { order.line_items.first! } - let(:order) { create :order_with_line_items } - let(:taxon) { create :taxon, name: 'first' } - - context 'with an invalid match policy' do - before do - rule.preferred_match_policy = 'invalid' - rule.save!(validate: false) - line_item.product.taxons << taxon - rule.taxons << taxon - end - - it 'raises' do - expect { - rule.eligible?(order) - }.to raise_error('unexpected match policy: "invalid"') - end - end - - context 'when a product has a taxon of a taxon rule' do - before do - product.taxons << taxon - rule.taxons << taxon - rule.save! - end - - it 'is actionable' do - expect(rule).to be_actionable(line_item) - end - end - - context 'when a product has a taxon child of a taxon rule' do - before do - taxon.children << taxon2 - product.taxons << taxon2 - rule.taxons << taxon - rule.save! - end - - it 'is actionable' do - expect(rule).to be_actionable(line_item) - end - end - - context 'when a product does not have taxon or child taxon of a taxon rule' do - before do - product.taxons << taxon2 - rule.taxons << taxon - rule.save! - end - - it 'is not actionable' do - expect(rule).not_to be_actionable(line_item) - end - end - end end From c1ed25e67f8991ce574c815edc81bb043efdca80 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 11:25:17 +0200 Subject: [PATCH 14/38] Deprecate Promotion#line_item_actionable and PromotionRule#actionable? --- .../spree/calculator/distributed_amount.rb | 8 ++-- core/app/models/spree/promotion.rb | 47 ++++++++++++------- .../actions/create_item_adjustments.rb | 4 +- .../actions/create_quantity_adjustments.rb | 6 +-- core/app/models/spree/promotion_rule.rb | 6 --- .../actions/create_item_adjustments_spec.rb | 6 +-- core/spec/models/spree/promotion_spec.rb | 8 +++- 7 files changed, 50 insertions(+), 35 deletions(-) diff --git a/core/app/models/spree/calculator/distributed_amount.rb b/core/app/models/spree/calculator/distributed_amount.rb index a472a737591..445a0771f49 100644 --- a/core/app/models/spree/calculator/distributed_amount.rb +++ b/core/app/models/spree/calculator/distributed_amount.rb @@ -15,18 +15,18 @@ class Calculator::DistributedAmount < Calculator def compute_line_item(line_item) return 0 unless line_item return 0 unless preferred_currency.casecmp(line_item.currency).zero? - return 0 unless calculable.promotion.line_item_actionable?(line_item.order, line_item) + return 0 unless calculable.promotion.line_item_eligible?(line_item) Spree::DistributedAmountsHandler.new( - actionable_line_items(line_item.order), + eligible_line_items(line_item.order), preferred_amount ).amount(line_item) end private - def actionable_line_items(order) + def eligible_line_items(order) order.line_items.select do |line_item| - calculable.promotion.line_item_actionable?(order, line_item) + calculable.promotion.line_item_eligible?(line_item) end end end diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index d41dae981e5..4c2e0edf6ce 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -211,24 +211,14 @@ def usage_count(excluded_orders: []) end def line_item_actionable?(order, line_item, promotion_code: nil) - return false unless line_item_eligible?(line_item) - - if eligible?(order, promotion_code: promotion_code) - rules = eligible_rules(order) - if rules.blank? - true - else - rules.send(match_all? ? :all? : :any?) do |rule| - rule.actionable? line_item - end - end - else - false - end + line_item_eligible?(line_item, promotion_code: promotion_code) end + deprecate line_item_actionable?: :line_item_eligible? - def line_item_eligible?(line_item) - !blacklisted?(line_item) && !!eligible_rules(line_item) + def line_item_eligible?(line_item, promotion_code: nil) + !blacklisted?(line_item) && + !!eligible_rules(line_item) && + deprecated_line_item_actionable?(line_item, promotion_code: promotion_code) end def used_by?(user, excluded_orders = []) @@ -256,6 +246,31 @@ def remove_from(order) private + def deprecated_line_item_actionable?(line_item, promotion_code: {}) + if eligible?(line_item.order, promotion_code: promotion_code) + rules = eligible_rules(line_item.order) + if rules.blank? + true + else + rules.send(match_all? ? :all? : :any?) do |rule| + if rule.respond_to?(:actionable?) + Spree::Deprecation.warn( + <<~WARN + The API of promotion rules has changed. Rather than specifying "actionable?" on your rule, create a new rule + that is applicable to line items and move the logic in your `actionable?` method to that rule's `eligible?` method. + WARN + ) + rule.actionable? line_item + else + true + end + end + end + else + false + end + end + def blacklisted?(promotable) case promotable when Spree::LineItem diff --git a/core/app/models/spree/promotion/actions/create_item_adjustments.rb b/core/app/models/spree/promotion/actions/create_item_adjustments.rb index 19a3bf5b10f..3ed12341bc5 100644 --- a/core/app/models/spree/promotion/actions/create_item_adjustments.rb +++ b/core/app/models/spree/promotion/actions/create_item_adjustments.rb @@ -35,7 +35,7 @@ def perform(payload = {}) # item_total and ship_total def compute_amount(adjustable) order = adjustable.is_a?(Order) ? adjustable : adjustable.order - return 0 unless promotion.line_item_actionable?(order, adjustable) + return 0 unless promotion.line_item_eligible?(adjustable) promotion_amount = calculator.compute(adjustable) promotion_amount ||= BigDecimal(0) promotion_amount = promotion_amount.abs @@ -89,7 +89,7 @@ def ensure_action_has_calculator def line_items_to_adjust(promotion, order) order.line_items.select do |line_item| line_item.adjustments.none? { |adjustment| adjustment.source == self } && - promotion.line_item_actionable?(order, line_item) + promotion.line_item_eligible?(line_item) end end end diff --git a/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb b/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb index 063dbd33478..60ff65e8348 100644 --- a/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb +++ b/core/app/models/spree/promotion/actions/create_quantity_adjustments.rb @@ -61,7 +61,7 @@ def compute_amount(line_item) adjustment_amount = adjustment_amount.abs order = line_item.order - line_items = actionable_line_items(order) + line_items = eligible_line_items(order) actioned_line_items = order.line_item_adjustments.reload. select { |adjustment| adjustment.source == self && adjustment.amount < 0 }. @@ -83,9 +83,9 @@ def compute_amount(line_item) private - def actionable_line_items(order) + def eligible_line_items(order) order.line_items.select do |item| - promotion.line_item_actionable? order, item + promotion.line_item_eligible? item end end diff --git a/core/app/models/spree/promotion_rule.rb b/core/app/models/spree/promotion_rule.rb index 5fe201b7955..79439c573a6 100644 --- a/core/app/models/spree/promotion_rule.rb +++ b/core/app/models/spree/promotion_rule.rb @@ -31,12 +31,6 @@ def eligible?(_promotable, _options = {}) raise NotImplementedError, "eligible? should be implemented in a sub-class of Spree::PromotionRule" end - # This states if a promotion can be applied to the specified line item - # It is true by default, but can be overridden by promotion rules to provide conditions - def actionable?(_line_item) - true - end - def eligibility_errors @eligibility_errors ||= ActiveModel::Errors.new(self) end diff --git a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb index ee842f535f2..ebfffe4b235 100644 --- a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb +++ b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb @@ -95,7 +95,7 @@ module Spree context "#compute_amount" do before { promotion.promotion_actions = [action] } - context "when the adjustable is actionable" do + context "when the adjustable is eligible" do it "calls compute on the calculator" do expect(action.calculator).to receive(:compute).with(line_item).and_call_original action.compute_amount(line_item) @@ -113,8 +113,8 @@ module Spree end end - context "when the adjustable is not actionable" do - before { allow(promotion).to receive(:line_item_actionable?) { false } } + context "when the adjustable is not eligible" do + before { allow(promotion).to receive(:line_ite_eligible?) { false } } it 'returns 0' do expect(action.compute_amount(line_item)).to eql(0) diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index ddde53c75ae..81097f00687 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -839,6 +839,12 @@ promotion.promotion_actions = [Spree::PromotionAction.new] end + around do |example| + Spree::Deprecation.silence do + example.run + end + end + subject { promotion.line_item_actionable? order, line_item } context 'when the order is eligible for promotion' do @@ -902,7 +908,7 @@ let(:promotion) { create(:promotion, per_code_usage_limit: 0) } let(:promotion_code) { create(:promotion_code, promotion: promotion) } - subject { promotion.line_item_actionable? order, line_item, promotion_code: promotion_code } + subject { promotion.line_item_eligible? line_item, promotion_code: promotion_code } it "returns false" do expect(subject).to eq false From 24509e947905b35c80b075b11c56bf21ab27a28d Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 12:03:58 +0200 Subject: [PATCH 15/38] Fix DistributedAmounts spec --- .../spree/calculator/distributed_amount_spec.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/core/spec/models/spree/calculator/distributed_amount_spec.rb b/core/spec/models/spree/calculator/distributed_amount_spec.rb index 17f9eee82fc..86d62d7caef 100644 --- a/core/spec/models/spree/calculator/distributed_amount_spec.rb +++ b/core/spec/models/spree/calculator/distributed_amount_spec.rb @@ -31,14 +31,13 @@ let(:first_product) { order.line_items.first.product } before do - rule = Spree::Promotion::Rules::Product.create!( - promotion: promotion, - product_promotion_rules: [ - Spree::ProductPromotionRule.new(product: first_product), - ], + order_rule = Spree::Promotion::Rules::Product.new( + products: [first_product], ) - promotion.rules << rule - promotion.save! + line_item_rule = Spree::Promotion::Rules::LineItemProduct.new( + products: [first_product], + ) + promotion.rules << order_rule << line_item_rule order.recalculate end From 7d5863e646ab44477ed73351ae104a7117411981 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 12:17:33 +0200 Subject: [PATCH 16/38] Fix CreateItemAdjustments spec --- .../actions/create_item_adjustments_spec.rb | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb index ebfffe4b235..5b138a27ba3 100644 --- a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb +++ b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb @@ -10,10 +10,11 @@ module Spree let(:action) { promotion.actions.first! } let(:line_item) { order.line_items.to_a.first } let(:payload) { { order: order, promotion: promotion } } + let(:rules) { [] } before do - allow(action).to receive(:promotion).and_return(promotion) promotion.promotion_actions = [action] + promotion.promotion_rules = rules end context "#perform" do @@ -47,12 +48,10 @@ module Spree end context "with products rules" do - let(:rule) { double Spree::Promotion::Rules::Product } - - before { allow(promotion).to receive(:eligible_rules) { [rule] } } - - context "when the rule is actionable" do - before { allow(rule).to receive(:actionable?).and_return(true) } + let(:rule) { Spree::Promotion::Rules::LineItemProduct.new } + let(:rules) { [rule] } + context "when the rule is eligible" do + before { allow(rule).to receive(:eligible?).and_return(true) } it "creates an adjustment" do expect { @@ -65,8 +64,8 @@ module Spree end end - context "when the rule is not actionable" do - before { allow(rule).to receive(:actionable?).and_return(false) } + context "when the rule is not eligible" do + before { allow(rule).to receive(:eligible?).and_return(false) } it "does not create an adjustment" do expect { @@ -114,7 +113,7 @@ module Spree end context "when the adjustable is not eligible" do - before { allow(promotion).to receive(:line_ite_eligible?) { false } } + before { allow(promotion).to receive(:line_item_eligible?) { false } } it 'returns 0' do expect(action.compute_amount(line_item)).to eql(0) From 0aaead247b3cb35515bf09cbaaf7b77cb64a7467 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 13:25:06 +0200 Subject: [PATCH 17/38] Fix for Promotion#line_item_eligible spec This method now runs through promotion.eligible, which checks for the presence of actions. --- core/spec/models/spree/promotion_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 81097f00687..4eb6141b64b 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -919,7 +919,7 @@ describe "#line_item_eligible?" do let(:line_item) { build(:line_item) } - let(:promotion) { build(:promotion) } + let(:promotion) { create(:promotion, :with_action) } let(:rules) { [] } subject { promotion.line_item_eligible?(line_item) } From c142e663464dad3eb2a34c7f4e1c38ffab86ef2e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 13:37:16 +0200 Subject: [PATCH 18/38] Fix spec for Promotion#line_item_actionable? We have to fix the spec setup a bit so the specs still run through. --- core/app/models/spree/promotion.rb | 2 +- core/spec/models/spree/promotion_spec.rb | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index 4c2e0edf6ce..992e3c661a0 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -213,7 +213,7 @@ def usage_count(excluded_orders: []) def line_item_actionable?(order, line_item, promotion_code: nil) line_item_eligible?(line_item, promotion_code: promotion_code) end - deprecate line_item_actionable?: :line_item_eligible? + deprecate line_item_actionable?: :line_item_eligible?, deprecator: Spree::Deprecation def line_item_eligible?(line_item, promotion_code: nil) !blacklisted?(line_item) && diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 4eb6141b64b..f03b46a3f4a 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -828,15 +828,15 @@ end describe '#line_item_actionable?' do - let(:order) { double Spree::Order } - let(:line_item) { double Spree::LineItem } - let(:true_rule) { stub_model Spree::PromotionRule, eligible?: true, applicable?: true, actionable?: true } - let(:false_rule) { stub_model Spree::PromotionRule, eligible?: true, applicable?: true, actionable?: false } + let(:order) { line_item.order } + let(:line_item) { build(:line_item) } + let(:true_rule) { double eligible?: true, applicable?: true, actionable?: true } + let(:false_rule) { double eligible?: true, applicable?: true, actionable?: false } let(:rules) { [] } before do - promotion.promotion_rules = rules - promotion.promotion_actions = [Spree::PromotionAction.new] + allow(promotion).to receive(:rules) { rules } + allow(promotion).to receive(:actions) { [Spree::PromotionAction.new] } end around do |example| From 884c2e0172f0a695956ed7b3b0d2f84fc1c4f45f Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 13:46:26 +0200 Subject: [PATCH 19/38] User Guides: Update Promotion Rules page We now have a few more promotion rules. --- .../users/promotions/promotion-rules.html.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/guides/source/users/promotions/promotion-rules.html.md b/guides/source/users/promotions/promotion-rules.html.md index 19020aa4ae2..4398cc43574 100644 --- a/guides/source/users/promotions/promotion-rules.html.md +++ b/guides/source/users/promotions/promotion-rules.html.md @@ -18,10 +18,17 @@ creating one. - **Item Total**: Eligible if the order total (before any adjustments) is less than or greater than a specified amount. - **One Use Per User**: Eligible for use one time per user. -- **Product(s)**: Eligible for specified products only. -- **Option Value(s)**: Eligible for specified variants (product option values) - only. -- **Taxon**: Eligible for products with specified taxons. +- **Order Product(s)**: Order is eligible if it has (or does not have) + the specified products. +- **Line Item Product(s)**: Line Items are eligible if they have (or do not have) + the specified products. +- **Order Option Value(s)**: Order is eligible if it has specified variants + (product option values) only. +- **Line Item Option Value(s)**: Line Items are eligible if they have specified + variants (product option values) only. +- **Order Taxon**: Order is eligible if products with specified taxons are in the order. +- **Line Item Taxon**: Line Items are eligible if they have products with specified + taxons. - **User**: Eligible for specified users. - **User Role**: Eligible for users with the specified user role. - **User Logged In**: Eligible for users who are logged in. @@ -36,6 +43,7 @@ Every time the promotion rules are re-checked, any promotional discounts are recalculated as well. For example, if the customer added more items to the cart, those new items could now be calculated against the promotion rules. +Note that line-item level rules do not apply to order-level discounts. ## Rule matching There are two types of rule matching in Solidus' promotion system: **Match all @@ -48,4 +56,3 @@ of these rules** or **Match any of these rules**. - **Match any of these rules**: This setting allows you to create more flexible promotions. For example, if you wanted to give a discount to customers who order your Tote Bag product *or* if it's their 5th order from your store. - From 5edffa81bef42bd4590e6016de9baaa43fb25c21 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 1 Apr 2022 13:51:59 +0200 Subject: [PATCH 20/38] Update Developer Guides for new Promotion API --- .../promotions/promotion-rules.html.md | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/guides/source/developers/promotions/promotion-rules.html.md b/guides/source/developers/promotions/promotion-rules.html.md index 6c3221a955b..e2507a50095 100644 --- a/guides/source/developers/promotions/promotion-rules.html.md +++ b/guides/source/developers/promotions/promotion-rules.html.md @@ -21,9 +21,14 @@ model][promotion-rules]: - `ItemTotal`: Eligible if the order total (before any adjustments) is less than or greater than a specified amount. - `OneUsePerUser`: Eligible for use one time for each user. -- `Product`: Eligible for specified products only. -- `OptionValue`: Eligible for specified variants (product option values) only. -- `Taxon`: Eligible for products with specified taxons. +- `Product`: Order is eligible if it contains specified products. +- `LineItemProduct`: Line Item is eligible if it has one of the specified products. +- `OptionValue`: Order is eligible if it contains specified variants + (product option values) only. +- `LineItemOptionValue`: Line Item is eligible if it has one of the + specified variants (product option values). +- `Taxon`: Order is eligible if any product has the specified taxons. +- `LineItemTaxon`: Line item is eligible if item's product has the specified taxons. - `User`: Eligible for specified users. - `UserRole`: Eligible for users with the specified user role. - `UserLoggedIn`: Eligible for users who are logged in. @@ -72,10 +77,6 @@ module Spree def eligible?(order, options = {}) ... end - - def actionable?(line_item) - ... - end ... ``` @@ -84,14 +85,16 @@ Note that the `applicable?` and `eligible?` are required: - `eligible?` should return `true` or `false` to indicate if the promotion is eligible for an order. - If your promotion supports discounts for some line items but not others, - define `actionable?` to return `true` when the specified line item meets the - criteria for the promotion. It should return `true` or `false` to indicate if - this line item can have a line item adjustment carried out on it. + define a separate promotion rule that is `applicable?` for line items: + + ```ruby + def applicable?(promotable) + promotable.is_a?(Spree::LineItem) + end + ``` + + This rule should contain the line item eligibility logic in its `eligible?` method. -For example, if you are giving a promotion on specific products only, -`eligible?` should return true if the order contains one of the products -eligible for promotion, and `actionable?` should return true when the line item -specified is one of the specific products for this promotion. Note that you can retrieve the associated `Spree::Promotion` information by calling the `promotion` method. @@ -122,7 +125,7 @@ en: models: # The presentation name of the promotion rule spree/promotion/rules/my_promotion_rule: My Promotion Rule - + # If you used a custom error message spree: eligibility_errors: @@ -131,4 +134,3 @@ en: ``` After a server restart, the new rule will be available from the Solidus admin promotion interface. - From 5b78a4389cfc0a7771ccddb22578a4214bc17ae0 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 17 Mar 2022 17:48:27 +0100 Subject: [PATCH 21/38] Deprecate Promotion#match_policy == any Using the "any" match policy on promotions creates a lot of difficulty when working with the promotion system. A recent poll on the Solidus Slack indicated that most stores do not use these. It is also from the user's point of view preferable to create separate promotions for each rule in this case, as this will allow store owners to set individual start and end dates and so on for each rule. --- .../views/spree/admin/promotions/_rules.html.erb | 16 +++++++++------- core/app/models/spree/promotion.rb | 7 +++++++ core/lib/spree/app_configuration.rb | 5 +++++ .../developers/promotions/overview.html.md | 9 ++++++++- .../promotions/promotion-rules.html.md | 3 +++ 5 files changed, 32 insertions(+), 8 deletions(-) diff --git a/backend/app/views/spree/admin/promotions/_rules.html.erb b/backend/app/views/spree/admin/promotions/_rules.html.erb index 4520aa3e09e..978b2d7c32d 100644 --- a/backend/app/views/spree/admin/promotions/_rules.html.erb +++ b/backend/app/views/spree/admin/promotions/_rules.html.erb @@ -19,13 +19,15 @@ <%= form_for @promotion, url: object_url, method: :put do |f| %>
-
- <% Spree::Promotion::MATCH_POLICIES.each do |policy| %> -
- -
- <% end %> -
+ <% if Spree::Config.allow_promotions_any_match_policy %> +
+ <% Spree::Promotion::MATCH_POLICIES.each do |policy| %> +
+ +
+ <% end %> +
+ <% end %>
<% if @promotion.rules.any? %> diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index 992e3c661a0..6cc6ebd1d0f 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -3,6 +3,7 @@ module Spree class Promotion < Spree::Base MATCH_POLICIES = %w(all any) + UNACTIVATABLE_ORDER_STATES = ["complete", "awaiting_return", "returned"] attr_reader :eligibility_errors @@ -176,6 +177,12 @@ def eligible_rules(promotable, options = {}) end specific_rules else + Spree::Deprecation.warn( + <<~WARN + Your promotion "#{name}" with ID #{id} has a match_policy of 'any'. + This is deprecated, please split the promotion into separate promotions for each rule. + WARN + ) unless specific_rules.any?(&eligible) @eligibility_errors = specific_rules.map(&:eligibility_errors).detect(&:present?) return nil diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 5802fff22f7..20dd0a9db5c 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -60,6 +60,11 @@ class AppConfiguration < Preferences::Configuration # @return [Boolean] When false, customers must create an account to complete an order (default: +true+) preference :allow_guest_checkout, :boolean, default: true + # @!attribute [rw] allow_promotions_any_match_policy + # @return [Boolean] When false, admins cannot create promotions with an "any" match policy (default: +false+) + # Create individual, separate promotions for each of your rules instead. + preference :allow_promotions_any_match_policy, :boolean, default: false + # @!attribute [rw] guest_token_cookie_options # @return [Hash] Add additional guest_token cookie options here (ie. domain or path) preference :guest_token_cookie_options, :hash, default: {} diff --git a/guides/source/developers/promotions/overview.html.md b/guides/source/developers/promotions/overview.html.md index 06740935e71..45c8279c559 100644 --- a/guides/source/developers/promotions/overview.html.md +++ b/guides/source/developers/promotions/overview.html.md @@ -49,6 +49,8 @@ Take note of the following promotion attributes: - `match_policy`: When set to `all`, all promotion rules must be met in order for the promotion to be eligible. When set to `any`, just one of the [promotion rules](#promotion-rules) must be met. + Using "any" is deprecated and will be removed in future Solidus versions. + You can create separate promotions for each rule you have in mind instead. - `path`: If the promotion is activated when the customer visits a URL, this value is the path for the URL. - `per_code_usage_limit`: Specifies how many times each code can be used before @@ -86,8 +88,13 @@ By default, `Spree::Promotion`s have a `match_policy` value of `all`, meaning that all of the promotion rules on a promotion must be met before the promotion is eligible. However, this can be changed to `any`. +Using "any" is deprecated and will be removed in future Solidus versions. +You can create separate promotions for each rule you have in mind instead. + An example of a typical promotion rule would be a minimum order total of $75 -USD or that a specific product is in the cart at checkout. +USD or that a specific product is in the cart at checkout. In this case, +create one promotion with the order total rule, and a separate one with the +product rule. For a list of available rule types and more information, see the [Promotion rules][promotion-rules] article. diff --git a/guides/source/developers/promotions/promotion-rules.html.md b/guides/source/developers/promotions/promotion-rules.html.md index e2507a50095..e467e67f706 100644 --- a/guides/source/developers/promotions/promotion-rules.html.md +++ b/guides/source/developers/promotions/promotion-rules.html.md @@ -59,6 +59,9 @@ Administrators can change the match policy when adding or editing a promotion. By default, promotions use the "Match all of these rules" setting, but they can be changed to use "Match any of these rules". +Using "any" is deprecated and will be removed in future Solidus versions. +You can create separate promotions for each rule you have in mind instead. + ## Register a custom promotion rule You can create a custom promotion rule by creating a new class that inherits From 8c0b9b5bcd2ddc1032522e2a272ee9d6a0d3539c Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 17 Mar 2022 18:04:03 +0100 Subject: [PATCH 22/38] Update migrations with unnecessary "any" match_policy A promotion that has a match_policy of "any", but not more than 1 rule, can also have a match_policy of "all" with no change in behavior. This will stop a lot of people from getting deprecation warnings. --- ...omotions_with_any_policy_to_all_if_possible.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb diff --git a/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb b/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb new file mode 100644 index 00000000000..631d9cfd6fe --- /dev/null +++ b/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class SetPromotionsWithAnyPolicyToAllIfPossible < ActiveRecord::Migration[5.2] + def up + Spree::Promotion.where(match_policy: :any).includes(:promotion_rules).all.each do |promotion| + if promotion.promotion_rules.length <= 1 + promotion.update(match_policy: :all) + end + end + end + + def down + # No-Op + end +end From 88299ce4dd33bdb9064a166ae3ae094e03ad5f36 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 17 Mar 2022 18:06:45 +0100 Subject: [PATCH 23/38] Add Changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad3fa43f154..8b99db8ecc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Solidus 3.2.0.alpha (master, unreleased) +- Promotions with a `match_policy` of `any` are deprecated. - Monkey patch Authentication Bypass by CSRF Weakness vulnerability on solidus_auth_devise for extra security [GHSA-5629-8855-gf4g](https://github.com/solidusio/solidus/security/advisories/GHSA-5629-8855-gf4g) - Fix ReDos vulnerability on Spree::EmailValidator::EMAIL_REGEXP [GHSA-qxmr-qxh6-2cc9](https://github.com/solidusio/solidus/security/advisories/GHSA-qxmr-qxh6-2cc9) From 127c19753bad71f4f75a99bd4222e3ce19494615 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 18 Mar 2022 09:47:14 +0100 Subject: [PATCH 24/38] Add RSpec metadata helper "deprecated_examples" This allows us to mark a block as testing deprecated code so that it can easily be identified and removed in the future. --- core/lib/spree/testing_support/deprecated_examples.rb | 9 +++++++++ core/spec/spec_helper.rb | 1 + 2 files changed, 10 insertions(+) create mode 100644 core/lib/spree/testing_support/deprecated_examples.rb diff --git a/core/lib/spree/testing_support/deprecated_examples.rb b/core/lib/spree/testing_support/deprecated_examples.rb new file mode 100644 index 00000000000..650c338d373 --- /dev/null +++ b/core/lib/spree/testing_support/deprecated_examples.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +RSpec.configure do |config| + config.around(:each, deprecated_examples: true) do |example| + Spree::Deprecation.silence do + example.run + end + end +end diff --git a/core/spec/spec_helper.rb b/core/spec/spec_helper.rb index 803b5deb1be..0bcb1b77a5c 100644 --- a/core/spec/spec_helper.rb +++ b/core/spec/spec_helper.rb @@ -8,6 +8,7 @@ require 'rspec/core' require 'spree/testing_support/partial_double_verification' +require 'spree/testing_support/deprecated_examples' require 'spree/testing_support/preferences' require 'spree/config' require 'with_model' From 4a61e8c638e4885d619773d5586593d4ce779121 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 18 Mar 2022 09:47:55 +0100 Subject: [PATCH 25/38] Deprecate Promotion specs that test "any" match policy --- core/spec/models/spree/promotion_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index f03b46a3f4a..3b845d36cc1 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -795,7 +795,7 @@ end end - context "with 'any' match policy" do + context "with 'any' match policy", :deprecated_examples do let(:promotable) { double('Promotable') } before do @@ -867,7 +867,7 @@ end end - context 'when the match policy is any' do + context 'when the match policy is any', :deprecated_examples do before { promotion.match_policy = 'any' } context 'when at least one rule allows action on the line item' do From 665f38c75cbfb142c8a5369838ee1a3b49d57358 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sat, 19 Mar 2022 13:12:21 +0100 Subject: [PATCH 26/38] Refactor Promotion#usage_count This can be done more neatly through the #discounted_orders method --- core/app/models/spree/promotion.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index 6cc6ebd1d0f..ca39995eae0 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -68,7 +68,7 @@ def self.with_coupon_code(val) ).first end - # All orders that have been discounted using this promotion + # All orders that have been discounted using this promotionp def discounted_orders Spree::Order. joins(:all_adjustments). From 1d68443578155c1af20315d314615b9a61fbba35 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 14 Mar 2022 16:48:13 +0100 Subject: [PATCH 27/38] Add line item discounts --- core/app/models/spree/line_item.rb | 1 + core/app/models/spree/line_item_discount.rb | 8 ++++++++ core/app/models/spree/promotion_action.rb | 2 ++ ...14152948_create_spree_line_item_discounts.rb | 14 ++++++++++++++ .../factories/line_item_discount_factory.rb | 17 +++++++++++++++++ .../line_item_discount_factory_spec.rb | 11 +++++++++++ .../models/spree/line_item_discount_spec.rb | 12 ++++++++++++ 7 files changed, 65 insertions(+) create mode 100644 core/app/models/spree/line_item_discount.rb create mode 100644 core/db/migrate/20220314152948_create_spree_line_item_discounts.rb create mode 100644 core/lib/spree/testing_support/factories/line_item_discount_factory.rb create mode 100644 core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb create mode 100644 core/spec/models/spree/line_item_discount_spec.rb diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index 719a40ed8ec..9ae84a07b31 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -16,6 +16,7 @@ class LineItem < Spree::Base has_one :product, through: :variant + has_many :discounts, class_name: "Spree::LineItemDiscount", inverse_of: :line_item, dependent: :destroy, autosave: true has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :destroy has_many :inventory_units, inverse_of: :line_item diff --git a/core/app/models/spree/line_item_discount.rb b/core/app/models/spree/line_item_discount.rb new file mode 100644 index 00000000000..452b0a409e6 --- /dev/null +++ b/core/app/models/spree/line_item_discount.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module Spree + class LineItemDiscount < Spree::Base + belongs_to :line_item, inverse_of: :discounts + belongs_to :promotion_action, -> { with_discarded }, inverse_of: :line_item_discounts + end +end diff --git a/core/app/models/spree/promotion_action.rb b/core/app/models/spree/promotion_action.rb index 9ddf9153a2d..096705b0db9 100644 --- a/core/app/models/spree/promotion_action.rb +++ b/core/app/models/spree/promotion_action.rb @@ -13,6 +13,8 @@ class PromotionAction < Spree::Base belongs_to :promotion, class_name: 'Spree::Promotion', inverse_of: :promotion_actions, optional: true + has_many :line_item_discounts, class_name: "Spree::LineItemDiscount", inverse_of: :promotion_action + scope :of_type, ->(type) { where(type: Array.wrap(type).map(&:to_s)) } scope :shipping, -> { of_type(Spree::Config.environment.promotions.shipping_actions.to_a) } diff --git a/core/db/migrate/20220314152948_create_spree_line_item_discounts.rb b/core/db/migrate/20220314152948_create_spree_line_item_discounts.rb new file mode 100644 index 00000000000..11ea4043f38 --- /dev/null +++ b/core/db/migrate/20220314152948_create_spree_line_item_discounts.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateSpreeLineItemDiscounts < ActiveRecord::Migration[5.2] + def change + create_table :spree_line_item_discounts do |t| + t.references :line_item, null: false + t.references :promotion_action, null: false + t.decimal :amount, precision: 10, scale: 2, null: false + t.string :label, null: false + + t.timestamps + end + end +end diff --git a/core/lib/spree/testing_support/factories/line_item_discount_factory.rb b/core/lib/spree/testing_support/factories/line_item_discount_factory.rb new file mode 100644 index 00000000000..d2bf7b169ab --- /dev/null +++ b/core/lib/spree/testing_support/factories/line_item_discount_factory.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spree/testing_support/factory_bot' +Spree::TestingSupport::FactoryBot.when_cherry_picked do + Spree::TestingSupport::FactoryBot.deprecate_cherry_picking + + require 'spree/testing_support/factories/line_item_factory' +end + +FactoryBot.define do + factory :line_item_discount, class: 'Spree::LineItemDiscount' do + amount { BigDecimal("-2.00") } + line_item + promotion_action { Spree::Promotion::Actions::CreateItemAdjustments.new } + label { "Cheap because promotion" } + end +end diff --git a/core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb b/core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb new file mode 100644 index 00000000000..8df1022a983 --- /dev/null +++ b/core/spec/lib/spree/core/testing_support/factories/line_item_discount_factory_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'line item discount factory' do + let(:factory_class) { Spree::LineItemDiscount } + + let(:factory) { :line_item_discount } + + it_behaves_like 'a working factory' +end diff --git a/core/spec/models/spree/line_item_discount_spec.rb b/core/spec/models/spree/line_item_discount_spec.rb new file mode 100644 index 00000000000..99db47fe6e7 --- /dev/null +++ b/core/spec/models/spree/line_item_discount_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::LineItemDiscount do + subject(:line_item_discount) { build(:line_item_discount) } + + it { is_expected.to respond_to(:line_item) } + it { is_expected.to respond_to(:promotion_action) } + it { is_expected.to respond_to(:amount) } + it { is_expected.to respond_to(:label) } +end From 5a873827e264b13729ecb4478d147d1b623ac40a Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 14 Mar 2022 17:09:54 +0100 Subject: [PATCH 28/38] Add shipment discounts --- core/app/models/spree/promotion_action.rb | 1 + core/app/models/spree/shipment.rb | 6 ++++++ core/app/models/spree/shipment_discount.rb | 8 ++++++++ ...314160849_create_spree_shipment_discounts.rb | 14 ++++++++++++++ .../factories/shipment_discount_factory.rb | 17 +++++++++++++++++ .../factories/shipment_discount_factory.rb | 11 +++++++++++ .../spec/models/spree/shipment_discount_spec.rb | 12 ++++++++++++ 7 files changed, 69 insertions(+) create mode 100644 core/app/models/spree/shipment_discount.rb create mode 100644 core/db/migrate/20220314160849_create_spree_shipment_discounts.rb create mode 100644 core/lib/spree/testing_support/factories/shipment_discount_factory.rb create mode 100644 core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb create mode 100644 core/spec/models/spree/shipment_discount_spec.rb diff --git a/core/app/models/spree/promotion_action.rb b/core/app/models/spree/promotion_action.rb index 096705b0db9..deb4d9daec2 100644 --- a/core/app/models/spree/promotion_action.rb +++ b/core/app/models/spree/promotion_action.rb @@ -14,6 +14,7 @@ class PromotionAction < Spree::Base belongs_to :promotion, class_name: 'Spree::Promotion', inverse_of: :promotion_actions, optional: true has_many :line_item_discounts, class_name: "Spree::LineItemDiscount", inverse_of: :promotion_action + has_many :shipment_discounts, class_name: "Spree::ShipmentDiscount", inverse_of: :promotion_action scope :of_type, ->(type) { where(type: Array.wrap(type).map(&:to_s)) } scope :shipping, -> { of_type(Spree::Config.environment.promotions.shipping_actions.to_a) } diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 9d6604adeae..274d4a5887d 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -7,6 +7,12 @@ class Shipment < Spree::Base belongs_to :order, class_name: 'Spree::Order', touch: true, inverse_of: :shipments, optional: true belongs_to :stock_location, class_name: 'Spree::StockLocation', optional: true + has_many :discounts, + class_name: "Spree::ShipmentDiscount", + foreign_key: :shipment_id, + dependent: :destroy, + inverse_of: :shipment, + autosave: true has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :delete_all has_many :inventory_units, dependent: :destroy, inverse_of: :shipment has_many :shipping_rates, -> { order(:cost) }, dependent: :destroy, inverse_of: :shipment diff --git a/core/app/models/spree/shipment_discount.rb b/core/app/models/spree/shipment_discount.rb new file mode 100644 index 00000000000..deb00bfbd39 --- /dev/null +++ b/core/app/models/spree/shipment_discount.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module Spree + class ShipmentDiscount < Spree::Base + belongs_to :shipment, inverse_of: :discounts + belongs_to :promotion_action, -> { with_discarded }, inverse_of: :shipment_discounts + end +end diff --git a/core/db/migrate/20220314160849_create_spree_shipment_discounts.rb b/core/db/migrate/20220314160849_create_spree_shipment_discounts.rb new file mode 100644 index 00000000000..d9a73021f35 --- /dev/null +++ b/core/db/migrate/20220314160849_create_spree_shipment_discounts.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateSpreeShipmentDiscounts < ActiveRecord::Migration[5.2] + def change + create_table :spree_shipment_discounts do |t| + t.references :shipment, null: false + t.references :promotion_action, null: false + t.decimal :amount, precision: 10, scale: 2, null: false + t.string :label, null: false + + t.timestamps + end + end +end diff --git a/core/lib/spree/testing_support/factories/shipment_discount_factory.rb b/core/lib/spree/testing_support/factories/shipment_discount_factory.rb new file mode 100644 index 00000000000..c09a5fbdeb1 --- /dev/null +++ b/core/lib/spree/testing_support/factories/shipment_discount_factory.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spree/testing_support/factory_bot' +Spree::TestingSupport::FactoryBot.when_cherry_picked do + Spree::TestingSupport::FactoryBot.deprecate_cherry_picking + + require 'spree/testing_support/factories/shipment_factory' +end + +FactoryBot.define do + factory :shipment_discount, class: 'Spree::ShipmentDiscount' do + amount { BigDecimal("-4.00") } + shipment + promotion_action { Spree::Promotion::Actions::CreateAdjustment.new } + label { "10% off" } + end +end diff --git a/core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb b/core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb new file mode 100644 index 00000000000..5a36b5773fe --- /dev/null +++ b/core/spec/lib/spree/core/testing_support/factories/shipment_discount_factory.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'shipment discount factory' do + let(:factory_class) { Spree::ShipmentDiscount } + + let(:factory) { :shipment_discount } + + it_behaves_like 'a working factory' +end diff --git a/core/spec/models/spree/shipment_discount_spec.rb b/core/spec/models/spree/shipment_discount_spec.rb new file mode 100644 index 00000000000..eedc32d122d --- /dev/null +++ b/core/spec/models/spree/shipment_discount_spec.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::ShipmentDiscount do + subject(:shipment_discount) { build(:shipment_discount) } + + it { is_expected.to respond_to(:shipment) } + it { is_expected.to respond_to(:promotion_action) } + it { is_expected.to respond_to(:amount) } + it { is_expected.to respond_to(:label) } +end From 9622cd7f0b77ad320ef511fab9605324754e132f Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 15 Mar 2022 11:21:00 +0100 Subject: [PATCH 29/38] Introduce Feature Flag for the new promotion system While promotions and actions will stay the same, the way the new promotion system gets activated will be quite different. Adds a global switch so developers can choose one or the other. --- core/lib/spree/app_configuration.rb | 7 +++++++ core/spec/lib/spree/app_configuration_spec.rb | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 20dd0a9db5c..babbb0d4035 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -207,6 +207,13 @@ class AppConfiguration < Preferences::Configuration # @return [Integer] Promotions to show per-page in the admin (default: +15+) preference :promotions_per_page, :integer, default: 15 + # @!attribute [rw] promotion_system + # + # Defines whether to use adjustment- or discount-based promotions. + # + # @return [Symbol] (can be :adjustments or :discounts) + preference :promotion_system, :symbol, default: :adjustments + # @!attribute [rw] require_master_price # @return [Boolean] Require a price on the master variant of a product (default: +true+) preference :require_master_price, :boolean, default: true diff --git a/core/spec/lib/spree/app_configuration_spec.rb b/core/spec/lib/spree/app_configuration_spec.rb index 5296419e6fc..d3eb6c33535 100644 --- a/core/spec/lib/spree/app_configuration_spec.rb +++ b/core/spec/lib/spree/app_configuration_spec.rb @@ -54,6 +54,12 @@ end end + describe '@promotion_system' do + it 'is `:adjustments` by default' do + expect(prefs[:promotion_system]).to eq(:adjustments) + end + end + describe '#environment' do class DummyClass; end; From bc197514d0067318a0846220ce353dc4a4d6f3ed Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 15 Mar 2022 11:34:14 +0100 Subject: [PATCH 30/38] Disable legacy promotion system in order contents OrderContents will always run the OrderUpdater, in which adjustments and discounts will be updated too. The design decision here is to skip anything to do with promotions in OrderContents and do it directly in the OrderUpdater instead. --- core/app/models/spree/order_contents.rb | 20 ++++++++---- core/spec/models/spree/order_contents_spec.rb | 32 +++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb index bc5962b6e6d..27ec9ac6f86 100644 --- a/core/app/models/spree/order_contents.rb +++ b/core/app/models/spree/order_contents.rb @@ -38,11 +38,13 @@ def update_cart(params) if order.update(params) unless order.completed? order.line_items = order.line_items.select { |li| li.quantity > 0 } - # Update totals, then check if the order is eligible for any cart promotions. - # If we do not update first, then the item total will be wrong and ItemTotal - # promotion rules would not be triggered. - reload_totals - PromotionHandler::Cart.new(order).activate + if legacy_promotion_system? + # Update totals, then check if the order is eligible for any cart promotions. + # If we do not update first, then the item total will be wrong and ItemTotal + # promotion rules would not be triggered. + reload_totals + PromotionHandler::Cart.new(order).activate + end order.ensure_updated_shipments end reload_totals @@ -71,14 +73,18 @@ def approve(user: nil, name: nil) private def after_add_or_remove(line_item, options = {}) - reload_totals + reload_totals if legacy_promotion_system? shipment = options[:shipment] shipment.present? ? shipment.update_amounts : order.ensure_updated_shipments - PromotionHandler::Cart.new(order, line_item).activate + PromotionHandler::Cart.new(order, line_item).activate if legacy_promotion_system? reload_totals line_item end + def legacy_promotion_system? + Spree::Config.promotion_system == :adjustments + end + def reload_totals @order.recalculate end diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb index dc18f2030e0..40bd5aaf5f8 100644 --- a/core/spec/models/spree/order_contents_spec.rb +++ b/core/spec/models/spree/order_contents_spec.rb @@ -112,6 +112,38 @@ include_context "discount changes order total" end + + context "with new discount-based promotion system", pending: "Waiting for implementation" do + around do |example| + with_unfrozen_spree_preference_store do + Spree::Config.promotion_system = :discounts + example.run + Spree::Config.promotion_system = :adjustments + end + end + + context "one active order promotion" do + let!(:action) { Spree::Promotion::Actions::CreateAdjustment.create(promotion: promotion, calculator: calculator) } + + it "creates valid discount on order" do + subject.add(variant, 1) + expect(subject.order.discounts.to_a.sum(&:amount)).not_to eq 0 + end + + include_context "discount changes order total" + end + + context "one active line item promotion" do + let!(:action) { Spree::Promotion::Actions::CreateItemAdjustments.create(promotion: promotion, calculator: calculator) } + + it "creates valid discount on order" do + subject.add(variant, 1) + expect(subject.order.line_items.flat_map(&:discounts).to_a.sum(&:amount)).not_to eq 0 + end + + include_context "discount changes order total" + end + end end describe 'tax calculations' do From 064b27480e089b652ba3a9c58dc6367c581423f7 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 16 Mar 2022 00:00:42 +0100 Subject: [PATCH 31/38] Add Promotion#activatable? This checks whether a promotion can at all be activated for the current order. It does not check whether anything is blocklisted, as that would prevent a line item to be discountable if any line items on the order were non-discountable, something I consider a bug with the legacy system. --- core/app/models/spree/promotion.rb | 7 ++ core/spec/models/spree/promotion_spec.rb | 107 ++++++++--------------- 2 files changed, 43 insertions(+), 71 deletions(-) diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index ca39995eae0..78cc12a84c9 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -228,6 +228,13 @@ def line_item_eligible?(line_item, promotion_code: nil) deprecated_line_item_actionable?(line_item, promotion_code: promotion_code) end + def activatable?(order) + promotion_code = order.order_promotions.detect { |op| op.promotion_id == id }&.promotion_code + active? && + !usage_limit_exceeded?(excluded_orders: [order]) && + !promotion_code&.usage_limit_exceeded?(excluded_orders: [order]) + end + def used_by?(user, excluded_orders = []) discounted_orders. complete. diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 3b845d36cc1..5e414fc3aa4 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -827,93 +827,58 @@ end end - describe '#line_item_actionable?' do - let(:order) { line_item.order } - let(:line_item) { build(:line_item) } - let(:true_rule) { double eligible?: true, applicable?: true, actionable?: true } - let(:false_rule) { double eligible?: true, applicable?: true, actionable?: false } - let(:rules) { [] } + describe "activatable?" do + let(:order) { create(:order) } + let(:promotion) { create(:promotion, :with_order_adjustment) } - before do - allow(promotion).to receive(:rules) { rules } - allow(promotion).to receive(:actions) { [Spree::PromotionAction.new] } - end + subject { promotion.activatable?(order) } - around do |example| - Spree::Deprecation.silence do - example.run - end - end + context "when promotion is expired" do + before { promotion.expires_at = Time.current - 10.days } - subject { promotion.line_item_actionable? order, line_item } + it { is_expected.to be false } + end - context 'when the order is eligible for promotion' do - context 'when there are no rules' do - it { is_expected.to be } + context "when promotion's usage limit is exceeded" do + before do + promotion.usage_limit = 1 + create(:completed_order_with_promotion, promotion: promotion) end - context 'when there are rules' do - context 'when the match policy is all' do - before { promotion.match_policy = 'all' } - - context 'when all rules allow action on the line item' do - let(:rules) { [true_rule] } - it { is_expected.to be } - end - - context 'when at least one rule does not allow action on the line item' do - let(:rules) { [true_rule, false_rule] } - it { is_expected.not_to be } - end - end - - context 'when the match policy is any', :deprecated_examples do - before { promotion.match_policy = 'any' } - - context 'when at least one rule allows action on the line item' do - let(:rules) { [true_rule, false_rule] } - it { is_expected.to be } - end - - context 'when no rules allow action on the line item' do - let(:rules) { [false_rule] } - it { is_expected.not_to be } - end - end - - context 'when the line item has an non-promotionable product' do - let(:rules) { [true_rule] } - let(:line_item) { build(:line_item) { |li| li.variant.product.promotionable = false } } - it { is_expected.not_to be } - end + it { is_expected.to be false } + end - context "if the promotion has ineligible line item rules" do - before do - expect(promotion).to receive(:line_item_eligible?) { false } - end + context "when promotion code's usage limit is exceeded" do + let(:promotion_code) { create(:promotion_code, promotion: promotion) } - it { is_expected.to be false } - end + before do + promotion.per_code_usage_limit = 1 + order.order_promotions.create!(promotion: promotion, promotion_code: promotion_code) + create(:completed_order_with_promotion, promotion: promotion) + promotion.codes.first.adjustments.update_all(eligible: true) end + + it { is_expected.to be false } end - context 'when the order is not eligible for the promotion' do - context "due to promotion expiration" do - before { promotion.starts_at = Time.current + 2.days } - it { is_expected.not_to be } + context "when promotion is at last usage on the same order" do + let(:order) { create(:completed_order_with_promotion, promotion: promotion) } + + before do + promotion.usage_limit = 1 end - context "due to promotion code not being eligible" do - let(:order) { create(:order) } - let(:promotion) { create(:promotion, per_code_usage_limit: 0) } - let(:promotion_code) { create(:promotion_code, promotion: promotion) } + it { is_expected.to be true } + end - subject { promotion.line_item_eligible? line_item, promotion_code: promotion_code } + context "when promotion code is at last usage on the same order" do + let(:order) { create(:completed_order_with_promotion, promotion: promotion) } - it "returns false" do - expect(subject).to eq false - end + before do + promotion.per_code_usage_limit = 1 end + + it { is_expected.to be true } end end From 1164ee0ec1ee5092badd63b0db6b87f9160b3e95 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 16 Mar 2022 10:58:24 +0100 Subject: [PATCH 32/38] CreateItemAdjustments: Add #discount --- .../actions/create_item_adjustments.rb | 13 ++++++++ .../actions/create_item_adjustments_spec.rb | 30 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/core/app/models/spree/promotion/actions/create_item_adjustments.rb b/core/app/models/spree/promotion/actions/create_item_adjustments.rb index 3ed12341bc5..6e418f61ecb 100644 --- a/core/app/models/spree/promotion/actions/create_item_adjustments.rb +++ b/core/app/models/spree/promotion/actions/create_item_adjustments.rb @@ -19,6 +19,19 @@ def preload_relations [:calculator] end + def can_discount?(klass) + klass == Spree::LineItem + end + + def discount(line_item) + discount = line_item.discounts.detect do |discount| + discount.promotion_action == self + end || line_item.discounts.build(promotion_action: self) + discount.label = I18n.t('spree.adjustment_labels.line_item', promotion: Spree::Promotion.model_name.human, promotion_name: promotion.name) + discount.amount = compute_amount(line_item) + discount + end + def perform(payload = {}) order = payload[:order] promotion = payload[:promotion] diff --git a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb index 5b138a27ba3..68fb65ec387 100644 --- a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb +++ b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb @@ -17,6 +17,36 @@ module Spree promotion.promotion_rules = rules end + + context "#discount" do + subject { action.discount(line_item) } + + before do + expect(action).to receive(:compute_amount).and_return(10) + action.promotion = promotion + end + + it "builds an unpersisted discount" do + expect(subject).to be_a(Spree::LineItemDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + end + + context "if the line item already has a discount" do + before do + line_item.discounts << build(:line_item_discount, line_item: line_item, promotion_action: action) + end + + it "changes the discounts label and price" do + expect(subject).to be_a(Spree::LineItemDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + expect(subject).to be_persisted + expect(subject).to be_changed + end + end + end + context "#perform" do # Regression test for https://github.com/spree/spree/issues/3966 context "when calculator computes 0" do From 9e4e492f76854e91ece70501476fa06c2d087250 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 16 Mar 2022 11:39:20 +0100 Subject: [PATCH 33/38] Add FreeShipping#discount --- .../spree/promotion/actions/free_shipping.rb | 13 +++++++++ core/config/locales/en.yml | 1 + .../promotion/actions/free_shipping_spec.rb | 28 +++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/core/app/models/spree/promotion/actions/free_shipping.rb b/core/app/models/spree/promotion/actions/free_shipping.rb index 628f3b4cede..9281a565e3b 100644 --- a/core/app/models/spree/promotion/actions/free_shipping.rb +++ b/core/app/models/spree/promotion/actions/free_shipping.rb @@ -4,6 +4,19 @@ module Spree class Promotion < Spree::Base module Actions class FreeShipping < Spree::PromotionAction + def can_discount?(klass) + klass == Spree::Shipment + end + + def discount(shipment) + discount = shipment.discounts.detect do |discount| + discount.promotion_action == self + end || shipment.discounts.build(promotion_action: self) + discount.label = I18n.t('spree.adjustment_labels.shipment', promotion: Spree::Promotion.model_name.human, promotion_name: promotion.name) + discount.amount = compute_amount(shipment) + discount + end + def perform(payload = {}) order = payload[:order] promotion_code = payload[:promotion_code] diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 086bd574944..4841bb42689 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -836,6 +836,7 @@ en: adjustment_labels: line_item: "%{promotion} (%{promotion_name})" order: "%{promotion} (%{promotion_name})" + shipment: "%{promotion} (%{promotion_name})" tax_rates: sales_tax: "%{name}" sales_tax_with_rate: "%{name} %{amount}" diff --git a/core/spec/models/spree/promotion/actions/free_shipping_spec.rb b/core/spec/models/spree/promotion/actions/free_shipping_spec.rb index 0b9a14c1ca3..b8956f1bead 100644 --- a/core/spec/models/spree/promotion/actions/free_shipping_spec.rb +++ b/core/spec/models/spree/promotion/actions/free_shipping_spec.rb @@ -10,6 +10,34 @@ let(:payload) { { order: order, promotion_code: promotion_code } } let(:promotion_code) { promotion.codes.first! } + context "#discount" do + subject { action.discount(shipment) } + + before do + expect(action).to receive(:compute_amount).and_return(10) + action.promotion = promotion + end + + it "builds an unpersisted discount" do + expect(subject).to be_a(Spree::ShipmentDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + end + + context "if the shipment already has a discount" do + before do + shipment.discounts << build(:shipment_discount, shipment: shipment, promotion_action: action) + end + + it "changes the discounts label and price" do + expect(subject).to be_a(Spree::ShipmentDiscount) + expect(subject.amount).to eq(10) + expect(subject.label).to eq("Promotion (Promo)") + expect(subject).to be_persisted + expect(subject).to be_changed + end + end + end # From promotion spec: describe "#perform" do before do From b2f6c715bc8aec66e44b2ce42624c537721175a1 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 15 Mar 2022 18:10:57 +0100 Subject: [PATCH 34/38] Introduce Spree::Discounts This module handles discounts from the order updater. The system will check all line items and shipments for any applicable discounts and select the best for each line item / shipment in an understandable, performant way. --- core/app/models/spree/discounts/chooser.rb | 16 +++++ .../spree/discounts/line_item_discounter.rb | 28 +++++++++ .../spree/discounts/order_discounter.rb | 61 +++++++++++++++++++ .../spree/discounts/shipment_discounter.rb | 28 +++++++++ core/app/models/spree/order_updater.rb | 14 ++++- core/lib/spree/app_configuration.rb | 6 ++ core/spec/models/spree/order_contents_spec.rb | 13 +--- 7 files changed, 151 insertions(+), 15 deletions(-) create mode 100644 core/app/models/spree/discounts/chooser.rb create mode 100644 core/app/models/spree/discounts/line_item_discounter.rb create mode 100644 core/app/models/spree/discounts/order_discounter.rb create mode 100644 core/app/models/spree/discounts/shipment_discounter.rb diff --git a/core/app/models/spree/discounts/chooser.rb b/core/app/models/spree/discounts/chooser.rb new file mode 100644 index 00000000000..5aaa9af9b7f --- /dev/null +++ b/core/app/models/spree/discounts/chooser.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class Chooser + def initialize(_discountable) + # This signature is here to provide context in case + # this needs to be customized + end + + def call(discounts) + [discounts.min_by(&:amount)].compact + end + end + end +end diff --git a/core/app/models/spree/discounts/line_item_discounter.rb b/core/app/models/spree/discounts/line_item_discounter.rb new file mode 100644 index 00000000000..11808930f6a --- /dev/null +++ b/core/app/models/spree/discounts/line_item_discounter.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class LineItemDiscounter + attr_reader :promotions + + def initialize(promotions:) + @promotions = promotions + end + + def call(line_item) + discounts = promotions.select do |promotion| + promotion.eligible_rules(line_item) + end.flat_map do |promotion| + promotion.actions.select do |action| + action.can_discount? Spree::LineItem + end.map do |action| + action.discount(line_item) + end + end + + chosen_discounts = Spree::Config.discount_chooser_class.new(line_item).call(discounts) + line_item.discounts = chosen_discounts + end + end + end +end diff --git a/core/app/models/spree/discounts/order_discounter.rb b/core/app/models/spree/discounts/order_discounter.rb new file mode 100644 index 00000000000..fa22675a2df --- /dev/null +++ b/core/app/models/spree/discounts/order_discounter.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class OrderDiscounter + attr_reader :order + + def initialize(order) + @order = order + end + + def call + discount_line_items + discount_shipments + end + + private + + def discount_line_items + line_item_discounter = LineItemDiscounter.new(promotions: promotions) + order.line_items.each { |line_item| line_item_discounter.call(line_item) } + end + + def discount_shipments + shipment_discounter = ShipmentDiscounter.new(promotions: promotions) + order.shipments.each { |shipment| shipment_discounter.call(shipment) } + end + + def promotions + @_promotions ||= begin + preloader = ActiveRecord::Associations::Preloader.new + (connected_order_promotions | sale_promotions).select do |promotion| + promotion.activatable?(order) + end.map do |promotion| + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::Product" }, :products) + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::Store" }, :stores) + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::Taxon" }, :taxons) + preloader.preload(promotion.rules.select { |r| r.type == "Spree::Promotion::Rules::User" }, :users) + preloader.preload(promotion.actions.select { |a| a.respond_to?(:calculator) }, :calculator) + promotion + end + end.select { |promotion| promotion.eligible_rules(order) } + end + + def connected_order_promotions + order.promotions.includes(promotion_includes).select(&:active?) + end + + def sale_promotions + Spree::Promotion.where(apply_automatically: true).active.includes(promotion_includes) + end + + def promotion_includes + [ + :promotion_rules, + :promotion_actions, + ] + end + end + end +end diff --git a/core/app/models/spree/discounts/shipment_discounter.rb b/core/app/models/spree/discounts/shipment_discounter.rb new file mode 100644 index 00000000000..0f6c704fe74 --- /dev/null +++ b/core/app/models/spree/discounts/shipment_discounter.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Spree + module Discounts + class ShipmentDiscounter + attr_reader :promotions + + def initialize(promotions:) + @promotions = promotions + end + + def call(shipment) + discounts = promotions.select do |promotion| + promotion.eligible_rules(shipment) + end.flat_map do |promotion| + promotion.actions.select do |action| + action.can_discount? Spree::Shipment + end.map do |action| + action.discount(shipment) + end + end + + chosen_discounts = Spree::Config.discount_chooser_class.new(shipment).call(discounts) + shipment.discounts = chosen_discounts + end + end + end +end diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 444cc665229..56012107a52 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -110,8 +110,12 @@ def recalculate_adjustments # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 # It also fits the criteria for sales tax as outlined here: # http://www.boe.ca.gov/formspubs/pub113/ - update_item_promotions - update_order_promotions + if Spree::Config.promotion_system == :adjustments + update_item_promotions + update_order_promotions + elsif Spree::Promotion.order_activatable?(order) + Spree::Config.discounter_class.new(order).call + end update_taxes update_cancellations update_item_totals @@ -246,7 +250,11 @@ def update_item_totals item.adjustment_total = item.adjustments. select(&:eligible?). reject(&:included?). - sum(&:amount) + sum(&:amount) + item.discounts.sum(&:amount) + item.promo_total = item.adjustments. + select(&:promotion?). + select(&:eligible?). + sum(&:amount) + item.discounts.sum(&:amount) if item.changed? item.update_columns( diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index babbb0d4035..00dbdc48767 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -321,6 +321,12 @@ def default_pricing_options # promotion_chooser_class allows extensions to provide their own PromotionChooser class_name_attribute :promotion_chooser_class, default: 'Spree::PromotionChooser' + # promotion_handler_class allows extensions to provide their own Discount Order Updater + class_name_attribute :discounter_class, default: 'Spree::Discounts::OrderDiscounter' + + # discount_chooser_class allows extensions to provide their own discount chooser + class_name_attribute :discount_chooser_class, default: 'Spree::Discounts::Chooser' + class_name_attribute :allocator_class, default: 'Spree::Stock::Allocator::OnHandFirst' class_name_attribute :shipping_rate_sorter_class, default: 'Spree::Stock::ShippingRateSorter' diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb index 40bd5aaf5f8..7a3dffa8ee6 100644 --- a/core/spec/models/spree/order_contents_spec.rb +++ b/core/spec/models/spree/order_contents_spec.rb @@ -113,7 +113,7 @@ include_context "discount changes order total" end - context "with new discount-based promotion system", pending: "Waiting for implementation" do + context "with new discount-based promotion system" do around do |example| with_unfrozen_spree_preference_store do Spree::Config.promotion_system = :discounts @@ -122,17 +122,6 @@ end end - context "one active order promotion" do - let!(:action) { Spree::Promotion::Actions::CreateAdjustment.create(promotion: promotion, calculator: calculator) } - - it "creates valid discount on order" do - subject.add(variant, 1) - expect(subject.order.discounts.to_a.sum(&:amount)).not_to eq 0 - end - - include_context "discount changes order total" - end - context "one active line item promotion" do let!(:action) { Spree::Promotion::Actions::CreateItemAdjustments.create(promotion: promotion, calculator: calculator) } From f7d78ab5f6235aa40c5210694fae4d272392654b Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 18 Mar 2022 18:26:27 +0100 Subject: [PATCH 35/38] Adapt Spree::Promotion#discounted_orders Now that we have this handy spot where all promotion counts come from, it's relatively easy to feature-flag the function. --- core/app/models/spree/promotion.rb | 26 ++++++++----- core/spec/models/spree/promotion_spec.rb | 48 ++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 9 deletions(-) diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index 78cc12a84c9..90385a2c3d6 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -70,15 +70,23 @@ def self.with_coupon_code(val) # All orders that have been discounted using this promotionp def discounted_orders - Spree::Order. - joins(:all_adjustments). - where( - spree_adjustments: { - source_type: "Spree::PromotionAction", - source_id: actions.map(&:id), - eligible: true - } - ).distinct + if Spree::Config.promotion_system == :adjustments + Spree::Order. + joins(:all_adjustments). + where( + spree_adjustments: { + source_type: "Spree::PromotionAction", + source_id: actions.map(&:id), + eligible: true + } + ).distinct + else + Spree::Order.where.not(spree_line_item_discounts: { id: nil }). + where(spree_line_item_discounts: { promotion_action_id: actions.map(&:id) }).or( + Spree::Order.where.not(spree_shipment_discounts: { id: nil }). + where(spree_shipment_discounts: { promotion_action_id: actions.map(&:id) }) + ).left_outer_joins(line_items: :discounts, shipments: :discounts).distinct + end end def as_json(options = {}) diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 5e414fc3aa4..88d93274501 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -1014,4 +1014,52 @@ expect(order.adjustment_total).to eq(-10) end end + + describe "#discounted_orders" do + around do |example| + with_unfrozen_spree_preference_store do + Spree::Config.promotion_system = :discounts + example.run + Spree::Config.promotion_system = :adjustments + end + end + + let(:promotion) { create(:promotion, :with_action) } + let(:other_promotion) { create(:promotion, :with_action) } + let(:other_action) { other_promotion.actions.first } + let(:action) { promotion.actions.first } + + let!(:order_with_line_item_discount) do + create(:order_with_line_items).tap do |order| + order.line_items.first.discounts << build(:line_item_discount, line_item: nil, promotion_action: action) + end + end + + let!(:order_with_shipment_discount) do + create(:order_with_line_items).tap do |order| + order.shipments.first.discounts << build(:shipment_discount, shipment: nil, promotion_action: action) + end + end + + let!(:order_with_line_item_and_shipment_discount) do + create(:order_with_line_items).tap do |order| + order.shipments.first.discounts << build(:shipment_discount, shipment: nil, promotion_action: action) + order.line_items.first.discounts << build(:line_item_discount, line_item: nil, promotion_action: action) + end + end + + let!(:order_without_discount) { create(:order_with_line_items) } + + let!(:order_with_other_discount) do + create(:order_with_line_items).tap do |order| + order.shipments.first.discounts << build(:shipment_discount, shipment: nil, promotion_action: other_action) + end + end + + subject { promotion.discounted_orders } + + it { is_expected.to be_a(ActiveRecord::Relation) } + + it { is_expected.to match_array([order_with_line_item_and_shipment_discount, order_with_shipment_discount, order_with_line_item_discount]) } + end end From b9858de3cdb3583af920fd171f42ea4167328712 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Sat, 19 Mar 2022 12:13:19 +0100 Subject: [PATCH 36/38] Fix Spree::Promotion#usage_count specs or new discount system --- core/spec/models/spree/promotion_spec.rb | 101 +++++++++++++++++------ 1 file changed, 76 insertions(+), 25 deletions(-) diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 88d93274501..7b29a68c638 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -357,39 +357,90 @@ end describe "#usage_count" do - let(:promotion) do - FactoryBot.create( - :promotion, - :with_order_adjustment, - code: "discount" - ) - end + context "with legacy promotion system" do + let(:promotion) do + FactoryBot.create( + :promotion, + :with_order_adjustment, + code: "discount" + ) + end - subject { promotion.usage_count } + subject { promotion.usage_count } - context "when the code is applied to a non-complete order" do - let(:order) { FactoryBot.create(:order_with_line_items) } - before { promotion.activate(order: order, promotion_code: promotion.codes.first) } - it { is_expected.to eq 0 } + context "when the code is applied to a non-complete order" do + let(:order) { FactoryBot.create(:order_with_line_items) } + before { promotion.activate(order: order, promotion_code: promotion.codes.first) } + it { is_expected.to eq 0 } + end + context "when the code is applied to a complete order" do + let!(:order) do + FactoryBot.create( + :completed_order_with_promotion, + promotion: promotion + ) + end + context "and the promo is eligible" do + it { is_expected.to eq 1 } + end + context "and the promo is ineligible" do + before { order.adjustments.promotion.update_all(eligible: false) } + it { is_expected.to eq 0 } + end + context "and the order is canceled" do + before { order.cancel! } + it { is_expected.to eq 0 } + it { expect(order.state).to eq 'canceled' } + end + end end - context "when the code is applied to a complete order" do - let!(:order) do + + context "with discount system" do + around do |example| + with_unfrozen_spree_preference_store do + Spree::Config.promotion_system = :discounts + example.run + Spree::Config.promotion_system = :adjustments + end + end + + let(:promotion) do FactoryBot.create( - :completed_order_with_promotion, - promotion: promotion + :promotion, + :with_line_item_adjustment, ) end - context "and the promo is eligible" do - it { is_expected.to eq 1 } - end - context "and the promo is ineligible" do - before { order.adjustments.promotion.update_all(eligible: false) } + + subject { promotion.usage_count } + + context "when the code is applied to a non-complete order" do + let(:order) { FactoryBot.create(:order_with_line_items) } + before do + order.order_promotions << Spree::OrderPromotion.new(promotion_code: promotion.codes.first, promotion: promotion) + order.recalculate + end + it { is_expected.to eq 0 } end - context "and the order is canceled" do - before { order.cancel! } - it { is_expected.to eq 0 } - it { expect(order.state).to eq 'canceled' } + + context "when the code is applied to a complete order" do + let!(:order) { FactoryBot.create(:order_ready_to_complete) } + + before do + order.order_promotions << Spree::OrderPromotion.new(promotion_code: promotion.codes.first, promotion: promotion) + order.recalculate + order.complete! + end + + context "and the promo is eligible" do + it { is_expected.to eq 1 } + end + + context "and the order is canceled" do + before { order.cancel! } + it { is_expected.to eq 0 } + it { expect(order.state).to eq 'canceled' } + end end end end From ad122e41aa88f66a465ccab45cb35b2f5564f578 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 29 Mar 2022 12:39:45 +0200 Subject: [PATCH 37/38] Add Priority Column to Spree::Promotions People currently use order-level promotions in order to allow cumulative promotions. If we want to allow cumulative promotions, we need to somehow be clear about which promotion has priority after which promotion. --- core/app/models/spree/promotion.rb | 45 ++++++++++--------- ...103345_add_priority_to_spree_promotions.rb | 17 +++++++ 2 files changed, 41 insertions(+), 21 deletions(-) create mode 100644 core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index 90385a2c3d6..149058de587 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -8,6 +8,8 @@ class Promotion < Spree::Base attr_reader :eligibility_errors + acts_as_list column: :priority + belongs_to :promotion_category, optional: true has_many :promotion_rules, autosave: true, dependent: :destroy, inverse_of: :promotion @@ -41,18 +43,18 @@ class Promotion < Spree::Base scope :advertised, -> { where(advertise: true) } scope :active, -> { has_actions.started_and_unexpired } scope :started_and_unexpired, -> do - table = arel_table - time = Time.current + table = arel_table + time = Time.current - where(table[:starts_at].eq(nil).or(table[:starts_at].lt(time))). - where(table[:expires_at].eq(nil).or(table[:expires_at].gt(time))) - end + where(table[:starts_at].eq(nil).or(table[:starts_at].lt(time))). + where(table[:expires_at].eq(nil).or(table[:expires_at].gt(time))) + end scope :has_actions, -> do - joins(:promotion_actions).distinct - end + joins(:promotion_actions).distinct + end scope :applied, -> { joins(:order_promotions).distinct } - self.whitelisted_ransackable_associations = ['codes'] + self.whitelisted_ransackable_associations = ["codes"] self.whitelisted_ransackable_attributes = %w[name path promotion_category_id] def self.ransackable_scopes(*) %i(active) @@ -74,17 +76,17 @@ def discounted_orders Spree::Order. joins(:all_adjustments). where( - spree_adjustments: { - source_type: "Spree::PromotionAction", - source_id: actions.map(&:id), - eligible: true - } - ).distinct + spree_adjustments: { + source_type: "Spree::PromotionAction", + source_id: actions.map(&:id), + eligible: true, + }, + ).distinct else Spree::Order.where.not(spree_line_item_discounts: { id: nil }). where(spree_line_item_discounts: { promotion_action_id: actions.map(&:id) }).or( - Spree::Order.where.not(spree_shipment_discounts: { id: nil }). - where(spree_shipment_discounts: { promotion_action_id: actions.map(&:id) }) + Spree::Order.where.not(spree_shipment_discounts: { id: nil }). + where(spree_shipment_discounts: { promotion_action_id: actions.map(&:id) }) ).left_outer_joins(line_items: :discounts, shipments: :discounts).distinct end end @@ -127,7 +129,7 @@ def activate(order:, line_item: nil, user: nil, path: nil, promotion_code: nil) line_item: line_item, user: user, path: path, - promotion_code: promotion_code + promotion_code: promotion_code, } # Track results from actions to see if any action has been taken. @@ -186,11 +188,12 @@ def eligible_rules(promotable, options = {}) specific_rules else Spree::Deprecation.warn( - <<~WARN - Your promotion "#{name}" with ID #{id} has a match_policy of 'any'. + <<~WARN + Your promotion "#{name}" with ID #{id} has a match_policy of 'any'. This is deprecated, please split the promotion into separate promotions for each rule. - WARN - ) + WARN + +) unless specific_rules.any?(&eligible) @eligibility_errors = specific_rules.map(&:eligibility_errors).detect(&:present?) return nil diff --git a/core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb b/core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb new file mode 100644 index 00000000000..412cc8505e5 --- /dev/null +++ b/core/db/migrate/20220329103345_add_priority_to_spree_promotions.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddPriorityToSpreePromotions < ActiveRecord::Migration[5.2] + def up + add_column :spree_promotions, :priority, :integer + sql = <<~SQL + UPDATE spree_promotions + SET priority = id + SQL + execute(sql) + change_column :spree_promotions, :priority, :integer, null: false + end + + def down + remove_column :spree_promotions, :priority, :integer, null: false + end +end From 408315dd3fbe6a15820b6629f040db795ad15bfe Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 29 Mar 2022 12:45:03 +0200 Subject: [PATCH 38/38] Add Priority to Promotion Actions Similar to the scenario of multiple promotions, we need to account for multiple actions on the same promotion. Also here we need to sort by explicit priority when allowing cumulative promotions. --- core/app/models/spree/promotion_action.rb | 10 ++++++---- ...2_add_priority_to_spree_promotion_actions.rb | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb diff --git a/core/app/models/spree/promotion_action.rb b/core/app/models/spree/promotion_action.rb index deb4d9daec2..989b53e3d76 100644 --- a/core/app/models/spree/promotion_action.rb +++ b/core/app/models/spree/promotion_action.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'spree/preferences/persistable' +require "spree/preferences/persistable" module Spree # Base class for all types of promotion action. @@ -11,7 +11,9 @@ class PromotionAction < Spree::Base include Spree::Preferences::Persistable include Spree::SoftDeletable - belongs_to :promotion, class_name: 'Spree::Promotion', inverse_of: :promotion_actions, optional: true + acts_as_list column: :priority, scope: :promotion_id + + belongs_to :promotion, class_name: "Spree::Promotion", inverse_of: :promotion_actions, optional: true has_many :line_item_discounts, class_name: "Spree::LineItemDiscount", inverse_of: :promotion_action has_many :shipment_discounts, class_name: "Spree::ShipmentDiscount", inverse_of: :promotion_action @@ -30,7 +32,7 @@ def preload_relations # # @note This method should be overriden in subclassses. def perform(_options = {}) - raise 'perform should be implemented in a sub-class of PromotionAction' + raise "perform should be implemented in a sub-class of PromotionAction" end # Removes the action from an order @@ -40,7 +42,7 @@ def perform(_options = {}) # @param order [Spree::Order] the order to remove the action from # @return [void] def remove_from(_order) - raise 'remove_from should be implemented in a sub-class of PromotionAction' + raise "remove_from should be implemented in a sub-class of PromotionAction" end def to_partial_path diff --git a/core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb b/core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb new file mode 100644 index 00000000000..78c6a1af3fa --- /dev/null +++ b/core/db/migrate/20220329104242_add_priority_to_spree_promotion_actions.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddPriorityToSpreePromotionActions < ActiveRecord::Migration[5.2] + def up + add_column :spree_promotion_actions, :priority, :integer + sql = <<~SQL + UPDATE spree_promotion_actions + SET priority = id + SQL + execute(sql) + change_column :spree_promotion_actions, :priority, :integer, null: false + end + + def down + remove_column :spree_promotion_actions, :priority, :integer, null: false + end +end