-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Modularization] Remove ServiceLocator.analytics dependency from POS code #15955
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
Conversation
Introduces a new PointOfSale library target to the package, including a basic module interface and version constant. Adds corresponding test target with initial tests for version and initialization.
This reverts commit 28f2d7a.
- Add Experiments and WooFoundation dependencies to PointOfSale module - Create POSDependencyProviding protocols to abstract ServiceLocator access - Use string-based analytics tracking to avoid WooAnalyticsEvent dependency - Enable independent building of PointOfSale module with clean interfaces
- Create POSAnalyticsAdapter() - Inject through environment to views and through initializer to service classes - Remove the ServiceLocator.analytics dependency from POS
Generated by 🚫 Danger |
|
|
jaclync
left a comment
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.
Nice work! Tested making a cash payment in POS and the events were tracked and sent remotely as before.
Keeping all changes in a development branch makes sense, maybe the dev branch can be merged to trunk by the end of the week / early next week since we might resume POS work after the HACK Week.
Modules/Sources/PointOfSale/Analytics/POSIneligibleUIAnalyticsEvent.swift
Show resolved
Hide resolved
Modules/Sources/PointOfSale/DependencyProviders/POSDependencyProviding.swift
Show resolved
Hide resolved
Modules/Sources/PointOfSale/DependencyProviders/POSDependencyProviding.swift
Show resolved
Hide resolved
Modules/Sources/PointOfSale/DependencyProviders/POSDependencyProviding.swift
Show resolved
Hide resolved
WooCommerce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetup.swift
Show resolved
Hide resolved
WooCommerce/Classes/POS/Presentation/Barcode Scanning/GameControllerBarcodeObserver.swift
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/POS/Analytics/POSCollectOrderPaymentAnalyticsTests.swift
Show resolved
Hide resolved
|
@jaclync I appreciate the review! 🙇 I know it's complicated to review since there are a lot of changes, and the future PRs make further changes on top of that. I have an idea in my mind of how this should be executed, but I discover new things along the way as I change plans. I will incorporate your comments in the next PR #15956 that I have. |
f589dd6
into
woomob-935-woo-pos-hack-week-pos-modularization

Description
More context: pdfdoF-7It-p2
Next PR: #15956
This is the start of the POS modularization work. For now, I'm merging everything into a development branch since things can still change a lot along the way and I don't want to be affecting trunk.
The goal of this PR is to remove one of the larger dependencies between POS and the main app code -
AnalyticsthroughServiceLocator. Also, create a platform on which to build further refactorings:PointOfSaleandPointOfSaleTeststarget modulesPOSAnalyticsProvidingprotocol within thePointOfSaleModulePOSAnalyticsAdapterwithin the Woo app code that implementsPOSAnalyticsProvidingto be injected into POS in the entry pointWooAnalyticsStatandWooAnalyticsEventtoWooFoundationto be reused between different modules.PointOfSalemodule. I also moved some models and entities that were used within analytics files to avoid changing things too much.ServiceLocator.analyticswith injected or environment version ofPOSAnalyticsProvidingIntermediate changes:
PointOfSalemodule types public to be used within the Woo app, I will turn them back to internal when refactoring is completePointOfSaleto avoid importing it into tens or hundreds of files within the Woo app. In the end, we should only have oneimport PointOfSalein the entry point (POSTabCoordinator)Steps to reproduce
Review a general approach. Thorough testing is not necessary since it's not targeting the trunk.
Testing information
Tested on iPad 18.5 simulator, iPad Air M2 26 device
RELEASE-NOTES.txtif necessary.