diff --git a/backend/app/assets/stylesheets/spree/backend/sections/_promotions.scss b/backend/app/assets/stylesheets/spree/backend/sections/_promotions.scss index b3921d39d47..687e5e19f51 100644 --- a/backend/app/assets/stylesheets/spree/backend/sections/_promotions.scss +++ b/backend/app/assets/stylesheets/spree/backend/sections/_promotions.scss @@ -18,11 +18,6 @@ } } -#promotion-policy-select { - overflow: auto; - margin-bottom: 1.5rem; -} - .promotion-block { padding: 0 1.25rem 0.5rem; background-color: lighten($color-border, 5); diff --git a/backend/app/views/spree/admin/promotions/_rules.html.erb b/backend/app/views/spree/admin/promotions/_rules.html.erb index cfcb0ad2e5d..de58398d657 100644 --- a/backend/app/views/spree/admin/promotions/_rules.html.erb +++ b/backend/app/views/spree/admin/promotions/_rules.html.erb @@ -18,16 +18,6 @@ <%= form_for @promotion, url: object_url, method: :put do |f| %>
- <% 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 6b4794b7a88..fcf3702b7e7 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -2,6 +2,7 @@ module Spree class Promotion < Spree::Base + # TODO: deprecate this in another PR and remove it here after merging it MATCH_POLICIES = %w(all any) UNACTIVATABLE_ORDER_STATES = ["complete", "awaiting_return", "returned"] @@ -166,27 +167,13 @@ def eligible_rules(promotable, options = {}) specific_rules = rules.select { |rule| rule.applicable?(promotable) } return [] if specific_rules.none? - if match_all? - # If there are rules for this promotion, but no rules for this - # particular promotable, then the promotion is ineligible by default. - unless specific_rules.all?(&eligible) - @eligibility_errors = specific_rules.map(&:eligibility_errors).detect(&:present?) - return nil - 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 - end - specific_rules.select(&eligible) + # If there are rules for this promotion, but no rules for this + # particular promotable, then the promotion is ineligible by default. + unless specific_rules.all?(&eligible) + @eligibility_errors = specific_rules.map(&:eligibility_errors).detect(&:present?) + return nil end + specific_rules end def products @@ -223,9 +210,7 @@ def line_item_actionable?(order, line_item, promotion_code: nil) if rules.blank? true else - rules.send(match_all? ? :all? : :any?) do |rule| - rule.actionable? line_item - end + rules.all? { |rule| rule.actionable? line_item } end else false @@ -270,10 +255,6 @@ def normalize_blank_values self[:path] = nil if self[:path].blank? end - def match_all? - match_policy == "all" - end - def apply_automatically_disallowed_with_paths return unless apply_automatically diff --git a/core/db/migrate/20230322085416_remove_match_policy_from_spree_promotion.rb b/core/db/migrate/20230322085416_remove_match_policy_from_spree_promotion.rb new file mode 100644 index 00000000000..39cdc474a97 --- /dev/null +++ b/core/db/migrate/20230322085416_remove_match_policy_from_spree_promotion.rb @@ -0,0 +1,5 @@ +class RemoveMatchPolicyFromSpreePromotion < ActiveRecord::Migration[5.2] + def change + remove_column :spree_promotions, :match_policy, :string, default: "all" + end +end diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 75d7ed31d8b..d5e6e8034bf 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -61,6 +61,7 @@ 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 + # Todo: remove this preference after merging https://github.com/solidusio/solidus/pull/4991 # @!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. diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 1dcdc483cae..b938c202f2f 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -745,6 +745,9 @@ context "#eligible_rules" do let(:promotable) { double('Promotable') } + let(:rule1) { Spree::PromotionRule.create!(promotion: promotion) } + let(:rule2) { Spree::PromotionRule.create!(promotion: promotion) } + it "true if there are no rules" do expect(promotion.eligible_rules(promotable)).to eq [] end @@ -755,74 +758,36 @@ expect(promotion.eligible_rules(promotable)).to eq [] end - context "with 'all' match policy" do - let(:rule1) { Spree::PromotionRule.create!(promotion: promotion) } - let(:rule2) { Spree::PromotionRule.create!(promotion: promotion) } - - before { promotion.match_policy = 'all' } - - context "when all rules are eligible" do - before do - allow(rule1).to receive_messages(eligible?: true, applicable?: true) - allow(rule2).to receive_messages(eligible?: true, applicable?: true) + context "when all rules are eligible" do + before do + allow(rule1).to receive_messages(eligible?: true, applicable?: true) + allow(rule2).to receive_messages(eligible?: true, applicable?: true) - promotion.promotion_rules = [rule1, rule2] - end - it "returns the eligible rules" do - expect(promotion.eligible_rules(promotable)).to eq [rule1, rule2] - end - it "does set anything to eligiblity errors" do - promotion.eligible_rules(promotable) - expect(promotion.eligibility_errors).to be_nil - end + promotion.promotion_rules = [rule1, rule2] end - - context "when any of the rules is not eligible" do - let(:errors) { double ActiveModel::Errors, empty?: false } - before do - allow(rule1).to receive_messages(eligible?: true, applicable?: true, eligibility_errors: nil) - allow(rule2).to receive_messages(eligible?: false, applicable?: true, eligibility_errors: errors) - - promotion.promotion_rules = [rule1, rule2] - end - it "returns nil" do - expect(promotion.eligible_rules(promotable)).to be_nil - end - it "sets eligibility errors to the first non-nil one" do - promotion.eligible_rules(promotable) - expect(promotion.eligibility_errors).to eq errors - end + it "returns the eligible rules" do + expect(promotion.eligible_rules(promotable)).to eq [rule1, rule2] + end + it "does set anything to eligiblity errors" do + promotion.eligible_rules(promotable) + expect(promotion.eligibility_errors).to be_nil end end - context "with 'any' match policy", :silence_deprecations do - let(:promotable) { double('Promotable') } - + context "when any of the rules is not eligible" do + let(:errors) { double ActiveModel::Errors, empty?: false } before do - promotion.match_policy = 'any' - end + allow(rule1).to receive_messages(eligible?: true, applicable?: true, eligibility_errors: nil) + allow(rule2).to receive_messages(eligible?: false, applicable?: true, eligibility_errors: errors) - it "should have eligible rules if any of the rules are eligible" do - true_rule = stub_model(Spree::PromotionRule, eligible?: true, applicable?: true) - promotion.promotion_rules = [true_rule] - expect(promotion.eligible_rules(promotable)).to eq [true_rule] + promotion.promotion_rules = [rule1, rule2] end - - context "when none of the rules are eligible" do - let(:rule) { Spree::PromotionRule.create!(promotion: promotion) } - let(:errors) { double ActiveModel::Errors, empty?: false } - before do - allow(rule).to receive_messages(eligible?: false, applicable?: true, eligibility_errors: errors) - - promotion.promotion_rules = [rule] - end - it "returns nil" do - expect(promotion.eligible_rules(promotable)).to be_nil - end - it "sets eligibility errors to the first non-nil one" do - promotion.eligible_rules(promotable) - expect(promotion.eligibility_errors).to eq errors - end + it "returns nil" do + expect(promotion.eligible_rules(promotable)).to be_nil + end + it "sets eligibility errors to the first non-nil one" do + promotion.eligible_rules(promotable) + expect(promotion.eligibility_errors).to eq errors end end end @@ -847,32 +812,14 @@ 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 + context 'when all rules allow action on the line item' do + let(:rules) { [true_rule] } + it { is_expected.to be } end - context 'when the match policy is any', :silence_deprecations 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 + 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 context 'when the line item has an non-promotionable product' do