Skip to content
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

Finalize Analytics Usage Tracking #5820

Conversation

shiki
Copy link
Member

@shiki shiki commented Jan 5, 2022

This enhances #5503 so that it can be merged. I created a separate PR so that it's hopefully easier to review.

Changes

Removed Singleton

I removed the StoreStatsUsageTracksEventEmitter singleton and replaced it with injected properties instead: 5a834ce.

In relation to this, I changed the My Store tab press detection to use viewWillAppear in b48094f to keep the StoreStatsUsageTracksEventEmitter instance within the StoreStats* classes. It felt incorrect to add the StoreStatsUsageTracksEventEmitter inside MainTabBarController. And subsequently, I realized that this causes duplication of "interaction tracking" since StoreStatsV4PeriodViewController. trackChangedTabIfNeeded is also executed when the merchant navigates back to the My Store tab. So I just removed it in 0316ac3.

Added Tracking to Old VCs

I noticed that there are OldStoreStats* classes now. I made sure to include the same interaction tracking to these classes.

Others

Testing

  1. Log in
  2. Navigate to My Store
  3. Do a few of the following:
    • Scrolling
    • Pull-to-refresh
    • Tapping on the bars in the chart
    • Changing the tab
    • Navigating to the My Store tab
    • Tapping on a product in the Top Performers list
  4. Confirm that after performing at least 5 of the interactions within more than 10 seconds, a used_analytics Tracks event is logged.
  5. Turn off the myStoreTabUpdates feature flag and repeat steps 3 and 4.

Author Checklist

  • If it's feasible, I have added unit tests.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Jan 5, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Jan 5, 2022

You can trigger an installable build for these changes by visiting CircleCI here.

While not the best, I think this is good enough so that we can keep the
UsageTracksEventEmitter dependency within the Stats-related code.
@shiki shiki changed the title Issue/5501 analytics tracking finalize Finalize Analytics Usage Tracking Jan 5, 2022
@shiki shiki added the feature: stats Related to stats, including Top Performers. label Jan 5, 2022
The StoreStatsV4PeriodViewController.trackChangedTabIfNeeded() is more than
enough to detect if the merchant navigated back to My Store.
@shiki shiki marked this pull request as ready for review January 5, 2022 23:06
@shiki shiki requested a review from jaclync January 5, 2022 23:06
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit: Code and testing with both feature flag states ✅

@shiki shiki merged commit 3b96f79 into issue/5501-prototype-analytics-tracking Jan 6, 2022
@shiki shiki deleted the issue/5501-analytics-tracking-finalize branch January 6, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: stats Related to stats, including Top Performers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants