diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index ef34d0eb9f..a0bd3e1541 100644 --- a/app/models/spree/adjustment.rb +++ b/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 diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb index fc5acb3f02..f05afee8c4 100644 --- a/app/models/spree/return_authorization.rb +++ b/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! diff --git a/db/migrate/20210319155627_update_return_adjustments.rb b/db/migrate/20210319155627_update_return_adjustments.rb new file mode 100644 index 0000000000..cd6a9788c6 --- /dev/null +++ b/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 diff --git a/lib/spree/core/calculated_adjustments.rb b/lib/spree/core/calculated_adjustments.rb index 944eff67b6..f773567647 100644 --- a/lib/spree/core/calculated_adjustments.rb +++ b/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 diff --git a/spec/helpers/checkout_helper_spec.rb b/spec/helpers/checkout_helper_spec.rb index 0283fabc23..4bfdb1b43b 100644 --- a/spec/helpers/checkout_helper_spec.rb +++ b/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 diff --git a/spec/lib/spree/core/calculated_adjustments_spec.rb b/spec/lib/spree/core/calculated_adjustments_spec.rb index 484654e2eb..f6870807e1 100644 --- a/spec/lib/spree/core/calculated_adjustments_spec.rb +++ b/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 diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 39e48cafc6..f1a580c234 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/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 diff --git a/spec/models/spree/return_authorization_spec.rb b/spec/models/spree/return_authorization_spec.rb index d37bac00ed..5fa696f522 100644 --- a/spec/models/spree/return_authorization_spec.rb +++ b/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