From ebe2923512b5f24e63aac187a1512261bfed1e4c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 11:20:03 +0100
Subject: [PATCH 01/20] Improve form outputs and error handling

---
 app/controllers/split_checkout_controller.rb | 12 ++++++++----
 app/helpers/application_helper.rb            | 18 +++++++++++++-----
 app/views/split_checkout/_details.html.haml  | 16 +++++++---------
 app/views/split_checkout/_payment.html.haml  |  7 ++++---
 app/views/split_checkout/_summary.html.haml  | 11 ++++-------
 5 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index f7d1e20cd7..073b9c6693 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -108,11 +108,8 @@ class SplitCheckoutController < ::BaseController
 
   def confirm_order
     return unless @order.confirmation? && params[:confirm_order]
+    return unless validate_terms_and_conditions!
 
-    if params["accept_terms"] != "1"
-      @order.errors.add(:base, "terms_not_accepted")
-      return false
-    end
     @order.confirm!
   end
 
@@ -139,6 +136,13 @@ class SplitCheckoutController < ::BaseController
     @checkout_step ||= params[:step]
   end
 
+  def validate_terms_and_conditions!
+    return true if params[:accept_terms]
+
+    @order.errors.add(:terms_and_conditions, t("split_checkout.errors.terms_not_accepted"))
+    false
+  end
+
   def order_params
     return @order_params unless @order_params.nil?
 
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index c00825f74d..e4e18601c7 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -4,14 +4,22 @@ module ApplicationHelper
   include RawParams
   include Pagy::Frontend
 
-  def error_message_on(object, method, _options = {})
+  def error_message_on(object, method, options = {})
     object = convert_to_model(object)
     obj = object.respond_to?(:errors) ? object : instance_variable_get("@#{object}")
-    if obj && obj.errors[method].present?
-      errors = obj.errors[method].map { |err| h(err) }.join('<br />').html_safe
-      content_tag(:span, errors, class: 'formError')
+
+    return "" unless obj && obj.errors[method].present?
+
+    errors = obj.errors[method].map { |err| h(err) }.join('<br />').html_safe
+
+    if options[:standalone]
+      content_tag(
+        :div,
+        content_tag(:span, errors, class: 'formError standalone'),
+        class: 'checkout-input'
+      )
     else
-      ''
+      content_tag(:span, errors, class: 'formError')
     end
   end
 
diff --git a/app/views/split_checkout/_details.html.haml b/app/views/split_checkout/_details.html.haml
index 0642870cbe..59504af294 100644
--- a/app/views/split_checkout/_details.html.haml
+++ b/app/views/split_checkout/_details.html.haml
@@ -71,10 +71,7 @@
 
     - display_ship_address = false
     - ship_method_description = nil
-    - if selected_shipping_method == nil && @order.errors.messages_for(:base).include?("no_shipping_method_selected")
-      %div.checkout-input
-        %span.formError.standalone
-          = t("split_checkout.step1.delivery_address.errors.no_shipping_method_selected")
+
     - @shipping_methods.each do |shipping_method|
       %div.checkout-input
         = fields_for shipping_method do |shipping_method_form|
@@ -93,6 +90,8 @@
           - if shipping_method.id == selected_shipping_method.to_i
             - ship_method_description = shipping_method.description
 
+    = f.error_message_on :shipping_method, standalone: true
+
     %div.checkout-input{"data-shippingmethod-target": "shippingMethodDescription", style: "display: #{ship_method_description == nil ? 'none' : 'block'}" }
       #distributor_address.panel
         %span{"data-shippingmethod-target": "shippingMethodDescriptionContent"} #{ship_method_description}
@@ -103,8 +102,8 @@
         = @order.order_cycle.pickup_time_for(@order.distributor)
     
     %div.checkout-input{ "data-toggle-target": "content", style: "display: #{display_ship_address ? 'block' : 'none'}" }
-      = f.check_box "Checkout.ship_address_same_as_billing", { id: "Checkout.ship_address_same_as_billing", "data-action": "shippingmethod#showHideShippingAddress", "data-shippingmethod-target": "shippingAddressCheckbox", checked: @ship_address_same_as_billing == "1" }
-      = f.label "Checkout.ship_address_same_as_billing", t(:checkout_address_same), { for: "Checkout.ship_address_same_as_billing" }
+      = f.check_box :ship_address_same_as_billing, { id: "ship_address_same_as_billing", name: "ship_address_same_as_billing", "data-action": "shippingmethod#showHideShippingAddress", "data-shippingmethod-target": "shippingAddressCheckbox", checked: @ship_address_same_as_billing == "1" }, 1, nil
+      = f.label :ship_address_same_as_billing, t(:checkout_address_same), { for: "ship_address_same_as_billing" }
 
     %div{"data-shippingmethod-target": "shippingMethodAddress", style: "display: #{display_ship_address == false || @ship_address_same_as_billing == "1" ? 'none' : 'block'}" }
       = f.fields :ship_address, model: @order.ship_address do |ship_address|
@@ -138,9 +137,8 @@
 
     - if spree_current_user
       %div.checkout-input{ "data-toggle-target": "content", style: "display: none" }
-        = f.check_box "Checkout.default_ship_address", { id: "Checkout.default_ship_address" }
-        = f.label "Checkout.default_ship_address", t(:checkout_default_ship_address), { for: "Checkout.default_ship_address" }
-
+        = f.check_box :default_ship_address, { id: "default_ship_address", name: "default_ship_address" }
+        = f.label :default_ship_address, t(:checkout_default_ship_address), { for: "default_ship_address" }
 
     .div.checkout-input
       = f.label :special_instructions, t(:checkout_instructions)
diff --git a/app/views/split_checkout/_payment.html.haml b/app/views/split_checkout/_payment.html.haml
index 4f4ded6433..02e9d7e28e 100644
--- a/app/views/split_checkout/_payment.html.haml
+++ b/app/views/split_checkout/_payment.html.haml
@@ -2,21 +2,22 @@
   %div.checkout-substep{"data-controller": "paymentmethod"}
     %div.checkout-title
       = t("split_checkout.step2.payment_method.title")
+
     - selected_payment_method = @order.payments&.with_state(:checkout)&.first&.payment_method_id
     - available_payment_methods.each do |payment_method| 
       %div.checkout-input
         = f.radio_button :payment_method_id, payment_method.id,
-          name: "order[payments_attributes][][payment_method_id]",
           id: "payment_method_#{payment_method.id}",
           name: "order[payments_attributes][][payment_method_id]",
           checked: (payment_method.id == selected_payment_method),
           "data-action": "paymentmethod#selectPaymentMethod",
           "data-paymentmethod-description": "#{payment_method.description}"
-        = f.label payment_method.id, "#{payment_method.name} (#{payment_or_shipping_price(payment_method, @order)})", {for: "payment_method_" + payment_method.id.to_s }
+        = f.label :payment_method_id, "#{payment_method.name} (#{payment_or_shipping_price(payment_method, @order)})", for: "payment_method_#{payment_method.id}"
+
+    = f.error_message_on :payment_method, standalone: true
 
     %div
       .panel{"data-paymentmethod-target": "panel", style: "display: none"} 
-        
 
   %div.checkout-substep
     = t("split_checkout.step2.explaination")
diff --git a/app/views/split_checkout/_summary.html.haml b/app/views/split_checkout/_summary.html.haml
index 1f6bd3fea2..57c9c18ab5 100644
--- a/app/views/split_checkout/_summary.html.haml
+++ b/app/views/split_checkout/_summary.html.haml
@@ -72,14 +72,11 @@
     = render 'spree/orders/summary', order: @order
 
 %div.checkout-substep.medium-6
-  - if @order.errors.messages_for(:base).include?("terms_not_accepted")
-    %div.checkout-input
-      %span.formError.standalone
-        = t("split_checkout.errors.terms_not_accepted")
-
   %div.checkout-input
-    = f.check_box :accept_terms, {id: 'accept_terms', name: "accept_terms", "checked": "#{all_terms_and_conditions_already_accepted?}"}
-    = f.label :accept_terms, t('split_checkout.step3.terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions.url, target: '_blank'), tos_link: link_to_platform_terms), {for: "accept_terms"}
+    = f.check_box :accept_terms, { id: "accept_terms", name: "accept_terms", "checked": "#{all_terms_and_conditions_already_accepted?}" }, 1, nil
+    = f.label :accept_terms, t('split_checkout.step3.terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions.url, target: '_blank'), tos_link: link_to_platform_terms), { for: "accept_terms" }
+
+  = f.error_message_on :terms_and_conditions, standalone: true
 
   %div.checkout-input
   = t("split_checkout.step3.agree")

From 3dd8ed85b4fdb55c85a2a6b921aadc81885cf7bf Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 11:28:55 +0100
Subject: [PATCH 02/20] Implement manual shipping method selection

---
 .../concerns/checkout_callbacks.rb            |  1 +
 app/models/concerns/order_shipment.rb         |  2 ++
 .../order_management/stock/estimator.rb       | 28 +++++++++++--------
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb
index f29b81d7dc..3e3e360bd4 100644
--- a/app/controllers/concerns/checkout_callbacks.rb
+++ b/app/controllers/concerns/checkout_callbacks.rb
@@ -27,6 +27,7 @@ module CheckoutCallbacks
 
   def load_order
     @order = current_order
+    @order.manual_shipping_selection = true
 
     redirect_to(main_app.shop_path) && return if redirect_to_shop?
     redirect_to_cart_path && return unless valid_order_line_items?
diff --git a/app/models/concerns/order_shipment.rb b/app/models/concerns/order_shipment.rb
index 267edf64f4..dd5ebeba01 100644
--- a/app/models/concerns/order_shipment.rb
+++ b/app/models/concerns/order_shipment.rb
@@ -13,6 +13,8 @@ require 'active_support/concern'
 module OrderShipment
   extend ActiveSupport::Concern
 
+  attr_accessor :manual_shipping_selection
+
   def shipment
     shipments.first
   end
diff --git a/engines/order_management/app/services/order_management/stock/estimator.rb b/engines/order_management/app/services/order_management/stock/estimator.rb
index c97d9c3b99..371ef9b980 100644
--- a/engines/order_management/app/services/order_management/stock/estimator.rb
+++ b/engines/order_management/app/services/order_management/stock/estimator.rb
@@ -22,17 +22,8 @@ module OrderManagement
 
         shipping_rates.sort_by! { |r| r.cost || 0 }
 
-        unless shipping_rates.empty?
-          if frontend_only
-            shipping_rates.each do |rate|
-              if rate.shipping_method.frontend?
-                rate.selected = true
-                break
-              end
-            end
-          else
-            shipping_rates.first.selected = true
-          end
+        unless shipping_rates.empty? || order.manual_shipping_selection
+          select_first_shipping_method(shipping_rates, frontend_only)
         end
 
         shipping_rates
@@ -40,6 +31,21 @@ module OrderManagement
 
       private
 
+      # Sets the first available shipping method to "selected".
+      # Note: seems like a hangover from Spree, we can probably just remove this at some point.
+      def select_first_shipping_method(shipping_rates, frontend_only)
+        if frontend_only
+          shipping_rates.each do |rate|
+            if rate.shipping_method.frontend?
+              rate.selected = true
+              break
+            end
+          end
+        else
+          shipping_rates.first.selected = true
+        end
+      end
+
       def shipping_methods(package)
         shipping_methods = package.shipping_methods
         shipping_methods.delete_if { |ship_method| !ship_method.calculator.available?(package) }

From 978c882ed96dd8ca269a60af4c53c27de5688e39 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 11:43:06 +0100
Subject: [PATCH 03/20] Don't select a random shipping method in Shipment

---
 app/models/spree/shipment.rb       | 6 ++++--
 spec/models/spree/shipment_spec.rb | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb
index 804e09de3f..bf83aa35fb 100644
--- a/app/models/spree/shipment.rb
+++ b/app/models/spree/shipment.rb
@@ -84,7 +84,9 @@ module Spree
     end
 
     def shipping_method
-      selected_shipping_rate.try(:shipping_method) || shipping_rates.first.try(:shipping_method)
+      method = selected_shipping_rate.try(:shipping_method)
+      method ||= shipping_rates.first.try(:shipping_method) unless order.manual_shipping_selection
+      method
     end
 
     def add_shipping_method(shipping_method, selected = false)
@@ -263,7 +265,7 @@ module Spree
         fee_adjustment.amount = selected_shipping_rate.cost if fee_adjustment.open?
         fee_adjustment.save!
         fee_adjustment.reload
-      elsif selected_shipping_rate_id
+      elsif shipping_method
         shipping_method.create_adjustment(adjustment_label,
                                           self,
                                           true,
diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb
index 1a18b3777c..f1f13edbb7 100644
--- a/spec/models/spree/shipment_spec.rb
+++ b/spec/models/spree/shipment_spec.rb
@@ -388,6 +388,7 @@ describe Spree::Shipment do
     end
 
     it "should create adjustment when not present" do
+      allow(shipment).to receive_messages(fee_adjustment: nil)
       allow(shipment).to receive_messages(selected_shipping_rate_id: 1)
       expect(shipping_method).to receive(:create_adjustment).with(shipment.adjustment_label,
                                                                   shipment, true, "open")

From 2786fb30baec5cc852f830d90421fd256a1938f1 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 11:48:49 +0100
Subject: [PATCH 04/20] Simplify order advancing process

---
 app/controllers/split_checkout_controller.rb | 13 +++----------
 app/services/order_workflow.rb               | 17 +++++++----------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 073b9c6693..251d94a8a9 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -37,8 +37,7 @@ class SplitCheckoutController < ::BaseController
     load_shipping_method
     handle_shipping_method_selection
 
-    confirm_or_update = confirm_order || update_order
-    if confirm_or_update && advance_order_state
+    if confirm_order || update_order
       clear_invalid_payments
       redirect_to_step
     else
@@ -117,19 +116,13 @@ class SplitCheckoutController < ::BaseController
     return unless params[:order]
     return if @order.state == "address" && params[:shipping_method_id].blank?
 
-    @order.update(order_params)
+    @order.update(order_params) && advance_order_state
   end
 
   def advance_order_state
     return true if @order.complete?
 
-    workflow_options = raw_params.slice(:shipping_method_id)
-
-    if @order.payments.empty?
-      OrderWorkflow.new(@order).advance_to_payment(workflow_options)
-    else
-      OrderWorkflow.new(@order).advance_to_confirmation(workflow_options)
-    end
+    OrderWorkflow.new(@order).advance_checkout(raw_params.slice(:shipping_method_id))
   end
 
   def checkout_step
diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index 8fc1615c77..8f8b3028c9 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -23,19 +23,16 @@ class OrderWorkflow
     result
   end
 
-  def advance_to_payment(options = {})
-    if options[:shipping_method_id]
-      order.select_shipping_method(options[:shipping_method_id])
-    end
+  def advance_to_payment
     advance_to_state("payment", advance_order_options)
   end
 
-  def advance_to_confirmation(options = {})
-    if options[:shipping_method_id]
-      order.select_shipping_method(options[:shipping_method_id])
-    end
+  def advance_checkout(options = {})
+    order.select_shipping_method(options[:shipping_method_id])
 
-    advance_to_state("confirmation")
+    advance_to = order.address? || order.delivery? ? "payment" : "confirmation"
+
+    advance_to_state(advance_to, advance_order_options.merge(options))
   end
 
   private
@@ -71,7 +68,7 @@ class OrderWorkflow
   end
 
   def after_transition_hook(options)
-    if order.state == "delivery" && (options[:shipping_method_id])
+    if order.state == "delivery"
       order.select_shipping_method(options[:shipping_method_id])
     end
 

From ac4d721de0aaebcb6edb471bd0ac9e37cb8bde6e Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 11:51:47 +0100
Subject: [PATCH 05/20] Extract #set_payment_amount method

---
 app/controllers/split_checkout_controller.rb | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 251d94a8a9..94d61277ef 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -146,14 +146,17 @@ class SplitCheckoutController < ::BaseController
       payments_attributes: [:payment_method_id]
     )
 
-    if @order_params[:payments_attributes]
-      # Set payment amount
-      @order_params[:payments_attributes].first[:amount] = @order.total
-    end
+    set_payment_amount
 
     @order_params
   end
 
+  def set_payment_amount
+    return unless @order_params[:payments_attributes]
+
+    @order_params[:payments_attributes].first[:amount] = @order.total
+  end
+
   def redirect_to_step
     case @order.state
     when "cart", "address", "delivery"

From 3fabe308cdb0602473eba030230be3a4f61c0469 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:06:39 +0100
Subject: [PATCH 06/20] Extract #shipping_and_billing_match? helper

---
 app/controllers/split_checkout_controller.rb | 7 -------
 app/helpers/checkout_helper.rb               | 4 ++++
 app/views/split_checkout/_details.html.haml  | 4 ++--
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 94d61277ef..98b3530e88 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -28,7 +28,6 @@ class SplitCheckoutController < ::BaseController
     @order.errors.clear
     @order.bill_address.errors.clear
     @order.ship_address.errors.clear
-    @ship_address_same_as_billing = "1" if ship_address_matches_bill_address?
   rescue Spree::Core::GatewayError => e
     rescue_from_spree_gateway_error(e)
   end
@@ -51,12 +50,6 @@ class SplitCheckoutController < ::BaseController
 
   private
 
-  def ship_address_matches_bill_address?
-    attrs_to_check = %w(firstname lastname address1 address2 state_id city zipcode phone country_id)
-    ((@order.bill_address.attributes.to_a -
-      @order.ship_address.attributes.to_a).map(&:first) & attrs_to_check).blank?
-  end
-
   def handle_shipping_method_selection
     return unless @shipping_method
 
diff --git a/app/helpers/checkout_helper.rb b/app/helpers/checkout_helper.rb
index 1285ce297c..c5b22b4784 100644
--- a/app/helpers/checkout_helper.rb
+++ b/app/helpers/checkout_helper.rb
@@ -1,6 +1,10 @@
 # frozen_string_literal: true
 
 module CheckoutHelper
+  def shipping_and_billing_match?(order)
+    order.ship_address == order.bill_address
+  end
+
   def guest_checkout_allowed?
     current_order.distributor.allow_guest_orders?
   end
diff --git a/app/views/split_checkout/_details.html.haml b/app/views/split_checkout/_details.html.haml
index 59504af294..dda5bd70d9 100644
--- a/app/views/split_checkout/_details.html.haml
+++ b/app/views/split_checkout/_details.html.haml
@@ -102,10 +102,10 @@
         = @order.order_cycle.pickup_time_for(@order.distributor)
     
     %div.checkout-input{ "data-toggle-target": "content", style: "display: #{display_ship_address ? 'block' : 'none'}" }
-      = f.check_box :ship_address_same_as_billing, { id: "ship_address_same_as_billing", name: "ship_address_same_as_billing", "data-action": "shippingmethod#showHideShippingAddress", "data-shippingmethod-target": "shippingAddressCheckbox", checked: @ship_address_same_as_billing == "1" }, 1, nil
+      = f.check_box :ship_address_same_as_billing, { id: "ship_address_same_as_billing", name: "ship_address_same_as_billing", "data-action": "shippingmethod#showHideShippingAddress", "data-shippingmethod-target": "shippingAddressCheckbox", checked: shipping_and_billing_match?(@order) }, 1, nil
       = f.label :ship_address_same_as_billing, t(:checkout_address_same), { for: "ship_address_same_as_billing" }
 
-    %div{"data-shippingmethod-target": "shippingMethodAddress", style: "display: #{display_ship_address == false || @ship_address_same_as_billing == "1" ? 'none' : 'block'}" }
+    %div{"data-shippingmethod-target": "shippingMethodAddress", style: "display: #{display_ship_address == false || shipping_and_billing_match?(@order) ? 'none' : 'block'}" }
       = f.fields :ship_address, model: @order.ship_address do |ship_address|
         %div.checkout-input
           = ship_address.label :address1, t("split_checkout.step1.address.address1.label")

From b7e1882bd8e45ba53fbacab22058a6109a0faa4c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:11:40 +0100
Subject: [PATCH 07/20] Simplify setting address params

---
 app/controllers/split_checkout_controller.rb | 48 ++++++--------------
 1 file changed, 13 insertions(+), 35 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 98b3530e88..450acf6058 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -34,7 +34,6 @@ class SplitCheckoutController < ::BaseController
 
   def update
     load_shipping_method
-    handle_shipping_method_selection
 
     if confirm_order || update_order
       clear_invalid_payments
@@ -50,12 +49,6 @@ class SplitCheckoutController < ::BaseController
 
   private
 
-  def handle_shipping_method_selection
-    return unless @shipping_method
-
-    populate_ship_address_params
-  end
-
   def load_shipping_method
     if params[:shipping_method_id]
       @shipping_method = Spree::ShippingMethod.where(id: params[:shipping_method_id]).first
@@ -66,34 +59,6 @@ class SplitCheckoutController < ::BaseController
     end
   end
 
-  def populate_ship_address_params
-    return unless params.dig(:order, :ship_address_attributes).present? &&
-                  params.dig(:order, :bill_address_attributes).present?
-
-    if params.dig(:order, "Checkout.ship_address_same_as_billing") == "1"
-      params[:order][:ship_address_attributes] = params[:order][:bill_address_attributes]
-      return
-    end
-
-    address_attrs = [
-      :firstname,
-      :lastname,
-      :phone,
-      :address1,
-      :address2,
-      :city,
-      :state_id,
-      :zipcode,
-      :country_id,
-      :id
-    ]
-    address_attrs.each do |attr|
-      next if params[:order][:ship_address_attributes][attr].present?
-
-      params[:order][:ship_address_attributes][attr] = params[:order][:bill_address_attributes][attr]
-    end
-  end
-
   def clear_invalid_payments
     @order.payments.with_state(:invalid).delete_all
   end
@@ -139,11 +104,24 @@ class SplitCheckoutController < ::BaseController
       payments_attributes: [:payment_method_id]
     )
 
+    set_address_details
     set_payment_amount
 
     @order_params
   end
 
+  def set_address_details
+    return unless @order_params[:ship_address_attributes] && @order_params[:bill_address_attributes]
+
+    if params[:ship_address_same_as_billing]
+      @order_params[:ship_address_attributes] = @order_params[:bill_address_attributes]
+    else
+      @order_params[:ship_address_attributes][:firstname] = @order_params[:bill_address_attributes][:firstname]
+      @order_params[:ship_address_attributes][:lastname] = @order_params[:bill_address_attributes][:lastname]
+      @order_params[:ship_address_attributes][:phone] = @order_params[:bill_address_attributes][:phone]
+    end
+  end
+
   def set_payment_amount
     return unless @order_params[:payments_attributes]
 

From 98004834332e7fa7cad6aa07dc70a6b82d6a6149 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:20:05 +0100
Subject: [PATCH 08/20] Simplify displaying selected shipping method

---
 app/controllers/split_checkout_controller.rb | 12 ------------
 app/views/split_checkout/_details.html.haml  |  3 +--
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 450acf6058..8245dd57a7 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -33,8 +33,6 @@ class SplitCheckoutController < ::BaseController
   end
 
   def update
-    load_shipping_method
-
     if confirm_order || update_order
       clear_invalid_payments
       redirect_to_step
@@ -49,16 +47,6 @@ class SplitCheckoutController < ::BaseController
 
   private
 
-  def load_shipping_method
-    if params[:shipping_method_id]
-      @shipping_method = Spree::ShippingMethod.where(id: params[:shipping_method_id]).first
-      @shipping_method_id = params[:shipping_method_id]
-    else
-      @shipping_method = @order.shipping_method
-      @shipping_method_id = @shipping_method&.id
-    end
-  end
-
   def clear_invalid_payments
     @order.payments.with_state(:invalid).delete_all
   end
diff --git a/app/views/split_checkout/_details.html.haml b/app/views/split_checkout/_details.html.haml
index dda5bd70d9..bd10cd0eb1 100644
--- a/app/views/split_checkout/_details.html.haml
+++ b/app/views/split_checkout/_details.html.haml
@@ -64,8 +64,7 @@
           = f.label :checkout_default_bill_address, t(:checkout_default_bill_address)
 
   %div.checkout-substep{ "data-controller": "toggle shippingmethod" }
-    - selected_shipping_method = @order.shipping_method&.id || @shipping_method_id
-    -# DELIVERY ADDRESS
+    - selected_shipping_method = @order.shipping_method&.id || params[:shipping_method_id]
     %div.checkout-title
       = t("split_checkout.step1.delivery_address.title")
 

From 6acb1f64844cf11766a877e6ff55a890feab9c5a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:25:20 +0100
Subject: [PATCH 09/20] Tidy up view variables

---
 app/views/split_checkout/_details.html.haml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/views/split_checkout/_details.html.haml b/app/views/split_checkout/_details.html.haml
index bd10cd0eb1..4e0be37ac9 100644
--- a/app/views/split_checkout/_details.html.haml
+++ b/app/views/split_checkout/_details.html.haml
@@ -85,7 +85,7 @@
           = shipping_method_form.label shipping_method.id, shipping_method.name, {for: "shipping_method_" + shipping_method.id.to_s }
           %em.light
             = payment_or_shipping_price(shipping_method, @order)
-          - display_ship_address = display_ship_address || (shipping_method.id == selected_shipping_method.to_i && shipping_method.require_ship_address)
+          - display_ship_address = (shipping_method.id == selected_shipping_method.to_i && shipping_method.require_ship_address)
           - if shipping_method.id == selected_shipping_method.to_i
             - ship_method_description = shipping_method.description
 
@@ -104,7 +104,7 @@
       = f.check_box :ship_address_same_as_billing, { id: "ship_address_same_as_billing", name: "ship_address_same_as_billing", "data-action": "shippingmethod#showHideShippingAddress", "data-shippingmethod-target": "shippingAddressCheckbox", checked: shipping_and_billing_match?(@order) }, 1, nil
       = f.label :ship_address_same_as_billing, t(:checkout_address_same), { for: "ship_address_same_as_billing" }
 
-    %div{"data-shippingmethod-target": "shippingMethodAddress", style: "display: #{display_ship_address == false || shipping_and_billing_match?(@order) ? 'none' : 'block'}" }
+    %div{"data-shippingmethod-target": "shippingMethodAddress", style: "display: #{!display_ship_address || shipping_and_billing_match?(@order) ? 'none' : 'block'}" }
       = f.fields :ship_address, model: @order.ship_address do |ship_address|
         %div.checkout-input
           = ship_address.label :address1, t("split_checkout.step1.address.address1.label")

From 57504f42d89f092a46b52346bd63e6a7104329ac Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:34:52 +0100
Subject: [PATCH 10/20] Move validations to state transitions

---
 app/controllers/split_checkout_controller.rb |  6 +-----
 app/models/spree/order/checkout.rb           | 20 ++++++++++++++++++++
 config/locales/en.yml                        |  4 ++--
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 8245dd57a7..952569c9e9 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -37,10 +37,7 @@ class SplitCheckoutController < ::BaseController
       clear_invalid_payments
       redirect_to_step
     else
-      if @shipping_method_id.blank?
-        @order.errors.add(:base, "no_shipping_method_selected")
-      end
-      flash.now[:error] = "#{I18n.t('split_checkout.errors.global')}"
+      flash.now[:error] = I18n.t('split_checkout.errors.global')
       render :edit
     end
   end
@@ -60,7 +57,6 @@ class SplitCheckoutController < ::BaseController
 
   def update_order
     return unless params[:order]
-    return if @order.state == "address" && params[:shipping_method_id].blank?
 
     @order.update(order_params) && advance_order_state
   end
diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index 148348ce1e..dd4d0a80e6 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -75,7 +75,9 @@ module Spree
 
               before_transition to: :delivery, do: :create_proposed_shipments
               before_transition to: :delivery, do: :ensure_available_shipping_rates
+              before_transition to: :payment, do: :validate_shipping_method!
               before_transition to: :payment, do: :create_tax_charge!
+              before_transition to: :confirmation, do: :validate_payment_method!
 
               after_transition to: :complete, do: :finalize!
               after_transition to: :resumed,  do: :after_resume
@@ -135,6 +137,24 @@ module Spree
               updated_at: Time.zone.now,
             )
           end
+
+          private
+
+          def validate_shipping_method!
+            return unless user && Flipper.enabled?(:split_checkout, user)
+            return if shipping_method.present?
+
+            errors.add :shipping_method, I18n.t('split_checkout.errors.select_a_shipping_method')
+            throw :halt
+          end
+
+          def validate_payment_method!
+            return unless user && Flipper.enabled?(:split_checkout, user)
+            return if payments.any?
+
+            errors.add :payment_method, I18n.t('split_checkout.errors.select_a_payment_method')
+            throw :halt
+          end
         end
       end
     end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f3a04446f6..0113a7e9a3 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1660,8 +1660,6 @@ en:
           label: Country
       delivery_address:
           title: Delivery address
-          errors:
-            no_shipping_method_selected: No shipping method selected
       submit: Next - Payment method
       cancel: Back to Edit basket
     step2:
@@ -1697,6 +1695,8 @@ en:
       required: Field cannot be blank
       invalid_number: "Please enter a valid phone number"
       invalid_email: "Please enter a valid email address"
+      select_a_shipping_method: Select a shipping method
+      select_a_payment_method: Select a payment method
   order_paid: PAID
   order_not_paid: NOT PAID
   order_total: Total order

From 4cff185b4b08df847e352939f08eef5a6b9017dc Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:36:27 +0100
Subject: [PATCH 11/20] Extract params handling to service

---
 app/controllers/split_checkout_controller.rb | 34 +----------
 app/services/checkout/params.rb              | 63 ++++++++++++++++++++
 2 files changed, 65 insertions(+), 32 deletions(-)
 create mode 100644 app/services/checkout/params.rb

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 952569c9e9..61276b4303 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -56,7 +56,7 @@ class SplitCheckoutController < ::BaseController
   end
 
   def update_order
-    return unless params[:order]
+    return if @order.errors.any?
 
     @order.update(order_params) && advance_order_state
   end
@@ -79,37 +79,7 @@ class SplitCheckoutController < ::BaseController
   end
 
   def order_params
-    return @order_params unless @order_params.nil?
-
-    @order_params = params.require(:order).permit(
-      :email, :shipping_method_id, :special_instructions,
-      bill_address_attributes: PermittedAttributes::Address.attributes,
-      ship_address_attributes: PermittedAttributes::Address.attributes,
-      payments_attributes: [:payment_method_id]
-    )
-
-    set_address_details
-    set_payment_amount
-
-    @order_params
-  end
-
-  def set_address_details
-    return unless @order_params[:ship_address_attributes] && @order_params[:bill_address_attributes]
-
-    if params[:ship_address_same_as_billing]
-      @order_params[:ship_address_attributes] = @order_params[:bill_address_attributes]
-    else
-      @order_params[:ship_address_attributes][:firstname] = @order_params[:bill_address_attributes][:firstname]
-      @order_params[:ship_address_attributes][:lastname] = @order_params[:bill_address_attributes][:lastname]
-      @order_params[:ship_address_attributes][:phone] = @order_params[:bill_address_attributes][:phone]
-    end
-  end
-
-  def set_payment_amount
-    return unless @order_params[:payments_attributes]
-
-    @order_params[:payments_attributes].first[:amount] = @order.total
+    @order_params ||= Checkout::Params.new(@order, params).call
   end
 
   def redirect_to_step
diff --git a/app/services/checkout/params.rb b/app/services/checkout/params.rb
new file mode 100644
index 0000000000..51d0dcb780
--- /dev/null
+++ b/app/services/checkout/params.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+module Checkout
+  class Params
+    def initialize(order, params)
+      @params = params
+      @order = order
+    end
+
+    def call
+      return {} unless params[:order]
+
+      apply_strong_parameters
+      set_address_details
+      set_payment_amount
+
+      @order_params
+    end
+
+    private
+
+    attr_reader :params, :order
+
+    def apply_strong_parameters
+      @order_params = params.require(:order).permit(
+        :email, :shipping_method_id, :special_instructions,
+        bill_address_attributes: ::PermittedAttributes::Address.attributes,
+        ship_address_attributes: ::PermittedAttributes::Address.attributes,
+        payments_attributes: [:payment_method_id]
+      )
+    end
+
+    def set_address_details
+      return unless addresses_present?
+
+      if params[:ship_address_same_as_billing]
+        set_ship_address_from_bill_address
+      else
+        set_basic_details
+      end
+    end
+
+    def set_payment_amount
+      return unless @order_params[:payments_attributes]
+
+      @order_params[:payments_attributes].first[:amount] = order.total
+    end
+
+    def addresses_present?
+      @order_params[:ship_address_attributes] && @order_params[:bill_address_attributes]
+    end
+
+    def set_ship_address_from_bill_address
+      @order_params[:ship_address_attributes] = @order_params[:bill_address_attributes]
+    end
+
+    def set_basic_details
+      [:firstname, :lastname, :phone].each do |attr|
+        @order_params[:ship_address_attributes][attr] = @order_params[:bill_address_attributes][attr]
+      end
+    end
+  end
+end

From 1fca7d2a6ca288d1d95396ffde547407d10e35e4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 12:36:45 +0100
Subject: [PATCH 12/20] Remove error-clearing from edit action

---
 app/controllers/split_checkout_controller.rb | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 61276b4303..f11c432aee 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -25,9 +25,6 @@ class SplitCheckoutController < ::BaseController
     # a version of paypal that uses this controller, and more specifically
     # the #action_failed method, then we can remove this call
     # OrderCheckoutRestart.new(@order).call
-    @order.errors.clear
-    @order.bill_address.errors.clear
-    @order.ship_address.errors.clear
   rescue Spree::Core::GatewayError => e
     rescue_from_spree_gateway_error(e)
   end

From 9d6e5e9420e2f016e01c08e29a20b470ed6f46f4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 1 Sep 2021 13:00:17 +0100
Subject: [PATCH 13/20] Remove currently unused code

---
 .../concerns/checkout_callbacks.rb            |  2 +-
 app/controllers/split_checkout_controller.rb  | 44 +------------------
 2 files changed, 2 insertions(+), 44 deletions(-)

diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb
index 3e3e360bd4..638389cd59 100644
--- a/app/controllers/concerns/checkout_callbacks.rb
+++ b/app/controllers/concerns/checkout_callbacks.rb
@@ -14,7 +14,7 @@ module CheckoutCallbacks
     prepend_before_action :require_distributor_chosen
 
     before_action :load_order, :associate_user, :load_saved_addresses
-    before_action :load_shipping_methods, :load_countries, if: -> { checkout_step == "details"}
+    before_action :load_shipping_methods, :load_countries, if: -> { params[:step] == "details"}
 
     before_action :ensure_order_not_completed
     before_action :ensure_checkout_allowed
diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index f11c432aee..7467a8efcc 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -15,18 +15,9 @@ class SplitCheckoutController < ::BaseController
   helper OrderHelper
 
   def edit
-    return handle_redirect_from_stripe if valid_payment_intent_provided?
-
-    redirect_to_step unless checkout_step
+    redirect_to_step unless params[:step]
 
     OrderWorkflow.new(@order).next if @order.cart?
-
-    # This is only required because of spree_paypal_express. If we implement
-    # a version of paypal that uses this controller, and more specifically
-    # the #action_failed method, then we can remove this call
-    # OrderCheckoutRestart.new(@order).call
-  rescue Spree::Core::GatewayError => e
-    rescue_from_spree_gateway_error(e)
   end
 
   def update
@@ -64,10 +55,6 @@ class SplitCheckoutController < ::BaseController
     OrderWorkflow.new(@order).advance_checkout(raw_params.slice(:shipping_method_id))
   end
 
-  def checkout_step
-    @checkout_step ||= params[:step]
-  end
-
   def validate_terms_and_conditions!
     return true if params[:accept_terms]
 
@@ -91,33 +78,4 @@ class SplitCheckoutController < ::BaseController
       redirect_to order_path(@order)
     end
   end
-
-  def valid_payment_intent_provided?
-    return false unless params["payment_intent"]&.starts_with?("pi_")
-
-    last_payment = OrderPaymentFinder.new(@order).last_payment
-    @order.state == "payment" &&
-      last_payment&.state == "requires_authorization" &&
-      last_payment&.response_code == params["payment_intent"]
-  end
-
-  def handle_redirect_from_stripe
-    return checkout_failed unless @order.process_payments!
-
-    if OrderWorkflow.new(@order).next && order_complete?
-      checkout_succeeded
-      redirect_to(order_path(@order)) && return
-    else
-      checkout_failed
-    end
-  end
-
-  def order_complete?
-    @order.state == "complete" || @order.completed?
-  end
-
-  def rescue_from_spree_gateway_error(error)
-    flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
-    action_failed(error)
-  end
 end

From 70513ae98950838b7c810b6815b731793a5c5f4d Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 5 Sep 2021 20:19:22 +0100
Subject: [PATCH 14/20] Don't transition to address before page load

---
 app/controllers/split_checkout_controller.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 7467a8efcc..3f872e296a 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -16,8 +16,6 @@ class SplitCheckoutController < ::BaseController
 
   def edit
     redirect_to_step unless params[:step]
-
-    OrderWorkflow.new(@order).next if @order.cart?
   end
 
   def update

From e3e53b1504bca35e0855190126d0fe49ed898db4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sun, 5 Sep 2021 21:17:15 +0100
Subject: [PATCH 15/20] Ensure feature toggle works for non-logged-in users

---
 app/models/spree/order.rb          | 2 +-
 app/models/spree/order/checkout.rb | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 301f17a422..1d37534b07 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -21,7 +21,7 @@ module Spree
         order.payment_required?
       }
       go_to_state :confirmation, if: ->(order) {
-        Flipper.enabled? :split_checkout, order.user
+        Flipper.enabled? :split_checkout
       }
       go_to_state :complete
     end
diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index dd4d0a80e6..d1e0d5d36c 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -141,7 +141,7 @@ module Spree
           private
 
           def validate_shipping_method!
-            return unless user && Flipper.enabled?(:split_checkout, user)
+            return unless Flipper.enabled?(:split_checkout)
             return if shipping_method.present?
 
             errors.add :shipping_method, I18n.t('split_checkout.errors.select_a_shipping_method')
@@ -149,7 +149,7 @@ module Spree
           end
 
           def validate_payment_method!
-            return unless user && Flipper.enabled?(:split_checkout, user)
+            return unless Flipper.enabled?(:split_checkout)
             return if payments.any?
 
             errors.add :payment_method, I18n.t('split_checkout.errors.select_a_payment_method')

From cfee804339c2caebc7326fe561c806cac98ba648 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 6 Sep 2021 14:19:13 +0100
Subject: [PATCH 16/20] Improve concern loading

It shouldn't need `require` or `prepend`
---
 app/models/concerns/order_shipment.rb | 4 +++-
 app/models/spree/order.rb             | 4 +---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/concerns/order_shipment.rb b/app/models/concerns/order_shipment.rb
index dd5ebeba01..0a1c30f625 100644
--- a/app/models/concerns/order_shipment.rb
+++ b/app/models/concerns/order_shipment.rb
@@ -13,7 +13,9 @@ require 'active_support/concern'
 module OrderShipment
   extend ActiveSupport::Concern
 
-  attr_accessor :manual_shipping_selection
+  included do
+    attr_accessor :manual_shipping_selection
+  end
 
   def shipment
     shipments.first
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 1d37534b07..a549cab3d9 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -4,12 +4,10 @@ require 'spree/order/checkout'
 require 'open_food_network/enterprise_fee_calculator'
 require 'open_food_network/feature_toggle'
 require 'open_food_network/tag_rule_applicator'
-require 'concerns/order_shipment'
 
 module Spree
   class Order < ApplicationRecord
-    prepend OrderShipment
-
+    include OrderShipment
     include Checkout
     include Balance
 

From c3b52ef00acaf9d00114177e4d6c098d6f59ffb2 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 6 Sep 2021 14:19:35 +0100
Subject: [PATCH 17/20] Remove payments validation

---
 app/models/spree/order.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index a549cab3d9..b9dbbc94dd 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -92,7 +92,6 @@ module Spree
     validates :email, presence: true,
                       format: /\A([\w.%+\-']+)@([\w\-]+\.)+(\w{2,})\z/i,
                       if: :require_email
-    validates :payments, presence: true, if: ->(order) { order.confirmation? && payment_required? }
 
     make_permalink field: :number
 

From 71412f8d4b18d5a3c2bb21fcdd9e7931b271ea03 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 6 Sep 2021 14:22:28 +0100
Subject: [PATCH 18/20] Update validations

---
 app/controllers/concerns/checkout_callbacks.rb |  1 +
 app/models/spree/order.rb                      | 14 +++++++++++---
 app/models/spree/order/checkout.rb             | 11 +----------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb
index 638389cd59..bc50a80b09 100644
--- a/app/controllers/concerns/checkout_callbacks.rb
+++ b/app/controllers/concerns/checkout_callbacks.rb
@@ -28,6 +28,7 @@ module CheckoutCallbacks
   def load_order
     @order = current_order
     @order.manual_shipping_selection = true
+    @order.checkout_processing = true
 
     redirect_to(main_app.shop_path) && return if redirect_to_shop?
     redirect_to_cart_path && return unless valid_order_line_items?
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index b9dbbc94dd..b3c2cc90b5 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -24,6 +24,9 @@ module Spree
       go_to_state :complete
     end
 
+    attr_accessor :use_billing
+    attr_accessor :checkout_processing
+
     token_resource
 
     belongs_to :user, class_name: Spree.user_class.to_s
@@ -79,8 +82,6 @@ module Spree
     before_validation :associate_customer, unless: :customer_id?
     before_validation :ensure_customer, unless: :customer_is_valid?
 
-    attr_accessor :use_billing
-
     before_create :link_by_email
     after_create :create_tax_charge!
 
@@ -137,6 +138,13 @@ module Spree
     scope :by_state, lambda { |state| where(state: state) }
     scope :not_state, lambda { |state| where.not(state: state) }
 
+    def initialize(*_args)
+      @checkout_processing = nil
+      @manual_shipping_selection = nil
+
+      super
+    end
+
     # For compatiblity with Calculator::PriceSack
     def amount
       line_items.inject(0.0) { |sum, li| sum + li.amount }
@@ -625,7 +633,7 @@ module Spree
 
     # Determine if email is required (we don't want validation errors before we hit the checkout)
     def require_email
-      return true unless new_record? || (state == 'cart')
+      return true unless (new_record? || cart?) && !checkout_processing
     end
 
     def ensure_line_items_present
diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index d1e0d5d36c..3671639bcc 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -75,7 +75,6 @@ module Spree
 
               before_transition to: :delivery, do: :create_proposed_shipments
               before_transition to: :delivery, do: :ensure_available_shipping_rates
-              before_transition to: :payment, do: :validate_shipping_method!
               before_transition to: :payment, do: :create_tax_charge!
               before_transition to: :confirmation, do: :validate_payment_method!
 
@@ -140,16 +139,8 @@ module Spree
 
           private
 
-          def validate_shipping_method!
-            return unless Flipper.enabled?(:split_checkout)
-            return if shipping_method.present?
-
-            errors.add :shipping_method, I18n.t('split_checkout.errors.select_a_shipping_method')
-            throw :halt
-          end
-
           def validate_payment_method!
-            return unless Flipper.enabled?(:split_checkout)
+            return unless checkout_processing
             return if payments.any?
 
             errors.add :payment_method, I18n.t('split_checkout.errors.select_a_payment_method')

From fb7a3a681b3a75b417e9a06737184efde14beb04 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 6 Sep 2021 14:25:16 +0100
Subject: [PATCH 19/20] Validate attributes per checkout step and decouple
 order-advancing

---
 app/controllers/split_checkout_controller.rb | 26 ++++++++++++++++----
 app/services/order_workflow.rb               |  4 +--
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb
index 3f872e296a..9849f10b19 100644
--- a/app/controllers/split_checkout_controller.rb
+++ b/app/controllers/split_checkout_controller.rb
@@ -21,6 +21,7 @@ class SplitCheckoutController < ::BaseController
   def update
     if confirm_order || update_order
       clear_invalid_payments
+      advance_order_state
       redirect_to_step
     else
       flash.now[:error] = I18n.t('split_checkout.errors.global')
@@ -36,7 +37,7 @@ class SplitCheckoutController < ::BaseController
 
   def confirm_order
     return unless @order.confirmation? && params[:confirm_order]
-    return unless validate_terms_and_conditions!
+    return unless validate_summary! && @order.errors.empty?
 
     @order.confirm!
   end
@@ -44,20 +45,35 @@ class SplitCheckoutController < ::BaseController
   def update_order
     return if @order.errors.any?
 
-    @order.update(order_params) && advance_order_state
+    @order.select_shipping_method(params[:shipping_method_id])
+    @order.update(order_params)
+    send("validate_#{params[:step]}!")
+
+    @order.errors.empty?
   end
 
   def advance_order_state
-    return true if @order.complete?
+    return if @order.complete?
 
     OrderWorkflow.new(@order).advance_checkout(raw_params.slice(:shipping_method_id))
   end
 
-  def validate_terms_and_conditions!
+  def validate_details!
+    return true if params[:shipping_method_id].present?
+
+    @order.errors.add :shipping_method, I18n.t('split_checkout.errors.select_a_shipping_method')
+  end
+
+  def validate_payment!
+    return true if params.dig(:order, :payments_attributes).present?
+
+    @order.errors.add :payment_method, I18n.t('split_checkout.errors.select_a_payment_method')
+  end
+
+  def validate_summary!
     return true if params[:accept_terms]
 
     @order.errors.add(:terms_and_conditions, t("split_checkout.errors.terms_not_accepted"))
-    false
   end
 
   def order_params
diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index 8f8b3028c9..b27a828370 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -28,9 +28,7 @@ class OrderWorkflow
   end
 
   def advance_checkout(options = {})
-    order.select_shipping_method(options[:shipping_method_id])
-
-    advance_to = order.address? || order.delivery? ? "payment" : "confirmation"
+    advance_to = order.state.in?(["cart", "address", "delivery"]) ? "payment" : "confirmation"
 
     advance_to_state(advance_to, advance_order_options.merge(options))
   end

From 3bcfc673f7b7b14972ee7a3ef48f8f8a7f0640ed Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 6 Sep 2021 20:27:10 +0100
Subject: [PATCH 20/20] Ensure selected shipping method is actually selected

Hashes with symbol keys != hashes with string keys :see_no_evil:
---
 app/controllers/checkout_controller.rb | 2 +-
 app/services/order_workflow.rb         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index cffa0877b7..6b0f08ba99 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -175,7 +175,7 @@ class CheckoutController < ::BaseController
         return action_failed unless @order.process_payments!
       end
 
-      next if OrderWorkflow.new(@order).next({ shipping_method_id: shipping_method_id })
+      next if OrderWorkflow.new(@order).next({ "shipping_method_id" => shipping_method_id })
 
       return action_failed
     end
diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index b27a828370..46596b67ce 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -37,7 +37,7 @@ class OrderWorkflow
 
   def advance_order_options
     shipping_method_id = order.shipping_method.id if order.shipping_method.present?
-    { shipping_method_id: shipping_method_id }
+    { "shipping_method_id" => shipping_method_id }
   end
 
   def advance_to_state(target_state, options = {})
@@ -67,7 +67,7 @@ class OrderWorkflow
 
   def after_transition_hook(options)
     if order.state == "delivery"
-      order.select_shipping_method(options[:shipping_method_id])
+      order.select_shipping_method(options["shipping_method_id"])
     end
 
     persist_all_payments if order.state == "payment"