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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 21 commits into from
Aug 1, 2020
Merged

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

Changes from 1 commit
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
4 changes: 2 additions & 2 deletions app/services/order_workflow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ def after_transition_hook(options)
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)
Copy link
Member

Choose a reason for hiding this comment

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

I'm still a bit confused what's happening here. Do we need a comment explaining that? My theory is:

  • During checkout the order is going to state payment and a payment object is created.
  • The order should go from state payment to complete but on error the state machine stays in payment.
  • The payment object's normal states are: checkout, pending, processing, completed
  • Error states are: void and invalid.
  • A failed checkout causes a database rollback which reverts the payment state from void or invalid to checkout or pending (I don't know which).
  • Despite the rollback, our in-memory payment object still has the failed state.
  • Checking for a state change in the database is a sign of a rollback and we override the database with our payment state.
  • In theory, if something else updated the payment record at the same time, like a webhook, we could accidentally override that. There is a potential race condition here but we are most likely fine because of the checkout sequence and synchronisation.

Do you agree with this?

What else could happen? I guess a payment could complete but something else fails and we override the payment state to mark it as completed. That's a good thing.

I reckon that the payment should happen outside of the state machine transition to not be affected by the rollback. The state machine transition just verifies that there is enough money for the order. Out of scope here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont agree about you being confused :-)
I think what's happening is exactly what you have described 👌 and that was what I thought was happening 👍
I used some of your description to add code comments. I think it's now better explained, thanks!

end
end
end
Expand Down