From f74c9a02a639900b136f081e88d8168bccfb7ab4 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Sat, 17 Sep 2016 08:03:19 -0600
Subject: [PATCH 1/5] Refactor create_item_adjustments_spec

To use in-memory items more.
---
 .../actions/create_item_adjustments_spec.rb   | 36 +++++++++++--------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb
index cab98366e5b..6f328dd76d8 100644
--- a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb
+++ b/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
@@ -128,26 +128,32 @@ module Actions
           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

From 0be285f4bd935b4bdaf32adf61f56075508f3c84 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Sat, 17 Sep 2016 08:25:52 -0600
Subject: [PATCH 2/5] Add ability to remove promotions via
 Promotion#remove_from

---
 core/app/models/spree/promotion.rb            | 14 +++++++
 .../promotion/actions/create_adjustment.rb    | 10 +++++
 .../actions/create_item_adjustments.rb        | 13 +++++++
 .../spree/promotion/actions/free_shipping.rb  | 13 +++++++
 core/app/models/spree/promotion_action.rb     | 17 +++++++++
 .../actions/create_adjustment_spec.rb         | 21 ++++++++++
 .../actions/create_item_adjustments_spec.rb   | 18 +++++++++
 .../promotion/actions/free_shipping_spec.rb   | 25 ++++++++++--
 .../models/spree/promotion_action_spec.rb     | 38 +++++++++++++++++++
 core/spec/models/spree/promotion_spec.rb      | 19 ++++++++++
 10 files changed, 185 insertions(+), 3 deletions(-)
 create mode 100644 core/spec/models/spree/promotion_action_spec.rb

diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb
index 9be48a7bb5b..ecca11bd508 100644
--- a/core/app/models/spree/promotion.rb
+++ b/core/app/models/spree/promotion.rb
@@ -220,6 +220,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 [undefined]
+    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)
diff --git a/core/app/models/spree/promotion/actions/create_adjustment.rb b/core/app/models/spree/promotion/actions/create_adjustment.rb
index 6735ec0bb9a..20b5bec8ddd 100644
--- a/core/app/models/spree/promotion/actions/create_adjustment.rb
+++ b/core/app/models/spree/promotion/actions/create_adjustment.rb
@@ -39,6 +39,16 @@ 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.
+        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
diff --git a/core/app/models/spree/promotion/actions/create_item_adjustments.rb b/core/app/models/spree/promotion/actions/create_item_adjustments.rb
index 11236cbde39..f8134f64c02 100644
--- a/core/app/models/spree/promotion/actions/create_item_adjustments.rb
+++ b/core/app/models/spree/promotion/actions/create_item_adjustments.rb
@@ -35,6 +35,19 @@ 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.
+        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)
diff --git a/core/app/models/spree/promotion/actions/free_shipping.rb b/core/app/models/spree/promotion/actions/free_shipping.rb
index ea7938f6f82..e48a0440f9b 100644
--- a/core/app/models/spree/promotion/actions/free_shipping.rb
+++ b/core/app/models/spree/promotion/actions/free_shipping.rb
@@ -30,6 +30,19 @@ 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.
+        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)
diff --git a/core/app/models/spree/promotion_action.rb b/core/app/models/spree/promotion_action.rb
index 28198fd38b3..cb0d8eb5510 100644
--- a/core/app/models/spree/promotion_action.rb
+++ b/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 [undefined]
+    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
diff --git a/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb b/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
index a532d88f572..52e32cd9fe4 100644
--- a/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
+++ b/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
@@ -58,6 +58,27 @@
     end
   end
 
+  describe '#remove_from' do
+    let(:action) { promotion.actions.first! }
+    let(:promotion) { create(:promotion, :with_order_adjustment) }
+
+    # this adjustment should not get removed
+    let!(:other_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([other_adjustment, @action_adjustment])
+
+      action.remove_from(order)
+
+      expect(order.adjustments).to eq([other_adjustment])
+    end
+  end
+
   context "#destroy" do
     before(:each) do
       action.calculator = Spree::Calculator::FlatRate.new(preferred_amount: 10)
diff --git a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb
index 6f328dd76d8..d1e71fb9684 100644
--- a/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb
+++ b/core/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb
@@ -122,6 +122,24 @@ 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 }
diff --git a/core/spec/models/spree/promotion/actions/free_shipping_spec.rb b/core/spec/models/spree/promotion/actions/free_shipping_spec.rb
index 1f40b70ef55..73c2fc4d4f6 100644
--- a/core/spec/models/spree/promotion/actions/free_shipping_spec.rb
+++ b/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
diff --git a/core/spec/models/spree/promotion_action_spec.rb b/core/spec/models/spree/promotion_action_spec.rb
new file mode 100644
index 00000000000..2c517760dfc
--- /dev/null
+++ b/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
diff --git a/core/spec/models/spree/promotion_spec.rb b/core/spec/models/spree/promotion_spec.rb
index b156a899cde..c0c4ec9681e 100644
--- a/core/spec/models/spree/promotion_spec.rb
+++ b/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? }
 

From da8efd6157c86cf9936955e8835e950692a9365e Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Mon, 19 Sep 2016 06:16:02 -0600
Subject: [PATCH 3/5] Change yarddoc from @return [undefined] to @return [void]

---
 core/app/models/spree/promotion.rb        | 2 +-
 core/app/models/spree/promotion_action.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/core/app/models/spree/promotion.rb b/core/app/models/spree/promotion.rb
index ecca11bd508..1dceb38fbda 100644
--- a/core/app/models/spree/promotion.rb
+++ b/core/app/models/spree/promotion.rb
@@ -223,7 +223,7 @@ def used_by?(user, excluded_orders = [])
     # 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 [undefined]
+    # @return [void]
     def remove_from(order)
       actions.each do |action|
         action.remove_from(order)
diff --git a/core/app/models/spree/promotion_action.rb b/core/app/models/spree/promotion_action.rb
index cb0d8eb5510..21dd6abc004 100644
--- a/core/app/models/spree/promotion_action.rb
+++ b/core/app/models/spree/promotion_action.rb
@@ -25,7 +25,7 @@ def perform(_options = {})
     # @note This method should be overriden in subclassses.
     #
     # @param order [Spree::Order] the order to remove the action from
-    # @return [undefined]
+    # @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|

From 4ad15d5d6ace556a8b91e20b698cd965a61b0f89 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Mon, 19 Sep 2016 06:23:35 -0600
Subject: [PATCH 4/5] Add more yarddocs

---
 core/app/models/spree/promotion/actions/create_adjustment.rb     | 1 +
 .../models/spree/promotion/actions/create_item_adjustments.rb    | 1 +
 core/app/models/spree/promotion/actions/free_shipping.rb         | 1 +
 3 files changed, 3 insertions(+)

diff --git a/core/app/models/spree/promotion/actions/create_adjustment.rb b/core/app/models/spree/promotion/actions/create_adjustment.rb
index 20b5bec8ddd..c7bfcfb6107 100644
--- a/core/app/models/spree/promotion/actions/create_adjustment.rb
+++ b/core/app/models/spree/promotion/actions/create_adjustment.rb
@@ -41,6 +41,7 @@ def compute_amount(calculable)
 
         # 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
diff --git a/core/app/models/spree/promotion/actions/create_item_adjustments.rb b/core/app/models/spree/promotion/actions/create_item_adjustments.rb
index f8134f64c02..7708e9596ca 100644
--- a/core/app/models/spree/promotion/actions/create_item_adjustments.rb
+++ b/core/app/models/spree/promotion/actions/create_item_adjustments.rb
@@ -38,6 +38,7 @@ def compute_amount(adjustable)
         # 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|
diff --git a/core/app/models/spree/promotion/actions/free_shipping.rb b/core/app/models/spree/promotion/actions/free_shipping.rb
index e48a0440f9b..b67ad5edea9 100644
--- a/core/app/models/spree/promotion/actions/free_shipping.rb
+++ b/core/app/models/spree/promotion/actions/free_shipping.rb
@@ -33,6 +33,7 @@ def compute_amount(shipment)
         # 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|

From 8f8a2f0f59290b41728fd2c8c7257c6b5f416b54 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Mon, 19 Sep 2016 06:41:00 -0600
Subject: [PATCH 5/5] Rename variable in spec

Make it more readable.
---
 .../spree/promotion/actions/create_adjustment_spec.rb      | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb b/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
index 52e32cd9fe4..37227c1eb51 100644
--- a/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
+++ b/core/spec/models/spree/promotion/actions/create_adjustment_spec.rb
@@ -62,8 +62,7 @@
     let(:action) { promotion.actions.first! }
     let(:promotion) { create(:promotion, :with_order_adjustment) }
 
-    # this adjustment should not get removed
-    let!(:other_adjustment) { create(:adjustment, order: order, source: nil) }
+    let!(:unrelated_adjustment) { create(:adjustment, order: order, source: nil) }
 
     before do
       action.perform(payload)
@@ -71,11 +70,11 @@
     end
 
     it 'removes the action adjustment' do
-      expect(order.adjustments).to match_array([other_adjustment, @action_adjustment])
+      expect(order.adjustments).to match_array([unrelated_adjustment, @action_adjustment])
 
       action.remove_from(order)
 
-      expect(order.adjustments).to eq([other_adjustment])
+      expect(order.adjustments).to eq([unrelated_adjustment])
     end
   end