Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,7 @@ public void onAppComesFromBackground() {
}

// Let's migrate the old editor preference if available in AppPrefs to the remote backend
SiteUtils.migrateAppWideMobileEditorPreferenceToRemote(mContext, mDispatcher);
SiteUtils.migrateAppWideMobileEditorPreferenceToRemote(mAccountStore, mSiteStore, mDispatcher);

if (mFirstActivityResumed) {
deferredInit();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ public enum DeletablePrefKey implements PrefKey {
NEWS_CARD_SHOWN_VERSION,
AVATAR_VERSION,
GUTENBERG_DEFAULT_FOR_NEW_POSTS,
USER_IN_GUTENBERG_ROLLOUT_GROUP,
maxme marked this conversation as resolved.
Show resolved Hide resolved
SHOULD_AUTO_ENABLE_GUTENBERG_FOR_THE_NEW_POSTS,
GUTENBERG_OPT_IN_DIALOG_SHOWN,

Expand Down Expand Up @@ -607,6 +608,14 @@ public static boolean isDefaultAppWideEditorPreferenceSet() {
return !"".equals(getString(DeletablePrefKey.GUTENBERG_DEFAULT_FOR_NEW_POSTS));
}

public static boolean isUserInGutenbergRolloutGroup() {
return getBoolean(DeletablePrefKey.GUTENBERG_DEFAULT_FOR_NEW_POSTS, false);
}

public static void setUserInGutenbergRolloutGroup() {
setBoolean(DeletablePrefKey.GUTENBERG_DEFAULT_FOR_NEW_POSTS, true);
}

public static void removeAppWideEditorPreference() {
remove(DeletablePrefKey.GUTENBERG_DEFAULT_FOR_NEW_POSTS);
}
Expand Down
40 changes: 38 additions & 2 deletions WordPress/src/main/java/org/wordpress/android/util/SiteUtils.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.wordpress.android.util;

import android.content.Context;
import android.text.TextUtils;

import androidx.annotation.NonNull;
Expand All @@ -10,6 +9,7 @@
import org.wordpress.android.fluxc.Dispatcher;
import org.wordpress.android.fluxc.generated.SiteActionBuilder;
import org.wordpress.android.fluxc.model.SiteModel;
import org.wordpress.android.fluxc.store.AccountStore;
import org.wordpress.android.fluxc.store.SiteStore;
import org.wordpress.android.fluxc.store.SiteStore.DesignateMobileEditorForAllSitesPayload;
import org.wordpress.android.fluxc.store.SiteStore.DesignateMobileEditorPayload;
Expand All @@ -25,6 +25,7 @@
public class SiteUtils {
public static final String GB_EDITOR_NAME = "gutenberg";
public static final String AZTEC_EDITOR_NAME = "aztec";
private static final int GB_ROLLOUT_PERCENTAGE = 5;

/**
* Migrate the old app-wide editor preference value to per-site setting. wpcom sites will make a network call
Expand All @@ -35,8 +36,43 @@ public class SiteUtils {
* -- 12.9 OPTED OUT (were auto-opted in but turned it OFF) -> turn all sites OFF in 13.0
*
*/
public static void migrateAppWideMobileEditorPreferenceToRemote(final Context context,
public static void migrateAppWideMobileEditorPreferenceToRemote(final AccountStore accountStore,
final SiteStore siteStore,
final Dispatcher dispatcher) {
// In a later version we might override mobile_editor setting if it's set to `aztec` and show a specific notice
// for these users ("We made a lot of progress on the block editor and we think it's now better than
// the classic editor, we switched it on, but you can change the configuration in your Site Settings").
// ^ This code should be here.

// If the user is already in the rollout group, we can skip this the migration.
if (AppPrefs.isUserInGutenbergRolloutGroup()) {
return;
}

// Check if the user has been "randomly" selected to enter the rollout group.
//
// For self hosted sites, there are often one or two users, and the user id is probably 0, 1 in these cases.
// If we exclude low ids, we won't get an not an homogeneous distribution over self hosted and WordPress.com users,
maxme marked this conversation as resolved.
Show resolved Hide resolved
// but the purpose of this is to do a progressive rollout, not an necessarily an homogeneous rollout.
//
// To exclude ids 0 and 1, to rollout for 10% users, we'll use a test like `id % 100 >= 90` instead of `id % 100 < 10`.
maxme marked this conversation as resolved.
Show resolved Hide resolved
if (accountStore.getAccount().getUserId() % 100 >= (100 - GB_ROLLOUT_PERCENTAGE)) {
daniloercoli marked this conversation as resolved.
Show resolved Hide resolved
// We want to make sure to enable Gutenberg only on the sites they didn't opt-out.
for (SiteModel site : siteStore.getSites()) {
if (TextUtils.isEmpty(site.getMobileEditor())) {
// Enable Gutenberg
enableBlockEditor(dispatcher, site);
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

@maxme maxme Nov 12, 2019

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

AnalyticsUtils.trackWithSiteDetails(Stat.EDITOR_GUTENBERG_ENABLED, site,
BlockEditorEnabledSource.ON_PROGRESSIVE_ROLLOUT.asPropertyMap());
// Show the info popup when the user creates a new post for the first time on this site
AppPrefs.setShowGutenbergInfoPopupForTheNewPosts(site.getUrl(), true);
}
}

// After enabling Gutenberg on these sites, we consider the user entered the rollout group
AppPrefs.setUserInGutenbergRolloutGroup();
}

if (!AppPrefs.isDefaultAppWideEditorPreferenceSet()) {
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ public class AnalyticsUtils {
public enum BlockEditorEnabledSource {
VIA_SITE_SETTINGS,
ON_SITE_CREATION,
ON_BLOCK_POST_OPENING;
ON_BLOCK_POST_OPENING,
ON_PROGRESSIVE_ROLLOUT;

public Map<String, Object> asPropertyMap() {
Map<String, Object> properties = new HashMap<>();
Expand Down