diff --git a/app/controllers/concerns/checkout_steps.rb b/app/controllers/concerns/checkout_steps.rb index 83eca5ed83..b9480998f7 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" @@ -36,12 +40,16 @@ 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" - 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 end diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 6b1266eaf8..9bcd6a709e 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -23,8 +23,12 @@ class SplitCheckoutController < ::BaseController before_action :hide_ofn_navigation, only: [:edit, :update] def edit - redirect_to_step_based_on_order unless 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? @@ -94,13 +98,29 @@ 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(shipping_method_updated) if details_step? || payment_step? @order.update_totals_and_states validate_current_step end + def recalculate_voucher(shipping_method_updated) + return if @order.voucher_adjustments.empty? + + return unless shipping_method_updated + + VoucherAdjustmentsService.new(@order).update + end + def validate_current_step Checkout::Validation.new(@order, params).call && @order.errors.empty? end @@ -114,4 +134,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 57626211a1..cd1d9c81f7 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/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index e76cc248c2..523e366f28 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 @@ -82,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 df46630878..349b4f71e0 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 } @@ -67,6 +64,45 @@ 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(response.status).to eq 200 + 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(response.status).to eq 200 + 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(response.status).to eq 200 + expect(order.reload.state).to eq("address") + end + end + end end end @@ -139,6 +175,77 @@ 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]) } + + 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) + + put(:update, params:) + + 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 end @@ -175,12 +282,30 @@ 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) 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 @@ -366,4 +491,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