Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove support for deprecated promo rules matching policy #5019

Merged
merged 1 commit into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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