[Woo POS] Consider updating CollectOrderPaymentUseCase for the existing app behavior that transforms a retryable payment error to non-retryable after the first retry fails

[malinajirka]

Ref: p1718735478831209/1718734994.548519-slack-C070SJRA8DP

I'm not sure that the existing behaviour of transforming a retryable error to a non-retryable error, the after the first retry fails, is the right approach. If the error comes up and says "try another card", but they try the same card, you do actually want them to be able to try again, not have to start over. Changing this means changing the existing code, but it's probably something we should consider a bug in the existing code.

[dangermattic]

🚫 Please add a type label (e.g. type: enhancement) and a feature label (e.g. feature: stats) to this issue.

[malinajirka]

I agree with Jaclyn that this is a bug in the existing code => the Android app doesn't behave this way, it fully relies on the Stripe SDK to tell us whether an error is retryable or not and I believe the iOS app should behave the same.

[jaclync]

it fully relies on the Stripe SDK to tell us whether an error is retryable or not and I believe the iOS app should behave the same.

@malinajirka (Not urgent) Could you point me to where Stripe indicates whether an error is retryable? In iOS I'm seeing a field "com.stripe-terminal:StripeShouldRetry" in the error user info, but I can't find it in the official documentation.

[malinajirka]

@jaclync This post summarizes the overall retry handling - p91TBi-520-p2. This is a link to Stripe's docs.

What I wrote in the above comment is misleading, sorry about that. It's not about which error is retryable and which isn't (all payments can be technically retried) but more about which step should be retried - collect payment/process payment/... .

[jaclync]

Thanks for the links Jirka! It looks like iOS does handle the retry action based on the Stripe doc using the payment intent's status.

I tried finding more context and saw that this PR #10673 introduced the alreadyRetried error case that's used for converting a retryable error to non-retryable after the first retry fails. I don't see specific info about this behavior though, @joshheald might you have some context as the PR author? Would removing this alreadyRetried error case and related code make sense? Apology if I'm missing anything obvious, a lot of this is still quite new to me 😅

---

A note from testing one scenario to tap an expired live card: the specific error reason from confirmPaymentIntent isn't shown as the payment intent is null from the error SCPConfirmPaymentIntentError that we don't handle. The retry just retries the original card remotely, which would fail because this error (expired card) isn't actually retryable. There's a field com.stripe-terminal:StripeShouldRetry in the error user info dictionary, but it's not documented officially and probably not worth using it. We could probably improve this flow though.

[joshheald]

I tried finding more context and saw that this PR #10673 introduced the alreadyRetried error case that's used for converting a retryable error to non-retryable after the first retry fails. I don't see specific info about this behavior though, @joshheald might you have some context as the PR author? Would removing this alreadyRetried error case and related code make sense?

Hah, I wish my memory was anything like that good... no more context I can reliably add...

I'd guess it was maybe just extra safety to avoid double charges, because of this line in the PR:

This led to most retry attempts failing, and where they worked, added risk that a payment may be charged twice.

But equally I could see it being a UX choice – if you've retried an error once, and it's failed, it seems unlikely that it's going to work again, so it's more useful to let them start over with a new payment intent. Given that the previous code had always used new payment intents, that may have just been a choice made for safety, to avoid people getting stuck in a loop of retrying a doomed payment intent.

I think that we could probably remove the alreadyRetried code and let the user retry indefinitely, as long as they've got another option – and I'm sure they do – to cancel out of the flow.

There's a field com.stripe-terminal:StripeShouldRetry in the error user info dictionary, but it's not documented officially and probably not worth using it. We could probably improve this flow though.

One thing to explore for this would be to add to our error handling in the processPayment function – it's documented that the error will have a declineCode when it's related to a card being declined, with an optional string being returned. If that can be compared against some set of strings which we think can't be retried, we could somehow indicate that the next retry should be a collectPaymentMethod, if that's the right thing to do, or just nil the payment intent and start with a new one – I'm not sure which is appropriate here.