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

Validate only AMP-enabled pages in Site Scan #6650

Closed
delawski opened this issue Oct 14, 2021 · 6 comments · Fixed by #6610 or #6683
Closed

Validate only AMP-enabled pages in Site Scan #6650

delawski opened this issue Oct 14, 2021 · 6 comments · Fixed by #6610 or #6683
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P0 High priority Site Scanning
Milestone

Comments

@delawski
Copy link
Collaborator

Feature Description

A list of URLs being validated by the Site Scan feature should vary depending on the current state of the Template Mode or Supported Templates option.


As part of the Site Scan feature proposed in #4719 and implemented in #6610, a client-side AMP validation is performed on a set of URL.

The URLs are provided by the ScannableURLProvider via ScannableURLsRestController. They are meant to represent various types of pages so that the Site Scan gives best results.

The existing implementation of the ScannableURLProvider does not take the current Template Mode or Supported Template types into consideration when returning the URLs. For instance, even though in the Reader mode only the post and page templates are AMP-enabled, the provider returns other URLs like the homepage or archives.

Moreover, despite the fact that the SiteScanContextProvider proposed in #6610 will correctly invalidate the scan results whenever a Template Mode or Supported Template types change, it will still not refetch the scannable URLs from the REST endpoint in such a case.

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@delawski delawski added Enhancement New feature or improvement of an existing one Site Scanning labels Oct 14, 2021
@delawski delawski added this to the v2.2 milestone Oct 14, 2021
@westonruter
Copy link
Member

Note that in #6520 I have changes which will allow ScannableURLsRestController to return URLs for pages that are actually have AMP enabled.

@westonruter
Copy link
Member

Also from #6520 (comment):

I haven't gotten the tests to run locally myself, so I'm shooting in the dark a bit. I suspect the issue is that now that \AmpProject\AmpWP\Validation\ScannableURLProvider::get_urls() only returns URLs on which AMP is enabled given the current template mode, if legacy Reader mode was initially selected then only singular posts will be returned among the URLs.

@delawski So this is something else that needs to be added to #6650. In addition to re-fetching the scannable URLs for site scanning after changing the template mode, the Done screen of the Onboarding Wizard will also need to fetch the preview URLs after the settings have been saved instead of exporting them up-front here:

'PREVIEW_URLS' => $this->get_preview_urls( $this->scannable_url_provider->get_urls() ),

@delawski
Copy link
Collaborator Author

Also from #6520 (comment):

I haven't gotten the tests to run locally myself, so I'm shooting in the dark a bit. I suspect the issue is that now that \AmpProject\AmpWP\Validation\ScannableURLProvider::get_urls() only returns URLs on which AMP is enabled given the current template mode, if legacy Reader mode was initially selected then only singular posts will be returned among the URLs.

@delawski So this is something else that needs to be added to #6650. In addition to re-fetching the scannable URLs for site scanning after changing the template mode, the Done screen of the Onboarding Wizard will also need to fetch the preview URLs after the settings have been saved instead of exporting them up-front here:

As per the next comment on #6520, this has been already addressed in #6610 (ffe1c0e).

@westonruter
Copy link
Member

@westonruter
Copy link
Member

Reopening because there is an outstanding issue not yet addressed with the merge of #6610: When legacy Reader mode is initially selected (as it currently is by default), the Site Scan in the onboarding wizard is only scanning the URLs that are available in legacy Reader mode (singular posts, pages, and attachments). In reality, since we're forcing Standard mode in the onboarding wizard's Site Scan, we should be obtaining the scannable URLs for Standard mode as well. Then once the user saves the settings in the onboarding wizard, the scannable URLs should be re-fetched without any option overrides to obtain the URLs to show for the site preview on the Done screen.

@dhaval-parekh
Copy link
Collaborator

QA Passed

✅ Only the AMP pages are returning from /wp-json/amp/v1/scannable-urls
✅ Only the AMP page gets validated during the site scanning.
✅ After changing the template mode or supported template on the "AMP Settings" page. Page is refetching the scannable URL.

@dhaval-parekh dhaval-parekh removed their assignment Dec 2, 2021
@fellyph fellyph assigned fellyph and unassigned fellyph Dec 9, 2021
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P0 High priority Site Scanning
Projects
None yet
5 participants