From 66aba465ac1d86aefcec66be1dc60cbd1ddd3726 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 12 Jul 2017 12:15:29 -0700 Subject: [PATCH] Rename shipment.update! to shipment.update_state Overriding shipment.update! prevented us from calling AR::Base#update! on a shipment. This renames Shipment#update! to Shipment#update_state and deprecates calling Shipment#update! with an order as an argument. --- core/app/models/spree/exchange.rb | 2 +- core/app/models/spree/order.rb | 4 ++-- core/app/models/spree/order_updater.rb | 2 +- core/app/models/spree/shipment.rb | 21 +++++++++++++++---- .../models/spree/order/finalizing_spec.rb | 2 +- core/spec/models/spree/order_updater_spec.rb | 2 +- core/spec/models/spree/shipment_spec.rb | 18 ++++++++-------- 7 files changed, 32 insertions(+), 19 deletions(-) diff --git a/core/app/models/spree/exchange.rb b/core/app/models/spree/exchange.rb index 70a31a27ac0..f04917111df 100644 --- a/core/app/models/spree/exchange.rb +++ b/core/app/models/spree/exchange.rb @@ -27,7 +27,7 @@ def perform! @order.shipments += shipments @order.save! shipments.each do |shipment| - shipment.update!(@order) + shipment.update_state shipment.finalize! end end diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 4a9880e6c12..531c9f847e4 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -417,7 +417,7 @@ def finalize! # update payment and shipment(s) states, and save updater.update_payment_state shipments.each do |shipment| - shipment.update!(self) + shipment.update_state shipment.finalize! end @@ -431,7 +431,7 @@ def finalize! end def fulfill! - shipments.each { |shipment| shipment.update!(self) if shipment.persisted? } + shipments.each { |shipment| shipment.update_state if shipment.persisted? } updater.update_shipment_state save! end diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 5f9e4429db7..ca19908dbe6 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -141,7 +141,7 @@ def update_shipment_amounts # give each of the shipments a chance to update themselves def update_shipments shipments.each do |shipment| - shipment.update!(order) + shipment.update_state end end diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 7a9aea67731..32e6f9671a3 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -305,10 +305,11 @@ def update_attributes_and_order(params = {}) end end - # Updates various aspects of the Shipment while bypassing any callbacks. Note that this method takes an explicit reference to the - # Order object. This is necessary because the association actually has a stale (and unsaved) copy of the Order and so it will not - # yield the correct results. - def update!(order) + # Updates the state of the Shipment bypassing any callbacks. + # + # If this moves the shipmnent to the 'shipped' state, after_ship will be + # called. + def update_state old_state = state new_state = determine_state(order) if new_state != old_state @@ -320,6 +321,18 @@ def update!(order) end end + def update!(order_or_attrs) + if order_or_attrs.is_a?(Spree::Order) + Spree::Deprecation.warn "Calling Shipment#update! with an order to update the shipments state is deprecated. Please use Shipment#update_state instead." + if order_or_attrs.object_id != order.object_id + Spree::Deprecation.warn "Additionally, update! is being passed an instance of order which isn't the same object as the shipment's order association" + end + update_state + else + super + end + end + def transfer_to_location(variant, quantity, stock_location) if quantity <= 0 raise ArgumentError diff --git a/core/spec/models/spree/order/finalizing_spec.rb b/core/spec/models/spree/order/finalizing_spec.rb index dee3d1edbaa..c6bed6ae63c 100644 --- a/core/spec/models/spree/order/finalizing_spec.rb +++ b/core/spec/models/spree/order/finalizing_spec.rb @@ -18,7 +18,7 @@ it "should sell inventory units" do order.shipments.each do |shipment| - expect(shipment).to receive(:update!) + expect(shipment).to receive(:update_state) expect(shipment).to receive(:finalize!) end order.finalize! diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb index 17b6db80dec..6f1d99e536e 100644 --- a/core/spec/models/spree/order_updater_spec.rb +++ b/core/spec/models/spree/order_updater_spec.rb @@ -483,7 +483,7 @@ def create_adjustment(label, amount) let(:shipment){ order.shipments[0] } it "updates each shipment" do - expect(shipment).to receive(:update!) + expect(shipment).to receive(:update_state) updater.update end diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 6eb4f62148e..ed064f66ef4 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -265,13 +265,13 @@ end end - context "#update!" do + context "#update_state" do shared_examples_for "immutable once shipped" do before { shipment.update_columns(state: 'shipped') } it "should remain in shipped state once shipped" do expect { - shipment.update!(order) + shipment.update_state }.not_to change { shipment.state } end end @@ -283,7 +283,7 @@ allow(shipment).to receive_messages(inventory_units: [mock_model(Spree::InventoryUnit, allow_ship?: false, canceled?: false)]) expect(shipment).to receive(:update_columns).with(state: 'pending', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end end @@ -293,7 +293,7 @@ # Set as ready so we can test for change shipment.update_attributes!(state: 'ready') expect(shipment).to receive(:update_columns).with(state: 'pending', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end end @@ -301,7 +301,7 @@ before { allow(order).to receive_messages paid?: true } it "should result in a 'ready' state" do expect(shipment).to receive(:update_columns).with(state: 'ready', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end it_should_behave_like 'immutable once shipped' it_should_behave_like 'pending if backordered' @@ -314,7 +314,7 @@ it "should result in a 'ready' state" do expect(shipment).to receive(:update_columns).with(state: 'ready', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end it_should_behave_like 'immutable once shipped' it_should_behave_like 'pending if backordered' @@ -325,7 +325,7 @@ it "should result in a 'pending' state" do shipment.state = 'ready' expect(shipment).to receive(:update_columns).with(state: 'pending', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end it_should_behave_like 'immutable once shipped' it_should_behave_like 'pending if backordered' @@ -336,7 +336,7 @@ it "should result in a 'ready' state" do shipment.state = 'pending' expect(shipment).to receive(:update_columns).with(state: 'ready', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end it_should_behave_like 'immutable once shipped' it_should_behave_like 'pending if backordered' @@ -348,7 +348,7 @@ expect(shipment).to receive :after_ship allow(shipment).to receive_messages determine_state: 'shipped' expect(shipment).to receive(:update_columns).with(state: 'shipped', updated_at: kind_of(Time)) - shipment.update!(order) + shipment.update_state end # Regression test for https://github.com/spree/spree/issues/4347