woocommerce-ios icon indicating copy to clipboard operation
woocommerce-ios copied to clipboard

[Woo POS] Move `Analytics` protocols to WooFoundation framework

Open jaclync opened this issue 9 months ago • 7 comments

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

Screenshot 2024-05-09 at 9 11 29 PM
  • [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

jaclync avatar May 09 '24 09:05 jaclync

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:

  • The tracks events must be validated in the Tracks system.
  • Verify the internal Tracks spreadsheet has also been updated.
  • Please consider registering any new events.
  • The PR must be assigned the category: tracks label.

Generated by :no_entry_sign: Danger

dangermattic avatar May 09 '24 09:05 dangermattic

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

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr12685-9ff4e39
Version18.5
Bundle IDcom.automattic.alpha.woocommerce
Commit9ff4e39aa30fd9ccec28ab830a1a8d24afc450fd
App Center BuildWooCommerce - Prototype Builds #9026
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot avatar May 09 '24 09:05 wpmobilebot

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?

bozidarsevo avatar May 09 '24 14:05 bozidarsevo

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.

jaclync avatar May 10 '24 01:05 jaclync

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 with WooAnalyticsStat. Only track(_ eventName:properties:error:) is necessary with the error properties logic for reuse in the POS framework
  • WooAnalytics file: for the app to keep using the WooAnalyticsStat enum in all existing event tracking use cases, the new track(_ eventName:properties:error:) updates the properties with an optional error and triggers remote event tracking. The original track(_ stat:properties:error:) updates the properties based on the WooAnalyticsStat enum as some cases add site properties - this is specific to the main app and the POS doesn't need to know

jaclync avatar May 13 '24 06:05 jaclync

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 🙇🏻‍♀️

jaclync avatar May 13 '24 06:05 jaclync

2nd pass LGTM!

iamgabrielma avatar May 13 '24 13:05 iamgabrielma