[Woo POS] [Cash & Receipts] Handle navigation when order completed via cash #14795
Conversation
|
|
iamgabrielma
changed the title
| .matchedGeometryEffect(id: Constants.matchedGeometryCashId, | ||
| in: totalsFieldAnimation) | ||
| } | ||
| } | ||
| .fullScreenCover(isPresented: $shouldShowCollectCashPayment) { |
There was a problem hiding this comment.
I'm not very happy with this duplication here, trying to merge both views under a common type doesn't seem to respond well to changes, specially when dismissing we don't seem to be able to go from the success view to starting a new order, or keeping them instantiated below other views. For reference I used something like the following:
// A common type for which view we'll be presenting as .fullScreenCover:
enum FullScreenCoverType: Identifiable {
case collectCash(total: PointOfSaleOrderTotals)
case paymentSuccess(total: PointOfSaleOrderTotals)
// With a common state, or nil for when there's nothing to present:
@State private var activeFullScreenCover: FullScreenCoverType?
// and we present one of the other when pressing the collection button or through internal state:
if case .loaded(let total) = posModel.orderState {
activeFullScreenCover = .collectCash(total: total.orderTotal)
}
// or
activeFullScreenCover = .paymentSuccess(total: total.orderTotal)
There was a problem hiding this comment.
Yeah, this is a bit strange. Having them in separate presentations is bound to make our life difficult when we want to make the transitions correct though.
My question is whether the fullscreenCover is the correct approach here.
We're asking the merchant to choose between cash and card payment. Both choices can lead to a successful payment. The card option is emphasised, but once the user makes a choice, either has equal weight/importance.
The choice to make it a card payment is made when the shopper taps their card. At that point, we push the new, full-screen "Processing" view into place and focus on the card payment.
For Cash, it might be appropriate to do an equivalent push when the merchant chooses that payment method, by tapping the Cash payment button.
Using fullscreenCover
If we're using a fullscreen cover for this, we should show the success screen in that same view. The New order button should first reset the underlying POS state, then dismiss the cover. That would be consistent use of the presentation approach – Cash takes you out of the main card payment flow, then you tap "Mark complete", and get the success or error all within your branch. If you close it before marking complete, you go back to the same place in card payment. If you tap New order, you don't see the POS rearranging itself underneath, it happens before the cash flow is dismissed.
You may need to think carefully about what "reset the underlying POS state" means – e.g. once you set the orderStage = .building in the PointOfSaleAggregateModel, as happens currently, the TotalsView will no longer be drawn, which causes the cash payment fullscreenCover to be dismissed immediately. The transition from the TotalsView back to editing works well by changing that state... but not so much when the success view is shown in a fullscreen cover.
With the fullscreenCover approach, we don't neccesarily need to use the same view for success. We could, but it might be simpler to have a Cash-specific success view. However, from my initial look at the code I think you'd still have to move the presentation of the fullscreenCover to the dashboard, otherwise it will auto-dismiss when you set the state back to building. Perhaps it's possible to tweak this with matched geometries, but I suspect not.
I was able to get it working to neatly present the success view this way... but it doesn't dismiss all that nicely because I didn't make any changes to the reset code.
single.full-screen.cover.mp4
Code for this (in TotalsView):
.fullScreenCover(isPresented: $shouldShowCollectCashPayment) {
if case .loaded(let total) = posModel.orderState {
NavigationStack {
PointOfSaleCollectCashView(orderTotal: total.orderTotal, onCashPaymentSuccess: {
shouldShowPaymentSuccessView = true
})
.matchedGeometryEffect(id: Constants.matchedGeometryCashId,
in: totalsFieldAnimation)
.navigationDestination(isPresented: $shouldShowPaymentSuccessView) {
let viewModel = PointOfSaleCardPresentPaymentSuccessMessageViewModel(formattedOrderTotal: total.orderTotal)
PointOfSaleCardPresentPaymentInLineMessage(messageType: .paymentSuccess(viewModel: viewModel))
.matchedGeometryEffect(id: Constants.matchedGeometryCashId,
in: totalsFieldAnimation)
.navigationBarHidden(true)
}
.navigationBarHidden(true)
}
}
}
Note that I wouldn't put all this in TotalsView for real code – it should be possible to do it within the CollectCashView, so you don't need the onCashPaymentSuccess closure.
Using an equivalent push to Card payments
I think we can improve the UX of the POS by keeping the navigation approaches for payments equivalent. We also won't need to do any special handling of the POS reset if we do it this way.
When the shopper taps a card, we would push the fullscreen Processing view. When the merchant taps Cash payment, we would push the fullscreen Cash payment view, allowing them to enter the amount and tap Mark complete.
Either way, when the payment action is successful, we push the Order complete view, and allow receipts to be printed and the POS to be reset with the New order button.
That's the theory. Unfortunately, it would need a little rework in the TotalsView code, which currently expects card payments only.
At the moment, it assumes that after the order is loaded, it's usually showing the cardReaderView, which incudes all card payment states (fullscreen and part) and the connect your reader prompt. It's often showing the totals fields, and it depends on the (card)paymentState to decide whether or not the overall TotalsView is fullscreen or not, which is how the "push" to payment processing is shown.
I've not done a deep dive on this, and it would need exploration, but I think we could keep the cardReaderView view builder, but potentially have it show the cash payment screen, as a fullscreen view.
To do it well, we would want to change PointOfSalePaymentState. I think it would be best as a nested state, demoting the existing enum to be PointOfSaleCardPaymentState, adding an equivalent for cash, and then having a case with an associated value for each at the top level. That can get unpleasant to deal with, so a struct might also be appropriate. Or you could try adding acceptingCash and cashPaymentSuccessful to the existing enum, along with a new convenience init.
My concern about adding cash-related states is the whole thing where the card payment service expects to update that state as events happen, and those events might happen right after the merchant taps the Cash payment button. On one hand – accepting a cash payment ought to be a branch in our flow, and we should cancel the card payment first and make that impossible... on the other, the Terminal SDK could end up doing stuff when we don't expect it, and if we ignore those updates and get out of sync with the service, that gets bad too.
This second option isn't well explored yet, and I'm happy to do that with you if you like. Let me know if/how you want to proceed... it'll need some more thought and planning right now, but I think lead to a better experience and more maintainable code in the future.
|
From initial testing, it works well, but various questions which are mostly about me orienting myself in the project after AFK:
Fine to merge with any of the above as it's feature flagged. I'll carry on with the review now. |
| @@ -153,6 +150,11 @@ private extension PointOfSaleCollectCashView { | |||
| value: "Mark payment as complete", | |||
| comment: "Button to mark a cash payment as completed" | |||
| ) | |||
| static let failedToCollectCashPayment = NSLocalizedString( | |||
| "pointOfSale.cashview.failedToCollectCashPayment", | |||
| value: "Error trying to process payment. Try again.", | |||
There was a problem hiding this comment.
Do we expect this to be the final error text for all errors? (I'd suggest not... but in that case it might be better to avoid merging with the key as it is now, or we just end up burning that key. You could append .draft, perhaps?)
What's the potential causes of the error; does a retry make sense?
Presumably we'll have validation errors as well, will they be shown in the same place?
There was a problem hiding this comment.
At the moment I think this is the only case where we should present this string, so it should be fine, as we only land in this catch block when we're unable to update the order. For amount validation errors we should be updating the string directly when we perform validateAmount() (not implemented yet, to be done here: #14749 )
That said it's better to be safe than sorry, I've added a .draft on 9bbd5a3 until we know for sure.
| .matchedGeometryEffect(id: Constants.matchedGeometryCashId, | ||
| in: totalsFieldAnimation) | ||
| } | ||
| } | ||
| .fullScreenCover(isPresented: $shouldShowCollectCashPayment) { |
There was a problem hiding this comment.
Yeah, this is a bit strange. Having them in separate presentations is bound to make our life difficult when we want to make the transitions correct though.
My question is whether the fullscreenCover is the correct approach here.
We're asking the merchant to choose between cash and card payment. Both choices can lead to a successful payment. The card option is emphasised, but once the user makes a choice, either has equal weight/importance.
The choice to make it a card payment is made when the shopper taps their card. At that point, we push the new, full-screen "Processing" view into place and focus on the card payment.
For Cash, it might be appropriate to do an equivalent push when the merchant chooses that payment method, by tapping the Cash payment button.
Using fullscreenCover
If we're using a fullscreen cover for this, we should show the success screen in that same view. The New order button should first reset the underlying POS state, then dismiss the cover. That would be consistent use of the presentation approach – Cash takes you out of the main card payment flow, then you tap "Mark complete", and get the success or error all within your branch. If you close it before marking complete, you go back to the same place in card payment. If you tap New order, you don't see the POS rearranging itself underneath, it happens before the cash flow is dismissed.
You may need to think carefully about what "reset the underlying POS state" means – e.g. once you set the orderStage = .building in the PointOfSaleAggregateModel, as happens currently, the TotalsView will no longer be drawn, which causes the cash payment fullscreenCover to be dismissed immediately. The transition from the TotalsView back to editing works well by changing that state... but not so much when the success view is shown in a fullscreen cover.
With the fullscreenCover approach, we don't neccesarily need to use the same view for success. We could, but it might be simpler to have a Cash-specific success view. However, from my initial look at the code I think you'd still have to move the presentation of the fullscreenCover to the dashboard, otherwise it will auto-dismiss when you set the state back to building. Perhaps it's possible to tweak this with matched geometries, but I suspect not.
I was able to get it working to neatly present the success view this way... but it doesn't dismiss all that nicely because I didn't make any changes to the reset code.
single.full-screen.cover.mp4
Code for this (in TotalsView):
.fullScreenCover(isPresented: $shouldShowCollectCashPayment) {
if case .loaded(let total) = posModel.orderState {
NavigationStack {
PointOfSaleCollectCashView(orderTotal: total.orderTotal, onCashPaymentSuccess: {
shouldShowPaymentSuccessView = true
})
.matchedGeometryEffect(id: Constants.matchedGeometryCashId,
in: totalsFieldAnimation)
.navigationDestination(isPresented: $shouldShowPaymentSuccessView) {
let viewModel = PointOfSaleCardPresentPaymentSuccessMessageViewModel(formattedOrderTotal: total.orderTotal)
PointOfSaleCardPresentPaymentInLineMessage(messageType: .paymentSuccess(viewModel: viewModel))
.matchedGeometryEffect(id: Constants.matchedGeometryCashId,
in: totalsFieldAnimation)
.navigationBarHidden(true)
}
.navigationBarHidden(true)
}
}
}
Note that I wouldn't put all this in TotalsView for real code – it should be possible to do it within the CollectCashView, so you don't need the onCashPaymentSuccess closure.
Using an equivalent push to Card payments
I think we can improve the UX of the POS by keeping the navigation approaches for payments equivalent. We also won't need to do any special handling of the POS reset if we do it this way.
When the shopper taps a card, we would push the fullscreen Processing view. When the merchant taps Cash payment, we would push the fullscreen Cash payment view, allowing them to enter the amount and tap Mark complete.
Either way, when the payment action is successful, we push the Order complete view, and allow receipts to be printed and the POS to be reset with the New order button.
That's the theory. Unfortunately, it would need a little rework in the TotalsView code, which currently expects card payments only.
At the moment, it assumes that after the order is loaded, it's usually showing the cardReaderView, which incudes all card payment states (fullscreen and part) and the connect your reader prompt. It's often showing the totals fields, and it depends on the (card)paymentState to decide whether or not the overall TotalsView is fullscreen or not, which is how the "push" to payment processing is shown.
I've not done a deep dive on this, and it would need exploration, but I think we could keep the cardReaderView view builder, but potentially have it show the cash payment screen, as a fullscreen view.
To do it well, we would want to change PointOfSalePaymentState. I think it would be best as a nested state, demoting the existing enum to be PointOfSaleCardPaymentState, adding an equivalent for cash, and then having a case with an associated value for each at the top level. That can get unpleasant to deal with, so a struct might also be appropriate. Or you could try adding acceptingCash and cashPaymentSuccessful to the existing enum, along with a new convenience init.
My concern about adding cash-related states is the whole thing where the card payment service expects to update that state as events happen, and those events might happen right after the merchant taps the Cash payment button. On one hand – accepting a cash payment ought to be a branch in our flow, and we should cancel the card payment first and make that impossible... on the other, the Terminal SDK could end up doing stuff when we don't expect it, and if we ignore those updates and get out of sync with the service, that gets bad too.
This second option isn't well explored yet, and I'm happy to do that with you if you like. Let me know if/how you want to proceed... it'll need some more thought and planning right now, but I think lead to a better experience and more maintainable code in the future.
The top level enum for POS Payment State now has 2 options: card or cash, and the specific payment state is known through their associated values
We no longer use fullscreencover to present the cash and success view but rely on the cash payment state, sharing the flow with card payment state
|
Thanks for the review and your input on how this should be handled @joshheald , greatly appreciated 🙇
Yes, no validation around the amount or order total is done yet, any value (or no value) passes the check. Will be tackled here: #14749
Is not logged yet, added here: #14808
Oh! I didn't know this was a thing, I see in the app that we have a specific "Record transaction details in order note" toggle to log those or not, I logged this separately here #14809 and will check with Android as well. fullscreenCover vs push payment stateThanks for the guidance here, I've tested a bit the fullScreenCover presentation but as you mention it feels less solid and more prone to future issues than pushing payment state and treating both cash and card payments with the same priority. In 25771f2 we make payment state to have only 2 cases, cash or card, and the state is handled via the associated values. As you mention is a bit more cumbersome to handle the cases, but I don't feel it's a big deal either, as also makes them clearer and more granular. On de82cd1 we let the system know that we're collecting cash or that cash has been successfully collected and then what should be presented is handled automatically. I'm also interested in any thoughts you might have about returning to the cart building stage upon we cancel/dismiss the cash view. Here: 32a7d29 I've went ahead with this for simplicity but I haven't dig deeper into what should be the card payment state if we cancel cash collection, so it felt safer to do it like this for now. There's a few details to handle still, I can handle those in this PR or in a follow-up for simplicity. For example:
Screen.Recording.2025-01-07.at.13.57.34.mov |
There was a problem hiding this comment.
Nice one getting the state updated to reflect reality! Generally looking better – some comments in line about improvements to the state management.
The main thing left to do is cancelling and re-instating card reader preparation. Comments inline for details.
You may also want to try some transition modifiers for moving between cash payment and cash success... or grouped geometry to have the title and buttons appear to move around... it'll need some experimentation, but it doesn't feel right to just jump from one to another.
Comments inline to answer most of the other questions...
| func cashPaymentSuccess() { | ||
| paymentState = .cash(.paymentSuccess) | ||
| } | ||
|
|
||
| @MainActor | ||
| func collectCashPayment() async throws { | ||
| try await orderController.collectCashPayment() |
There was a problem hiding this comment.
cashPaymentSuccess() should probably be triggered here, instead of from the view.
There was a problem hiding this comment.
You're totally right, updated on 9e1a5c4
| @@ -199,6 +199,14 @@ extension PointOfSaleAggregateModel { | |||
| } | |||
| } | |||
|
|
|||
| func startCashPayment() { | |||
| paymentState = .cash(.collectingCash) | |||
There was a problem hiding this comment.
Setting the payment state is always risky – the card payment service can just change it. We'll have to do it for this approach to work... but while PointOfSaleAggregateModel is the source of truth for the payment state, it can be mutated by outside events.
We should cancel the prepared card payment here, and ideally ignore/silence card reader events that happen until the cash payment is completed, or cancelled. Note that cancelling a payment is not instant, and might fail (this is unlikely, unless the shopper has tapped their card.)
Currently, the card reader is still listening for payments in the background, and it's possible to pay for an order twice – cash and card – if you tap the card right after the cash payment is made. See the video; I checked and the card payment did go through, even though the order is paid by cash.
cash.and.card.payment.mp4
There was a problem hiding this comment.
I might need some further guidance here to make this more solid. I've been trying what would be the best way to handle disallowing card payments while cash collection is ongoing, and the closer I got is here: 2ee6163
- Starting to collect cash triggers
isCashPaymentInProgress - When new payment events go through, if the payment event is of type
.cardandisCashPaymentInProgressis true, then we cancel it. - If cash collection is cancelled, we re-enable card payment events via
startPaymentWhenCardReaderConnected()so the user can just tap without further setup - If cash collection is successful, we just trigger back
isCashPaymentInProgressto false.
While it seems to work 90% of the time, I still see some strange behaviour with the payment state sometimes when trying to collect cash and tapping the card in the reader:
- At times, the view has gone blank when the cash view is rendered and we tap the card reader, despite the state not changing from
.cash(.collectingCash)at any point. - At other times, if we start a new order, and attempt to pay with card, it seemed to need a few attempts to work. One time I also saw the error
CardPresentPaymentInvalidatablePaymentOrchestratorError, then it worked normally. - I'm not quite sure when I should use
cancelCardReaderPreparation()versus justcardPresentPaymentService.cancelPayment(). I went ahead with the later so we do not cancelstartPaymentOnCardReaderConnection.
There's a specific issue logged for handling this on #14682 , we can either continue in this PR, or I can revert the last change and move it there, whatever you prefer.
| @@ -34,7 +31,7 @@ struct PointOfSaleCollectCashView: View { | |||
| VStack(alignment: .center, spacing: 20) { | |||
| HStack { | |||
| Button(action: { | |||
| dismiss() | |||
| posModel.addMoreToCart() | |||
There was a problem hiding this comment.
We shouldn't take the merchant all the way back to the item list if they go back from cash payment – they may just want to take a card payment instead.
I think this should be something else, posModel.cancelCashPayment(), which would take the merchant back to the finalizing step, and restart the card reader preparation if it's connected.
There was a problem hiding this comment.
True, updated on d01819a. At this point we do not need to set .finalizing specifically, as it's already set from checking out, so switching payment state to the initial state is enough to navigate the user back. Added some tests as well 👍
WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift
Outdated
Show resolved
Hide resolved
Part of #14602
Description
This PR addresses POS navigation when we attempt to collect a cash payment for an order:
Currently we're using the
PointOfSaleCardPresentPaymentSuccessMessageViewfor both card payments and cash payments success scenarios, and the differentPointOfSaleCardPresentPaymentMessageTypecases are tied to the card payment state.I've approached this without touching the card payment state or the card service but by introducing a property to react alongside the card payment messages and leaving the completion handling to the app side, as the POS does nothing regarding cash. Happy to iterate as needed, as another option could be to use some sort of
PaymentResultstate to track both card or cash state as needed.Testing
.acceptCashForPointOfSaleflagError:
Screen.Recording.2025-01-06.at.16.21.16.mov
Success:
Screen.Recording.2025-01-06.at.16.22.38.mov
RELEASE-NOTES.txtif 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: