diff --git a/backend/spec/controllers/spree/admin/promotions_controller_spec.rb b/backend/spec/controllers/spree/admin/promotions_controller_spec.rb index 59825eabd6a..a381288bd76 100644 --- a/backend/spec/controllers/spree/admin/promotions_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/promotions_controller_spec.rb @@ -5,9 +5,11 @@ describe Spree::Admin::PromotionsController, type: :controller do stub_authorization! - let!(:promotion1) { create(:promotion, name: "name1", code: "code1", path: "path1") } - let!(:promotion2) { create(:promotion, name: "name2", code: "code2", path: "path2") } - let!(:promotion3) { create(:promotion, name: "name2", code: "code3", path: "path3", expires_at: Date.yesterday) } + let!(:promotion1) { create(:promotion, :with_action, name: "name1", code: "code1", path: "path1") } + let!(:promotion2) { create(:promotion, :with_action, name: "name2", code: "code2", path: "path2") } + let!(:promotion3) do + create(:promotion, :with_action, name: "name2", code: "code3", path: "path3", expires_at: Date.yesterday) + end let!(:category) { create :promotion_category } describe "#show" do diff --git a/backend/spec/features/admin/promotions/promotion_spec.rb b/backend/spec/features/admin/promotions/promotion_spec.rb index 95fefe3eabb..15ad98fba8d 100644 --- a/backend/spec/features/admin/promotions/promotion_spec.rb +++ b/backend/spec/features/admin/promotions/promotion_spec.rb @@ -14,7 +14,7 @@ end context 'when promotion is active' do - given!(:promotion) { create :promotion } + given!(:promotion) { create :promotion, :with_action } scenario 'promotion status is active' do visit spree.admin_promotions_path diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb index 574dd01bc8f..75c5f476fdc 100644 --- a/core/app/models/spree/promotion.rb +++ b/core/app/models/spree/promotion.rb @@ -39,11 +39,20 @@ class Promotion < Spree::Base scope :coupons, -> { joins(:codes).distinct } scope :advertised, -> { where(advertise: true) } scope :active, -> do + return started_and_unexpired if Spree::Config.consider_actionless_promotion_active == true + + has_actions.started_and_unexpired + end + scope :started_and_unexpired, -> do table = arel_table time = Time.current + where(table[:starts_at].eq(nil).or(table[:starts_at].lt(time))). where(table[:expires_at].eq(nil).or(table[:expires_at].gt(time))) end + scope :has_actions, -> do + joins(:promotion_actions) + end scope :applied, -> { joins(:order_promotions).distinct } self.whitelisted_ransackable_associations = ['codes'] @@ -84,7 +93,7 @@ def not_expired? end def active? - started? && not_expired? + started? && not_expired? && (Spree::Config.consider_actionless_promotion_active || actions.present?) end def inactive? diff --git a/core/app/models/spree/promotion_handler/coupon.rb b/core/app/models/spree/promotion_handler/coupon.rb index 5993d7e744f..9e0e4673404 100644 --- a/core/app/models/spree/promotion_handler/coupon.rb +++ b/core/app/models/spree/promotion_handler/coupon.rb @@ -13,9 +13,9 @@ def initialize(order) def apply if coupon_code.present? - if promotion.present? && promotion.actions.exists? + if promotion.present? && promotion.active? && promotion.actions.exists? handle_present_promotion(promotion) - elsif promotion_code && promotion_code.promotion.inactive? + elsif promotion_code&.promotion&.expired? set_error_code :coupon_code_expired else set_error_code :coupon_code_not_found diff --git a/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt b/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt index ca2d1f792f1..cfdd21e07e5 100644 --- a/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt +++ b/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt @@ -45,6 +45,11 @@ Spree.config do |config| # their previous location first. config.redirect_back_on_unauthorized = true + # Set this configuration to `true` to allow promotions + # with no associated actions to be considered active for use by customers. + # See https://github.com/solidusio/solidus/pull/3749 for more info. + config.consider_actionless_promotion_active = false + # Set this configuration to `false` to avoid running validations when # updating an order. Be careful since you can end up having inconsistent # data in your database turning it on. diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 63ddfc47706..3db13eab392 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -217,6 +217,10 @@ class AppConfiguration < Preferences::Configuration # @return [Integer] Promotions to show per-page in the admin (default: +15+) preference :promotions_per_page, :integer, default: 15 + # @!attribute [rw] disable_actionless_promotion_validation + # @return [Boolean] Promotions should have actions associated before being considered active (default: +true+) + preference :consider_actionless_promotion_active, :boolean, default: true + # @!attribute [rw] raise_with_invalid_currency # Whether to raise an exception if trying to set a line item currency # different from the order currency. When false a validation error diff --git a/core/lib/spree/core/engine.rb b/core/lib/spree/core/engine.rb index c6e77e9a79b..4802f66e672 100644 --- a/core/lib/spree/core/engine.rb +++ b/core/lib/spree/core/engine.rb @@ -65,7 +65,14 @@ class Engine < ::Rails::Engine caller ) end - + if Spree::Config.consider_actionless_promotion_active == true + Spree::Deprecation.warn( + 'Spree::Config.consider_actionless_promotion_active set to true is ' \ + 'deprecated. Please note that by switching this value, ' \ + 'promotions with no actions will be considered active.', + caller + ) + end if Spree::Config.run_order_validations_on_order_updater != true Spree::Deprecation.warn( 'Spree::Config.run_order_validations_on_order_updater set to false is ' \ diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb index 2e315647c43..b304203639f 100644 --- a/core/lib/spree/testing_support/dummy_app.rb +++ b/core/lib/spree/testing_support/dummy_app.rb @@ -121,6 +121,7 @@ class Application < ::Rails::Application config.use_combined_first_and_last_name_in_address = true config.use_legacy_order_state_machine = false config.use_custom_cancancan_actions = false + config.consider_actionless_promotion_active = false if ENV['ENABLE_ACTIVE_STORAGE'] config.image_attachment_module = 'Spree::Image::ActiveStorageAttachment' diff --git a/core/lib/spree/testing_support/factories/promotion_factory.rb b/core/lib/spree/testing_support/factories/promotion_factory.rb index 31b7d9e837b..fe76fff67bb 100644 --- a/core/lib/spree/testing_support/factories/promotion_factory.rb +++ b/core/lib/spree/testing_support/factories/promotion_factory.rb @@ -16,6 +16,12 @@ end end + trait :with_action do + after(:create) do |promotion, _evaluator| + promotion.actions << Spree::Promotion::Actions::CreateAdjustment.new + end + end + trait :with_line_item_adjustment do transient do adjustment_rate { 10 } diff --git a/core/spec/models/spree/product_spec.rb b/core/spec/models/spree/product_spec.rb index 69846e7a081..1badf707c57 100644 --- a/core/spec/models/spree/product_spec.rb +++ b/core/spec/models/spree/product_spec.rb @@ -461,9 +461,7 @@ class Extension < Spree::Base # Regression test for https://github.com/spree/spree/issues/4416 context "#possible_promotions" do - let!(:promotion) do - create(:promotion, advertise: true, starts_at: 1.day.ago) - end + let!(:promotion) { create(:promotion, :with_action, advertise: true, starts_at: 1.day.ago) } let!(:rule) do Spree::Promotion::Rules::Product.create( promotion: promotion, diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb index 6fa8dcbf94e..6504d35e4a9 100644 --- a/core/spec/models/spree/promotion_spec.rb +++ b/core/spec/models/spree/promotion_spec.rb @@ -62,6 +62,36 @@ end end + describe '.active' do + subject { described_class.active } + + let(:promotion) { create(:promotion, starts_at: Date.yesterday, name: "name1") } + + before { promotion } + + it "doesn't return promotion without actions" do + expect(subject).to be_empty + end + + context 'when promotion has an action' do + let(:promotion) { create(:promotion, :with_action, starts_at: Date.yesterday, name: "name1") } + + it 'returns promotion with action' do + expect(subject).to match [promotion] + end + end + + context 'with consider_actionless_promotion_active true' do + before do + stub_spree_preferences(consider_actionless_promotion_active: true) + end + + it "returns promotions without actions" do + expect(subject).to match [promotion] + end + end + end + describe "#apply_automatically" do subject { build(:promotion) } @@ -85,10 +115,9 @@ end describe "#save" do - let(:promotion) { Spree::Promotion.create(name: "delete me") } + let(:promotion) { create(:promotion, :with_action, name: 'delete me') } before(:each) do - promotion.actions << Spree::Promotion::Actions::CreateAdjustment.new promotion.rules << Spree::Promotion::Rules::FirstOrder.new promotion.save! end @@ -347,6 +376,8 @@ end context "#inactive" do + let(:promotion) { create(:promotion, :with_action) } + it "should not be exipired" do expect(promotion).not_to be_inactive end @@ -459,40 +490,92 @@ end context "#active" do - it "should be active" do - expect(promotion.active?).to eq(true) - end - - it "should not be active if it hasn't started yet" do - promotion.starts_at = Time.current + 1.day + it "shouldn't be active if it has started already" do + promotion.starts_at = Time.current - 1.day expect(promotion.active?).to eq(false) end - it "should not be active if it has already ended" do - promotion.expires_at = Time.current - 1.day + it "shouldn't be active if it has not ended yet" do + promotion.expires_at = Time.current + 1.day expect(promotion.active?).to eq(false) end - it "should be active if it has started already" do + it "shouldn't be active if current time is within starts_at and expires_at range" do promotion.starts_at = Time.current - 1.day - expect(promotion.active?).to eq(true) + promotion.expires_at = Time.current + 1.day + expect(promotion.active?).to eq(false) end - it "should be active if it has not ended yet" do - promotion.expires_at = Time.current + 1.day - expect(promotion.active?).to eq(true) + it "shouldn't be active if there are no start and end times set" do + promotion.starts_at = nil + promotion.expires_at = nil + expect(promotion.active?).to eq(false) end - it "should be active if current time is within starts_at and expires_at range" do - promotion.starts_at = Time.current - 1.day - promotion.expires_at = Time.current + 1.day - expect(promotion.active?).to eq(true) + context 'when promotion has an action' do + let(:promotion) { create(:promotion, :with_action, name: "name1") } + + it "should be active if it has started already" do + promotion.starts_at = Time.current - 1.day + expect(promotion.active?).to eq(true) + end + + it "should be active if it has not ended yet" do + promotion.expires_at = Time.current + 1.day + expect(promotion.active?).to eq(true) + end + + it "should be active if current time is within starts_at and expires_at range" do + promotion.starts_at = Time.current - 1.day + promotion.expires_at = Time.current + 1.day + expect(promotion.active?).to eq(true) + end + + it "should be active if there are no start and end times set" do + promotion.starts_at = nil + promotion.expires_at = nil + expect(promotion.active?).to eq(true) + end end - it "should be active if there are no start and end times set" do - promotion.starts_at = nil - promotion.expires_at = nil - expect(promotion.active?).to eq(true) + context 'with consider_actionless_promotion_active true' do + before { stub_spree_preferences(consider_actionless_promotion_active: true) } + + it "should be active" do + expect(promotion.active?).to eq(true) + end + + it "should not be active if it hasn't started yet" do + promotion.starts_at = Time.current + 1.day + expect(promotion.active?).to eq(false) + end + + it "should not be active if it has already ended" do + promotion.expires_at = Time.current - 1.day + expect(promotion.active?).to eq(false) + end + + it "should be active if it has started already" do + promotion.starts_at = Time.current - 1.day + expect(promotion.active?).to eq(true) + end + + it "should be active if it has not ended yet" do + promotion.expires_at = Time.current + 1.day + expect(promotion.active?).to eq(true) + end + + it "should be active if current time is within starts_at and expires_at range" do + promotion.starts_at = Time.current - 1.day + promotion.expires_at = Time.current + 1.day + expect(promotion.active?).to eq(true) + end + + it "should be active if there are no start and end times set" do + promotion.starts_at = nil + promotion.expires_at = nil + expect(promotion.active?).to eq(true) + end end end @@ -779,6 +862,7 @@ before do promotion.promotion_rules = rules + promotion.promotion_actions = [Spree::PromotionAction.new] allow(promotion.rules).to receive(:for) { rules } end