-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
User Profile: track events #16327
User Profile: track events #16327
Conversation
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
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.
Hola @ScoutHarris 👋
Changes look good. Tested and confirmed the events were dispatched.
I had one question about the name. (cc @develric)
Other than that
case .userProfileShown: | ||
return "user_profile_shown" | ||
case .userProfileSiteShown: | ||
return "user_profile_site_shown" |
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.
Would it make sense to call these liker_profile_*
rather than user_profile_*
? I'm thinking that someone looking at tracks might at a glance assume that user_profile referred to the logged in user's own profile.
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.
Well, per the User Profile designs (pbArwn-K3-p2), this sheet could be displayed from somewhere other than the liker. So I made it specific to the sheet, not the presenter. That's why I added a source to user_profile_shown
. I could add a source
to user_profile_site_shown
if we like.
Or, I could change them to user_profile_sheet_*
if that's better. Which it may be...
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.
So I made it specific to the sheet, not the presenter
Oh I see. Makes sense.
I could change them to user_profile_sheet_*
I think I'm a fan :)
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.
Can do. Thanks!
Ref: #15662
New Events
Two new events are added to track:
User profile shown:
Tracked: user_profile_sheet_shown <source: like_notification_list>
.source
property is there so that in the future if the User Profile is shown from somewhere else it can be specified.Site viewed:
Tracked: user_profile_sheet_site_shown <>
Changed Event
I added a
source
property to thereader_blog_previewed
event so that we know it was shown from the User Profile. Examples:Tracked: reader_blog_previewed <site: Site Name, source: user_profile, subscription_count: 99>
Tracked: reader_blog_previewed <site: Site Name, source: reader, subscription_count: 99>
cc @develric
To test:
newLikeNotifications
feature.Tracked: user_profile_sheet_shown <source: like_notification_list>
is logged.Tracked: user_profile_sheet_site_shown <>
is logged.Tracked: reader_blog_previewed <site: Site Name, source: user_profile, subscription_count: 99>
is logged if the site is internal. (You'll have to change the siteID here.)Regression Notes
Potential unintended areas of impact
N/A. Just adding some tracks.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A. Just adding some tracks.
What automated tests I added (or what prevented me from doing so)
N/A. Just adding some tracks.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.