-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Gutenberg] Add blog ID to editor session events #15465
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 trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
235ced4
to
0685f50
Compare
AnalyticsTrackerWrapper is not serializeable so, don't try to serialize and deserialize it. Instead, re-hydrate the field after restoring from the Bundle.
WordPress/src/main/java/org/wordpress/android/util/analytics/AnalyticsTrackerWrapper.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/util/analytics/AnalyticsTrackerWrapper.kt
Outdated
Show resolved
Hide resolved
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 @hypest 👍
I tested the PR on a Pixel 5 (Android 12) and everything worked as expected. The code also looks good 🎉
ps. I've added a couple of minor comments/suggestions but nothing blocking.
Thanks for the review @antonis ! I've addressed the comments so far, let me know if you have more feedback, thanks! |
WordPress/src/main/java/org/wordpress/android/util/analytics/AnalyticsTrackerWrapper.kt
Outdated
Show resolved
Hide resolved
Since that variant checks for null properties anyway. Also, doesn't instantiate a new AnalyticsTrackerWrapper as the shorter trackWithSiteDetails does.
Following suit with wordpress-mobile/WordPress-iOS#17085, this PR adds the
blog_id
to the editor session events.This is done essentially by reusing the AnalyticsUtils.trackWithSiteDetails() helper method.
All editor session events have been augmented with this PR:
wpandroid_editor_session_start
wpandroid_editor_session_end
wpandroid_editor_session_switch_editor
wpandroid_editor_session_template_apply
Note: I had to refactor some of the Analytics code to allow for passing around
AnalyticsTrackerWrapper
to make the methods testable.To test
blog_id
property
wpandroid_editor_session_start
wpandroid_editor_session_switch_editor
wpandroid_editor_session_end
To test the automated tests
blog_id
property insertion at the "source"PostEditorAnalyticsSessionTest
via AndroidStudio or in the CLI via./gradlew :WordPress:cleanTestWordpressWasabiDebugUnitTest :WordPress:testWordpressWasabiDebugUnitTest --tests "org.wordpress.android.ui.posts.editor.PostEditorAnalyticsSessionTest"
Argument(s) are different! Wanted:
error where the blog_id is entirely missing from theActual invocations have different arguments:
parts.Another way to test is to:
MapHasEntry("blog_id" to blog_id)
to something likeMapHasEntry("blog_id" to 2222L)
Argument(s) are different!
where the actual invocations include1234L
but the test waiting for2222L
.Regression Notes
Potential unintended areas of impact
As I had to refactor the
AnalyticsUtils.trackWithSiteDetails()
method a bit (to make it more testable viaAnalyticsTrackerWrapper
), tracks that use that method may be impacted if buggy.What I did to test those areas of impact (or what existing automated tests I relied on)
I tried sharing a photo to WP starting from the camera app, and that triggered the
wpandroid_share_to_wp_succeeded
event, which I verified appearing OK in the backend.Added tests that verify that the tracks have the
blog_id
property.PR submission checklist:
RELEASE-NOTES.txt
if necessary.