Skip to content

Commit

Permalink
Remove unnecessary shipping rates callback
Browse files Browse the repository at this point in the history
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, 7ba53b2, 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.
  • Loading branch information
mamhoff committed May 13, 2017
1 parent 9880679 commit 97a01b9
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 45 deletions.
3 changes: 0 additions & 3 deletions core/app/models/spree/order/checkout.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 0 additions & 16 deletions core/spec/models/spree/order/checkout_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
31 changes: 5 additions & 26 deletions frontend/spec/controllers/spree/checkout_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 97a01b9

Please sign in to comment.