-
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
Gutenberg progressive rollout #10752
Gutenberg progressive rollout #10752
Conversation
I'll work on the iOS patch when the logic is validated and this is merged. |
WordPress/src/main/java/org/wordpress/android/util/SiteUtils.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/util/SiteUtils.java
Outdated
Show resolved
Hide resolved
You can test the changes on this Pull Request by downloading the APK here. |
for (SiteModel site : siteStore.getSites()) { | ||
if (TextUtils.isEmpty(site.getMobileEditor())) { | ||
// Enable Gutenberg | ||
enableBlockEditor(dispatcher, site); |
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 code in enableBlockEditor
does make a network call to the backend. If you have many sites in the app there is a spike in network calls, and some of those can even fail...(We've experienced this problem when migrated the Editor pref from app-wide to site settings).
We should re-use the action SiteActionBuilder.newDesignateMobileEditorForAllSitesAction
, like here: https://github.com/wordpress-mobile/WordPress-Android/pull/10752/files#diff-cb360ad2878e66ffe65d8c54967921d6R83
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.
Will this action override all sites configuration? Even sites that have been switched back to Aztec? (that's the reason why I used enableBlockEditor
instead).
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.
Yes, it does override all sites configuration. I think there is a way to by-pass this behavior, we need to add a new action to FluxC that does set the parameter set_only_if_empty
to true
on the underlying network call.
Unfortunately making sequential network calls, even if you delay it from the client, may run into race condition on the PHP backend, and there is no guarantee that the call succeed as expected.
Make sure to test via Developer console first, to check if the parameter is working fine or not.
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.
I'll try the set_only_if_empty
parameter. If it doesn't work there is actually another thing we could do: check if there is one aztec
setting for any of the user's sites:
- If there is none, then enable gutenberg for all sites.
- If there is at least one, then don't put this user in the rollout cohort.
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.
I like your idea above, it should help on making the logic simpler, and dones't required FluxC changes.
We should also make sure the device has network connectivity, since the action is "fired" but we're not listening for errors I guess. So better to try when the device is online only.
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.
Logic updated in 2ff8e8d - users who have at least one aztec enabled site will be excluded from the cohort
Just to make sure I understand: the modulo logic will only take part in deciding whether the user is in the rollout cohort, so, on a different device or after uninstall/reinstall the same user will get prompted again... we are not going to keep a flag remotely about whether they got prompted already or not, right? |
Yes.
No they won't get prompted again.
We're not keeping a remote flag for this, but we're updating the remote mobile editor setting. We only show the dialog when the mobile editor setting is changed by the app (ie. not a user initiated action) from |
This is ready for another pass. |
I've tested this again with detailed testing steps, and it's working as expected. I've only found a small edge case, where the device is with spotty connectivity (or airplane mode) and the code that does the progressive rollout here https://github.com/wordpress-mobile/WordPress-Android/pull/10752/files#diff-cb360ad2878e66ffe65d8c54967921d6R65 does set the local flag, but unable to set the editor preferences on the remote.
Additional testing steps
I think would be safer for us to check if the device is online before executing the rollout, without going crazy on checking other conditions in the code, since it's already quite complex to follow the flows. |
Agreed. We'll ignore the I added a network test in eec0ca7 |
35ac6e7
to
eec0ca7
Compare
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.
LGTM!
Fixes #10747 - Implement a client only way to make a progressive rollout for the Gutenberg editor. I currently set an arbitrary number of 5%, but since we already have ~30% of users posting with Gutenberg, this should only move the needle to 3.5% ((100%-30%) x %5 = 3.5%) ie. when adding someone to the rollout group, there is a 30% chance they're already using Gutenberg. This number is even more skewed that this code exclude most self hosted users. It's arbitrary anyway and we only want to make this progressive (5% now, then 10%, 20%, 50%, 100% later).
I targeted 13.7, our target release for v2.
Test 1:
GB_ROLLOUT_PERCENTAGE
to0
Test 2:
GB_ROLLOUT_PERCENTAGE
to100
Notes:
EDITOR_GUTENBERG_ENABLED
event:ON_PROGRESSIVE_ROLLOUT
.EditPostActivity
but we'll remove all of this at some point, so I'm not touching it.AppPrefs.isUserInGutenbergRolloutGroup
to avoid re-doing the same logic. This might also be reused later if we want to change the rollout strategy.Checks:
RELEASE-NOTES.txt
if necessary.