Always mark failed order as pending before re-attempting a payment #14746

staskus · 2024-12-20T18:26:47Z

Description

From WooCommerce 8.6+ order is set to failed when card_present payment payment fails. It triggers a chain reaction on WooCommerce, which performs actions when the status changes to failed, most importantly sending email receipts.

However, if we make another failed payment on an order with an already failed status, the actions are no longer triggered.

In #14475, I made a change that sets order to pending when re-collecting the payment from Order Details view. It covers most of the scenarios but not all. It didn't take into account payment retries or later introduced a fallback payment method.

On the web, setting order status to pending is the first action when processing payment.

Of course, it's not ideal to make such a change from the client side and likely we should consider looking for backend solutions as well 🤔 .

Steps to reproduce

Retrying the same payment won't generate an email for another reason pcreKM-3aC-p2. However, this fix should allow repeated payment to work once the backend is improved.

For now, using a fallback payment method flow allows to test a specific scenario.

Re-collect order (Scenario already works on 21.3)

Create an order with failure amount (1.05)
Make Tap to Pay / Card Reader payment
In failed payment alert, enter and send email, confirm email arrives
Come back to the order
Tap 'Collect Payment' again and pay with Tap to Pay / Card Reader
Failed payment alert should display the email, confirm email arrives

Fallback payment method flow (Scenario doesn't work on 21.3)

Create an order with 1.02 amount
Tap to Pay
In failed payment alert, enter and send email, confirm email arrives
Dimiss failed the payment alert with 'Try Another Payment Method'
Select Tap to Pay again
Failed payment alert should display the email, confirm email arrives

Testing information

Tested on iPhone 14 Pro 17.7

This change only affects failed orders which are only set to failed from WooCommerce 8.6. It won't affect all the other existing flows.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

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 all devices (phone/tablet) and no regressions are added.

wpmobilebot · 2024-12-20T18:38:14Z

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

	App Name	WooCommerce iOS
	Build Number	pr14746-ef6a065
	Version	21.3
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`ef6a065`
	App Center Build	WooCommerce - Prototype Builds #12337

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

jaclync

Had some questions about the order status update, but testing on an iPhone device worked so didn't want to block the PR.

Re-collect order (Scenario already works on 21.3)

For two WPCOM test stores with WC 9.5.1 and WooPayments 8.6.1, I didn't receive an email from failed payments on iPad 10th gen iOS 18.0 simulator. But when testing on iPhone device worked, not sure why 😅 Both with simulated card reader.

Just curious, how is email sent from core, it probably costs something to send email?

jaclync · 2024-12-23T01:13:40Z

WooCommerce/Classes/ViewRelated/Orders/Collect Payments/CollectOrderPaymentUseCase.swift

+        let action = OrderAction.updateOrderStatus(siteID: order.siteID,
+                                                   orderID: order.orderID,
+                                                   status: .pending, onCompletion: { _ in })
+        stores.dispatch(action)


❓ do we need to wait for the async order status update to complete before the next step in order payment collection for the email feature to work consistently? Probably no issues in good network conditions, but there might be edge cases like if this API request fails or takes longer than the next payment steps. On the other hand, if we wait for this order status remote update, it adds to the total time to process a payment.

After taking a look at the OrderACtion.updateOrderStatus implementation, it looks like it's just a local update in storage (and the return value seems misleading with the old status but this is unrelated to the PR). How does this have the effect remotely to trigger the order status pending -> failed actions on the server side, is the order updated remotely somewhere else? If so, maybe a comment is helpful for future developers to know how this order status update takes effect.

Of course, it's not ideal to make such a change from the client side and likely we should consider looking for backend solutions as well 🤔 .

If Woo mobile is the only client of the payments/orders/orderID/capture_terminal_payment endpoint (or another endpoint made for payment), I think we can add the order status update there.

After taking a look at the OrderACtion.updateOrderStatus implementation, it looks like it's just a local update in storage (and the return value seems misleading with the old status but this is unrelated to the PR

This action calls func updateOrder(siteID: Int64, orderID: Int64, status: OrderStatusEnum, onCompletion: @escaping (Error?) -> Void) method which does call remote.

However, I agree that it should be done clearer.

Probably no issues in good network conditions, but there might be edge cases like if this API request fails or takes longer than the next payment steps. On the other hand, if we wait for this order status remote update, it adds to the total time to process a payment.

It's true, and yes, I don't want to be making the request even slower.

If Woo mobile is the only client of the payments/orders/orderID/capture_terminal_payment endpoint (or another endpoint made for payment), I think we can add the order status update there.

That's a good idea and I would prefer that. I will not merge this PR for now and explore this option.

We're not calling our API for the failed payment, only Stripe API.

But I think there's more more options to explore:

Could payment gateways trigger woocommerce_order_status_failed_notification which is used to send emails during the repeated payment failure. The more I look at these local changes, the more I dislike them.

I opened up this topic pcreKM-3aC-p2#comment-4213 here. Maybe it could be improved together with other necessary improvements.

Another potential solution is to set the order to pending while checking order eligibility. That allows to wait for completion while integrating within already existing request and not prolonging the payment:

f6aeb04

I'll leave it at that for now.

Another potential solution is to set the order to pending while checking order eligibility. That allows to wait for completion while integrating within already existing request and not prolonging the payment:

f6aeb04

Yea, setting the order status as pending at the beginning of the payment could work, can double-check with the team just to make sure there are no side effects as lots of actions and filters depend on the order status. In the commit it looks like we can replace the remote order retrieval with the remote order status update, so performance wise it's likely similar to before.

jaclync · 2024-12-23T01:33:17Z

...mmerce/WooCommerceTests/ViewModels/CardPresentPayments/CollectOrderPaymentUseCaseTests.swift

@@ -361,6 +361,75 @@ final class CollectOrderPaymentUseCaseTests: XCTestCase {
        // Then
        XCTAssertEqual(mockPaymentOrchestrator.spyChannel, .pos)
    }
+
+    func test_collectPayment_when_order_failed_then_marked_as_pending() async throws {


thanks for adding the test cases!

dangermattic · 2024-12-23T13:17:29Z

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

…nding-when-making-payment

wpmobilebot · 2025-01-10T03:47:24Z

Version 21.4 has now entered code-freeze, so the milestone of this PR has been updated to 21.5.

Always mark failed payment as pending

b48304e

staskus added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. labels Dec 20, 2024

staskus added this to the 21.4 milestone Dec 20, 2024

staskus requested review from jaclync and iamgabrielma December 20, 2024 18:26

jaclync self-assigned this Dec 23, 2024

jaclync approved these changes Dec 23, 2024

View reviewed changes

staskus added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 23, 2024

staskus marked this pull request as draft December 23, 2024 13:58

staskus added 3 commits December 23, 2024 19:07

Set order to pending when checking order eligibility

f6aeb04

Remove markFailedOrderPending

f1205d1

Merge branch 'trunk' into task/ttp-req-always-mark-failed-order-as-pe…

ef6a065

…nding-when-making-payment

wpmobilebot modified the milestones: 21.4, 21.5 Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always mark failed order as pending before re-attempting a payment #14746

Always mark failed order as pending before re-attempting a payment #14746

staskus commented Dec 20, 2024 •

edited by jaclync

Loading

wpmobilebot commented Dec 20, 2024 •

edited

Loading

jaclync left a comment •

edited

Loading

jaclync Dec 23, 2024

jaclync Dec 23, 2024

jaclync Dec 23, 2024

staskus Dec 23, 2024

staskus Dec 23, 2024

staskus Dec 23, 2024

staskus Dec 23, 2024

jaclync Dec 30, 2024

jaclync Dec 23, 2024

dangermattic commented Dec 23, 2024 •

edited

Loading

wpmobilebot commented Jan 10, 2025

Always mark failed order as pending before re-attempting a payment #14746

Are you sure you want to change the base?

Always mark failed order as pending before re-attempting a payment #14746

Conversation

staskus commented Dec 20, 2024 • edited by jaclync Loading

Description

Steps to reproduce

Re-collect order (Scenario already works on 21.3)

Fallback payment method flow (Scenario doesn't work on 21.3)

Testing information

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Dec 20, 2024 • edited Loading

jaclync left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangermattic commented Dec 23, 2024 • edited Loading

wpmobilebot commented Jan 10, 2025

staskus commented Dec 20, 2024 •

edited by jaclync

Loading

wpmobilebot commented Dec 20, 2024 •

edited

Loading

jaclync left a comment •

edited

Loading

dangermattic commented Dec 23, 2024 •

edited

Loading