[Experiment] Card payments adaptor stream #12736
Conversation
joshheald
added
status: do not merge
|
|
Generated by 🚫 Danger |
| import Yosemite | ||
| import class WooFoundation.CurrencyFormatter | ||
|
|
||
| class CardPresentPaymentsAdaptor { |
There was a problem hiding this comment.
💬 Payments newbie questions (I've edited it after understanding more about the changes):
- Is this meant to be a singleton in the app for any card-present payments-related actions and states, including onboarding, reader connection, and payment collection from any entry point? While looking at the reader connection code, the Menu Payments setup seems different from the Orders tab. This adapter solves the problem with one single implementation that maintains various states, with
posbeing a new entry point. - For reuse in the current POS framework, do you think it makes sense to have a protocol for this in Yosemite (given that we don't have plans to create new frameworks) so that the singleton uses the interface? It might help with unit testing as well.
There was a problem hiding this comment.
Is this meant to be a singleton in the app for any card-present payments-related actions and states, including onboarding, reader connection, and payment collection from any entry point? [...] This adapter solves the problem with one single implementation that maintains various states, with pos being a new entry point.
Yes, exactly.
We may also want to add a separate connection function from the same entry point, but it didn't seem essential to this experiment.
In the app right now, the vast majority of people connect the reader during payment.
While looking at the reader connection code, the Menu Payments setup seems different from the Orders tab.
Yeah, the menu setup screen is a bit different, but it does use the connection controllers. It would be better if we had a single source of truth for these controllers, which we could add via this adaptor in the MVP or beyond, but it's not essential.
For reuse in the current POS framework, do you think it makes sense to have a protocol for this in Yosemite (given that we don't have plans to create new frameworks) so that the singleton uses the interface? It might help with unit testing as well.
I've written this in the App framework, initially thinking about the situation of POS being in the App framework and removing the POS framework.
If we do continue with the POS framework, we have to make some choices if we use this approach...
We certainly could move the code that this depends on to Yosemite, but the singleton itself would need to live there too; using a shared interface wouldn't get us the code reuse benefits of the adaptor.
Alternatively, we could copy all the card reader code, and this adaptor, from the App framework to the POS framework, and build it out from there. That might let us improve on a "fork" (sort of) of the card reader code for the POS, which we could then merge back to the app target, or move down to Yosemite for sharing.
| self.siteID = siteID | ||
| } | ||
|
|
||
| /// The problem with this approach is that making the usecase also makes the preflight controller, which makes the two connection controllers. |
There was a problem hiding this comment.
What are the reasons this adaptor can't keep the 2 connection controllers and then pass them in a preflight controller like in https://github.com/woocommerce/woocommerce-ios/pull/12735/files#diff-2c42212ff82dbd5a932472da2fdfa48bf6af9ca77d78c4d5ea7ad3985b47632cR53?
There was a problem hiding this comment.
Because the connection controllers would also need to have the eventAdaptor, which has to be short-lived as it is tied to the same lifecycle as the stream.
Perhaps we could have a long-lived eventAdaptor which could be given a new streamContinuation whenever a new payment started. That seems like it would be quite error-prone, as streams may not be finished before they're replaced, and if we use streams I want to make it more difficult to misuse them than they are currently.
| } | ||
| } | ||
|
|
||
| enum CardPresentPaymentEvent { |
There was a problem hiding this comment.
My guess is that this is for the consumer of this adaptor to have any UI implementation / side tasks for these cases?
There was a problem hiding this comment.
Right now, yes.
I'm going to experiment with adapting the existing UI to be presentable without compromises in SwiftUI, but I'm not particularly hopeful about that.
|
Closing as this PR is not active anymore. |
Part of: #12734
Description
This is another (incomplete) take on #12735, but using a stream for outputting the events.
There are issues with this approach, particularly because the stream's lifecycle should probably match the lifecycle of the
collectPaymentcall, and everything that does. However, I struggled to make the connection controllers long-lived under those circumstances.It's probably still possible, so I thought I'd share this as another similar solution for thoughts... but #12735 is more promising right now, I think.
CC. @iamgabrielma @bozidarsevo @jaclync