Openfoodnetwork: StateMachine::InvalidTransition in checkout#update

[mkllnk]

Error in OpenFoodNetwork

StateMachine::InvalidTransition in checkout#update
Cannot transition state via :restart_checkout from :payment (Reason(s): State cannot transition via "restart checkout")

View on Bugsnag

Stacktrace

app/services/restart_checkout.rb:22 - reset_state_to_cart
app/services/restart_checkout.rb:10 - call
app/controllers/checkout_controller.rb:142 - update_failed
app/controllers/checkout_controller.rb:40 - update

View full stacktrace

Created by Maikel via Bugsnag

[luisramos0]

documenting what I am thinking about this so others can comment while I am at it:

here we get most of these from failed payments but we also have some validation errors when the order is already in address state.
I think forcing a move from address state to cart state is obvious and not dangerous.
but I wonder in case of failed payment if we should move the order back to cart or leave it in payment state.

option 1

moving back to cart after payment state - maybe the existing payment (in pending/failed state) will cause trouble in the following checkout attempts? so we need to move state to cart and clear payments which will leave no trace to the manager neither to the dev debuggers...

option 2

maybe keeping the order in payment state will cause trouble in the cart, or is a new cart generated immediately after if user goes to checkout page?

diff --git a/app/services/restart_checkout.rb b/app/services/restart_checkout.rb
index 4792d070e..a6127cf65 100644
--- a/app/services/restart_checkout.rb
+++ b/app/services/restart_checkout.rb
@@ -5,7 +5,7 @@ class RestartCheckout
   def call
-    return if order.cart?
+    return if order.cart? && order.address? && order.payment?

     reset_state_to_cart
     clear_shipments

[luisramos0]

I think option 2 could be viable, we keep the order in address or payment state after an error, and the cart in the frontoffice keeps working for the user and in the backoffice we can see there was a problem for the user either in address or payment (if user has not managed to fix it in another checkout attempt) because the order state will be address or payment. This is better than option 1 where we would not be able to know that the user had a problem in that order because the order state would be the standard "cart".

So I tried option 2 and the frontoffice seems to work fine. But...

1. I dont understand why is the transition from payment to cart working in the automated test but not for these errors we are trying to address???
2. I wonder: am I suggesting just deleting the restart checkout and keep the workflow where it is when there is an error? Because in option 2 I am basically suggesting making the RestartCheckout service do nothing if state is address or payment.

I need feedback from other devs.

[mkllnk]

Just a thought: Did you check that order.update_attribute(:state, "payment") is successful in the spec? Or does it silently fail?

I would also go back in the Git history to find out why we introduced the restart checkout logic. That might give a hint which scenario we need to cover and which ones we may not need to cover.

[luisramos0]

Thanks for your feedback Maikel.
Checked now, good idea but the state goes to payment and back to cart: the spec is correct.

The source is stripe integration and it's really cleanup_order not restart_checkout:
https://github.com/openfoodfoundation/openfoodnetwork/pull/1892

After this investigation I think we cannot go for option 2, because having the order state in payment without a payment is not a good state.

So, we need to try option 1 which is to force the workflow to cart in some way, even if it means deleting/recreating/cloning the order and keeping at least the line items and ideally the address data if present.

[luisramos0]

ok, one important finding, I can replicate this error:
"Cannot transition state via :restart_checkout from :payment (Reason(s): Shipping address first name can't be blank, Shipping address last name can't be blank,"
by simply checking out with a random bogus credit card and no shipping address filled in, when the payment fails, the restart checkout fails because shipping address is empty. Shipping address is empty because I am using a pickup ship method.

If I dont fill in shipping address, the checkout fails and user doesnt see an error, if I fill in shipping address user sees flash with error message related to payment.

if the ship method is pick up, the ship_address is explicitly cleared with clear_ship_address. And then the restart checkout workflow fails with missing ship address.
I still have to find out where the shipping address validation is happening in the workflow...
OR, I can just delete the clear_ship_address logic in the checkout_controller: #4252

[Matt-Yorkley]

Nice investigation! That's something I've noticed recently when looking at bugs: we have order/checkout logic in some controllers that isn't in the models, which can make it hard to write tests that accurately reflect what's happening with orders at different steps...