From fde65570f7771ba2482cf50b6691a6aa89e2cbe6 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 26 Jul 2016 11:08:37 -0700 Subject: [PATCH 1/2] Remove update_adjustments AR callbacks The existing behaviour was that line_items and shipments had their adjustments recalculated after save. This would update the item's adjustments and the item's _total columns. However this doesn't update the order's _total columns until order.update! is run. Since order.update! will update these values anyways, and since the order is in an inconsistent state until order.update! is run, we should remove these callbacks. --- core/app/models/spree/line_item.rb | 12 ----------- core/app/models/spree/shipment.rb | 12 ----------- core/spec/models/spree/line_item_spec.rb | 19 ----------------- core/spec/models/spree/shipment_spec.rb | 26 ------------------------ 4 files changed, 69 deletions(-) diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index c65ea17fb82..c99cebdcb93 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -26,7 +26,6 @@ class LineItem < Spree::Base after_create :update_tax_charge after_save :update_inventory - after_save :update_adjustments before_destroy :update_inventory before_destroy :destroy_inventory_units @@ -167,17 +166,6 @@ def destroy_inventory_units inventory_units.destroy_all end - def update_adjustments - if quantity_changed? - update_tax_charge # Called to ensure pre_tax_amount is updated. - recalculate_adjustments - end - end - - def recalculate_adjustments - Spree::ItemAdjustments.new(self).update - end - def update_tax_charge Spree::Tax::ItemAdjuster.new(self).adjust! end diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 3c092554bc9..e7f3e7cc391 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -10,8 +10,6 @@ class Shipment < Spree::Base has_many :state_changes, as: :stateful has_many :cartons, -> { uniq }, through: :inventory_units - after_save :update_adjustments - before_validation :set_cost_zero_when_nil before_destroy :ensure_can_destroy @@ -404,20 +402,10 @@ def manifest_unstock(item) stock_location.unstock item.variant, item.quantity, self end - def recalculate_adjustments - Spree::ItemAdjustments.new(self).update - end - def set_cost_zero_when_nil self.cost = 0 unless cost end - def update_adjustments - if cost_changed? && state != 'shipped' - recalculate_adjustments - end - end - def ensure_can_destroy unless pending? errors.add(:state, :cannot_destroy, state: state) diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index 1a7516c9077..58008505ad3 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -26,25 +26,6 @@ end context "#save" do - context "line item changes" do - before do - line_item.quantity = line_item.quantity + 1 - end - - it "triggers adjustment total recalculation" do - expect(line_item).to receive(:update_tax_charge) # Regression test for https://github.com/spree/spree/issues/4671 - expect(line_item).to receive(:recalculate_adjustments) - line_item.save - end - end - - context "line item does not change" do - it "does not trigger adjustment total recalculation" do - expect(line_item).not_to receive(:recalculate_adjustments) - line_item.save - end - end - context "target_shipment is provided" do it "verifies inventory" do line_item.target_shipment = Spree::Shipment.new diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 9b255bc2f96..9df709fd7fc 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -585,32 +585,6 @@ end end - context "after_save" do - context "line item changes" do - before do - shipment.cost = shipment.cost + 10 - end - - it "triggers adjustment total recalculation" do - expect(shipment).to receive(:recalculate_adjustments) - shipment.save - end - - it "does not trigger adjustment recalculation if shipment has shipped" do - shipment.state = 'shipped' - expect(shipment).not_to receive(:recalculate_adjustments) - shipment.save - end - end - - context "line item does not change" do - it "does not trigger adjustment total recalculation" do - expect(shipment).not_to receive(:recalculate_adjustments) - shipment.save - end - end - end - context "currency" do it "returns the order currency" do expect(shipment.currency).to eq(order.currency) From fd3dff16f7477bf2cc91911bd50b7d0a7dbe07e4 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 26 Jul 2016 13:47:30 -0700 Subject: [PATCH 2/2] Add CHANGELOG entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78de757b711..934dc378e19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,12 @@ * Removed "Clear cache" button from the admin [#1275](https://github.com/solidusio/solidus/pull/1275) +* Adjustments and totals are no longer updated when saving a Shipment or LineItem. + + Previously adjustments and total columns were updated after saving a Shipment or LineItem. + This was unnecessary since it didn't update the order totals, and running + order.update! would recalculate the adjustments and totals again. + ## Solidus 1.3.0 (unreleased) * Order now requires a `store_id` in validations