From 9a91d02568227e1b563cb9acec25efaecd89906b Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 10:37:14 -0800
Subject: [PATCH 01/14] Perform a full update! after each order transition

---
 core/app/models/spree/order/checkout.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/core/app/models/spree/order/checkout.rb b/core/app/models/spree/order/checkout.rb
index 3b94d4b186c..ccae17b4ed0 100644
--- a/core/app/models/spree/order/checkout.rb
+++ b/core/app/models/spree/order/checkout.rb
@@ -125,8 +125,7 @@ def define_state_machine!
             after_transition to: :canceled, do: :after_cancel
 
             after_transition from: any - :cart, to: any - [:confirm, :complete] do |order|
-              order.update_totals
-              order.persist_totals
+              order.update!
             end
 
             after_transition do |order, transition|

From e74f9db100100186f5f3f70a695f61eb7af0a27b Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 10:41:20 -0800
Subject: [PATCH 02/14] Use OrderUpdater#update! in Order#empty

---
 core/app/models/spree/order.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 486228a316f..79a4372ce41 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -472,12 +472,10 @@ def merge!(*args)
 
     def empty!
       line_items.destroy_all
-      updater.update_item_count
       adjustments.destroy_all
       shipments.destroy_all
 
-      update_totals
-      persist_totals
+      update!
     end
 
     alias_method :has_step?, :has_checkout_step?

From 0bb5dc8db061f40473f8f3405c0f637ba11c884c Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 10:58:47 -0800
Subject: [PATCH 03/14] Use update! from PromotionHandler::Coupon

---
 core/app/models/spree/promotion_handler/coupon.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/core/app/models/spree/promotion_handler/coupon.rb b/core/app/models/spree/promotion_handler/coupon.rb
index 145ef627367..cd752a5a0c5 100644
--- a/core/app/models/spree/promotion_handler/coupon.rb
+++ b/core/app/models/spree/promotion_handler/coupon.rb
@@ -95,8 +95,7 @@ def determine_promotion_application_result
         discount ||= order.adjustments.promotion.detect(&detector)
 
         if discount && discount.eligible
-          order.update_totals
-          order.persist_totals
+          order.update!
           set_success_code :coupon_code_applied
         elsif order.promotions.with_coupon_code(order.coupon_code)
           # if the promotion exists on an order, but wasn't found above,

From e706293879ff9165a7b0fa7ab31bcb6ea46f71de Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 11:15:43 -0800
Subject: [PATCH 04/14] Avoid Updater method in ensure_promotions_eligible

Instead of using a (soon to be) private method in OrderUpdater, this
method now does what is asked of it. Tests adjustments one-by-one to
ensure that they remain eligible.
---
 core/app/models/spree/adjustment.rb           | 14 +++++++++++++-
 core/app/models/spree/order.rb                |  7 +++++--
 core/spec/models/spree/promotion_code_spec.rb |  4 ++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/core/app/models/spree/adjustment.rb b/core/app/models/spree/adjustment.rb
index 33db6252d1a..2cbd5ffe49b 100644
--- a/core/app/models/spree/adjustment.rb
+++ b/core/app/models/spree/adjustment.rb
@@ -105,7 +105,7 @@ def update!
         self.amount = source.compute_amount(adjustable)
 
         if promotion?
-          self.eligible = source.promotion.eligible?(adjustable, promotion_code: promotion_code)
+          self.eligible = calculate_eligibility
         end
 
         # Persist only if changed
@@ -116,6 +116,18 @@ def update!
       amount
     end
 
+    # Calculates based on attached promotion (if this is a promotion
+    # adjustment) whether this promotion is still eligible.
+    # @api private
+    # @return [true,false] Whether this adjustment is eligible
+    def calculate_eligibility
+      if !finalized? && source && promotion?
+        source.promotion.eligible?(adjustable, promotion_code: promotion_code)
+      else
+        eligible?
+      end
+    end
+
     private
 
     def require_promotion_code?
diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 79a4372ce41..2826b15600e 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -794,9 +794,12 @@ def ensure_inventory_units
     end
 
     def ensure_promotions_eligible
-      updater.update_adjustment_total
-      if promo_total_changed?
+      adjustment_changed = all_adjustments.eligible.promotion.any? do |adjustment|
+        !adjustment.calculate_eligibility
+      end
+      if adjustment_changed
         restart_checkout_flow
+        update!
         errors.add(:base, Spree.t(:promotion_total_changed_before_complete))
       end
       errors.empty?
diff --git a/core/spec/models/spree/promotion_code_spec.rb b/core/spec/models/spree/promotion_code_spec.rb
index f4054fe9446..e4fed5fdc44 100644
--- a/core/spec/models/spree/promotion_code_spec.rb
+++ b/core/spec/models/spree/promotion_code_spec.rb
@@ -164,21 +164,25 @@
         order.complete!
       end
     end
+
     it "makes the promotion ineligible" do
       expect{
         order.complete
       }.to change{ promo_adjustment.reload.eligible }.to(false)
     end
+
     it "adjusts the promo_total" do
       expect{
         order.complete
       }.to change(order, :promo_total).by(10)
     end
+
     it "increases the total to remove the promo" do
       expect{
         order.complete
       }.to change(order, :total).from(30).to(40)
     end
+
     it "resets the state of the order" do
       expect{
         order.complete

From 438f45be075394ba67a6a96bdfb4c68538c96d6d Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 11:57:11 -0800
Subject: [PATCH 05/14] Call update! from set_shipments_cost

---
 core/app/models/spree/order.rb                     | 3 +--
 core/spec/models/spree/order/state_machine_spec.rb | 3 ++-
 core/spec/models/spree/order_spec.rb               | 3 +--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 2826b15600e..836deb2f5b8 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -568,8 +568,7 @@ def shipping_eq_billing_address?
 
     def set_shipments_cost
       shipments.each(&:update_amounts)
-      updater.update_shipment_total
-      persist_totals
+      update!
     end
 
     def is_risky?
diff --git a/core/spec/models/spree/order/state_machine_spec.rb b/core/spec/models/spree/order/state_machine_spec.rb
index e7b7a9323fd..4ba824b9cf9 100644
--- a/core/spec/models/spree/order/state_machine_spec.rb
+++ b/core/spec/models/spree/order/state_machine_spec.rb
@@ -43,7 +43,8 @@
       end
 
       it "adjusts tax rates when transitioning to delivery" do
-        expect(Spree::Tax::OrderAdjuster).to receive(:new).once.with(order).and_call_original
+        # Fixme: once would be better
+        expect(Spree::Tax::OrderAdjuster).to receive(:new).twice.with(order).and_call_original
         order.next!
       end
     end
diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb
index f3d89eef55c..387a429ca0d 100644
--- a/core/spec/models/spree/order_spec.rb
+++ b/core/spec/models/spree/order_spec.rb
@@ -107,8 +107,7 @@
 
     it "update and persist totals" do
       expect(shipment).to receive :update_amounts
-      expect(order.updater).to receive :update_shipment_total
-      expect(order.updater).to receive :persist_totals
+      expect(order.updater).to receive :update
 
       order.set_shipments_cost
     end

From 32779fee84305a0c3e5fb9bb03b513b74368c2b9 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 14:08:02 -0800
Subject: [PATCH 06/14] Don't call to-be-private Updater methods in spec

---
 core/spec/models/spree/order_updater_spec.rb | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb
index 779b4fa3655..6e688896edb 100644
--- a/core/spec/models/spree/order_updater_spec.rb
+++ b/core/spec/models/spree/order_updater_spec.rb
@@ -16,20 +16,22 @@ module Spree
       context 'with refund' do
         it "updates payment totals" do
           create(:payment_with_refund, order: order, amount: 33.25, refund_amount: 3)
-          Spree::OrderUpdater.new(order).update_payment_total
+          updater.update
           expect(order.payment_total).to eq(30.25)
         end
       end
 
       it "update item total" do
-        updater.update_item_total
-        expect(order.item_total).to eq(20)
+        expect {
+          updater.update
+        }.to change { order.item_total }.to 20
       end
 
       it "update shipment total" do
         create(:shipment, order: order, cost: 10)
-        updater.update_shipment_total
-        expect(order.shipment_total).to eq(10)
+        expect {
+          updater.update
+        }.to change { order.shipment_total }.to 10
       end
 
       context 'with order promotion followed by line item addition' do
@@ -60,7 +62,7 @@ module Spree
         create(:adjustment, adjustable: order, order: order, source: nil, amount: 10)
 
         expect {
-          updater.update_adjustment_total
+          updater.update
         }.to change {
           order.adjustment_total
         }.from(0).to(10)
@@ -483,17 +485,17 @@ def create_adjustment(label, amount)
 
         it "updates each shipment" do
           expect(shipment).to receive(:update!)
-          updater.update_shipments
+          updater.update
         end
 
         it "refreshes shipment rates" do
           expect(shipment).to receive(:refresh_rates)
-          updater.update_shipments
+          updater.update
         end
 
         it "updates the shipment amount" do
           expect(shipment).to receive(:update_amounts)
-          updater.update_shipments
+          updater.update
         end
       end
     end

From 50096228517cb6004afad416f07cf0788f3aa3db Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Tue, 21 Mar 2017 15:21:14 -0700
Subject: [PATCH 07/14] Make some methods of OrderUpdater private

---
 core/app/models/spree/order_updater.rb | 118 ++++++++++++-------------
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb
index 8778064e571..a0282bf997e 100644
--- a/core/app/models/spree/order_updater.rb
+++ b/core/app/models/spree/order_updater.rb
@@ -32,6 +32,65 @@ def run_hooks
       update_hooks.each { |hook| order.send hook }
     end
 
+    # Updates the +shipment_state+ attribute according to the following logic:
+    #
+    # shipped   when all Shipments are in the "shipped" state
+    # partial   when at least one Shipment has a state of "shipped" and there is another Shipment with a state other than "shipped"
+    #           or there are InventoryUnits associated with the order that have a state of "sold" but are not associated with a Shipment.
+    # ready     when all Shipments are in the "ready" state
+    # backorder when there is backordered inventory associated with an order
+    # pending   when all Shipments are in the "pending" state
+    #
+    # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
+    def update_shipment_state
+      if order.backordered?
+        order.shipment_state = 'backorder'
+      else
+        # get all the shipment states for this order
+        shipment_states = shipments.states
+        if shipment_states.size > 1
+          # multiple shiment states means it's most likely partially shipped
+          order.shipment_state = 'partial'
+        else
+          # will return nil if no shipments are found
+          order.shipment_state = shipment_states.first
+          # TODO: inventory unit states?
+          # if order.shipment_state && order.inventory_units.where(:shipment_id => nil).exists?
+          #   shipments exist but there are unassigned inventory units
+          #   order.shipment_state = 'partial'
+          # end
+        end
+      end
+
+      order.state_changed('shipment')
+      order.shipment_state
+    end
+
+    # Updates the +payment_state+ attribute according to the following logic:
+    #
+    # paid          when +payment_total+ is equal to +total+
+    # balance_due   when +payment_total+ is less than +total+
+    # credit_owed   when +payment_total+ is greater than +total+
+    # failed        when most recent payment is in the failed state
+    #
+    # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
+    def update_payment_state
+      last_state = order.payment_state
+      if payments.present? && payments.valid.size == 0 && order.outstanding_balance != 0
+        order.payment_state = 'failed'
+      elsif order.state == 'canceled' && order.payment_total == 0
+        order.payment_state = 'void'
+      else
+        order.payment_state = 'balance_due' if order.outstanding_balance > 0
+        order.payment_state = 'credit_owed' if order.outstanding_balance < 0
+        order.payment_state = 'paid' if !order.outstanding_balance?
+      end
+      order.state_changed('payment') if last_state != order.payment_state
+      order.payment_state
+    end
+
+    private
+
     # This will update and select the best promotion adjustment, update tax
     # adjustments, update cancellation adjustments, and then update the total
     # fields (promo_total, included_tax_total, additional_tax_total, and
@@ -114,65 +173,6 @@ def persist_totals
       order.save!(validate: false)
     end
 
-    # Updates the +shipment_state+ attribute according to the following logic:
-    #
-    # shipped   when all Shipments are in the "shipped" state
-    # partial   when at least one Shipment has a state of "shipped" and there is another Shipment with a state other than "shipped"
-    #           or there are InventoryUnits associated with the order that have a state of "sold" but are not associated with a Shipment.
-    # ready     when all Shipments are in the "ready" state
-    # backorder when there is backordered inventory associated with an order
-    # pending   when all Shipments are in the "pending" state
-    #
-    # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
-    def update_shipment_state
-      if order.backordered?
-        order.shipment_state = 'backorder'
-      else
-        # get all the shipment states for this order
-        shipment_states = shipments.states
-        if shipment_states.size > 1
-          # multiple shiment states means it's most likely partially shipped
-          order.shipment_state = 'partial'
-        else
-          # will return nil if no shipments are found
-          order.shipment_state = shipment_states.first
-          # TODO: inventory unit states?
-          # if order.shipment_state && order.inventory_units.where(shipment_id: nil).exists?
-          #   shipments exist but there are unassigned inventory units
-          #   order.shipment_state = 'partial'
-          # end
-        end
-      end
-
-      order.state_changed('shipment')
-      order.shipment_state
-    end
-
-    # Updates the +payment_state+ attribute according to the following logic:
-    #
-    # paid          when +payment_total+ is equal to +total+
-    # balance_due   when +payment_total+ is less than +total+
-    # credit_owed   when +payment_total+ is greater than +total+
-    # failed        when most recent payment is in the failed state
-    #
-    # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
-    def update_payment_state
-      last_state = order.payment_state
-      if payments.present? && payments.valid.size == 0 && order.outstanding_balance != 0
-        order.payment_state = 'failed'
-      elsif order.state == 'canceled' && order.payment_total == 0
-        order.payment_state = 'void'
-      else
-        order.payment_state = 'balance_due' if order.outstanding_balance > 0
-        order.payment_state = 'credit_owed' if order.outstanding_balance < 0
-        order.payment_state = 'paid' if !order.outstanding_balance?
-      end
-      order.state_changed('payment') if last_state != order.payment_state
-      order.payment_state
-    end
-
-    private
-
     def round_money(n)
       (n * 100).round / 100.0
     end

From 4ad32da96500ce56ea75aedeb4d3f868bf269eb5 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 15:19:00 -0800
Subject: [PATCH 08/14] Remove call to refresh_rates in OrderUpdater

Previously refresh_rates was called from OrderUpdater on completed
Orders. However, refresh_rates has no effect on completed orders, so
this was pointless.
---
 core/app/models/spree/order_updater.rb       | 1 -
 core/spec/models/spree/order_updater_spec.rb | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb
index a0282bf997e..20ff45740e9 100644
--- a/core/app/models/spree/order_updater.rb
+++ b/core/app/models/spree/order_updater.rb
@@ -128,7 +128,6 @@ def update_shipments
       shipments.each do |shipment|
         next unless shipment.persisted?
         shipment.update!(order)
-        shipment.refresh_rates
         shipment.update_amounts
       end
     end
diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb
index 6e688896edb..86b07aa1c4d 100644
--- a/core/spec/models/spree/order_updater_spec.rb
+++ b/core/spec/models/spree/order_updater_spec.rb
@@ -488,11 +488,6 @@ def create_adjustment(label, amount)
           updater.update
         end
 
-        it "refreshes shipment rates" do
-          expect(shipment).to receive(:refresh_rates)
-          updater.update
-        end
-
         it "updates the shipment amount" do
           expect(shipment).to receive(:update_amounts)
           updater.update

From b671384453ac1d5c4bec979801464b95b40f1e02 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 15:23:20 -0800
Subject: [PATCH 09/14] Remove break on non-persisted Shipments

I don't think this is necessary, and has just been kept around. We
should never get an unsaved shipment here. Even if we did, it would have
unexpected behaviour as it would be saved later.
---
 core/app/models/spree/order_updater.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb
index 20ff45740e9..a3409290ae2 100644
--- a/core/app/models/spree/order_updater.rb
+++ b/core/app/models/spree/order_updater.rb
@@ -126,7 +126,6 @@ def update_totals
     # give each of the shipments a chance to update themselves
     def update_shipments
       shipments.each do |shipment|
-        next unless shipment.persisted?
         shipment.update!(order)
         shipment.update_amounts
       end

From 8dc5ae4eb2a6e47ef0459eb3d599a32d31c2ef7c Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 15:30:58 -0800
Subject: [PATCH 10/14] Call Shipment#update_amounts from OrderUpdater

Previously, this was only called on completed orders. It's a simple and
fast update, so we might as well perform it on every single update. This
allows us to remove the need for Order#set_shipments_cost
---
 core/app/models/spree/order_updater.rb | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb
index a3409290ae2..18bd9bd308f 100644
--- a/core/app/models/spree/order_updater.rb
+++ b/core/app/models/spree/order_updater.rb
@@ -17,6 +17,7 @@ def initialize(order)
     def update
       @order.transaction do
         update_item_count
+        update_shipment_amounts
         update_totals
         if order.completed?
           update_payment_state
@@ -123,11 +124,16 @@ def update_totals
       update_adjustment_total
     end
 
+    def update_shipment_amounts
+      shipments.each do |shipment|
+        shipment.update_amounts
+      end
+    end
+
     # give each of the shipments a chance to update themselves
     def update_shipments
       shipments.each do |shipment|
         shipment.update!(order)
-        shipment.update_amounts
       end
     end
 

From 47cbca15b697a607ef8166669c67141adc748fb4 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Tue, 21 Mar 2017 15:21:33 -0700
Subject: [PATCH 11/14] Don't call set_shipments_cost as part of checkout

This is now done as part of order.update! and is unnecessary
---
 core/app/models/spree/order/checkout.rb            | 2 --
 core/spec/models/spree/order/checkout_spec.rb      | 6 ------
 core/spec/models/spree/order/state_machine_spec.rb | 3 +--
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/core/app/models/spree/order/checkout.rb b/core/app/models/spree/order/checkout.rb
index ccae17b4ed0..dd21bb5c8fb 100644
--- a/core/app/models/spree/order/checkout.rb
+++ b/core/app/models/spree/order/checkout.rb
@@ -79,7 +79,6 @@ def define_state_machine!
               end
 
               after_transition to: :complete, do: :add_payment_sources_to_wallet
-              before_transition to: :payment, do: :set_shipments_cost
               before_transition to: :payment, do: :add_default_payment_from_wallet
 
               before_transition to: :confirm, do: :add_store_credit_payments
@@ -99,7 +98,6 @@ def define_state_machine!
               before_transition to: :delivery, do: :ensure_shipping_address
               before_transition to: :delivery, do: :create_proposed_shipments
               before_transition to: :delivery, do: :ensure_available_shipping_rates
-              before_transition to: :delivery, do: :set_shipments_cost
               before_transition from: :delivery, do: :apply_free_shipping_promotions
             end
 
diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb
index 1474aa31ea5..324e98ce8b9 100644
--- a/core/spec/models/spree/order/checkout_spec.rb
+++ b/core/spec/models/spree/order/checkout_spec.rb
@@ -254,11 +254,6 @@ def assert_state_changed(order, from, to)
           allow(order).to receive(:ensure_available_shipping_rates) { true }
         end
 
-        it 'should invoke set_shipment_cost' do
-          expect(order).to receive(:set_shipments_cost)
-          order.next!
-        end
-
         it 'should update shipment_total' do
           expect { order.next! }.to change{ order.shipment_total }.by(10.00)
         end
@@ -300,7 +295,6 @@ def assert_state_changed(order, from, to)
         end
 
         it "transitions to payment" do
-          expect(order).to receive(:set_shipments_cost)
           order.next!
           assert_state_changed(order, 'delivery', 'payment')
           expect(order.state).to eq('payment')
diff --git a/core/spec/models/spree/order/state_machine_spec.rb b/core/spec/models/spree/order/state_machine_spec.rb
index 4ba824b9cf9..e7b7a9323fd 100644
--- a/core/spec/models/spree/order/state_machine_spec.rb
+++ b/core/spec/models/spree/order/state_machine_spec.rb
@@ -43,8 +43,7 @@
       end
 
       it "adjusts tax rates when transitioning to delivery" do
-        # Fixme: once would be better
-        expect(Spree::Tax::OrderAdjuster).to receive(:new).twice.with(order).and_call_original
+        expect(Spree::Tax::OrderAdjuster).to receive(:new).once.with(order).and_call_original
         order.next!
       end
     end

From d55027a7b9c5dfcb7a51303543ece79634db9c84 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 16:07:29 -0800
Subject: [PATCH 12/14] Stop calling set_shipments_cost in OrderUpdateAttr

Previously OrderUpdateAttributes would call set_shipment_cost just
in case any shipments had their selected shipping rate changed.

set_shipment_cost would set the Shipment's cost from the shipping rate,
and would update the order total, but wouldn't properly recalculate
taxes or promotions.

This commit removes the call to set_shipment_cost. Instead,
order.update! should be called after OrderUpdateAttributes. This is done
everywhere OrderUpdateAttributes is used in Solidus itself, and was
already required (though not explicitly) for correct behaviour.
---
 .../models/spree/order_update_attributes.rb   |  7 +---
 .../spree/order_update_attributes_spec.rb     | 39 -------------------
 2 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/core/app/models/spree/order_update_attributes.rb b/core/app/models/spree/order_update_attributes.rb
index a67d43752e4..6bcf0513a61 100644
--- a/core/app/models/spree/order_update_attributes.rb
+++ b/core/app/models/spree/order_update_attributes.rb
@@ -17,12 +17,7 @@ def apply
       assign_order_attributes
       assign_payments_attributes
 
-      if order.save
-        order.set_shipments_cost if order.shipments.any?
-        true
-      else
-        false
-      end
+      order.save
     end
 
     private
diff --git a/core/spec/models/spree/order_update_attributes_spec.rb b/core/spec/models/spree/order_update_attributes_spec.rb
index 5ed13696091..97113658e8b 100644
--- a/core/spec/models/spree/order_update_attributes_spec.rb
+++ b/core/spec/models/spree/order_update_attributes_spec.rb
@@ -39,44 +39,5 @@ module Spree
         end
       end
     end
-
-    context 'when changing shipping method' do
-      let!(:order) { create(:order_with_line_items, shipping_method: shipping_method1) }
-      let(:shipment){ order.shipments.first }
-      let!(:zone) { create(:zone) }
-      let!(:shipping_method1){ create(:shipping_method, cost: 10, zones: [zone]) }
-      let!(:shipping_method2){ create(:shipping_method, cost: 20, zones: [zone]) }
-
-      let(:attributes) do
-        {
-          shipments_attributes: {
-            0 => { selected_shipping_rate_id: shipping_method2, id: shipment.id }
-          }
-        }
-      end
-
-      it "updates shipment costs" do
-        zone.zone_members.create!(zoneable: order.ship_address.country)
-        order.create_proposed_shipments
-        order.set_shipments_cost
-
-        shipping_rate2 = shipment.shipping_rates.find_by(shipping_method_id: shipping_method2.id)
-
-        expect(order.shipment_total).to eq(10)
-
-        # We need an order which doesn't have shipping_rates loaded
-        order.reload
-
-        described_class.new(
-          order,
-          shipments_attributes: {
-            0 => { selected_shipping_rate_id: shipping_rate2.id, id: shipment.id }
-          }
-        ).apply
-
-        expect(order.shipment_total).to eq(20)
-        expect(order.shipments.first.cost).to eq(20)
-      end
-    end
   end
 end

From 85407b617752bc41473f5099c5756325cbc23fa7 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 16:27:03 -0800
Subject: [PATCH 13/14] Avoid calling set_shipments_cost

---
 core/spec/models/spree/order/checkout_spec.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb
index 324e98ce8b9..3966c566d62 100644
--- a/core/spec/models/spree/order/checkout_spec.rb
+++ b/core/spec/models/spree/order/checkout_spec.rb
@@ -327,7 +327,7 @@ def assert_state_changed(order, from, to)
         context "with a shipment that has a price" do
           before do
             shipment.shipping_rates.first.update_column(:cost, 10)
-            order.set_shipments_cost
+            order.update!
           end
 
           it "transitions to payment" do
@@ -339,7 +339,6 @@ def assert_state_changed(order, from, to)
         context "with a shipment that is free" do
           before do
             shipment.shipping_rates.first.update_column(:cost, 0)
-            order.set_shipments_cost
           end
 
           it "skips payment, transitions to confirm" do

From e5fce467190eaa89772f7d09bf04abc3c7385dd8 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 23 Jan 2017 16:27:16 -0800
Subject: [PATCH 14/14] Deprecate set_shipments_cost

---
 core/app/models/spree/order.rb       | 1 +
 core/spec/models/spree/order_spec.rb | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 836deb2f5b8..a932f78f663 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -570,6 +570,7 @@ def set_shipments_cost
       shipments.each(&:update_amounts)
       update!
     end
+    deprecate set_shipments_cost: :update!, deprecator: Spree::Deprecation
 
     def is_risky?
       payments.risky.count > 0
diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb
index 387a429ca0d..11bfc4c3e06 100644
--- a/core/spec/models/spree/order_spec.rb
+++ b/core/spec/models/spree/order_spec.rb
@@ -109,7 +109,9 @@
       expect(shipment).to receive :update_amounts
       expect(order.updater).to receive :update
 
-      order.set_shipments_cost
+      Spree::Deprecation.silence do
+        order.set_shipments_cost
+      end
     end
   end