From 4b99d28e4897827a8343debcefc183f5927ed81f Mon Sep 17 00:00:00 2001
From: Patrick McMorran <pat.mcmorran@fcpeuro.com>
Date: Sat, 6 Jun 2020 22:38:36 -0400
Subject: [PATCH 1/6] added logic to indicate a the status of a promotion as
 inactive when no actions are assigned

---
 core/app/models/spree/promotion.rb              | 17 ++++++++++++++---
 .../models/spree/promotion_handler/coupon.rb    |  4 ++--
 .../templates/config/initializers/spree.rb.tt   |  5 +++++
 core/lib/spree/app_configuration.rb             |  4 ++++
 core/lib/spree/core/engine.rb                   |  9 ++++++++-
 core/spec/models/spree/product_spec.rb          |  3 +++
 core/spec/models/spree/promotion_spec.rb        |  4 ++++
 7 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb
index 574dd01bc8f..557679a9c17 100644
--- a/core/app/models/spree/promotion.rb
+++ b/core/app/models/spree/promotion.rb
@@ -39,10 +39,21 @@ class Promotion < Spree::Base
     scope :coupons, -> { joins(:codes).distinct }
     scope :advertised, -> { where(advertise: true) }
     scope :active, -> do
+      if Spree::Config.actionless_promotion_inactive
+        has_actions.started_and_unexpired
+      else
+        started_and_unexpired
+      end
+    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)))
+      joins(:promotion_actions).
+          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 }
 
@@ -84,7 +95,7 @@ def not_expired?
     end
 
     def active?
-      started? && not_expired?
+      started? && not_expired? && (Spree::Config.actionless_promotion_inactive ? actions.present? : true)
     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..b358f85f70c 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 `false` to allow promotions
+  # with no associated actions to be active for use by customers.
+  # See https://github.com/solidusio/solidus/issues/2777 for more info.
+  config.actionless_promotion_inactive = true
+
   # 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..e67ae96e154 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 activation (default: +true+)
+    preference :actionless_promotion_inactive, :boolean, default: false
+
     # @!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..5e95afd65b0 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.actionless_promotion_inactive == true
+          Spree::Deprecation.warn(
+            'Spree::Config.actionless_promotion_inactive set to false is ' \
+            'deprecated. Please note that by switching this value, ' \
+            'promotions with no actions will be 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/spec/models/spree/product_spec.rb b/core/spec/models/spree/product_spec.rb
index 69846e7a081..881edc5eca3 100644
--- a/core/spec/models/spree/product_spec.rb
+++ b/core/spec/models/spree/product_spec.rb
@@ -470,6 +470,9 @@ class Extension < Spree::Base
           products: [product]
         )
       end
+      before do
+        promotion.actions << Spree::Promotion::Actions::CreateAdjustment.new
+      end
 
       it "lists the promotion as a possible promotion" do
         expect(product.possible_promotions).to include(promotion)
diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb
index 6fa8dcbf94e..22e0945f6b2 100644
--- a/core/spec/models/spree/promotion_spec.rb
+++ b/core/spec/models/spree/promotion_spec.rb
@@ -347,6 +347,10 @@
   end
 
   context "#inactive" do
+    before do
+      promotion.actions << Spree::Promotion::Actions::CreateAdjustment.new
+    end
+
     it "should not be exipired" do
       expect(promotion).not_to be_inactive
     end

From 6d634fd784f8a2c031140d2b0c1e67a6f917bd3d Mon Sep 17 00:00:00 2001
From: Daniele Palombo <daniele.palombo@gmail.com>
Date: Fri, 2 Oct 2020 09:39:56 +0200
Subject: [PATCH 2/6] Make pref to consider actionless active explicit

---
 core/app/models/spree/promotion.rb                        | 8 ++++----
 .../install/templates/config/initializers/spree.rb.tt     | 8 ++++----
 core/lib/spree/app_configuration.rb                       | 4 ++--
 core/lib/spree/core/engine.rb                             | 6 +++---
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb
index 557679a9c17..32875660327 100644
--- a/core/app/models/spree/promotion.rb
+++ b/core/app/models/spree/promotion.rb
@@ -39,10 +39,10 @@ class Promotion < Spree::Base
     scope :coupons, -> { joins(:codes).distinct }
     scope :advertised, -> { where(advertise: true) }
     scope :active, -> do
-      if Spree::Config.actionless_promotion_inactive
-        has_actions.started_and_unexpired
-      else
+      if Spree::Config.consider_actionless_promotion_active
         started_and_unexpired
+      else
+        has_actions.started_and_unexpired
       end
     end
     scope :started_and_unexpired, -> do
@@ -95,7 +95,7 @@ def not_expired?
     end
 
     def active?
-      started? && not_expired? && (Spree::Config.actionless_promotion_inactive ? actions.present? : true)
+      started? && not_expired? && (Spree::Config.consider_actionless_promotion_active || actions.present?)
     end
 
     def inactive?
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 b358f85f70c..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,10 +45,10 @@ Spree.config do |config|
   # their previous location first.
   config.redirect_back_on_unauthorized = true
 
-  # Set this configuration to `false` to allow promotions
-  # with no associated actions to be active for use by customers.
-  # See https://github.com/solidusio/solidus/issues/2777 for more info.
-  config.actionless_promotion_inactive = 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
diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb
index e67ae96e154..3db13eab392 100644
--- a/core/lib/spree/app_configuration.rb
+++ b/core/lib/spree/app_configuration.rb
@@ -218,8 +218,8 @@ class AppConfiguration < Preferences::Configuration
     preference :promotions_per_page, :integer, default: 15
 
     # @!attribute [rw] disable_actionless_promotion_validation
-    #   @return [Boolean] Promotions should have actions associated before activation (default: +true+)
-    preference :actionless_promotion_inactive, :boolean, default: false
+    #   @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
diff --git a/core/lib/spree/core/engine.rb b/core/lib/spree/core/engine.rb
index 5e95afd65b0..76c142b5b66 100644
--- a/core/lib/spree/core/engine.rb
+++ b/core/lib/spree/core/engine.rb
@@ -65,11 +65,11 @@ class Engine < ::Rails::Engine
             caller
           )
         end
-        if Spree::Config.actionless_promotion_inactive == true
+        if Spree::Config.consider_actionless_promotion_active != true
           Spree::Deprecation.warn(
-            'Spree::Config.actionless_promotion_inactive set to false is ' \
+            'Spree::Config.consider_actionless_promotion_active set to true is ' \
             'deprecated. Please note that by switching this value, ' \
-            'promotions with no actions will be active.',
+            'promotions with no actions will be considered active.',
             caller
           )
         end

From 5a969acbc4e5b1d4db56558b05608d2f0e611f24 Mon Sep 17 00:00:00 2001
From: Daniele Palombo <daniele.palombo@gmail.com>
Date: Fri, 2 Oct 2020 09:45:43 +0200
Subject: [PATCH 3/6] Show warning for deprecated value

---
 core/lib/spree/core/engine.rb               | 2 +-
 core/lib/spree/testing_support/dummy_app.rb | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/core/lib/spree/core/engine.rb b/core/lib/spree/core/engine.rb
index 76c142b5b66..4802f66e672 100644
--- a/core/lib/spree/core/engine.rb
+++ b/core/lib/spree/core/engine.rb
@@ -65,7 +65,7 @@ class Engine < ::Rails::Engine
             caller
           )
         end
-        if Spree::Config.consider_actionless_promotion_active != true
+        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, ' \
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'

From b42f552865d0a1cbf7ed72118bd90e63a6a2bb34 Mon Sep 17 00:00:00 2001
From: Daniele Palombo <daniele.palombo@gmail.com>
Date: Fri, 2 Oct 2020 09:47:16 +0200
Subject: [PATCH 4/6] Remove joins(:promotion_actions)

Remove the join table otherwise started_and_unexpired scope will return
only the promotion with actions, and the code doesn't reproduce the
deprecated behavior
---
 core/app/models/spree/promotion.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb
index 32875660327..bea7af0bbf8 100644
--- a/core/app/models/spree/promotion.rb
+++ b/core/app/models/spree/promotion.rb
@@ -48,9 +48,9 @@ class Promotion < Spree::Base
     scope :started_and_unexpired, -> do
       table = arel_table
       time = Time.current
-      joins(:promotion_actions).
-          where(table[:starts_at].eq(nil).or(table[:starts_at].lt(time))).
-          where(table[:expires_at].eq(nil).or(table[:expires_at].gt(time)))
+
+      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)

From 306d402c5b3d5611b308c756f4bf1160afd0c9df Mon Sep 17 00:00:00 2001
From: Daniele Palombo <daniele.palombo@gmail.com>
Date: Fri, 2 Oct 2020 09:48:21 +0200
Subject: [PATCH 5/6] Add with_action trait

---
 .../controllers/spree/admin/promotions_controller_spec.rb | 8 +++++---
 .../spree/testing_support/factories/promotion_factory.rb  | 6 ++++++
 core/spec/models/spree/product_spec.rb                    | 7 +------
 3 files changed, 12 insertions(+), 9 deletions(-)

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/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 881edc5eca3..1badf707c57 100644
--- a/core/spec/models/spree/product_spec.rb
+++ b/core/spec/models/spree/product_spec.rb
@@ -461,18 +461,13 @@ 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,
           products: [product]
         )
       end
-      before do
-        promotion.actions << Spree::Promotion::Actions::CreateAdjustment.new
-      end
 
       it "lists the promotion as a possible promotion" do
         expect(product.possible_promotions).to include(promotion)

From 109c1bb96a5189b26dfa142193227a44f9a81b4c Mon Sep 17 00:00:00 2001
From: Daniele Palombo <daniele.palombo@gmail.com>
Date: Fri, 2 Oct 2020 09:49:47 +0200
Subject: [PATCH 6/6] Refactor Spree::Promotion#active method

---
 .../admin/promotions/promotion_spec.rb        |   2 +-
 core/app/models/spree/promotion.rb            |   8 +-
 core/spec/models/spree/promotion_spec.rb      | 132 ++++++++++++++----
 3 files changed, 110 insertions(+), 32 deletions(-)

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 bea7af0bbf8..75c5f476fdc 100644
--- a/core/app/models/spree/promotion.rb
+++ b/core/app/models/spree/promotion.rb
@@ -39,11 +39,9 @@ class Promotion < Spree::Base
     scope :coupons, -> { joins(:codes).distinct }
     scope :advertised, -> { where(advertise: true) }
     scope :active, -> do
-      if Spree::Config.consider_actionless_promotion_active
-        started_and_unexpired
-      else
-        has_actions.started_and_unexpired
-      end
+      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
diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb
index 22e0945f6b2..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,9 +376,7 @@
   end
 
   context "#inactive" do
-    before do
-      promotion.actions << Spree::Promotion::Actions::CreateAdjustment.new
-    end
+    let(:promotion) { create(:promotion, :with_action) }
 
     it "should not be exipired" do
       expect(promotion).not_to be_inactive
@@ -463,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
 
@@ -783,6 +862,7 @@
 
     before do
       promotion.promotion_rules = rules
+      promotion.promotion_actions = [Spree::PromotionAction.new]
       allow(promotion.rules).to receive(:for) { rules }
     end