[Woo POS][Modularization] Remove all ServiceLocator usage from POS code #15956

staskus · 2025-07-29T20:29:54Z

More context: pdfdoF-7It-p2
Continuation of #15955

Description

Removing the remaining ServiceLocator usage and injecting dependencies to POS from the entry point. These 6 were enough to cover all the ServiceLocator usage:

public protocol POSDependencyProviding {
    var analytics: POSAnalyticsProviding { get }
    var stores: POSStoresProviding { get }
    var currency: CurrencySettings { get }
    var featureFlags: POSFeatureFlagProviding { get }
    var session: POSSessionManagerProviding { get }
    var connectivity: ConnectivityObserver { get }
}

Payments

I'm leaving CardPaymentService and related dependencies outside of the POS module. At least for now. They would require a separate refactoring to untangle from IPP, and at the moment, they are injected through CardPresentPaymentFacade dependency that is defined for POS.

Steps to reproduce

Review a general approach. Thorough testing is not necessary since it's not targeting the trunk.

CI should succeed
POS should launch
POS events should be tracked

Testing information

Tested on iPad 18.5 simulator, iPad Air M2 26 device

Screenshots

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

dangermattic · 2025-07-29T20:30:39Z

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

Generated by 🚫 Danger

wpmobilebot · 2025-07-29T20:39:06Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr15956-04fd036`
	Version	`22.9`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`04fd036`
	Installation URL	1equgr78n73sg

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

…OrderPaymentUseCaseAdaptor

It only requires a single dispatch method that is used within POS application

CardPresentPaymentService and its dependencies have a direct relationship with the current IPP implementation which would need to be refactored separately to be able to move to the POS module. For now, keeping it within the Woo app.

… environment in POS

…deObserver

staskus · 2025-07-30T21:45:59Z

It's yesterday's work. It's an open question whether it's worth reviewing now or waiting for the end of the week, if the end result is fine, and then going back and reviewing the PRs. I'll try to stack a couple on top of each other. cc @jaclync @iamgabrielma

In case you're interested,

Today, I progressed more and reached a point where moving the majority of the POS code resulted in successfully building PointOfSale module. I did it by moving all POS files to PointOfSale module, trying to build, and adding any dependencies that are missing to the module or commenting stuff out until everything successfully built. I found it hard to detect all the dependencies from the Woo target within the POS code, so this way the compiler helped me.

However, this is all untidy. Woo app dependencies within POS will either need to be:

Moved to a shared module (WooFoundation, Yosemite...)
Injected to POS through the adaptor or some other way
Duplicated in the POS target
... or just move away outside the POS folder since they are not really used

I make a decision on a case-by-case basis for these dependencies, which really depends on whether it's just an extension or a large class with a lot of entangled dependencies.

Here's a list of remaining dependencies that I'm in process of addressing:

DeepLinkNavigator
SafariView
SupportForm
CardPresentPaymentsOnboardingViewModel
CardPresentPaymentError
CardPresentPaymentInvalidatablePaymentOrchestrator
CardPresentPaymentsConnectionControllerManager
POSCollectOrderPaymentAnalytics
CollectOrderPaymentAnalytics
WooAnalytics event constants between IPP and POS
OrderTotalsCalculator
PaymentCaptureCelebrationProtocol
VersionHelpers
POSEntryPointEligibilityCheckerProtocol
SelectedSiteSettingsProtocol
WooAnalytics+WooApp
View+Conditionals
.product(name: "CocoaLumberjackSwift", package: "CocoaLumberjack")
scrollVerticallyIfNeeded
ScrollableVStack
KeyboardObserver
POSCollectOrderPaymentPreviewAnalytics
POSPreviewEntryPointEligibilityChecker
PointOfSaleEntryPointView
UIImage+Woo
CollectOrderPaymentUseCaseError
WCSettingsWebView
String+Helpers
Array+Helpers
View+Measurements
AddEditCoupon view
POSCouponCreationSheetModifier
BottomSheetListSelector
AddEditCouponViewMode
POSProductFactory
WooRoundedBorderTextFieldStyle
SwiftUI Shimmer
View+RoundedBorder
readableCountry extension CountryCode
ReceiptEligibilityUseCase
IndefiniteCircularProgressViewStyle
View+autofocustextmodifier
ProductImageThumbnail
FormattableAmountTextFieldViewModel
FormattableAmountTextField
ServiceLocator currency settings
PriceFieldFormatter
CurrencySettings
Sanitized
WaitingTimeTracker
trackSize(size.) modifier
WooConstants

Once all these dependencies find their place to be used within the PointOfSale module, we could have a v1. A PointOfSale module with the majority of POS-only code, which is dependent on other modules within the app, and has some injected dependencies from the Woo app.

jaclync

Glad seeing ServiceLocator usage removed from POS! Just an issue from the unsupported currency flow that can be fixed later.

It's an open question whether it's worth reviewing now or waiting for the end of the week, if the end result is fine, and then going back and reviewing the PRs. I'll try to stack a couple on top of each other.

Having incremental work reviewed along the way sounds fine to me, the PR size has been manageable so far given a bunch of diffs are just moving files and updating dependencies.

Here's a list of remaining dependencies that I'm in process of addressing:

Let me/Gabriel (if he has time) know if we can help pick up some dependencies in the list, especially for the ones we worked on. I can pick up the eligibility related ones if you haven't started. For shared views, we can also consider creating a new module like WooFoundationUI/WooUI though maybe keeping all in existing modules is more practical.

jaclync · 2025-07-30T22:39:37Z

Modules/Sources/PointOfSale/DependencyProviders/POSDependencyProviding.swift

 public protocol POSStoresProviding {
-    var sessionManager: POSSessionManagerProviding { get }
-    // Add other stores manager methods as needed
+    func dispatch(_ action: Yosemite.Action)


was hoping not having to have this in the POS module, but I guess there's still some usage 😅

You're actually right. I think we can avoid it, I think it's only been used in the card payment code, which will be outside the POS now. 👍

jaclync · 2025-07-30T22:42:01Z

Modules/Sources/PointOfSale/Environment/POSEnvironmentKeys.swift

+}
+
+// Default implementations for testing/previews
+private struct DefaultPOSStores: POSStoresProviding {


nit: since we also have POSPreviewServices that includes individual dependencies for previews, how about removing the default dependencies here if the purpose is for SwiftUI previews? Previews that need it can initialize POSPreviewServices and set the required environment keys. If we miss setting an environment key, having the app crash would make it more discoverable than some features silently failing IMO.

I don't know if we can avoid setting a default value, but I will check. What we could do is to reuse these Default stores within the Preview service.

If we miss setting an environment key, having the app crash would make it more discoverable than some features silently failing IMO.

That's true, but it's also unlikely that we forget, given that we inject it at the entry point. When we add a new dependency when developing a feature, we will see that something is not working as it should.

jaclync · 2025-07-30T22:50:08Z

WooCommerce/Classes/POS/Adaptors/POSServiceLocatorAdaptor.swift

+    init(featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService,
+         storesManager: StoresManager = ServiceLocator.stores,
+         connectivityObserver: ConnectivityObserver = ServiceLocator.connectivityObserver,
+         currencySettings: CurrencySettings = ServiceLocator.currencySettings,


just a note that ServiceLocator.currencySettings could change if the currency settings (site settings) sync is triggered throughout the store life cycle. The POS eligibility check should ensure the currency is eligible for POS, but if the check is within the POS module, we might need to update the currency in POSDependencyProviding in case the merchant changes the currency settings in core and retry in the app to be eligible for POS. We can test it when we move the eligibility check code.

I need to check how it works. We could always return the latest ServiceLocator.currencySettings when users need currency settings, instead of injecting the value during initialization. I'll check if that helps. I didn't understand if the issues you described below are related to my changes or happen in general even on trunk.

jaclync · 2025-07-30T23:05:03Z

WooCommerce/Classes/POS/TabBar/POSTabCoordinator.swift

                                                                            stores: storesManager,
-                                                                            collectOrderPaymentAnalyticsTracker: collectOrderPaymentAnalyticsTracker)
+                                                                            collectOrderPaymentAnalyticsTracker: collectOrderPaymentAnalyticsTracker,
+                                                                            currencySettings: serviceAdaptor.currency)


similar to a previous comment, the currency could change if the merchant updates it to make the store eligible for POS. Let's make sure to test the unsupported currency flow later. If there's an issue, we can move the currency parameter from the initializer to CardPresentPaymentFacade.collectPayment since that seems to be where the currency is used for formatting order amount.

I tested the unsupported currency flow, and the payment failed with SCPError 9020 (Stripe API error). Payment succeeded in the next POS launch after the currency was updated to be eligible for POS. We can fix this separately.

Simulator.Screen.Recording.-.iPad.Pro.11-inch.M4.-.2025-07-31.at.08.32.10.mp4

Another issue with payments in a happy path, iPad Pro 11in M4 iOS 18.5 simulator with simulated reader enabled: after payment success (heard a cha-ching and payment success event logged), the UI is stuck on the processing screen:

Simulator.Screen.Recording.-.iPad.Pro.11-inch.M4.-.2025-07-31.at.09.12.46.mp4

🤔 Thanks, I'll check what could be the issue.

I tested the unsupported currency flow, and the payment failed with SCPError 9020 (Stripe API error). Payment succeeded in the next POS launch after the currency was updated to be eligible for POS. We can fix this separately.

I'll fix this by reverting the. Changes in the payment adaptor and card payment service. Those changes are no longer needed since this code will remain in the POS module.

This is a hard thing when trying to do refactoring in small chunks, each step may invalidate the previous one since it was only needed or it looked to be needed as an intermediate step.

Another issue with payments in a happy path, iPad Pro 11in M4 iOS 18.5 simulator with simulated reader enabled: after payment success (heard a cha-ching and payment success event logged), the UI is stuck on the processing screen:

I'm getting this on trunk as well.

staskus · 2025-07-31T07:41:25Z

@jaclync, thank you very much again for review and catching some broken cases, really valuable!

Let me/Gabriel (if he has time) know if we can help pick up some dependencies in the list, especially for the ones we worked on.

Okay. I will try to leave a task with a list after the day of what could be tackled next. I think it would be counter-productive to work on these things simultaneously due to many conflicts. However, we could tackle it one after another in different time zones.

…mentUseCaseAdaptor

…services are accessed

staskus · 2025-07-31T12:36:42Z

@jaclync Thanks again for an incredible review.

Removed stores. dispatch from POS dependencies
Addressed currency issue by continuing to use ServiceLocator within card service and adapters since it won't be moved to POS. Additionally, change the way the adaptors are implemented to always make sure the latest service dependency is returned
Addressed non-succeeding card payments here Complete card payment only after alert presentation is done #15967

staskus added type: task An internally driven task. feature: POS labels Jul 29, 2025

staskus mentioned this pull request Jul 29, 2025

[Woo POS][Modularization] Remove ServiceLocator.analytics dependency from POS code #15955

Merged

1 task

Base automatically changed from woomob-935-woo-pos-hack-week-pos-modularization-service-locator-analytics to woomob-935-woo-pos-hack-week-pos-modularization July 30, 2025 07:21

staskus added 20 commits July 30, 2025 09:22

Create POSServiceAdapter to inject all services through environment

af6cbb6

Access analytics through serviceAdapter

c5b5e2f

Prepare environment keys for used Woo app Services

374490e

Remove ServiceLocator.currencySettings from PointOfSaleOrderController

66ef0f9

Remove ServiceLocator.currencySettings from PointOfSaleCollectCashView

fa04aec

Remove ServiceLocator.currencySettings from CollectCashViewHelper

30e7847

Remove ServiceLocator.currencySettings from CardPresentPaymentCollect…

dfce2e3

…OrderPaymentUseCaseAdaptor

Implement POSStoresProviding

13774ee

It only requires a single dispatch method that is used within POS application

Remove unused StoresManager from PointOfSaleOrderController

ea74d5c

Implement POSPreviewStores

4ea7efa

Rename adapters to adaptors for consistency

d4bb0c0

Remove ServiceLocator.featureFlagService dependency from POS

529ad19

Remove store from POS dependencies

1d91c1c

Remove ServiceLocator.stores.sessionManager dependency from POS

a9ccf02

Move ConnectivityObserver to WooFoundation for reusability and set to…

00e8ec6

… environment in POS

Move DefaultConnectivityObserverTests to WooFoundationTests

c4d38ed

Clarify POSAnalyticsKey documentation for SwiftUI

5952d3b

Use dependency injection for POSServiceLocatorAdaptor to enable testing

234ee86

Document analytics parameter in observer init for GameControllerBarco…

a4947b3

…deObserver

staskus force-pushed the woomob-935-woo-pos-hack-week-pos-modularization-service-locator-remaining branch from d3a23f1 to a4947b3 Compare July 30, 2025 07:40

staskus added this to the 23.0 milestone Jul 30, 2025

staskus marked this pull request as ready for review July 30, 2025 07:42

staskus requested a review from jaclync July 30, 2025 21:31

staskus requested review from iamgabrielma and jaclync and removed request for jaclync July 30, 2025 21:31

jaclync approved these changes Jul 31, 2025

View reviewed changes

staskus added 4 commits July 31, 2025 09:42

Remove POSStoresProviding from POS dependencies

0867129

Revert removal of ServiceLocator in CardPresentPaymentCollectOrderPay…

86b78cc

…mentUseCaseAdaptor

Replace previews with default empty values

3468eaa

Structure POSServiceLocatorAdaptor to always use ServiceLocator when …

04fd036

…services are accessed

staskus mentioned this pull request Jul 31, 2025

Complete card payment only after alert presentation is done #15967

Merged

1 task

staskus merged commit 3342f01 into woomob-935-woo-pos-hack-week-pos-modularization Jul 31, 2025
13 checks passed

staskus deleted the woomob-935-woo-pos-hack-week-pos-modularization-service-locator-remaining branch July 31, 2025 12:37

staskus mentioned this pull request Jul 31, 2025

[Woo POS][Modularization] Batch 1: Make Woo app dependencies reusable for PointOfSale module #15968

Merged

1 task

staskus mentioned this pull request Sep 16, 2025

[Woo POS] Modularization: Bring existing changes to trunk #16132

Merged

1 task

[Woo POS][Modularization] Remove all ServiceLocator usage from POS code #15956

[Woo POS][Modularization] Remove all ServiceLocator usage from POS code #15956

Uh oh!

Conversation

staskus commented Jul 29, 2025

Description

Payments

Steps to reproduce

Testing information

Screenshots

Uh oh!

dangermattic commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staskus commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaclync Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus commented Jul 31, 2025

Uh oh!

staskus commented Jul 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dangermattic commented Jul 29, 2025 •

edited

Loading

wpmobilebot commented Jul 29, 2025 •

edited

Loading

staskus commented Jul 30, 2025 •

edited

Loading

jaclync Jul 31, 2025 •

edited

Loading