From 5266d95910a9072b128e048fee0502edac9d7925 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 24 Jul 2020 18:09:58 +0100
Subject: [PATCH 01/21] Move method closer to related/similar methods

---
 app/controllers/checkout_controller.rb | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index c83b5e361d..1c8ebbcbaa 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -138,14 +138,6 @@ class CheckoutController < Spree::StoreController
     current_order.payments.destroy_all if request.put?
   end
 
-  def rescue_from_spree_gateway_error(error)
-    flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
-    respond_to do |format|
-      format.html { render :edit }
-      format.json { render json: { flash: flash.to_hash }, status: :bad_request }
-    end
-  end
-
   def valid_payment_intent_provided?
     return false unless params["payment_intent"]&.starts_with?("pi_")
 
@@ -261,6 +253,14 @@ class CheckoutController < Spree::StoreController
     end
   end
 
+  def rescue_from_spree_gateway_error(error)
+    flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
+    respond_to do |format|
+      format.html { render :edit }
+      format.json { render json: { flash: flash.to_hash }, status: :bad_request }
+    end
+  end
+
   def permitted_params
     PermittedAttributes::Checkout.new(params).call
   end

From 1bf946d1242d5788d7ed9e3c207614477a0e2ae3 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 24 Jul 2020 18:14:32 +0100
Subject: [PATCH 02/21] Reused code in checkout controller, the reponse for the
 case when there is a stripe exception anywhere is the same as when the update
 action fails

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

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 1c8ebbcbaa..3d068f1a8b 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -255,10 +255,9 @@ class CheckoutController < Spree::StoreController
 
   def rescue_from_spree_gateway_error(error)
     flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
-    respond_to do |format|
-      format.html { render :edit }
-      format.json { render json: { flash: flash.to_hash }, status: :bad_request }
-    end
+    # This can also happen during the edit action
+    #   but the response needed here is the same as when the update action fails
+    update_failed_response
   end
 
   def permitted_params

From b23b707b5d9daa61fa166244c9a17f37834566a0 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 24 Jul 2020 18:27:51 +0100
Subject: [PATCH 03/21] Notify bugsnag and execute post checkout actions (reset
 to cart state) whenever there's a payment gateway exceeption raised

---
 app/controllers/checkout_controller.rb | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 3d068f1a8b..8c4aea8cc4 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -254,6 +254,9 @@ class CheckoutController < Spree::StoreController
   end
 
   def rescue_from_spree_gateway_error(error)
+    Bugsnag.notify(error)
+    checkout_failed
+
     flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
     # This can also happen during the edit action
     #   but the response needed here is the same as when the update action fails

From ec0d06af542f167f23e049816000acf573f96db6 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Fri, 24 Jul 2020 18:29:47 +0100
Subject: [PATCH 04/21] Reuse update_failed method as the code needed is
 exactly the same

---
 app/controllers/checkout_controller.rb | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 8c4aea8cc4..dd1a67c702 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -254,13 +254,10 @@ class CheckoutController < Spree::StoreController
   end
 
   def rescue_from_spree_gateway_error(error)
-    Bugsnag.notify(error)
-    checkout_failed
-
     flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
     # This can also happen during the edit action
-    #   but the response needed here is the same as when the update action fails
-    update_failed_response
+    #   but the actions and response needed are exactly the same as when the update action fails
+    update_failed(error)
   end
 
   def permitted_params

From d8a96c9d34cfd1df47b0c3e3e9b02eff6fb5f849 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 19:19:46 +0100
Subject: [PATCH 05/21] Bring order checkout workflow and some of its specs
 from spree_core

---
 app/models/spree/order/checkout.rb       | 182 +++++++++++++
 spec/models/spree/order/checkout_spec.rb | 331 +++++++++++++++++++++++
 2 files changed, 513 insertions(+)
 create mode 100644 app/models/spree/order/checkout.rb

diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
new file mode 100644
index 0000000000..0b70dda37a
--- /dev/null
+++ b/app/models/spree/order/checkout.rb
@@ -0,0 +1,182 @@
+module Spree
+  class Order < ActiveRecord::Base
+    module Checkout
+      def self.included(klass)
+        klass.class_eval do
+          class_attribute :next_event_transitions
+          class_attribute :previous_states
+          class_attribute :checkout_flow
+          class_attribute :checkout_steps
+          class_attribute :removed_transitions
+
+          def self.checkout_flow(&block)
+            if block_given?
+              @checkout_flow = block
+              define_state_machine!
+            else
+              @checkout_flow
+            end
+          end
+
+          def self.define_state_machine!
+            self.checkout_steps = {}
+            self.next_event_transitions = []
+            self.previous_states = [:cart]
+            self.removed_transitions = []
+
+            # Build the checkout flow using the checkout_flow defined either
+            # within the Order class, or a decorator for that class.
+            #
+            # This method may be called multiple times depending on if the
+            # checkout_flow is re-defined in a decorator or not.
+            instance_eval(&checkout_flow)
+
+            klass = self
+
+            # To avoid a ton of warnings when the state machine is re-defined
+            StateMachine::Machine.ignore_method_conflicts = true
+            # To avoid multiple occurrences of the same transition being defined
+            # On first definition, state_machines will not be defined
+            state_machines.clear if respond_to?(:state_machines)
+            state_machine :state, :initial => :cart do
+              klass.next_event_transitions.each { |t| transition(t.merge(:on => :next)) }
+
+              # Persist the state on the order
+              after_transition do |order|
+                order.state = order.state
+                order.save
+              end
+
+              event :cancel do
+                transition :to => :canceled, :if => :allow_cancel?
+              end
+
+              event :return do
+                transition :to => :returned, :from => :awaiting_return, :unless => :awaiting_returns?
+              end
+
+              event :resume do
+                transition :to => :resumed, :from => :canceled, :if => :allow_resume?
+              end
+
+              event :authorize_return do
+                transition :to => :awaiting_return
+              end
+
+              if states[:payment]
+                before_transition :to => :complete do |order|
+                  order.process_payments! if order.payment_required?
+                end
+              end
+
+              before_transition :from => :cart, :do => :ensure_line_items_present
+
+              before_transition :to => :delivery, :do => :create_proposed_shipments
+              before_transition :to => :delivery, :do => :ensure_available_shipping_rates
+
+              after_transition :to => :complete, :do => :finalize!
+              after_transition :to => :delivery, :do => :create_tax_charge!
+              after_transition :to => :resumed,  :do => :after_resume
+              after_transition :to => :canceled, :do => :after_cancel
+            end
+          end
+
+          def self.go_to_state(name, options={})
+            self.checkout_steps[name] = options
+            previous_states.each do |state|
+              add_transition({:from => state, :to => name}.merge(options))
+            end
+            if options[:if]
+              self.previous_states << name
+            else
+              self.previous_states = [name]
+            end
+          end
+
+          def self.insert_checkout_step(name, options = {})
+            before = options.delete(:before)
+            after = options.delete(:after) unless before
+            after = self.checkout_steps.keys.last unless before || after
+
+            cloned_steps = self.checkout_steps.clone
+            cloned_removed_transitions = self.removed_transitions.clone
+            self.checkout_flow do
+              cloned_steps.each_pair do |key, value|
+                self.go_to_state(name, options) if key == before
+                self.go_to_state(key, value)
+                self.go_to_state(name, options) if key == after
+              end
+              cloned_removed_transitions.each do |transition|
+                self.remove_transition(transition)
+              end
+            end
+          end
+
+          def self.remove_checkout_step(name)
+            cloned_steps = self.checkout_steps.clone
+            cloned_removed_transitions = self.removed_transitions.clone
+            self.checkout_flow do
+              cloned_steps.each_pair do |key, value|
+                self.go_to_state(key, value) unless key == name
+              end
+              cloned_removed_transitions.each do |transition|
+                self.remove_transition(transition)
+              end
+            end
+          end
+
+          def self.remove_transition(options={})
+            self.removed_transitions << options
+            self.next_event_transitions.delete(find_transition(options))
+          end
+
+          def self.find_transition(options={})
+            return nil if options.nil? || !options.include?(:from) || !options.include?(:to)
+            self.next_event_transitions.detect do |transition|
+              transition[options[:from].to_sym] == options[:to].to_sym
+            end
+          end
+
+          def self.next_event_transitions
+            @next_event_transitions ||= []
+          end
+
+          def self.checkout_steps
+            @checkout_steps ||= {}
+          end
+
+          def self.add_transition(options)
+            self.next_event_transitions << { options.delete(:from) => options.delete(:to) }.merge(options)
+          end
+
+          def checkout_steps
+            steps = self.class.checkout_steps.each_with_object([]) { |(step, options), checkout_steps|
+              next if options.include?(:if) && !options[:if].call(self)
+              checkout_steps << step
+            }.map(&:to_s)
+            # Ensure there is always a complete step
+            steps << "complete" unless steps.include?("complete")
+            steps
+          end
+
+          def has_checkout_step?(step)
+            step.present? ? self.checkout_steps.include?(step) : false
+          end
+
+          def checkout_step_index(step)
+            self.checkout_steps.index(step)
+          end
+
+          def self.removed_transitions
+            @removed_transitions ||= []
+          end
+
+          def can_go_to_state?(state)
+            return false unless self.state.present? && has_checkout_step?(state) && has_checkout_step?(self.state)
+            checkout_step_index(state) > checkout_step_index(self.state)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/models/spree/order/checkout_spec.rb b/spec/models/spree/order/checkout_spec.rb
index 8eed383c6a..4cc9ca5101 100644
--- a/spec/models/spree/order/checkout_spec.rb
+++ b/spec/models/spree/order/checkout_spec.rb
@@ -1,6 +1,337 @@
 require 'spec_helper'
 
 describe Spree::Order do
+  let(:order) { Spree::Order.new }
+
+  context "with default state machine" do
+    let(:transitions) do
+      [
+        { :address => :delivery },
+        { :delivery => :payment },
+        { :payment => :confirm },
+        { :confirm => :complete },
+        { :payment => :complete },
+        { :delivery => :complete }
+      ]
+    end
+
+    it "has the following transitions" do
+      transitions.each do |transition|
+        transition = Spree::Order.find_transition(:from => transition.keys.first, :to => transition.values.first)
+        transition.should_not be_nil
+      end
+    end
+
+    it "does not have a transition from delivery to confirm" do
+      transition = Spree::Order.find_transition(:from => :delivery, :to => :confirm)
+      transition.should be_nil
+    end
+      
+    it '.find_transition when contract was broken' do
+      Spree::Order.find_transition({foo: :bar, baz: :dog}).should be_false
+    end
+
+    it '.remove_transition' do
+      options = {:from => transitions.first.keys.first, :to => transitions.first.values.first}
+      Spree::Order.stub(:next_event_transition).and_return([options])
+      Spree::Order.remove_transition(options).should be_true
+    end
+
+    it '.remove_transition when contract was broken' do
+      Spree::Order.remove_transition(nil).should be_false
+    end
+
+    context "#checkout_steps" do
+      context "when confirmation not required" do
+        before do
+          order.stub :confirmation_required? => false
+          order.stub :payment_required? => true
+        end
+
+        specify do
+          order.checkout_steps.should == %w(address delivery payment complete)
+        end
+      end
+
+      context "when confirmation required" do
+        before do
+          order.stub :confirmation_required? => true
+          order.stub :payment_required? => true
+        end
+
+        specify do
+          order.checkout_steps.should == %w(address delivery payment confirm complete)
+        end
+      end
+
+      context "when payment not required" do
+        before { order.stub :payment_required? => false }
+        specify do
+          order.checkout_steps.should == %w(address delivery complete)
+        end
+      end
+
+      context "when payment required" do
+        before { order.stub :payment_required? => true }
+        specify do
+          order.checkout_steps.should == %w(address delivery payment complete)
+        end
+      end
+    end
+
+    it "starts out at cart" do
+      order.state.should == "cart"
+    end
+
+    it "transitions to address" do
+      order.line_items << FactoryGirl.create(:line_item)
+      order.email = "user@example.com"
+      order.next!
+      order.state.should == "address"
+    end
+
+    it "cannot transition to address without any line items" do
+      order.line_items.should be_blank
+      lambda { order.next! }.should raise_error(StateMachine::InvalidTransition, /#{Spree.t(:there_are_no_items_for_this_order)}/)
+    end
+
+    context "from address" do
+      before do
+        order.state = 'address'
+        order.stub(:has_available_payment)
+        shipment = FactoryGirl.create(:shipment, :order => order)
+        order.email = "user@example.com"
+        order.save!
+      end
+
+      it "transitions to delivery" do
+        order.stub(:ensure_available_shipping_rates => true)
+        order.next!
+        order.state.should == "delivery"
+      end
+
+      context "cannot transition to delivery" do
+        context "if there are no shipping rates for any shipment" do
+          specify do
+            transition = lambda { order.next! }
+            transition.should raise_error(StateMachine::InvalidTransition, /#{Spree.t(:items_cannot_be_shipped)}/)
+          end
+        end
+      end
+    end
+
+    context "from delivery" do
+      before do
+        order.state = 'delivery'
+      end
+
+      context "with payment required" do
+        before do
+          order.stub :payment_required? => true
+        end
+
+        it "transitions to payment" do
+          order.next!
+          order.state.should == 'payment'
+        end
+      end
+
+      context "without payment required" do
+        before do
+          order.stub :payment_required? => false
+        end
+
+        it "transitions to complete" do
+          order.next!
+          order.state.should == "complete"
+        end
+      end
+    end
+
+    context "from payment" do
+      before do
+        order.state = 'payment'
+      end
+
+      context "with confirmation required" do
+        before do
+          order.stub :confirmation_required? => true
+        end
+
+        it "transitions to confirm" do
+          order.next!
+          order.state.should == "confirm"
+        end
+      end
+
+      context "without confirmation required" do
+        before do
+          order.stub :confirmation_required? => false
+          order.stub :payment_required? => true
+        end
+
+        it "transitions to complete" do
+          order.should_receive(:process_payments!).once.and_return true
+          order.next!
+          order.state.should == "complete"
+        end
+      end
+
+      # Regression test for #2028
+      context "when payment is not required" do
+        before do
+          order.stub :payment_required? => false
+        end
+
+        it "does not call process payments" do
+          order.should_not_receive(:process_payments!)
+          order.next!
+          order.state.should == "complete"
+        end
+      end
+    end
+  end
+
+  context "subclassed order" do
+    # This causes another test above to fail, but fixing this test should make
+    #   the other test pass
+    class SubclassedOrder < Spree::Order
+      checkout_flow do
+        go_to_state :payment
+        go_to_state :complete
+      end
+    end
+
+    it "should only call default transitions once when checkout_flow is redefined" do
+      order = SubclassedOrder.new
+      order.stub :payment_required? => true
+      order.should_receive(:process_payments!).once
+      order.state = "payment"
+      order.next!
+      order.state.should == "complete"
+    end
+  end
+
+  context "re-define checkout flow" do
+    before do
+      @old_checkout_flow = Spree::Order.checkout_flow
+      Spree::Order.class_eval do
+        checkout_flow do
+          go_to_state :payment
+          go_to_state :complete
+        end
+      end
+    end
+
+    after do
+      Spree::Order.checkout_flow(&@old_checkout_flow)
+    end
+
+    it "should not keep old event transitions when checkout_flow is redefined" do
+      Spree::Order.next_event_transitions.should == [{:cart=>:payment}, {:payment=>:complete}]
+    end
+
+    it "should not keep old events when checkout_flow is redefined" do
+      state_machine = Spree::Order.state_machine
+      state_machine.states.any? { |s| s.name == :address }.should be_false
+      known_states = state_machine.events[:next].branches.map(&:known_states).flatten
+      known_states.should_not include(:address)
+      known_states.should_not include(:delivery)
+      known_states.should_not include(:confirm)
+    end
+  end
+
+  # Regression test for #3665
+  context "with only a complete step" do
+    before do
+      @old_checkout_flow = Spree::Order.checkout_flow
+      Spree::Order.class_eval do
+        checkout_flow do
+          go_to_state :complete
+        end
+      end
+    end
+
+    after do
+      Spree::Order.checkout_flow(&@old_checkout_flow)
+    end
+
+    it "does not attempt to process payments" do
+      order.stub_chain(:line_items, :present?).and_return(true)
+      order.should_not_receive(:payment_required?)
+      order.should_not_receive(:process_payments!)
+      order.next!
+    end
+
+  end
+
+  context "insert checkout step" do
+    before do
+      @old_checkout_flow = Spree::Order.checkout_flow
+      Spree::Order.class_eval do
+        insert_checkout_step :new_step, before: :address
+      end
+    end
+
+    after do
+      Spree::Order.checkout_flow(&@old_checkout_flow)
+    end
+
+    it "should maintain removed transitions" do
+      transition = Spree::Order.find_transition(:from => :delivery, :to => :confirm)
+      transition.should be_nil
+    end
+
+    context "before" do
+      before do
+        Spree::Order.class_eval do
+          insert_checkout_step :before_address, before: :address
+        end
+      end
+
+      specify do
+        order = Spree::Order.new
+        order.checkout_steps.should == %w(new_step before_address address delivery complete)
+      end
+    end
+
+    context "after" do
+      before do
+        Spree::Order.class_eval do
+          insert_checkout_step :after_address, after: :address
+        end
+      end
+
+      specify do
+        order = Spree::Order.new
+        order.checkout_steps.should == %w(new_step address after_address delivery complete)
+      end
+    end
+  end
+
+  context "remove checkout step" do
+    before do
+      @old_checkout_flow = Spree::Order.checkout_flow
+      Spree::Order.class_eval do
+        remove_checkout_step :address
+      end
+    end
+
+    after do
+      Spree::Order.checkout_flow(&@old_checkout_flow)
+    end
+
+    it "should maintain removed transitions" do
+      transition = Spree::Order.find_transition(:from => :delivery, :to => :confirm)
+      transition.should be_nil
+    end
+
+    specify do
+      order = Spree::Order.new
+      order.checkout_steps.should == %w(delivery complete)
+    end
+  end
+
   describe 'event :restart_checkout' do
     let(:order) { create(:order) }
 

From e99f0dc6b70fc447d8d965e2c2a9c1df916d8eed Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 19:29:53 +0100
Subject: [PATCH 06/21] Rubocop autocorrect and easy rubocop issues

---
 app/models/spree/order/checkout.rb       | 93 +++++++++++++-----------
 spec/models/spree/order/checkout_spec.rb | 66 +++++++++--------
 2 files changed, 85 insertions(+), 74 deletions(-)

diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index 0b70dda37a..ffecf63f23 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Spree
   class Order < ActiveRecord::Base
     module Checkout
@@ -38,8 +40,8 @@ module Spree
             # To avoid multiple occurrences of the same transition being defined
             # On first definition, state_machines will not be defined
             state_machines.clear if respond_to?(:state_machines)
-            state_machine :state, :initial => :cart do
-              klass.next_event_transitions.each { |t| transition(t.merge(:on => :next)) }
+            state_machine :state, initial: :cart do
+              klass.next_event_transitions.each { |t| transition(t.merge(on: :next)) }
 
               # Persist the state on the order
               after_transition do |order|
@@ -48,46 +50,46 @@ module Spree
               end
 
               event :cancel do
-                transition :to => :canceled, :if => :allow_cancel?
+                transition to: :canceled, if: :allow_cancel?
               end
 
               event :return do
-                transition :to => :returned, :from => :awaiting_return, :unless => :awaiting_returns?
+                transition to: :returned, from: :awaiting_return, unless: :awaiting_returns?
               end
 
               event :resume do
-                transition :to => :resumed, :from => :canceled, :if => :allow_resume?
+                transition to: :resumed, from: :canceled, if: :allow_resume?
               end
 
               event :authorize_return do
-                transition :to => :awaiting_return
+                transition to: :awaiting_return
               end
 
               if states[:payment]
-                before_transition :to => :complete do |order|
+                before_transition to: :complete do |order|
                   order.process_payments! if order.payment_required?
                 end
               end
 
-              before_transition :from => :cart, :do => :ensure_line_items_present
+              before_transition from: :cart, do: :ensure_line_items_present
 
-              before_transition :to => :delivery, :do => :create_proposed_shipments
-              before_transition :to => :delivery, :do => :ensure_available_shipping_rates
+              before_transition to: :delivery, do: :create_proposed_shipments
+              before_transition to: :delivery, do: :ensure_available_shipping_rates
 
-              after_transition :to => :complete, :do => :finalize!
-              after_transition :to => :delivery, :do => :create_tax_charge!
-              after_transition :to => :resumed,  :do => :after_resume
-              after_transition :to => :canceled, :do => :after_cancel
+              after_transition to: :complete, do: :finalize!
+              after_transition to: :delivery, do: :create_tax_charge!
+              after_transition to: :resumed,  do: :after_resume
+              after_transition to: :canceled, do: :after_cancel
             end
           end
 
-          def self.go_to_state(name, options={})
-            self.checkout_steps[name] = options
+          def self.go_to_state(name, options = {})
+            checkout_steps[name] = options
             previous_states.each do |state|
-              add_transition({:from => state, :to => name}.merge(options))
+              add_transition({ from: state, to: name }.merge(options))
             end
             if options[:if]
-              self.previous_states << name
+              previous_states << name
             else
               self.previous_states = [name]
             end
@@ -96,43 +98,44 @@ module Spree
           def self.insert_checkout_step(name, options = {})
             before = options.delete(:before)
             after = options.delete(:after) unless before
-            after = self.checkout_steps.keys.last unless before || after
+            after = checkout_steps.keys.last unless before || after
 
-            cloned_steps = self.checkout_steps.clone
-            cloned_removed_transitions = self.removed_transitions.clone
-            self.checkout_flow do
+            cloned_steps = checkout_steps.clone
+            cloned_removed_transitions = removed_transitions.clone
+            checkout_flow do
               cloned_steps.each_pair do |key, value|
-                self.go_to_state(name, options) if key == before
-                self.go_to_state(key, value)
-                self.go_to_state(name, options) if key == after
+                go_to_state(name, options) if key == before
+                go_to_state(key, value)
+                go_to_state(name, options) if key == after
               end
               cloned_removed_transitions.each do |transition|
-                self.remove_transition(transition)
+                remove_transition(transition)
               end
             end
           end
 
           def self.remove_checkout_step(name)
-            cloned_steps = self.checkout_steps.clone
-            cloned_removed_transitions = self.removed_transitions.clone
-            self.checkout_flow do
+            cloned_steps = checkout_steps.clone
+            cloned_removed_transitions = removed_transitions.clone
+            checkout_flow do
               cloned_steps.each_pair do |key, value|
-                self.go_to_state(key, value) unless key == name
+                go_to_state(key, value) unless key == name
               end
               cloned_removed_transitions.each do |transition|
-                self.remove_transition(transition)
+                remove_transition(transition)
               end
             end
           end
 
-          def self.remove_transition(options={})
-            self.removed_transitions << options
-            self.next_event_transitions.delete(find_transition(options))
+          def self.remove_transition(options = {})
+            removed_transitions << options
+            next_event_transitions.delete(find_transition(options))
           end
 
-          def self.find_transition(options={})
+          def self.find_transition(options = {})
             return nil if options.nil? || !options.include?(:from) || !options.include?(:to)
-            self.next_event_transitions.detect do |transition|
+
+            next_event_transitions.detect do |transition|
               transition[options[:from].to_sym] == options[:to].to_sym
             end
           end
@@ -146,12 +149,15 @@ module Spree
           end
 
           def self.add_transition(options)
-            self.next_event_transitions << { options.delete(:from) => options.delete(:to) }.merge(options)
+            next_event_transitions << { options.delete(:from) => options.delete(:to) }.
+              merge(options)
           end
 
           def checkout_steps
-            steps = self.class.checkout_steps.each_with_object([]) { |(step, options), checkout_steps|
+            steps = self.class.checkout_steps.
+              each_with_object([]) { |(step, options), checkout_steps|
               next if options.include?(:if) && !options[:if].call(self)
+
               checkout_steps << step
             }.map(&:to_s)
             # Ensure there is always a complete step
@@ -159,12 +165,12 @@ module Spree
             steps
           end
 
-          def has_checkout_step?(step)
-            step.present? ? self.checkout_steps.include?(step) : false
+          def checkout_step?(step)
+            step.present? ? checkout_steps.include?(step) : false
           end
 
           def checkout_step_index(step)
-            self.checkout_steps.index(step)
+            checkout_steps.index(step)
           end
 
           def self.removed_transitions
@@ -172,7 +178,10 @@ module Spree
           end
 
           def can_go_to_state?(state)
-            return false unless self.state.present? && has_checkout_step?(state) && has_checkout_step?(self.state)
+            return false unless self.state.present? &&
+                                checkout_step?(state) &&
+                                checkout_step?(self.state)
+
             checkout_step_index(state) > checkout_step_index(self.state)
           end
         end
diff --git a/spec/models/spree/order/checkout_spec.rb b/spec/models/spree/order/checkout_spec.rb
index 4cc9ca5101..75a3ae6a95 100644
--- a/spec/models/spree/order/checkout_spec.rb
+++ b/spec/models/spree/order/checkout_spec.rb
@@ -6,33 +6,34 @@ describe Spree::Order do
   context "with default state machine" do
     let(:transitions) do
       [
-        { :address => :delivery },
-        { :delivery => :payment },
-        { :payment => :confirm },
-        { :confirm => :complete },
-        { :payment => :complete },
-        { :delivery => :complete }
+        { address: :delivery },
+        { delivery: :payment },
+        { payment: :confirm },
+        { confirm: :complete },
+        { payment: :complete },
+        { delivery: :complete }
       ]
     end
 
     it "has the following transitions" do
       transitions.each do |transition|
-        transition = Spree::Order.find_transition(:from => transition.keys.first, :to => transition.values.first)
+        transition = Spree::Order.find_transition(from: transition.keys.first,
+                                                  to: transition.values.first)
         transition.should_not be_nil
       end
     end
 
     it "does not have a transition from delivery to confirm" do
-      transition = Spree::Order.find_transition(:from => :delivery, :to => :confirm)
+      transition = Spree::Order.find_transition(from: :delivery, to: :confirm)
       transition.should be_nil
     end
-      
+
     it '.find_transition when contract was broken' do
-      Spree::Order.find_transition({foo: :bar, baz: :dog}).should be_false
+      Spree::Order.find_transition({ foo: :bar, baz: :dog }).should be_false
     end
 
     it '.remove_transition' do
-      options = {:from => transitions.first.keys.first, :to => transitions.first.values.first}
+      options = { from: transitions.first.keys.first, to: transitions.first.values.first }
       Spree::Order.stub(:next_event_transition).and_return([options])
       Spree::Order.remove_transition(options).should be_true
     end
@@ -44,8 +45,8 @@ describe Spree::Order do
     context "#checkout_steps" do
       context "when confirmation not required" do
         before do
-          order.stub :confirmation_required? => false
-          order.stub :payment_required? => true
+          order.stub confirmation_required?: false
+          order.stub payment_required?: true
         end
 
         specify do
@@ -55,8 +56,8 @@ describe Spree::Order do
 
       context "when confirmation required" do
         before do
-          order.stub :confirmation_required? => true
-          order.stub :payment_required? => true
+          order.stub confirmation_required?: true
+          order.stub payment_required?: true
         end
 
         specify do
@@ -65,14 +66,14 @@ describe Spree::Order do
       end
 
       context "when payment not required" do
-        before { order.stub :payment_required? => false }
+        before { order.stub payment_required?: false }
         specify do
           order.checkout_steps.should == %w(address delivery complete)
         end
       end
 
       context "when payment required" do
-        before { order.stub :payment_required? => true }
+        before { order.stub payment_required?: true }
         specify do
           order.checkout_steps.should == %w(address delivery payment complete)
         end
@@ -92,20 +93,21 @@ describe Spree::Order do
 
     it "cannot transition to address without any line items" do
       order.line_items.should be_blank
-      lambda { order.next! }.should raise_error(StateMachine::InvalidTransition, /#{Spree.t(:there_are_no_items_for_this_order)}/)
+      lambda { order.next! }.should raise_error(StateMachine::InvalidTransition,
+                                                /#{Spree.t(:there_are_no_items_for_this_order)}/)
     end
 
     context "from address" do
       before do
         order.state = 'address'
         order.stub(:has_available_payment)
-        shipment = FactoryGirl.create(:shipment, :order => order)
+        shipment = FactoryGirl.create(:shipment, order: order)
         order.email = "user@example.com"
         order.save!
       end
 
       it "transitions to delivery" do
-        order.stub(:ensure_available_shipping_rates => true)
+        order.stub(ensure_available_shipping_rates: true)
         order.next!
         order.state.should == "delivery"
       end
@@ -114,7 +116,8 @@ describe Spree::Order do
         context "if there are no shipping rates for any shipment" do
           specify do
             transition = lambda { order.next! }
-            transition.should raise_error(StateMachine::InvalidTransition, /#{Spree.t(:items_cannot_be_shipped)}/)
+            transition.should raise_error(StateMachine::InvalidTransition,
+                                          /#{Spree.t(:items_cannot_be_shipped)}/)
           end
         end
       end
@@ -127,7 +130,7 @@ describe Spree::Order do
 
       context "with payment required" do
         before do
-          order.stub :payment_required? => true
+          order.stub payment_required?: true
         end
 
         it "transitions to payment" do
@@ -138,7 +141,7 @@ describe Spree::Order do
 
       context "without payment required" do
         before do
-          order.stub :payment_required? => false
+          order.stub payment_required?: false
         end
 
         it "transitions to complete" do
@@ -155,7 +158,7 @@ describe Spree::Order do
 
       context "with confirmation required" do
         before do
-          order.stub :confirmation_required? => true
+          order.stub confirmation_required?: true
         end
 
         it "transitions to confirm" do
@@ -166,8 +169,8 @@ describe Spree::Order do
 
       context "without confirmation required" do
         before do
-          order.stub :confirmation_required? => false
-          order.stub :payment_required? => true
+          order.stub confirmation_required?: false
+          order.stub payment_required?: true
         end
 
         it "transitions to complete" do
@@ -180,7 +183,7 @@ describe Spree::Order do
       # Regression test for #2028
       context "when payment is not required" do
         before do
-          order.stub :payment_required? => false
+          order.stub payment_required?: false
         end
 
         it "does not call process payments" do
@@ -204,7 +207,7 @@ describe Spree::Order do
 
     it "should only call default transitions once when checkout_flow is redefined" do
       order = SubclassedOrder.new
-      order.stub :payment_required? => true
+      order.stub payment_required?: true
       order.should_receive(:process_payments!).once
       order.state = "payment"
       order.next!
@@ -228,7 +231,7 @@ describe Spree::Order do
     end
 
     it "should not keep old event transitions when checkout_flow is redefined" do
-      Spree::Order.next_event_transitions.should == [{:cart=>:payment}, {:payment=>:complete}]
+      Spree::Order.next_event_transitions.should == [{ cart: :payment }, { payment: :complete }]
     end
 
     it "should not keep old events when checkout_flow is redefined" do
@@ -262,7 +265,6 @@ describe Spree::Order do
       order.should_not_receive(:process_payments!)
       order.next!
     end
-
   end
 
   context "insert checkout step" do
@@ -278,7 +280,7 @@ describe Spree::Order do
     end
 
     it "should maintain removed transitions" do
-      transition = Spree::Order.find_transition(:from => :delivery, :to => :confirm)
+      transition = Spree::Order.find_transition(from: :delivery, to: :confirm)
       transition.should be_nil
     end
 
@@ -322,7 +324,7 @@ describe Spree::Order do
     end
 
     it "should maintain removed transitions" do
-      transition = Spree::Order.find_transition(:from => :delivery, :to => :confirm)
+      transition = Spree::Order.find_transition(from: :delivery, to: :confirm)
       transition.should be_nil
     end
 

From 51de5269dcb8bcf84bfe8b5757ea61ce010c45e3 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 19:50:34 +0100
Subject: [PATCH 07/21] Fix specs in checkout_spec

---
 spec/models/spree/order/checkout_spec.rb | 107 ++++++++---------------
 1 file changed, 37 insertions(+), 70 deletions(-)

diff --git a/spec/models/spree/order/checkout_spec.rb b/spec/models/spree/order/checkout_spec.rb
index 75a3ae6a95..d4ac24d7e4 100644
--- a/spec/models/spree/order/checkout_spec.rb
+++ b/spec/models/spree/order/checkout_spec.rb
@@ -8,8 +8,6 @@ describe Spree::Order do
       [
         { address: :delivery },
         { delivery: :payment },
-        { payment: :confirm },
-        { confirm: :complete },
         { payment: :complete },
         { delivery: :complete }
       ]
@@ -17,83 +15,63 @@ describe Spree::Order do
 
     it "has the following transitions" do
       transitions.each do |transition|
+        puts transition.keys.first
+        puts transition.values.first
         transition = Spree::Order.find_transition(from: transition.keys.first,
                                                   to: transition.values.first)
-        transition.should_not be_nil
+        expect(transition).to_not be_nil
       end
     end
 
     it "does not have a transition from delivery to confirm" do
       transition = Spree::Order.find_transition(from: :delivery, to: :confirm)
-      transition.should be_nil
+      expect(transition).to be_nil
     end
 
     it '.find_transition when contract was broken' do
-      Spree::Order.find_transition({ foo: :bar, baz: :dog }).should be_false
+      expect(Spree::Order.find_transition({ foo: :bar, baz: :dog })).to be_falsy
     end
 
     it '.remove_transition' do
       options = { from: transitions.first.keys.first, to: transitions.first.values.first }
       Spree::Order.stub(:next_event_transition).and_return([options])
-      Spree::Order.remove_transition(options).should be_true
+      expect(Spree::Order.remove_transition(options)).to be_truthy
     end
 
     it '.remove_transition when contract was broken' do
-      Spree::Order.remove_transition(nil).should be_false
+      expect(Spree::Order.remove_transition(nil)).to be_falsy
     end
 
     context "#checkout_steps" do
-      context "when confirmation not required" do
-        before do
-          order.stub confirmation_required?: false
-          order.stub payment_required?: true
-        end
-
-        specify do
-          order.checkout_steps.should == %w(address delivery payment complete)
-        end
-      end
-
-      context "when confirmation required" do
-        before do
-          order.stub confirmation_required?: true
-          order.stub payment_required?: true
-        end
-
-        specify do
-          order.checkout_steps.should == %w(address delivery payment confirm complete)
-        end
-      end
-
       context "when payment not required" do
         before { order.stub payment_required?: false }
         specify do
-          order.checkout_steps.should == %w(address delivery complete)
+          expect(order.checkout_steps).to eq %w(address delivery complete)
         end
       end
 
       context "when payment required" do
         before { order.stub payment_required?: true }
         specify do
-          order.checkout_steps.should == %w(address delivery payment complete)
+          expect(order.checkout_steps).to eq %w(address delivery payment complete)
         end
       end
     end
 
     it "starts out at cart" do
-      order.state.should == "cart"
+      expect(order.state).to eq "cart"
     end
 
     it "transitions to address" do
       order.line_items << FactoryGirl.create(:line_item)
       order.email = "user@example.com"
       order.next!
-      order.state.should == "address"
+      expect(order.state).to eq "address"
     end
 
     it "cannot transition to address without any line items" do
-      order.line_items.should be_blank
-      lambda { order.next! }.should raise_error(StateMachine::InvalidTransition,
+      expect(order.line_items).to be_blank
+      expect(lambda { order.next! }).to raise_error(StateMachine::InvalidTransition,
                                                 /#{Spree.t(:there_are_no_items_for_this_order)}/)
     end
 
@@ -101,7 +79,7 @@ describe Spree::Order do
       before do
         order.state = 'address'
         order.stub(:has_available_payment)
-        shipment = FactoryGirl.create(:shipment, order: order)
+        order.shipments << create(:shipment)
         order.email = "user@example.com"
         order.save!
       end
@@ -109,14 +87,14 @@ describe Spree::Order do
       it "transitions to delivery" do
         order.stub(ensure_available_shipping_rates: true)
         order.next!
-        order.state.should == "delivery"
+        expect(order.state).to eq "delivery"
       end
 
       context "cannot transition to delivery" do
         context "if there are no shipping rates for any shipment" do
           specify do
             transition = lambda { order.next! }
-            transition.should raise_error(StateMachine::InvalidTransition,
+            expect(transition).to raise_error(StateMachine::InvalidTransition,
                                           /#{Spree.t(:items_cannot_be_shipped)}/)
           end
         end
@@ -135,7 +113,7 @@ describe Spree::Order do
 
         it "transitions to payment" do
           order.next!
-          order.state.should == 'payment'
+          expect(order.state).to eq 'payment'
         end
       end
 
@@ -146,7 +124,7 @@ describe Spree::Order do
 
         it "transitions to complete" do
           order.next!
-          order.state.should == "complete"
+          expect(order.state).to eq "complete"
         end
       end
     end
@@ -156,27 +134,16 @@ describe Spree::Order do
         order.state = 'payment'
       end
 
-      context "with confirmation required" do
-        before do
-          order.stub confirmation_required?: true
-        end
-
-        it "transitions to confirm" do
-          order.next!
-          order.state.should == "confirm"
-        end
-      end
-
-      context "without confirmation required" do
+     context "when payment is required" do
         before do
           order.stub confirmation_required?: false
           order.stub payment_required?: true
         end
 
         it "transitions to complete" do
-          order.should_receive(:process_payments!).once.and_return true
+          expect(order).to receive(:process_payments!).once.and_return true
           order.next!
-          order.state.should == "complete"
+          expect(order.state).to eq "complete"
         end
       end
 
@@ -187,9 +154,9 @@ describe Spree::Order do
         end
 
         it "does not call process payments" do
-          order.should_not_receive(:process_payments!)
+          expect(order).to_not receive(:process_payments!)
           order.next!
-          order.state.should == "complete"
+          expect(order.state).to eq "complete"
         end
       end
     end
@@ -208,10 +175,10 @@ describe Spree::Order do
     it "should only call default transitions once when checkout_flow is redefined" do
       order = SubclassedOrder.new
       order.stub payment_required?: true
-      order.should_receive(:process_payments!).once
+      expect(order).to receive(:process_payments!).once
       order.state = "payment"
       order.next!
-      order.state.should == "complete"
+      expect(order.state).to eq "complete"
     end
   end
 
@@ -231,16 +198,16 @@ describe Spree::Order do
     end
 
     it "should not keep old event transitions when checkout_flow is redefined" do
-      Spree::Order.next_event_transitions.should == [{ cart: :payment }, { payment: :complete }]
+      expect(Spree::Order.next_event_transitions).to eq [{ cart: :payment }, { payment: :complete }]
     end
 
     it "should not keep old events when checkout_flow is redefined" do
       state_machine = Spree::Order.state_machine
-      state_machine.states.any? { |s| s.name == :address }.should be_false
+      expect(state_machine.states.any? { |s| s.name == :address }).to be_falsy
       known_states = state_machine.events[:next].branches.map(&:known_states).flatten
-      known_states.should_not include(:address)
-      known_states.should_not include(:delivery)
-      known_states.should_not include(:confirm)
+      expect(known_states).to_not include(:address)
+      expect(known_states).to_not include(:delivery)
+      expect(known_states).to_not include(:confirm)
     end
   end
 
@@ -261,8 +228,8 @@ describe Spree::Order do
 
     it "does not attempt to process payments" do
       order.stub_chain(:line_items, :present?).and_return(true)
-      order.should_not_receive(:payment_required?)
-      order.should_not_receive(:process_payments!)
+      expect(order).to_not receive(:payment_required?)
+      expect(order).to_not receive(:process_payments!)
       order.next!
     end
   end
@@ -281,7 +248,7 @@ describe Spree::Order do
 
     it "should maintain removed transitions" do
       transition = Spree::Order.find_transition(from: :delivery, to: :confirm)
-      transition.should be_nil
+      expect(transition).to be_nil
     end
 
     context "before" do
@@ -293,7 +260,7 @@ describe Spree::Order do
 
       specify do
         order = Spree::Order.new
-        order.checkout_steps.should == %w(new_step before_address address delivery complete)
+        expect(order.checkout_steps).to eq %w(new_step before_address address delivery complete)
       end
     end
 
@@ -306,7 +273,7 @@ describe Spree::Order do
 
       specify do
         order = Spree::Order.new
-        order.checkout_steps.should == %w(new_step address after_address delivery complete)
+        expect(order.checkout_steps).to eq %w(new_step address after_address delivery complete)
       end
     end
   end
@@ -325,12 +292,12 @@ describe Spree::Order do
 
     it "should maintain removed transitions" do
       transition = Spree::Order.find_transition(from: :delivery, to: :confirm)
-      transition.should be_nil
+      expect(transition).to be_nil
     end
 
     specify do
       order = Spree::Order.new
-      order.checkout_steps.should == %w(delivery complete)
+      expect(order.checkout_steps).to eq %w(delivery complete)
     end
   end
 

From e80337a458d87ef26a6ec84e085600741c02f4ad Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 20:07:31 +0100
Subject: [PATCH 08/21] Transpec checkout_spec

---
 spec/models/spree/order/checkout_spec.rb | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/spec/models/spree/order/checkout_spec.rb b/spec/models/spree/order/checkout_spec.rb
index d4ac24d7e4..5f49792aa2 100644
--- a/spec/models/spree/order/checkout_spec.rb
+++ b/spec/models/spree/order/checkout_spec.rb
@@ -34,7 +34,7 @@ describe Spree::Order do
 
     it '.remove_transition' do
       options = { from: transitions.first.keys.first, to: transitions.first.values.first }
-      Spree::Order.stub(:next_event_transition).and_return([options])
+      allow(Spree::Order).to receive(:next_event_transition).and_return([options])
       expect(Spree::Order.remove_transition(options)).to be_truthy
     end
 
@@ -44,14 +44,14 @@ describe Spree::Order do
 
     context "#checkout_steps" do
       context "when payment not required" do
-        before { order.stub payment_required?: false }
+        before { allow(order).to receive_messages payment_required?: false }
         specify do
           expect(order.checkout_steps).to eq %w(address delivery complete)
         end
       end
 
       context "when payment required" do
-        before { order.stub payment_required?: true }
+        before { allow(order).to receive_messages payment_required?: true }
         specify do
           expect(order.checkout_steps).to eq %w(address delivery payment complete)
         end
@@ -78,14 +78,14 @@ describe Spree::Order do
     context "from address" do
       before do
         order.state = 'address'
-        order.stub(:has_available_payment)
+        allow(order).to receive(:has_available_payment)
         order.shipments << create(:shipment)
         order.email = "user@example.com"
         order.save!
       end
 
       it "transitions to delivery" do
-        order.stub(ensure_available_shipping_rates: true)
+        allow(order).to receive_messages(ensure_available_shipping_rates: true)
         order.next!
         expect(order.state).to eq "delivery"
       end
@@ -108,7 +108,7 @@ describe Spree::Order do
 
       context "with payment required" do
         before do
-          order.stub payment_required?: true
+          allow(order).to receive_messages payment_required?: true
         end
 
         it "transitions to payment" do
@@ -119,7 +119,7 @@ describe Spree::Order do
 
       context "without payment required" do
         before do
-          order.stub payment_required?: false
+          allow(order).to receive_messages payment_required?: false
         end
 
         it "transitions to complete" do
@@ -136,8 +136,8 @@ describe Spree::Order do
 
      context "when payment is required" do
         before do
-          order.stub confirmation_required?: false
-          order.stub payment_required?: true
+          allow(order).to receive_messages confirmation_required?: false
+          allow(order).to receive_messages payment_required?: true
         end
 
         it "transitions to complete" do
@@ -150,7 +150,7 @@ describe Spree::Order do
       # Regression test for #2028
       context "when payment is not required" do
         before do
-          order.stub payment_required?: false
+          allow(order).to receive_messages payment_required?: false
         end
 
         it "does not call process payments" do
@@ -174,7 +174,7 @@ describe Spree::Order do
 
     it "should only call default transitions once when checkout_flow is redefined" do
       order = SubclassedOrder.new
-      order.stub payment_required?: true
+      allow(order).to receive_messages payment_required?: true
       expect(order).to receive(:process_payments!).once
       order.state = "payment"
       order.next!
@@ -227,7 +227,7 @@ describe Spree::Order do
     end
 
     it "does not attempt to process payments" do
-      order.stub_chain(:line_items, :present?).and_return(true)
+      allow(order).to receive_message_chain(:line_items, :present?).and_return(true)
       expect(order).to_not receive(:payment_required?)
       expect(order).to_not receive(:process_payments!)
       order.next!

From 734fce5ce7c6b4a0904006db45589cd9ba478c3a Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Sat, 25 Jul 2020 20:55:25 +0100
Subject: [PATCH 09/21] Add code to persist payments after failed payments. The
 state machine rollbacks the transactions, with this we keep record of what
 went wrong.

---
 app/controllers/checkout_controller.rb | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index dd1a67c702..b79df645c0 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -152,11 +152,23 @@ class CheckoutController < Spree::StoreController
       checkout_succeeded
       redirect_to(order_path(@order)) && return
     else
+      persist_all_payments if @order.state == "payment"
       flash[:error] = order_error
       checkout_failed
     end
   end
 
+  # When a payment fails, the order state machine rollbacks all transactions
+  #   Here we ensure we always persist all payments
+  def persist_all_payments
+    @order.payments.each do |payment|
+      original_payment_state = payment.state
+      if original_payment_state != payment.reload.state
+        payment.update(state: original_payment_state)
+      end
+    end
+  end
+
   def checkout_workflow(shipping_method_id)
     while @order.state != "complete"
       if @order.state == "payment"

From 26eee4631f22a9e5bda4968a65634ae5e17a5529 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 23:40:49 +0100
Subject: [PATCH 10/21] Rename AdvanceOrderService to OrderWorkflow

---
 .../spree/admin/orders/customer_details_controller.rb  |  2 +-
 app/controllers/spree/admin/orders_controller.rb       |  2 +-
 app/jobs/subscription_placement_job.rb                 |  2 +-
 .../{advance_order_service.rb => order_workflow.rb}    |  6 +++---
 ...ce_order_service_spec.rb => order_workflow_spec.rb} | 10 +++++-----
 5 files changed, 11 insertions(+), 11 deletions(-)
 rename app/services/{advance_order_service.rb => order_workflow.rb} (94%)
 rename spec/services/{advance_order_service_spec.rb => order_workflow_spec.rb} (89%)

diff --git a/app/controllers/spree/admin/orders/customer_details_controller.rb b/app/controllers/spree/admin/orders/customer_details_controller.rb
index 82c0c2347e..3239ffff01 100644
--- a/app/controllers/spree/admin/orders/customer_details_controller.rb
+++ b/app/controllers/spree/admin/orders/customer_details_controller.rb
@@ -23,7 +23,7 @@ module Spree
               @order.associate_user!(Spree.user_class.find_by(email: @order.email))
             end
 
-            AdvanceOrderService.new(@order).call
+            OrderWorkflow.new(@order).complete
 
             @order.shipments.map(&:refresh_rates)
             flash[:success] = Spree.t('customer_details_updated')
diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb
index 8cde99be81..e0b07fbbd9 100644
--- a/app/controllers/spree/admin/orders_controller.rb
+++ b/app/controllers/spree/admin/orders_controller.rb
@@ -35,7 +35,7 @@ module Spree
       def edit
         @order.shipments.map(&:refresh_rates)
 
-        AdvanceOrderService.new(@order).call
+        OrderWorkflow.new(@order).complete
 
         # The payment step shows an error of 'No pending payments'
         # Clearing the errors from the order object will stop this error
diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb
index 8b5df2b159..320c7b199a 100644
--- a/app/jobs/subscription_placement_job.rb
+++ b/app/jobs/subscription_placement_job.rb
@@ -66,7 +66,7 @@ class SubscriptionPlacementJob
   end
 
   def move_to_completion(order)
-    AdvanceOrderService.new(order).call!
+    OrderWorkflow.new(order).complete!
   end
 
   def unavailable_stock_lines_for(order)
diff --git a/app/services/advance_order_service.rb b/app/services/order_workflow.rb
similarity index 94%
rename from app/services/advance_order_service.rb
rename to app/services/order_workflow.rb
index 1377c36147..ab4673b970 100644
--- a/app/services/advance_order_service.rb
+++ b/app/services/order_workflow.rb
@@ -1,15 +1,15 @@
-class AdvanceOrderService
+class OrderWorkflow
   attr_reader :order
 
   def initialize(order)
     @order = order
   end
 
-  def call
+  def complete
     advance_order(advance_order_options)
   end
 
-  def call!
+  def complete!
     advance_order!(advance_order_options)
   end
 
diff --git a/spec/services/advance_order_service_spec.rb b/spec/services/order_workflow_spec.rb
similarity index 89%
rename from spec/services/advance_order_service_spec.rb
rename to spec/services/order_workflow_spec.rb
index 58a2fcb03a..5b2b6f0bf5 100644
--- a/spec/services/advance_order_service_spec.rb
+++ b/spec/services/order_workflow_spec.rb
@@ -1,6 +1,6 @@
 require "spec_helper"
 
-describe AdvanceOrderService do
+describe OrderWorkflow do
   let!(:distributor) { create(:distributor_enterprise) }
   let!(:order) do
     create(:order_with_totals_and_distribution, distributor: distributor,
@@ -13,7 +13,7 @@ describe AdvanceOrderService do
 
   it "transitions the order multiple steps" do
     expect(order.state).to eq("cart")
-    service.call
+    service.complete
     order.reload
     expect(order.state).to eq("complete")
   end
@@ -30,7 +30,7 @@ describe AdvanceOrderService do
 
     it "retains delivery method of the order" do
       order.select_shipping_method(shipping_method_b.id)
-      service.call
+      service.complete
       order.reload
       expect(order.shipping_method).to eq(shipping_method_b)
     end
@@ -38,7 +38,7 @@ describe AdvanceOrderService do
 
   context "when raising on error" do
     it "transitions the order multiple steps" do
-      service.call!
+      service.complete!
       order.reload
       expect(order.state).to eq("complete")
     end
@@ -49,7 +49,7 @@ describe AdvanceOrderService do
       end
 
       it "raises error" do
-        expect { service.call! }.to raise_error(StateMachine::InvalidTransition)
+        expect { service.complete! }.to raise_error(StateMachine::InvalidTransition)
       end
     end
   end

From c3f99050fdba69acad5b814de472a39b2ddb51b1 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 23:43:07 +0100
Subject: [PATCH 11/21] Move advance_order_state from checkout_controller to
 OrderWorkflow service

---
 app/controllers/checkout_controller.rb | 13 ++-----------
 app/services/order_workflow.rb         |  8 ++++++++
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index b79df645c0..2654e5c0ff 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -148,7 +148,7 @@ class CheckoutController < Spree::StoreController
   end
 
   def handle_redirect_from_stripe
-    if advance_order_state(@order) && order_complete?
+    if OrderWorkflow.new(@order).next && order_complete?
       checkout_succeeded
       redirect_to(order_path(@order)) && return
     else
@@ -177,7 +177,7 @@ class CheckoutController < Spree::StoreController
 
       @order.select_shipping_method(shipping_method_id) if @order.state == "delivery"
 
-      next if advance_order_state(@order)
+      next if OrderWorkflow.new(@order).next
 
       return update_failed
     end
@@ -194,15 +194,6 @@ class CheckoutController < Spree::StoreController
     true
   end
 
-  # Perform order.next, guarding against StaleObjectErrors
-  def advance_order_state(order)
-    tries ||= 3
-    order.next
-  rescue ActiveRecord::StaleObjectError
-    retry unless (tries -= 1).zero?
-    false
-  end
-
   def order_error
     if @order.errors.present?
       @order.errors.full_messages.to_sentence
diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index ab4673b970..1351bfeee6 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -13,6 +13,14 @@ class OrderWorkflow
     advance_order!(advance_order_options)
   end
 
+  def next
+    tries ||= 3
+    order.next
+  rescue ActiveRecord::StaleObjectError
+    retry unless (tries -= 1).zero?
+    false
+  end
+
   private
 
   def advance_order_options

From 9cbcf1448550c9dcbc103727b2d1ccf9b60fe65a Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 23:50:47 +0100
Subject: [PATCH 12/21] Move shipping method id setting code to OrderWorkflow
 service

---
 app/controllers/checkout_controller.rb | 4 +---
 app/services/order_workflow.rb         | 8 ++++++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 2654e5c0ff..66d5b445f7 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -175,9 +175,7 @@ class CheckoutController < Spree::StoreController
         return if redirect_to_payment_gateway
       end
 
-      @order.select_shipping_method(shipping_method_id) if @order.state == "delivery"
-
-      next if OrderWorkflow.new(@order).next
+      next if OrderWorkflow.new(@order).next({ shipping_method_id: shipping_method_id })
 
       return update_failed
     end
diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index 1351bfeee6..d333149283 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -13,9 +13,13 @@ class OrderWorkflow
     advance_order!(advance_order_options)
   end
 
-  def next
+  def next(options = {})
     tries ||= 3
-    order.next
+    result = order.next
+
+    after_transition_hook(options)
+
+    result
   rescue ActiveRecord::StaleObjectError
     retry unless (tries -= 1).zero?
     false

From ac5882e3e6f334abdb834539847b4e3cabfdd666 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 23:55:36 +0100
Subject: [PATCH 13/21] Refactor OrderWorkflow

---
 app/services/order_workflow.rb | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index d333149283..f3d8b7143a 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -14,15 +14,11 @@ class OrderWorkflow
   end
 
   def next(options = {})
-    tries ||= 3
-    result = order.next
+    result = advance_order_one_step
 
     after_transition_hook(options)
 
     result
-  rescue ActiveRecord::StaleObjectError
-    retry unless (tries -= 1).zero?
-    false
   end
 
   private
@@ -47,6 +43,14 @@ class OrderWorkflow
     end
   end
 
+  def advance_order_one_step
+    tries ||= 3
+    order.next
+  rescue ActiveRecord::StaleObjectError
+    retry unless (tries -= 1).zero?
+    false
+  end
+
   def after_transition_hook(options)
     if order.state == "delivery"
       order.select_shipping_method(options[:shipping_method_id]) if options[:shipping_method_id]

From 07005594ff09a688f8a1af0a5a6afe4c3d1325cc Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Tue, 28 Jul 2020 23:56:43 +0100
Subject: [PATCH 14/21] Move payments persistence code to order workflow
 service

---
 app/controllers/checkout_controller.rb | 12 ------------
 app/services/order_workflow.rb         | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 66d5b445f7..6280be51bb 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -152,23 +152,11 @@ class CheckoutController < Spree::StoreController
       checkout_succeeded
       redirect_to(order_path(@order)) && return
     else
-      persist_all_payments if @order.state == "payment"
       flash[:error] = order_error
       checkout_failed
     end
   end
 
-  # When a payment fails, the order state machine rollbacks all transactions
-  #   Here we ensure we always persist all payments
-  def persist_all_payments
-    @order.payments.each do |payment|
-      original_payment_state = payment.state
-      if original_payment_state != payment.reload.state
-        payment.update(state: original_payment_state)
-      end
-    end
-  end
-
   def checkout_workflow(shipping_method_id)
     while @order.state != "complete"
       if @order.state == "payment"
diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index f3d8b7143a..b7db840ef8 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -55,5 +55,19 @@ class OrderWorkflow
     if order.state == "delivery"
       order.select_shipping_method(options[:shipping_method_id]) if options[:shipping_method_id]
     end
+
+    persist_all_payments if order.state == "payment"
   end
+
+  # When a payment fails, the order state machine rollbacks all transactions
+  #   Here we ensure we always persist all payments
+  def persist_all_payments
+    order.payments.each do |payment|
+      original_payment_state = payment.state
+      if original_payment_state != payment.reload.state
+        payment.update(state: original_payment_state)
+      end
+    end
+  end
+
 end

From fe0c04b65043d83736e120cf1a33f47afc4a4044 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Wed, 29 Jul 2020 12:03:09 +1000
Subject: [PATCH 15/21] Complete renaming of AdvanceOrderService to
 OrderWorkflow

---
 app/controllers/spree/admin/payments_controller.rb     | 2 +-
 lib/tasks/sample_data/order_factory.rb                 | 2 +-
 spec/controllers/admin/proxy_orders_controller_spec.rb | 2 +-
 spec/jobs/subscription_confirm_job_spec.rb             | 4 ++--
 spec/services/order_factory_spec.rb                    | 2 +-
 spec/services/order_syncer_spec.rb                     | 6 +++---
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb
index 408e6e5c81..bfdb4fd2ff 100644
--- a/app/controllers/spree/admin/payments_controller.rb
+++ b/app/controllers/spree/admin/payments_controller.rb
@@ -37,7 +37,7 @@ module Spree
 
             redirect_to admin_order_payments_path(@order)
           else
-            AdvanceOrderService.new(@order).call!
+            OrderWorkflow.new(@order).complete!
 
             flash[:success] = Spree.t(:new_order_completed)
             redirect_to edit_admin_order_url(@order)
diff --git a/lib/tasks/sample_data/order_factory.rb b/lib/tasks/sample_data/order_factory.rb
index 29d4cf03c6..9614f685a0 100644
--- a/lib/tasks/sample_data/order_factory.rb
+++ b/lib/tasks/sample_data/order_factory.rb
@@ -45,7 +45,7 @@ class OrderFactory
 
   def create_complete_order
     order = create_cart_order
-    AdvanceOrderService.new(order).call
+    OrderWorkflow.new(order).complete
     order
   end
 
diff --git a/spec/controllers/admin/proxy_orders_controller_spec.rb b/spec/controllers/admin/proxy_orders_controller_spec.rb
index 0bc63e3cb8..8d74a8d901 100644
--- a/spec/controllers/admin/proxy_orders_controller_spec.rb
+++ b/spec/controllers/admin/proxy_orders_controller_spec.rb
@@ -77,7 +77,7 @@ describe Admin::ProxyOrdersController, type: :controller do
     before do
       # Processing order to completion
       allow(Spree::OrderMailer).to receive(:cancel_email) { double(:email, deliver: true) }
-      AdvanceOrderService.new(order).call!
+      OrderWorkflow.new(order).complete!
       proxy_order.reload
       proxy_order.cancel
       allow(controller).to receive(:spree_current_user) { user }
diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb
index 603da10a10..96f718efd1 100644
--- a/spec/jobs/subscription_confirm_job_spec.rb
+++ b/spec/jobs/subscription_confirm_job_spec.rb
@@ -19,7 +19,7 @@ describe SubscriptionConfirmJob do
     let(:proxy_orders) { job.send(:unconfirmed_proxy_orders) }
 
     before do
-      AdvanceOrderService.new(order).call!
+      OrderWorkflow.new(order).complete!
     end
 
     it "returns proxy orders that meet all of the criteria" do
@@ -126,7 +126,7 @@ describe SubscriptionConfirmJob do
     let(:order) { proxy_order.initialise_order! }
 
     before do
-      AdvanceOrderService.new(order).call!
+      OrderWorkflow.new(order).complete!
       allow(job).to receive(:send_confirmation_email).and_call_original
       setup_email
       expect(job).to receive(:record_order)
diff --git a/spec/services/order_factory_spec.rb b/spec/services/order_factory_spec.rb
index c67cbaaf0e..394de7eaeb 100644
--- a/spec/services/order_factory_spec.rb
+++ b/spec/services/order_factory_spec.rb
@@ -47,7 +47,7 @@ describe OrderFactory do
     end
 
     it "retains address, delivery, and payment attributes until completion of the order" do
-      AdvanceOrderService.new(order).call
+      OrderWorkflow.new(order).complete
 
       order.reload
 
diff --git a/spec/services/order_syncer_spec.rb b/spec/services/order_syncer_spec.rb
index 9990425f6a..2cd6010736 100644
--- a/spec/services/order_syncer_spec.rb
+++ b/spec/services/order_syncer_spec.rb
@@ -409,7 +409,7 @@ describe OrderSyncer do
 
       context "when order is complete" do
         it "does not update the line_item quantities and adds the order to order_update_issues with insufficient stock" do
-          AdvanceOrderService.new(order).call
+          OrderWorkflow.new(order).complete
 
           expect(syncer.sync!).to be true
 
@@ -423,7 +423,7 @@ describe OrderSyncer do
         it "does not update the line_item quantities and adds the order to order_update_issues with out of stock" do
           # this single item available is used when the order is completed below, making the item out of stock
           variant.update_attribute(:on_hand, 1)
-          AdvanceOrderService.new(order).call
+          OrderWorkflow.new(order).complete
 
           expect(syncer.sync!).to be true
 
@@ -507,7 +507,7 @@ describe OrderSyncer do
       end
 
       context "when order is complete" do
-        before { AdvanceOrderService.new(order).call }
+        before { OrderWorkflow.new(order).complete }
 
         it "does not add line_item and adds the order to order_update_issues" do
           expect(syncer.sync!).to be true

From ad00971ca86fedb5ddd1a9b15d5e9e7148a3d343 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Wed, 29 Jul 2020 14:15:17 +0100
Subject: [PATCH 16/21] Improve readability and add bugsnag error (now in the
 checkout_failed method) when checkout_fails while handling stripe redirect

---
 app/controllers/checkout_controller.rb       | 30 +++++++++-----------
 spec/controllers/checkout_controller_spec.rb |  4 +--
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 6280be51bb..b801213e09 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -39,13 +39,13 @@ class CheckoutController < Spree::StoreController
 
     # This is only required because of spree_paypal_express. If we implement
     # a version of paypal that uses this controller, and more specifically
-    # the #update_failed method, then we can remove this call
+    # the #action_failed method, then we can remove this call
     OrderCheckoutRestart.new(@order).call
   end
 
   def update
     params_adapter = Checkout::FormDataAdapter.new(permitted_params, @order, spree_current_user)
-    return update_failed unless @order.update(params_adapter.params[:order])
+    return action_failed unless @order.update(params_adapter.params[:order])
 
     fire_event('spree.checkout.update')
 
@@ -54,7 +54,7 @@ class CheckoutController < Spree::StoreController
     rescue_from_spree_gateway_error(e)
   rescue StandardError => e
     flash[:error] = I18n.t("checkout.failed")
-    update_failed(e)
+    action_failed(e)
   end
 
   # Clears the cached order. Required for #current_order to return a new order
@@ -152,7 +152,6 @@ class CheckoutController < Spree::StoreController
       checkout_succeeded
       redirect_to(order_path(@order)) && return
     else
-      flash[:error] = order_error
       checkout_failed
     end
   end
@@ -165,7 +164,7 @@ class CheckoutController < Spree::StoreController
 
       next if OrderWorkflow.new(@order).next({ shipping_method_id: shipping_method_id })
 
-      return update_failed
+      return action_failed
     end
 
     update_response
@@ -193,7 +192,7 @@ class CheckoutController < Spree::StoreController
       checkout_succeeded
       update_succeeded_response
     else
-      update_failed(RuntimeError.new("Order not complete after the checkout workflow"))
+      action_failed(RuntimeError.new("Order not complete after the checkout workflow"))
     end
   end
 
@@ -219,19 +218,18 @@ class CheckoutController < Spree::StoreController
     end
   end
 
-  def update_failed(error = RuntimeError.new(order_error))
-    Bugsnag.notify(error)
-
-    flash[:error] = order_error if flash.blank?
-    checkout_failed
-    update_failed_response
+  def action_failed(error = RuntimeError.new(order_error))
+    checkout_failed(error)
+    action_failed_response
   end
 
-  def checkout_failed
+  def checkout_failed(error = RuntimeError.new(order_error))
+    Bugsnag.notify(error)
+    flash[:error] = order_error if flash.blank?
     Checkout::PostCheckoutActions.new(@order).failure
   end
 
-  def update_failed_response
+  def action_failed_response
     respond_to do |format|
       format.html do
         render :edit
@@ -244,9 +242,7 @@ class CheckoutController < Spree::StoreController
 
   def rescue_from_spree_gateway_error(error)
     flash[:error] = t(:spree_gateway_error_flash_for_checkout, error: error.message)
-    # This can also happen during the edit action
-    #   but the actions and response needed are exactly the same as when the update action fails
-    update_failed(error)
+    action_failed(error)
   end
 
   def permitted_params
diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb
index ecb18971db..e6412bebe3 100644
--- a/spec/controllers/checkout_controller_spec.rb
+++ b/spec/controllers/checkout_controller_spec.rb
@@ -298,7 +298,7 @@ describe CheckoutController, type: :controller do
     end
   end
 
-  describe "#update_failed" do
+  describe "#action_failed" do
     let(:restart_checkout) { instance_double(OrderCheckoutRestart, call: true) }
 
     before do
@@ -312,7 +312,7 @@ describe CheckoutController, type: :controller do
       expect(restart_checkout).to receive(:call)
       expect(controller).to receive(:respond_to)
 
-      controller.send(:update_failed)
+      controller.send(:action_failed)
     end
   end
 end

From da4abf66179618ced45abbe4c8f3724d5d5bc23e Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Wed, 29 Jul 2020 21:57:55 +0100
Subject: [PATCH 17/21] Add a comment to explain the necessity of the first
 rescue in the update action

---
 app/controllers/checkout_controller.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index b801213e09..479d740581 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -51,6 +51,8 @@ class CheckoutController < Spree::StoreController
 
     checkout_workflow(params_adapter.shipping_method_id)
   rescue Spree::Core::GatewayError => e
+    # This rescue is not replaceable by the generic rescue_from above because otherwise the rescue
+    #   below (StandardError) would catch the GatewayError and report it as a generic error
     rescue_from_spree_gateway_error(e)
   rescue StandardError => e
     flash[:error] = I18n.t("checkout.failed")

From 9e9e0d0bd8600def5eb469bc793d626bca365c67 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Wed, 29 Jul 2020 22:01:15 +0100
Subject: [PATCH 18/21] Remove rescue_from and just add the rescue to the edit
 action, the update action has a different logic where there is a generic
 rescue StandardError after the GatewayError rescue

---
 app/controllers/checkout_controller.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb
index 479d740581..1dbffc71e1 100644
--- a/app/controllers/checkout_controller.rb
+++ b/app/controllers/checkout_controller.rb
@@ -32,8 +32,6 @@ class CheckoutController < Spree::StoreController
 
   helper 'spree/orders'
 
-  rescue_from Spree::Core::GatewayError, with: :rescue_from_spree_gateway_error
-
   def edit
     return handle_redirect_from_stripe if valid_payment_intent_provided?
 
@@ -41,6 +39,8 @@ class CheckoutController < Spree::StoreController
     # 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
@@ -51,8 +51,6 @@ class CheckoutController < Spree::StoreController
 
     checkout_workflow(params_adapter.shipping_method_id)
   rescue Spree::Core::GatewayError => e
-    # This rescue is not replaceable by the generic rescue_from above because otherwise the rescue
-    #   below (StandardError) would catch the GatewayError and report it as a generic error
     rescue_from_spree_gateway_error(e)
   rescue StandardError => e
     flash[:error] = I18n.t("checkout.failed")

From 2136eecd0968476878f1ab797beb809a14b9cafe Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Wed, 29 Jul 2020 22:34:45 +0100
Subject: [PATCH 19/21] Avoid reloading the payment every time, so that
 in-memory data is not wiped out

When checkout fails and the payment states dont match (inside the if), in-memory data of the failed payment can be lost but updating the payment state is the fundamental part here so that further checkout attempts work. We may improve this update statement so that all the data of the failed payment is persisted
---
 app/services/order_workflow.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index b7db840ef8..c2a7e9dc95 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -64,8 +64,8 @@ class OrderWorkflow
   def persist_all_payments
     order.payments.each do |payment|
       original_payment_state = payment.state
-      if original_payment_state != payment.reload.state
-        payment.update(state: original_payment_state)
+      if original_payment_state != Spree::Payment.find(payment.id).state
+        payment.reload.update(state: original_payment_state)
       end
     end
   end

From e739c5185ef9a7b79807ac0eb53649a2720d2621 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Wed, 29 Jul 2020 23:30:34 +0100
Subject: [PATCH 20/21] Add specs to verify that Spree::Core::Gateway
 exceptions are handled correctly

---
 spec/controllers/checkout_controller_spec.rb | 38 +++++++++++++++++---
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb
index e6412bebe3..0d285dab08 100644
--- a/spec/controllers/checkout_controller_spec.rb
+++ b/spec/controllers/checkout_controller_spec.rb
@@ -67,10 +67,31 @@ describe CheckoutController, type: :controller do
         allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true
       end
 
-      it "does not redirect" do
-        expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(true)
-        get :edit
-        expect(response).to be_success
+      describe "order variants are distributed in the OC" do
+        before do
+          expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(true)
+        end
+
+        it "does not redirect" do
+          get :edit
+          expect(response).to be_success
+        end
+
+        it "returns a specific flash message when Spree::Core::GatewayError occurs" do
+          order_checkout_restart = double(:order_checkout_restart)
+          allow(OrderCheckoutRestart).to receive(:new) { order_checkout_restart }
+          call_count = 0
+          allow(order_checkout_restart).to receive(:call) do
+            call_count += 1
+            raise Spree::Core::GatewayError.new("Gateway blow up") if call_count == 1
+          end
+
+          spree_post :edit
+
+          expect(response.status).to eq(200)
+          flash_message = I18n.t(:spree_gateway_error_flash_for_checkout, error: "Gateway blow up")
+          expect(flash[:error]).to eq flash_message
+        end
       end
 
       describe "when the order is in payment state and a stripe payment intent is provided" do
@@ -230,6 +251,15 @@ describe CheckoutController, type: :controller do
       expect(response.body).to eq({ errors: {}, flash: { error: I18n.t("checkout.failed") } }.to_json)
     end
 
+    it "returns a specific error on Spree::Core::GatewayError" do
+      allow(order).to receive(:update).and_raise(Spree::Core::GatewayError.new("Gateway blow up"))
+      spree_post :update, format: :json, order: {}
+
+      expect(response.status).to eq(400)
+      flash_message = I18n.t(:spree_gateway_error_flash_for_checkout, error: "Gateway blow up")
+      expect(json_response["flash"]["error"]).to eq flash_message
+    end
+
     describe "stale object handling" do
       it "retries when a stale object error is encountered" do
         allow(OrderCompletionReset).to receive(:new).with(controller, order) { reset_order_service }

From 0359d103b2e151d48b2b0340957a7ecf6277fbd9 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Thu, 30 Jul 2020 17:12:41 +0100
Subject: [PATCH 21/21] Improve code comments on dodgy and/but critical
 checkout process method

---
 app/services/order_workflow.rb | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index c2a7e9dc95..b55ed43ce1 100644
--- a/app/services/order_workflow.rb
+++ b/app/services/order_workflow.rb
@@ -59,15 +59,21 @@ class OrderWorkflow
     persist_all_payments if order.state == "payment"
   end
 
-  # When a payment fails, the order state machine rollbacks all transactions
-  #   Here we ensure we always persist all payments
+  # When a payment fails, the order state machine stays in 'payment' and rollbacks all transactions
+  #   This rollback also reverts the payment state from 'failed', 'void' or 'invalid' to 'pending'
+  #   Despite the rollback, the in-memory payment still has the correct state, so we persist it
   def persist_all_payments
     order.payments.each do |payment|
-      original_payment_state = payment.state
-      if original_payment_state != Spree::Payment.find(payment.id).state
-        payment.reload.update(state: original_payment_state)
+      in_memory_payment_state = payment.state
+      if different_from_db_payment_state?(in_memory_payment_state, payment.id)
+        payment.reload.update(state: in_memory_payment_state)
       end
     end
   end
 
+  # Verifies if the in-memory payment state is different from the one stored in the database
+  #   This is be done without reloading the payment so that in-memory data is not changed
+  def different_from_db_payment_state?(in_memory_payment_state, payment_id)
+    in_memory_payment_state != Spree::Payment.find(payment_id).state
+  end
 end