fix(profile): click analytics [MOSOWEB-43] #67
Conversation
fix(discover): add dropdown menu to recommendation cards [MOSOWEB-72]
… can send extra_keys mastodon ID and handle
…lick function of the follow button
…the more options dropdown to their dialogues' confirm flows
jpezninjo
force-pushed
the
fix/profile-analytics
branch
2 times, most recently
from
942fa3a
to
a680560
Compare
…ord() calls with helper functions
jpezninjo
force-pushed
the
fix/profile-analytics
branch
from
b5e74b5
to
1a86114
Compare
| import { toggleFollowAccount, useRelationship } from '~~/composables/masto/relationship' | ||
|
|
||
| const { account, command, context, ...props } = defineProps<{ | ||
| const { account, command, context, gleanContext, ...props } = defineProps<{ |
There was a problem hiding this comment.
gleanContext would be profile in profile.follow.unfollow
Several components use AccountFollowButton
There was a problem hiding this comment.
How do you feel about renaming these ones to include something specific to it being the follow button? For instance profile.follow-btn.unfollow - I feel like that might make it a little more clear but also may be overkill?
There was a problem hiding this comment.
I thought "follow" would be sufficient to denote the button :( but your comment is proof that it isn't. I will change all these to follow-btn
| action = 'follow.unfollow' | ||
| else if (relationship?.requested) | ||
| action = 'follow.withdraw-follow-request' | ||
| else if ((relationship ? relationship.followedBy : context === 'followedBy') && account.locked) |
There was a problem hiding this comment.
this horrendous conditional comes from somewhere else in the <template> code below
| return `${gleanContext}.${action}` | ||
| }) | ||
|
|
||
| function handleClick() { |
There was a problem hiding this comment.
I did some clever ordering with the contents of this function. I'm open to some non-clever alternatives
There was a problem hiding this comment.
The reordering of contents in this function might be causing the bug where the unfollow modal shows up when trying to unmute someone
There was a problem hiding this comment.
Ooh good catch. I was unaware of this "mixed" use case. That counts as a 7th state for this horrible button component :'(
There was a problem hiding this comment.
Let's make sure that whatever changes we make, the end result matches what is on production. We don't want to introduce any usability differences in this PR, only analytics events. We can follow up with improvements to this terrible button in a future PR.
There was a problem hiding this comment.
Pushed up fixing the bug by removing my cleverness and bringing back the original order of events
There was a problem hiding this comment.
Verified that I'm no longer seeing that bug!
| @@ -21,39 +23,63 @@ const { client } = $(useMasto()) | |||
| const useStarFavoriteIcon = usePreferences('useStarFavoriteIcon') | |||
| const { share, isSupported: isShareSupported } = useShare() | |||
|
|
|||
| function recordEngagement(dataGlean) { | |||
There was a problem hiding this comment.
helper function that just calls engagement.record and helps us avoid code bloat
| share({ url: location.href }) | ||
| } | ||
|
|
||
| async function toggleReblogs() { | ||
| if (!relationship!.showingReblogs && await openConfirmDialog({ |
There was a problem hiding this comment.
you're gonna see me separate out condition && openConfirmDialog alot (4 times) in this PR
| confirm: t('confirm.show_reblogs.confirm'), | ||
| cancel: t('confirm.show_reblogs.cancel'), | ||
| }) !== 'confirm') | ||
| return |
There was a problem hiding this comment.
perhaps we should add analytics to the cancel cases of these dialogues?
| :text="$t('menu.open_in_original_site')" | ||
| icon="i-ri:arrow-right-up-line" | ||
| :command="command" | ||
| data-glean="profile.more.open_in_original_site" |
There was a problem hiding this comment.
Example of a link that we could do further work in order to add 'target account'-identifiers if we really wanted to
| /> | ||
| </template> | ||
|
|
||
| <template v-else> | ||
| <NuxtLink to="/pinned"> | ||
| <CommonDropdownItem :text="$t('account.pinned')" icon="i-ri:pushpin-line" :command="command" /> | ||
| <CommonDropdownItem is="button" :text="$t('account.pinned')" icon="i-ri:pushpin-line" :command="command" data-glean="profile.more.goto_pinned" /> |
There was a problem hiding this comment.
This until the end of the file are more links lacking 'target account'-identifiers
jpezninjo
changed the title
telemetry/engagementProfileEvents.js
Outdated
| 'profile.notify_start': { | ||
| engagement_type: 'general', | ||
| }, | ||
| 'profile.notify_stop': { | ||
| engagement_type: 'general', | ||
| }, |
There was a problem hiding this comment.
How do you feel about profile.notify.start and profile.notify.stop instead of using underscores?
telemetry/engagementProfileEvents.js
Outdated
| 'profile.more.open_in_original_site': { | ||
| engagement_type: 'general', | ||
| }, | ||
| 'profile.more.share_account': { |
There was a problem hiding this comment.
How do you get to this action? It doesn't show up in the dropdown for me?
There was a problem hiding this comment.
I hardcoded this one to make it show in the dropdown in order to test
There was a problem hiding this comment.
I put together a PR to be merged into this one that updates the event names to use kebab-case consistently when part of the identifier needs to be multiple words: #68
|
This is really excellent @jpezninjo - you've done great work on this!! I still need to test a few things and will do that on Monday, but in the meantime I did find one bug: When I'm following someone and have muted them, when I click the
|
| @@ -56,6 +58,7 @@ const userSettings = useUserSettings() | |||
| :to="getAccountFollowersRoute(account)" | |||
| replace text-secondary | |||
| exact-active-class="text-primary" | |||
| data-glean="profile.details.followers" | |||
There was a problem hiding this comment.
I'm not able to get these profile.details.* events to fire in the console. I only see the link_click following by page_view event. Not seeing them log to the Glean ping debugger either.
There was a problem hiding this comment.
<a> current require you to click on them directly instead of any nested children
There was a problem hiding this comment.
Whoops this screenshot is for a different UI. But similar reason, the needs to be clicked directly instead of their children -
There was a problem hiding this comment.
Okay cool. I'm sure there are some optimizations we can make, but I don't wanna hold up this PR for that so I'm cool with this for now.
There was a problem hiding this comment.
Sounds good to me. We can solve this with something applicable to all anchor tags
| @@ -34,6 +36,7 @@ const tabs = $computed<CommonRouteTabOption[]>(() => [ | |||
| }, | |||
| display: t('tab.media'), | |||
| icon: 'i-ri:camera-2-line', | |||
| dataGlean: 'profile.tabs.media', | |||
There was a problem hiding this comment.
Same here with these events - I don't see the engagement events fire in console or ping debugger, I only see the link_click and page_view events
There was a problem hiding this comment.
My reply for this went to the wrong thread. See #67 (comment)
… are also following
|
Testing is looking good!! I did find reference to the different
|
There was a problem hiding this comment.
Tested and everything is looking good. I'm sure there are optimizations we can make to the engagement events vs link/button clicks for things like the Posts/Replies/Media buttons, but we can do that in a separate PR.
Great job on this! Thank you for taking this and running with it and then crushing all the feedback!
|
@wtfluckey thank you SOO much for all the testing you've done on this and ensuring everything is okay
|
Goal
Add engagement analytics events to stuff on the Profile Page MOSOWEB-43
To Do:
Notes:
I refrained from putting in
mastodon_account_idandmastodon_account_handlefor links<a>just as a way to cut myself off from adding more work to this already large PR. Will followup with discussion and probably another PR.This should've broken this up into several PRs. I went crazy plugging in
data-glean="name"into everything before realizing I needed to hook into callback functions to add in the extra keysmastodon_account_idandmastodon_account_handle.@wtfluckey and I paired to decide to call
engagement.record()directly.New Events: