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

Turn off Developer Tools when user preference is disabled #4955

Merged
merged 17 commits into from
Jul 1, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jun 30, 2020

Summary

Fixes #2673.
Fixes #4480.
See #2069.

To accompany the “technical user” question in the onboarding wizard, there is now a Developer Tools preference on the user edit screen:

image

This checkbox is only presented to users who can manage_options. Actually, the checkbox is presented to users who can amp_validate, a new meta capability that is mapped to the manage_options primitive capability by default. This allows a site to customize which users can access developer tools, but by default it is limited to administrators (whereas currently it is anyone who can edit_posts).

As opposed to the DevTools toggle being null by default, it is now true by default. Admins can turn it off if they don't want it. The first time the wizard is entered, it does not consider the current user's preferences so it will ask whether they are technical without auto-selecting one option.

When an administrator has DevTools turned off, then the admin menu items for Validated URLs and Validation Errors do not appear:

Before After
image image

They are also hidden in the At a Glance widget on the Dashboard:

Before After
image image

And the admin bar simply shows a link back to the non-AMP version, with no Validate link, CSS usage info, or paired browsing (aside: the link text is not entirely intuitive here):

Before After
image image

Nevertheless, even though an administrator has turned off DevTools, they can still access them manually by going to /wp-admin/edit.php?post_type=amp_validated_url. All the screens then show up in the admin menu. This will be critical for allowing administrators to access the validation screens in response to the Site Health test proposed in #1756 (comment).

Naturally, when Developer Tools are turned off, no validation warning messages will appear in the editor. Additionally, no synchronous validation is performed during saving. (Scheduling a job to validate an edit to a published post needs to be done as part of #1756.) This will address the slowness of synchronous loopback requests (#2069) for non-administrators and admins with DevTools turned off.

Additionally, loopback requests to perform validation are also no longer performed when activating a new plugin if the user has DevTools turned off.

Lastly, when DevTools are enabled they will also now apply in Reader mode for the first time.

image

As part of this, the admin bar is now also shown in the Reader mode template. Please note that in order to support scripts that are added for the admin bar, this PR introduces wp_print_head_scripts() at amp_post_template_head and wp_print_footer_scripts() at amp_post_template_footer. Nevertheless, wp_enqueue_scripts() is not called so it's up to plugins that want to extend the sidebar in legacy Reader mode to enqueue the scripts at the admin_bar_init action.

While Developer Tools are restricted to administrator users who can amp_validate which is mapped to manage_options by default, this can be overridden for example to let editors access dev tools via code like this:

add_filter(
	'map_meta_cap',
	function ( $caps, $cap, $user_id ) {
		if ( 'amp_validate' === $cap && user_can( $user_id, 'edit_others_posts' ) ) {
			$position = array_search( $cap, $caps, true );
			if ( false !== $position ) {
				$caps[ $position ] = 'edit_others_posts';
			}
		}
		return $caps;
	},
	10,
	3
);

That then moves the DevTools to the top-level menu item:

image

image

Todo

  • Add more tests.

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

…cess

* Add dev tools checkbox on the user edit screen.
* Map amp_validate to manage_options by default.
* Use amp_validate capability for editing amp_validated_url posts and amp_validation_error terms.
* Allow access to dev tools if user has the capability but hide the UI if not enabled.
* Hide validation info from At a Glance on the Dashboard if dev tools not enabled.
* Prevent showing Validate link, validation info, and paired browsing in AMP admin bar menu if dev tools not enabled.
* Let users who can manage_options have dev tools enabled by default.
Also removes test whose commit (18b85ea) indicates the code was indeed dead. From #1242.
* Remove obsolete notice taking user to hidden developer tools when Reader mode is selected.
* Prevent validating after plugin activation if dev tools is not enabled.
@westonruter westonruter added this to the v1.6 milestone Jun 30, 2020
@googlebot googlebot added the cla: yes Signed the Google CLA label Jun 30, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jun 30, 2020

Plugin builds for 642f85e are ready 🛎️!

Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

This is super good. I went over the various scenarios, via the setup wizard, from the user profile editing screen, and the flow is intuitive. This feature represents a huge improvement in the UX, especially for non tech-savvy folks.

*
* @return DevToolsUserAccess
*/
private static function get_dev_tools_user_access() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this method is not being used anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in 74ae110

includes/validation/class-amp-validated-url-post-type.php Outdated Show resolved Hide resolved
includes/validation/class-amp-validation-manager.php Outdated Show resolved Hide resolved
tests/php/validation/test-class-amp-validation-manager.php Outdated Show resolved Hide resolved
tests/php/validation/test-class-amp-validation-manager.php Outdated Show resolved Hide resolved
tests/php/validation/test-class-amp-validation-manager.php Outdated Show resolved Hide resolved
tests/php/validation/test-class-amp-validation-manager.php Outdated Show resolved Hide resolved
@westonruter westonruter requested a review from pierlon July 1, 2020 04:00
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Really great job done here 👍

@westonruter westonruter merged commit 9248cac into develop Jul 1, 2020
@westonruter westonruter deleted the add/dev-tools-disabling branch July 1, 2020 04:25
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA
Projects
None yet
4 participants