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) 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/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..a36bb5efe90 --- /dev/null +++ b/core/db/migrate/20220317165036_set_promotions_with_any_policy_to_all_if_possible.rb @@ -0,0 +1,20 @@ +# 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) + 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 + + def down + # No-Op + end +end 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/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/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 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' 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