[Experiment] Card payments facade/adaptors with UI #12760

joshheald · 2024-05-16T17:22:33Z

Description

Builds on #12735 to add the onboarding and payments UI.

This adapts and reuses the existing payments code and, as much as possible, the UI.

Onboarding UI is presented without the UIKit wrapper used in the existing app.

Connection and Payments UI is reimplemented, but relies on wrapped versions of the existing view models. The wrappers only exist to make the VMs identifiable, and otherwise wouldn't be required.

TODOs

These reimplemented UIs don't have buttons yet, so you can't go all the way through the flow unless it wouldn't require a button tap.

At the moment, we're using a fake order so the payment doesn't actually work.

I'll work on those tomorrow to fully prove the concept.

Screenshots

look.ma.no.buttons.mp4

At this point, I think the adaptor could be used as a singleton to give us one source of truth for card reader connections. I’ve cut some corners in the POC and this still doesn’t totally prove that we can avoid UIKit use, but I _think_ the principle holds.

Note that the `showOnboarding` function is not yet called, as the AlertsHandler doesn’t look like the best API. I’m going to turn it into streams

# Conflicts: # WooCommerce/Classes/ViewRelated/Hub Menu/HubMenu.swift

dangermattic · 2024-05-16T17:23:05Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

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

Generated by 🚫 Danger

iamgabrielma · 2024-05-17T02:48:19Z

WooCommerce/Classes/POS/Presentation/PointOfSalePaymentsTestView.swift

+
+class PointOfSalePaymentsTestViewModel: ObservableObject {
+    let cardPresentPayments: CardPresentPayments
+    var cancellables = Set<AnyCancellable>()


cancellables is not currently being used, in case that you see any odd behaviour with the expected subscription lifecycle

iamgabrielma · 2024-05-17T03:00:26Z

WooCommerce/Classes/ViewModels/CardPresentPayments/CardPresentPaymentsModalViewModel.swift

@@ -2,13 +2,12 @@ import UIKit

 /// Abstracts configuration and contents of the modal screens presented
 /// during operations related to Card Present Payments
-protocol CardPresentPaymentsModalViewModel {
+typealias CardPresentPaymentsModalViewModel = CardPresentPaymentsModalContent & CardPresentPaymentsModalActions


💯 makes more sense to group the protocols like this and allows us to start decoupling from UIKit

Yeah. I'm not sure exactly how to handle the actions yet, but I'll probably add another actions protocol and make the existing view models implement it without using the viewController they otherwise get passed. We'll see.

iamgabrielma · 2024-05-17T03:17:15Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+
+/// This is really a re-implementation of the CardPresentPaymentsOnboardingPresenter, as it needs to take the calls to `showOnboardingIfRequired` and
+/// route the output to a SwiftUI view for display, rather than directly displaying on the viewController that's passed in.
+final class CardPresentPaymentsOnboardingPresenterAdaptor: CardPresentPaymentsOnboardingPresenting {


From what I understand this adapter is just a copy of the existing CardPresentPaymentsOnboardingPresenter to be used through our SwiftUI implementation, it just ignores the usage of the UIViewController property within showOnboardingIfRequired(:), that we need to declare to conform to the protocol, right?

Yep pretty much.

It replaces the use of that UIViewController property with use of its onboardingScreenViewModelPublisher to emit the UI events for our new code to display.

In the older implementation, the CardPresentPaymentsOnboardingPresenter has the responsibility for actually presenting the UI, which this adaptor doesn't.

iamgabrielma · 2024-05-17T03:22:49Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+            return await withTaskCancellationHandler {
+                return await withCheckedContinuation { continuation in
+                    orderPaymentUseCase.collectPayment(using: discoveryMethod) { error in
+                        // TODO: even though we have a tri-state result type, perhaps we should throw these errors.


I had a similar question when checking how we handle the collectPayment async result task within PointOfSaleDashboardView. Should we throw errors on .failure, or should we continue handling it as one of the result types of the collect payment state?

I think trying it out and seeing what's nicer at the callsite is worthwhile on these calls... I'm not going to do it in the experiment but it would be nice to figure out our preference and be consistent.

iamgabrielma · 2024-05-17T03:36:11Z

Looking great. Any idea where we should handle the differences between app and pos regarding, for example, reader types, supported country, supported plugins, etc, ... ? Perhaps this should be done in the adaptor, where we can initiate or exit/cancel the process early based on eligibility/limitations?

iamgabrielma · 2024-05-17T07:30:36Z

WooCommerce/Classes/POS/Presentation/PointOfSalePaymentsTestView.swift

+        self.cardPresentPayments = CardPresentPaymentsAdaptor(siteID: siteID)
+        cardPresentPayments.paymentScreenEventPublisher
+            .print("🅿️ payment event")
+            .map { event -> InPersonPaymentsViewModel? in


Are we creating a new instance of InPersonPaymentsViewModel every time that this event happens and is passed as an associated type? Would it be any concern if that's the case? I don't see anything that could cause troubles but I'm wondering: If it's a new instance we have to be careful when observing changes, or losing the reference may imply stop listening to changes.

We are, and I'm not 100% sure if it's a concern yet... I don't think it is though.

It's poorly named – InPersonPaymentsViewModel is essentially OnboardingScreenViewModel – so it should be pretty simple, short lived, and safe to make new ones when the onboarding screen changes.

joshheald · 2024-05-17T09:42:59Z

Any idea where we should handle the differences between app and pos regarding, for example, reader types, supported country, supported plugins, etc, ... ? Perhaps this should be done in the adaptor, where we can initiate or exit/cancel the process early based on eligibility/limitations?

No, I don't think it should be in the adaptor – ideally we'll be able to use the adaptor from both the app and pos code that needs to take payments, and make them both use SwiftUI so we can improve the app uses as well. That's all well outside POS MVP scope, and subject to whether we keep the app uses of card payments. But if we do it, refactoring the payments code in future becomes way easier.

The adaptor's job is adapting from the UIKit-focus and store/action usage of the existing code, to a SwiftUI and async/await friendly API for card present payments.

So we should handle the differences elsewhere. The gate between the app and POS experiences is one option. Another is the code which actually shows the UI and uses the adaptor (PointOfSalePaymentsTestView and ViewModel in this PR) – e.g. not showing the Enable Pay in Person onboarding step might be done here.

Another way is to have two different CardPresentPaymentsConfiguration for each country, one for POS and one for App. These could be passed in to collectPayment and used accordingly – though that may need some validation of whether it would result in an unsupported reader being disconnected when we move from App to POS, for example. (From our discussion the other day though, we probably don't actually need to do that, it'll just work, so let's allow the other readers if they're supported.)

wpmobilebot · 2024-05-17T12:25:28Z

📲 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	pr12760-ff82cc7
	Version	18.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`ff82cc7`
	App Center Build	WooCommerce - Prototype Builds #9153

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

Using the NullObject pattern for the RootViewController dependency prevents some strange background UI and reduces the risk of errors

jaclync

Thanks a lot @joshheald for digging into all different parts of the payments flow, and coming up with a solution for us to reuse the UI (not the UX for now as it looks like) with minimal changes to the existing code! LGTM to proceed as our POS payment-related service 👍

Left some questions marked with ❓ but mostly are just for clarifying. I'm slightly concerned about all the nil rootViewController cases that present some webview or other UI that doesn't trigger the delegate/callback toward the bottom of the PR diffs. Plus potentially missing some nil cases, but they should only affect the POS usage and I'm sure we can figure it out.

jaclync · 2024-05-23T02:45:25Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+    /// The adaptor/facade is intended to be created once when we switch stores, and to be a singleton.
+    /// We might as well make a new one when we switch stores though.
+    init(siteID: Int64)


❓ might an initializer requirement in the protocol prevent us from passing other dependencies if needed in the future?

Yeah, agreed. I think this was just here so I could copy a simple representation to the P2 post, it's not something we need in the real protocol when we go further, AFAIK.

jaclync · 2024-05-23T02:48:51Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+    /// `paymentScreenEventPublisher` provides a stream of events relating to a payment, including their view models,
+    /// for subscribers to display to the user. e.g. onboarding screens, connection progress, payment progress, card reader messages
+    /// We may want separate streams for these depending on the way we consume it
+    var paymentScreenEventPublisher: AnyPublisher<CardPresentPaymentEvent?, Never> { get }


Nice having a publisher to emit all events that we want to present UI/UX in the payment flow 💯 If we want to add support for just the card reader connection flow (in the POC pfoUAQ-HJ-p2), can we add a similar publisher like readerConnectionEventPublisher?

❓ What does a nil value in the stream mean, what's the recommended way for the observer to handle it? It might be worth mentioning in the comment if the value needs to be optional.

In this example, which is closely based on the existing UI/UX, nil means "don't show any card payment UI", which I think we need to be able to do to remove modals which are finished with (e.g. when cancelling, or when someone taps the last button after a payment.)

That's a good question though, because it's not immediately clear what that would mean if the UI were more ambient, showing the progress in a corner of the screen or similar. In that case, we'd probably always want a state.

Perhaps an idle event is required here...

jaclync · 2024-05-23T02:50:32Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+    ///   - discoveryMethod: Allows specifying Tap to Pay or external reader. For POS, this would default to external.
+    /// - Returns: `CardPresentPaymentResult` for a success, failure, or cancellation.
+    func collectPayment(for order: Order,
+                        using discoveryMethod: CardReaderDiscoveryMethod) async -> CardPresentPaymentResult


Really great being able to use async/await 👍

jaclync · 2024-05-23T03:08:15Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+    /// `paymentScreenEventPublisher` provides a stream of events relating to a payment, including their view models,
+    /// for subscribers to display to the user.
+    /// This is long lived, and used to display UI, not to communicate the success or failure of a particular payment
+    // TODO: figure out whether there's any reason to use a Combine publisher or AsyncStream here.


I haven't used AsyncStream nor seen existing usage in the app, but it could be nice to try if it fits our use case. My understanding of the major advantages of using Combine are: being able to do all kinds of operations (mapping, merging, combining, etc.), and being compatible with what we've been using in the app. Some considerations might be how to cancel and achieve the merging behavior like in paymentScreenEventPublisher's current implementation.

jaclync · 2024-05-23T03:09:43Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+    // TODO: decide whether we actually want to expose this, or just internally assign `paymentScreenEventSubject` to each short-lived stream instead.
+    var paymentScreenEventPublisher: AnyPublisher<CardPresentPaymentEvent?, Never>
+
+    private let currencyFormatter: CurrencyFormatter = CurrencyFormatter(currencySettings: ServiceLocator.currencySettings)


nit: this could be a reason to move the initializer out of the protocol to enable dependencies other than the site ID, unless there's some reason to include the initializer. CurrencySettings could be DI'ed for unit testing.

jaclync · 2024-05-23T04:03:01Z

WooCommerce/Classes/CardPresentPayments/CardPaymentsAdaptor.swift

+                        case .ready = readiness else {
+                    return
+                }
+                // TODO: perhaps we should have a more specific "success" event here.


+1 to emit a specific "onboarding success/ready" event.

jaclync · 2024-05-23T04:19:50Z

WooCommerce/Classes/POS/Presentation/PointOfSalePaymentsTestView.swift

+    let cardPresentPayments: CardPresentPayments
+    var cancellables = Set<AnyCancellable>()
+
+    @Published var onboardingViewModels: InPersonPaymentsViewModel?


nit: just to match the value type

Suggested change

@Published var onboardingViewModels: InPersonPaymentsViewModel?

@Published var onboardingViewModel: InPersonPaymentsViewModel?

jaclync · 2024-05-23T04:21:49Z

WooCommerce/Classes/ViewModels/CardPresentPayments/CardPresentModalBluetoothRequired.swift

@@ -48,7 +48,11 @@ final class CardPresentModalBluetoothRequired: CardPresentPaymentsModalViewModel
    }

    func didTapSecondaryButton(in viewController: UIViewController?) {
-        viewController?.dismiss(animated: true, completion: {[weak self] in
+        guard let viewController else {
+            return primaryAction()


❓ just wanted to make sure, the dismiss isn't needed before calling primaryAction()?

In the case where a UIKit call is made for this, it probably is... but we can't dismiss, if we don't have a viewController...

The other way I'm tempted to improve this is to update the CardPresentPaymentsModalViewModel protocol to make it handle both SwiftUI and UIKit, by providing two handlers for each button, one which requires a UIViewController (for UIKit) and one which doesn't accept one (for SwiftUI).

jaclync · 2024-05-23T04:25:35Z

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

@@ -601,7 +604,7 @@ private extension CollectOrderPaymentUseCase {
        let noticePresenter = DefaultNoticePresenter()
        let notice = Notice(title: Localization.failedReceiptPrintNoticeText,
                                    feedbackType: .error)
-        noticePresenter.presentingViewController = rootViewController
+        noticePresenter.presentingViewController = rootViewController as? UIViewController


nit ❓ : does the nil case get to show the notice, or an equivalent error event is emitted?

Same question about the nil UIViewController in presentEmailForm.

I didn't handle receipts in this POC, because I don't think there's any need for us to use the existing code for it, and it's still in question whether they're in scope for MVP.

It's kind of tacked on to the end of the payment code... so this never gets called without a real UIViewController.

We might be able to make the Presenting protocol handle this a bit more gracefully.

jaclync · 2024-05-23T04:29:09Z

WooCommerce/Classes/ViewRelated/CardPresentPayments/CardReaderConnectionController.swift

+    // TODO: The use of UIViewController here is pretty problematic when using it from SwiftUI,
+    // we need an alternative when it gets consumed through the CardPresentPaymentsAdaptor


+100, I encountered this when trying to understand all the card reader connection UI states in #12723. It also looks like other views have a similar problem with the web view coupled with the business logic and not abstracted out. 😅

I can't figure out a way to adapt these without changing the underlying implementation at all right now.

We should be able to ignore most of these webview launching handlers, as it's an implementation detail of the UIKit view model which we can do separately in SwiftUI... but I think we need this one to work pretty soon, because it's essentially an onboarding issue, but handled during connection.

It feels like much of this needs to be moved to the AlertProvider, where we can inject a different one for use from SwiftUI, but the else case would need to be handled as that changes the alert shown in the connection controller.

malinajirka · 2024-07-16T15:40:57Z

Closing as this PR is not active anymore.

joshheald added 16 commits May 14, 2024 17:19

WIP card payments adaptor - async

d92d714

Remove incorrect eventstream

ec38af8

adapt payment alerts to remove UIKit dependencies

3140510

Add onboarding presenter adaptor to show SwiftUI screens

78fd151

Note that the `showOnboarding` function is not yet called, as the AlertsHandler doesn’t look like the best API. I’m going to turn it into streams

Use a stream to provide payment events to callers

e43476b

Add protocol for facade, with doccomments

e5b5387

Add basic cancellation support

fc542cb

Show onboarding from a test view on POS screen

c7ef1ef

Add note about onboarding

302a8bf

Merge branch 'trunk' into experiment/card-payments-adaptor-with-ui

33a7149

# Conflicts: # WooCommerce/Classes/ViewRelated/Hub Menu/HubMenu.swift

Move test view to new POS folder

f52dae4

Split existing modal protocol to content/actions

eef366a

Directly pass the content for display in an alert

34278c2

Show payment alerts modally on new view

a2ef1bf

Put test payment flow on dashboard view

f62654e

joshheald added status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: POS labels May 16, 2024

iamgabrielma reviewed May 17, 2024

View reviewed changes

joshheald added 4 commits May 17, 2024 11:36

Add no-UIViewController handler to payments alerts

5e028bf

Use a real order for test payments

0783315

Show buttons on connection and payment modals

7a09e21

dismiss payment modal on completion

fdddbf4

joshheald added 5 commits May 20, 2024 15:26

Fix accidental scheme change commit

21b3583

Fix glitchy UI due to UIViewController dependency

01d599e

Using the NullObject pattern for the RootViewController dependency prevents some strange background UI and reduces the risk of errors

Update comments now view controller not required

a1b954b

Add notes

6cd9120

Prevent multiple-continuation of async task

ff82cc7

jaclync reviewed May 23, 2024

View reviewed changes

malinajirka closed this Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Experiment] Card payments facade/adaptors with UI #12760

[Experiment] Card payments facade/adaptors with UI #12760

joshheald commented May 16, 2024

dangermattic commented May 16, 2024

iamgabrielma May 17, 2024

joshheald May 17, 2024

iamgabrielma May 17, 2024 •

edited

Loading

joshheald May 17, 2024

iamgabrielma May 17, 2024

joshheald May 17, 2024

iamgabrielma May 17, 2024

joshheald May 17, 2024

iamgabrielma commented May 17, 2024

iamgabrielma May 17, 2024

joshheald May 17, 2024

joshheald commented May 17, 2024

wpmobilebot commented May 17, 2024 •

edited

Loading

jaclync left a comment

jaclync May 23, 2024

joshheald May 28, 2024

jaclync May 23, 2024

jaclync May 23, 2024

joshheald May 28, 2024

jaclync May 23, 2024

jaclync May 23, 2024

jaclync May 23, 2024

jaclync May 23, 2024

jaclync May 23, 2024

jaclync May 23, 2024

joshheald May 28, 2024

jaclync May 23, 2024

joshheald May 28, 2024 •

edited

Loading

jaclync May 23, 2024

joshheald May 28, 2024

malinajirka commented Jul 16, 2024

	@Published var onboardingViewModels: InPersonPaymentsViewModel?
	@Published var onboardingViewModel: InPersonPaymentsViewModel?

		// TODO: The use of UIViewController here is pretty problematic when using it from SwiftUI,
		// we need an alternative when it gets consumed through the CardPresentPaymentsAdaptor

[Experiment] Card payments facade/adaptors with UI #12760

[Experiment] Card payments facade/adaptors with UI #12760

Conversation

joshheald commented May 16, 2024

Description

TODOs

Screenshots

dangermattic commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamgabrielma May 17, 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

iamgabrielma commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshheald commented May 17, 2024

wpmobilebot commented May 17, 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

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

joshheald May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malinajirka commented Jul 16, 2024

iamgabrielma May 17, 2024 •

edited

Loading

wpmobilebot commented May 17, 2024 •

edited

Loading

joshheald May 28, 2024 •

edited

Loading