[Experiment] Card payments adaptor #12735
Conversation
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.
joshheald
added
the
status: do not merge
|
|
Generated by 🚫 Danger |
| } | ||
|
|
||
| func collectPayment(for order: Order, | ||
| using discoveryMethod: CardReaderDiscoveryMethod) async -> CardPresentPaymentResult { |
There was a problem hiding this comment.
Would it make sense to switch over a result type here?
| using discoveryMethod: CardReaderDiscoveryMethod) async -> CardPresentPaymentResult { | |
| using discoveryMethod: CardReaderDiscoveryMethod) async -> Result<Order, CardPresentPaymentError> { |
The enum can be simplified and moved away as well:
- enum CardPresentPaymentResult {
- case success(Order)
+ enum CardPresentPaymentError: Error {
case failure(CardPaymentErrorProtocol)
case cancellation
}
And the continuation would also be clearer:
- continuation.resume(returning: .cancellation)
+ continuation.resume(returning: .failure(.cancellation))There was a problem hiding this comment.
We could, and I probably wouldn't even pick it up at review if someone did that... but personally I don't really like to use Result for tri-states like this.
Cancellation isn't an error or failure, it's a user action and we're doing what they want us to. I think that making it a failure tends to confuse the call site, as you have to unwrap the errors and check whether they were actually cancellations before you handle them.
| } | ||
| } | ||
|
|
||
| extension CardPresentPaymentsAdaptor: CardPresentPaymentsOnboardingPresenting, CardPresentPaymentAlertsPresenting { |
There was a problem hiding this comment.
Will the adaptor also split responsibilities here along *_OnboardingPresenting and *_AlertsPresenting? Or we keep all in one?
There was a problem hiding this comment.
I don't have a strong feeling on splitting them. We should do it if we found that it made it easier to use at the call site, e.g. because the SwiftUI view modifiers we use to present each were different.
For the purposes of the experiment, it doesn't make a lot of difference though.
| // In here we still need to handle the button actions wrt the UIViewControllers the closures are passed. | ||
| // That said, very few of the alerts actually use the UIVCs, so it might be just as easy to remove the dependency from both sides |
There was a problem hiding this comment.
We should also update (or remove) var image: UIImage { get } property since uses a specific UIKit image type. The auxiliaryAttributedButtonTitle property is not a problem for SwiftUI, but it's used only once through all modals, so maybe is worth take a look and see if we should remove it as well for simplicity.
There was a problem hiding this comment.
Yeah, I'm not totally sure how much we want to carry across from the CardPresentPaymentsModalViewModel here, but the nice thing is that we don't need to include the image if we don't want to. We can handle that however we choose within the consuming code – perhaps the image is the same for all of them, perhaps we determine it based on the title, or we add an enum somewhere for translating (back?) to a more declarative type of output.
| func showOnboardingIfRequired(from: UIViewController, readyToCollectPayment: @escaping () -> Void) { | ||
| paymentsAlertHandler?.showOnboarding() | ||
| } |
There was a problem hiding this comment.
actually try to use it from a view tomorrow.
Not sure if you've already thought/solved this, but this function would still require to pass a UIViewController in, keeping UIKit as dependency. How should we go about it? Some sort of extension to the UIViewController?
| func showOnboardingIfRequired(from: UIViewController, readyToCollectPayment: @escaping () -> Void) { | |
| paymentsAlertHandler?.showOnboarding() | |
| } | |
| extension UIViewController: Presenter { } | |
| protocol Presenter { | |
| func present(_ viewControllerToPresent: UIViewController, animated: Bool, completion: (() -> Void)?) | |
| var navigationController: UINavigationController? { get } | |
| func show(_ vc: UIViewController, sender: Any?) | |
| } | |
| func showOnboardingIfRequired(from: Presenter, readyToCollectPayment: @escaping () -> Void) { | |
| paymentsAlertHandler?.showOnboarding() | |
| } |
Problem is that we'll have troubles in SwiftUI when handling this method, perhaps by using generics for the view adapter and a hosting controller? 🤔
There was a problem hiding this comment.
this function would still require to pass a UIViewController in, keeping UIKit as dependency. How should we go about it? Some sort of extension to the UIViewController?
The UIViewController gets passed in from the existing Card Payments (UIKit-heavy) code that we're adapting... and then we ignore it, so there's no need to pass one in from SwiftUI.
This is the adaptor impleementing the OnboardingPresenter protocol, for the benefit of the existing payments code, and then translating (adapting) the showOnboardingIfRequired call to a more SwiftUI friendly handler. That paymentsAlertHandler is what gets passed in from SwiftUI, and it doesn't know about the UIViewController.
There was a problem hiding this comment.
still require to pass a
UIViewControllerin, keeping UIKit as dependency. How should we go about it? Some sort of extension to the UIViewController?
PS – this is exactly what I intend to do for rootViewController, so that I can pass in an adaptor implementation instead of UIViewController() as I'm doing at the moment. It may be that we can just ignore whatever calls we get on rootViewController when using it via the adaptor, but if we can't, this is the kind of approach I'd take.
| let preflightController = await CardPresentPaymentPreflightController( | ||
| siteID: siteID, | ||
| configuration: CardPresentConfigurationLoader().configuration, | ||
| rootViewController: UIViewController(), |
There was a problem hiding this comment.
just for my learning, it looks like we can just pass any view controller here and in CollectOrderPaymentUseCase, does that mean this rootViewController parameter isn't really necessary?
There was a problem hiding this comment.
At the moment it is used, by the existing code. I'm only passing this as a temporary measure so that I could work on onboarding, etc. Here's the list where it's used at the moment:
CardPresentPaymentAlertsPresenterpresent(present, animated: false)indismissSeveralFoundAndPresentanddismissCommonAndPresent- these are used to show the UIKit payment alerts, so in our adapted case we won't need to use them because we'll publish the view models for the caller to display those alerts.
CollectOrderPaymentUseCaseself.rootViewController.presentedViewController?.dismiss()in failure handler forattemptPaymentincollectPayment.- I think that by managing our own display of alerts in the caller, we can avoid this need too
presentused in receipts- Receipts – I'm not tackling these head on at the moment, because they're really something that comes after the payment flow.
- set as
noticePresenter.presentingViewControllerwhen receipt printing fails
CardPresentPreflightControlleronboardingPresenter.showOnboardingIfRequired(from: rootViewController)–- safe to ignore since we don't use it in our implementation of the adaptor for onboarding
So all in all, from the adaptor we may (end up) being able to get away without a VC here.
I'd do that by replacing the requirement for a UIViewController, with a requirement for a protocol with the above methods, and then pass in a null object from the adaptor so we don't end up with warnings or UIKit getting confused about a VC outside the view hierarchy being used for presentation.
| } | ||
| } | ||
|
|
||
| extension CardPresentPaymentsAdaptor: CardPresentPaymentsOnboardingPresenting, CardPresentPaymentAlertsPresenting { |
There was a problem hiding this comment.
Might be a naive question so feel free to correct: I think it'd be nice if the consumer only knows about the payments/reader-related interface like collectPayment(for:using:), and not the functions/variables that come with these protocol conformance. We can also create a protocol for the adaptor to address this if possible.
There was a problem hiding this comment.
Yep, agreed. I didn't do it initially in the experiment but then did it to answer a P2 question, and it makes it clearer what the consumer should know about.
Having the adaptor implement a bunch of other protocols to do its work makes it harder to have clarity there, but the protocol helps.
|
Closing as this PR is not active anymore. |
Part of: #12734
Description
This is WIP.
I've wrapped all the existing payments code in an adaptor, and provided an interface to start a payment (only.)
Note that this is in the App target, and intended to be used from there. Using it from the POS target would take extra work, which may, or may not, be worthwhile.
The output alerts/onboarding can be handled by providing a class that conforms to
CardPresentPaymentsAlertHandling. Not all this UI would need to be new, we could reuse the existing SwiftUI screens but present them differently. The idea is that is the most that would be new.The adaptor is intended to perhaps be used as a singleton, or at least be as long lived as a particular store is selected. That way, the same connection controllers are used every time we take a payment (through the adaptor), and we should have just one source of truth for that.
I'm going to continue working on this and actually try to use it from a view tomorrow. This isn't the only possible approach, and nothing's definite, but I'd love your thoughts on it so far.
@bozidarsevo @jaclync @iamgabrielma