preserve shipping method when shipments change

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

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) }
 

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
 
@@ -509,19 +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?)
-        self.shipments.destroy_all
-        self.update_column(:shipment_total, 0)
+        # 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'
@@ -530,6 +532,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
 

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

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

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
 

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!

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

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)

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

core/spec/models/spree/order_contents_spec.rb

@@ -18,17 +18,17 @@
     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
     end
 
     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

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