-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Set gutenbergEnabled to false if all sites use Aztec #11060
Set gutenbergEnabled to false if all sites use Aztec #11060
Conversation
Previously, gutenbergEnabled would remain unset.
👋 @hypest ! You may want to give this a look since you and @daniloercoli were most closely connected to the earlier changes to this code. |
You can test the changes on this Pull Request by downloading the APK here. |
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.
Change and new tests look good and run correctly for me. Looks good!
One question, I wasn't able to figure out how to execute Scenario 1 from your testing steps. wordpress-mobile/gutenberg-mobile#1714 seems to focus on the wpandroid_editor_session_end
event, but I don't think the WP Android app lets you start (or end) an editor session if you do not have any sites (conditions for Scenario 1), so I couldn't trigger this event to test. Is there another relevant event that would send user_info_gutenberg_enabled
as null
? If not it may be useful to know that no events should be sent with user_info_gutenberg_enabled
as null
when a user is running an app with this change going forward.
Thanks for the review @cameronvoell ! 🙇
That is a tricky one to test for sure. 😄 The way I validated this was through both (a) checking manual logs I added to the app to verify the property value was
Events like |
Thanks for clarifying that Scenario for me. Ready to merge! |
Fixes wordpress-mobile/gutenberg-mobile#1714
This fixes an issue that began occurring with the
user_info_gutenberg_enabled
boolean property to our analytics events. In particular, we switched from sendingfalse
if a user was only using Aztec to sendingnull
. iOS did not make this change and continued sendingfalse
in that scenario (as did Android until recently). This PR fixes fixes that by setting thegutenbergEnabled
metadata to be false if all of a user's sites use Aztec.Background
This issue occurs because we made a change in July to check to see whether the
gutenbergEnabled
metadata has been set and only set that property on analytics events if it has been set. At the same time, we also made a change to only update the metadata'sgutenbergEnabled
property to betrue
if at least one of the user's sites were using gutenberg. We do not do anything, however, if none of the user's sites use gutenberg. As a result, thegutenbergEnabled
metadata property remained unset, and anull
value was sent if the user used Aztec on all their sites.With the changes in this PR, we now only send
null
if the user has no sites. Otherwise we sendtrue
if any of the user's sites use gutenberg (this behavior is unchanged) and we sendfalse
if all the user's sites use Aztec (before this change, this would have been sent asnull
). This should reverse the "swap" that occurred betweennull
andfalse
values in August 2019.To test
Scenario 1: No Sites
user_info_gutenberg_enabled
property on the analytics events is set tonull
Scenario 2: All sites using Aztec
user_info_gutenberg_enabled
property on the analytics events is set tofalse
Scenario 3: At least one site using Gutenberg
user_info_gutenberg_enabled
property on the analytics events is set tofalse
PR submission checklist
I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txt
if necessary.