Merge pull request #11677 from rioug/11359-vouchers-fix-tax-moving-back-in-checkout

[vouchers] fix tax calculation when moving back in the checkout process

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

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

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

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

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