-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Jetpack Focus: Add analytics tracking for the Jetpack menu card #19786
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
Conversation
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
momo-ozawa
left a comment
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.
Works as described, but the unit tests in JetpackBrandingMenuCardPresenterTests are failing because the phase doesn't get updated properly
| private let persistenceStore: UserPersistentRepository | ||
| private let featureFlagStore: RemoteFeatureFlagStore | ||
| private let currentDateProvider: CurrentDateProvider | ||
| private let phase: JetpackFeaturesRemovalCoordinator.GeneralPhase |
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.
The unit tests for this class are failing because the phase doesn't get updated when you do remoteFeatureFlagsStore.removalPhaseX = true
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.
Now that I think about this, other than the tests, if the phase changes, we'll have to refresh the presenter to carry the correct value. So given that, I think it's best to revert this change altogether.
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.
Updated in 9dc4c28
Let me know what you think!
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 given that, I think it's best to revert this change altogether.
Agreed!
Closes #19450
Description
This PR builds on #19782 and #19785 and implements analytics tracking for the menu card
Testing Instructions
JetpackBrandingMenuCardPresenter.swiftline 123secondsInDayto any small value, 1, for exampleremove_feature_card_displayedremove_feature_card_tappedremove_feature_card_link_tappedremove_feature_card_menu_accessedremove_feature_card_remind_later_tappedsecondsInDayto 1, then wait for 7 seconds)remove_feature_card_hide_tappedRegression 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)
N/A
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.