[Woo POS][Cash & Receipts] Render cash button and navigate to cash view #14724

iamgabrielma · 2024-12-18T14:27:39Z

Description

This PR adds the cash payment button to the POS dashboard, and the initial navigation back and forth between both screens.

Please disregard the UI details on the cash view, we'll address the design when implementing the cash functionality and error handling.

Changes

Adds two new payment states: .acceptingCash and .cashPaymentSuccessful
Adds a PointOfSaleCollectCashView
As with Android, we present the view as standalone view, rendered from the POS dashboard. Upon tapping in cash collection we switch the payment state, which drives rendering the new view, upon navigating back, we return to the previous payment state which restores the dashboard as it was.

Testing

Enable .acceptCashForPointOfSale
Run the POS on a physical device
Start to process a cart checkout, with a disconnected reader, observe the cash payment buttons appears and it's tappable below the reader not connected error:

Observe the order total appears on the the right top corner below the navigation button

Connect the reader and observe the button still renders. Tapping the button and navigating back and forth retains the payment state, so the reader should remain connected:

RPReplay_Final1734600371.MP4

At the moment "Mark payment as complete" is not functional.

Screenshots

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.

iamgabrielma · 2024-12-19T10:24:43Z

👋 there's no rush to review this as targets 21.4, it can wait to next week.

joshheald

Is it supposed to be modal, or a push navigation? It looks and feels like it's supposed to be a fullscreen cover modal, but it's not totally clear because of the back button. Certainly fullscreen cover would be a closer match to the bottom sheet shown in Wagner's walkthrough.

A couple of areas to look at:

The transition to and from cash payments is weird, and I think it's because of the view being presented from the dashboard.
Payment state – as a general principle, don't mess with the source of truth, it'll cause bugs and make everything complicated!

Transition weirdness

I don't think presenting cash payments from the dashboard is ideal – perhaps try a fullscreenCover over the Totals view if it should be modal. If you do this, use a close button not the back button.

Note the way the underlying totals views navigate in and out. Fixing this is going to be difficult with the way you're presenting it at the moment, I'd advise against it to keep things simple! A fullscreenCover presentation should just avoid all these issues.

Transitions.for.cash.payment.mp4

Payment state

The aggregate model exposes the source of truth for (card) payments, but it comes from the card payment service – you can't just change it. If you do, the card payment service will keep doing its thing and that makes the cash payment go away.

When the merchant taps Cash payment, they are implicitly saying "This is not a card payment". So we should cancel the card payment preparation, otherwise the reader will just be sitting there waiting to be tapped. The simulator shows this really well because it insta-taps.

Fortunately we already support cancellations and re-preparation well: we do it when you go back to the item list and forward to the checkout again. Those functions should be all you need to handle the state management for cash payments.

card.payment.interfering.with.cash.mp4

Various other comments inline. I know the UI's not done yet, but just a note – that back arrow is very Androidy, and perhaps these designs are Android-first so need some translation. I think we should use a chevron.backward where we need similar for iOS, but you could raise it with Wagner to check. This feels like somewhere platform convention comes before the design system.

joshheald · 2024-12-19T11:23:03Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

@@ -61,6 +61,7 @@ class PointOfSaleAggregateModel: ObservableObject, PointOfSaleAggregateModelProt

    private var startPaymentOnCardReaderConnection: AnyCancellable?
    private var cardReaderDisconnection: AnyCancellable?
+    private var latestPaymentState: PointOfSalePaymentState? = nil


To avoid issues around keeping state up to date, it would be better to avoid this duplication. Can we get what we need from the paymentState?

In this case you mean to keep track of "latest state before it switches to new state" internally within PointOfSalePaymentState, and then access that? It definitely feels better, yes. For the moment I've reverted the changes to payment state with the changes to navigation, but I'll take that into account going forward.

joshheald · 2024-12-19T11:25:31Z

WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift

+    func collectCashPayment() {
+        // Capture the latest payment state before switching to cash collection
+        // so we can return if the collection is cancelled
+        latestPaymentState = paymentState
+        paymentState = .acceptingCash
+    }
+
+    func cancelCashPayment() {
+        if let latestPaymentState = latestPaymentState {
+            paymentState = latestPaymentState
+        }
+    }


I'm not sure that this is particularly safe. Setting the state back to something it used to be risks it being out-of-sync with the card payment service, and what happens then?

It would probably be better to cancel the reader preparation when they tap cash payment, then trigger it again if they cancel. I believe that's what's being done on Android too, so keeping platform consistency would be good.

That makes sense, thanks for the pointer. I've reverted these changes for the time being.

It would probably be better to cancel the reader preparation when they tap cash payment, then trigger it again if they cancel. I believe that's what's being done on Android too, so keeping platform consistency would be good.

👍 I've added a note to #14682 for when implementing collecting and cancelling

joshheald · 2024-12-19T11:26:46Z

WooCommerce/Classes/POS/Models/PointOfSalePaymentState.swift

+                .cardPaymentSuccessful,
+                .acceptingCash,
+                .cashPaymentSuccessful:


Is the distinction between card and cash needed on the success state? If so, perhaps they should be distinguished using an associated value?

Should acceptingCash actually be a state here? Probably, but it's arguable.

Good question. Some of the presentation logic for the rest of the collection flow checks if .cardPaymentSuccessful, which won't make much sense as state when the order is paid successfully via cash. Since moving the cash view presentation to not rely on payment state I think right now we can get rid of .acceptingCash, but we still need to do something with .cardPaymentSuccessful.

I'll check both into using a new state, or as you mention making this a .paymentSuccessful state with associated value for cash or card 👍

joshheald · 2024-12-19T11:28:57Z

WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift

+    private var orderTotal: String? {
+        if case .loaded(let totals) = posModel.orderState {
+            return totals.orderTotal
+        }
+        return nil
+    }


Perhaps you can simplify this view by eliminating the potential for nil here?

If you required that totals gets passed in, then only allow this view to be presented when the orderState is loaded, you'd avoid it entirely.

Oh definitely, updated here: 714dfe5

joshheald · 2024-12-19T11:32:21Z

WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift

+            Button(action: {
+                Task { @MainActor in
+                    await markComplete()
+                }
+            }, label: {
+                HStack(spacing: Constants.buttonSpacing) {
+                    if isLoading {
+                        ProgressView()
+                            .progressViewStyle(CircularProgressViewStyle())
+                            .tint(Color.posPrimaryTextInverted)
+                    } else {
+                        Text(Localization.markPaymentCompletedButtonTitle)
+                            .font(Constants.buttonFont)
+                    }
+                }
+                .frame(maxWidth: .infinity)
+            })


Perhaps another place where an AsyncButton would be good? It'd need a binding so that the text field's onSubmit also triggered the progress view, but that seems like a useful option.

Agree! I'll make the change with #14602

joshheald · 2024-12-19T11:33:18Z

WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift

+            .padding(Constants.buttonPadding)
+            .frame(maxWidth: .infinity)
+            .foregroundColor(Color.posPrimaryTextInverted)
+            .background(Color.posOverlayFillInverted)
+            .cornerRadius(Constants.buttonCornerRadius)
+            .contentShape(Rectangle())


I'm guessing some of these are shared with other buttons? It'd be good to put them in a button style for sharing and to make the view easier to read.

Yes, we have an existing issue to make the async button a reusable component, I've logged it along: #14626

joshheald · 2024-12-19T11:34:43Z

WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift

+            comment: "Title of the cash payment navigation back button"
+        )
+        static let markPaymentCompletedButtonTitle = NSLocalizedString(
+            "pointOfSale.cashview.back.navigation.title",


This key is wrong 😬 copy-pasta from above

Oops! Fixed: 714dfe5

joshheald · 2024-12-19T11:38:37Z

WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift

+        )
+        static let markPaymentCompletedButtonTitle = NSLocalizedString(
+            "pointOfSale.cashview.back.navigation.title",
+            value: "Mark payment as complete",


This is a weird way to word it... I can see it's in the designs, but maybe it's worth discussing some more? It feels very programmery, I think the merchant action is more like Accept payment

Yeah, I'll raise it with Wagner along the navigation icons for each platform

joshheald · 2024-12-19T11:41:39Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                    switch posModel.paymentState {
+                    case .acceptingCash:
+                        PointOfSaleCollectCashView()
+                    default:
+                        TotalsView()
+                            .accessibilitySortPriority(2)
+                            .transition(.move(edge: .trailing))
+                    }


This feels like the wrong place for the views to be switched. The button to collect cash is on the TotalsView, it seems like that should also do the presentation, rather than bounce out to the dashboard.

I know TotalsView is big, but collecting cash still seems like part of its responsibilities, not the dashboard's.

Sounds good, I've moved this to be a modal presentation from Totals: 6eb26ef

Along with the usage of a chevron for the cash view when navigating back, we also update the receipt view with the same icon for consistency

iamgabrielma · 2024-12-20T05:17:52Z

Thanks for the review and pointers @joshheald , very much appreciated.

Is it supposed to be modal, or a push navigation? It looks and feels like it's supposed to be a fullscreen cover modal, but it's not totally clear because of the back button. Certainly fullscreen cover would be a closer match to the bottom sheet shown in Wagner's walkthrough

That's a good question, I think there's a bit of mix here, it looks like push navigation (from the Android demo this also seems to be pushed from the side) but then the design demo shows a modal. I've make the changes to present it from the totals view as a full screen modal and definitely feels much better and less prone to error.

The aggregate model exposes the source of truth for (card) payments, but it comes from the card payment service – you can't just change it. If you do, the card payment service will keep doing its thing and that makes the cash payment go away.

I wasn't feeling very good about this, and I guess the gut feeling was right 😅 I've removed the navigation interaction with the payment state for the time being. I've also updated the conditions when the button should be rendered so it only happens when the payment state is .idle for now. I'll need to test this further when playing with reader connection/cancellation:

Screen.Recording.2024-12-20.at.16.18.13.mov

that back arrow is very Androidy, and perhaps these designs are Android-first so need some translation. I think we should use a chevron.backward where we need similar for iOS

Yes, I've updated the arrow to a chevron (both in cash and receipt views) for consistency, I'll bring this up with design in any case, along with the string recommendations.

Changes:

✅ Present the view as a full-screen modal from totals
✅ Pass order totals values from the paren
✅ Updated navigation style to be more iOS
✅ Removed changes to payment state
✅ Added additional conditions for rendering the cash button

jaclync

I will finish the review with a physical card reader in a few hours, posting the initial review first.

In dark mode, iPad 10th gen iOS 18.0 simulator, I noticed some unexpected behavior from the cart view only when the feature flag is enabled as in the screencast below:

When navigating to the totals screen, the top bar in the cart view has a different background color
After dismissing the cash payment view, the cart view also gets shifted vertically briefly

This cart view behavior does not take place when the feature flag is disabled or in trunk.

Simulator.Screen.Recording.-.iPad.10th.generation.-.2024-12-23.at.10.55.53.mp4

Not sure if it's from some animation or some part of presentation of the cash payment view?

jaclync · 2024-12-23T03:02:47Z

WooCommerce/Classes/POS/Presentation/TotalsView.swift

+        .fullScreenCover(isPresented: $shouldShowCollectCashPayment) {
+            if case .loaded(let total) = posModel.orderState {
+                PointOfSaleCollectCashView(isVisible: $shouldShowCollectCashPayment,


❓ It looks like isVisible is only used for dismissing the PointOfSaleCollectCashView fullscreen cover, can the view be dismissed with the @Environment(\.dismiss) private var dismiss? If it's really needed, just a dismiss closure would be more clear than isVisible parameter since fullScreenCover(isPresented: $shouldShowCollectCashPayment) already uses the binding to present the view.

Ah good catch, that was my initial approach but didn’t work correctly as we were pushing navigation into the stack, dismissing the whole POS, but since we’ve moved to a modal presentation instead it's much simpler to use dismiss. Changed on 8d723fa

jaclync · 2024-12-23T03:04:21Z

WooCommerce/Classes/POS/ViewHelpers/CartViewHelper.swift

-        case .idle, .validatingOrderError, .acceptingCard:
+        case .idle,
+                .validatingOrderError,
+                .acceptingCard:


nit: it doesn't look like there are any changes, coulud be reverted.

Reverted! e656b3b

jaclync · 2024-12-23T03:05:32Z

WooCommerce/Classes/POS/Presentation/TotalsView.swift

@@ -291,6 +316,8 @@ private extension TotalsView {
    enum Constants {
        static let pricesIdealWidth: CGFloat = 382
        static let verticalSpacing: CGFloat = 56
+        static let buttonHeight: CGFloat = 56


nit: fixed button height might result in text being clipped with a large dynamic font size, and/or multiline text. If a fixed height is needed, @ScaledMetric can be considered.

I attempted to make the changes here but I found several other issues through the view when using dynamic font sizes, so I'll tackle these separately, as an example some additional space is starting to show up at the top of the view:

jaclync · 2024-12-23T03:09:22Z

WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift

+    @State private var isLoading: Bool = false
+    @State private var errorMessage: String?
+
+    @Binding var isVisible: Bool


nit: does this need to be public? (If this binding is still needed, from another comment in TotalsView)

No need! Removed since we're using dismiss now

jaclync

Just tested on a physical tablet and this step didn't work on my end:

Connect the reader and observe the button still renders. Tapping the button and navigating back and forth retains the payment state, so the reader should remain connected:

After connecting the reader, the Cash payment CTA is not shown anymore:

RPReplay_Final1734936422.MP4

This doesn't seem expected from the screencast for this step in the PR description?

When dismissing the cash view, the animation for cart seems to jump vertically. It seems to be resolved by adding the cash view to matched geometry

iamgabrielma · 2024-12-23T12:28:33Z

Thanks for the review and testing @jaclync !

When navigating to the totals screen, the top bar in the cart view has a different background color

I'm unable to replicate this, now in iOS17 nor 18 🤔 do you see it in the physical device as well?

After dismissing the cash payment view, the cart view also gets shifted vertically briefly

I see the same, the cart definitely seems to jump when dismissing. I fixed it on c8a00b2 by using the matching geometry. That said I'm seeing some oddness in the simulator that does not happen in physical device, I dropped a comment in slack here: p1734945310025859-C025A8VV728

After connecting the reader, the Cash payment CTA is not shown anymore [...] This doesn't seem expected from the screencast for this step in the PR description?

Good catch, yes and no. My fault as I didn't update the instructions: We don't want to retain the state anymore, but to cancel the payment intent if the operator decides to use cash (this will be implemented later), but you're right that the cash payment button should appear when we're accepting card but haven't tapped yet. I've addressed this here: 57a66b6

jaclync

The cash payment CTA is shown when the reader is ready now 👍

It's interesting I don't see the cart view header background color change in iOS 17.5.1 iPad device, only in one iOS 18.0 iPad 10th gen simulator. In another iOS 18.0 iPad Pro 11in simulator, the background color only changed briefly when the vertical move happened after dismissing the cash view 😅

Since this doesn't happen when the feature flag is disabled, we can try testing on an iOS 18 physical tablet to confirm outside of this PR. I can upgrade my iPad to iOS 18 if needed.

iOS 17.5.1 device	iOS 18.0 simulator

iamgabrielma · 2024-12-24T01:50:12Z

Since this doesn't happen when the feature flag is disabled, we can try testing on an iOS 18 physical tablet to confirm outside of this PR. I can upgrade my iPad to iOS 18 if needed.

Thanks for the screenshots! Let's upgrade either yours or mine so we still keep one in iOS17 for testing. I'll drop a follow-up in Slack 👍

mokagio · 2024-12-30T09:13:06Z

@iamgabrielma as you reported internally, p1734920797831869-slack-CC7L49W13, the prototype build fails. Buried. inthe logs, I found these compilation errors

❌ /opt/ci/builds/builder/automattic/woocommerce-ios/WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift:131:26: cannot find 'PointOfSalePreviewItemsController' in scope
        itemsController: PointOfSalePreviewItemsController(),
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
❌ /opt/ci/builds/builder/automattic/woocommerce-ios/WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift:132:36: cannot find 'CardPresentPaymentPreviewService' in scope
        cardPresentPaymentService: CardPresentPaymentPreviewService(),
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
❌ /opt/ci/builds/builder/automattic/woocommerce-ios/WooCommerce/Classes/POS/Presentation/PointOfSaleCollectCashView.swift:133:26: cannot find 'PointOfSalePreviewOrderController' in scope
        orderController: PointOfSalePreviewOrderController())
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

https://buildkite.com/automattic/woocommerce-ios/builds/27034#0193f136-9e0e-4804-ae58-1c83a6164959/735-8363

…into task/14603-add-cash-option-to-totals-view

wpmobilebot · 2025-01-06T02:18:29Z

📲 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	pr14724-4e5a727
	Version	21.3
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`4e5a727`
	App Center Build	WooCommerce - Prototype Builds #12349

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

Render cash button and navigate to cash view

64707e8

iamgabrielma added type: task An internally driven task. feature: POS labels Dec 18, 2024

iamgabrielma added 2 commits December 19, 2024 15:51

render totals in cash view navigation

0061a43

move strings to extension

ab37226

iamgabrielma added this to the 21.4 milestone Dec 19, 2024

update button style

6144cf7

iamgabrielma marked this pull request as ready for review December 19, 2024 10:22

iamgabrielma requested review from jaclync and joshheald December 19, 2024 10:23

joshheald self-assigned this Dec 19, 2024

joshheald reviewed Dec 19, 2024

View reviewed changes

iamgabrielma added 4 commits December 20, 2024 10:53

Make cash view modal presentation from totals

6eb26ef

Pass order total to cash view from parent

714dfe5

update backbutton icon on receipt for consistency

c9178c0

Along with the usage of a chevron for the cash view when navigating back, we also update the receipt view with the same icon for consistency

Revert changes to payment state for cash navigation

71547ba

This was referenced Dec 20, 2024

[Woo POS] [Cash & Receipts] Handle payment intent cancellation when navigating to cash #14682

Open

[Woo POS] [Cash & Receipts] Handle order and flow when marked as completed via cash #14602

Open

Render button only when payment state idle

5f9bc07

iamgabrielma requested a review from joshheald December 20, 2024 05:18

Merge branch 'trunk' into task/14603-add-cash-option-to-totals-view

c5923ef

jaclync self-assigned this Dec 23, 2024

jaclync reviewed Dec 23, 2024

View reviewed changes

iamgabrielma added 4 commits December 23, 2024 16:18

revert styling

e656b3b

Update cash view to dismiss via environment

8d723fa

Resolved animation oddness

c8a00b2

When dismissing the cash view, the animation for cart seems to jump vertically. It seems to be resolved by adding the cash view to matched geometry

Show button when card is acceptingCard state

57a66b6

iamgabrielma requested a review from jaclync December 23, 2024 12:29

jaclync approved these changes Dec 24, 2024

View reviewed changes

iamgabrielma added 2 commits January 6, 2025 08:11

Merge branch 'trunk' of https://github.com/woocommerce/woocommerce-ios …

b840317

…into task/14603-add-cash-option-to-totals-view

Wrap preview in debug precompiler flag

4e5a727

iamgabrielma merged commit 7f796e4 into trunk Jan 6, 2025
12 checks passed

iamgabrielma deleted the task/14603-add-cash-option-to-totals-view branch January 6, 2025 02:45

[Woo POS][Cash & Receipts] Render cash button and navigate to cash view #14724

[Woo POS][Cash & Receipts] Render cash button and navigate to cash view #14724

Conversation

iamgabrielma commented Dec 18, 2024 • edited by jaclync Loading

Description

Changes

Testing

Screenshots

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

iamgabrielma commented Dec 19, 2024

joshheald left a comment

Choose a reason for hiding this comment

Transition weirdness

Payment state

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

joshheald Dec 19, 2024 • 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

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

iamgabrielma commented Dec 20, 2024 • edited Loading

jaclync left a 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

jaclync left a comment

Choose a reason for hiding this comment

iamgabrielma commented Dec 23, 2024 • edited Loading

jaclync left a comment

Choose a reason for hiding this comment

iamgabrielma commented Dec 24, 2024

mokagio commented Dec 30, 2024

wpmobilebot commented Jan 6, 2025

iamgabrielma commented Dec 18, 2024 •

edited by jaclync

Loading

joshheald Dec 19, 2024 •

edited

Loading

iamgabrielma commented Dec 20, 2024 •

edited

Loading

iamgabrielma commented Dec 23, 2024 •

edited

Loading