Merge pull request #7167 from Matt-Yorkley/adjustments-returns

[Adjustments] Update return adjustments

app/models/spree/adjustment.rb

@@ -66,7 +66,7 @@ module Spree
     scope :optional, -> { where(mandatory: false) }
     scope :charge, -> { where('amount >= 0') }
     scope :credit, -> { where('amount < 0') }
-    scope :return_authorization, -> { where(source_type: "Spree::ReturnAuthorization") }
+    scope :return_authorization, -> { where(originator_type: "Spree::ReturnAuthorization") }
     scope :inclusive, -> { where(included: true) }
     scope :additional, -> { where(included: false) }
 
@@ -100,12 +100,14 @@ module Spree
     # adjustment calculations would not performed on proper values
     def update!(calculable = nil, force: false)
       return if immutable? && !force
+      return if originator.blank?
 
-      # Fix for Spree issue #3381
-      # If we attempt to call 'source' before the reload, then source is currently
-      # the order object. After calling a reload, the source is the Shipment.
-      reload
-      originator.update_adjustment(self, calculable || source) if originator.present?
+      amount = originator.compute_amount(calculable || adjustable)
+
+      update_columns(
+        amount: amount,
+        updated_at: Time.zone.now,
+      )
     end
 
     def currency

app/models/spree/return_authorization.rb

@@ -64,6 +64,11 @@ module Spree
       order.shipped_shipments.collect{ |s| s.inventory_units.to_a }.flatten
     end
 
+    # Used when Adjustment#update! wants to update the related adjustment
+    def compute_amount(*_args)
+      -amount.abs
+    end
+
     private
 
     def must_have_shipped_units
@@ -89,10 +94,13 @@ module Spree
         Spree::StockMovement.create!(stock_item_id: iu.find_stock_item.id, quantity: 1)
       end
 
-      credit = Adjustment.new(amount: amount.abs * -1, label: Spree.t(:rma_credit))
-      credit.source = self
-      credit.adjustable = order
-      credit.save
+      Adjustment.create(
+        amount: -amount.abs,
+        label: I18n.t('spree.rma_credit'),
+        order: order,
+        adjustable: order,
+        originator: self
+      )
 
       order.return if inventory_units.all?(&:returned?)
       order.update!

db/migrate/20210319155627_update_return_adjustments.rb

@@ -0,0 +1,17 @@
+class UpdateReturnAdjustments < ActiveRecord::Migration[5.0]
+  class Spree::Adjustment < ActiveRecord::Base
+    belongs_to :source, polymorphic: true
+  end
+
+  def up
+    Spree::Adjustment.where(source_type: 'Spree::ReturnAuthorization').update_all(
+      "originator_id = source_id, originator_type = 'Spree::ReturnAuthorization', source_id = NULL, source_type = NULL"
+    )
+  end
+
+  def down
+    Spree::Adjustment.where(originator_type: 'Spree::ReturnAuthorization').update_all(
+      "source_id = originator_id, source_type = 'Spree::ReturnAuthorization', originator_id = NULL, originator_type = NULL"
+    )
+  end
+end

lib/spree/core/calculated_adjustments.rb

@@ -54,21 +54,8 @@ module Spree
             end
           end
 
-          # Updates the amount of the adjustment using our Calculator and
-          #   calling the +compute+ method with the +calculable+
-          #   referenced passed to the method.
-          def update_adjustment(adjustment, calculable)
-            # Adjustment calculations done on Spree::Shipment objects MUST
-            # be done on their to_package'd variants instead
-            # It's only the package that contains the correct information.
-            # See https://github.com/spree/spree_active_shipping/pull/96 et. al
-            calculable = calculable.to_package if calculable.is_a?(Spree::Shipment)
-            adjustment.update_column(:amount, compute_amount(calculable))
-          end
-
           # Calculate the amount to be used when creating an adjustment
           # NOTE: May be overriden by classes where this module is included into.
-          # Such as Spree::Promotion::Action::CreateAdjustment.
           def compute_amount(calculable)
             calculator.compute(calculable)
           end

spec/helpers/checkout_helper_spec.rb

@@ -57,5 +57,18 @@ describe CheckoutHelper, type: :helper do
       expect(admin_fee_summary.label).to eq I18n.t(:orders_form_admin)
       expect(admin_fee_summary.amount).to eq 123
     end
+
+    context "with return authorization adjustments" do
+      let!(:return_adjustment) {
+        create(:adjustment, originator_type: 'Spree::ReturnAuthorization', adjustable: order,
+                            source: nil, order: order)
+      }
+
+      it "includes return adjustments" do
+        adjustments = helper.checkout_adjustments_for(order)
+
+        expect(adjustments).to include return_adjustment
+      end
+    end
   end
 end

spec/lib/spree/core/calculated_adjustments_spec.rb

@@ -66,13 +66,4 @@ describe Spree::Core::CalculatedAdjustments do
       end
     end
   end
-
-  context "#update_adjustment" do
-    it "should update the adjustment using its calculator (and the specified source)" do
-      adjustment = double(:adjustment).as_null_object
-      calculable = double :calculable
-      expect(adjustment).to receive(:update_column).with(:amount, 10)
-      tax_rate.update_adjustment(adjustment, calculable)
-    end
-  end
 end

spec/models/spree/adjustment_spec.rb

@@ -9,7 +9,7 @@ module Spree
 
     describe "scopes" do
       let!(:arbitrary_adjustment) { create(:adjustment, source: nil, label: "Arbitrary") }
-      let!(:return_authorization_adjustment) { create(:adjustment, source: create(:return_authorization)) }
+      let!(:return_authorization_adjustment) { create(:adjustment, originator: create(:return_authorization)) }
 
       it "returns return_authorization adjustments" do
         expect(Spree::Adjustment.return_authorization.to_a).to eq [return_authorization_adjustment]
@@ -18,39 +18,39 @@ module Spree
 
     context "#update!" do
       context "when originator present" do
-        let(:originator) { double("originator", update_adjustment: nil) }
+        let(:originator) { instance_double(EnterpriseFee, compute_amount: 10.0) }
+
         before do
-          allow(originator).to receive_messages update_amount: true
           allow(adjustment).to receive_messages originator: originator, label: 'adjustment', amount: 0
         end
 
         it "should do nothing when closed" do
           adjustment.close
-          expect(originator).not_to receive(:update_adjustment)
+          expect(originator).not_to receive(:compute_amount)
           adjustment.update!
         end
 
         it "should do nothing when finalized" do
           adjustment.finalize
-          expect(originator).not_to receive(:update_adjustment)
+          expect(originator).not_to receive(:compute_amount)
           adjustment.update!
         end
 
-        it "should ask the originator to update_adjustment" do
-          expect(originator).to receive(:update_adjustment)
+        it "should ask the originator to recalculate the amount" do
+          expect(originator).to receive(:compute_amount)
           adjustment.update!
         end
 
         context "using the :force argument" do
           it "should update adjustments without changing their state" do
-            expect(originator).to receive(:update_adjustment)
+            expect(originator).to receive(:compute_amount)
             adjustment.update!(force: true)
             expect(adjustment.state).to eq "open"
           end
 
           it "should update closed adjustments" do
             adjustment.close
-            expect(originator).to receive(:update_adjustment)
+            expect(originator).to receive(:compute_amount)
             adjustment.update!(force: true)
           end
         end
@@ -58,7 +58,7 @@ module Spree
 
       it "should do nothing when originator is nil" do
         allow(adjustment).to receive_messages originator: nil
-        expect(adjustment).not_to receive(:amount=)
+        expect(adjustment).not_to receive(:update_columns)
         adjustment.update!
       end
     end
@@ -491,5 +491,21 @@ module Spree
         end
       end
     end
+
+    context "return authorization adjustments" do
+      let!(:return_authorization) { create(:return_authorization, amount: 123) }
+      let(:order) { return_authorization.order }
+      let!(:return_adjustment) {
+        create(:adjustment, originator: return_authorization, order: order,
+                            adjustable: order, amount: 456)
+      }
+
+      describe "#update!" do
+        it "sets a negative value equal to the return authorization amount" do
+          expect { return_adjustment.update! }.
+            to change { return_adjustment.reload.amount }.to(-123)
+        end
+      end
+    end
   end
 end

spec/models/spree/return_authorization_spec.rb

@@ -82,11 +82,15 @@ describe Spree::ReturnAuthorization do
 
     it "should add credit for specified amount" do
       return_authorization.amount = 20
-      mock_adjustment = double
-      expect(mock_adjustment).to receive(:source=).with(return_authorization)
-      expect(mock_adjustment).to receive(:adjustable=).with(order)
-      expect(mock_adjustment).to receive(:save)
-      expect(Spree::Adjustment).to receive(:new).with(amount: -20, label: Spree.t(:rma_credit)).and_return(mock_adjustment)
+
+      expect(Spree::Adjustment).to receive(:create).with(
+        amount: -20,
+        label: I18n.t('spree.rma_credit'),
+        order: order,
+        adjustable: order,
+        originator: return_authorization
+      )
+
       return_authorization.receive!
     end