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

Move editor preference option to Site Settings screen #10254

Merged

Conversation

@daniloercoli
Copy link
Contributor

commented Jul 19, 2019

Move the "default editor for new posts" toggle from App Settings screen to Site Settings screen.

Details here: wordpress-mobile/gutenberg-mobile#1233

Test 1 - Account already migrated to version 12.9 and that has seen the auto-enable dialog
(the app has the global option set to TRUE)

  • Open app settings and see there is no editor option
  • Go to site settings. The default editor preference is there and it should be TRUE
  • Change it to false
  • Re-open settings, it should show the correct value
  • Tap to create a new post, the right editor (Aztec) should be shown

Test 2 - Account already migrated to version 12.9 that has not yet seen the auto-enable dialog
(so basically an account that didn't open a GB based post in v12.9)

  • Open app settings and see there is no editor option
  • Go to site settings. The default editor preference is there and it should be false
  • Tap to create a new post, the right editor (Aztec) should be shown
  • Open a blocks based post
  • The dialog that informs of the auto-enable should appear
  • Re-open settings, it should show the correct value (true)
  • Tap to create a new post, the right editor (GB) should be shown.

Test 3 - Account already migrated to version 12.9 that has set the editor option to FALSE after the auto-opt in dialog.
(so basically an account that opened a GB based post in v12.9, but turned it OFF later)

  • Open app settings and see there is no editor option
  • Go to site settings. The default editor preference is there and it should be false
  • Tap to create a new post, the right editor (Aztec) should be shown
  • Open a blocks based post
  • NO dialog that informs of the auto-enable should appear
  • Re-open settings, it should show the correct value (false)
  • Tap to create a new post, the right editor (Aztec) should be shown.

Test 4 - NEW account

  • Open app settings and see there is no editor option
  • Go to site settings. The default editor preference is there and it should be true
  • Tap to create a new post, the right editor (GB) should be shown
  • Go to site settings and change it to false
  • Re-open settings, it should show the correct value
  • Tap to create a new post, the right editor (Aztec) should be shown.

Test 5 - Self hosted sites - Upgrading from 12.9 where the block editor was enabled globally

  • Open app settings and see there is no editor option
  • Go to site settings. The default editor preference is there and it should be true
  • Tap to create a new post, the right editor (GB) should be shown
  • Go to site settings and change it to false
  • Re-open settings, it should show the correct value
  • Tap to create a new post, the right editor (Aztec) should be shown

Test 6 - Self hosted sites - new install

  • Open app settings and see there is no editor option
  • Go to site settings. The default editor preference is there and it should be false
  • Tap to create a new post, the right editor (Aztec) should be shown
  • Go to site settings and change it to true
  • Re-open settings, it should show the correct value
  • Tap to create a new post, the right editor (GB) should be shown

Update release notes:

  • [ x ] If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

daniloercoli added some commits Jul 19, 2019

Migrate the editor setting to the the Site setting screen, and add th…
…e logic that does handle the migration, and the read/write of the property in site model.
Make sure to default to GB editor for new account on WPCOM. Old accou…
…nts and wporg sites will get the app preference setting and then default to Aztec.
Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…
…ss-Android into gb/move-editor-preference-toggle-to-site-settings

* 'develop' of https://github.com/wordpress-mobile/WordPress-Android:
  Bump version number
  Update release-toolkit
  Update release-toolkit to 0.6.0
  Update metadata strings
  Update App Store descriptions
  Update release notes for 12.9
  Revert setting editor preferecen to Remote
  Update to the latest release-toolkit
  Update metadata download lane
  Remove unused actions
  Update finalize lane
  Remove redundant check

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/prefs/AppSettingsFragment.java

@daniloercoli daniloercoli added this to the 13.0 milestone Jul 19, 2019

daniloercoli added some commits Jul 22, 2019

Make sure to check the web side of the editor setting, before showing…
… the block editor on new posts. Both of the web and the mobile settings need to be `gutenberg`.
The previous commit was taking in consideration only half of the back…
…end code. Let's make sure to flip to GB for new users accodingly to the backend.
Create a new Editor preference section, and move the toggle out "Writ…
…ing" to be able to show it on self-hosted sites also.
@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Tried:

WP.com/Jetpack sites

Create a new WP.com site on mobile and check Site Settings → should be Gutenberg
⚠️ Create a new WP.com site on mobile and start a new post → should be Gutenberg and show the info popup. Result: it's Gutenberg but no popup.

Clean app, clean backend, start new post → should be Aztec
Clean app, "Aztec" in backend, start new post → should be Aztec
Clean app, "Gutenberg" in backend, start new post → should be Gutenberg
Clean app, clean backend, open Gutenberg post → auto-enables Gutenberg
Clean app, clean backend, open Gutenberg post (auto-enables GB), start new post → should be Gutenberg
Clean app, clean backend, open Gutenberg post (auto-enables GB), open a GB post again→ Gutenberg but no popup
Clean app, clean backend, open Gutenberg post (auto-enables GB), new post in other site → should be Aztec
Clean app, clean backend, open Gutenberg post (auto-enables GB), open GB post in other site → auto-enables Gutenberg

Toggling the setting in a site doesn't toggle it in others

Migrate from v12.9 with GB ON, start new post → should be Gutenberg in all sites: seems to be Aztec on current site and Gutenberg on other site
Migrate from v12.9 with GB OFF and been auto-enabled, start new post → should be Aztec in all sites
Migrate from v12.9 with GB OFF and never auto-enabled, start new post → should be Aztec in all sites
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and start new post → should be Aztec
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and open GB post → auto-enables Gutenberg on the site

Selfhosted sites

Self-hosted site added, start new post → should be Aztec
Self-hosted site added, open a GB post → auto-enables Gutenberg
Self-hosted site added, open a GB post (should auto-enable GB), start new post → should be Gutenberg
Self-hosted site added, open a GB post (should auto-enable GB), clean app data, re-add self-hosted site, start new post → should be Aztec
Self-hosted site added, open a GB post (should auto-enable GB), clean app data, re-add self-hosted site, open a GB post → auto-enables Gutenberg

Migrate a selfhosted from v12.9 with GB OFF and been auto-enabled, start new post → should be Aztec in all selfhosted sites
Migrate from v12.9 with GB OFF and never auto-enabled, start new post → should be Aztec in all selfhosted sites
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and start new post → should be Aztec
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and open GB post → auto-enables Gutenberg on the site

@daniloercoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Migrate from v12.9 with GB ON, start new post → should be Gutenberg in all sites: seems to be Aztec on current site, Gutenberg after changing to other site.

Not sure what is happening here. For a short time the mSiteStore object in WPMainActivity is still returning the old version of the SiteModel for the current site. Even if you reload the site info from the DB after the "setDefaultEditor" network call returned with success for the current site, SiteModel site = mSiteStore.getSiteByLocalId(getSelectedSite().getId()); the mSiteStore does return an old version of the model.
If you switch tab, ie go to Reader, then go back to My site, it then has the updated version, and the app starts GB correctly.

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…
…ss-Android into gb/move-editor-preference-toggle-to-site-settings

* 'develop' of https://github.com/wordpress-mobile/WordPress-Android: (22 commits)
  Gutenberg mobile release v1.10.0 (#10286)
  Fix a problem reported in code review where the mHasUnsupportedBlocks variable was not properly initializated before accessing it
  Add unit test for content description helpet
  Remove string resource that was not translatable
  Replace inline function with boolean param
  Remove focusable=false
  Remove importantForAccessibility flag
  Remove needless blank line
  Show publish action on post pending review when user has publish rights
  Add tests for displayed buttons on post list items
  Disable graph for accessibility
  Disable graph legend for accessibility
  Improvements to the value and column items
  Rename build_test_releases lane
  Update fastlane
  Improvements for the Quick Scan items
  Improvements to the overview card
  Always add the property unsupported_blocks to session_start event
  Remove redundant local variable
  Track unsupported block lists at session_start only
  ...
@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

For a short time the mSiteStore object in WPMainActivity is still returning the old version of the SiteModel for the current site. Even if you reload the site info from the DB after the "setDefaultEditor" network call returned with success for the current site, SiteModel site = mSiteStore.getSiteByLocalId(getSelectedSite().getId()); the mSiteStore does return an old version of the model.
If you switch tab, ie go to Reader, then go back to My site, it then has the updated version, and the app starts GB correctly.

Hmm, not sure, maybe @planarvoid or have a clue of what might be happening here and the model object is not updating in time? Does that ring any bells perhaps?

daniloercoli added some commits Jul 28, 2019

Remove AppPrefs functions that were used to set/read the flag that go…
…verned the Block Editor popup.

With the migration in place, that happens as soon as the app starts, there is no big advantage to use that local flag. We can just check the "emptiness" of the mobileEditor preference on the site.
Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…
…ss-Android into gb/move-editor-preference-toggle-to-site-settings

* 'develop' of https://github.com/wordpress-mobile/WordPress-Android: (261 commits)
  Update release nots
  Remove click listener on non-clickable items
  Fix description on the collapse button in stats
  Update color references of primary from blue to wordpress blue
  Update color reference of success from green 5 (#a4f5c8) to green 50 (#008a20)
  Cleanup content description helper
  Use header instead of resources as content description helper params
  Fix tests
  Improve content description in the value item in stats
  Remove formatted string from content descriptions
  Add content description to all the list items
  Add version number to studio color section
  Remove hot color shades
  Update color references from hot red 900 (#70150c) to red 90 (#451313)
  Update color references from hot red 800 (#8e140c) to red 80 (#691c1c)
  Update color references from hot red 700 (#ac120b) to red 70 (#8a2424)
  Update color references from hot red 600 (#cb0c07) to red 60 (#b32d2e)
  Update color references from hot red 500 (#eb0001) to red 50 (#d63638)
  Update color references from hot red 400 (#ff4b1c) to red 40 (#e65054)
  Update color references from hot red 300 (#ff8248) to red 30 (#f86368)
  ...
@daniloercoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

For a short time the mSiteStore object in WPMainActivity is still returning the old version of the SiteModel for the current site. Even if you reload the site info from the DB after the "setDefaultEditor" network call returned with success for the current site, SiteModel site = mSiteStore.getSiteByLocalId(getSelectedSite().getId()); the mSiteStore does return an old version of the model.
If you switch tab, ie go to Reader, then go back to My site, it then has the updated version, and the app starts GB correctly.

Hmm, not sure, maybe @planarvoid or have a clue of what might be happening here and the model object is not updating in time? Does that ring any bells perhaps?

@hypest I've investigated the issue a bit more and seems that SiteModel returned by SiteStore doesn't always contain the correct value for mobile_editor. Even if the property is correctly stored on the backend. I guess there is a problem on FluxC somewhere, since the onSiteChanged event in WPMainActivity here: https://github.com/wordpress-mobile/WordPress-Android/blob/gb/move-editor-preference-toggle-to-site-settings/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java#L1136 could receive a model with empty value set for editor.
This is not happening all the time though, and once the value Gutenberg is received on the client app it always stays the same. FluxC will continue emitting onSiteChanged event with the correct value.

Anyway, I think we can investigate this better during the next week, and patch the RC.

@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

@daniloercoli , I had a look at the code and what the app is trying to do and I think there maybe is a race condition. The app will try to fetch the site editor setting (see here) while in the same time try to migrate the previous App setting (see here). The two network calls are in flight at the same time and their response will get saved to the DB. The "designate editor" call seems to finish first and the correct value is written. But then, the other call arrives, and overwrites the value. If you have cleared the backend then that's what stays (null).

I added some Logs to see the sequence of events and here's what I saw: (the network calls and DB saves favor the fetchSiteEditors path and the "un-migrated" backend setting is saved last)

2019-07-29 00:14:26.069 Launching designateMobileEditor
2019-07-29 00:14:26.155 Launching fetchSiteEditors
2019-07-29 00:14:27.273 got designate reply: gutenberg
2019-07-29 00:14:27.280 got fetchEditors reply: 
2019-07-29 00:14:27.295 saving to DB: gutenberg
2019-07-29 00:14:27.297 onSiteEditorsChanged https://stefanosuser.wordpress.com/wp-admin/, gutenberg
2019-07-29 00:14:27.298 saving to DB: 
2019-07-29 00:14:27.298 onSiteEditorsChanged https://stefanosuser.wordpress.com/wp-admin/, 
@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

⚠️ Create a new WP.com site on mobile and start a new post → should be Gutenberg and show the info popup. Result: it's Gutenberg but no popup.

Added a detail to the testcase which makes the test "yellow": we need to show the info popup when starting a new Gutenberg post on a new site.

@daniloercoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

⚠️ Create a new WP.com site on mobile and start a new post → should be Gutenberg and show the info popup. Result: it's Gutenberg but no popup.

Added a detail to the testcase which makes the test "yellow": we need to show the info popup when starting a new Gutenberg post on a new site.

Fixed in ff934cd

daniloercoli added some commits Jul 29, 2019

Listen on `onSiteEditorsChanged` in WPMainActiity to be sure the new …
…SiteModel is correctly updated into the app when the details of the mobile editor set is refreshed from the remote backend.

This also introduce a new FluxC hash, that fixes a problem on upgrading site details into the DB.
Make sure to reload editor details from the remote backend at startup…
…, together with site details.

This happens only when the migration v12.9 -> 13.0 has already happened.
Update FluxC hash and make sure to write the value of the editor imme…
…diatelly to the DB, before starting network calls.
@daniloercoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@daniloercoli , I had a look at the code and what the app is trying to do and I think there maybe is a race condition. The app will try to fetch the site editor setting (see here) while in the same time try to migrate the previous App setting (see here). The two network calls are in flight at the same time and their response will get saved to the DB. The "designate editor" call seems to finish first and the correct value is written. But then, the other call arrives, and overwrites the value. If you have cleared the backend then that's what stays (null).

I added some Logs to see the sequence of events and here's what I saw: (the network calls and DB saves favor the fetchSiteEditors path and the "un-migrated" backend setting is saved last)

These are the steps to replicate the (already fixed) problem

  1. install v12.9
  2. switch GB on in App Settings
  3. stay on the Me tab
  4. Run the latest branch via AS
  5. Tap on the big "new post" button at the bottom middle
  6. GB comes up
@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Trying the latest (8b2de41)

WP.com/Jetpack sites

Create a new WP.com site on mobile and check Site Settings → should be Gutenberg
Create a new WP.com site on mobile and start a new post → should be Gutenberg and show the info popup

Clean app, clean backend, start new post → should be Aztec
Clean app, "Aztec" in backend, start new post → should be Aztec
Clean app, "Gutenberg" in backend, start new post → should be Gutenberg
Clean app, clean backend, open Gutenberg post → auto-enables Gutenberg
Clean app, clean backend, open Gutenberg post (auto-enables GB), start new post → should be Gutenberg
Clean app, clean backend, open Gutenberg post (auto-enables GB), open a GB post again→ Gutenberg but no popup
Clean app, clean backend, open Gutenberg post (auto-enables GB), new post in other site → should be Aztec
Clean app, clean backend, open Gutenberg post (auto-enables GB), open GB post in other site → auto-enables Gutenberg

Toggling the setting in a site doesn't toggle it in others

⚠️ Migrate from v12.9 with GB ON, start new post → should be Gutenberg in all sites. Result: the value is set to GB in all sites indeed, but the first time a post is started is Aztec. We'll tackle this in a different PR.
Migrate from v12.9 with GB OFF and been auto-enabled, start new post → should be Aztec in all sites
Migrate from v12.9 with GB OFF and never auto-enabled, start new post → should be Aztec in all sites
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and start new post → should be Aztec
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and open GB post → auto-enables Gutenberg on the site

Selfhosted sites

Performed by Danilo in #10254 (comment)

@daniloercoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Tested with self-hosted sites!

Self-hosted site added, start new post → should be Aztec
Self-hosted site added, open a GB post → auto-enables Gutenberg
Self-hosted site added, open a GB post (should auto-enable GB), start new post → should be Gutenberg
Self-hosted site added, open a GB post (should auto-enable GB), clean app data, re-add self-hosted site, start new post → should be Aztec
Self-hosted site added, open a GB post (should auto-enable GB), clean app data, re-add self-hosted site, open a GB post → auto-enables Gutenberg

Migrate a selfhosted from v12.9 with GB OFF and been auto-enabled, start new post → should be Aztec in all selfhosted sites
Migrate from v12.9 with GB OFF and never auto-enabled, start new post → should be Aztec in all selfhosted sites
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and start new post → should be Aztec
Migrate from v12.9 with GB OFF and never auto-enabled, open a GB post (should auto-enable GB), change to other site and open GB post → auto-enables Gutenberg on the site

@hypest

hypest approved these changes Jul 29, 2019

Copy link
Contributor

left a comment

LGTM @daniloercoli !

@hypest hypest assigned daniloercoli and unassigned daniloercoli Jul 29, 2019

@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

The WPiOS PR is also approved now so, merging these both.

@daniloercoli

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Great team working on this PR!

@daniloercoli daniloercoli merged commit 8163d8d into develop Jul 29, 2019

4 checks passed

Peril All green. Congrats.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@daniloercoli daniloercoli deleted the gb/move-editor-preference-toggle-to-site-settings branch Jul 29, 2019

@jtreanor jtreanor referenced this pull request Jul 30, 2019
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.