From 246125e32f62fe0652c22acbaa72725f71312c12 Mon Sep 17 00:00:00 2001 From: Massimiliano Lattanzio Date: Fri, 31 Jan 2020 12:43:34 +0100 Subject: [PATCH 1/5] Add model validation on promotion code creation Otherwise it will be possible to create useless promotions codes on promotions that apply automatically to all orders --- core/app/models/spree/promotion_code.rb | 5 +++++ core/config/locales/en.yml | 4 ++++ core/spec/models/spree/promotion_code_spec.rb | 7 +++++++ 3 files changed, 16 insertions(+) diff --git a/core/app/models/spree/promotion_code.rb b/core/app/models/spree/promotion_code.rb index a6afc02e8eb..9c616c5e58c 100644 --- a/core/app/models/spree/promotion_code.rb +++ b/core/app/models/spree/promotion_code.rb @@ -7,6 +7,7 @@ class Spree::PromotionCode < Spree::Base validates :value, presence: true, uniqueness: { allow_blank: true } validates :promotion, presence: true + validate :promotion_not_apply_automatically, on: :create before_save :normalize_code @@ -37,6 +38,10 @@ def usage_limit promotion.per_code_usage_limit end + def promotion_not_apply_automatically + errors.add(:base, :disallowed_with_apply_automatically) if promotion.apply_automatically + end + private def normalize_code diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index d0618afe577..d97b524fc93 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -478,6 +478,10 @@ en: apply_automatically: disallowed_with_code: Disallowed for promotions with a code disallowed_with_path: Disallowed for promotions with a path + spree/promotion_code: + attributes: + base: + disallowed_with_apply_automatically: Could not create promotion code on promotion that apply automatically spree/refund: attributes: amount: diff --git a/core/spec/models/spree/promotion_code_spec.rb b/core/spec/models/spree/promotion_code_spec.rb index 496c12a1351..e048dfdcdd5 100644 --- a/core/spec/models/spree/promotion_code_spec.rb +++ b/core/spec/models/spree/promotion_code_spec.rb @@ -203,4 +203,11 @@ }.to change{ order.reload.state }.from("confirm").to("address") end end + + it "cannot create promotion code on apply automatically promotion" do + promotion = create(:promotion, apply_automatically: true) + expect { + create(:promotion_code, promotion: promotion) + }.to raise_error ActiveRecord::RecordInvalid, "Validation failed: Could not create promotion code on promotion that apply automatically" + end end From 9e468d4967387abb4fd7ad08fc247590f2d1084c Mon Sep 17 00:00:00 2001 From: Massimiliano Lattanzio Date: Sat, 1 Feb 2020 13:42:19 +0100 Subject: [PATCH 2/5] Disable codes creation on apply-auto promos in admin Deleted useless button/route in admin UI to create new promotion code if a promotion applies automatically. --- .../controllers/spree/admin/promotion_codes_controller.rb | 7 ++++++- .../app/views/spree/admin/promotion_codes/index.html.erb | 2 +- .../spree/admin/promotion_codes_controller_spec.rb | 7 +++++++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/backend/app/controllers/spree/admin/promotion_codes_controller.rb b/backend/app/controllers/spree/admin/promotion_codes_controller.rb index 6c19fa51ff6..6ee3601c6cb 100644 --- a/backend/app/controllers/spree/admin/promotion_codes_controller.rb +++ b/backend/app/controllers/spree/admin/promotion_codes_controller.rb @@ -23,7 +23,12 @@ def index def new @promotion = Spree::Promotion.accessible_by(current_ability, :read).find(params[:promotion_id]) - @promotion_code = @promotion.promotion_codes.build + if @promotion.apply_automatically + flash[:error] = t('activerecord.errors.models.spree/promotion_code.attributes.base.disallowed_with_apply_automatically') + redirect_to admin_promotion_promotion_codes_url(@promotion) + else + @promotion_code = @promotion.promotion_codes.build + end end def create diff --git a/backend/app/views/spree/admin/promotion_codes/index.html.erb b/backend/app/views/spree/admin/promotion_codes/index.html.erb index 97b9e8e005b..75e00969544 100644 --- a/backend/app/views/spree/admin/promotion_codes/index.html.erb +++ b/backend/app/views/spree/admin/promotion_codes/index.html.erb @@ -4,7 +4,7 @@ <% content_for :page_actions do %>
  • - <% if can?(:create, Spree::PromotionCode) %> + <% if can?(:create, Spree::PromotionCode) && !@promotion.apply_automatically? %> <%= link_to t('spree.create_promotion_code'), new_admin_promotion_promotion_code_path(promotion_id: @promotion.id), class: 'btn btn-primary' %> <% end %> diff --git a/backend/spec/controllers/spree/admin/promotion_codes_controller_spec.rb b/backend/spec/controllers/spree/admin/promotion_codes_controller_spec.rb index 7ff525aff10..65f2719fc5c 100644 --- a/backend/spec/controllers/spree/admin/promotion_codes_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/promotion_codes_controller_spec.rb @@ -29,4 +29,11 @@ expect(flash[:error]).not_to be_nil expect(Spree::PromotionCode.where(promotion: promotion).count).to eql(3) end + + it "can't create a new code on promotions that apply automatically" do + apply_automatically_promotion = create(:promotion, apply_automatically: true) + get :new, params: { promotion_id: apply_automatically_promotion.id } + expect(response).to redirect_to(spree.admin_promotion_promotion_codes_path(apply_automatically_promotion)) + expect(flash[:error]).not_to be_nil + end end From 9a3a5ba79ae2aba9a1e7e113e82acb5562115df7 Mon Sep 17 00:00:00 2001 From: Massimiliano Lattanzio Date: Sat, 1 Feb 2020 13:52:03 +0100 Subject: [PATCH 3/5] Add note for admin on promotion creation To explain that if promotion is created with apply automatically option, promo codes creation will be disabled for that promotion. --- .../app/views/spree/admin/promotions/_activations_new.html.erb | 2 +- core/config/locales/en.yml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/app/views/spree/admin/promotions/_activations_new.html.erb b/backend/app/views/spree/admin/promotions/_activations_new.html.erb index c47bf6b74e8..6eb18d510ee 100644 --- a/backend/app/views/spree/admin/promotions/_activations_new.html.erb +++ b/backend/app/views/spree/admin/promotions/_activations_new.html.erb @@ -4,7 +4,7 @@
    diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index d97b524fc93..d63fbb26f47 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -1407,6 +1407,8 @@ en: is specified, the promotion will never expire. starts_at: This determines when the promotion can be applied to orders.
    If no value is specified, the promotion will be immediately available. + promo_code_will_be_disabled: Selecting this option, promo codes will be disabled for this promotion + because all its rules / actions will be applied automatically to all orders. spree/stock_location: active: 'This determines whether stock from this location can be used when building packages.
    Default: Checked' From fd74a09154f9c48af8d68227cf89dab6b45e1d9e Mon Sep 17 00:00:00 2001 From: Massimiliano Lattanzio Date: Sat, 1 Feb 2020 17:33:13 +0100 Subject: [PATCH 4/5] Remove validation on apply-auto promos with codes This is important for backward compatibility since if we keep validating against promotions with codes as well, it will be impossible to update existing apply-auto promotions that already have a code associated. Since promotion codes are created in a second step, the creation validation have to be done in that step. --- core/app/models/spree/promotion.rb | 5 ++--- core/config/locales/en.yml | 1 - core/spec/models/spree/promotion_spec.rb | 6 ------ 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index a6004f0f3ed..18b99d21c80 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -32,7 +32,7 @@ class Promotion < Spree::Base validates :usage_limit, numericality: { greater_than: 0, allow_nil: true } validates :per_code_usage_limit, numericality: { greater_than_or_equal_to: 0, allow_nil: true } validates :description, length: { maximum: 255 } - validate :apply_automatically_disallowed_with_codes_or_paths + validate :apply_automatically_disallowed_with_paths before_save :normalize_blank_values @@ -257,10 +257,9 @@ def match_all? match_policy == "all" end - def apply_automatically_disallowed_with_codes_or_paths + def apply_automatically_disallowed_with_paths return unless apply_automatically - errors.add(:apply_automatically, :disallowed_with_code) if codes.any? errors.add(:apply_automatically, :disallowed_with_path) if path.present? end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index d63fbb26f47..0640cba2e02 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -476,7 +476,6 @@ en: spree/promotion: attributes: apply_automatically: - disallowed_with_code: Disallowed for promotions with a code disallowed_with_path: Disallowed for promotions with a path spree/promotion_code: attributes: diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 954368da0f2..db81d1607b4 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -76,12 +76,6 @@ expect(subject).to be_valid end - it "invalidates the promotion when it has a code" do - subject.codes.build(value: "foo") - expect(subject).to_not be_valid - expect(subject.errors).to include(:apply_automatically) - end - it "invalidates the promotion when it has a path" do subject.path = "foo" expect(subject).to_not be_valid From 03a3a9f705654b375c4cc2cb0c1eb4b9b1b26852 Mon Sep 17 00:00:00 2001 From: Massimiliano Lattanzio Date: Sun, 2 Feb 2020 12:35:39 +0100 Subject: [PATCH 5/5] Disable requiring promo code on apply-auto promos That requirement generate an error adding any product to cart when exist a promotion that apply automatically without any code. --- core/app/models/spree/adjustment.rb | 2 +- core/app/models/spree/order_promotion.rb | 2 +- core/spec/models/spree/adjustment_spec.rb | 17 +++++++++++++ .../spec/models/spree/order_promotion_spec.rb | 24 +++++++++++++++++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/adjustment.rb b/core/app/models/spree/adjustment.rb index 29ddc713323..88130659847 100644 --- a/core/app/models/spree/adjustment.rb +++ b/core/app/models/spree/adjustment.rb @@ -159,7 +159,7 @@ def calculate_eligibility private def require_promotion_code? - promotion? && source.promotion.codes.any? + promotion? && !source.promotion.apply_automatically && source.promotion.codes.any? end def repair_adjustments_associations_on_create diff --git a/core/app/models/spree/order_promotion.rb b/core/app/models/spree/order_promotion.rb index b2a5464c2fc..1e24d5eaeac 100644 --- a/core/app/models/spree/order_promotion.rb +++ b/core/app/models/spree/order_promotion.rb @@ -21,7 +21,7 @@ class OrderPromotion < Spree::Base private def require_promotion_code? - promotion && promotion.codes.any? + promotion && !promotion.apply_automatically && promotion.codes.any? end end end diff --git a/core/spec/models/spree/adjustment_spec.rb b/core/spec/models/spree/adjustment_spec.rb index 1fb64af735d..f6cb9857b1e 100644 --- a/core/spec/models/spree/adjustment_spec.rb +++ b/core/spec/models/spree/adjustment_spec.rb @@ -199,6 +199,23 @@ it { is_expected.to include("can't be blank") } end end + + context "when the adjustment is a promotion that apply automatically adjustment" do + let(:adjustment) { build(:adjustment, source: promotion.actions.first) } + let(:promotion) { create(:promotion, :with_order_adjustment, apply_automatically: true) } + + context "when the promotion does not have a code" do + it { is_expected.to be_blank } + end + + context "when the promotion has a code" do + let!(:promotion_code) do + promotion.codes << build(:promotion_code, promotion: promotion) + end + + it { is_expected.to be_blank } + end + end end describe 'repairing adjustment associations' do diff --git a/core/spec/models/spree/order_promotion_spec.rb b/core/spec/models/spree/order_promotion_spec.rb index 109a651ad73..c564c46474c 100644 --- a/core/spec/models/spree/order_promotion_spec.rb +++ b/core/spec/models/spree/order_promotion_spec.rb @@ -30,4 +30,28 @@ it { is_expected.to include("can't be blank") } end end + + describe "promotion code presence error on promotion that apply automatically" do + subject do + order_promotion.promotion.apply_automatically = true + order_promotion.promotion.save! + order_promotion.valid? + order_promotion.errors[:promotion_code] + end + + let(:order_promotion) { build(:order_promotion) } + let(:promotion) { order_promotion.promotion } + + context "when the promotion does not have a code" do + it { is_expected.to be_blank } + end + + context "when the promotion has a code" do + let!(:promotion_code) do + promotion.codes << build(:promotion_code, promotion: promotion) + end + + it { is_expected.to be_blank } + end + end end