From 3fcdd9620f3713f674f21bb450358f645ac99233 Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 23 Mar 2023 10:30:56 +0100 Subject: [PATCH] [Extract][Has TODOs]Remove support for deprecated promo rules matching policy Matching "all" rules is the only supported path now. You can have the same effect of the previoys "any" matching policy by creating multiple promotions with the same configuration. This change also removes the rake task we provided to update the existing promotions. That script needs to be executed on previous versions of Solidus only. Ref https://github.com/solidusio/solidus/pull/4304 --- api/lib/spree/api_configuration.rb | 2 +- .../spree/backend/sections/_promotions.scss | 5 - .../spree/admin/promotions/_rules.html.erb | 10 -- core/app/models/spree/promotion.rb | 35 ++---- ...emove_match_policy_from_spree_promotion.rb | 5 + core/lib/spree/app_configuration.rb | 1 + ...plit_promotions_with_any_match_policy.rake | 33 ----- ...t_promotions_with_any_match_policy_spec.rb | 35 ------ core/spec/models/spree/promotion_spec.rb | 115 +++++------------- 9 files changed, 46 insertions(+), 195 deletions(-) create mode 100644 core/db/migrate/20230322085416_remove_match_policy_from_spree_promotion.rb delete mode 100644 core/lib/tasks/solidus/split_promotions_with_any_match_policy.rake delete mode 100644 core/spec/lib/tasks/solidus/split_promotions_with_any_match_policy_spec.rb diff --git a/api/lib/spree/api_configuration.rb b/api/lib/spree/api_configuration.rb index 522723aedb9..3247c64dd39 100644 --- a/api/lib/spree/api_configuration.rb +++ b/api/lib/spree/api_configuration.rb @@ -114,7 +114,7 @@ class ApiConfiguration < Preferences::Configuration preference :promotion_attributes, :array, default: [ :id, :name, :description, :expires_at, :starts_at, :type, :usage_limit, - :match_policy, :advertise, :path + :advertise, :path ] preference :store_attributes, :array, default: [ 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/lib/tasks/solidus/split_promotions_with_any_match_policy.rake b/core/lib/tasks/solidus/split_promotions_with_any_match_policy.rake deleted file mode 100644 index ece805597d5..00000000000 --- a/core/lib/tasks/solidus/split_promotions_with_any_match_policy.rake +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -namespace :solidus do - desc "Split Promotions with 'any' match policy" - task split_promotions_with_any_match_policy: :environment do - Spree::Promotion.where(match_policy: :any).includes(:promotion_rules).all.each do |promotion| - if promotion.promotion_rules.length <= 1 - promotion.update!(match_policy: :all) - elsif promotion.active? - promotion.rules.map do |rule| - new_promotion = promotion.dup - new_promotion.promotion_rules = [rule] - new_promotion.match_policy = "all" - new_promotion.promotion_actions = promotion.actions.map do |action| - new_action = action.dup - if action.respond_to?(:calculator) - new_action.calculator = action.calculator.dup - end - new_action.promotion = new_promotion - new_action.save! - new_action - end - new_promotion.expires_at = promotion.expires_at - new_promotion.starts_at = Time.current - new_promotion.save! - end - promotion.update!(expires_at: Time.current) - end - end - - Spree::Order.where(completed_at: nil).each { |order| Spree::PromotionHandler::Cart.new(order).activate } - end -end diff --git a/core/spec/lib/tasks/solidus/split_promotions_with_any_match_policy_spec.rb b/core/spec/lib/tasks/solidus/split_promotions_with_any_match_policy_spec.rb deleted file mode 100644 index dffad197957..00000000000 --- a/core/spec/lib/tasks/solidus/split_promotions_with_any_match_policy_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -path = Spree::Core::Engine.root.join('lib/tasks/solidus/split_promotions_with_any_match_policy.rake') - -RSpec.describe 'solidus' do - describe 'split_promotions_with_any_match_policy' do - include_context( - 'rake', - task_path: path, - task_name: 'solidus:split_promotions_with_any_match_policy' - ) - let(:rule_1) { Spree::Promotion::Rules::Product.new } - let(:rule_2) { Spree::Promotion::Rules::Product.new } - let(:rule_3) { Spree::Promotion::Rules::Product.new } - let(:rule_4) { Spree::Promotion::Rules::Product.new } - let!(:promotion_with_all_match_policy) { create(:promotion, :with_action, promotion_rules: [rule_1, rule_2]) } - let!(:promotion_with_any_match_policy) { create(:promotion, :with_action, match_policy: "any", promotion_rules: [rule_3, rule_4]) } - - subject { task.invoke } - it 'does not touch promotions with an all match policy' do - expect { task.invoke }.not_to change(promotion_with_all_match_policy, :expires_at) - end - - it "replaces promotions with any match policy with new ones, one for each rule" do - expect { task.invoke }.to change { promotion_with_any_match_policy.reload.expires_at }.from(nil) - - expect(Spree::Promotion.count).to eq(4) - expect(promotion_with_any_match_policy.expires_at).to be_present - expect(promotion_with_any_match_policy.rules).to be_empty - expect((Spree::Promotion.all - [promotion_with_all_match_policy, promotion_with_any_match_policy]).flat_map(&:rules)).to match_array([rule_3, rule_4]) - end - end -end diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 85877670b25..dc7677bac57 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 @@ -754,74 +757,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 @@ -846,32 +811,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