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

[luisramos0]

What? Why?

Closes #5816 and #5818
I am putting these two fixes together in one PR because they are not too big but mostly so we can test these things together, they are different but closely related.

Regarding #5816
The handler of any gateway exceptions is used for the StripeSCA authorization step. Whenever there's an error on checkout we need to reset the order to cart state.
We were not doing that before and that was causing the order to stay in the payment state after an authorization error. Because the order was in the payment state, the callback to update order totals was not executed on the second attempt and that caused not all fees to be collected #5816

This is a generic improvement so it will make the checkout code more resilient for some other cases as well.

Regarding #5818
Here we are adding an alternative to the infamous after_rollback callback called persist_invalid for the payment model.
On checkout, in the situation where we are handling a redirect from stripe and capturing the payment, the payment is correctly set to failed state but the order workflow state machine rolls it back to pending state causing #5818. Here we are adding some logic to the OrderWorkflow service to persist the payments after a failure so we get the data for debugging.
Related to this there's a good refactoring with code moved from checkout controller to the OrderWorkflow service.

Additionally, while investigating I thought I'd have to patch the checkout workflow code, in the end I didnt but I ended up bringing the checkout.rb file from spree_core to OFN. No changes to it, just rubocop and transpec.

What should we test?

We need to verify the issue is solved.
The code changed is related to any errors that happen on the stripe side so we can test any other error scenario on stripe and verify that fixing it will process the payment in stripe with the correct amount. For example, we can check if this will also work if we use a card that needs external authorization and we fail the validation.

I just verified that #5816 is also present when we use a card like 4000 0027 6000 3184 and press "fail authentication". So:

• on first attempt use 4000 0027 6000 3184 and press "FAIL authentication"
• on second attempt use 4000 0027 6000 3184 and press "COMPLETE authentication"
  This test case should fail before this PR and pass after this PR 👍

Release notes

Changelog Category: Fixed
Fixes StripeSCA problem where the second attempt to make a payment would fail even the card and details were valid.

[luisramos0]

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

[sauloperez]

Good one 👏 ! I think it'd be a good idea to create a tech debt issue to find out about

the payment is correctly set to failed state but the order workflow state machine rolls it back to pending state causing #5818

and fix it properly. IMO this part of the codebase added callbacks on top of things there weren't properly working and this led to a lot of complexity that is making it hard to change.

[sauloperez]

looks like we had an infinite loop in the build.

[luisramos0]

ah, yeah, I missed that. I'll have a look now,. Thanks!

[luisramos0]

I tried to fix the build but this after_rollback callback breaks the specs because the specs always rollback the transactions...

I saw that Spree also removes this callback in v2.3.

So I tried to create a better solution. It was hard but I think I do have a slightly better workaround for this.

[mkllnk]

Some nice refactoring in here. You will be so happy when we finally rewrite that state machine. 😄

[mkllnk]

The specs need more work, moving back to in-dev.

[mkllnk]

I fixed all the obvious specs but there are still some failing:

rspec ./spec/features/consumer/shopping/checkout_spec.rb[1:1:4:2:2:5:1:1]
rspec ./spec/features/consumer/shopping/checkout_spec.rb[1:1:4:2:2:5:2:1]
rspec ./spec/features/consumer/shopping/shopping_spec.rb:414

The last one passed locally and may be flaky but the first two are definitely broken by this pull request. The credit card secrets are not passed onto the payment gateway and without valid credit card number it fails.

git bisect tells me that 0700559 broke the spec.

[mkllnk]

Reloading the payment is a problem:

openfoodnetwork/app/services/order_workflow.rb

Line 67 in 0700559

if original_payment_state != payment.reload.state

While the records look exactly the same, reloading gets rid of in-memory data like the credit card secrets.

payment.source.number
# "4111111111111111"

payment.reload.source.number
# nil

The easy fix is:

diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb
index b7db840ef..c2a7e9dc9 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

But I'm not sure about the implications. Having rollback code and then working against it sounds broken. Maybe we need to question the rollback code?

[mkllnk]

@luisramos0 I'm handing the keyboard back to you. Let me know what you think. I guess we can go ahead with this fix and create a tech debt issue. Or we investigate fixing the callback. You may have a better overview of that and it may make more sense to tackle that in bye-bye-spree.

[luisramos0]

hey @mkllnk nice async pair programming! Thank you!

I think your fix works! I am not sure the persisted failed payments will end up with all the info but it doesnt matter for now, first checkout will now work and second, we will have spree_log_entries which is the most important.

I added a few more simple commits.
I think this is ready to move forward.

[mkllnk]

I think that this is good to go. Can't wait to clean this up one day.

We have a similar problem in Ceres Fair Food. In some very rare circumstances, a rollback happens. But it actually rolls back the creation of the payment. The result is a gap in the sequence of payment ids and in some cases the payment has gone through. Rollbacks are dangerous.

[mkllnk]

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.

[luisramos0]

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!

[mkllnk]

Ace! That is so much clearer. 🤓

[filipefurtad0]

Hey @luisramos0 and reviewers,

• About Stripe-SCA payment do not include ship/transaction fees (on a second payment attempt) #5816

Reproduced issue and re-ran the procedure, after staging - for both Stripe-SCA and Connect (which was already working before). This looks good, transaction fee is now correctly charged, after failing the payment once, with card ...0069. Amounts match in stripe-dashboard and on Payments (while editing the order):

Also verified we now get a warning on Bugsnag/Slack on this type of failed payments - which is really awesome, surely helpful as well for issues such as #5785 ! This works for both stripe methods.

• About Transaction fees charged several times, after using an invalid 3D-card and retrying with valid cards #5818

Reproduced the issue before staging. True, it seems to break as well by manually failing the 3D authentication process. So, after this PR the process goes as expected, after the first failed attempt:

No additional transaction fees are added, for each failed attempt - I tried several other invalid cards, such as 4000008260003178 (insufficient funds) or 4000008400001629 (card_declined), verifying that these errors appear in Bugsnag and in stripe's Dashboard, correctly:

Both issues are solved - and we've got data on errors now. Awesome work 💪
Good to go.

[luisramos0]

This is great news. I will include this in this release 3.2.1 making it fully ready for the stripeSCA rollout 🎉