Do async site scan after theme activation #6859

delawski · 2022-01-28T13:39:42Z

Summary

This PR reuses the async site scan notice logic introduced in #6685 so that a site scan is done after theme activation, too.

Right now on both plugins.php and themes.php screens the scan results will include plugin and theme issues.

This PR also updates the Site Scan results for themes. Previously, a parent theme causing AMP incompatibilities was greyed-out while it should not (#6958):

Before	After

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

github-actions · 2022-01-28T13:47:27Z

Plugin builds for f7f2121 are ready 🛎️!

Download development build
Download production build

delawski · 2022-02-02T13:58:35Z

Please note that the site scan notification may need some design review and improvements since right now we show both plugin and theme issues at the same time:

dhaval-parekh

Changes looks good to me.

This utilizes the async site scan notice that is shown after plugin activation. Right now, both on `plugins.php` and on `themes.php` screens, the scan results will include plugin and theme issues.

codecov · 2022-02-21T21:06:05Z

Codecov Report

Merging #6859 (346a83d) into develop (0ba4427) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 346a83d differs from pull request most recent head 3531d9e. Consider uploading reports for the commit 3531d9e to get more accurate results

@@              Coverage Diff              @@
##             develop    #6859      +/-   ##
=============================================
+ Coverage      77.80%   77.81%   +0.01%     
- Complexity      6767     6771       +4     
=============================================
  Files            267      267              
  Lines          21548    21560      +12     
=============================================
+ Hits           16765    16777      +12     
  Misses          4783     4783

Flag	Coverage Δ
javascript	`64.04% <ø> (ø)`
php	`78.57% <100.00%> (+0.01%)`	⬆️
unit	`78.57% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/AmpWpPlugin.php	`100.00% <ø> (ø)`
src/Admin/AfterActivationSiteScan.php	`100.00% <100.00%> (ø)`

westonruter

Sorry for the delay in reviewing.

westonruter · 2022-02-28T21:17:19Z

src/Admin/AfterActivationSiteScan.php

+	/**
+	 * HTML ID for the app root sibling element.
+	 *
+	 * Since there is no action for adding notice on `themes.php` screen, we need to inject React root element with JS.
+	 * It is an ID of a success notice saying "New theme activated. Visit site" on the `themes.php` screen.
+	 *
+	 * @var string
+	 */
+	const APP_ROOT_SIBLING_ID = 'message2';


Classic 😄

westonruter · 2022-02-28T21:18:48Z

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

+
+		root = document.createElement( 'div' );
+		root.setAttribute( 'id', APP_ROOT_ID );
+		rootSibling.after( root );


Nice. The after method is available for every browser except IE11. I need to start using it!

westonruter · 2022-02-28T21:29:04Z

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

+			<p>
+				{ createInterpolateElement(
+					sprintf(
+						// translators: %s stands for a theme name.
+						__( 'AMP compatibility issues discovered with the <b>%s</b> theme.', 'amp' ),
+						themeName,
+					),
+					{
+						b: <strong />,
+					},
+				) }
+			</p>
+			<div className="amp-site-scan-notice__cta">
+				<a
+					href={ AMP_COMPATIBLE_THEMES_URL }
+					className="button"
+					{ ...isExternalUrl( AMP_COMPATIBLE_THEMES_URL ) ? { target: '_blank', rel: 'noopener noreferrer' } : {} }
+				>
+					{ __( 'View AMP-Compatible Themes', 'amp' ) }
+				</a>
+			</div>


Can this add the list of URLs in the same way as PluginsWithAmpIncompatibility does?

westonruter · 2022-02-28T21:30:13Z

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

+			<p>
+				{ createInterpolateElement(
+					sprintf(
+						// translators: %s stands for a theme name.
+						__( 'AMP compatibility issues discovered with the <b>%s</b> theme.', 'amp' ),
+						themeName,
+					),
+					{
+						b: <strong />,
+					},
+				) }
+			</p>


There may actually be two themes that have compatibility issues, the parent theme and the child theme. Therefore I suggest that the structure here follow closely what you did in PluginsWithAmpIncompatibility and have the list of themes along with the URLs that they have issues detected on.

Definitely. Check out #6859 (comment) below.

westonruter · 2022-02-28T21:30:50Z

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

+		return null;
+	}
+
+	const themeMeta = themes.find( ( theme ) => theme.stylesheet === themeWithAmpIncompatibility.slug );


As noted below, this should also account for there being a child theme active.

src/Admin/AfterActivationSiteScan.php

westonruter · 2022-02-28T21:47:15Z

src/Admin/AfterActivationSiteScan.php

@@ -197,6 +235,11 @@ protected function add_preload_rest_paths() {
 				[ 'author', 'name', 'plugin', 'status', 'version' ],
 				'/wp/v2/plugins'
 			),
+			add_query_arg(
+				'_fields',
+				[ 'author', 'name', 'status', 'stylesheet', 'version' ],


Per above, this will also need to include template for the sake of child themes:

Suggested change

[ 'author', 'name', 'status', 'stylesheet', 'version' ],

[ 'author', 'name', 'status', 'stylesheet', 'template', 'version' ],

I've just committed that but I'm now wondering if we actually need to know the parent theme (i.e. the template)? We don't show an information like this anywhere in the UI so I guess we could just skip that.

I think you'd need it here potentially:

const themeMeta = themes.find( ( theme ) => theme.stylesheet === themeWithAmpIncompatibility.slug ); const themeName = themeMeta?.name?.rendered ?? themeMeta?.name;

We are now using template, too. As noted in #6859 (comment), the parent/child themes were not handled well in the Site Scan results. This should be fixed with 11da989.

Co-authored-by: Weston Ruter <westonruter@google.com>

delawski · 2022-03-05T17:59:50Z

@westonruter While reviewing your feedback I realized that the Site Scan is not handling correctly cases when child and parent themes cause AMP incompatibilities. The parent theme is greyed-out as if it was inactive (because its status literally is inactive):

I've issued a new ticket #6958 mainly for tracking/QA reasons because this issue should be fixed with 11da989:

It also takes care of the post-activation notice:

…theme-activation-site-scan * 'develop' of github.com:ampproject/amp-wp: (158 commits) Include SCSS files in the lint-staged config for stylelint Fix stylelint issue Rename 'allowed' to 'associated' Use named functions Add label for why the validation data is being shown Remove wrapping of site-scan-results__source-detail Bump lint-staged from 12.3.4 to 12.3.5 Update perimssions for gutenberg-packages-update workflow Add approve step for auto-merge workflow Add notice step Move conditional from matrix job to its steps Only run PHP feature tests when php files have changed Only run phpunit tests if a PHP file has changed Update auto-merge workflow based on latest GH docs Bump node-fetch from 2.6.5 to 2.6.7 Bump postcss from 8.4.7 to 8.4.8 Bump eslint-plugin-jsdoc from 37.9.6 to 37.9.7 Improve test coverage Prevent site scan progress bar overflow bug in Safari Hide default details summary marker on Safari ...

westonruter · 2022-03-08T23:41:18Z

I've issued a new ticket #6958 mainly for tracking/QA reasons because this issue should be fixed with 11da989:

Indeed!

Before	After

westonruter · 2022-03-09T00:25:13Z

Looks like tests are failing after I merged #6933. This error is also happening in develop so it's not unique to this branch.

westonruter

Good work

westonruter · 2022-03-08T23:54:28Z

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

+		];
+
+		// Display the theme information at the top when on the `themes.php` screen.
+		if ( document.location.href.includes( 'themes.php' ) ) {


This works, although it may be a bit more elegant to pass this information via the amp-site-scan-notice external dependency.

Co-authored-by: Piotr Delawski <piotr.delawski@gmail.com>

delawski added Enhancement New feature or improvement of an existing one Site Scanning labels Jan 28, 2022

delawski added this to the v2.3 milestone Jan 28, 2022

delawski requested review from westonruter and dhaval-parekh January 28, 2022 13:39

delawski mentioned this pull request Jan 28, 2022

Improve page cache detection test #6849

Merged

2 tasks

maitreyie-chavan modified the milestones: v2.3, v2.2.2 Feb 9, 2022

dhaval-parekh approved these changes Feb 15, 2022

View reviewed changes

Do async site scan after theme activation

9566a93

This utilizes the async site scan notice that is shown after plugin activation. Right now, both on `plugins.php` and on `themes.php` screens, the scan results will include plugin and theme issues.

delawski force-pushed the add/6814-theme-activation-site-scan branch from 8696ef2 to 9566a93 Compare February 21, 2022 16:44

Fix eslint issue

346a83d

Use FQN for class in .phpstorm.meta.php

3531d9e

westonruter requested changes Feb 28, 2022

View reviewed changes

delawski and others added 5 commits March 4, 2022 16:03

Fix link to AMP compatible themes listing

79025eb

Co-authored-by: Weston Ruter <westonruter@google.com>

Fetch theme template property in case of child themes

7e951dd

Co-authored-by: Weston Ruter <westonruter@google.com>

Allow for listing multiple themes in post-activation notification

dfd130a

Ensure parent theme is listed if child theme is active

11da989

Update unit test

aaa506b

delawski mentioned this pull request Mar 5, 2022

Parent theme with AMP incompatibilities is greyed-out in the Site Scan results #6958

Closed

delawski requested a review from westonruter March 5, 2022 18:04

westonruter added 2 commits March 8, 2022 15:57

Fix type check by passing explicit string as href prop

d49bcab

Improve strings to account for one or more issues

f7f2121

westonruter approved these changes Mar 9, 2022

View reviewed changes

westonruter merged commit 10398bb into develop Mar 9, 2022

westonruter deleted the add/6814-theme-activation-site-scan branch March 9, 2022 00:25

westonruter added a commit that referenced this pull request Mar 9, 2022

Do async site scan after theme activation (#6859)

2fdb1ff

Co-authored-by: Piotr Delawski <piotr.delawski@gmail.com>

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do async site scan after theme activation #6859

Do async site scan after theme activation #6859

delawski commented Jan 28, 2022 •

edited

Loading

github-actions bot commented Jan 28, 2022 •

edited

Loading

delawski commented Feb 2, 2022

dhaval-parekh left a comment

codecov bot commented Feb 21, 2022 •

edited

Loading

westonruter left a comment

westonruter Feb 28, 2022

westonruter Feb 28, 2022

westonruter Feb 28, 2022

westonruter Feb 28, 2022

delawski Mar 5, 2022

westonruter Feb 28, 2022

westonruter Feb 28, 2022

delawski Mar 4, 2022

westonruter Mar 4, 2022

delawski Mar 5, 2022

delawski commented Mar 5, 2022

westonruter commented Mar 8, 2022

westonruter commented Mar 9, 2022

westonruter left a comment

westonruter Mar 8, 2022

	[ 'author', 'name', 'status', 'stylesheet', 'version' ],
	[ 'author', 'name', 'status', 'stylesheet', 'template', 'version' ],

Do async site scan after theme activation #6859

Do async site scan after theme activation #6859

Conversation

delawski commented Jan 28, 2022 • edited Loading

Summary

Checklist

github-actions bot commented Jan 28, 2022 • edited Loading

delawski commented Feb 2, 2022

dhaval-parekh left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2022 • edited Loading

Codecov Report

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delawski commented Mar 5, 2022

westonruter commented Mar 8, 2022

westonruter commented Mar 9, 2022

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delawski commented Jan 28, 2022 •

edited

Loading

github-actions bot commented Jan 28, 2022 •

edited

Loading

codecov bot commented Feb 21, 2022 •

edited

Loading