-
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
Empty Stats: Add event tracking for Publicize / Blogging Reminders #17313
Empty Stats: Add event tracking for Publicize / Blogging Reminders #17313
Conversation
You can trigger an installable build for these changes by visiting CircleCI here. |
27648b0
to
82f9684
Compare
Notes
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
The default view count 0 was being supplied to itemToDisplay, which was causing .social to be returned when it should have been returning nil.
60a3d9d
to
b4fb243
Compare
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.
Great work!
Since you've already cleaned up some of my code 😄 could you just please get rid of nil coalescing operator here?
And case let
syntax would be great here as well! (so I don't have to do this)
I have just two concerns:
stats_blogging_reminders_nudge_shown
wasn't triggered for me 😬, I will check again (few times)didChangePublicizeServices
method ofSharingViewControllerDelegate
could fire even if there are no changes (but that's not the scope of this PR - just pointing out here)
case is GrowAudienceCell.HintType where !insightsToShow.contains(.growAudience): | ||
case let hintType as GrowAudienceCell.HintType where !insightsToShow.contains(.growAudience): |
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.
👌
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.
@momo-ozawa it worked just fine for all the described events, except for the blogging reminders nudge appearing. No events are fired at all.
At least here, inside loadPinnedCards
the execution is hitting the break
. Let me know if you need more info!
case InsightType.customize where !insightsToShow.contains(.customize): | ||
insightsToShow = insightsToShow.filter { $0 != .growAudience } | ||
insightsToShow.insert(.customize, at: 0) | ||
|
||
// Work around to make sure customize insights shown is tracked only once |
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.
Is this to prevent the event from being fired multiple times?
if that's the case, this is something we should not be worried about, given that normally we look at "unique" events.
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.
Actually, this is a workaround to make sure the correct event will be fired once.
If you go to a site with more than 30 views (i.e. customize should be showing) and comment out the if viewsCount != nilstatement
, you'll see that the publicize_nudge_shown
event is fired right before the correct event, customize_insights_shown
.
I tried doing b4fb243 in an attempt to fix this behavior, but that actually ended up breaking the presentation logic. (after dismissing a card, the next card shown would be the incorrect card, etc.) Maybe @nikola-milicevic has some insight on how to address this issue without breaking the presentation logic?
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.
Oh ok! If that's the case then don't worry. Just wanted to make sure.
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.
stats_blogging_reminders_nudge_shown wasn't triggered for me 😬, I will check again (few times)
Checked again, few times, all good 😊
Not sure what happened.
I have the same issue 😬 |
🤯 Hmmmm. Looking into this. So weird -- in your screenshot |
4f78e81
to
8b9b022
Compare
8b9b022
to
74e47ec
Compare
This fixes an issue where the following condition was failing: where !insightsToShow.contains(.growAudience)
@leandroalonso @nikola-milicevic |
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.
I also validated all the events.
Part of #17307
Description
This PR adds event tracking for the following events:
To test
To review this PR, please do the following:
Event table
Enable post sharing
button on the Publicize cardDismiss
button on the Publicize cardSet up blogging reminders
button on the Blogging Reminders cardDismiss
button on the Blogging Reminders cardRegression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested and made sure events fire properly with the correct triggers / properties
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.