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

Handle case in Site Scanning when no scannable URL is available #6856

Merged
merged 2 commits into from
Jan 28, 2022

Conversation

delawski
Copy link
Collaborator

Summary

Fixes #6855

In the Reader mode, only post types are used for performing a site scan. If there are no posts, pages or other custom post types available for a scan, a request made to the /wp-json/amp/v1/scannable-urls endpoint returns an empty array.

With this change, an empty array is treated as a valid response so that the list of scannable URLs kept in the Site Scan state is emptied. This case is then handled gracefully on the frontend where an error message is rendered. The message says that there are no URLs available for a scan.

Even though the error message has already been part of the Site Scan UI, it was displayed only if there were no scannable URLs available initially, right after a page load.

Screenshot 2022-01-26 at 14 11 36

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

In the Reader mode, only post types are used for performing a site scan. If there are no posts, pages or other custom post types available for a scan, a request made to the `/wp-json/amp/v1/scannable-urls` endpoint returns an empty array.

With this change, an empty array is treated as a valid response so that the list of scannable URLs kept in the Site Scan state is emptied. This case is then handled gracefully on the frontend where an error message is rendered. The message says that there are no URLs available for a scan.

Even though the error message has already been part of the Site Scan UI, it was displayed only if there were no scannable URLs available initially, right after a page load.
@delawski delawski added Bug Something isn't working Site Scanning labels Jan 26, 2022
@delawski delawski added this to the v2.2.1 milestone Jan 26, 2022
@delawski delawski self-assigned this Jan 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 26, 2022

Plugin builds for 1dec195 are ready 🛎️!

@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #6856 (3e03e12) into develop (0bb7948) will decrease coverage by 0.81%.
The diff coverage is 100.00%.

❗ Current head 3e03e12 differs from pull request most recent head 1dec195. Consider uploading reports for the commit 1dec195 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6856      +/-   ##
=============================================
- Coverage      78.36%   77.55%   -0.82%     
  Complexity      6721     6721              
=============================================
  Files            202      267      +65     
  Lines          20276    21460    +1184     
=============================================
+ Hits           15890    16644     +754     
- Misses          4386     4816     +430     
Flag Coverage Δ
javascript 63.68% <100.00%> (?)
php 78.36% <ø> (ø)
unit 78.36% <ø> (ø)

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

Impacted Files Coverage Δ
...src/components/site-scan-context-provider/index.js 34.48% <100.00%> (ø)
assets/src/block-editor/store/selectors.js 100.00% <0.00%> (ø)
...onents/site-scan-results/site-scan-sources-list.js 100.00% <0.00%> (ø)
assets/src/components/amp-setting-toggle/index.js 100.00% <0.00%> (ø)
assets/src/common/helpers/is-external-url.js 100.00% <0.00%> (ø)
...block-validation/components/error/error-content.js 100.00% <0.00%> (ø)
...sets/src/block-validation/components/icon/index.js 100.00% <0.00%> (ø)
assets/src/components/amp-info/index.js 100.00% <0.00%> (ø)
...s/src/components/options-context-provider/index.js 2.17% <0.00%> (ø)
...idation/components/error/get-error-source-title.js 100.00% <0.00%> (ø)
... and 56 more

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Behavior checks out. I registered a Recipe custom post type that had no posts created for it.

Before

Before once I updated the supported post types to only have Recipes, the Site Scan allowed by to initiate a scan but then it failed:

before.mov

After

After the fix, as soon as I saved the settings it immediately showed the appropriate error message up front:

after.mov

@westonruter westonruter merged commit 14ddbf0 into develop Jan 28, 2022
@westonruter westonruter deleted the fix/6855-error-when-no-scannable-urls branch January 28, 2022 22:59
@yogeshbeniwal
Copy link

yogeshbeniwal commented Jan 29, 2022

@westonruter For the scenario where few of the post type have post and few doesn't. What will happen in that case. Will site scan continue or throws error.

@westonruter
Copy link
Member

@yogeshbeniwal As long as one of the post types have an AMP URL available, the site scan should proceed as expected.

@fellyph Would you confirm?

@fellyph
Copy link
Collaborator

fellyph commented Jan 31, 2022

@yogeshbeniwal @westonruter

Yes, only the post type with one or more posts will be part of the scan. This fix was for users who select a post type with no posts.

@yogeshbeniwal
Copy link

Thank you @westonruter @fellyph for confirmation.

@westonruter westonruter changed the title Handle case when no scannable URL is available Handle case in Site Scanning when no scannable URL is available Feb 3, 2022
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Site Scanning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Site scan fail with custom post type
4 participants