[Woo POS][Modularization] Batch 1: Make Woo app dependencies reusable for PointOfSale module #15968
Conversation
CardPresentPaymentsOnboardingViewModel is a strong IPP dependency on POS which drags a lot of dependencies. Solution is to pass CardPresentPaymentOnboardingViewFactory instead of CardPresentPaymentsOnboardingViewModel that hides away ViewModel implementation. ViewModel is injected at an adaptor level - CardPresentPaymentOnboardingAdaptor.
…ntPaymentsConnectionControllerManager to POS Adaptors
POSCollectOrderPaymentAnalytics depends on concrete IPP implementation that is not mean to be moved to POS. - Rename POSCollectOrderPaymentAnalytics to POSCollectOrderPaymentAnalyticsAdaptor - Split POSCollectOrderPaymentAnalyticsTracking POS protocol from IPP CollectOrderPaymentAnalyticsTracking protocol - Only connect POSCollectOrderPaymentAnalyticsTracking and CollectOrderPaymentAnalyticsTracking at the implementation point
Generated by 🚫 Danger |
|
|
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
Thanks for the updates! Noticed an issue when testing onboarding for a store with incomplete onboarding state where support and learn more CTAs aren't tappable:
Simulator.Screen.Recording.-.iPad.Air.13-inch.M3.-.2025-08-01.at.12.15.22.mp4
This doesn't happen in trunk. I'll also take a look later.
| static func orderCreationFailed( | ||
| usesGiftCard: Bool, | ||
| errorContext: String, | ||
| errorDescription: String, | ||
| errorType: OrderCreationErrorType? = nil | ||
| ) -> WooAnalyticsEvent { | ||
| var properties: [String: WooAnalyticsEventPropertyType] = [ | ||
| "use_gift_card": usesGiftCard, | ||
| "error_context": errorContext, | ||
| "error_description": errorDescription | ||
| ] | ||
|
|
||
| if let errorType { | ||
| properties["error_type"] = errorType.rawValue | ||
| } | ||
|
|
||
| return WooAnalyticsEvent(statName: .orderCreationFailed, properties: properties) | ||
| } |
There was a problem hiding this comment.
nit: this seems to be tracked in both POS and IPP, WDYT having this extension in WooFoundation for example for use? this way, any modifications to the tracking apply to both use cases.
There was a problem hiding this comment.
I don't know if we should strive to reuse 100%. Since it's the only event so far that had the same logic, I felt it was fine to keep it separately next to other POS events. In fact, I think we track OrderCreationErrorType only on POS for now.
| static let lineWidth: CGFloat = 10.0 | ||
| static let backgroundOpacity: CGFloat = 0.2 | ||
| public extension IndefiniteCircularProgressViewStyle { | ||
| public enum Constants { |
There was a problem hiding this comment.
nit: getting 'public' modifier is redundant for enum declared in a public extension warning
| public enum Constants { | |
| enum Constants { |
| } | ||
|
|
||
| enum Localization { | ||
| public enum Localization { |
There was a problem hiding this comment.
nit: does this string need to be public?
| /// Helpers for working with versions (e.g. comparing two version strings) | ||
| /// | ||
| final class VersionHelpers { | ||
| public final class VersionHelpers { |
There was a problem hiding this comment.
nice move, I've been hoping to use this in Yosemite while it was in the app layer.
|
|
||
| /// External view service abstraction | ||
| public protocol POSExternalViewProviding { | ||
| func createSupportFormView(isPresented: Binding<Bool>) -> AnyView |
There was a problem hiding this comment.
nit: a bit scary seeing AnyView usage due to the rumored performance impact, any chance this could return a view builder to avoid AnyView?
| /// App icon (iPhone size) - used in receipt eligibility banner | ||
| /// | ||
| static var appIconDefault: UIImage { | ||
| return UIImage(named: "AppIcon60x60")! |
There was a problem hiding this comment.
is the image catalog shared automatically across modules? these images aren't used in POS yet.
There was a problem hiding this comment.
So far as I have tested, it works. Both on the device and the simulator, although I wouldn't have expected that. I specified main .bundle now. We can tell once we move the POS code to the module if everything works all right.
| SafariView(url: WooConstants.URLs.pointOfSaleDocumentation.asURL()) | ||
| DocumentationView() |
There was a problem hiding this comment.
just wondering, what are the reasons for this change that require a new DocumentationView from UIKit?
There was a problem hiding this comment.
I thought it's easier to copy rather than to move it into the common place. But probably wiser to move it to WooFoundation instead.
| onboardingScreenViewModelSubject.send(.showOnboarding(viewModel: onboardingViewModel, onCancel: { [weak self] in | ||
| self?.readinessSubscription = nil | ||
| })) | ||
| onboardingScreenViewModelSubject.send(.showOnboarding( | ||
| factory: .init( | ||
| configuration: onboardingViewModel, | ||
| createView: { [weak self] in | ||
| guard let self else { return EmptyView() } | ||
| return CardPresentPaymentsOnboardingView(viewModel: onboardingViewModel) | ||
| }), onCancel: { [weak self] in | ||
| self?.readinessSubscription = nil | ||
| }) | ||
| ) |
There was a problem hiding this comment.
is the requirement of the factory wrapper because CardPresentPaymentsOnboardingView isn't accessible in the POS module? Just wanted to confirm because AnyView down the line might come with performance tradeoff. Moving all the payments code to a new module in the future might resolve this.
There was a problem hiding this comment.
I will check if I can work around AnyView change.
Yes, the factory wrapper is required since CardPresentPaymentsOnboardingView and CardPresentPaymentsOnboardingViewModel are IPP views, and then they drag some other dependencies such as CardPresentPaymentsOnboardingUseCase, which in turn drag even more dependencies together with them.
I imagine refactoring them is an option, but it would be a time-consuming option.
I'm jumping onto a task to fix a UI test failure in trunk, sharing my progress so far. The only change that I've tried and fixes the issue so far is: /// Displays the WooPayments onboarding UI based on the state.
struct PointOfSaleCardPresentPaymentOnboardingView: View {
@ObservedObject var viewModel: PointOfSaleCardPresentPaymentOnboardingViewModel
var body: some View {
VStack(spacing: Constants.verticalSpacing) {
- AnyView(viewModel.onboardingViewFactory.createView())
+ CardPresentPaymentsOnboardingView(viewModel: viewModel.onboardingViewFactory.configuration as! CardPresentPaymentsOnboardingViewModel)
// Hides the navigation bar title `navigationTitle` in `CardPresentPaymentsOnboardingView`.
.toolbar(.hidden)
}
.posModalCloseButton(action: viewModel.cancelOnboarding,
accessibilityLabel: Localization.cancelOnboarding)
.safariSheet(url: $viewModel.onboardingURL)
.posModalSizing()
}
}My guess is that there's something about SwiftUI updates. Before this PR, the payment onboarding VM changes probably trigger |
|
Re With the limited debugging today without a working solution, it seems like the issue is with It's possible that |
|
@jaclync, thanks for checking it deeply! I was investigating the removal |
|
Version |
|
@jaclync, thanks for the review. Only came back to it now!
|
staskus
merged commit c4810b3
into
woomob-935-woo-pos-hack-week-pos-modularization
staskus
deleted the
woomob-935-woo-pos-hack-week-pos-modularization-woo-app-dependencies
branch
More context: pdfdoF-7It-p2
Last PRs: #15955, #15956
WOOMOB-935
Description
I identified a list of remaining dependencies that POS code depends on the Woo app. I started tracking them more tidily within the Linear task WOOMOB-935. Feel free to pick any of those dependencies to make them reusable for POS module.
I drew an arbitrary line to make a pull request before proceeding to further refactoring.
For each of the dependencies, I need to make a decision:
For now, I decided not to create another module (e.g
WooFoundationUI). However, that would be an easy change in the future if we decide to splitWooFoundationin more parts. We have many options for the future, including centralizing payments logic in somePaymentsmodule and then reusing it between IPP and POS. For now, this is outside the scope.Moved dependencies
Moved to WooFoundation Module
Shared utilities and UI components used across multiple modules
Core Utilities:
String & Collection Extensions:
String+Helpers → Move to WooFoundation
pluralize,isNotEmpty) used throughout appArray+Helpers → Move to WooFoundation
isNotEmpty,popFirst, etc.) widely usedSwiftUI View Modifiers:
View+Conditionals → Move to WooFoundation
renderedIf,if) used across modulesView+Measurements → Move to WooFoundation
measureHeight,measureWidth) used in multiple modulesView+RoundedBorder → Move to WooFoundation
View+ScrollModifiers → Move to WooFoundation
scrollVerticallyIfNeededaccessibility helperView+SizeTracker → Move to WooFoundation
View+AutofocusTextModifier → Move to WooFoundation
UI Components:
ScrollableVStack → Move to WooFoundation
WooRoundedBorderTextFieldStyle → Move to WooFoundation
IndefiniteCircularProgressViewStyle → Move to WooFoundation
KeyboardObserver → Move to WooFoundation
Moved to Yosemite Module
Business logic components that belong in data layer
Created POS-Specific Abstractions
Main app services requiring dependency injection
Navigation Services:
External View Services:
Moved to POS Adaptors away from POS module
Components that bridge main app to POS module
Decision: These components need access to both POS module and main app services
POS-Specific Assets
POS-specific image assets
Removed Unused Dependencies
POSSessionManagerProviding → Remove
CardPresentPaymentError → Delete
Card Present Payment Dependencies
Remaining dependencies to address
Steps to reproduce
Testing information
Tested on iPad 18.5 simulator, iPad Air M2 26 device
RELEASE-NOTES.txtif necessary.