Do not consider promotions without actions as active

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

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

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?

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

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.

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

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 ' \

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'

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 }

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,

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