Setup wizard: technical background screen by johnwatkins0 · Pull Request #4833 · ampproject/amp-wp

johnwatkins0 · 2020-06-10T17:07:16Z

Summary

This will resolve #4705. It creates the "Are you technical?" screen that currently looks like this:

Additional changes

Creates a AMP_User_Manager class to handle user-specific plugin features such as user meta and user capabilities.
Within that class, adds logic required to turn on amp developer tools by default for administrators or those with the amp_validate user capability. All other users are blocked from turning on developer tools at all.

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

westonruter · 2020-06-14T05:49:09Z

includes/options/class-amp-user-options.php

+	const USER_OPTIONS_KEY = 'amp_user_options';
+
+	/**
+	 * Key for the user option (stored in amp_features user meta) enabling or disabling developer tools.
+	 *
+	 * @var string
+	 */
+	const OPTION_DEVELOPER_TOOLS = 'developer_tools';
+
+	/**
+	 * Sets up hooks.
+	 */
+	public static function init() {
+		add_filter( 'amp_setup_wizard_data', [ __CLASS__, 'inject_setup_wizard_data' ] );
+		add_action( 'rest_api_init', [ __CLASS__, 'register_user_meta' ] );
+	}
+
+	/**
+	 * Registers user meta related to validation management.
+	 *
+	 * @since 1.6.0
+	 */
+	public static function register_user_meta() {
+		register_meta(
+			'user',
+			self::USER_OPTIONS_KEY,
+			[
+				'show_in_rest' => [
+					'schema' => [
+						'default'    => [
+							self::OPTION_DEVELOPER_TOOLS => 'unset',
+						],
+						'type'       => 'object',
+						'properties' => [
+							self::OPTION_DEVELOPER_TOOLS => [
+								'type' => 'string',
+								'enum' => [
+									'disabled',
+									'enabled',
+									'unset',
+								],
+							],
+						],
+					],
+				],
+				'single'       => true,
+				'type'         => 'object',
+			]
+		);
+	}


I think we can just have a simple scalar user meta key for amp_dev_tools_enabled. It can be a boolean because it will either be on or off. If the user cannot manage_options (or per #2673 amp_validate) then it will always be false. It would be true by default for users who have that capability. In other words, I don't think we need to have a separate option that controls whether dev tools are enabled or disabled by default for users.

…ard-technical-question

…zard-technical-question

…-wizard-technical-question

schlessera · 2020-06-17T02:06:13Z

I think that https://github.com/phpstan/phpstan/blob/master/phpstan.phar points to a build of the latest commit, so alternatively, we could switch to that one while waiting for the fixes to be released.

…-wizard-technical-question

johnwatkins0 · 2020-06-18T03:44:08Z

I've pushed updates to this, including merging in the dependency injection work. @westonruter Let me know if you think anything else needs to happen in this PR regarding validation capabilities. @schlessera The new class DevToolsUserAccess is added via the new infrastructure.

schlessera · 2020-06-18T04:06:00Z

src/Admin/DevToolsUserAccess.php

+	 * Sets up hooks.
+	 */
+	public function register() {
+		add_action( 'rest_api_init', [ $this, 'register_rest_field' ] );


As this service is only needed for REST requests, it would be preferrable to make it "delayed" to only register at the rest_api_init action.

Right now, it always instantiates and then hooks into rest_api_init itself.

But for cases like this, the service container has smart behavior that adds an optimization.
If you add the Delayed interface with a registration action of rest_api_init instead, it will only instantiate at the rest_api_init action instead. So, as the instantiation itself is moved to that action, the service will not instantiate when the current request is not a REST request.

Yes, that makes sense. I've updated to add Delayed in cbf11ad. With this approach, I suppose there's no need to add callbacks to the rest_api_init hook, so I also changed register to call the register_rest_field method directly.

Yes, exactly, that's how I would have built it as well.

Oh, I assumed now you meant you've used the WordPress register_rest_field() method immediately within DevToolsUserAccess::register(). There's no need for the DevToolsUserAccess::register_rest_field() method anymore, that is just a superfluous method call overhead.

I've added it back in because we will need it later, to be able to add the user profile field.

schlessera

This is what I originally meant.

schlessera · 2020-06-18T13:58:55Z

src/Admin/DevToolsUserAccess.php

+	 * @action rest_api_init
+	 */
+	public function register() {
+		$this->register_rest_field();


No need for a bogus method call overhead here, you can just do the registration of the field directly within the register(), there's not much else to do anyway...

Suggested change

$this->register_rest_field();

register_rest_field(

'user',

self::USER_FIELD_DEVELOPER_TOOLS_ENABLED,

[

'get_callback' => [ $this, 'rest_get_dev_tools_enabled' ],

'update_callback' => [ $this, 'rest_update_dev_tools_enabled' ],

'schema' => [

'description' => __( 'Whether the user has enabled dev tools.', 'amp' ),

'type' => 'boolean',

],

]

);

Yes, good point! Updated in 71b8363

schlessera · 2020-06-18T13:59:48Z

src/Admin/DevToolsUserAccess.php

+
+	/**
+	 * Registers a rest field corresponding to the dev tools enabled user meta field.
+	 *
+	 * @since 1.6.0
+	 */
+	public function register_rest_field() {
+		register_rest_field(
+			'user',
+			self::USER_FIELD_DEVELOPER_TOOLS_ENABLED,
+			[
+				'get_callback'    => [ $this, 'rest_get_dev_tools_enabled' ],
+				'update_callback' => [ $this, 'rest_update_dev_tools_enabled' ],
+				'schema'          => [
+					'description' => __( 'Whether the user has enabled dev tools.', 'amp' ),
+					'type'        => 'boolean',
+				],
+			]
+		);
+	}


No need for this anymore then.

Suggested change

/**

* Registers a rest field corresponding to the dev tools enabled user meta field.

*

* @since 1.6.0

*/

public function register_rest_field() {

register_rest_field(

'user',

self::USER_FIELD_DEVELOPER_TOOLS_ENABLED,

[

'get_callback' => [ $this, 'rest_get_dev_tools_enabled' ],

'update_callback' => [ $this, 'rest_update_dev_tools_enabled' ],

'schema' => [

'description' => __( 'Whether the user has enabled dev tools.', 'amp' ),

'type' => 'boolean',

],

]

);

}

…705-setup-wizard-technical-question * 'develop' of github.com:ampproject/amp-wp: (77 commits) Supply DEFAULT_BASE_BRANCH in .travis.yml to fix wp-dev-lib issue Further fix PluginSuppressionTest Fix test logic for updating suppressed plugins Use simple comparison for version change since false causes PHP 7.4 error Update dependency autoprefixer to v9.8.1 Update amphtml to v2006112352000 Update dependency @babel/cli to v7.10.3 Eliminate long-deprecated AMP_Blacklist_Sanitizer Sync amp-toolbox-test-suite Remove obsolete scripts to create-gutenberg-test-pos Eliminate removable references to ~master~ branch Improve variable name and comment Centralize option sanitization logic into Service Update Suppressed Plugins description Add remaining tests for PluginSuppression Account for widget_display_callback filter not being in the_widget() prior to WP 5.3 Account for block editor not being present Add majority of tests for PluginSuppression; account for the_widget() Update dependency moment to v2.27.0 Configure ServiceBasedPlugin for plugin use instead of theme use ...

westonruter · 2020-06-20T04:42:34Z

src/Admin/DevToolsUserAccess.php

+	 * @return string Registration action to use.
+	 */
+	public static function get_registration_action() {
+		return 'rest_api_init';


If not in this PR, in a subsequent PR we'll need to expand this so that we can also register a user profile field. We should perhaps not use Delayed in that case.

Done in 307aa8c

westonruter · 2020-06-20T04:46:41Z

src/Admin/DevToolsUserAccess.php

+			);
+		}
+
+		return update_user_meta( $user->ID, self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, boolval( $new_value ) );


Rather than storing a boolean, WordPress core stores a string "true" and "false" for user options:

This gets around the problem of storing a true boolean value in a MySQL TEXT field. I think we should do the same.

Suggested change

return update_user_meta( $user->ID, self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, boolval( $new_value ) );

return update_user_meta( $user->ID, self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, wp_json_encode( (bool) $new_value );

Done in 3139da7.

Something to think about is that when a user is created in WordPress, it will by default supply all the user meta for, including those boolean options. So it will set the meta for rich_editing, syntax_highlighting, show_admin_bar_front, and comment_shortcuts. See the code in wp_insert_user(): https://github.com/WordPress/wordpress-develop/blob/1db25e3d3841e47e07eb71bf2b2c609cb37e71a1/src/wp-includes/user.php#L1763-L1774

That being the case, it's different that we are not providing an initial value here. Nevertheless, I think it's probably the right call because we don't know what the user's default choice should be.

westonruter · 2020-06-20T04:50:02Z

src/Admin/DevToolsUserAccess.php

+		$meta = get_user_meta( $user['id'] );
+
+		if ( is_array( $meta ) && array_key_exists( self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, $meta ) ) {
+			return boolval(
+				is_array( $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ] ) && ! empty( $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ] )
+					? reset( $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ] )
+					: $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ]
+			);
+		}


Per below, this could then be simplified to:

Suggested change

$meta = get_user_meta( $user['id'] );

if ( is_array( $meta ) && array_key_exists( self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, $meta ) ) {

return boolval(

is_array( $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ] ) && ! empty( $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ] )

? reset( $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ] )

: $meta[ self::USER_FIELD_DEVELOPER_TOOLS_ENABLED ]

);

}

$meta_value = get_user_meta( $user['id'], self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, true );

if ( '' !== $meta_value ) ) {

return rest_sanitize_boolean( $meta_value );

}

Done in 3139da7

johnwatkins0 added 7 commits June 9, 2020 16:41

Add user option and create user option provider component

faa9603

Add technical background page

ea8a124

Add phpunit tests

da88496

Include user options changes in wizard unsaved changes warning

45eb693

User options provider parameter updates

658dffb

Add file comment on user icons svg file

a78dcf7

Setup wizard technical background: minor updates

e95c653

googlebot added the cla: yes Signed the Google CLA label Jun 10, 2020

johnwatkins0 changed the title ~~Feature/4705 setup wizard technical question~~ Setup wizard: technical background screen Jun 10, 2020

Base automatically changed from feature/4702-choose-reader-theme to develop June 12, 2020 16:43

westonruter reviewed Jun 14, 2020

View reviewed changes

westonruter added this to the v1.6 milestone Jun 15, 2020

johnwatkins0 changed the base branch from develop to feature/4811-visual-polish June 15, 2020 14:24

Merge branch 'feature/4811-visual-polish' into feature/4705-setup-wiz…

a757865

…ard-technical-question

johnwatkins0 self-assigned this Jun 15, 2020

Merge branch 'feature/4812-error-boundary' into feature/4705-setup-wi…

bcc40f0

…zard-technical-question

johnwatkins0 added the Blocked label Jun 15, 2020

johnwatkins0 added 2 commits June 15, 2020 12:46

Fix reader REST endpoint

81ba707

Unit tests

866c4a6

Base automatically changed from feature/4811-visual-polish to develop June 15, 2020 23:05

johnwatkins0 added 10 commits June 15, 2020 20:10

Technical background selection

140f060

Merge remote-tracking branch 'origin/develop' into feature/4705-setup…

fa921e2

…-wizard-technical-question

Text casing updates

db2a7c8

Merge remote-tracking branch 'origin/develop' into feature/4705-setup…

c80489a

…-wizard-technical-question

Rename user manager class

fa41cc0

Change verb tense of state variables

8c67d31

Fix REST endpoint

fbbb9ce

Clarify phpunit tests

176f830

Test updates

d8c7922

Remove stepper current item bg color

7bb916c

johnwatkins0 added 5 commits June 17, 2020 20:45

Update tests

a22d7c5

Merge remote-tracking branch 'origin/develop' into feature/4705-setup…

2ad9d9a

…-wizard-technical-question

phpcs fix

0094372

Improve comments

505cefc

phpstan updates

b32acbb

johnwatkins0 requested review from schlessera and westonruter June 18, 2020 03:36

johnwatkins0 requested a review from pierlon June 18, 2020 03:44

schlessera requested changes Jun 18, 2020

View reviewed changes

Make DevToolsUserAccess delayed

cbf11ad

johnwatkins0 requested a review from schlessera June 18, 2020 13:41

johnwatkins0 added 2 commits June 18, 2020 08:42

Remove register hook check in PHPUnit test

328eb2d

Improve accuracy of description in schema

1aa79d4

schlessera requested changes Jun 18, 2020

View reviewed changes

Simplify DevtoolsUserAccess registration

71b8363

johnwatkins0 requested a review from schlessera June 18, 2020 14:29

johnwatkins0 mentioned this pull request Jun 18, 2020

Setup wizard: Mode selection screen #4883

Closed

3 tasks

westonruter and others added 3 commits June 19, 2020 17:07

Update jsdoc for UserContextProvider; improve tech question description

4f2f049

Update Jest snapshot

60d510b

westonruter reviewed Jun 20, 2020

View reviewed changes

westonruter added 2 commits June 19, 2020 22:18

Opt out of using Delayed for DevToolsUserAccess

307aa8c

Store true/false strings in usermeta for amp_dev_tools_enabled

3139da7

westonruter approved these changes Jun 20, 2020

View reviewed changes

westonruter merged commit dc29d81 into develop Jun 20, 2020

westonruter deleted the feature/4705-setup-wizard-technical-question branch June 20, 2020 05:48

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

	return update_user_meta( $user->ID, self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, boolval( $new_value ) );
	return update_user_meta( $user->ID, self::USER_FIELD_DEVELOPER_TOOLS_ENABLED, wp_json_encode( (bool) $new_value );

Conversation

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

Uh oh!

Summary

Additional changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera commented Jun 17, 2020

Uh oh!

johnwatkins0 commented Jun 18, 2020

Uh oh!

schlessera Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

schlessera left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnwatkins0 commented Jun 10, 2020 •

edited

Loading

schlessera Jun 18, 2020 •

edited

Loading

johnwatkins0 Jun 18, 2020 •

edited

Loading