Force Standard mode when fetching scannable URLs in Wizard #6683

delawski · 2021-11-03T19:57:55Z

Summary

This PR fixes an issue described in #6650 (comment):

[ ... ] 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.

A new argument is introduced to the /amp/v1/scannable-url endpoint: force_standard_mode. When it is set to true, the Standard template mode will be used for generating a list of scannable URLs.

This update allows us to fetch a correct set of URLs needed in the Site Scan step of the Onboarding Wizard.

Whenever AMP settings are saved, a new set of scannable URLs is fetched. This way we can guarantee that only relevant pages are listed in last step of the Wizard (the Review panel).

Update

This PR has been updated with a fix to #6692. The amp-first query parameter has been dropped in favour of amp_validate[force_standard_mode]=1. Moreover, the Site Scan in the Wizard will cache the validation results. This way, if a user selects the Standard mode in the Wizard, they will not see the "Stale results" notice on the Settings screen anymore.

Also, when landing on the AMP Settings screen after completing the Onboarding Wizard, a scan-if-stale query parameter is added to the URL. If the scan results are stale, a site scan will be initiated automatically and the query parameter will be removed from the URL.

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 · 2021-11-03T20:03:46Z

Plugin builds for ee32eb7 are ready 🛎️!

Download development build
Download production build

delawski · 2021-11-03T19:59:57Z

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

-	useEffect( () => {
-		if ( status ) {
-			return;
-		}
-
-		if ( ! validateNonce ) {
-			throw new Error( 'Invalid site scan configuration' );
-		}
-
-		dispatch( { type: ACTION_SCANNABLE_URLS_REQUEST } );
-	}, [ status, validateNonce ] );


Since the scannable URLs are now fetched in a different way, there was no reason to keep the useEffect. The validateNonce check can be done directly in the context provider body.

delawski · 2021-11-03T20:07:30Z

src/Validation/ScannableURLsRestController.php

+		// Allow query parameter to force a response to be served with Standard mode (AMP-first). This is used as
+		// part of Site Scanning in order to determine if the primary theme is suitable for serving AMP.
+		if ( ! amp_is_canonical() && $request->get_param( 'force_standard_mode' ) ) {
+			add_filter(
+				'option_' . AMP_Options_Manager::OPTION_NAME,
+				function ( $options ) {
+					$options[ Option::THEME_SUPPORT ] = AMP_Theme_Support::STANDARD_MODE_SLUG;
+
+					return $options;
+				}
+			);
+		}
+


This is very much inspired by a similar filter introduced in #6615.

I was trying to write a unit test for that but I couldn't get the option filter to actually override the template mode (even though it works as expected on the live site). After adding the following test to the ScannableURLsRestControllerTest.php, I was still getting just 2 scannable URLs (the assertGreaterThan assertion failed):

b/tests/php/src/Validation/ScannableURLsRestControllerTest.php --- a/tests/php/src/Validation/ScannableURLsRestControllerTest.php (revision 2540a67282420840f4c6cf109dc960b3db589970) +++ b/tests/php/src/Validation/ScannableURLsRestControllerTest.php (date 1635969779679) @@ -142,6 +142,14 @@ $this->assertNull( $scannable_url_entry['stale'] ); } } + + // Test `force_standard_mode` query parameter. + $request_with_forced_standard_mode = new WP_REST_Request( 'GET', '/amp/v1/scannable-urls', [ 'force_standard_mode' => true ] ); + $response_with_forced_standard_mode = rest_get_server()->dispatch( $request_with_forced_standard_mode ); + + $this->assertFalse( $response_with_forced_standard_mode->is_error() ); + $scannable_urls_in_standard_mode = $response_with_forced_standard_mode->get_data(); + $this->assertGreaterThan( 2, count( $scannable_urls_in_standard_mode ), 'Expected more than two URLs since in Standard mode.' ); } /**

It's probably because the options are cached in a member variable:

amp-wp/src/Validation/ScannableURLProvider.php

Lines 79 to 87 in 46e0b76

private function get_options() {

if ( null === $this->options ) {

$this->options = array_merge(

AMP_Options_Manager::get_options(),

$this->option_overrides

);

}

return $this->options;

}

I determined this was indeed the case, but there was also a secondary cause: when an option is not set in the DB, the option_{$option_name} filter does not apply. Instead, the default_option_{$option_name} applies. In the context of the unit tests, the option is not set here, so that is also why it was failing.

As opposed to a filter, I was originally thinking that ScannableURLProvider could have a set_options method that would allow you to override the options used for scanning. However, I see that this will not fully work because amp_is_post_supported() does not consider the options stored in ScannableURLProvider but rather the options stored in the DB.

So it seems filters are the way to go, and to remove any caching in ScannableURLProvider.

Oh, and a third reason this doesn't work is the [ 'force_standard_mode' => true ] was being passed in as the $attributes and not as params. I believe it should have been done as follows:

// Test `force_standard_mode` query parameter. $request_with_forced_standard_mode = new WP_REST_Request( 'GET', '/amp/v1/scannable-urls' ); $request_with_forced_standard_mode->set_param( 'force_standard_mode', 'true' );

I've addressed these concerns in bf243ab and 4c8d969.

delawski · 2021-11-03T20:22:02Z

Note that the failing E2E tests should be fixed in #6675. Once it's approved and merged, develop can be merged back to this branch and the CI error should be gone.

…cannable-urls-force-standard-mode * 'develop' of github.com:ampproject/amp-wp: (23 commits) Use exact match for gutenberg slugs Bump axios from 0.21.1 to 0.24.0 Bump @babel/core from 7.15.8 to 7.16.0 Update `package-lock.json` Further harmonize themes and plugins API filtering Reuse obtained plugins array and count Increase JS unit test coverage Clean up Validated URLs index before some E2E tests Use XPath in E2E test to ensure iframe src has been updated Fix pagination of AMP-compatible theme list page Fix pagination of AMP-compatible plugins list page Bump eslint-plugin-jsdoc from 36.1.1 to 37.0.3 Bump svgo from 2.7.0 to 2.8.0 Bump lint-staged from 11.2.3 to 11.2.6 Bump postcss from 8.3.9 to 8.3.11 Treat shortcode error like plugin and omit Gutenberg Bump @babel/plugin-proposal-class-properties from 7.14.5 to 7.16.0 Update Gutenberg package dependencies Bump eslint-plugin-import from 2.24.2 to 2.25.2 Move setup code to `beforeAll` lifecycle hook ...

westonruter · 2021-11-04T22:07:19Z

Alas, no:

…LProvider

westonruter · 2021-11-04T23:26:39Z

@delawski What about the change to replace amp-first with amp_validate[force_standard_mode]=1 and then to cache the site scan results when in the Onboarding Wizard so that by chance if they selected Standard mode the results would not be stale? We should still automatically initiate a scan after leaving the onboarding wizard in case Standard mode was not selected. I don't think we have an issue for this yet, but all of this could be tackled together there.

Update: See #6692.

…ery param See: #6692

…t titles

…ndard-mode' into feature/scannable-urls-force-standard-mode

…now that amp-first is removed

…ndard-mode' into feature/scannable-urls-force-standard-mode

delawski · 2021-11-05T19:18:42Z

Er, but then it may have been removed by the time that hasQueryArg checks for it above.

Yes, we need to read it on the client side so that the site scan can be initiated.

westonruter · 2021-11-05T19:20:55Z

I found an issue with the Done screen:

Set template mode to Transitional.
Initiated site scan from Settings Screen
Opened wizard
Passed through Site Scan (which scanned 8 URLs as expected)
Switched template mode to Reader
Selected LEGACY thene.
Problem: I see URLs on the Done screen which are not available in Reader mode, like the Category and Date archives.

The problem is that after the options are saved, the force_standard_mode parameter is still being passed to the scannable-urls endpoint:

/wp-json/amp/v1/scannable-urls?_fields%5B0%5D=url&_fields%5B1%5D=amp_url&_fields%5B2%5D=type&_fields%5B3%5D=label&force_standard_mode=1&_locale=user

This parameter should only be present when opening the Wizard. Once the options have been saved and the user is at the Done screen, the parameter should be omitted.

westonruter · 2021-11-05T19:22:34Z

Yes, we need to read it on the client side so that the site scan can be initiated.

It's still initially available client-side. WordPress core uses the same replaceState method to scrub removable query vars. See wp_admin_canonical_url().

But that would run earlier than our code, so it would break things.

…een from Customizer after selecting Reader mode in Wizard

delawski · 2021-11-05T19:44:01Z

The problem is that after the options are saved, the force_standard_mode parameter is still being passed to the scannable-urls endpoint:
/wp-json/amp/v1/scannable-urls?_fields%5B0%5D=url&_fields%5B1%5D=amp_url&_fields%5B2%5D=type&_fields%5B3%5D=label&force_standard_mode=1&_locale=user
This parameter should only be present when opening the Wizard. Once the options have been saved and the user is at the Done screen, the parameter should be omitted.

This has been addressed in e464460

…nd mode being forced

westonruter · 2021-11-05T22:41:09Z

Witn ee32eb7 the run time of PHPUnit tests is reduced by 5 seconds.

milindmore22 · 2021-12-08T16:19:58Z

QA Passed
Wizard Site Scanner is forcing Standard Mode with force_standard_mode parameter as observed in the request.
https://amp-local.test/?amp_validate%5Bcache%5D=true&amp_validate%5Bcache_bust%5D=0.7579931808456741&amp_validate%5Bforce_standard_mode%5D=true&amp_validate%5Bnonce%5D=09a867751491632767b4438b9019d1ae&amp_validate%5Bomit_stylesheets%5D=true

delawski added 2 commits November 3, 2021 20:21

Allow forcing Standard mode when fetching scannable URLs

cd8e0b4

Fetch scannable URLs when needed; force Standard mode in Wizard

2540a67

delawski added the Site Scanning label Nov 3, 2021

delawski requested review from westonruter and dhaval-parekh November 3, 2021 19:57

delawski commented Nov 3, 2021

View reviewed changes

Update unit test

22590c6

westonruter added this to the v2.2 milestone Nov 4, 2021

westonruter added 2 commits November 4, 2021 15:59

Eliminate caching of options and supportable_templates in ScannableUR…

bf243ab

…LProvider

Make filtering of options more robust

4c8d969

westonruter added 2 commits November 4, 2021 16:27

Store force_standard_mode in constant

65c27b0

Remove option overrides from ScannableURLProvider

426af03

westonruter mentioned this pull request Nov 5, 2021

Avoid showing Stale Results when landing on Settings Screen from Onboarding Wizard #6692

Closed

delawski and others added 11 commits November 5, 2021 15:20

Use amp_validate[force_standard_mode]=1 in favour of amp-first qu…

95ca977

…ery param See: #6692

Add E2E test covering Wizard to Settings screen flow

b82d40b

Fix Test_AMP_Validation_Manager::test_init after 95ca977

8560351

Auto-start site scan after completing Wizard if results are stale

8537b32

Update eslint rule name to jest/lowercase-name; change some E2E tes…

184569c

…t titles

Merge remote-tracking branch 'origin/feature/scannable-urls-force-sta…

56908c3

…ndard-mode' into feature/scannable-urls-force-standard-mode

Remove obsolete filter_options_when_force_standard_mode_request code …

883a88b

…now that amp-first is removed

Always drop scan-if-stale query param on Settings screen

a7b6756

Revert eslint rule name change to jest/prefer-lowercase-title

defe048

Merge remote-tracking branch 'origin/feature/scannable-urls-force-sta…

87c0dd0

…ndard-mode' into feature/scannable-urls-force-standard-mode

Update E2E tests

9a27e3b

delawski and others added 6 commits November 5, 2021 20:28

Do not force_standard_mode when fetching scannable URLs by default

e464460

Fix E2E test

58d4b20

Add amp prefix to scan-if-stale query var

655ed23

Ensure automatic re-scan is initiated after returning to Settings scr…

d7adcd6

…een from Customizer after selecting Reader mode in Wizard

Use constant for amp-scan-if-stale query param

8e1f722

Add amp-scan-if-stale param to Onboarding Wizard "Close" link

4277226

westonruter and others added 7 commits November 5, 2021 12:49

Add missing AMP_SCAN_IF_STALE export on Settings screen

7928432

Bump up timeout for some E2E tests

93ffb36

E2E: allow more time for Settings screen to load after completing Wizard

be6a029

Combine Setting after Wizard E2E tests; always wait longer after Wizard

e0495cb

Test restriction of scannable URLs when in legacy Reader mode

9989bbf

Ensure ALL_TEMPLATES_SUPPORTED is consistent between scannable URLs a…

7606643

…nd mode being forced

Add response mocking to speed up PHPUnit tests

ee32eb7

westonruter approved these changes Nov 5, 2021

View reviewed changes

westonruter merged commit d903d69 into develop Nov 5, 2021

westonruter deleted the feature/scannable-urls-force-standard-mode branch November 5, 2021 22:47

maitreyie-chavan assigned fellyph and milindmore22 Dec 8, 2021

westonruter mentioned this pull request Dec 8, 2021

Site scanning performed multiple times on Onboarding Wizard #6741

Closed

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

Force Standard mode when fetching scannable URLs in Wizard #6683

Force Standard mode when fetching scannable URLs in Wizard #6683

delawski commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading

delawski Nov 3, 2021

delawski Nov 3, 2021 •

edited

Loading

westonruter Nov 4, 2021

westonruter Nov 4, 2021

delawski commented Nov 3, 2021

westonruter commented Nov 4, 2021 •

edited

Loading

westonruter commented Nov 4, 2021 •

edited

Loading

delawski commented Nov 5, 2021

westonruter commented Nov 5, 2021

westonruter commented Nov 5, 2021

delawski commented Nov 5, 2021 •

edited

Loading

westonruter commented Nov 5, 2021

milindmore22 commented Dec 8, 2021

	private function get_options() {
	if ( null === $this->options ) {
	$this->options = array_merge(
	AMP_Options_Manager::get_options(),
	$this->option_overrides
	);
	}
	return $this->options;
	}

Force Standard mode when fetching scannable URLs in Wizard #6683

Force Standard mode when fetching scannable URLs in Wizard #6683

Conversation

delawski commented Nov 3, 2021 • edited Loading

Summary

Update

Checklist

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

delawski Nov 3, 2021

Choose a reason for hiding this comment

delawski Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

westonruter Nov 4, 2021

Choose a reason for hiding this comment

westonruter Nov 4, 2021

Choose a reason for hiding this comment

delawski commented Nov 3, 2021

westonruter commented Nov 4, 2021 • edited Loading

westonruter commented Nov 4, 2021 • edited Loading

delawski commented Nov 5, 2021

westonruter commented Nov 5, 2021

westonruter commented Nov 5, 2021

delawski commented Nov 5, 2021 • edited Loading

westonruter commented Nov 5, 2021

milindmore22 commented Dec 8, 2021

delawski commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading

delawski Nov 3, 2021 •

edited

Loading

westonruter commented Nov 4, 2021 •

edited

Loading

westonruter commented Nov 4, 2021 •

edited

Loading

delawski commented Nov 5, 2021 •

edited

Loading