Track gutenberg_enabled event during rollout #11045
Conversation
|
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Looks good to me. I did leave one comment, but I think it is not an issue in practice, so I'll go ahead and approve so you can merge this if you agree.
I actually spent close to an hour unintentionally verifying that the app's "Collect Information" toggle works when it is toggled off. 🤦♂️ Once I figured out why my device wasn't sending any analytics though, I was able to verify that with this PR the gutenberg_enabled event is getting properly sent for the progressive rollout with the relevant source property. 🎉
| @@ -103,6 +104,15 @@ public static void migrateAppWideMobileEditorPreferenceToRemote(final AccountSto | |||
| } | |||
| } | |||
|
|
|||
| private static void trackGutenbergEnabledForNonGutenbergSites(final SiteStore siteStore) { | |||
| for (SiteModel site : siteStore.getSites()) { | |||
| if (!TextUtils.equals(site.getMobileEditor(), GB_EDITOR_NAME)) { | |||
There was a problem hiding this comment.
I think this is really just a theoretical concern related to the known issue that we're tracking when the network request is initiated, but in theory if the dispatch network call (line 85) somehow completed and updated the site object before we got to this if check, would it result in analytics improperly not being sent off because the site's mobile editor would have already been updated?
I have trouble seeing how this could actually happen in practice, but I wanted to mention it in case you think it could occur more easily than I think.
There was a problem hiding this comment.
Yeah, good point, better to move this before we dispatch the event.
|
@mchowning I moved the analytics tracking code. Can you do a second pass on that one? |
|
Change look good to me @maxme ! I might include something in the comment about why we're taking the obviously-less-than-ideal approach of not only sending analytics without checking whether the network call succeeded, but actually sending off the event before we even try to make the network call. Obviously, someone could check the commit behind the change and get back to this PR though, so I don't think updating the comment is critical. |
Part fix for wordpress-mobile/gutenberg-mobile#1718
There is another upcoming PR for
developbut I'm waiting for #11029 to be merged before opening it (code is ready here: https://github.com/wordpress-mobile/WordPress-Android/tree/issue/1718-track-gutenberg-rollout-v2)Known issue: it's tracked when the network request is initiated, not when we get the response.
PR submission checklist:
RELEASE-NOTES.txtif necessary.