From 97a01b96fbdc103965a37962d40df6fea814bbf1 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <martin@stembolt.com>
Date: Sat, 13 May 2017 22:27:13 +0200
Subject: [PATCH] Remove unnecessary shipping rates callback

The callback `Spree::Order#ensure_available_shipping_rates` was
explicitly written to be run before delivery. Running it before the
`complete` step leaves the order in the `confirm` state, but without
any shipments, and results in an error message does not make sense
in that context ("We were unable to calculate shipping rates"). In fact,
we have not even tried.

It's almost impossible to setup an order such that this callback would
actually trigger in a standard Solidus installation.

The commit that introduced the callback, 7ba53b2c, intends to move line
item availability validations to the before complete callback, instead
of "all the time". That makes sense, but this callback has somehow sneaked
in there without any explanation of why its necessary.

I delete a model spec that was introduced in e7450ec, testing the behaviour
in question. The spec set the order up in a way that a normal checkout never would.
If you get through delivery and payment to the confirm step, and then
do literally anything to the order, it will be reset to either cart or address
state.

I edit another spec to test what the callback should actually do: Alert the user
that for their recently entered shipping address, no shipping rates can be calculated.
---
 core/app/models/spree/order/checkout.rb       |  3 --
 core/spec/models/spree/order/checkout_spec.rb | 16 ----------
 .../spree/checkout_controller_spec.rb         | 31 +++----------------
 3 files changed, 5 insertions(+), 45 deletions(-)

diff --git a/core/app/models/spree/order/checkout.rb b/core/app/models/spree/order/checkout.rb
index dd21bb5c8fb..e4c6ac8a6f4 100644
--- a/core/app/models/spree/order/checkout.rb
+++ b/core/app/models/spree/order/checkout.rb
@@ -108,9 +108,6 @@ def define_state_machine!
             # calls matter so that we do not process payments
             # until validations have passed
             before_transition to: :complete, do: :validate_line_item_availability
-            if states[:delivery]
-              before_transition to: :complete, do: :ensure_available_shipping_rates
-            end
             before_transition to: :complete, do: :ensure_promotions_eligible
             before_transition to: :complete, do: :ensure_line_item_variants_are_not_deleted
             before_transition to: :complete, do: :ensure_inventory_units
diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb
index 3966c566d62..f0e0d005c5e 100644
--- a/core/spec/models/spree/order/checkout_spec.rb
+++ b/core/spec/models/spree/order/checkout_spec.rb
@@ -589,22 +589,6 @@ def assert_state_changed(order, from, to)
       end
     end
 
-    context 'a shipment has no shipping rates' do
-      let(:order) { create(:order_with_line_items, state: 'confirm') }
-      let(:shipment) { order.shipments.first }
-
-      before do
-        shipment.shipping_rates.destroy_all
-      end
-
-      it 'clears the shipments and fails the transition' do
-        expect(order.complete).to eq(false)
-        expect(order.errors[:base]).to include(Spree.t(:items_cannot_be_shipped))
-        expect(order.shipments.count).to eq(0)
-        expect(Spree::InventoryUnit.where(shipment_id: shipment.id).count).to eq(0)
-      end
-    end
-
     context 'the order is already paid' do
       let(:order) { create(:order_with_line_items) }
 
diff --git a/frontend/spec/controllers/spree/checkout_controller_spec.rb b/frontend/spec/controllers/spree/checkout_controller_spec.rb
index 50dd22604a7..863972753a6 100644
--- a/frontend/spec/controllers/spree/checkout_controller_spec.rb
+++ b/frontend/spec/controllers/spree/checkout_controller_spec.rb
@@ -323,39 +323,18 @@ def post_address
           expect(response).to redirect_to(spree.checkout_state_path('address'))
         end
       end
-    end
-
-    context "fails to transition to complete from confirm" do
-      let(:order) do
-        FactoryGirl.create(:order_with_line_items).tap(&:next!)
-      end
-
-      before do
-        allow(controller).to receive_messages current_order: order
-        allow(controller).to receive_messages check_authorization: true
-      end
 
       context "when the country is not a shippable country" do
-        before do
-          order.ship_address.tap do |address|
-            # A different country which is not included in the list of shippable countries
-            australia = create(:country, name: "Australia")
-            # update_columns to get around readonly restriction when testing
-            address.update_columns(country_id: australia.id, state_name: 'Victoria')
-          end
+        let(:foreign_address) { create(:address, country_iso_code: "CA") }
 
-          payment_method = FactoryGirl.create(:simple_credit_card_payment_method)
-          payment = FactoryGirl.create(:payment, payment_method: payment_method)
-          order.payments << payment
+        before do
+          order.update(shipping_address: foreign_address)
         end
 
         it "due to no available shipping rates for any of the shipments" do
-          expect(order.shipments.count).to eq(1)
-          order.shipments.first.shipping_rates.delete_all
-          order.update_attributes(state: 'confirm')
-          put :update, params: { state: order.state, order: {} }
+          put :update, params: { state: "address", order: {} }
           expect(flash[:error]).to eq(Spree.t(:items_cannot_be_shipped))
-          expect(response).to redirect_to(spree.checkout_state_path('confirm'))
+          expect(response).to redirect_to(spree.checkout_state_path('address'))
         end
       end
     end