Do async site scan on plugin activation instead of synchronous validation by delawski · Pull Request #6685 · ampproject/amp-wp

delawski · 2021-11-04T18:04:08Z

Summary

Screen.Recording.2021-11-23.at.09.37.04.PM.mp4

This PR renders an admin notice box whenever a plugin is activated. The admin notice starts a full Site Scan immediately and presents validation results as soon as the scan is complete:

Scan in progress after plugin activation	Scan complete - no plugin issues

Scan complete - URLs list collapsed	Scan complete - URLs list expanded

Note that there is a slight difference in the implementation compared to the design. After Site Scan is done, we're listing out all plugins causing AMP incompatibilities along with the URLs on which the issues were discovered. Also, a CTA to AMP compatible plugins is not hidden inside the <details> element but it's rather outside of it (since there might be multiple <details> elements in a single notice.

According to the design, we should be showing only issues related to the plugin (or multiple plugins) that has just been activated. The CTA should be collapsed by default inside the <details> element.

On one hand, the designed approach would be more complex in terms of the implementation. Since Core doesn't store a list of plugins that were last activated (or is it?), we would have to persist it on our own, probably using the Options API. On the other hand, though, from a user point of view, I believe it's better to let a user know about every plugin that causes AMP validation errors, not just the one that has been activated.

In any case, I'm happy to change the implementation so that it matches the design.

Implementation Details

A new JS entry point has been added to the admin group: amp-site-scan-notice.
Since the admin notice requires the validation issues to be grouped by a different "dimension", i.e. not by a URL but rather a plugin, the getSlugsFromValidationResults helper had to be transformed into a more robust getSourcesFromScannableUrls function. The arrays returned by getSlugsFromValidationResults contained just the plugin/theme slugs like [ 'amp', 'pwa' ]. With getSourcesFromScannableUrls, the returned arrays also contain a list of URLs where the issues were found, e.g. [ { slug: 'amp', urls: [ ... ] }, { slug: 'pwa', urls: [ ... ] } ].
The PHP code related to the post-plugin-activation admin notice has been refactored out of class-amp-validation-manager.php and into a new class PluginActivationSiteScan.

Checklist

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

…gins

delawski · 2021-12-01T12:54:34Z

@westonruter I've updated the post-scan notice according to your suggestions. Also, the message for non-technical users has been updated.

One plugin causing AMP incompatibilities

Technical user	Non-technical user

Two or more plugins causing AMP incompatibilities

Technical user	Non-technical user

milindmore22 · 2021-12-01T17:11:26Z

When I tried to activate any plugin, I got the following error

{
  "message": "destroy is not a function",
  "stack": "TypeError: destroy is not a function\n    at commitHookEffectListUnmount (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:19845:13)\n    at commitPassiveHookEffects (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:19903:13)\n    at HTMLUnknownElement.callCallback (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:182:16)\n    at Object.invokeGuardedCallbackDev (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:231:18)\n    at invokeGuardedCallback (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:286:33)\n    at flushPassiveEffectsImpl (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:22988:11)\n    at unstable_runWithPriority (https://amp-local.test/wp-includes/js/dist/vendor/react.js?ver=16.13.1:2685:14)\n    at runWithPriority$1 (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:11174:12)\n    at flushPassiveEffects (https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:22955:14)\n    at https://amp-local.test/wp-includes/js/dist/vendor/react-dom.js?ver=16.13.1:22834:13"
}

delawski · 2021-12-02T13:39:11Z

@milindmore22 Thank you for the report. I'm not able to reproduce the error. It happens only if Gutenberg is not activated. I'm investigating it now.

delawski · 2021-12-02T14:56:25Z

The issue described by @milindmore22 was caused by the fact that Gutenberg comes with React 17 while WordPress 5.8 is contains React 16.13. Version 17 brought some changes to how useEffect cleanup functions are handled. With 18178b1, the Site Scan feature should work well with the older version of React, too. What's more, we'll now be testing if the feature works as expected even without Gutenberg (f2a0937).

Along the way, I noticed some other issues:

wp-components has not been set as a dependency to the new amp-site-scan-notice stylesheet causing some minor visual issues (fixed in f6f5ff5).
Site scan has been needlessly triggered on a plugin network activation (fixed in b17b93a).

delawski · 2021-12-02T15:30:29Z

While working on this issue, I noticed a potential bug with a multisite setup: #6755

milindmore22

LGTM 👍🏼

…sync-scan-on-plugin-activation * 'develop' of github.com:ampproject/amp-wp: Add test case for Bento lacking layout attributes; improve test coverage Fix phpcs alignment warning Cast term IDs to integers instead of using sanitize_key() Use strict check for in_array() and improve comment Derive intrinsic layout when max-width defined Validate supported layouts for converted Bento components; add support for fill and nodisplay Update AMP_Base_Sanitier::set_layout() to account for 100% width/height styles Harmonize Bento component name parsing Fix variable name Fix typos in comments Add support for converting Bento component dimensions from inline styles to AMP layout Further improve test converage and remove dead code Improve test coverage for ID attribute changes Improve Bento sanitizer and add tests Prevent bento components from being treated as light shadow DOM wrappers Prevent JS error on validated URL screen when no validation errors present Fix handling of ID attributes during sanitizer conversion Introduce Bento sanitizer to deal with BentoJS components Prevent completely tree-shaken styles from triggering excessive CSS

assets/src/components/site-scan-context-provider/index.js

westonruter · 2021-12-02T22:01:27Z

assets/src/admin/site-scan-notice/plugins-with-amp-incompatibility.js

+	if ( ! userIsTechnical ) {
+		return (
+			<p
+				dangerouslySetInnerHTML={ {
+					__html: sprintf(
+						/* translators: %s is a name of a plugin or a list of plugin names separated with commas. */
+						_n(
+							'AMP compatibility issues discovered with %s.',
+							'AMP compatibility issues discovered with the following plugins: %s.',
+							pluginsWithAmpIncompatibility.length,
+							'amp',
+						),
+						pluginsWithAmpIncompatibility
+							.map( ( pluginWithAmpIncompatibility ) => `<b>${ pluginNames[ pluginWithAmpIncompatibility.slug ] }</b>` )
+							.join( ', ' ),
+					),
+				} }
+			/>
+		);
+	}
+


I appreciate the concern for users who are not technical, but the additional information exposed below doesn't have anything technical on it. It just has the URLs on which issues were encountered. Listing out the URLs on which errors occur is helpful even for non-technical users so that they can check them out to see if anything is broken. Therefore I don't think this condition is needed.

Something I can see as being helpful in terms of making it more user-friendly is for the original <title> of the validated URL to be stored and for that to be displayed as the link text instead of URL. But we can explore that in a subsequent improvement.

westonruter

This is so great and so much better than before. Not only does it no longer slow down the activation of a plugin, it also scans all the scannable URLs instead of just one. 💯

delawski · 2021-12-03T10:09:18Z

assets/src/admin/site-scan-notice/notice.js

+					<a
+						href={ AMP_COMPATIBLE_PLUGINS_URL }
+						className="button"
+						{ ...isExternalUrl( AMP_COMPATIBLE_PLUGINS_URL ) ? { target: '_blank', rel: 'noopener noreferrer' } : {} }


Note that there's no need to explicitly add noopener if noreferrer is already there.

That being said, it seems that noopener noreferrer is a common practice, e.g. the <ExternalLink> from @wordpress/components sets both rel types (plus an external type).

If that's the case, let's keep both noopener and noreferrer.

delawski · 2021-12-03T10:12:03Z

assets/src/admin/site-scan-notice/plugins-with-amp-incompatibility.js

+								<a href={ url } target="_blank" rel="noopener noreferrer">
+									{ url }
+								</a>


I guess it'd be best to use ExternalLink from @wordpress/components instead of a plain <a> tag.

Scratch that. ExternalLink adds an icon next to a link and there seem to be no way to remove the icon at the JS-level (other than hiding it with CSS):

assets/src/components/site-scan-context-provider/index.js

delawski · 2021-12-03T10:20:11Z

tests/e2e/specs/admin/after-plugin-activation.js

+		await page.waitForSelector( `tr[data-slug="${ slug }"] .deactivate a` );
+	}
+
+	describe( 'for a user', () => {


We can now safely remove this nested describe since we're not testing different user types anymore.

This has been addressed in a followup PR: #6758.

delawski added Developer Tools Site Scanning Validation labels Nov 4, 2021

delawski added this to the v2.2 milestone Nov 4, 2021

delawski force-pushed the feature/async-scan-on-plugin-activation branch from 51fc15e to f9ca991 Compare November 17, 2021 13:25

delawski mentioned this pull request Nov 19, 2021

Validated URL data may not be included with Support request #6707

Closed

delawski added 8 commits November 23, 2021 10:48

Discontinue doing synchronous validation requests upon activating plu…

3a5a93c

…gins

Do async Site Scan in admin notice after activating plugin

80deda6

Suppress stylelint check around usage of dashicons font

be44ec1

Add missing phpdoc and use more accurate method names

8148d25

Ensure scannable URLs are fetched before starting site scan

68a6214

Keep post-plugin-activation site scan logic in single component

555af01

Hide details for non-technical users

68f68c2

Add link to Plugin Suppression panel

17bedd5

delawski force-pushed the feature/async-scan-on-plugin-activation branch from b45c4c0 to 17bedd5 Compare November 23, 2021 09:50

Extract PluginActivationSiteScan code to separate class

3857c7a

delawski force-pushed the feature/async-scan-on-plugin-activation branch from 8f1a04f to 3857c7a Compare November 23, 2021 12:23

delawski added 6 commits November 23, 2021 16:26

Retrieve source slug and URL on which validation error was found

a8738a9

Add class name to AdminNotice

0622bf6

Display plugins causing AMP issues as separate component

5a2e770

Add styles to admin notice site scan component

756dddf

Display AMP URLs if not in Standard template mode

d0070f3

Add E2E tests for automatic Site Scan after plugin activation

0f720e7

delawski self-assigned this Nov 23, 2021

Add JS docblock

4eb2054

delawski marked this pull request as ready for review November 23, 2021 20:34

delawski requested a review from pierlon as a code owner November 23, 2021 20:34

delawski requested review from dhaval-parekh, milindmore22 and westonruter November 23, 2021 20:34

delawski added 3 commits December 1, 2021 12:24

Ensure demo plugin is deactivated before running tests

985e882

Improve plugin activation post-site-scan message copy

9bd4c44

Update E2E tests

e0b2a1c

delawski requested a review from westonruter December 1, 2021 12:54

delawski added 4 commits December 2, 2021 14:51

Ensure timeout has been set before trying to clear it

18178b1

Enqueue missing wp-components CSS dependency

f6f5ff5

Do not scan site for network activated plugins

b17b93a

Cover case when Gutenberg is not active with E2E tests

f2a0937

milindmore22 approved these changes Dec 2, 2021

View reviewed changes

westonruter reviewed Dec 2, 2021

View reviewed changes

assets/src/components/site-scan-context-provider/index.js Show resolved Hide resolved

westonruter reviewed Dec 2, 2021

View reviewed changes

westonruter added 4 commits December 2, 2021 14:12

Show URLs with validation issues to non-technical users as well

473fdbd

Open validated URLs in new window to prevent accidental loss

6e8191c

Add noopener to external link

e23dfa9

Harden is_needed check to include has_cap

6c15d77

westonruter approved these changes Dec 2, 2021

View reviewed changes

westonruter enabled auto-merge December 2, 2021 22:57

westonruter merged commit 78092fe into develop Dec 2, 2021

westonruter deleted the feature/async-scan-on-plugin-activation branch December 2, 2021 23:03

delawski commented Dec 3, 2021

View reviewed changes

delawski mentioned this pull request Dec 3, 2021

Fix minor issues with async site scan on plugin activation #6758

Merged

2 tasks

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021

westonruter mentioned this pull request Dec 23, 2021

Do async site scanning after theme activation #6814

Closed

delawski mentioned this pull request Jan 28, 2022

Do async site scan after theme activation #6859

Merged

2 tasks

Conversation

delawski commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation Details

Checklist

Uh oh!

delawski commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milindmore22 commented Dec 1, 2021

Uh oh!

delawski commented Dec 2, 2021

Uh oh!

delawski commented Dec 2, 2021

Uh oh!

delawski commented Dec 2, 2021

Uh oh!

milindmore22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

westonruter Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

delawski Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

delawski Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

delawski Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

delawski Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

delawski Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

delawski Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

delawski commented Nov 4, 2021 •

edited

Loading

delawski commented Dec 1, 2021 •

edited

Loading