From 57c12fc5b0c7b9070e2d97a56ee50214b89108c5 Mon Sep 17 00:00:00 2001
From: Greg <gvaughn@bonobos.com>
Date: Mon, 4 Jan 2016 13:52:36 -0600
Subject: [PATCH 1/2] preserve shipping method when shipments change

The main impetus of this is Tulip in which the guides have a much more
non-linear approach to creating an order, but the same problem exists
through the web client too.

Other approaches have been tried, but lets try this within the
Estimator itself. An extra side-effect of this is that we don't
delete shipments in Order#ensure_updated_shipments so that's poorly
named now, but it also raises a question of why it's called when it
is. This means a shipment will stick with an order if created before we
reach the "delivery" state of the order. Upon transition into "delivery"
then #created_proposed_shipments gets called to ensure they're correct.
The new logic in the Estimator now will re-select any prior shipping
method/rate selection the customer had previously made.
---
 .../spree/api/line_items_controller_spec.rb   | 38 ++++---------------
 .../spree/api/orders_controller_spec.rb       | 14 -------
 core/app/models/spree/order.rb                |  6 +--
 core/app/models/spree/shipment.rb             | 20 ++--------
 core/app/models/spree/stock/estimator.rb      | 21 ++++++++--
 .../factories/order_factory.rb                |  5 ++-
 .../factories/shipment_factory.rb             |  7 ++--
 .../core/unreturned_item_charger_spec.rb      |  1 +
 core/spec/models/spree/order/checkout_spec.rb |  2 +-
 .../spree/order_shipping_method_spec.rb       | 33 ++++++++++++++++
 core/spec/models/spree/order_spec.rb          | 22 -----------
 core/spec/models/spree/shipment_spec.rb       |  8 ++--
 .../spec/models/spree/stock/estimator_spec.rb | 17 ++++++++-
 13 files changed, 93 insertions(+), 101 deletions(-)
 create mode 100644 core/spec/models/spree/order_shipping_method_spec.rb

diff --git a/api/spec/controllers/spree/api/line_items_controller_spec.rb b/api/spec/controllers/spree/api/line_items_controller_spec.rb
index 3f63317b1f7..90d6c4f73e3 100644
--- a/api/spec/controllers/spree/api/line_items_controller_spec.rb
+++ b/api/spec/controllers/spree/api/line_items_controller_spec.rb
@@ -94,7 +94,7 @@ module Spree
         api_put :update, :id => line_item.id, :line_item => { :quantity => 101 }
         expect(response.status).to eq(200)
         order.reload
-        expect(order.total).to eq(1010) # 10 original due to factory, + 1000 in this test
+        expect(order.total).to eq(1110) # 101 quantity * 10 item price + 100 shipping cost
         expect(json_response).to have_attributes(attributes)
         expect(json_response["quantity"]).to eq(101)
       end
@@ -117,41 +117,17 @@ module Spree
         expect { line_item.reload }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
-      context "order contents changed after shipments were created" do
-        let!(:order) { Order.create }
-        let!(:line_item) { order.contents.add(product.master) }
+      context "order contents changed after shipments were created and order is completed" do
 
-        before { order.create_proposed_shipments }
-
-        it "clear out shipments on create" do
-          expect(order.reload.shipments).not_to be_empty
-          api_post :create, :line_item => { :variant_id => product.master.to_param, :quantity => 1 }
-          expect(order.reload.shipments).to be_empty
+        before do
+          allow(order).to receive_messages completed?: true
+          allow(Order).to receive_message_chain :includes, find_by!: order
         end
 
-        it "clear out shipments on update" do
+        it "doesn't destroy shipments or restart checkout flow" do
           expect(order.reload.shipments).not_to be_empty
-          api_put :update, :id => line_item.id, :line_item => { :quantity => 1000 }
-          expect(order.reload.shipments).to be_empty
-        end
-
-        it "clear out shipments on delete" do
+          api_post :create, :line_item => { :variant_id => product.master.to_param, :quantity => 1 }
           expect(order.reload.shipments).not_to be_empty
-          api_delete :destroy, :id => line_item.id
-          expect(order.reload.shipments).to be_empty
-        end
-
-        context "order is completed" do
-          before do
-            allow(order).to receive_messages completed?: true
-            allow(Order).to receive_message_chain :includes, find_by!: order
-          end
-
-          it "doesn't destroy shipments or restart checkout flow" do
-            expect(order.reload.shipments).not_to be_empty
-            api_post :create, :line_item => { :variant_id => product.master.to_param, :quantity => 1 }
-            expect(order.reload.shipments).not_to be_empty
-          end
         end
       end
     end
diff --git a/api/spec/controllers/spree/api/orders_controller_spec.rb b/api/spec/controllers/spree/api/orders_controller_spec.rb
index cd7f947466a..c0aceb5c55e 100644
--- a/api/spec/controllers/spree/api/orders_controller_spec.rb
+++ b/api/spec/controllers/spree/api/orders_controller_spec.rb
@@ -499,20 +499,6 @@ module Spree
         expect(json_response["user_id"]).to eq(original_id)
       end
 
-      context "order has shipments" do
-        before { order.create_proposed_shipments }
-
-        it "clears out all existing shipments on line item udpate" do
-          previous_shipments = order.shipments
-          api_put :update, :id => order.to_param, :order => {
-            :line_items => {
-              0 => { :id => line_item.id, :quantity => 10 }
-            }
-          }
-          expect(order.reload.shipments).to be_empty
-        end
-      end
-
       context "with a line item" do
         let(:order) { create(:order_with_line_items) }
 
diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 02c90c4c017..d9727e6c779 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -497,8 +497,7 @@ def create_proposed_shipments
         raise CannotRebuildShipments.new(Spree.t(:cannot_rebuild_shipments_shipments_not_pending))
       else
         adjustments.shipping.destroy_all
-        shipments.destroy_all
-        self.shipments = Spree::Stock::Coordinator.new(self).shipments
+        self.shipments.replace(Spree::Stock::Coordinator.new(self).shipments)
       end
     end
 
@@ -516,8 +515,6 @@ def apply_free_shipping_promotions
     # e.g. customer goes back from payment step and changes order items
     def ensure_updated_shipments
       if !completed? && shipments.all?(&:pending?)
-        self.shipments.destroy_all
-        self.update_column(:shipment_total, 0)
         restart_checkout_flow
       end
 
@@ -530,6 +527,7 @@ def restart_checkout_flow
         state: 'cart',
         updated_at: Time.now,
       )
+      # Note: what if this did contents.advance instead of self.next! ?
       self.next! if self.line_items.size > 0
     end
 
diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb
index 40a6143e610..40ff6f11543 100644
--- a/core/app/models/spree/shipment.rb
+++ b/core/app/models/spree/shipment.rb
@@ -94,6 +94,9 @@ def can_transition_from_canceled_to_ready?
     alias display_amount display_cost
 
     def add_shipping_method(shipping_method, selected = false)
+      # Note: there's some danger if selected = true that we end up with more than 1
+      # selected shipping rate since this does not clear the flag from others.
+      # #selected_shipping_rate_id= handles that case though.
       shipping_rates.create(shipping_method: shipping_method, selected: selected, cost: cost)
     end
 
@@ -172,22 +175,7 @@ def refresh_rates
       return shipping_rates if shipped? || order.completed?
       return [] unless can_get_rates?
 
-      # StockEstimator.new assigment below will replace the current shipping_method
-      original_shipping_method_id = shipping_method.try!(:id)
-
-      new_rates = Stock::Estimator.new(order).shipping_rates(to_package)
-
-      # If one of the new rates matches the previously selected shipping
-      # method, select that instead of the default provided by the estimator.
-      # Otherwise, keep the default.
-      selected_rate = new_rates.detect{ |rate| rate.shipping_method_id == original_shipping_method_id }
-      if selected_rate
-        new_rates.each do |rate|
-          rate.selected = (rate == selected_rate)
-        end
-      end
-
-      self.shipping_rates = new_rates
+      self.shipping_rates.replace(Stock::Estimator.new(order).shipping_rates(to_package))
       self.save!
 
       shipping_rates
diff --git a/core/app/models/spree/stock/estimator.rb b/core/app/models/spree/stock/estimator.rb
index 10d5d9603bd..6fe77a36850 100644
--- a/core/app/models/spree/stock/estimator.rb
+++ b/core/app/models/spree/stock/estimator.rb
@@ -19,14 +19,27 @@ def initialize(order)
       def shipping_rates(package, frontend_only = true)
         rates = calculate_shipping_rates(package)
         rates.select! { |rate| rate.shipping_method.frontend? } if frontend_only
-        choose_default_shipping_rate(rates)
+        choose_selected_shipping_rate(rates)
         sort_shipping_rates(rates)
       end
 
       private
-      def choose_default_shipping_rate(shipping_rates)
-        unless shipping_rates.empty?
-          shipping_rates.min_by(&:cost).selected = true
+      def choose_selected_shipping_rate(shipping_rates)
+        return if shipping_rates.empty?
+        shipping_rate = find_preselected_shipping_rate(shipping_rates) || shipping_rates.min_by(&:cost)
+        shipping_rate.selected = true
+      end
+
+      def find_preselected_shipping_rate(shipping_rates)
+        # TODO this method assumes a single shipment per order.
+        # If we have multiple shipments this will assign one of the preselected
+        # shipping methods to one of the shipments, which is far from ideal, but
+        # no worse than the prior behavior of always resetting to cheapest.
+        if order.shipments.count > 0
+          shipping_method = order.shipments.map {|s| s.selected_shipping_rate.try!(:shipping_method) }.compact.uniq.first
+          if shipping_method
+            shipping_rates.detect {|rate| rate.shipping_method_id == shipping_method.id }
+          end
         end
       end
 
diff --git a/core/lib/spree/testing_support/factories/order_factory.rb b/core/lib/spree/testing_support/factories/order_factory.rb
index 044600bbf5f..221938ffaf0 100644
--- a/core/lib/spree/testing_support/factories/order_factory.rb
+++ b/core/lib/spree/testing_support/factories/order_factory.rb
@@ -27,6 +27,7 @@
         line_items_attributes { [{}] * line_items_count }
         shipment_cost 100
         shipping_method nil
+        select_shipping_rate false
       end
 
       after(:create) do |order, evaluator|
@@ -36,7 +37,9 @@
         end
         order.line_items.reload
 
-        create(:shipment, order: order, cost: evaluator.shipment_cost, shipping_method: evaluator.shipping_method, address: evaluator.ship_address)
+        create(:shipment, order: order, cost: evaluator.shipment_cost,
+               shipping_method: evaluator.shipping_method, address: evaluator.ship_address,
+               select_shipping_rate: evaluator.select_shipping_rate)
         order.shipments.reload
 
         order.update!
diff --git a/core/lib/spree/testing_support/factories/shipment_factory.rb b/core/lib/spree/testing_support/factories/shipment_factory.rb
index 7e8c6ed9b7b..d3cd32e9a62 100644
--- a/core/lib/spree/testing_support/factories/shipment_factory.rb
+++ b/core/lib/spree/testing_support/factories/shipment_factory.rb
@@ -8,11 +8,12 @@
 
     transient do
       shipping_method nil
+      select_shipping_rate false
     end
 
-    after(:create) do |shipment, evalulator|
-      shipping_method = evalulator.shipping_method || create(:shipping_method, cost: evalulator.cost)
-      shipment.add_shipping_method(shipping_method, true)
+    after(:create) do |shipment, evaluator|
+      shipping_method = evaluator.shipping_method || create(:shipping_method, cost: evaluator.cost)
+      shipment.add_shipping_method(shipping_method, evaluator.select_shipping_rate)
 
       shipment.order.line_items.each do |line_item|
         line_item.quantity.times do
diff --git a/core/spec/lib/spree/core/unreturned_item_charger_spec.rb b/core/spec/lib/spree/core/unreturned_item_charger_spec.rb
index 581a624a304..d189f33fee3 100644
--- a/core/spec/lib/spree/core/unreturned_item_charger_spec.rb
+++ b/core/spec/lib/spree/core/unreturned_item_charger_spec.rb
@@ -9,6 +9,7 @@
   let(:exchange_shipment) do
     create(:shipment,
            order: shipped_order,
+           select_shipping_rate: true,
            state: 'shipped',
            stock_location: original_stock_location,
            created_at: 5.days.ago)
diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb
index 44ec98e74fc..74415444edf 100644
--- a/core/spec/models/spree/order/checkout_spec.rb
+++ b/core/spec/models/spree/order/checkout_spec.rb
@@ -325,7 +325,7 @@ def assert_state_changed(order, from, to)
 
       context "correctly determining payment required based on shipping information" do
         let(:shipment) do
-          FactoryGirl.create(:shipment)
+          FactoryGirl.create(:shipment, select_shipping_rate: true)
         end
 
         before do
diff --git a/core/spec/models/spree/order_shipping_method_spec.rb b/core/spec/models/spree/order_shipping_method_spec.rb
new file mode 100644
index 00000000000..f182673da33
--- /dev/null
+++ b/core/spec/models/spree/order_shipping_method_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe Spree::Order do
+  let(:order) { create(:order_with_line_items) }
+  let(:quadcopter) { create(:shipping_method, name: "Quads'r'us", code: "QUADSRUS", cost: 200) }
+
+  it "preserves selected shipping method when update_cart and next! called" do
+    #choose more expensive quadcopter shipping method
+    original_shipment = order.shipments.first
+    quadcopter_rate = original_shipment.add_shipping_method(quadcopter)
+    original_shipment.selected_shipping_rate_id = quadcopter_rate.id
+    expect(order.shipments.first.shipping_method).to eq quadcopter
+
+    order.contents.update_cart({}) #contents don't really matter
+    order.contents.advance
+
+    expect(order.shipments.first.shipping_method).to eq quadcopter
+  end
+
+  let(:cheap_ups_cost) { 10 }
+
+  it "reaching delivery step with no explicit selection selects cheapest shipping rate" do
+    # ... even if the shipping method is created after the order
+    order.contents.add(create(:variant), 1)
+    cheap_ups = create(:shipping_method, name: "Cheap UPS Ground", cost: cheap_ups_cost)
+
+    order.contents.advance
+    final_selected_shipping_rate = order.shipments.first.selected_shipping_rate
+
+    expect(final_selected_shipping_rate.shipping_method).to eq cheap_ups
+    expect(final_selected_shipping_rate.cost).to eq cheap_ups_cost
+  end
+end
diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb
index 6891b1ec19d..05b83d00972 100644
--- a/core/spec/models/spree/order_spec.rb
+++ b/core/spec/models/spree/order_spec.rb
@@ -268,22 +268,11 @@ def merge!(other_order, user = nil)
           order.shipments.create!
         end
 
-        it "destroys current shipments" do
-          order.ensure_updated_shipments
-          expect(order.shipments).to be_empty
-        end
-
         it "puts order back in address state" do
           order.ensure_updated_shipments
           expect(order.state).to eql "cart"
         end
 
-        it "resets shipment_total" do
-          order.update_column(:shipment_total, 5)
-          order.ensure_updated_shipments
-          expect(order.shipment_total).to eq(0)
-        end
-
         it "does nothing if any shipments are ready" do
           shipment = create(:shipment, order: subject, state: "ready")
           expect { subject.ensure_updated_shipments }.not_to change { subject.reload.shipments }
@@ -305,17 +294,6 @@ def merge!(other_order, user = nil)
         order.shipments.create!
       end
 
-      it "destroys current shipments" do
-        order.ensure_updated_shipments
-        expect(order.shipments).to be_empty
-      end
-
-      it "resets shipment_total" do
-        order.update_column(:shipment_total, 5)
-        order.ensure_updated_shipments
-        expect(order.shipment_total).to eq(0)
-      end
-
       it "puts the order in the cart state" do
         order.ensure_updated_shipments
         expect(order.state).to eq "cart"
diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb
index 47131ce7b9f..f0363c0f800 100644
--- a/core/spec/models/spree/shipment_spec.rb
+++ b/core/spec/models/spree/shipment_spec.rb
@@ -184,11 +184,11 @@
       before { allow(shipment).to receive(:can_get_rates?){ true } }
 
       it 'should request new rates, and maintain shipping_method selection' do
-        expect(Spree::Stock::Estimator).to receive(:new).with(shipment.order).and_return(mock_estimator)
-        allow(shipment).to receive_messages(shipping_method: shipping_method2)
+        # select more expensive shipping_method2
+        shipping_method2_rate = shipment.add_shipping_method(shipping_method2)
+        shipment.selected_shipping_rate_id = shipping_method2_rate.id
 
-        expect(shipment.refresh_rates).to eq(shipping_rates)
-        expect(shipment.reload.selected_shipping_rate.shipping_method_id).to eq(shipping_method2.id)
+        expect(shipment.selected_shipping_rate.shipping_method_id).to eq(shipping_method2.id)
       end
 
       it 'should handle no shipping_method selection' do
diff --git a/core/spec/models/spree/stock/estimator_spec.rb b/core/spec/models/spree/stock/estimator_spec.rb
index 112fe05caca..2399821740c 100644
--- a/core/spec/models/spree/stock/estimator_spec.rb
+++ b/core/spec/models/spree/stock/estimator_spec.rb
@@ -4,7 +4,7 @@ module Spree
   module Stock
     describe Estimator, :type => :model do
       let!(:shipping_method) { create(:shipping_method) }
-      let(:package) { build(:stock_package, contents: inventory_units.map { |i| ContentItem.new(inventory_unit) }) }
+      let(:package) { build(:stock_package, contents: inventory_units.map { |i| ContentItem.new(i) }) }
       let(:order) { build(:order_with_line_items) }
       let(:inventory_units) { order.inventory_units }
 
@@ -101,6 +101,21 @@ module Stock
 
             subject.shipping_rates(package)
           end
+
+          context "user has a selected shipping rate" do
+            let(:order) { create(:order_with_line_items, select_shipping_rate: true) }
+
+            before do
+              #need to override from parent context
+              allow(package).to receive_messages(:shipping_methods => Spree::ShippingMethod.all.to_a)
+            end
+
+            it "preserves the user's choice" do
+              preselected_rate = order.shipments.first.selected_shipping_rate
+              rates = subject.shipping_rates(package)
+              expect(rates.detect(&:selected).shipping_method_id).to eq preselected_rate.shipping_method_id
+            end
+          end
         end
 
         context "involves backend only shipping methods" do

From 84680845cb07b0866f818527346ad527edd41cc8 Mon Sep 17 00:00:00 2001
From: Greg <gvaughn@bonobos.com>
Date: Thu, 7 Jan 2016 16:08:48 -0600
Subject: [PATCH 2/2] rename and comment ensure_updated_shipments

---
 core/app/models/spree/order.rb                | 19 +++++++----
 core/app/models/spree/order_contents.rb       |  4 +--
 core/spec/models/spree/order_contents_spec.rb | 20 ++++++------
 core/spec/models/spree/order_spec.rb          | 32 ++++---------------
 4 files changed, 30 insertions(+), 45 deletions(-)

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index d9727e6c779..6129a93c76c 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -508,17 +508,22 @@ def apply_free_shipping_promotions
       persist_totals
     end
 
-    # Clean shipments and make order back to address state
-    #
-    # At some point the might need to force the order to transition from address
-    # to delivery again so that proper updated shipments are created.
-    # e.g. customer goes back from payment step and changes order items
-    def ensure_updated_shipments
+    # This method used to be known as ensure_updated_shipments but it stopped
+    # messing with shipments in order to preserve a user's selected shipping method.
+    # That exposed some oddness, which the new name represents.
+    # As of this writing, this method is only called from OrderContents, but is
+    # called every time the order is modified. Which means that we're almost
+    # always restarting the checkout flow at the 'cart' state for every modification
+    # to a pre-completed order.
+    # TODO understand this at a coarser grain and improve it
+    def probably_restart_checkout_flow
       if !completed? && shipments.all?(&:pending?)
+        # NOTE: if there are no shipments, [].all?(&:pending?) is true
+        # which is probably not what we want
         restart_checkout_flow
       end
-
     end
+    alias :ensure_updated_shipments :probably_restart_checkout_flow
 
     def restart_checkout_flow
       return if self.state == 'cart'
diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb
index 9d3610162b1..a166ab13163 100644
--- a/core/app/models/spree/order_contents.rb
+++ b/core/app/models/spree/order_contents.rb
@@ -30,7 +30,7 @@ def update_cart(params)
           # promotion rules would not be triggered.
           reload_totals
           PromotionHandler::Cart.new(order).activate
-          order.ensure_updated_shipments
+          order.probably_restart_checkout_flow
         end
         reload_totals
         true
@@ -60,7 +60,7 @@ def approve(user: nil, name: nil)
       def after_add_or_remove(line_item, options = {})
         reload_totals
         shipment = options[:shipment]
-        shipment.present? ? shipment.update_amounts : order.ensure_updated_shipments
+        shipment.present? ? shipment.update_amounts : order.probably_restart_checkout_flow
         PromotionHandler::Cart.new(order, line_item).activate
         ItemAdjustments.new(line_item).update
         reload_totals
diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb
index d5a3e4e470f..3010c5f6416 100644
--- a/core/spec/models/spree/order_contents_spec.rb
+++ b/core/spec/models/spree/order_contents_spec.rb
@@ -18,9 +18,9 @@
     end
 
     context 'given a shipment' do
-      it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do
+      it "ensure shipment calls update_amounts instead of order calling probably_restart_checkout_flow" do
         shipment = create(:shipment)
-        expect(subject.order).to_not receive(:ensure_updated_shipments)
+        expect(subject.order).to_not receive(:probably_restart_checkout_flow)
         expect(shipment).to receive(:update_amounts)
         subject.add(variant, 1, shipment: shipment)
       end
@@ -28,7 +28,7 @@
 
     context 'not given a shipment' do
       it "ensures updated shipments" do
-        expect(subject.order).to receive(:ensure_updated_shipments)
+        expect(subject.order).to receive(:probably_restart_checkout_flow)
         subject.add(variant)
       end
     end
@@ -116,10 +116,10 @@
     end
 
     context 'given a shipment' do
-      it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do
+      it "ensure shipment calls update_amounts instead of order calling probably_restart_checkout_flow" do
         line_item = subject.add(variant, 1)
         shipment = create(:shipment)
-        expect(subject.order).to_not receive(:ensure_updated_shipments)
+        expect(subject.order).to_not receive(:probably_restart_checkout_flow)
         expect(shipment).to receive(:update_amounts)
         subject.remove(variant, 1, shipment: shipment)
       end
@@ -128,7 +128,7 @@
     context 'not given a shipment' do
       it "ensures updated shipments" do
         line_item = subject.add(variant, 1)
-        expect(subject.order).to receive(:ensure_updated_shipments)
+        expect(subject.order).to receive(:probably_restart_checkout_flow)
         subject.remove(variant)
       end
     end
@@ -164,10 +164,10 @@
 
   context "#remove_line_item" do
     context 'given a shipment' do
-      it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do
+      it "ensure shipment calls update_amounts instead of order calling probably_restart_checkout_flow" do
         line_item = subject.add(variant, 1)
         shipment = create(:shipment)
-        expect(subject.order).to_not receive(:ensure_updated_shipments)
+        expect(subject.order).to_not receive(:probably_restart_checkout_flow)
         expect(shipment).to receive(:update_amounts)
         subject.remove_line_item(line_item, shipment: shipment)
       end
@@ -176,7 +176,7 @@
     context 'not given a shipment' do
       it "ensures updated shipments" do
         line_item = subject.add(variant, 1)
-        expect(subject.order).to receive(:ensure_updated_shipments)
+        expect(subject.order).to receive(:probably_restart_checkout_flow)
         subject.remove_line_item(line_item)
       end
     end
@@ -239,7 +239,7 @@
     end
 
     it "ensures updated shipments" do
-      expect(subject.order).to receive(:ensure_updated_shipments)
+      expect(subject.order).to receive(:probably_restart_checkout_flow)
       subject.update_cart params
     end
   end
diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb
index 05b83d00972..bb42443953d 100644
--- a/core/spec/models/spree/order_spec.rb
+++ b/core/spec/models/spree/order_spec.rb
@@ -269,19 +269,19 @@ def merge!(other_order, user = nil)
         end
 
         it "puts order back in address state" do
-          order.ensure_updated_shipments
+          order.probably_restart_checkout_flow
           expect(order.state).to eql "cart"
         end
 
         it "does nothing if any shipments are ready" do
           shipment = create(:shipment, order: subject, state: "ready")
-          expect { subject.ensure_updated_shipments }.not_to change { subject.reload.shipments }
+          expect { subject.probably_restart_checkout_flow }.not_to change { subject.reload.shipments }
           expect { shipment.reload }.not_to raise_error
         end
 
         it "does nothing if any shipments are shipped" do
           shipment = create(:shipment, order: subject, state: "shipped")
-          expect { subject.ensure_updated_shipments }.not_to change { subject.reload.shipments }
+          expect { subject.probably_restart_checkout_flow }.not_to change { subject.reload.shipments }
           expect { shipment.reload }.not_to raise_error
         end
       end
@@ -295,7 +295,7 @@ def merge!(other_order, user = nil)
       end
 
       it "puts the order in the cart state" do
-        order.ensure_updated_shipments
+        order.probably_restart_checkout_flow
         expect(order.state).to eq "cart"
       end
     end
@@ -308,21 +308,9 @@ def merge!(other_order, user = nil)
         order.shipments.create!
       end
 
-      it "does not destroy the current shipments" do
-        expect {
-          order.ensure_updated_shipments
-        }.not_to change { order.shipments }
-      end
-
-      it "does not reset the shipment total" do
-        expect {
-          order.ensure_updated_shipments
-        }.not_to change { order.shipment_total }
-      end
-
       it "does not put the order back in the address state" do
         expect {
-          order.ensure_updated_shipments
+          order.probably_restart_checkout_flow
         }.not_to change { order.state }
       end
     end
@@ -334,15 +322,7 @@ def merge!(other_order, user = nil)
         order.shipments.create!
 
         expect {
-          order.ensure_updated_shipments
-        }.not_to change { order.shipment_total }
-
-        expect {
-          order.ensure_updated_shipments
-        }.not_to change { order.shipments }
-
-        expect {
-          order.ensure_updated_shipments
+          order.probably_restart_checkout_flow
         }.not_to change { order.state }
       end
     end