woocommerce-ios
woocommerce-ios copied to clipboard
[Woo POS] Move `Analytics` protocols to WooFoundation framework
Depends on https://github.com/woocommerce/woocommerce-ios/pull/12677
Why
The goal of this PR is to move the Analytics
protocol from the app framework to the WooFoundation framework so that the analytics instance can be DI'ed to the new POS framework. The usage of this change will be in a separate PR, with an example in https://github.com/woocommerce/woocommerce-ios/commit/95a8554e90f5cc543b5ef9af60316d6747877998 where a randomly picked event is tracked when the POS view appears.
How
Some decisions that I made for the initial implementation, any early feedback is welcome and I can make any changes before making the PR official for review!
What's moved to WooFoundation
-
Analytics
protocol - Dependencies in the protocol
-
AnalyticsProvider
protocol -
WooAnalyticsEventPropertyType
protocol -
WooAnalyticsStat
enum:- This contains the most diffs (1231 new/deleted lines)
- I was debating about whether to move this, or updating its usage in
Analytics
to be a more generic string enum. In the end, I decided to include this because:- It will require quite some changes to update the enum usage in the protocol
- It's nice having a consolidated list of events to avoid accidentally having two events of the same name
-
WooAnalyticsEvent
struct:- This one isn't necessary, since this struct is to allow type-checking on the event properties and a cleaner way of specifying events. But I thought this could make event tracking code easier in the POS framework as well
-
Why WooFoundation
As WooFoundation can be the lowest level of dependency for any framework with UI, this feels like a good place to have the protocols (no implementation) of major dependencies. This way, any framework (like POS) that depends on it can specify the dependency in protocol, and then the framework caller can pass whatever implementation to the framework object.
Testing instructions
- [x] @jaclync tests that Tracks events tracked in the app still show up in Tracks
Feel free to do a confidence test to ensure the Tracks events from app usage still show up in the console and Tracks.
Screenshots
- [x] I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txt
if necessary.
1 Warning | |
---|---|
:warning: | This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews. |
1 Message | |
---|---|
:book: | This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed:
|
Generated by :no_entry_sign: Danger
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
App Name | ![]() |
|
Build Number | pr12685-9ff4e39 | |
Version | 18.5 | |
Bundle ID | com.automattic.alpha.woocommerce | |
Commit | 9ff4e39aa30fd9ccec28ab830a1a8d24afc450fd | |
App Center Build | WooCommerce - Prototype Builds #9026 |
Do we need to update https://github.com/woocommerce/woocommerce-ios/blob/trunk/docs/tracking-events.md?
Maybe just to write that import protocol WooFoundation.Analytics
needs to be used?
Do we need to update https://github.com/woocommerce/woocommerce-ios/blob/trunk/docs/tracking-events.md? Maybe just to write that
import protocol WooFoundation.Analytics
needs to be used?
Yup good idea! I added some notes in the last 2 commits.
Update: I've read that we're pausing the framework work in pfoUAQ-EF-p2#comment-517. Please skip reviewing this PR today and I'll post an update when the decision is confirmed. The following was drafted before I heard about the news.
Thanks for all the suggestions and feedback above! I believe I've responded to all the comments and updated the PR based on them, it's ready for another pass. The diffs now are much smaller, though there are still lots of changed files from the Analytics
protocol import. I've done a pass of the import review myself to ensure there are no empty lines etc., and I'd recommend focusing on these files:
-
Analytics
protocol: the stat enum parameter has been replaced with a string, so that the POS framework doesn't have to be tied to using a String enum. 3 tracking methods are also removed as the main app event tracking use cases are tracking withWooAnalyticsStat
. Onlytrack(_ eventName:properties:error:)
is necessary with the error properties logic for reuse in the POS framework -
WooAnalytics
file: for the app to keep using theWooAnalyticsStat
enum in all existing event tracking use cases, the newtrack(_ eventName:properties:error:)
updates the properties with an optional error and triggers remote event tracking. The originaltrack(_ stat:properties:error:)
updates the properties based on theWooAnalyticsStat
enum as some cases add site properties - this is specific to the main app and the POS doesn't need to know
Update: as other POS work is based on this PR and all the changes are within WooCommerce & WooFoundation frameworks, please continue with review today @iamgabrielma @joshheald @bozidarsevo 🙇🏻♀️
2nd pass LGTM!