From d0e38c8d105351fe30491416f1186b75a9701830 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 18 Oct 2023 09:36:37 +1100 Subject: [PATCH 1/7] Add order event back_to_payment and back_to_address It gives the possibility to move an order back to payment state or address state when going through the checkout steps --- app/models/spree/order/checkout.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index e76cc248c2..6bd9a5b919 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -71,6 +71,14 @@ module Spree transition to: :complete, from: :confirmation end + event :back_to_payment do + transition to: :payment, from: :confirmation + end + + event :back_to_address do + transition to: :address, from: [:payment, :confirmation] + end + before_transition from: :cart, do: :ensure_line_items_present before_transition to: :delivery, do: :create_proposed_shipments From b09054a76a84f35e3031fdf71dd01d8f29b8f51e Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 18 Oct 2023 09:45:05 +1100 Subject: [PATCH 2/7] Update order state when moving back through checkout step Some important logic happens after the order transition from one state to another. In particular, voucher are recalculated. To fix any inconsistency, we make sure the order is the state matching the checkout step we are on. --- app/controllers/concerns/checkout_steps.rb | 18 ++++++++-- app/controllers/split_checkout_controller.rb | 2 ++ .../split_checkout_controller_spec.rb | 36 +++++++++++++++++++ 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/checkout_steps.rb b/app/controllers/concerns/checkout_steps.rb index 83eca5ed83..556c934166 100644 --- a/app/controllers/concerns/checkout_steps.rb +++ b/app/controllers/concerns/checkout_steps.rb @@ -13,6 +13,10 @@ module CheckoutSteps params[:step] == "payment" end + def details_step? + params[:step] == "details" + end + def redirect_to_step_based_on_order case @order.state when "cart", "address", "delivery" @@ -39,9 +43,19 @@ module CheckoutSteps def check_step case @order.state when "cart", "address", "delivery" - redirect_to checkout_step_path(:details) unless params[:step] == "details" + redirect_to checkout_step_path(:details) unless details_step? when "payment" - redirect_to checkout_step_path(:payment) if params[:step] == "summary" + redirect_to checkout_step_path(:payment) if summary_step? end end + + def update_order_state + if @order.state == "confirmation" && payment_step? + @order.back_to_payment + end + + return unless @order.state.in?(["payment", "confirmation"]) && details_step? + + @order.back_to_address + end end diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 6b1266eaf8..a855774874 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -24,6 +24,8 @@ class SplitCheckoutController < ::BaseController def edit redirect_to_step_based_on_order unless params[:step] + + update_order_state if params[:step] check_step if params[:step] return if available_shipping_methods.any? diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index df46630878..c4ead98e3f 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -67,6 +67,42 @@ describe SplitCheckoutController, type: :controller do expect(response).to redirect_to checkout_step_path(:payment) end end + + context "when order state is 'confirmation'" do + before do + order.update!(state: "confirmation") + end + + context "when loading payment step" do + it "updates the order state to payment" do + get :edit, params: { step: "payment" } + + expect(order.reload.state).to eq("payment") + end + end + + context "when loading address step" do + it "updates the order state to address" do + get :edit, params: { step: "details" } + + expect(order.reload.state).to eq("address") + end + end + end + + context "when order state is 'payment'" do + context "when loading address step" do + before do + order.update!(state: "payment") + end + + it "updates the order state to address" do + get :edit, params: { step: "details" } + + expect(order.reload.state).to eq("address") + end + end + end end end From f9d2deeb1731b8a50990928e63e2a30a4b98c921 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 20 Oct 2023 15:28:26 +1100 Subject: [PATCH 3/7] Small clean up --- spec/controllers/split_checkout_controller_spec.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index c4ead98e3f..afa3d89274 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -8,10 +8,7 @@ describe SplitCheckoutController, type: :controller do let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } let(:order_cycle) { create(:order_cycle, distributors: [distributor]) } let(:exchange) { order_cycle.exchanges.outgoing.first } - let(:order) { - create(:order_with_line_items, line_items_count: 1, distributor:, - order_cycle:) - } + let(:order) { create(:order_with_line_items, line_items_count: 1, distributor:, order_cycle:) } let(:payment_method) { distributor.payment_methods.first } let(:shipping_method) { distributor.shipping_methods.first } @@ -211,7 +208,7 @@ describe SplitCheckoutController, type: :controller do } end - it "updates and redirects to payment step" do + it "updates and redirects to summary step" do put(:update, params:) expect(response).to redirect_to checkout_step_path(:summary) From 5ba21e486a60bb325dd52286cc783c534b7f2117 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 20 Oct 2023 15:29:29 +1100 Subject: [PATCH 4/7] Recalculate voucher voucher adjusment when needed We need to recalculate the voucher adjustment(s) in the following scenarii : When a voucher as been added to the order and : * Moving to the payment step from details step, to take into account potential change in shipment fees. (this happen if we return to the details step after reaching the summary step) * Moving to the summary step from payment step, to take into account payment fees --- app/controllers/split_checkout_controller.rb | 12 +++ app/models/spree/order/checkout.rb | 5 -- .../split_checkout_controller_spec.rb | 75 +++++++++++++++++++ 3 files changed, 87 insertions(+), 5 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index a855774874..67d52c496a 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -98,11 +98,23 @@ class SplitCheckoutController < ::BaseController @order.select_shipping_method(params[:shipping_method_id]) @order.update(order_params) + # We need to update voucher to take into account: + # * when moving away from "details" step : potential change in shipping method fees + # * when moving away from "payment" step : payment fees + recalculate_voucher if details_step? || payment_step? @order.update_totals_and_states validate_current_step end + def recalculate_voucher + return if @order.voucher_adjustments.empty? + + return if @order.shipment.shipping_method.id == params[:shipping_method_id].to_i + + VoucherAdjustmentsService.new(@order).update + end + def validate_current_step Checkout::Validation.new(@order, params).call && @order.errors.empty? end diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index 6bd9a5b919..523e366f28 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -90,11 +90,6 @@ module Spree order.update_totals_and_states end - after_transition to: :confirmation do |order| - VoucherAdjustmentsService.new(order).update - order.update_totals_and_states - end - after_transition to: :complete, do: :finalize! after_transition to: :resumed, do: :after_resume after_transition to: :canceled, do: :after_cancel diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index afa3d89274..2d45f1b661 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -172,6 +172,56 @@ describe SplitCheckoutController, type: :controller do expect(order.user.ship_address).to eq(order.ship_address) end end + + describe "with a voucher" do + let(:checkout_params) do + { + order: { + email: user.email, + bill_address_attributes: address.to_param, + ship_address_attributes: address.to_param + }, + shipping_method_id: order.shipment.shipping_method.id.to_s + } + end + + let(:voucher) { create(:voucher_flat_rate, enterprise: distributor) } + let(:service) { mock_voucher_adjustment_service } + + before do + voucher.create_adjustment(voucher.code, order) + end + + it "doesn't recalculate the voucher adjustment" do + expect(service).to_not receive(:update) + + put(:update, params:) + + expect(response).to redirect_to checkout_step_path(:payment) + end + + context "when updating shipping method" do + let(:checkout_params) do + { + order: { + email: user.email, + bill_address_attributes: address.to_param, + ship_address_attributes: address.to_param + }, + shipping_method_id: new_shipping_method.id.to_s + } + end + let(:new_shipping_method) { create(:shipping_method, distributors: [distributor]) } + + it "recalculates the voucher adjustment" do + expect(service).to receive(:update) + + put(:update, params:) + + expect(response).to redirect_to checkout_step_path(:payment) + end + end + end end end @@ -214,6 +264,24 @@ describe SplitCheckoutController, type: :controller do expect(response).to redirect_to checkout_step_path(:summary) expect(order.reload.state).to eq "confirmation" end + + describe "with a voucher" do + let(:voucher) { create(:voucher_flat_rate, enterprise: distributor) } + + before do + voucher.create_adjustment(voucher.code, order) + end + + # so we need to recalculate voucher to account for payment fees + it "recalculates the voucher adjustment" do + service = mock_voucher_adjustment_service + expect(service).to receive(:update) + + put(:update, params:) + + expect(response).to redirect_to checkout_step_path(:summary) + end + end end context "with payment fees" do @@ -399,4 +467,11 @@ describe SplitCheckoutController, type: :controller do it_behaves_like "handling stock issues", "payment" it_behaves_like "handling stock issues", "summary" end + + def mock_voucher_adjustment_service + voucher_adjustment_service = instance_double(VoucherAdjustmentsService) + allow(VoucherAdjustmentsService).to receive(:new).and_return(voucher_adjustment_service) + + voucher_adjustment_service + end end From 25af178011dbf084d0d264d583921d20e2a58655 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 20 Oct 2023 16:40:02 +1100 Subject: [PATCH 5/7] Refactor updating order state It makes the code a bit easier to read --- app/controllers/concerns/checkout_steps.rb | 14 ++++--------- app/controllers/split_checkout_controller.rb | 21 +++++++++++++++---- app/models/spree/order.rb | 4 ++++ .../split_checkout_controller_spec.rb | 3 +++ 4 files changed, 28 insertions(+), 14 deletions(-) diff --git a/app/controllers/concerns/checkout_steps.rb b/app/controllers/concerns/checkout_steps.rb index 556c934166..b9480998f7 100644 --- a/app/controllers/concerns/checkout_steps.rb +++ b/app/controllers/concerns/checkout_steps.rb @@ -40,6 +40,10 @@ module CheckoutSteps redirect_to_step_based_on_order end + # Checkout step and allowed order state + # * step details : order state in cart, address or delivery + # * step payment : order state is payment + # * step summary : order state is confirmation def check_step case @order.state when "cart", "address", "delivery" @@ -48,14 +52,4 @@ module CheckoutSteps redirect_to checkout_step_path(:payment) if summary_step? end end - - def update_order_state - if @order.state == "confirmation" && payment_step? - @order.back_to_payment - end - - return unless @order.state.in?(["payment", "confirmation"]) && details_step? - - @order.back_to_address - end end diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 67d52c496a..ed41eb1609 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -23,10 +23,12 @@ class SplitCheckoutController < ::BaseController before_action :hide_ofn_navigation, only: [:edit, :update] def edit - redirect_to_step_based_on_order unless params[:step] - - update_order_state if params[:step] - check_step if params[:step] + if params[:step].blank? + redirect_to_step_based_on_order + else + update_order_state + check_step + end return if available_shipping_methods.any? @@ -128,4 +130,15 @@ class SplitCheckoutController < ::BaseController def order_params @order_params ||= Checkout::Params.new(@order, params, spree_current_user).call end + + # Update order state based on the step we are loading to avoid discrepancy between step and order + # state. We need to do this when moving back to a previous checkout step, the update action takes + # care of moving the order state forward. + def update_order_state + return @order.back_to_payment if @order.confirmation? && payment_step? + + return unless @order.after_delivery_state? && details_step? + + @order.back_to_address + end end diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 2a3c825e44..9e41af5d5c 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -616,6 +616,10 @@ module Spree state.in?(["cart", "address", "delivery"]) end + def after_delivery_state? + state.in?(["payment", "confirmation"]) + end + private def reapply_tax_on_changed_address diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 2d45f1b661..c6c24e52b7 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -74,6 +74,7 @@ describe SplitCheckoutController, type: :controller do it "updates the order state to payment" do get :edit, params: { step: "payment" } + expect(response.status).to eq 200 expect(order.reload.state).to eq("payment") end end @@ -82,6 +83,7 @@ describe SplitCheckoutController, type: :controller do it "updates the order state to address" do get :edit, params: { step: "details" } + expect(response.status).to eq 200 expect(order.reload.state).to eq("address") end end @@ -96,6 +98,7 @@ describe SplitCheckoutController, type: :controller do it "updates the order state to address" do get :edit, params: { step: "details" } + expect(response.status).to eq 200 expect(order.reload.state).to eq("address") end end From d5d043880a577554dcf9c2788bbfec0246e6fd46 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 23 Oct 2023 14:45:15 +1100 Subject: [PATCH 6/7] Fix scenario when no shipment available I though that once the shipping method was set it's available on the order, but apparently it's not always the case. At least some of the test scenario have order with no shipment, thus no shipping method set. --- app/controllers/split_checkout_controller.rb | 2 +- spec/controllers/split_checkout_controller_spec.rb | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index ed41eb1609..17d7f36dbd 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -112,7 +112,7 @@ class SplitCheckoutController < ::BaseController def recalculate_voucher return if @order.voucher_adjustments.empty? - return if @order.shipment.shipping_method.id == params[:shipping_method_id].to_i + return if @order.shipping_method&.id == params[:shipping_method_id].to_i VoucherAdjustmentsService.new(@order).update end diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index c6c24e52b7..e6aee5c82e 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -223,6 +223,20 @@ describe SplitCheckoutController, type: :controller do expect(response).to redirect_to checkout_step_path(:payment) end + + context "when no shipments available" do + before do + order.shipments.destroy_all + end + + it "recalculates the voucher adjustment" do + expect(service).to receive(:update) + + put(:update, params:) + + expect(response).to redirect_to checkout_step_path(:payment) + end + end end end end From 33de80f13ccb3f9e10411c43a3de5589445dc3d0 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Fri, 27 Oct 2023 14:20:31 +1100 Subject: [PATCH 7/7] Fix checking if shipping method changed We now check if the shipping method changed before we actually select it. Fix the related spec.The spec was wrong because order.select_shipping_method fails silently. That means the shipping method wasn't getting updated on the order, thus the test was passing. --- app/controllers/split_checkout_controller.rb | 10 +++++++--- spec/controllers/split_checkout_controller_spec.rb | 7 +++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 17d7f36dbd..9bcd6a709e 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -98,21 +98,25 @@ class SplitCheckoutController < ::BaseController def update_order return if params[:confirm_order] || @order.errors.any? + # Checking if shipping method updated before @order get updated. We can't use this guard + # clause in recalculate_voucher as by then the @order.shipping method would be the new one + shipping_method_updated = @order.shipping_method&.id != params[:shipping_method_id].to_i + @order.select_shipping_method(params[:shipping_method_id]) @order.update(order_params) # We need to update voucher to take into account: # * when moving away from "details" step : potential change in shipping method fees # * when moving away from "payment" step : payment fees - recalculate_voucher if details_step? || payment_step? + recalculate_voucher(shipping_method_updated) if details_step? || payment_step? @order.update_totals_and_states validate_current_step end - def recalculate_voucher + def recalculate_voucher(shipping_method_updated) return if @order.voucher_adjustments.empty? - return if @order.shipping_method&.id == params[:shipping_method_id].to_i + return unless shipping_method_updated VoucherAdjustmentsService.new(@order).update end diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index e6aee5c82e..349b4f71e0 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -216,6 +216,13 @@ describe SplitCheckoutController, type: :controller do end let(:new_shipping_method) { create(:shipping_method, distributors: [distributor]) } + before do + # Add a shipping rates for the new shipping method to prevent + # order.select_shipping_method from failing + order.shipment.shipping_rates << + Spree::ShippingRate.create(shipping_method: new_shipping_method, selected: true) + end + it "recalculates the voucher adjustment" do expect(service).to receive(:update)