Skip to content

Commit

Permalink
Remove support for deprecated promo rules matching policy
Browse files Browse the repository at this point in the history
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 #4304
  • Loading branch information
kennyadsl committed Apr 18, 2023
1 parent 04a4d68 commit 86cd437
Show file tree
Hide file tree
Showing 10 changed files with 44 additions and 228 deletions.
2 changes: 1 addition & 1 deletion api/lib/spree/api_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 0 additions & 10 deletions backend/app/views/spree/admin/promotions/_rules.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,6 @@

<%= form_for @promotion, url: object_url, method: :put do |f| %>
<fieldset class="no-border-top">
<% if Spree::Config.allow_promotions_any_match_policy %>
<div id="promotion-policy-select" class="align-center row">
<% Spree::Promotion::MATCH_POLICIES.each do |policy| %>
<div class="col-6">
<label><%= f.radio_button :match_policy, policy %> <%= t "spree.promotion_form.match_policies.#{policy}" %></label>
</div>
<% end %>
</div>
<% end %>

<div id="rules" class="filter_list">
<% if @promotion.rules.any? %>
<div class="col-12">
Expand Down
37 changes: 7 additions & 30 deletions core/app/models/spree/promotion.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

module Spree
class Promotion < Spree::Base

autoload(:MATCH_POLICIES, "spree/promotion/match_policies")

UNACTIVATABLE_ORDER_STATES = ["complete", "awaiting_return", "returned"]

attr_reader :eligibility_errors
Expand Down Expand Up @@ -167,27 +164,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
Expand Down Expand Up @@ -224,9 +207,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
Expand Down Expand Up @@ -271,10 +252,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

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class RemoveMatchPolicyFromSpreePromotion < ActiveRecord::Migration[5.2]
def change
remove_column :spree_promotions, :match_policy, :string, default: "all"
end
end
21 changes: 0 additions & 21 deletions core/lib/spree/app_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,27 +61,6 @@ 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
def allow_promotions_any_match_policy=(value)
if value == true
Spree::Deprecation.warn <<~MSG
Solidus 4.0 will remove support for combining promotion rules with the "any" match policy.
Instead, it's suggested to create individual, separate promotions for each of your current
rules combined with the "any" policy. To automate this task, you can use the provided
task:
bin/rake solidus:split_promotions_with_any_match_policy
MSG
end

preferences[:allow_promotions_any_match_policy] = value
end


# @!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: {}
Expand Down
2 changes: 0 additions & 2 deletions core/lib/spree/promotion/match_policies.rb

This file was deleted.

33 changes: 0 additions & 33 deletions core/lib/tasks/solidus/split_promotions_with_any_match_policy.rake

This file was deleted.

This file was deleted.

122 changes: 31 additions & 91 deletions core/spec/models/spree/promotion_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -993,11 +940,4 @@
expect(order.adjustment_total).to eq(-10)
end
end

describe "MATCH_POLICIES" do
it "prints a deprecation warning when used" do
expect(Spree::Deprecation).to receive(:warn).once.with(/Spree::Promotion::MATCH_POLICIES is deprecated/)
expect(Spree::Promotion::MATCH_POLICIES).to eq %w(all any)
end
end
end

0 comments on commit 86cd437

Please sign in to comment.