Skip to content

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Oct 14, 2021

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

  1. Open the block editor.
  2. Tap the three-dot menu in the top right.
  3. Tap Switch to HTML Mode.
  4. Tap the three-dot menu in the top right.
  5. Tap Switch to Visual Mode.
  6. Close the editor.
  7. Verify the following Tracks events include the correct blog_id
    property
    • wpandroid_editor_session_start
    • wpandroid_editor_session_switch_editor
    • wpandroid_editor_session_end

To test the automated tests

  1. Comment out the blog_id property insertion at the "source"
  2. Run the PostEditorAnalyticsSessionTest via AndroidStudio or in the CLI via ./gradlew :WordPress:cleanTestWordpressWasabiDebugUnitTest :WordPress:testWordpressWasabiDebugUnitTest --tests "org.wordpress.android.ui.posts.editor.PostEditorAnalyticsSessionTest"
  3. Notice the tests failing with Argument(s) are different! Wanted: error where the blog_id is entirely missing from the Actual invocations have different arguments: parts.

Another way to test is to:

  1. Modify MapHasEntry("blog_id" to blog_id) to something like MapHasEntry("blog_id" to 2222L)
  2. Run the tests as before and notice them error with Argument(s) are different! where the actual invocations include 1234L but the test waiting for 2222L.

Regression Notes

  1. Potential unintended areas of impact
    As I had to refactor the AnalyticsUtils.trackWithSiteDetails() method a bit (to make it more testable via AnalyticsTrackerWrapper), tracks that use that method may be impacted if buggy.

  2. 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.

  1. What automated tests I added (or what prevented me from doing so)
    Added tests that verify that the tracks have the blog_id property.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 14, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 14, 2021

You can test the changes on this Pull Request by downloading the APKs:

@hypest hypest force-pushed the gb/add-blog-id-to-editor-session-events branch from 235ced4 to 0685f50 Compare October 15, 2021 14:54
@hypest hypest marked this pull request as ready for review October 29, 2021 15:12
@hypest hypest requested a review from antonis October 29, 2021 15:13
Copy link
Contributor

@antonis antonis left a 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.

@hypest
Copy link
Contributor Author

hypest commented Nov 2, 2021

Thanks for the review @antonis ! I've addressed the comments so far, let me know if you have more feedback, thanks!

Since that variant checks for null properties anyway. Also, doesn't
instantiate a new AnalyticsTrackerWrapper as the shorter
trackWithSiteDetails does.
@hypest hypest self-assigned this Nov 3, 2021
@hypest hypest merged commit ce15843 into develop Nov 3, 2021
@hypest hypest deleted the gb/add-blog-id-to-editor-session-events branch November 3, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants