Skip to content
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

Conversation

samiuelson
Copy link
Contributor

@samiuelson samiuelson commented Dec 3, 2024

Failed payment error handling

Closes: #12827

Description

Design specs: TfaZ4LUkEwEGrxfnEFzvJj-fi-2710_173633

⚠️ Don't merge until the base branch is merged into 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:

  1. Retry payment
  2. Exit order

Steps to reproduce

  1. Go to Woo POS (Menu more > Point of Sale)
  2. Add items to cart and click "Checkout"
  3. Ensure reader is connected
  4. Tap the card to the reader and simulate failed payment. An easy way to do that is to tap a real card when running a debug build.
  5. Verify that "Payment failed" screen is shown
  6. Test "Try another payment method" and "Exit order" buttons

Testing information

  • Payment failed screen should match design specs
  • "Try another payment method" button action should redirect back to Checkout and allow tapping card again
  • "Exit order" button should redirect to Cart, and clear the cart, allowing to make a new order
  • Failed payment state restoration across config changes

The tests that have been performed

  • Samsung Tab S9 FE, android 14
  • Tested "failed payment" state in different scenarios
  • Tested "Try another payment method" button action
  • Tested "Exit order" button action
  • Tested state restoration with "don't keep activities" enabled

Images/gif

Screen_recording_20241204_164738.mp4
  • I have considered if this change warrants release notes and have added them to 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.
12827

Sorry, something went wrong.

@dangermattic
Copy link
Collaborator

dangermattic commented Dec 3, 2024

1 Error
🚫 This PR is tagged with status: do not merge label(s).
1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 3, 2024

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit8b21898
Direct Downloadwoocommerce-wear-prototype-build-pr13057-8b21898.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 3, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit8b21898
Direct Downloadwoocommerce-prototype-build-pr13057-8b21898.apk

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
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
Copy link
Contributor

@kidinov kidinov left a 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(
Copy link
Contributor

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?

Copy link
Contributor Author

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

@samiuelson samiuelson marked this pull request as draft December 6, 2024 09:46
@samiuelson samiuelson marked this pull request as ready for review December 6, 2024 15:04
@samiuelson
Copy link
Contributor Author

Thanks for the review, @kidinov!

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

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:

  1. Try payment again is shown in case the failed payment state provides onRetry lambda. Clicking that button results in onRetry being called, and retrying payment (not even necessarily requiring the card to be tapped again).
  2. Try another payment method is shown in case onRetry lambda is null. Clicking that button cancels current payment intent, redirects back to Checkout and starts payment flow again.
  3. Go back to checkout is shown only in pair with Try payment again. Clicking that button has the same effect as Try another payment method.

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

If I touch the card reader a couple times and leave the flow, it'll deliver the error state a few times

This should be fixed now.

I didn't get this on video but I saw 125 USD prices on the final screen once. Seems liek some caching issue.

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 dataState.value is not set from any device data). Are you still able to reproduce it?

Let me know what you think!

@samiuelson samiuelson requested a review from kidinov December 6, 2024 16:34
when (paymentState) {
is CardReaderPaymentState.CollectingPayment,
is CardReaderPaymentState.LoadingData ->
handlePaymentState(paymentState as CardReaderPaymentState)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No cast needed here

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samiuelson 👋

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

Base automatically changed from custom-payment-ui-8-reader-not-connected to custom-payment-ui-6-use-payment-controller-in-pos December 11, 2024 16:09
@samiuelson
Copy link
Contributor Author

Thanks for review, @kidinov!

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

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).

@samiuelson samiuelson merged commit 41b3dc8 into custom-payment-ui-6-use-payment-controller-in-pos Dec 11, 2024
4 of 8 checks passed
@samiuelson samiuelson deleted the custom-payment-ui-9-payment-failed-states branch December 11, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: do not merge Dependent on another PR, ready for review but not ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants