From de1d4a79645ea6f17e26c8e555fb7d7f56077ffd Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 17 Mar 2022 17:48:27 +0100 Subject: [PATCH 1/5] 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 d24bdfdea76..a2038fe38fc 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 @@ -163,6 +164,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 d167d0c83f2..5f9baf11933 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 6c3221a955b..b71e0bfcbcb 100644 --- a/guides/source/developers/promotions/promotion-rules.html.md +++ b/guides/source/developers/promotions/promotion-rules.html.md @@ -54,6 +54,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 e09c89f1f5d71534d3c639e66c12b9df41f3a4d5 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 17 Mar 2022 18:04:03 +0100 Subject: [PATCH 2/5] 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..040debff78d --- /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 4bdd628d142d36fb7e5e09d3a8304cd94442ffff Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 17 Mar 2022 18:06:45 +0100 Subject: [PATCH 3/5] Add Changelog entry --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad3fa43f154..a9c1aa6c961 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,9 +43,31 @@ of Solidus recommendations (that files are monkey patches; they don't use the you can place those files in `app/overrides/` and remove the `decorator` suffix. +### Changes to the promotion system + +Promotions with a `match_policy` of `any` are deprecated. If you have promotions +with such a match policy, try running the following rake task: + +```bash +bin/rake solidus:split_promotions_with_any_match_policy +``` + +This will create separate promotions for each of the rules of your promotions with `any` +match policy, which should have the same outcome for customers. + +Creating new promotions with `any` match policy is turned off by default. If you still want +to create promotions like that (knowing they will not be supported in the future), you can +set a temporary flag in your `config/initializers/spree.rb` file: + +```ruby +# Allow creating new promotions with an `any` match policy. Unsupported in the future. +config.allow_promotions_any_match_policy = true +``` + ### Core - Add configuration option for `migration_path` [#4190](https://github.com/solidusio/solidus/pull/4190) ([SuperGoodSoft](https://github.com/supergoodsoft/)) +- Deprecate Promotion `any` Match Policy [#4304](https://github.com/solidusio/solidus/pull/4304) ([mamhoff](https://www.github.com/mamhoff)) ## Solidus 3.1.5 (v3.1, 2021-12-20) From b86706f24a6d68e16307f0bad6d5904b8e27a55c Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 18 Mar 2022 09:47:14 +0100 Subject: [PATCH 4/5] Add RSpec metadata helper "silence_deprecations" 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/silence_deprecations.rb | 9 +++++++++ core/spec/spec_helper.rb | 1 + 2 files changed, 10 insertions(+) create mode 100644 core/lib/spree/testing_support/silence_deprecations.rb diff --git a/core/lib/spree/testing_support/silence_deprecations.rb b/core/lib/spree/testing_support/silence_deprecations.rb new file mode 100644 index 00000000000..0bcefe6bce5 --- /dev/null +++ b/core/lib/spree/testing_support/silence_deprecations.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +RSpec.configure do |config| + config.around(:each, silence_deprecations: 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..1c1cab82818 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/silence_deprecations' require 'spree/testing_support/preferences' require 'spree/config' require 'with_model' From b3ea86375d0504dffda13092575288c139ed3900 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Fri, 18 Mar 2022 09:47:55 +0100 Subject: [PATCH 5/5] Deprecate Promotion specs that test "any" match policy --- ...ions_with_any_policy_to_all_if_possible.rb | 5 +++ ...plit_promotions_with_any_match_policy.rake | 33 +++++++++++++++++ ...t_promotions_with_any_match_policy_spec.rb | 35 +++++++++++++++++++ core/spec/models/spree/promotion_spec.rb | 4 +-- 4 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 core/lib/tasks/solidus/split_promotions_with_any_match_policy.rake create mode 100644 core/spec/lib/tasks/solidus/split_promotions_with_any_match_policy_spec.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 index 040debff78d..a36bb5efe90 100644 --- 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 @@ -5,6 +5,11 @@ 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) + else + raise StandardError, <<~MSG + You have promotions with a match policy of any and more than one rule. Please + run `bundle exec rake solidus:split_promotions_with_any_match_policy`. + MSG end end end 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 new file mode 100644 index 00000000000..ece805597d5 --- /dev/null +++ b/core/lib/tasks/solidus/split_promotions_with_any_match_policy.rake @@ -0,0 +1,33 @@ +# 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 new file mode 100644 index 00000000000..dffad197957 --- /dev/null +++ b/core/spec/lib/tasks/solidus/split_promotions_with_any_match_policy_spec.rb @@ -0,0 +1,35 @@ +# 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 2b2294d572b..1dcdc483cae 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", :silence_deprecations do let(:promotable) { double('Promotable') } before do @@ -861,7 +861,7 @@ end end - context 'when the match policy is any' do + 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