Initialize plugin_configured as true if plugin options are not empty by johnwatkins0 · Pull Request #5042 · ampproject/amp-wp

johnwatkins0 · 2020-07-14T15:09:30Z

Summary

For the setup wizard, we added a plugin_configured option. When false, certain values are initialized to null in the UI until the user makes decisions about plugin configuration.

We need to skip that behavior if the plugin was already configured at some point before plugin_configured was added.

With this change, when the plugin options from the database are not empty, we set plugin_configured to true. As a result, the UI will reflect previously selected options instead of initializing any fields to null.

Test steps

Switch to a pre-1.6 version of the plugin -- e.g., checkout 1.5.5 -- reinstall composer and node modules, and rebuild
Clear your options: wp option delete amp-options
Visit the plugin settings page and save some settings.
Checkout this branch, reinstall composer and node modules, and rebuild
Visit the plugin settings page and confirm that your settings are not empty. They should also be reflected in the wizard

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2020-07-14T15:27:38Z

Plugin builds for 19fe024 are ready 🛎️!

Download development build
Download production build

johnwatkins0 · 2020-07-14T16:09:36Z

I need to check the failing e2e test.

westonruter · 2020-07-14T18:13:32Z

includes/amp-helper-functions.php

+	$options = get_option( AMP_Options_Manager::OPTION_NAME, [] );
+
+	// Initialize the plugin_configured option, setting it to true if options already exist.
+	if ( is_admin() && current_user_can( 'manage_options' )  ) {
+		if ( empty( $options ) ) {
+			AMP_Options_Manager::update_option( Option::PLUGIN_CONFIGURED, false );
+		} elseif ( ! isset( $options[ Option::PLUGIN_CONFIGURED ] ) ) {
+			AMP_Options_Manager::update_option( Option::PLUGIN_CONFIGURED, true );
+		}
+	}
+


I think we can handle this with default values in AMP_Options_Manager::get_options(). I'll make the change.

Go for it. I'll leave it alone.

Does afaf9a6 do the trick?

Not quite…

That's what I tried at first but it didn't work, I think because of this: https://github.com/ampproject/amp-wp/blob/develop/includes/amp-helper-functions.php#L145-L160

Once AMP_Options_Manager::update_option( Option::VERSION, AMP__VERSION ); is called, the options will no longer be empty, and that runs way before get_options is called again.

That's why I was trying to squeeze in the initialization of plugin_configured before that point.

It seems to be working for me.

If I reset the option to an old value:

echo '{"theme_support":"transitional","supported_post_types":["post"],"analytics":[],"all_templates_supported":false,"supported_templates":["is_singular"],"version":"1.5.5"}' | wp option set amp-options --format=json

Then:

$ wp eval 'var_dump(AMP_Options_Manager::get_option("plugin_configured"));' bool(true)

Whereas if I delete the option entirely:

$ wp option delete amp-options Success: Deleted 'amp-options' option.

Then I get:

$ wp eval 'var_dump(AMP_Options_Manager::get_option("plugin_configured"));' bool(false)

Hm, all right, I'll verify when I make the change for the settings page.

westonruter · 2020-07-14T18:30:32Z

@johnwatkins0 One more thing:

When deleting the amp-options setting, the admin screen should show up with the default values populating the form, as opposed to now:

The wizard, however, should have its fields empty if plugin_configured is false.

westonruter · 2020-07-14T18:39:21Z

The wizard, however, should have its fields empty if plugin_configured is false.

I think we need to pass some flag in \AmpProject\AmpWP\Admin\OptionsMenu::enqueue_assets() in POPULATE_DEFAULT_VALUES which will force the components to populate the values even if ! fetchedOptions.plugin_configured. So including something like this:

diff --git a/assets/src/components/options-context-provider/index.js b/assets/src/components/options-context-provider/index.js
index 678ab0205..7990d4ef2 100644
--- a/assets/src/components/options-context-provider/index.js
+++ b/assets/src/components/options-context-provider/index.js
@@ -68,7 +68,7 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 					return;
 				}
 
-				if ( fetchedOptions.plugin_configured === false ) {
+				if ( ! POPULATE_DEFAULT_VALUES && fetchedOptions.plugin_configured === false ) {
 					fetchedOptions.mobile_redirect = true;
 					fetchedOptions.reader_theme = null;
 					fetchedOptions.theme_support = null;
diff --git a/src/Admin/OptionsMenu.php b/src/Admin/OptionsMenu.php
index 828f62b0b..46e614776 100644
--- a/src/Admin/OptionsMenu.php
+++ b/src/Admin/OptionsMenu.php
@@ -241,6 +241,7 @@ class OptionsMenu implements Conditional, Service, Registerable {
 			],
 			'OPTIONS_REST_ENDPOINT'              => rest_url( 'amp/v1/options' ),
 			'READER_THEMES_REST_ENDPOINT'        => rest_url( 'amp/v1/reader-themes' ),
+			'POPULATE_DEFAULT_VALUES'            => true,
 			'IS_CORE_THEME'                      => in_array(
 				get_stylesheet(),
 				AMP_Core_Theme_Sanitizer::get_supported_themes(),

westonruter · 2020-07-14T19:08:06Z

I think we need to pass some flag in \AmpProject\AmpWP\Admin\OptionsMenu::enqueue_assets() in POPULATE_DEFAULT_VALUES which will force the components to populate the values even if ! fetchedOptions.plugin_configured. So including something like this:

I'm giving this a shot.

…when not set up

westonruter · 2020-07-14T19:20:10Z

@johnwatkins0 there, please check cdc872e.

Here's the matrix for how the wizard and settings screen look (as expected):

Screen	Options Empty (new install)	Pre-populated Options (upgrading from 1.5.5)
Settings
Wizard

westonruter · 2020-07-14T19:35:57Z

(I'll need help with adapting/adding E2E tests.)

johnwatkins0 · 2020-07-14T19:35:57Z

@westonruter The code looks right. I'll fix these e2e tests that are now expectedly failing. But -- isn't it supposed to default to Reader mode when options are empty? That's how mine works locally, which differs from your screenshot.

johnwatkins0 · 2020-07-14T19:36:47Z

For me, on initial load with empty options, the settings screen has Reader mode selected with the legacy theme selected.

westonruter · 2020-07-14T19:45:56Z

If you have an AMP-compatible theme active, then the default mode should be Standard or Transitional per this logic here:

amp-wp/includes/options/class-amp-options-manager.php

Lines 96 to 104 in 9f7b74c

    
           // Migrate legacy method of specifying the mode. 
        
           $theme_support = AMP_Theme_Support::get_theme_support_args(); 
        
           if ( $theme_support && ! isset( $options[ Option::THEME_SUPPORT ] ) ) { 
        
           	if ( empty( $theme_support[ AMP_Theme_Support::PAIRED_FLAG ] ) ) { 
        
           		$defaults[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG; 
        
           	} else { 
        
           		$defaults[ Option::THEME_SUPPORT ] = AMP_Theme_Support::TRANSITIONAL_MODE_SLUG; 
        
           	} 
        
           }

What theme do you have active?

westonruter · 2020-07-14T19:52:15Z

I see there is also a bug in AMP_Core_Theme_Sanitizer::extend_theme_support() in that it will fail to add amp theme support if a core theme doesn't have any special theme support args. This resulted in Twenty Sixteen getting Transitional mode by default, but Twenty Twenty did not. I'll fix.

johnwatkins0 · 2020-07-14T19:53:18Z

Yeah, I was using Twenty Twenty and getting Reader by default.

westonruter · 2020-07-14T20:16:50Z

@johnwatkins0 Please check if 677c20e fixes the issue with the expected default mode.

johnwatkins0 · 2020-07-14T20:29:34Z

@johnwatkins0 Please check if 677c20e fixes the issue with the expected default mode.

Success:

Using theme twenty twenty
Pulled branch and rebuilt
wp option delete amp-options
Visit plugin settings screen
Transitional selected

…e active

westonruter · 2020-07-14T20:40:10Z

There's one more bug I found that was missed from #5010: when a child theme of a core theme is active, we should not default to Transitional or Standard because all bets are off as to whether it actually supports AMP. Fixed in f8c8390. We'll only be able to change that once we have the theme scan feature (#4795).

Initialize plugin_configured as true if plugin options are not empty

8bfd162

googlebot added the cla: yes Signed the Google CLA label Jul 14, 2020

johnwatkins0 marked this pull request as ready for review July 14, 2020 15:24

johnwatkins0 self-assigned this Jul 14, 2020

Clear options before plugin notice test

f14a17f

johnwatkins0 requested review from pierlon and westonruter and removed request for pierlon and westonruter July 14, 2020 15:55

johnwatkins0 added 2 commits July 14, 2020 12:24

Initialize plugin_configured earlier

43bce66

Only initialize plugin configured in admin

59b5846

westonruter reviewed Jul 14, 2020

View reviewed changes

westonruter added 2 commits July 14, 2020 11:17

Move overriding default PLUGIN_CONFIGURED to get_options

afaf9a6

Add tests for plugin_configured default value

4c85412

Fix NewFunctionCallTrailingComma.FoundInFunctionCall

0d8de79

Populate settings screen with default values, but leave wizard blank …

cdc872e

…when not set up

johnwatkins0 and others added 2 commits July 14, 2020 15:04

Fix and remove soon-to-be-obsolete e2e tests

7a1eab1

Fix extending core theme support when theme lacks special args

677c20e

Add missing theme test data

384f6eb

Prevent defaulting to Transitional mode when child theme of core them…

f8c8390

…e active

westonruter mentioned this pull request Jul 14, 2020

Revamp notice appearing in AMP Enabled toggle, revamp how plugin determines whether AMP can be disabled #5010

Merged

3 tasks

westonruter approved these changes Jul 14, 2020

View reviewed changes

westonruter added this to the v1.6 milestone Jul 14, 2020

Fix regression in get_options due to placement of variable assignment

19fe024

westonruter merged commit 6c5cca2 into develop Jul 14, 2020

westonruter deleted the fix/5020-empty-settings-on-upgrade branch July 14, 2020 21:15

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020

johnwatkins0 added the WS:UX Work stream for UX/Front-end label Aug 12, 2020

Conversation

johnwatkins0 commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test steps

Checklist

Uh oh!

github-actions bot commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnwatkins0 commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

johnwatkins0 commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnwatkins0 commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

johnwatkins0 commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

johnwatkins0 commented Jul 14, 2020

Uh oh!

westonruter commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnwatkins0 commented Jul 14, 2020 •

edited

Loading

github-actions bot commented Jul 14, 2020 •

edited

Loading

johnwatkins0 commented Jul 14, 2020 •

edited

Loading