Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset order state to cart in case the stripe SCA authorization step fails #5824

Merged
merged 21 commits into from
Aug 1, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5266d95
Move method closer to related/similar methods
luisramos0 Jul 24, 2020
1bf946d
Reused code in checkout controller, the reponse for the case when the…
luisramos0 Jul 24, 2020
b23b707
Notify bugsnag and execute post checkout actions (reset to cart state…
luisramos0 Jul 24, 2020
ec0d06a
Reuse update_failed method as the code needed is exactly the same
luisramos0 Jul 24, 2020
d8a96c9
Bring order checkout workflow and some of its specs from spree_core
luisramos0 Jul 28, 2020
e99f0dc
Rubocop autocorrect and easy rubocop issues
luisramos0 Jul 28, 2020
51de526
Fix specs in checkout_spec
luisramos0 Jul 28, 2020
e80337a
Transpec checkout_spec
luisramos0 Jul 28, 2020
734fce5
Add code to persist payments after failed payments. The state machine
luisramos0 Jul 25, 2020
26eee46
Rename AdvanceOrderService to OrderWorkflow
luisramos0 Jul 28, 2020
c3f9905
Move advance_order_state from checkout_controller to OrderWorkflow se…
luisramos0 Jul 28, 2020
9cbcf14
Move shipping method id setting code to OrderWorkflow service
luisramos0 Jul 28, 2020
ac5882e
Refactor OrderWorkflow
luisramos0 Jul 28, 2020
0700559
Move payments persistence code to order workflow service
luisramos0 Jul 28, 2020
fe0c04b
Complete renaming of AdvanceOrderService to OrderWorkflow
mkllnk Jul 29, 2020
ad00971
Improve readability and add bugsnag error (now in the checkout_failed…
luisramos0 Jul 29, 2020
da4abf6
Add a comment to explain the necessity of the first rescue in the upd…
luisramos0 Jul 29, 2020
9e9e0d0
Remove rescue_from and just add the rescue to the edit action, the up…
luisramos0 Jul 29, 2020
2136eec
Avoid reloading the payment every time, so that in-memory data is not…
luisramos0 Jul 29, 2020
e739c51
Add specs to verify that Spree::Core::Gateway exceptions are handled …
luisramos0 Jul 29, 2020
0359d10
Improve code comments on dodgy and/but critical checkout process method
luisramos0 Jul 30, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 9 additions & 21 deletions app/controllers/checkout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,6 @@ def before_payment
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_")

Expand All @@ -156,7 +148,7 @@ def valid_payment_intent_provided?
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
Expand All @@ -171,9 +163,7 @@ def checkout_workflow(shipping_method_id)
return if redirect_to_payment_gateway
end

@order.select_shipping_method(shipping_method_id) if @order.state == "delivery"

next if advance_order_state(@order)
next if OrderWorkflow.new(@order).next({ shipping_method_id: shipping_method_id })

return update_failed
end
Expand All @@ -190,15 +180,6 @@ def redirect_to_payment_gateway
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
Expand Down Expand Up @@ -261,6 +242,13 @@ def update_failed_response
end
end

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
luisramos0 marked this conversation as resolved.
Show resolved Hide resolved
# but the actions and response needed are exactly the same as when the update action fails
update_failed(error)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the key difference here is the call to checkout_failed in update_failed that executes PostCheckoutActions that will reset the order state to cart 👍


def permitted_params
PermittedAttributes::Checkout.new(params).call
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def update
@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')
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/spree/admin/orders_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def new
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
Expand Down
2 changes: 1 addition & 1 deletion app/jobs/subscription_placement_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def handle_empty_order(order, changes)
end

def move_to_completion(order)
AdvanceOrderService.new(order).call!
OrderWorkflow.new(order).complete!
end

def unavailable_stock_lines_for(order)
Expand Down
191 changes: 191 additions & 0 deletions app/models/spree/order/checkout.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
# frozen_string_literal: true

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 = {})
checkout_steps[name] = options
previous_states.each do |state|
add_transition({ from: state, to: name }.merge(options))
end
if options[:if]
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 = checkout_steps.keys.last unless before || after

cloned_steps = checkout_steps.clone
cloned_removed_transitions = removed_transitions.clone
checkout_flow do
cloned_steps.each_pair do |key, value|
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|
remove_transition(transition)
end
end
end

def self.remove_checkout_step(name)
cloned_steps = checkout_steps.clone
cloned_removed_transitions = removed_transitions.clone
checkout_flow do
cloned_steps.each_pair do |key, value|
go_to_state(key, value) unless key == name
end
cloned_removed_transitions.each do |transition|
remove_transition(transition)
end
end
end

def self.remove_transition(options = {})
removed_transitions << options
next_event_transitions.delete(find_transition(options))
end

def self.find_transition(options = {})
return nil if options.nil? || !options.include?(:from) || !options.include?(:to)

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)
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 checkout_step?(step)
step.present? ? checkout_steps.include?(step) : false
end

def checkout_step_index(step)
checkout_steps.index(step)
end

def self.removed_transitions
@removed_transitions ||= []
end

def can_go_to_state?(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
end
end
end
end
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
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

def next(options = {})
result = advance_order_one_step

after_transition_hook(options)

result
end

private

def advance_order_options
Expand All @@ -35,9 +43,31 @@ def advance_order!(options)
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]
end

persist_all_payments if order.state == "payment"
end

# When a payment fails, the order state machine rollbacks all transactions
luisramos0 marked this conversation as resolved.
Show resolved Hide resolved
# 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