Add Site Scan Feature

[delawski]

Summary

• Before reviewing this, please review, merge, and merge develop into this branch: Add AMP-first preview and args to validate request for Site Scanning #6615

---

Fixes #4719
Fixes #6650

The Site Scan feature is added to both the Onboarding Wizard and to the Settings screen.

Onboarding Wizard

In the Wizard, a new step has been added that triggers a site scan automatically. The scan is always done in the Standard mode and its results are not stored.

Scan in progress	Scan complete

Based on the scan results, the best template mode is recommended in the next step. Note that the recommendations step and the copy has been updated according to the recommendation matrix.

Settings Screen

The Site Scan panel on the Settings Screen is more robust than the one you can find in the Wizard.

First of all, it shows scan results based on cached validation errors. It also indicates stale state of the results.

Stale results	Fresh results

It is possible to trigger new scan whenever the template mode is changed and saved. What's more, if a scan is in progress, it will be cancelled in such a case.

Scan in progress	Cancelled scan

In case there are no errors, a success message will be shown. If none of the URLs could be scanned (an edge case), an error message will be displatyed.

Success	Error

Todos

• Fix existing E2E tests
• Add new E2E tests for the site scan
• Increase JS unit tests coverage

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

[westonruter]

@delawski There we go, quick and easy: 3986b6e.

In the Onboarding Wizard you can fetch just the necessary data (the URLs themselves) via a request like /wp-json/amp/v1/scannable-urls/?_fields=url,amp_url,type,label.

On the Settings screen you can omit the _fields and it will include validation_errors, validated_url_post, and stale.

[westonruter]

The Site Scan results always showed as stale on the settings screen because the settings screen was passing amp-first. I've fixed this in 9b3f951 but now something else has come up. When I've got a site in Transitional mode or Reader mode, the URLs being scanned aren't the AMP ones. This is resulting in the request failing with a AMP_NOT_REQUESTED code:

So the SiteScanContextProvider needs to be aware of the current template mode, and use the amp_url if it is not Standard.

Similarly, if a given template type is not enabled for AMP (e.g. when in legacy Reader mode the archive templates aren't available), then such requests will be returning a different error: AMP_NOT_AVAILABLE. In this case, site scan should just silently ignore the request, and even better prevent it from being made in the first place. This goes back to my note about URLScanningContext in #6615 (comment). There could be an available boolean prop that is associated with each URL which would allow SiteScanContextProvider to bypass them. But this can be done later in a separate PR. In the meantime, just make sure that the AMP_NOT_REQUESTED and AMP_NOT_AVAILABLE error cases are handled.

[delawski]

In the meantime, just make sure that the AMP_NOT_REQUESTED and AMP_NOT_AVAILABLE error cases are handled.

@westonruter I've updated the logic in the SiteScanContextProvider so that those error codes are ignored. Also, I can correctly see that with the cache flag I'm receiving a 201 Created instead of 200 OK status code.

Unfortunately, I'm still getting stale validation results whenever I'm refreshing the Settings page. I'm not sure why that happens.

---

On top of that, I've been working today on improving the frontend logic.

Right now, if a user changes the template mode and saves the settings, the site scan results are going to be marked as stale immediately and the "Rescan Site" button will be shown.

Template mode changed - no issues in last scan	Template mode changed - found issues in last scan

If the template mode is switched when the scan is in progress, it will be cancelled and a message will be displayed:

If no errors are found by the Site Scan, a short message is displayed. We may want to improve this screen visually a bit (what are your thoughts, @jwold?):

[westonruter]

Quick note regarding error handling: I did a quick for stale results by activating Yoast, and when I did so I got an error:

The issue is that Yoast redirects attachment page links to the attachment files themselves, so that is why there is an error. So if the in this case as well, when the response is not application/json, the site scan should be probably skipped as if AMP_NOT_AVAILABLE had been encountered.

[westonruter]

Or generally, if there is any error with the fetching of the URL like this, instead of throwing up the error screen takeover, it would probably be better to list out the errors in the panel as part of the results.

[delawski]

Question: I switched the theme to a non-compatible theme (Hestia) and I didn't see Reader as the recommended template mode:

Shouldn't it be?

@westonruter It turns out that the template mode recommendation logic on the AMP Settings screen is different than the one we use in the Onboarding Wizard. It doesn't make any use of the Site Scan results.

I went ahead and refactored the recommendation logic out of the Wizard and into the common helpers. Then, I've re-used that logic for generating the notices and messages in the Template Mode selection on the AMP Settings screen. This is all done in cf218ea.

Now, for a non-technical user and Hestia theme, the Reader mode is recommended and the Standard mode is actually discouraged (it follows the recommendation matrix):

[delawski]

@milindmore22 That's interesting. I've installed bbPress and imported the test data. When scanning in the Reader mode, I'm seeing Gutenberg as the only plugin causing AMP incompatibility:

Validated URLs page says the same. However, we get more context now. It's not only Gutenberg, it's the Core, too:

However, it's clear that bbPress is the root problem. When I deactivate the plugin, I get no issues:

@westonruter What are your thoughts? Is there a way we could attribute an error to a plugin that is in fact causing it?

[westonruter]

I activated Hestia (a theme lacking AMP compatibility) and the result is it shows Standard mode as being recommended, when it shouldn't be:

Since there are validation errors coming from Hestia, there should be no recommendation to use Standard mode, even if the user is technical.

Interestingly, as soon as I started the scan, it showed Reader as recommended but then once it finished it showed Standard.

[delawski]

Since there are validation errors coming from Hestia, there should be no recommendation to use Standard mode, even if the user is technical.

@westonruter Yeah, it makes sense to me. I didn't implement this change yet just because there might be more cases like this since the current recommendation logic maps the matrix in the doc. However, what I did is I made the recommendation logic much more explicit and wrapped it in a custom hook (7920d78). This way we can go through the logic and the matrix and fix all the issues.

Interestingly, as soon as I started the scan, it showed Reader as recommended but then once it finished it showed Standard.

This flicker should not happen anymore. Right now the logic is encapsulated in a custom hook where we keep the recommendation state untouched as long as the options, user meta or scannable URLs are being saved/fetched.

One more addition is a notice in the Template Mode section saying that the recommendation may be inaccurate whenever the site scan results become stale:

[codecov]

Codecov Report

Merging #6610 (88a1fd9) into develop (746d7ac) will decrease coverage by 0.98%.
The diff coverage is 63.15%.

❗ Current head 88a1fd9 differs from pull request most recent head 8836b54. Consider uploading reports for the commit 8836b54 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6610      +/-   ##
=============================================
- Coverage      77.37%   76.38%   -0.99%     
- Complexity      6465     6475      +10     
=============================================
  Files            197      261      +64     
  Lines          19432    20697    +1265     
=============================================
+ Hits           15035    15809     +774     
- Misses          4397     4888     +491     

Flag	Coverage Δ
javascript	57.59% <41.91%> (?)
php	77.51% <98.98%> (+0.14%)	⬆️
unit	77.51% <98.98%> (+0.14%)	⬆️

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

Impacted Files	Coverage Δ
assets/src/components/amp-drawer/index.js	73.07% <ø> (ø)
...ssets/src/components/template-mode-option/index.js	78.94% <0.00%> (ø)
src/AmpWpPlugin.php	100.00% <ø> (ø)
src/Validation/URLValidationRESTController.php	92.78% <ø> (ø)
...src/components/site-scan-context-provider/index.js	19.00% <19.00%> (ø)
...mponents/use-template-mode-recommendation/index.js	48.27% <48.27%> (ø)
src/Validation/ScannableURLsRestController.php	98.50% <98.50%> (ø)
...ts/src/common/helpers/get-plugin-slug-from-file.js	100.00% <100.00%> (ø)
assets/src/components/amp-notice/index.js	100.00% <100.00%> (ø)
...ns-context-provider/use-normalized-plugins-data.js	100.00% <100.00%> (ø)
... and 63 more

[westonruter]

Another magnum opus‽

[fellyph]

QA Passed

• Site Scan behave as expected
• JS test mentioned at the task passing