[Woo POS] Consider an order that is already paid as success in CollectOrderPaymentUseCase

[jaclync]

Closes: #13128
Based on #13165

Description

As capture_terminal_payment could succeed on the server side but fail returning the response, and if order sync still fails after the graceful handling in #13165, the app/POS results in a server error state. After retrying by canceling and collecting payment again, it currently results in an "already paid" error state.

server-side error	after retrying - already paid error

If we know that the order is already paid, transitioning to the success state would be a better UX instead of a non-retryable error state in POS. The changes in this PR should only affect POS and not the existing app, since the order might not always be up to date.

Steps to reproduce

Adapted from #13165:

Existing IPP experience (no changes are expected)

1. Set up Proxyman or Charles to intercept traffic for your test device
2. Launch the app using a store eligible for IPP
3. Navigate to Orders > +
4. Add a new order, and tap Collect Payment
5. Select Card reader and connect to the reader if needed
6. Enable breakpoints in your proxy for the response of POST requests to https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/*/rest-api/ and GET requests to https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/*/rest-api/?json=true&path=/wc/v3/orders/*
7. Tap your card on the reader
8. Abort the capture_terminal_payment response when the breakpoint is hit (equivalent to poor network conditions meaning it was lost.)
9. Shortly after, abort the orders/* response when the breakpoint is hit --> the server-side error state should be shown
10. Dismiss the error alert
11. Tap Collect Payment for the same order again --> the "already paid" error alert should be shown

New POS experience

1. Set up Proxyman or Charles to intercept traffic for your test device
2. Launch the app using a store eligible for IPP
3. Navigate to Menu > Point of Sale mode
4. Add an item to your basket and tap Checkout
5. Enable breakpoints in your proxy for the response of POST requests to https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/*/rest-api/ and GET requests to https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/*/rest-api/?json=true&path=/wc/v3/orders/*
6. Shortly after, abort the orders/* response when the breakpoint is hit --> the server-side error state should be shown
7. Tap Cancel payment and then Collect payment again
8. Tap your card on the reader
9. Observe that the payment shows as successful

Testing information

Screenshots

After this PR, the success state should be shown:

---

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

[wpmobilebot]

📲 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	pr13178-0555475
	Version	19.2
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	0555475
	App Center Build	WooCommerce - Prototype Builds #9804

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

[joshheald]

I don't think this is the right approach, because of the change in the existing app.

I think there, we should still show an "order already paid" error, and adapt it in the POS only.

More detail in the in-line comment.

[joshheald]

I don't think we should return success here, in the existing flows. The error is clearer for some of the situations which can come up.

For POS, where we're really confident that the order's just been created, we know that the most likely (perhaps only possible) reason for hitting this error is after retry after the missed network connection as covered in #13165. In that case, the shopper will have just tapped their card, so it's not confusing to see a success payment.

For the existing flow, where we rely more on storage and orders can be much older, it's not so safe to show success here. A merchant could have an outdated copy of the order from storage, tap Collect Payment, then immediately see that the order's complete without the customer tapping their card, nor any explanation of how it's complete.

In that situation, they're likely to think that the app is wrong and that they need to redo the order. The error would have explained it much more clearly.

The best place to do this for the new experience is probably in CardPresentPaymentAlertPresenterAdaptor.present, e.g. with a case like this one:

    case .paymentError(error: CollectOrderPaymentUseCaseError.orderAlreadyPaid, _, _),
                .paymentErrorNonRetryable(error: CollectOrderPaymentUseCaseError.orderAlreadyPaid, _):
            paymentEventSubject.send(.show(eventDetails: .paymentSuccess(done: {})))

It probably needs to do more than just that, e.g. we'll need to clean up something in the facade, but that's my idea for a starting point... although from a quick try, it does look like it "just works".

[jaclync]

I don't think we should return success here, in the existing flows. The error is clearer for some of the situations which can come up.

For POS, where we're really confident that the order's just been created, we know that the most likely (perhaps only possible) reason for hitting this error is after retry after the missed network connection as covered in #13165. In that case, the shopper will have just tapped their card, so it's not confusing to see a success payment.

For the existing flow, where we rely more on storage and orders can be much older, it's not so safe to show success here. A merchant could have an outdated copy of the order from storage, tap Collect Payment, then immediately see that the order's complete without the customer tapping their card, nor any explanation of how it's complete.

In that situation, they're likely to think that the app is wrong and that they need to redo the order. The error would have explained it much more clearly.

Thanks a lot for the explanation about why this change shouldn't be applied to the existing flow, it makes sense. @staskus and I had a pair programming session to work on this issue, and your suggested fix works from my testing! (I tested that this scenario results in success for POS and remains an error in the existing flow in order creation).

It probably needs to do more than just that, e.g. we'll need to clean up something in the facade, but that's my idea for a starting point... although from a quick try, it does look like it "just works".

For this, I'd like to check with you. There are also 2 other TODO comments about calling the facade's cancelPayment for further cleanup. I tried triggering CardPresentPaymentService.cancelPayment in the event's cancelPayment closure, but the current implementation of cancellation is to also call the event's cancelPayment closure:

woocommerce-ios/WooCommerce/Classes/POS/Card Present Payments/CardPresentPaymentCollectOrderPaymentUseCaseAdaptor.swift

Lines 144 to 146 in 802b6bb

	.paymentError(_, _, cancelPayment: let cancelPayment),
	.paymentErrorNonRetryable(_, cancelPayment: let cancelPayment):
	cancelPayment()

In CardPresentPaymentCollectOrderPaymentUseCaseAdaptor, we could also change how we handle the cancellation. An option is to cancel the payment intent, which I think we could do for non-retryable payment errors (there could be exceptions that I didn't know of). Does this sound like the right way to go, canceling the payment intent for non-retryable payment errors (moving this line to the .processing cases below)?

For retryable payment errors, is there any cleanup we should do? In place of this TODO comment.

If this is a bit too much to take on before your AFK, I can create a separate task for this further cancelation cleanup for later.

[joshheald]

For this case I don't think it would be appropriate or needed to call cancelPayment. The orderAlreadyPaid error is thrown before we create the payment intent, so there's nothing to cancel yet.

From our testing, it's working without further cleanup, so that's great. It makes sense actually, because we throw away the CollectOrderPaymentUseCase when the merchant taps New transaction.

However, CollectOrderPaymentUseCase doesn't know anything about this success, so we're tracking this as a payment failure in our analytics... or actually two failures:

We're not really dealing with this yet, but it would be nice to track success if we can. This can be delayed but let's capture it in a ticket. It's fine to track the failures, but we should track the success as well at the end.

I think perhaps we should defer those TODOs for after my AFK – we'll have a new project to tighten everything up and deal with the TODOs... I can't dig in to them now.

[jaclync]

However, CollectOrderPaymentUseCase doesn't know anything about this success, so we're tracking this as a payment failure in our analytics... or actually two failures:

We're not really dealing with this yet, but it would be nice to track success if we can. This can be delayed but let's capture it in a ticket. It's fine to track the failures, but we should track the success as well at the end.

Good catch on the tracking side, yea we'd want to make sure to track the success state for this case. I created a draft task for this, currently in the "Blocked" column because we don't have an analytics plan yet. I suggested tracking POS success when the view model receives the payment success event and is about to show the success UI.

[jaclync]

Thanks for working on the changes and pushing the commits @staskus, I added a commit to move the handling of the "already paid" error before the default error handling because Swift executes switch cases in order.

@joshheald I've updated the PR & description if you get a chance to review it! If not, feel free to just briefly share what you think makes the most sense - leaving the PR open, creating a separate issue for further cleanup, etc.

[jaclync]

Thanks for reviewing & testing this Josh 🙏 merging it now!