-
Notifications
You must be signed in to change notification settings - Fork 131
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.
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
[POS][Custom payment UI] – Failed payment error handling #13057
[POS][Custom payment UI] – Failed payment error handling #13057
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.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
give home screen is at checkout, when exit order clicked after failed payment, then should show cart
give home screen is at checkout, when payment processing started, then should show full screen totals state
give home screen is at checkout, processing payment, when payment fails, then should show full screen totals state
give home screen is at checkout, failed payment, when retry payment clicked, then should show cart with totals
...erce/android/ui/woopos/home/totals/payment/processing/WooPosTotalsPaymentProcessingScreen.kt
Show resolved
Hide resolved
given non-empty cart, when card payment is aborted, then should clear the cart
given order draft created and reader connected, when card tapped, should show payment processing screen
given payment failed, when retry clicked, then should retry
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.
LGTM! Cool that the animation worked pretty out of the box
I found a weird behavior though. I have a product which was 125 USD prices, I changed it to 0.20 cents to emulate an error. Not it shows the error first, but when I try again it passed
12-05--16-43.mp4
If I touch the card reader a couple times and leave the flow, it'll deliver the error state a few times
12-05--16-47.mp4
I didn't get this on video but I saw 125 USD prices on the final screen once. Seems liek some caching issue
} | ||
} | ||
} | ||
} | ||
|
||
private fun buildPaymentFailedState(): PaymentFailed = PaymentFailed( |
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.
Why don't we use string id from PaymentFlowError
to show an error text that helps to resolve the issue?
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.
I initially wanted to do that in a separate PR, but it turned out to be very simple, so I did it in this commit – 8b4f5f7
Exit order -> Go back to checkout
Thanks for the review, @kidinov!
I've analyzed the iOS implementation (p1733476692009279-slack-C070SJRA8DP), and I updated the error handling logic in 5b12dd8 and 6c2a7e7 accordingly. In nutshell, the actions behind buttons are modified:
Now, in case of $0.20 purchase, the error should persist when you click "Try payment again". However, the transaction is successful when you restart the payment collection from scratch by clicking "Go back to checkout". This seems to be the intended behavior and happens in IPP as well (see the video below) – the payment fails the first time, also consecutive retries fail, however, if we cancel that payment intent and repeat the payment action from scratch, it goes through. updated-error-handling.mp4
This should be fixed now.
This is weird. I tried the same scenario—updating the price of a product to $0.20—but I didn't manage to reproduce it. The total value comes directly from the server's order creation response though, so it could have been a backend issue (total in Let me know what you think! |
when (paymentState) { | ||
is CardReaderPaymentState.CollectingPayment, | ||
is CardReaderPaymentState.LoadingData -> | ||
handlePaymentState(paymentState as CardReaderPaymentState) |
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.
No cast needed here
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.
Done - 8b21898
paymentStateText = paymentState.javaClass.simpleName | ||
) | ||
} else { | ||
val order = totalsRepository.getOrderById(dataState.value.orderId) |
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.
Are you sure that in both cases where you get the order, it won't be better just to crash if it's not in the DB? Or is there a legit case when its null?
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.
Yeah, probably that makes more sense to crash. Done - 8b21898
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.
I retested, looks good! I left a couple of comments too, please take a look
This seems to be the intended behavior and happens in IPP as well (see the video below) – the payment fails the first time, also consecutive retries fail, however, if we cancel that payment intent and repeat the payment action from scratch, it goes through.
You think this is like that because it's a test mode?
What's weird is that now it looks like that we don't show proper message why the payment fails for the amounts lower than minimal amount. Like something wrong either on the backend side and we get the wrong response or on our side
Thanks for review, @kidinov!
I reproduced that behavior (both successful $0.20 payment the second time in a row and lack of specific error message) on tag 21.2 before payments refactor was merged into trunk, so it's definitely backend behavior. My bet would be it's because of the test mode in Woo Payments, but might be worth looking into it (creating issue here). |
41b3dc8
into
custom-payment-ui-6-use-payment-controller-in-pos
Failed payment error handling
Closes: #12827
Description
Design specs: TfaZ4LUkEwEGrxfnEFzvJj-fi-2710_173633
trunk
.The intent of this PR is to display the "Payment failed" full-screen state in case the payment fails. The error screen should allow two actions:
Steps to reproduce
Testing information
The tests that have been performed
Images/gif
Screen_recording_20241204_164738.mp4
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: