Skip to content

Limit Classic editor access on Simple WP.com sites to existing Classic posts#14381

Merged
cameronvoell merged 8 commits intodevelopfrom
gutenberg/remove-classic-on-simple-sites
Apr 2, 2021
Merged

Limit Classic editor access on Simple WP.com sites to existing Classic posts#14381
cameronvoell merged 8 commits intodevelopfrom
gutenberg/remove-classic-on-simple-sites

Conversation

@cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented Mar 31, 2021

The next step after notifying users that the classic editor is going away is to actually remove it, starting with Simple sites.

I think this change doesn't warrant inclusion in RELEASE-NOTES.txt since we already notified users about this change in 16.9, but if anyone thinks differently I'd love to hear. Perhaps we could include it in release notes but exclude it from the Play Store version notes.

Addresses wordpress-mobile/gutenberg-mobile#3092 (for WPAndroid)

To test

Note: The toggle switch referred to below is the "Use block editor" toggle switch found in My Site → Settings. This is only available to site admins.

  1. With APK from this PR, log into a WP.com account
  2. Choose a Simple site where you are an admin
  3. Verify that there is no toggle switch
  4. Using the current Play Store version of the app – not the app built from this branch – log into the same WP.com account
  5. Choose the same Simple site where you are an admin, locate the toggle switch and turn it off (meaning use the classic editor)
  6. Moving back over to the app built from this branch – locate the same Simple site
  7. Create a new post and ensure that the block editor is shown (not the classic editor)

Regression Notes

  1. Potential unintended areas of impact

This PR removes the classic editor on Simple sites. However, we're leaving it in place for scenarios where the block editor cannot be used instead. These scenarios include editing a classic post (i.e. a post that contains only classic content). We have to check to make sure these scenarios still work on Simple sites. We should also check to make sure all other site types (Atomic/Jetpack/self-hosted) are unaffected by this change.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • Test Simple WP.com sites do not show a toggle switch (used camiams.home.blog)
  • Test Simple WP.com sites defaulted on mobile to Classic use Gutenberg for new posts (used camiams.home.blog)
  • Regression test Atomic sites show a toggle switch (https://americanswimming9.wpcomstaging.com/)
  • Regression test Jetpack-connected self-hosted sites show a toggle switch (used coderovers.io)
  • Regression test self-hosted sites show a toggle switch (used codrovers.io)
  • Regression test Simple WP.com site users open existing Classic posts in the Classic editor (camiams.home.blog)
  1. What automated tests I added (or what prevented me from doing so)

We are disabling the Classic Editor tests until they can be migrated to executing on existing classic posts, instead of creating new ones. That work is tracked with this ticket here: #14389

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 Mar 31, 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 Mar 31, 2021

You can test the changes on this Pull Request by downloading the APK here.

@cameronvoell cameronvoell force-pushed the gutenberg/remove-classic-on-simple-sites branch from d27a7a2 to 02e06f2 Compare March 31, 2021 23:24
@cameronvoell cameronvoell marked this pull request as ready for review April 1, 2021 00:24
@guarani guarani self-requested a review April 1, 2021 00:27
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Simple sites using the test steps above and this looks good!
The code looks good from my point of view, but I'm not an Android dev 😄

nit pick: there's a mention of App Store above that probably refers to the Play Store

@cameronvoell
Copy link
Contributor Author

@mchowning An additional review on this would be welcome since after Paul approved, I added the plan we discussed for disabling classic tests until they are migrated to executing on an existing classic post. 🙏

return true;
} else {
return site.getMobileEditor().equals(SiteUtils.GB_EDITOR_NAME);
boolean isSimpleWPCom = site.isWPCom() && !site.isWPComAtomic();
Copy link
Contributor

@mchowning mchowning Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocking issue, but what would you think about instead extracting out something along these lines:

public static boolean alwaysDefaultToGutenberg(SiteModel site) {
  return site.isWPCom() && !site.isWPComAtomic();
}

and then using that method both here and at SiteSettingsFragment::1005?

My thinking is that we always want those two checks to be the same (anytime the editor toggle is unavailable we want to also want to force the default editor to be Gutenberg), and having the logic in a shared method makes it more likely they'll stay in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice suggestion, added here: 8d7b3dd

@mchowning
Copy link
Contributor

This all looks good to me. 👍 I had one comment, but it's not a blocker.

@cameronvoell cameronvoell merged commit e3bf6a0 into develop Apr 2, 2021
@cameronvoell cameronvoell deleted the gutenberg/remove-classic-on-simple-sites branch April 2, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants