Merge pull request #1451 from jordan-brough/add-promotion-remove-from

Also: Add a CHANGELOG entry

CHANGELOG.md

@@ -1,5 +1,14 @@
 ## Solidus 2.1.0 (master, unreleased)
 
+*   Add `Spree::Promotion#remove_from` and `Spree::PromotionAction#remove_from`
+
+    This will allow promotions to be removed from orders and allows promotion
+    actions to define how to reverse their side effects on an order.
+
+    For now `PromotionAction` provides a default remove_from method, with a
+    deprecation warning that subclasses should define their own remove_from
+    method.
+
 *   Remove `is_default` boolean from `Spree::Price` model
 
     This boolean used to mean "the price to be used". With the new

core/app/models/spree/promotion.rb

@@ -211,6 +211,20 @@ def used_by?(user, excluded_orders = [])
       end
     end
 
+    # Removes a promotion and any adjustments or other side effects from an
+    # order.
+    # @param order [Spree::Order] the order to remove the promotion from.
+    # @return [void]
+    def remove_from(order)
+      actions.each do |action|
+        action.remove_from(order)
+      end
+      # note: this destroys the join table entry, not the promotion itself
+      order.promotions.destroy(self)
+      order.order_promotions.reset
+      order_promotions.reset
+    end
+
     private
 
     def blacklisted?(promotable)

core/app/models/spree/promotion/actions/create_adjustment.rb

@@ -39,6 +39,17 @@ def compute_amount(calculable)
           [(calculable.item_total + calculable.ship_total), amount].min * -1
         end
 
+        # Removes any adjustments generated by this action from the order.
+        # @param order [Spree::Order] the order to remove the action from.
+        # @return [void]
+        def remove_from(order)
+          order.adjustments.each do |adjustment|
+            if adjustment.source == self
+              order.adjustments.destroy(adjustment)
+            end
+          end
+        end
+
         private
 
         # Tells us if there if the specified promotion is already associated with the line item

core/app/models/spree/promotion/actions/create_item_adjustments.rb

@@ -35,6 +35,20 @@ def compute_amount(adjustable)
           [adjustable.amount, promotion_amount].min * -1
         end
 
+        # Removes any adjustments generated by this action from the order's
+        #  line items.
+        # @param order [Spree::Order] the order to remove the action from.
+        # @return [void]
+        def remove_from(order)
+          order.line_items.each do |line_item|
+            line_item.adjustments.each do |adjustment|
+              if adjustment.source == self
+                line_item.adjustments.destroy(adjustment)
+              end
+            end
+          end
+        end
+
         private
 
         def create_adjustment(adjustable, order, promotion_code)

core/app/models/spree/promotion/actions/free_shipping.rb

@@ -30,6 +30,20 @@ def compute_amount(shipment)
           shipment.cost * -1
         end
 
+        # Removes any adjustments generated by this action from the order's
+        #  shipments.
+        # @param order [Spree::Order] the order to remove the action from.
+        # @return [void]
+        def remove_from(order)
+          order.shipments.each do |shipment|
+            shipment.adjustments.each do |adjustment|
+              if adjustment.source == self
+                shipment.adjustments.destroy(adjustment)
+              end
+            end
+          end
+        end
+
         private
 
         def promotion_credit_exists?(shipment)

core/app/models/spree/promotion_action.rb

@@ -19,5 +19,22 @@ class PromotionAction < Spree::Base
     def perform(_options = {})
       raise 'perform should be implemented in a sub-class of PromotionAction'
     end
+
+    # Removes the action from an order
+    #
+    # @note This method should be overriden in subclassses.
+    #
+    # @param order [Spree::Order] the order to remove the action from
+    # @return [void]
+    def remove_from(order)
+      Spree::Deprecation.warn("#{self.class.name.inspect} does not define #remove_from. The default behavior may be incorrect and will be removed in a future version of Solidus.", caller)
+      [order, *order.line_items, *order.shipments].each do |item|
+        item.adjustments.each do |adjustment|
+          if adjustment.source == self
+            item.adjustments.destroy(adjustment)
+          end
+        end
+      end
+    end
   end
 end

core/spec/models/spree/promotion/actions/create_adjustment_spec.rb

@@ -58,6 +58,26 @@
     end
   end
 
+  describe '#remove_from' do
+    let(:action) { promotion.actions.first! }
+    let(:promotion) { create(:promotion, :with_order_adjustment) }
+
+    let!(:unrelated_adjustment) { create(:adjustment, order: order, source: nil) }
+
+    before do
+      action.perform(payload)
+      @action_adjustment = order.adjustments.where(source: action).first!
+    end
+
+    it 'removes the action adjustment' do
+      expect(order.adjustments).to match_array([unrelated_adjustment, @action_adjustment])
+
+      action.remove_from(order)
+
+      expect(order.adjustments).to eq([unrelated_adjustment])
+    end
+  end
+
   context "#destroy" do
     before(:each) do
       action.calculator = Spree::Calculator::FlatRate.new(preferred_amount: 10)

core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb

@@ -4,11 +4,11 @@ module Spree
   class Promotion
     module Actions
       describe CreateItemAdjustments, type: :model do
-        let(:order) { create(:order) }
+        let(:order) { create(:order_with_line_items, line_items_count: 1) }
         let(:promotion) { create(:promotion, :with_line_item_adjustment, adjustment_rate: adjustment_amount) }
         let(:adjustment_amount) { 10 }
         let(:action) { promotion.actions.first! }
-        let!(:line_item) { create(:line_item, order: order) }
+        let(:line_item) { order.line_items.to_a.first }
         let(:payload) { { order: order, promotion: promotion } }
 
         before do
@@ -122,32 +122,56 @@ module Actions
           end
         end
 
+        describe '#remove_from' do
+          # this adjustment should not get removed
+          let!(:other_adjustment) { create(:adjustment, adjustable: line_item, order: order, source: nil) }
+
+          before do
+            action.perform(payload)
+            @action_adjustment = line_item.adjustments.where(source: action).first!
+          end
+
+          it 'removes the action adjustment' do
+            expect(line_item.adjustments).to match_array([other_adjustment, @action_adjustment])
+
+            action.remove_from(order)
+
+            expect(line_item.adjustments).to eq([other_adjustment])
+          end
+        end
+
         context "#destroy" do
           let!(:action) { promotion.actions.first }
           let(:other_action) { other_promotion.actions.first }
           let(:promotion) { create(:promotion, :with_line_item_adjustment) }
           let(:other_promotion) { create(:promotion, :with_line_item_adjustment) }
 
-          it "destroys adjustments for incompleted orders" do
-            order = Order.create
-            action.adjustments.create!(label: "Check", amount: 0, order: order, adjustable: order)
+          context 'with incomplete orders' do
+            let(:order) { create(:order) }
 
-            expect {
-              action.destroy
-            }.to change { Adjustment.count }.by(-1)
+            it 'destroys adjustments' do
+              order.adjustments.create!(label: 'Check', amount: 0, order: order, source: action)
+
+              expect {
+                action.destroy
+              }.to change { Adjustment.count }.by(-1)
+            end
           end
 
-          it "nullifies adjustments for completed orders" do
-            order = Order.create(completed_at: Time.current)
-            adjustment = action.adjustments.create!(label: "Check", amount: 0, order: order, adjustable: order)
+          context 'with complete orders' do
+            let(:order) { create(:completed_order_with_totals) }
 
-            expect {
-              action.destroy
-            }.to change { adjustment.reload.source_id }.from(action.id).to nil
+            it 'nullifies adjustments for completed orders' do
+              adjustment = order.adjustments.create!(label: 'Check', amount: 0, order: order, source: action)
+
+              expect {
+                action.destroy
+              }.to change { adjustment.reload.source_id }.from(action.id).to nil
+            end
           end
 
           it "doesnt mess with unrelated adjustments" do
-            other_action.adjustments.create!(label: "Check", amount: 0, order: order, adjustable: order)
+            order.adjustments.create!(label: "Check", amount: 0, order: order, source: action)
 
             expect {
               action.destroy

core/spec/models/spree/promotion/actions/free_shipping_spec.rb

@@ -2,10 +2,11 @@
 
 describe Spree::Promotion::Actions::FreeShipping, type: :model do
   let(:order) { create(:completed_order_with_totals) }
-  let(:promotion_code) { create(:promotion_code, value: 'somecode') }
-  let(:promotion) { promotion_code.promotion }
-  let(:action) { Spree::Promotion::Actions::FreeShipping.create }
+  let(:shipment) { order.shipments.to_a.first }
+  let(:promotion) { create(:promotion, code: 'somecode', promotion_actions: [action]) }
+  let(:action) { Spree::Promotion::Actions::FreeShipping.new }
   let(:payload) { { order: order, promotion_code: promotion_code } }
+  let(:promotion_code) { promotion.codes.first! }
 
   # From promotion spec:
   context "#perform" do
@@ -37,4 +38,22 @@
       end
     end
   end
+
+  describe '#remove_from' do
+    # this adjustment should not get removed
+    let!(:other_adjustment) { create(:adjustment, adjustable: shipment, order: order, source: nil) }
+
+    before do
+      action.perform(payload)
+      @action_adjustment = shipment.adjustments.where(source: action).first!
+    end
+
+    it 'removes the action adjustment' do
+      expect(shipment.adjustments).to match_array([other_adjustment, @action_adjustment])
+
+      action.remove_from(order)
+
+      expect(shipment.adjustments).to eq([other_adjustment])
+    end
+  end
 end

core/spec/models/spree/promotion_action_spec.rb

@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe Spree::PromotionAction, type: :model do
+  describe '#remove_from' do
+    class MyPromotionAction < Spree::PromotionAction
+      def perform(options = {})
+        order = options[:order]
+        order.adjustments.create!(amount: 1, order: order, source: self, label: 'foo')
+        true
+      end
+    end
+
+    let(:action) { promotion.actions.first! }
+    let!(:promotion) { create(:promotion, promotion_actions: [MyPromotionAction.new]) }
+    let(:order) { create(:order) }
+
+    # this adjustment should not get removed
+    let!(:other_adjustment) { create(:adjustment, order: order, source: nil) }
+
+    before do
+      action.perform(order: order)
+      @action_adjustment = order.adjustments.where(source: action).first!
+    end
+
+    it 'removes the action adjustment' do
+      expect(order.adjustments).to match_array([other_adjustment, @action_adjustment])
+
+      expect(Spree::Deprecation).to(
+        receive(:warn).
+        with(/"MyPromotionAction" does not define #remove_from/, anything)
+      )
+
+      action.remove_from(order)
+
+      expect(order.adjustments).to eq([other_adjustment])
+    end
+  end
+end

core/spec/models/spree/promotion_spec.rb

@@ -176,6 +176,25 @@
     end
   end
 
+  describe '#remove_from' do
+    let(:promotion) { create(:promotion, :with_line_item_adjustment) }
+    let(:order) { create(:order_with_line_items) }
+
+    before do
+      promotion.activate(order: order)
+    end
+
+    it 'removes the promotion' do
+      expect(order.promotions).to include(promotion)
+      expect(order.line_items.flat_map(&:adjustments)).to be_present
+
+      promotion.remove_from(order)
+
+      expect(order.promotions).to be_empty
+      expect(order.line_items.flat_map(&:adjustments)).to be_empty
+    end
+  end
+
   describe "#usage_limit_exceeded?" do
     subject { promotion.usage_limit_exceeded? }