-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TTPReq] Integrate Education steps after Terms of Service are accepted #14580
[TTPReq] Integrate Education steps after Terms of Service are accepted #14580
Conversation
…tate Parts of Tap to Pay onboarding are presented by iOS, such as Terms of Service acceptance view. We need to communicate these state changes to main parts of Woo app.
Implement Stripe LocalMobileReaderDelegate localMobileReaderDidAcceptTermsOfService method and emit didAcceptTermsOfService event through a builtInReaderOnboardingState PassthroughSubject. It allows users of the service to subscribe to onboarding events and react when terms of service conditions are accepted.
Creating a separate presentation method since we want to present merchant education separately from connection modals. Present TapToPayEducationViewViewHostingController on top of existing modal view controller.
- Observe onboarding state - When terms of service are accepted, present merchant education - Delay finalizing connection flow while merchant education is ongoing to prevent further actions such as payment from proceeding
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial questions, I will finish manual testing after that!
@@ -0,0 +1,5 @@ | |||
/// A type that represents the possible states of the built-in (Tap to Pay) card reader onboarding flow. | |||
public enum BuiltInCardReaderOnboardingState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking question: do we plan to add more cases to the onboarding state? If not, I wonder if emitting Void
from the publisher (like var builtInCardReaderAcceptToSEvents: AnyPublisher<Void, Never> { get }
) on ToS acceptance event is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't plan to add any more cases. I created it following the logic of other similar states. However, builtInCardReaderAcceptToSEvents
is a good idea and would avoid creating this additional enum
.
@@ -97,4 +97,8 @@ final class CardPresentPaymentsAlertPresenterAdaptor: CardPresentPaymentAlertsPr | |||
func reset() { | |||
latestReaderConnectionHandler = nil | |||
} | |||
|
|||
func presentMerchantEducation(completion: @escaping () -> Void) { | |||
completion() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's not presented, maybe because TTP isn't supported in POS? Maybe it's worth a clarifying comment that this will be updated when TTP is supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it wouldn't be presented for POS, unless we start supporting iPhones or collecting payments through TTP. That comes back to your later question about putting presentMerchantEducation
in a presenter or choosing a different approach.
guard let self = self else { return } | ||
|
||
onboardingEvents | ||
.subscribe(on: DispatchQueue.main) | ||
.sink { [weak self] event in | ||
guard let self = self else { return } | ||
switch event { | ||
case .didAcceptTermsOfService: | ||
self.isMerchantEducationInProgress.send(true) | ||
self.alertsPresenter.presentMerchantEducation { [weak self] in | ||
self?.isMerchantEducationInProgress.send(false) | ||
} | ||
} | ||
} | ||
.store(in: &self.subscriptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: can remove a few selves
guard let self = self else { return } | |
onboardingEvents | |
.subscribe(on: DispatchQueue.main) | |
.sink { [weak self] event in | |
guard let self = self else { return } | |
switch event { | |
case .didAcceptTermsOfService: | |
self.isMerchantEducationInProgress.send(true) | |
self.alertsPresenter.presentMerchantEducation { [weak self] in | |
self?.isMerchantEducationInProgress.send(false) | |
} | |
} | |
} | |
.store(in: &self.subscriptions) | |
guard let self else { return } | |
onboardingEvents | |
.subscribe(on: DispatchQueue.main) | |
.sink { [weak self] event in | |
guard let self else { return } | |
switch event { | |
case .didAcceptTermsOfService: | |
isMerchantEducationInProgress.send(true) | |
alertsPresenter.presentMerchantEducation { [weak self] in | |
self?.isMerchantEducationInProgress.send(false) | |
} | |
} | |
} | |
.store(in: &subscriptions) |
@@ -6,6 +6,7 @@ protocol CardPresentPaymentAlertsPresenting<AlertDetails> { | |||
associatedtype AlertDetails | |||
func present(viewModel: AlertDetails) | |||
func presentWCSettingsWebView(adminURL: URL, completion: @escaping () -> Void) | |||
func presentMerchantEducation(completion: @escaping () -> Void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Just feeling a bit uncertain about this, as this is just for TTP and I'm not sure if it requires a separate function in fear of this protocol growing very long in the future with new views like this 🤔 @joshheald WDYT about having a separate function here (maybe renaming to presentTTPMerchantEducation
since this is specific to TTP), or include this merchant education in present(viewModel: AlertDetails)
? This education UI has a different presentation style as a fullscreen sheet though, it probably requires more work to allow a view model other than CardPresentPaymentsModalViewModel
in the UIKit version CardPresentPaymentAlertsPresenter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was going back and forth on how to better approach this.
Integrating within func present(viewModel: AlertDetails)
would go against a current setup. We want a different ViewModel and a different View for TTP education.
We could also create a separate presenter whose sole responsibility would be to present TTP education on top of the existing context. (Example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integrating within
func present(viewModel: AlertDetails)
would go against a current setup. We want a different ViewModel and a different View for TTP education.
I was thinking of refactoring the UIKit version CardPresentPaymentAlertsPresenter
to use a generic AlertDetails
similar to the POS version so that we can decide how to present an alert detail. But it will likely require significant changes to maintain backward compatibility.
We could also create a separate presenter whose sole responsibility would be to present TTP education on top of the existing context. (Example)
Nice, I like this approach - it doesn't add complexity to the alerts presenter and the merchant education UI is not an "alert" anyway. When the time comes to support TTP in POS, we can update BuiltInCardReaderMerchantEducationPresenter
to maybe a protocol to support both UIKit and SwiftUI. Lemme know what you think of going with this approach!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will start manual testing after this thread concludes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the second approach of having a separate presenter for education is reasonable. Non-TTP-related controllers don't have to have any knowledge of it. I will include it in the main PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in dab2e9d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why you went back and forth here!
I was thinking of refactoring the UIKit version CardPresentPaymentAlertsPresenter to use a generic AlertDetails similar to the POS version so that we can decide how to present an alert detail. But it will likely require significant changes to maintain backward compatibility.
In some ways it does seem a little strange not to include alerts which (currently) only show during a built in reader connection in CardPresentPaymentBuiltInReaderConnectionAlertsProvider
. However... just because they (currently) only happen during that connection doesn't make them a connection alert. The needs we have for education are different than we're showing information about an ongoing connection. It's not even quite part of the connection flow; it sort of floats above it.
It would be perfectly valid to show education outside a connection flow as well, and if we did that, it would seem strange if we needed a CardPresentPaymentEventDetails
to do it. In the new design, we'd be fine: just have some new CardPresentPaymentEventDetails
case like showTapToPayEducation
, which the presenter would then use to create a TapToPayEducationViewModel
for presentation. When we wanted to present the education flow outside of the connection flow, we just create the TapToPayEducationViewModel
directly and don't care about the payment event details, as it's not happening in a payment context. The event is the user tapping something else in the app, instead.
The new design is more flexible, because CardPresentPaymentEventDetails
is not a view model, it's just details about an event, and the presenter can decide what view model to make for each event and how to present it.
Unfortunately under the existing UIKit design, it's not so clean, as we have the same type of concrete view model class produced for every event, and it does need to be used to present a view. So unless we refactor the existing code to use the new approach, we can't reasonably include the education step in this way.
It would probably be ideal to do the refactor. But, it's also not factored in to the project so probably not a reasonable choice here. Since we're not doing it now, let's avoid making it harder to do it in the future. I think that having a separate presenter meets that need. We should try to keep it simple though!
WooCommerce/Classes/ViewRelated/Orders/Collect Payments/CardPresentPaymentAlertsPresenter.swift
Outdated
Show resolved
Hide resolved
@joshheald, we discussed how to better integrate education within the TTP flow prior. It will be great to get your input when you have time. Thanks! |
…after-terms-and-conditions-are-accepted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! LGTM 🚀
From testing on an iPhone Pro Max iOS 18.1, there were some edge case issues that shouldn't block the PR:
- Rotating to landscape becomes an empty screen, it's probably related to the SwiftUI view using
NavigationView
instead ofNavigationStack
ScreenRecording_12-05-2024.10-35-24_1.MP4
- After tapping
Done
, the app was stuck at this configuration view. I waited for a few minutes and saw a few Xcode performance warnings:
ResultsController.swift:135 Performing I/O on the main thread by reading or writing to a database can cause slow launches. This is known to cause slow launches for your users.
- I saw several warnings from various use cases in the app like payment methods.
I tried reproducing this, but deleting and reinstalling the app doesn't always prompt me the ToS again.
...iewRelated/Dashboard/Settings/CardReadersV2/Tap to Pay Education/TapToPayEducationView.swift
Outdated
Show resolved
Hide resolved
...iewRelated/Dashboard/Settings/CardReadersV2/Tap to Pay Education/TapToPayEducationView.swift
Outdated
Show resolved
Hide resolved
...ce/Classes/ViewRelated/CardPresentPayments/BuiltInCardReaderMerchantEducationPresenter.swift
Outdated
Show resolved
Hide resolved
@jaclync, thanks for the review!
Again me confusing the
I'll test and check that as well. It could be related to those warnings but could also be related to my approach of delaying |
@jaclync, FYI for the future one more way to test is to use a simulated card reader, and add:
Simulated card reader always calls |
I've no idea why it's happening 🤔 I tested a few times, both with simulated and real updates. The easiest way I found to trigger real configuration updates is by removing the device and switching the store to a different country. However, the configuration doesn't get stuck for me. I suppose you hit some sort of issue that may not be related to education updates. RPReplay_Final1733382886.MP4 |
I assumed TTP still has to be tested on a physical eligible iPhone, not in a simulator (I didn't see the TTP entry points). I tried this |
…after-terms-and-conditions-are-accepted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do detailed testing, but did see the screens showing up. I couldn't get my phone to do a config update though.
I've left a few thoughts and questions – that doesn't mean you have to make any or all of these changes, it's quite subjective and I suspect any solution will have a bunch of similar trade offs to consider. Happy to chat about any of them synchronously if that would help.
/// The Publisher that emits when TTP Terms and Services are accepted | ||
var builtInCardReaderAcceptToSEvents: AnyPublisher<Void, Never> { get } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a very specific publisher for a single event, which happens rarely. What do you think about that approach?
On one hand, at least it's easy to understand when it will publish... on the other, it seems like a fairly heavyweight solution.
Since this only happens during connection, and usually happens right before a software update, would it be convenient to include it as part of CardReaderSoftwareUpdateState
, using associated values?
I suppose that isn't great type design, because it won't be relevant to most software updates.
Alternatively perhaps, change the signature of func connect(_ reader: options:) -> AnyPublisher<CardReader, Error>
to return a publisher with more detailed events informing about the connection process, including this one?
What you have seems similar in design to the softwareUpdateEvents
publisher; that usually fires during a connection but is separate from connection. However, in the software updates case it actually can be used at other times, for non-mandatory updates which the user initiates – I don't think that's true for TTP terms acceptance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this only happens during connection, and usually happens right before a software update, would it be convenient to include it as part of CardReaderSoftwareUpdateState, using associated values?
I suppose that isn't great type design, because it won't be relevant to most software updates.
I agree. Software update events are not related to ToS acceptance.
Alternatively perhaps, change the signature of func connect(_ reader: options:) -> AnyPublisher<CardReader, Error> to return a publisher with more detailed events informing about the connection process, including this one?
Thanks. I'll investigate how this connect
method looks and works and where the ToS would fit.
I only reviewed connectedReaders
, discoveredReaders
, readerEvents
, and softwareUpdateEvents
publishers and concluded that ToS event doesn't particularly fit in any. The closest that I think is public var readerEvents: AnyPublisher<CardReaderEvent, Never>
.
isMerchantEducationInProgress.send(true) | ||
presenter.presentMerchantEducation { [weak self] in | ||
guard let self else { return } | ||
isMerchantEducationInProgress.send(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should modify self.state
, instead of having a separate state item here?
I think that would be the way to keep returnSuccess
as a synchronous action, as we'd know whether eductation was in progress or not before we called it.
The tricky thing with this suggestion is that we don't neccesarily want a full-fledged showingEducation
state, because it'll often coincide with the software update states. Perhaps we'd want another associated value on the updating
state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I began by changing state
here and using associated values. This is what we've spoken about previously. There were a couple of issues and overcomplications with it:
- I couldn't programmatically guarantee that education only happens in specific states. In practice, it could be visible when connecting, updating, and connecting. But if it happens at times I wouldn't expect, I wouldn't be able to set the
state
. It could be a minor issue and not likely to happen. - Adding associated values to existing states meant that the education state needed to be set at any time one of those states was set.
state = .connectToReader(education: false)
,state = .updating(progress:0, education: state.educationState)
. It felt a bit too intrusive.
That would be the way to keep returnSuccess as a synchronous action, as we'd know whether education was in progress or not before we called it.
True. I would still need to think about how to keep it synchronous. Likely not calling returnSuccess
if state contains education
being in progress. But then calling returnSuccess
when merchant presentation is dismissed and we have specific state
. Could be a similar concern to the previous point where we would affect multiple code paths in the controller.
Overall, setting a separate state var simplified the approach but could be considered inconsistent since it didn't use the existing mechanism of the controller. I'll think one more time if existing state setup could be used without introducing too many changes or risk.
isMerchantEducationInProgress.eraseToAnyPublisher() | ||
.filter { $0 == false } | ||
.first() | ||
.sink { [weak self] _ in | ||
guard let self else { return } | ||
onCompletion?(.success(result)) | ||
state = .idle | ||
} | ||
.store(in: &subscriptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how this works but it makes the function a little misleading. It's changed to "maybe return success sometime", because this is asynchronous, and combine-based.
} | ||
|
||
final class BuiltInCardReaderMerchantEducationPresenter: BuiltInCardReaderMerchantEducationPresenting { | ||
private weak var rootViewController: UIViewController? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the root view controller around, as a concrete UIViewController
at least, has been a pain point in the past. For new code, I'd recommend using the ViewControllerPresenting
protocol instead, improving it if needed. Of course, if you can find a way to avoid passing it at all, that's even better!
} | ||
} | ||
|
||
@MainActor @preconcurrency required dynamic init?(coder aDecoder: NSCoder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, what's a dynamic init
?! New keywords, I'll have to look that one up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Xcode 16 generated this one for me 😀
let viewModel = TapToPayEducationViewModel(flow: .onboarding, onDismiss: onDismiss) | ||
super.init(rootView: TapToPayEducationView(viewModel: viewModel)) | ||
|
||
viewModel.onDismiss = { [weak self] in | ||
guard let self else { return } | ||
|
||
dismiss(animated: true) { | ||
onDismiss() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole dance around self is really annoying 😢
Is it still actually required? Do hosting controllers still not respect the dismiss Environment value? This post suggests it should work, since you do have a NavigationStack... (I wouldn't bother with the more complex solutions there though...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this annoying dance since I needed to know when the education view 'did' dismiss rather than 'will' dismiss. I'll check the post you linked, thanks!
@@ -27,7 +27,7 @@ final class TapToPayEducationViewModel: ObservableObject { | |||
let cardPresentPaymentsOnboardingUseCase: CardPresentPaymentsOnboardingUseCaseProtocol | |||
let siteID: Int64 | |||
|
|||
private let onDismiss: () -> Void | |||
var onDismiss: () -> Void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onDismiss
implies a notification that it is being dismissed, but I think this is actually a dismissAction
– calling it dismisses the view, and then the other onDismiss
(which was passed in to the hosting controller) is called to notify something that it was dismissed as well.
…apToPayEducationView
Addressed:
Kept as it is:
|
…after-terms-and-conditions-are-accepted
Closes: #14327
Description
Integrate merchant education into TTP connection flow and present it after Terms of Service are accepted. Two main flows should be supported:
More information: #14327
Solution
builtInCardReaderAcceptToSEvents
publisher inStripeCardReaderService
to emit an event when TOS is accepted.BuiltInCardReaderMerchantEducationPresenter
to presentTapToPayEducationViewViewHostingController
on top of already presented modals.BuiltInCardReaderConnectionController
to observebuiltInCardReaderOnboardingEvents
and present merchant educationBuiltInCardReaderConnectionController
states to includeeducationInProgress
in associated values. It is used to determine whether the connection process should complete or be set towaitToComplete
if connection has finished but the education is still ongoing.There are a few key decisions made here that could be approached in different ways:
StripeCardReaderService
communicates that TOS is acceptedSteps to reproduce
Prerequisites between each test case:
tapToPayEducation
inDefaultFeatureFlagService
Test Case 1: Set up Tap to Pay
Test Case 2: Payment
Refunds
I will handle refund testing and any necessary changes in another task.
Testing information
I only did manual testing. The one place that contains the main business logic for the flow is
BuiltInCardReaderConnectionController
which currently doesn't have any tests. Do you think it makes sense to start covering it with tests?Videos
Set up Tap to Pay flow
Set.up.TTP.mov
Payment Flow
Payment.flow.MP4
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: