Improve post Site Scan action items #6698

delawski · 2021-11-08T18:17:35Z

Summary

This PR improves the action items for when there are theme and/or plugin incompatibilities.

Here are the todo items from the original issue:

The action item to review and suppress plugins should rather only appear if there are plugins with incompatibilities, and this action item should appear inside the plugins box.
In the incompatible themes box, this is probably where the recommendation to change to Reader mode should be. Or else to consider switching to an alternative theme (see below).
In the boxes for themes and plugins, there should be links to the AMP Compatible tabs for the the Theme Install and Plugin Install screens, if theme/plugin installation is available (i.e. if filesystem is not read only). If the theme/plugin installation is not available, there should rather be ExternalLinks going to the theme/plugin ecosystem pages on amp-wp.org.
When a site scan completes, the suppressed_plugins option should be re-fetched so that the suppressed table is updated with which plugins have validation errors.
Discontinue doing synchronous re-validation of validated URLs when updating suppressed plugins. Instead of doing this, initiating a re-scan should be done.

Here's the updated Site Scan panel on the AMP Settings screen where the plugin/theme action items are located inside the respective sub-panels:

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

delawski · 2021-11-08T18:25:14Z

@westonruter The only item left to do is adding links to the AMP Compatible tabs for themes and plugins. I was wondering about the filesystem check you mentioned in the original issue. I think we need to not only check the filesystem but also user capabilities. Do you think we could reuse some of the logic from can_install_theme()? I guess we could do something similar for plugins and then pass two flags to the frontend so that appropriate links are rendered on the Settings page.

codecov · 2021-11-10T13:51:24Z

Codecov Report

Merging #6698 (253e5fd) into develop (92453d4) will decrease coverage by 0.06%.
The diff coverage is 20.68%.

❗ Current head 253e5fd differs from pull request most recent head 87e609c. Consider uploading reports for the commit 87e609c to get more accurate results

@@              Coverage Diff              @@
##             develop    #6698      +/-   ##
=============================================
- Coverage      76.67%   76.61%   -0.07%     
- Complexity      6543     6545       +2     
=============================================
  Files            263      265       +2     
  Lines          20956    20989      +33     
=============================================
+ Hits           16069    16080      +11     
- Misses          4887     4909      +22

Flag	Coverage Δ
javascript	`58.19% <14.81%> (-0.75%)`	⬇️
php	`77.76% <100.00%> (+<0.01%)`	⬆️
unit	`77.76% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/src/components/options-context-provider/index.js	`2.22% <0.00%> (-0.56%)`	⬇️
...src/components/site-scan-context-provider/index.js	`35.11% <37.50%> (+0.43%)`	⬆️
assets/src/common/helpers/is-external-url.js	`100.00% <100.00%> (ø)`
src/Admin/OptionsMenu.php	`94.44% <100.00%> (+0.08%)`	⬆️
...onents/site-scan-results/site-scan-sources-list.js	`100.00% <0.00%> (ø)`

github-actions · 2021-11-10T13:52:54Z

Plugin builds for 7114e14 are ready 🛎️!

Download development build
Download production build

westonruter · 2021-11-11T18:55:16Z

The only item left to do is adding links to the AMP Compatible tabs for themes and plugins. I was wondering about the filesystem check you mentioned in the original issue. I think we need to not only check the filesystem but also user capabilities. Do you think we could reuse some of the logic from can_install_theme()? I guess we could do something similar for plugins and then pass two flags to the frontend so that appropriate links are rendered on the Settings page.

@delawski That's right. If the filesystem is not writable, the install_themes and install_plugins capabilities will return false due to logic in map_meta_cap(). But then going ahead with the additional checks in \AmpProject\AmpWP\Admin\ReaderThemes::can_install_theme() would make it even more robust, although I think it may be overkill since we don't actually need to perform the installation ourselves. I think that was done specifically for the case where a site may require SFTP credentials to be provided to do the update. In contrast, all we need to do is provide a link to the theme/plugin install screens. So for that, all we need to do is check if current_user_can('install_themes') and current_user_can('install_plugins'). The logic would be as follows:

$theme_link  = current_user_can( 'install_themes' ) ? admin_url( '/theme-install.php?browse=amp-compatible' ) : 'https://amp-wp.org/ecosystem/themes/';
$plugin_link = current_user_can( 'install_plugins' ) ? admin_url( '/plugin-install.php?tab=amp-compatible' ) : 'https://amp-wp.org/ecosystem/plugins/';

westonruter · 2021-11-11T19:06:16Z

And when the capability checks return false, we should have the external link icon to go with the links.

This prevents the progress bar starting at 100% in the Onboarding Wizard site scan.

delawski · 2021-11-15T17:55:43Z

@westonruter Thank you for the suggestions. I've used the proposed logic in fd06053:

Also, I found a bug in the Site Scan in the Wizard. The progress bar has been shown too early, when the state was not stable yet causing the progress bar to start at 100%. This has been fixed in 3621298.

Lastly, I've included a Site Scan reducer one-line fix (253e5fd) to the issue you've revealed in #6705 (comment).

delawski · 2021-11-15T17:58:17Z

I've just noticed a new action item on the original issue:

Discontinue doing synchronous re-validation of validated URLs when updating suppressed plugins. Instead of doing this, initiating a re-scan should be done.

I'll take care of that next.

…synchronous validation

delawski · 2021-11-17T13:22:23Z

I've just noticed a new action item on the original issue:

Discontinue doing synchronous re-validation of validated URLs when updating suppressed plugins. Instead of doing this, initiating a re-scan should be done.

I'll take care of that next.

This has been addressed in 87e609c:

Screen.Recording.2021-11-17.at.02.20.16.PM.mp4

westonruter · 2021-11-17T17:55:25Z

Regarding this phpunit failure:

This seems to be an issue in PHP 8 and/or WP 5.9-alpha, so I'll investigate. It's unrelated to this PR anyway.

westonruter · 2021-11-17T20:09:01Z

This seems to be an issue in PHP 8 and/or WP 5.9-alpha, so I'll investigate. It's unrelated to this PR anyway.

It's this change in WordPress core: WordPress/wordpress-develop@5d5c9bf

Easy to fix. See #6727.

…te-scan-results-panel * 'develop' of github.com:ampproject/amp-wp: Fix test_amp_remove_paired_endpoint in WP 5.9 by removing empty query string

westonruter · 2021-11-17T23:46:01Z

I just realized there is another factor for the recommendation matrix. Namely, if the user has a plugin suppressed then we should never recommend Standard mode, which we are doing now:

If a plugin is being suppressed, then this normally means that you're in a paired AMP mode and you don't want the plugin running on the AMP version but you do want it running on the non-AMP version. If you are in Standard mode, then essentially the user should rather just have deactivated the plugin entirely as that is how it would be behaving.

So this would look something like this:

diff --git a/assets/src/components/use-template-mode-recommendation/index.js b/assets/src/components/use-template-mode-recommendation/index.js
index d6b9dee9d..dfea95155 100644
--- a/assets/src/components/use-template-mode-recommendation/index.js
+++ b/assets/src/components/use-template-mode-recommendation/index.js
@@ -60,12 +60,14 @@ export function useTemplateModeRecommendation() {
  * @param {boolean} args.hasFreshSiteScanResults Whether fresh site scan results are available.
  * @param {boolean} args.hasThemeIssues          Whether the site scan found themes with AMP incompatibility.
  * @param {boolean} args.userIsTechnical         Whether the user answered yes to the technical question.
+ * @param {boolean} args.hasSuppressedPlugins
  */
 export function getTemplateModeRecommendation( {
 	hasPluginIssues,
 	hasFreshSiteScanResults,
 	hasThemeIssues,
 	userIsTechnical,
+	hasSuppressedPlugins,
 } ) {
 	/* eslint-disable @wordpress/no-unused-vars-before-return */
 	const mobileRedirectionNote = __( 'If automatic mobile redirection is enabled, the AMP version of the content will be served on mobile devices. If AMP-to-AMP linking is enabled, once users are on an AMP page, they will continue navigating your AMP content.', 'amp' );
@@ -298,17 +300,18 @@ export function getTemplateModeRecommendation( {
 					],
 				},
 				[ TRANSITIONAL ]: {
-					recommendationLevel: NOT_RECOMMENDED,
+					recommendationLevel: hasSuppressedPlugins ? RECOMMENDED : NOT_RECOMMENDED,
 					details: [
 						transitionalModeDescription,
 						notRecommendedDueToCompleteCompatibility,
 					],
 				},
 				[ STANDARD ]: {
-					recommendationLevel: RECOMMENDED,
+					recommendationLevel: hasSuppressedPlugins ? NOT_RECOMMENDED : RECOMMENDED,
 					details: [
 						standardModeDescription,
 						__( 'Recommended as you have an AMP-compatible theme and no issues were detected with any of the plugins on your site.', 'amp' ),
+						hasSuppressedPlugins ? __( 'Not recommended because you have suppressed plugins.', 'amp' ) : null,
 					],
 				},
 			};
@@ -326,7 +329,7 @@ export function getTemplateModeRecommendation( {
 					],
 				},
 				[ TRANSITIONAL ]: {
-					recommendationLevel: NEUTRAL,
+					recommendationLevel: hasSuppressedPlugins ? RECOMMENDED : NEUTRAL,
 					details: [
 						transitionalModeDescription,
 						__( 'Recommended if you can’t commit to choosing plugins that are AMP compatible when extending your site. This mode will make it easy to keep AMP content even if non-AMP-compatible plugins are used later on.', 'amp' ),
@@ -337,6 +340,7 @@ export function getTemplateModeRecommendation( {
 					details: [
 						standardModeDescription,
 						__( 'Recommended if you can commit to always choosing plugins that are AMP compatible when extending your site.', 'amp' ),
+						hasSuppressedPlugins ? __( 'Not recommended because you have suppressed plugins.', 'amp' ) : null,
 					],
 				},
 			};

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

westonruter · 2021-11-18T00:11:18Z

assets/src/components/site-scan-results/plugins-with-amp-incompatibility.js


 /**
 * WordPress dependencies
 */
 import { __ } from '@wordpress/i18n';
-import { useMemo } from '@wordpress/element';
+import { createInterpolateElement, useMemo } from '@wordpress/element';


westonruter · 2021-11-18T00:11:55Z

assets/src/components/site-scan-results/plugins-with-amp-incompatibility.js

+			{ showHelpText && (
+				<p>
+					{ createInterpolateElement(
+						__( 'Because of plugin issues we’ve uncovered, you may want to <a>review and suppress plugins</a>.', 'amp' ),


Aside: With createInterpolateElement, what happens when you have more than one <a> in your string?

You can use whatever tag name you wish, e.g. you could do something like this:

createInterpolateElement( 'Visit <ArchiveLink>archive</ArchiveLink> or return to <HomeLink>homepage</HomeLink>', { ArchiveLink: <a href="/archive" />, HomeLink: <a href="/" />, } );

Co-authored-by: Weston Ruter <[email protected]>

delawski · 2021-11-18T15:02:59Z

I just realized there is another factor for the recommendation matrix. Namely, if the user has a plugin suppressed then we should never recommend Standard mode, which we are doing now

Thanks for providing that diff. I've added it along with a logic for determining the hasSuppressedPlugins in 0de088d.

westonruter

🥇

westonruter · 2021-11-23T01:30:20Z

assets/src/components/use-template-mode-recommendation/index.js

+		// Plugins are considered suppressed only if they are active (included in the `suppressible_plugins` object).
+		const hasSuppressedPlugins = Object.entries( currentOptions?.suppressed_plugins || {} )
+			.some( ( [ slug, suppressed ] ) => suppressed && Boolean( currentOptions.suppressible_plugins?.[ slug ] ) );


Nicely done

westonruter · 2021-11-23T01:39:47Z

assets/src/components/options-context-provider/index.js

+				const fetchedPluginSuppression = await apiFetch( {
+					path: addQueryArgs( optionsRestPath, {
+						_fields: [ 'suppressed_plugins', 'suppressible_plugins' ],
+					} ),
+				} );


westonruter · 2021-11-23T01:43:20Z

assets/src/components/options-context-provider/index.js

+				setOriginalOptions( {
+					...originalOptions,
+					...fetchedPluginSuppression,
+				} );


This is great. I was thinking that this could have the effect of overriding a user's unsaved changes to plugin suppression, but this won't be the case the original options are updated, not the modified otpions.

delawski requested review from westonruter and dhaval-parekh November 8, 2021 18:17

delawski marked this pull request as ready for review November 10, 2021 13:49

delawski marked this pull request as draft November 10, 2021 13:52

delawski added 4 commits November 15, 2021 14:57

Move post-scan action items to plugins/themes boxes

af79d44

Refetch plugin suppression data after completing site scan

62aa5b0

Prevent rendering empty notice element

617713a

Fix unit tests

074f845

delawski force-pushed the update/site-scan-results-panel branch from d9759f3 to 074f845 Compare November 15, 2021 14:20

delawski added 4 commits November 15, 2021 18:27

Add links to AMP compatible themes and plugins

fd06053

Add unit tests for SiteScanSourcesList component

d24f6b4

Stay in loading state until the Wizard scan has started

3621298

This prevents the progress bar starting at 100% in the Onboarding Wizard site scan.

Do not mutate state outside of reducer

253e5fd

delawski marked this pull request as ready for review November 15, 2021 17:51

delawski added 2 commits November 16, 2021 19:56

Trigger async scan when updating suppressed plugins instead of doing …

87e609c

…synchronous validation

Merge branch 'develop' into update/site-scan-results-panel

1d60b17

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

delawski added Enhancement New feature or improvement of an existing one Site Scanning Validation labels Nov 17, 2021

Merge branch 'develop' of github.com:ampproject/amp-wp into update/si…

310ba0f

…te-scan-results-panel * 'develop' of github.com:ampproject/amp-wp: Fix test_amp_remove_paired_endpoint in WP 5.9 by removing empty query string

westonruter added 2 commits November 17, 2021 15:50

Improve theme/plugin site scan copy

21fda0e

Remove obsolete SUPPRESSED_PLUGINS_ERRORING_URLS option

cd03b79

westonruter reviewed Nov 18, 2021

View reviewed changes

delawski and others added 3 commits November 18, 2021 13:04

Remove redundant optional chaining operator

5a16e08

Co-authored-by: Weston Ruter <[email protected]>

Don't recommend Standard mode if there are any suppressed plugins

0de088d

Add test for new prop

7114e14

dhaval-parekh approved these changes Nov 19, 2021

View reviewed changes

westonruter approved these changes Nov 23, 2021

View reviewed changes

westonruter merged commit f484b76 into develop Nov 23, 2021

westonruter deleted the update/site-scan-results-panel branch November 23, 2021 01:48

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve post Site Scan action items #6698

Improve post Site Scan action items #6698

delawski commented Nov 8, 2021 •

edited

Loading

delawski commented Nov 8, 2021

codecov bot commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021 •

edited

Loading

westonruter commented Nov 11, 2021

westonruter commented Nov 11, 2021

delawski commented Nov 15, 2021

delawski commented Nov 15, 2021

delawski commented Nov 17, 2021

westonruter commented Nov 17, 2021

westonruter commented Nov 17, 2021 •

edited

Loading

westonruter commented Nov 17, 2021

westonruter Nov 18, 2021

westonruter Nov 18, 2021

delawski Nov 18, 2021

delawski commented Nov 18, 2021 •

edited

Loading

westonruter left a comment

westonruter Nov 23, 2021

westonruter Nov 23, 2021

westonruter Nov 23, 2021

Improve post Site Scan action items #6698

Improve post Site Scan action items #6698

Conversation

delawski commented Nov 8, 2021 • edited Loading

Summary

Checklist

delawski commented Nov 8, 2021

codecov bot commented Nov 10, 2021 • edited Loading

Codecov Report

github-actions bot commented Nov 10, 2021 • edited Loading

westonruter commented Nov 11, 2021

westonruter commented Nov 11, 2021

delawski commented Nov 15, 2021

delawski commented Nov 15, 2021

delawski commented Nov 17, 2021

westonruter commented Nov 17, 2021

westonruter commented Nov 17, 2021 • edited Loading

westonruter commented Nov 17, 2021

westonruter Nov 18, 2021

Choose a reason for hiding this comment

westonruter Nov 18, 2021

Choose a reason for hiding this comment

delawski Nov 18, 2021

Choose a reason for hiding this comment

delawski commented Nov 18, 2021 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

westonruter Nov 23, 2021

Choose a reason for hiding this comment

westonruter Nov 23, 2021

Choose a reason for hiding this comment

westonruter Nov 23, 2021

Choose a reason for hiding this comment

delawski commented Nov 8, 2021 •

edited

Loading

codecov bot commented Nov 10, 2021 •

edited

Loading

github-actions bot commented Nov 10, 2021 •

edited

Loading

westonruter commented Nov 17, 2021 •

edited

Loading

delawski commented Nov 18, 2021 •

edited

Loading