From c4d5eec7fd983191c00264266d26479ea8ac8280 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 9 Sep 2019 17:46:07 +0100 Subject: [PATCH 1/3] Covering restart_checkout code with more tests to clarify behaviour with different order.ship_address objects The edge case here is when ship_address is present but empty, on the checkout_controller we are going to move from using an empty ship_address to using a non-empty one. We keep the original case where this spec was testing with a nil order.ship_address --- spec/services/restart_checkout_spec.rb | 44 ++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/spec/services/restart_checkout_spec.rb b/spec/services/restart_checkout_spec.rb index 2413408994..f0514c9b5f 100644 --- a/spec/services/restart_checkout_spec.rb +++ b/spec/services/restart_checkout_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe RestartCheckout do - let(:order) { create(:order) } + let(:order) { create(:order_with_distributor) } describe "#call" do context "when the order is already in the 'cart' state" do @@ -12,6 +12,7 @@ describe RestartCheckout do end context "when the order is in a subsequent state" do + # 'pending' is the only shipment state possible for incomplete orders let!(:shipment_pending) { create(:shipment, order: order, state: 'pending') } let!(:payment_failed) { create(:payment, order: order, state: 'failed') } let!(:payment_checkout) { create(:payment, order: order, state: 'checkout') } @@ -20,12 +21,43 @@ describe RestartCheckout do order.update_attribute(:state, "payment") end - # NOTE: at the time of writing, it was not possible to create a shipment - # with a state other than 'pending' when the order has not been - # completed, so this is not a case that requires testing. - it "resets the order state, and clears incomplete shipments and payments" do - RestartCheckout.new(order).call + context "when order ship address is nil" do + before { order.ship_address = nil } + it "resets the order state, and clears incomplete shipments and payments" do + RestartCheckout.new(order).call + + expect_cart_state_and_reset_adjustments + end + end + + context "when order ship address is not empty" do + before { order.ship_address = order.address_from_distributor } + + it "resets the order state, and clears incomplete shipments and payments" do + RestartCheckout.new(order).call + + expect_cart_state_and_reset_adjustments + end + end + + context "when order ship address is empty" do + before { order.ship_address = Spree::Address.default } + + it "does not reset the order state nor clears incomplete shipments and payments" do + expect do + RestartCheckout.new(order).call + end.to raise_error(StateMachine::InvalidTransition) + + expect(order.state).to eq 'payment' + expect(order.shipments.count).to eq 1 + expect(order.adjustments.shipping.count).to eq 0 + expect(order.payments.count).to eq 2 + expect(order.adjustments.payment_fee.count).to eq 2 + end + end + + def expect_cart_state_and_reset_adjustments expect(order.state).to eq 'cart' expect(order.shipments.count).to eq 0 expect(order.adjustments.shipping.count).to eq 0 From cc7461e69278c8ff99481688f4654a0b74beec0d Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 9 Sep 2019 16:58:47 +0100 Subject: [PATCH 2/3] Remove clear_ship_address from checkout_controller because it is setting an empty address on order.ship_address which is breaking the reset_checkout process in some cases. This logic is already repeated in the before_save hook in the OrderUpdater where the distributor address is put into the ship_address on order.finalize In cases the order is not to be finalized we keep the ship_address sent from the client as we may need it to make the order workflow work properly --- app/controllers/checkout_controller.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index c1fe12ce53..240e736645 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -152,7 +152,6 @@ class CheckoutController < Spree::CheckoutController end def update_failed - clear_ship_address RestartCheckout.new(@order).call respond_to do |format| @@ -165,15 +164,6 @@ class CheckoutController < Spree::CheckoutController end end - # When we have a pickup Shipping Method, - # we clone the distributor address into ship_address before_save - # We don't want this data in the form, so we clear it out - def clear_ship_address - unless current_order.shipping_method.andand.require_ship_address - current_order.ship_address = Spree::Address.default - end - end - def load_order @order = current_order redirect_to(main_app.shop_path) && return unless @order && @order.checkout_allowed? From fb65c64c6855f92341aedaffac6615a6870a9b74 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 9 Sep 2019 17:43:16 +0100 Subject: [PATCH 3/3] Keep the ship_address clearing logic (this time reusing the OrderUpdate method) thus making the restart_checkout process work for these cases (because order.ship_address is not empty) OrderUpdater#shipping_address_from_distributor uses order.address_from_distributor to set order.ship_address when order is not delivery: this will clear the ship address as it was done previously without setting an empty address like Spree::Address.default --- app/controllers/checkout_controller.rb | 1 + spec/controllers/checkout_controller_spec.rb | 24 ++++++-------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 240e736645..71e9239523 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -152,6 +152,7 @@ class CheckoutController < Spree::CheckoutController end def update_failed + current_order.updater.shipping_address_from_distributor RestartCheckout.new(@order).call respond_to do |format| diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index ac7f180e2d..b67a23b8ac 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -78,30 +78,19 @@ describe CheckoutController, type: :controller do allow(controller).to receive(:current_order).and_return(order) end - it "does not clone the ship address from distributor when shipping method requires address" do - get :edit - expect(assigns[:order].ship_address.address1).to be_nil - end - - it "clears the ship address when re-rendering edit" do - expect(controller).to receive(:clear_ship_address).and_return true + it "set shipping_address_from_distributor when re-rendering edit" do + expect(order.updater).to receive(:shipping_address_from_distributor) allow(order).to receive(:update_attributes).and_return false spree_post :update, format: :json, order: {} end - it "clears the ship address when the order state cannot be advanced" do - expect(controller).to receive(:clear_ship_address).and_return true + it "set shipping_address_from_distributor when the order state cannot be advanced" do + expect(order.updater).to receive(:shipping_address_from_distributor) allow(order).to receive(:update_attributes).and_return true allow(order).to receive(:next).and_return false spree_post :update, format: :json, order: {} end - it "only clears the ship address with a pickup shipping method" do - allow(order).to receive_message_chain(:shipping_method, :andand, :require_ship_address).and_return false - expect(order).to receive(:ship_address=) - controller.send(:clear_ship_address) - end - context "#update with shipping_method_id" do let(:test_shipping_method_id) { "111" } @@ -266,10 +255,11 @@ describe CheckoutController, type: :controller do before do controller.instance_variable_set(:@order, order) allow(RestartCheckout).to receive(:new) { restart_checkout } + allow(controller).to receive(:current_order) { order } end - it "clears the shipping address and restarts the checkout" do - expect(controller).to receive(:clear_ship_address) + it "set shipping_address_from_distributor and restarts the checkout" do + expect(order.updater).to receive(:shipping_address_from_distributor) expect(restart_checkout).to receive(:call) expect(controller).to receive(:respond_to)