From a62a2cb52f03df21d659309f148f87f647abbd33 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 22 Nov 2018 16:49:29 +0100 Subject: [PATCH 1/6] Extract #restart_checkout to a service --- app/controllers/checkout_controller.rb | 12 ++++-------- app/services/restart_checkout.rb | 15 +++++++++++++++ spec/controllers/checkout_controller_spec.rb | 13 +++++++++++-- 3 files changed, 30 insertions(+), 10 deletions(-) create mode 100644 app/services/restart_checkout.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 8cba0d8a0e..162f3c529f 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -18,7 +18,7 @@ class CheckoutController < Spree::CheckoutController # This is only required because of spree_paypal_express. If we implement # a version of paypal that uses this controller, and more specifically # the #update_failed method, then we can remove this call - restart_checkout + RestartCheckout.new(@order).restart_checkout end def update @@ -139,7 +139,8 @@ class CheckoutController < Spree::CheckoutController def update_failed clear_ship_address - restart_checkout + RestartCheckout.new(@order).restart_checkout + respond_to do |format| format.html do render :edit @@ -159,12 +160,7 @@ class CheckoutController < Spree::CheckoutController end def restart_checkout - return if @order.state == 'cart' - @order.restart_checkout! # resets state to 'cart' - @order.update_attributes!(shipping_method_id: nil) - @order.shipments.with_state(:pending).destroy_all - @order.payments.with_state(:checkout).destroy_all - @order.reload + RestartCheckout.new(@order).restart_checkout end def skip_state_validation? diff --git a/app/services/restart_checkout.rb b/app/services/restart_checkout.rb new file mode 100644 index 0000000000..d7dd10520b --- /dev/null +++ b/app/services/restart_checkout.rb @@ -0,0 +1,15 @@ +# Resets the passed order to cart state while clearing associated payments and shipments +class RestartCheckout + def initialize(order) + @order = order + end + + def restart_checkout + return if @order.state == 'cart' + @order.restart_checkout! # resets state to 'cart' + @order.update_attributes!(shipping_method_id: nil) + @order.shipments.with_state(:pending).destroy_all + @order.payments.with_state(:checkout).destroy_all + @order.reload + end +end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index b373ae64b5..a202de7dec 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -189,11 +189,14 @@ describe CheckoutController, type: :controller do describe "Paypal routing" do let(:payment_method) { create(:payment_method, type: "Spree::Gateway::PayPalExpress") } + let(:restart_checkout) { instance_double(RestartCheckout, restart_checkout: true) } + before do allow(controller).to receive(:current_distributor) { distributor } allow(controller).to receive(:current_order_cycle) { order_cycle } allow(controller).to receive(:current_order) { order } - allow(controller).to receive(:restart_checkout) + + allow(RestartCheckout).to receive(:new) { restart_checkout } end it "should check the payment method for Paypalness if we've selected one" do @@ -205,14 +208,20 @@ describe CheckoutController, type: :controller do end describe "#update_failed" do + let(:restart_checkout) do + instance_double(RestartCheckout, restart_checkout: true) + end + before do controller.instance_variable_set(:@order, order) + allow(RestartCheckout).to receive(:new) { restart_checkout } end it "clears the shipping address and restarts the checkout" do expect(controller).to receive(:clear_ship_address) - expect(controller).to receive(:restart_checkout) + expect(restart_checkout).to receive(:restart_checkout) expect(controller).to receive(:respond_to) + controller.send(:update_failed) end end From 3b681a59bafcb4400f16ab1db19386008f4f5ee1 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Nov 2018 11:43:10 +0100 Subject: [PATCH 2/6] Move controller tests to service class tests --- app/controllers/checkout_controller.rb | 4 -- spec/controllers/checkout_controller_spec.rb | 42 ----------------- spec/services/restart_checkout_spec.rb | 48 ++++++++++++++++++++ 3 files changed, 48 insertions(+), 46 deletions(-) create mode 100644 spec/services/restart_checkout_spec.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 162f3c529f..59017f7ed5 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -159,10 +159,6 @@ class CheckoutController < Spree::CheckoutController end end - def restart_checkout - RestartCheckout.new(@order).restart_checkout - end - def skip_state_validation? true end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index a202de7dec..59a08811f9 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -225,46 +225,4 @@ describe CheckoutController, type: :controller do controller.send(:update_failed) end end - - describe "#restart_checkout" do - let!(:shipment_pending) { create(:shipment, order: order, state: 'pending') } - let!(:payment_checkout) { create(:payment, order: order, state: 'checkout') } - let!(:payment_failed) { create(:payment, order: order, state: 'failed') } - - before do - order.update_attribute(:shipping_method_id, shipment_pending.shipping_method_id) - controller.instance_variable_set(:@order, order.reload) - end - - context "when the order is already in the 'cart' state" do - it "does nothing" do - expect(order).to_not receive(:restart_checkout!) - controller.send(:restart_checkout) - end - end - - context "when the order is in a subsequent state" do - before 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 - expect(order).to receive(:restart_checkout!).and_call_original - expect(order.shipping_method_id).to_not be nil - expect(order.shipments.count).to be 1 - expect(order.adjustments.shipping.count).to be 1 - expect(order.payments.count).to be 2 - expect(order.adjustments.payment_fee.count).to be 2 - controller.send(:restart_checkout) - expect(order.reload.state).to eq 'cart' - expect(order.shipping_method_id).to be nil - expect(order.shipments.count).to be 0 - expect(order.adjustments.shipping.count).to be 0 - expect(order.payments.count).to be 1 - expect(order.adjustments.payment_fee.count).to be 1 - end - end - end end diff --git a/spec/services/restart_checkout_spec.rb b/spec/services/restart_checkout_spec.rb new file mode 100644 index 0000000000..17db99ace1 --- /dev/null +++ b/spec/services/restart_checkout_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe RestartCheckout do + let(:order) { create(:order) } + + describe "#restart_checkout" do + let!(:shipment_pending) { create(:shipment, order: order, state: 'pending') } + let!(:payment_checkout) { create(:payment, order: order, state: 'checkout') } + let!(:payment_failed) { create(:payment, order: order, state: 'failed') } + + before do + order.update_attribute(:shipping_method_id, shipment_pending.shipping_method_id) + end + + context "when the order is already in the 'cart' state" do + it "does nothing" do + expect(order).to_not receive(:restart_checkout!) + RestartCheckout.new(order).restart_checkout + end + end + + context "when the order is in a subsequent state" do + before 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 + expect(order).to receive(:restart_checkout!).and_call_original + expect(order.shipping_method_id).to_not be nil + expect(order.shipments.count).to be 1 + expect(order.adjustments.shipping.count).to be 1 + expect(order.payments.count).to be 2 + expect(order.adjustments.payment_fee.count).to be 2 + + RestartCheckout.new(order).restart_checkout + + expect(order.reload.state).to eq 'cart' + expect(order.shipping_method_id).to be nil + expect(order.shipments.count).to be 0 + expect(order.adjustments.shipping.count).to be 0 + expect(order.payments.count).to be 1 + expect(order.adjustments.payment_fee.count).to be 1 + end + end + end +end From 99463427cebe00ff48d2ca4ccc3c51f2f9f626d4 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Nov 2018 11:46:57 +0100 Subject: [PATCH 3/6] Refactor RestartCheckout service just extracted --- app/services/restart_checkout.rb | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/app/services/restart_checkout.rb b/app/services/restart_checkout.rb index d7dd10520b..9bbd476197 100644 --- a/app/services/restart_checkout.rb +++ b/app/services/restart_checkout.rb @@ -5,11 +5,29 @@ class RestartCheckout end def restart_checkout - return if @order.state == 'cart' - @order.restart_checkout! # resets state to 'cart' - @order.update_attributes!(shipping_method_id: nil) - @order.shipments.with_state(:pending).destroy_all - @order.payments.with_state(:checkout).destroy_all - @order.reload + return if order.cart? + + reset_state_to_cart + clear_shipments + clear_payments + + order.reload + end + + private + + attr_reader :order + + def reset_state_to_cart + order.restart_checkout! + end + + def clear_shipments + order.update_attributes!(shipping_method_id: nil) + order.shipments.with_state(:pending).destroy_all + end + + def clear_payments + order.payments.with_state(:checkout).destroy_all end end From 2c8a1f5e78d9bb78e495fb5a666a0f747d2190cc Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Nov 2018 11:51:23 +0100 Subject: [PATCH 4/6] Check for value and not object identity in spec --- spec/services/restart_checkout_spec.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/services/restart_checkout_spec.rb b/spec/services/restart_checkout_spec.rb index 17db99ace1..e1cd40a355 100644 --- a/spec/services/restart_checkout_spec.rb +++ b/spec/services/restart_checkout_spec.rb @@ -28,20 +28,20 @@ describe RestartCheckout do # '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 expect(order).to receive(:restart_checkout!).and_call_original - expect(order.shipping_method_id).to_not be nil - expect(order.shipments.count).to be 1 - expect(order.adjustments.shipping.count).to be 1 - expect(order.payments.count).to be 2 - expect(order.adjustments.payment_fee.count).to be 2 + expect(order.shipping_method_id).to_not eq nil + expect(order.shipments.count).to eq 1 + expect(order.adjustments.shipping.count).to eq 1 + expect(order.payments.count).to eq 2 + expect(order.adjustments.payment_fee.count).to eq 2 RestartCheckout.new(order).restart_checkout expect(order.reload.state).to eq 'cart' - expect(order.shipping_method_id).to be nil - expect(order.shipments.count).to be 0 - expect(order.adjustments.shipping.count).to be 0 - expect(order.payments.count).to be 1 - expect(order.adjustments.payment_fee.count).to be 1 + expect(order.shipping_method_id).to eq nil + expect(order.shipments.count).to eq 0 + expect(order.adjustments.shipping.count).to eq 0 + expect(order.payments.count).to eq 1 + expect(order.adjustments.payment_fee.count).to eq 1 end end end From 88afa70f37cafbd0f3657efb0471db93e5a1a5a9 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Nov 2018 11:56:48 +0100 Subject: [PATCH 5/6] Simplify and speed up service tests --- spec/services/restart_checkout_spec.rb | 27 +++++++++----------------- 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/spec/services/restart_checkout_spec.rb b/spec/services/restart_checkout_spec.rb index e1cd40a355..56451f8b15 100644 --- a/spec/services/restart_checkout_spec.rb +++ b/spec/services/restart_checkout_spec.rb @@ -4,14 +4,6 @@ describe RestartCheckout do let(:order) { create(:order) } describe "#restart_checkout" do - let!(:shipment_pending) { create(:shipment, order: order, state: 'pending') } - let!(:payment_checkout) { create(:payment, order: order, state: 'checkout') } - let!(:payment_failed) { create(:payment, order: order, state: 'failed') } - - before do - order.update_attribute(:shipping_method_id, shipment_pending.shipping_method_id) - end - context "when the order is already in the 'cart' state" do it "does nothing" do expect(order).to_not receive(:restart_checkout!) @@ -20,23 +12,22 @@ describe RestartCheckout do end context "when the order is in a subsequent state" do + let!(:shipment_pending) { create(:shipment, order: order, state: 'pending') } + let!(:payment_checkout) { create(:payment, order: order, state: 'checkout') } + let!(:payment_failed) { create(:payment, order: order, state: 'failed') } + before do + order.shipping_method_id = shipment_pending.shipping_method_id 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. + # 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 - expect(order).to receive(:restart_checkout!).and_call_original - expect(order.shipping_method_id).to_not eq nil - expect(order.shipments.count).to eq 1 - expect(order.adjustments.shipping.count).to eq 1 - expect(order.payments.count).to eq 2 - expect(order.adjustments.payment_fee.count).to eq 2 - RestartCheckout.new(order).restart_checkout - expect(order.reload.state).to eq 'cart' + expect(order.state).to eq 'cart' expect(order.shipping_method_id).to eq nil expect(order.shipments.count).to eq 0 expect(order.adjustments.shipping.count).to eq 0 From 453b2a99de4b1989dc473386613a53188c2ce82e Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 23 Nov 2018 12:00:45 +0100 Subject: [PATCH 6/6] Rename redundant #restart_checkout to #call --- app/controllers/checkout_controller.rb | 4 ++-- app/services/restart_checkout.rb | 2 +- spec/controllers/checkout_controller_spec.rb | 8 +++----- spec/services/restart_checkout_spec.rb | 6 +++--- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 59017f7ed5..3ffe896db1 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -18,7 +18,7 @@ class CheckoutController < Spree::CheckoutController # This is only required because of spree_paypal_express. If we implement # a version of paypal that uses this controller, and more specifically # the #update_failed method, then we can remove this call - RestartCheckout.new(@order).restart_checkout + RestartCheckout.new(@order).call end def update @@ -139,7 +139,7 @@ class CheckoutController < Spree::CheckoutController def update_failed clear_ship_address - RestartCheckout.new(@order).restart_checkout + RestartCheckout.new(@order).call respond_to do |format| format.html do diff --git a/app/services/restart_checkout.rb b/app/services/restart_checkout.rb index 9bbd476197..79ef9acdb5 100644 --- a/app/services/restart_checkout.rb +++ b/app/services/restart_checkout.rb @@ -4,7 +4,7 @@ class RestartCheckout @order = order end - def restart_checkout + def call return if order.cart? reset_state_to_cart diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 59a08811f9..43c937a892 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -189,7 +189,7 @@ describe CheckoutController, type: :controller do describe "Paypal routing" do let(:payment_method) { create(:payment_method, type: "Spree::Gateway::PayPalExpress") } - let(:restart_checkout) { instance_double(RestartCheckout, restart_checkout: true) } + let(:restart_checkout) { instance_double(RestartCheckout, call: true) } before do allow(controller).to receive(:current_distributor) { distributor } @@ -208,9 +208,7 @@ describe CheckoutController, type: :controller do end describe "#update_failed" do - let(:restart_checkout) do - instance_double(RestartCheckout, restart_checkout: true) - end + let(:restart_checkout) { instance_double(RestartCheckout, call: true) } before do controller.instance_variable_set(:@order, order) @@ -219,7 +217,7 @@ describe CheckoutController, type: :controller do it "clears the shipping address and restarts the checkout" do expect(controller).to receive(:clear_ship_address) - expect(restart_checkout).to receive(:restart_checkout) + expect(restart_checkout).to receive(:call) expect(controller).to receive(:respond_to) controller.send(:update_failed) diff --git a/spec/services/restart_checkout_spec.rb b/spec/services/restart_checkout_spec.rb index 56451f8b15..1337e2c581 100644 --- a/spec/services/restart_checkout_spec.rb +++ b/spec/services/restart_checkout_spec.rb @@ -3,11 +3,11 @@ require 'spec_helper' describe RestartCheckout do let(:order) { create(:order) } - describe "#restart_checkout" do + describe "#call" do context "when the order is already in the 'cart' state" do it "does nothing" do expect(order).to_not receive(:restart_checkout!) - RestartCheckout.new(order).restart_checkout + RestartCheckout.new(order).call end end @@ -25,7 +25,7 @@ describe RestartCheckout do # 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).restart_checkout + RestartCheckout.new(order).call expect(order.state).to eq 'cart' expect(order.shipping_method_id).to eq nil