-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[IPP/POS] Automatically Cancel Payment Intents #13719
base: trunk
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
paymentIntent.status == PaymentIntentStatus.REQUIRES_CONFIRMATION | ||
if (paymentIntent.status != PaymentIntentStatus.SUCCEEDED && | ||
paymentIntent.status != PaymentIntentStatus.CANCELED && | ||
paymentIntent.status != PaymentIntentStatus.REQUIRES_CAPTURE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 Previously, we were whitelisting statuses that should be cancelled. I believe this is not correct and we should attempt to cancel all nonfinal statuses. Ideally even Requires capture
, however, this one is more tricky as mentioned in the description of this PR.
} | ||
} | ||
} finally { | ||
if (!eligibleForRetry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that if the PaymentIntent can be used for retries => we shouldn't cancel it as we'd break the retry logic, but we should cancel all other non-final payment intents.
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Closes: #13593
Description
Note: The code complexity introduced in this PR is IMO not worth merging, especially considering it doesn't solve all the edge-cases. I'm creating the PR so we can discuss it and document the decision, however, I'd personally vote for leaving the code as is (prior this PR). cc @kidinov @samiuelson @AnirudhBhat in case you believe this PR is still worth merging or if you believe we should continue looking into this.
This PR adds an additional safety measure to ensure the app doesn't leave Payment Intents in an uncaptured state (on hold state) - this is not a critical issue, since the payment authorization expires after a few days and the Payment Intent gets cancelled automatically.
We added a basic safety measure a few weeks ago in this PR which prevents users from leaving the payment flow by tapping on the back button. However, this solution doesn't cover edge cases and is dependent on the implementation of the client app which could potentially bite us in the future. The goal of this PR is to ensure this issue is solved even if the client app doesn't suppress the back button.
This PR doesn't completely solve the problem since we are not cancelling Payment Intents in the
Requires Capture
state, since the Stripe SDK doesn't change the status of the Payment Intent after it's successfully captured -> this is understandable, since capturing payment intent doesn't involve communication with the SDK at all, the app communicates only with the Woo site and the site communicates with Stripe's backend. We could potentially try to cancel PI in theRequires capture
state and rely on the fact that completed payments can't be cancelled, however, this doesn't work either, since Stripe refunds the payment in such case which is likely not what we want.Bottom line is, that I believe preventing users from pressing the back is a good enough solution and it's likely not worth further effort to try to fix this in the CardReaderModule layer.
Steps to reproduce
Revert changes done in this PR to ensure you can easily reproduce the issue.
Testing information
The tests that have been performed
I have tested many different scenarios
Images/gif
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: