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

Force Standard mode when fetching scannable URLs in Wizard #6683

Merged
merged 32 commits into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
cd8e0b4
Allow forcing Standard mode when fetching scannable URLs
delawski Nov 3, 2021
2540a67
Fetch scannable URLs when needed; force Standard mode in Wizard
delawski Nov 3, 2021
22590c6
Update unit test
delawski Nov 3, 2021
43efaed
Merge branch 'develop' of github.com:ampproject/amp-wp into feature/s…
westonruter Nov 4, 2021
bf243ab
Eliminate caching of options and supportable_templates in ScannableUR…
westonruter Nov 4, 2021
4c8d969
Make filtering of options more robust
westonruter Nov 4, 2021
65c27b0
Store force_standard_mode in constant
westonruter Nov 4, 2021
426af03
Remove option overrides from ScannableURLProvider
westonruter Nov 5, 2021
95ca977
Use `amp_validate[force_standard_mode]=1` in favour of `amp-first` qu…
delawski Nov 5, 2021
b82d40b
Add E2E test covering Wizard to Settings screen flow
delawski Nov 5, 2021
8560351
Fix Test_AMP_Validation_Manager::test_init after 95ca97798e
westonruter Nov 5, 2021
8537b32
Auto-start site scan after completing Wizard if results are stale
delawski Nov 5, 2021
184569c
Update eslint rule name to `jest/lowercase-name`; change some E2E tes…
delawski Nov 5, 2021
56908c3
Merge remote-tracking branch 'origin/feature/scannable-urls-force-sta…
delawski Nov 5, 2021
883a88b
Remove obsolete filter_options_when_force_standard_mode_request code …
westonruter Nov 5, 2021
a7b6756
Always drop `scan-if-stale` query param on Settings screen
delawski Nov 5, 2021
defe048
Revert eslint rule name change to `jest/prefer-lowercase-title`
delawski Nov 5, 2021
87c0dd0
Merge remote-tracking branch 'origin/feature/scannable-urls-force-sta…
delawski Nov 5, 2021
9a27e3b
Update E2E tests
delawski Nov 5, 2021
e464460
Do not `force_standard_mode` when fetching scannable URLs by default
delawski Nov 5, 2021
58d4b20
Fix E2E test
delawski Nov 5, 2021
655ed23
Add amp prefix to scan-if-stale query var
westonruter Nov 5, 2021
d7adcd6
Ensure automatic re-scan is initiated after returning to Settings scr…
westonruter Nov 5, 2021
8e1f722
Use constant for amp-scan-if-stale query param
westonruter Nov 5, 2021
4277226
Add `amp-scan-if-stale` param to Onboarding Wizard "Close" link
delawski Nov 5, 2021
7928432
Add missing AMP_SCAN_IF_STALE export on Settings screen
westonruter Nov 5, 2021
93ffb36
Bump up timeout for some E2E tests
delawski Nov 5, 2021
be6a029
E2E: allow more time for Settings screen to load after completing Wizard
delawski Nov 5, 2021
e0495cb
Combine Setting after Wizard E2E tests; always wait longer after Wizard
delawski Nov 5, 2021
9989bbf
Test restriction of scannable URLs when in legacy Reader mode
westonruter Nov 5, 2021
7606643
Ensure ALL_TEMPLATES_SUPPORTED is consistent between scannable URLs a…
westonruter Nov 5, 2021
ee32eb7
Add response mocking to speed up PHPUnit tests
westonruter Nov 5, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions assets/src/components/site-scan-context-provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ function siteScanReducer( state, action ) {
...state,
status: STATUS_REQUEST_SCANNABLE_URLS,
currentlyScannedUrlIndex: INITIAL_STATE.currentlyScannedUrlIndex,
forceStandardMode: action?.forceStandardMode ?? state.forceStandardMode,
};
}
case ACTION_SCANNABLE_URLS_FETCH: {
Expand Down Expand Up @@ -169,14 +170,12 @@ function siteScanReducer( state, action ) {
* Context provider for site scanning.
*
* @param {Object} props Component props.
* @param {boolean} props.ampFirst Whether scanning should be done with Standard mode being forced.
* @param {?any} props.children Component children.
* @param {boolean} props.fetchCachedValidationErrors Whether to fetch cached validation errors on mount.
* @param {string} props.scannableUrlsRestPath The REST path for interacting with the scannable URL resources.
* @param {string} props.validateNonce The AMP validate nonce.
*/
export function SiteScanContextProvider( {
ampFirst = false,
children,
fetchCachedValidationErrors = false,
scannableUrlsRestPath,
Expand All @@ -193,10 +192,11 @@ export function SiteScanContextProvider( {
const {
cache,
currentlyScannedUrlIndex,
forceStandardMode,
scannableUrls,
status,
} = state;
const urlType = ampFirst || themeSupport === STANDARD ? 'url' : 'amp_url';
const urlType = forceStandardMode || themeSupport === STANDARD ? 'url' : 'amp_url';
const previewPermalink = scannableUrls?.[ 0 ]?.[ urlType ] ?? '';

/**
Expand Down Expand Up @@ -232,17 +232,9 @@ export function SiteScanContextProvider( {
/**
* Preflight check.
*/
useEffect( () => {
if ( status ) {
return;
}

if ( ! validateNonce ) {
throw new Error( 'Invalid site scan configuration' );
}

dispatch( { type: ACTION_SCANNABLE_URLS_REQUEST } );
}, [ status, validateNonce ] );
Comment on lines -235 to -245
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if ( ! validateNonce ) {
throw new Error( 'Invalid site scan configuration' );
}

/**
* This component sets state inside async functions. Use this ref to prevent
Expand All @@ -253,6 +245,13 @@ export function SiteScanContextProvider( {
hasUnmounted.current = true;
}, [] );

const fetchScannableUrls = useCallback( ( args = {} ) => {
dispatch( {
type: ACTION_SCANNABLE_URLS_REQUEST,
forceStandardMode: args?.forceStandardMode,
} );
}, [] );

const startSiteScan = useCallback( ( args = {} ) => {
dispatch( {
type: ACTION_SCAN_INITIALIZE,
Expand All @@ -271,9 +270,9 @@ export function SiteScanContextProvider( {
const previousDidSaveOptions = usePrevious( didSaveOptions );
useEffect( () => {
if ( ! previousDidSaveOptions && didSaveOptions ) {
dispatch( { type: ACTION_SCANNABLE_URLS_REQUEST } );
fetchScannableUrls();
}
}, [ didSaveOptions, previousDidSaveOptions ] );
}, [ didSaveOptions, fetchScannableUrls, previousDidSaveOptions ] );

/**
* Fetch scannable URLs from the REST endpoint.
Expand All @@ -291,6 +290,7 @@ export function SiteScanContextProvider( {
const response = await apiFetch( {
path: addQueryArgs( scannableUrlsRestPath, {
_fields: fetchCachedValidationErrors ? [ ...fields, 'validation_errors', 'stale' ] : fields,
force_standard_mode: forceStandardMode ? 1 : undefined,
} ),
} );

Expand All @@ -306,7 +306,7 @@ export function SiteScanContextProvider( {
setAsyncError( e );
}
} )();
}, [ fetchCachedValidationErrors, scannableUrlsRestPath, setAsyncError, status ] );
}, [ fetchCachedValidationErrors, forceStandardMode, scannableUrlsRestPath, setAsyncError, status ] );

/**
* Scan site URLs sequentially.
Expand All @@ -322,7 +322,7 @@ export function SiteScanContextProvider( {
try {
const url = scannableUrls[ currentlyScannedUrlIndex ][ urlType ];
const args = {
'amp-first': ampFirst || undefined,
'amp-first': forceStandardMode || undefined,
amp_validate: {
cache: cache || undefined,
nonce: validateNonce,
Expand Down Expand Up @@ -362,13 +362,14 @@ export function SiteScanContextProvider( {

dispatch( { type: ACTION_SCAN_NEXT_URL } );
} )();
}, [ ampFirst, cache, currentlyScannedUrlIndex, scannableUrls, setAsyncError, status, urlType, validateNonce ] );
}, [ cache, currentlyScannedUrlIndex, forceStandardMode, scannableUrls, setAsyncError, status, urlType, validateNonce ] );

return (
<SiteScan.Provider
value={ {
cancelSiteScan,
currentlyScannedUrlIndex,
fetchScannableUrls,
hasSiteScanResults,
isBusy: [ STATUS_IDLE, STATUS_IN_PROGRESS ].includes( status ),
isCancelled: status === STATUS_CANCELLED,
Expand All @@ -391,7 +392,6 @@ export function SiteScanContextProvider( {
}

SiteScanContextProvider.propTypes = {
ampFirst: PropTypes.bool,
children: PropTypes.any,
fetchCachedValidationErrors: PropTypes.bool,
scannableUrlsRestPath: PropTypes.string,
Expand Down
1 change: 0 additions & 1 deletion assets/src/onboarding-wizard/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ export function Providers( { children } ) {
>
<TemplateModeOverrideContextProvider>
<SiteScanContextProvider
ampFirst={ true }
scannableUrlsRestPath={ SCANNABLE_URLS_REST_PATH }
validateNonce={ VALIDATE_NONCE }
>
Expand Down
4 changes: 3 additions & 1 deletion assets/src/onboarding-wizard/pages/done/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
import { Navigation } from '../../components/navigation-context-provider';
import { Options } from '../../../components/options-context-provider';
import { ReaderThemes } from '../../../components/reader-themes-context-provider';
import { SiteScan } from '../../../components/site-scan-context-provider';
import { User } from '../../../components/user-context-provider';
import { IconLaptopToggles } from '../../../components/svg/icon-laptop-toggles';
import { IconLaptopSearch } from '../../../components/svg/icon-laptop-search';
Expand All @@ -47,6 +48,7 @@ export function Done() {
const { didSaveDeveloperToolsOption, saveDeveloperToolsOption, savingDeveloperToolsOption } = useContext( User );
const { canGoForward, setCanGoForward } = useContext( Navigation );
const { downloadedTheme, downloadingTheme, downloadingThemeError } = useContext( ReaderThemes );
const { isFetchingScannableUrls } = useContext( SiteScan );
const {
hasPreview,
isPreviewingAMP,
Expand Down Expand Up @@ -83,7 +85,7 @@ export function Done() {
}
}, [ didSaveDeveloperToolsOption, savingDeveloperToolsOption, saveDeveloperToolsOption ] );

if ( savingOptions || savingDeveloperToolsOption || downloadingTheme || hasOptionsChanges ) {
if ( savingOptions || savingDeveloperToolsOption || downloadingTheme || hasOptionsChanges || isFetchingScannableUrls ) {
return <Saving />;
}

Expand Down
7 changes: 7 additions & 0 deletions assets/src/onboarding-wizard/setup-wizard.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import PropTypes from 'prop-types';
import { Logo } from '../components/svg/logo';
import { UnsavedChangesWarning } from '../components/unsaved-changes-warning';
import { useWindowWidth } from '../utils/use-window-width';
import { SiteScan } from '../components/site-scan-context-provider';
import { Stepper } from './components/stepper';
import { Nav, CloseLink } from './components/nav';
import { Navigation } from './components/navigation-context-provider';
Expand Down Expand Up @@ -46,6 +47,12 @@ function PageComponentSideEffects( { children } ) {
export function SetupWizard( { closeLink, finishLink, appRoot } ) {
const { isMobile } = useWindowWidth();
const { activePageIndex, currentPage: { title, PageComponent, showTitle }, moveBack, moveForward, pages } = useContext( Navigation );
const { fetchScannableUrls } = useContext( SiteScan );

// Fetch the AMP-first scannable URLs needed for the Site Scan step.
useEffect( () => {
fetchScannableUrls( { forceStandardMode: true } );
}, [ fetchScannableUrls ] );

const PageComponentWithSideEffects = useMemo( () => () => (
<PageComponentSideEffects>
Expand Down
1 change: 0 additions & 1 deletion assets/src/settings-page/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ function Providers( { children } ) {
<PluginsContextProvider hasErrorBoundary={ true }>
<ThemesContextProvider hasErrorBoundary={ true }>
<SiteScanContextProvider
ampFirst={ false }
fetchCachedValidationErrors={ true }
scannableUrlsRestPath={ SCANNABLE_URLS_REST_PATH }
validateNonce={ VALIDATE_NONCE }
Expand Down
9 changes: 7 additions & 2 deletions assets/src/settings-page/site-scan.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import useDelayedFlag from '../utils/use-delayed-flag';
export function SiteScan( { onSiteScan } ) {
const {
cancelSiteScan,
fetchScannableUrls,
hasSiteScanResults,
isBusy,
isCancelled,
Expand All @@ -58,9 +59,13 @@ export function SiteScan( { onSiteScan } ) {
const hasSiteIssues = themesWithAmpIncompatibility.length > 0 || pluginsWithAmpIncompatibility.length > 0;

/**
* Cancel scan when component unmounts.
* Fetch scannable URLs on mount; cancel site scan if the component unmounts.
*/
useEffect( () => () => cancelSiteScan(), [ cancelSiteScan ] );
useEffect( () => {
fetchScannableUrls();

return cancelSiteScan;
}, [ cancelSiteScan, fetchScannableUrls ] );

/**
* Delay the `isCompleted` flag so that the progress bar stays at 100% for a
Expand Down
6 changes: 4 additions & 2 deletions src/Admin/OnboardingWizardSubmenuPage.php
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,10 @@ protected function add_preload_rest_paths() {
'/amp/v1/options',
'/amp/v1/reader-themes',
add_query_arg(
'_fields',
[ 'url', 'amp_url', 'type', 'label' ],
[
'_fields' => [ 'url', 'amp_url', 'type', 'label' ],
'force_standard_mode' => 1,
],
'/amp/v1/scannable-urls'
),
'/wp/v2/plugins',
Expand Down
25 changes: 24 additions & 1 deletion src/Validation/ScannableURLsRestController.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@

namespace AmpProject\AmpWP\Validation;

use AMP_Options_Manager;
use AMP_Theme_Support;
use AMP_Validated_URL_Post_Type;
use AMP_Validation_Manager;
use AmpProject\AmpWP\Infrastructure\Delayed;
use AmpProject\AmpWP\Infrastructure\Registerable;
use AmpProject\AmpWP\Infrastructure\Service;
use AmpProject\AmpWP\Option;
use AmpProject\AmpWP\PairedRouting;
use WP_Error;
use WP_Post;
Expand Down Expand Up @@ -77,7 +80,14 @@ public function register() {
'methods' => WP_REST_Server::READABLE,
'callback' => [ $this, 'get_items' ],
'permission_callback' => [ $this, 'get_items_permissions_check' ],
'args' => [],
'args' => [
'force_standard_mode' => [
'description' => __( 'Indicates whether to force Standard template mode.', 'amp' ),
'type' => 'boolean',
'required' => false,
'default' => false,
],
],
],
'schema' => [ $this, 'get_public_item_schema' ],
]
Expand Down Expand Up @@ -112,6 +122,19 @@ public function get_items_permissions_check( $request ) {
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function get_items( $request ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
// 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;
}
);
}

Copy link
Collaborator Author

@delawski delawski Nov 3, 2021

Choose a reason for hiding this comment

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

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.' );
 	}
 
 	/**

Copy link
Member

Choose a reason for hiding this comment

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

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

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' );

Copy link
Member

Choose a reason for hiding this comment

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

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

return rest_ensure_response(
array_map(
function ( $item ) use ( $request ) {
Expand Down