Skip to content

Commit

Permalink
Trigger async scan when updating suppressed plugins instead of doing …
Browse files Browse the repository at this point in the history 
…synchronous validation
  • Loading branch information
@delawski
delawski committed Nov 16, 2021
1 parent 253e5fd commit 87e609c
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 87 deletions.
7 changes: 5 additions & 2 deletions assets/src/components/options-context-provider/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
const [ updates, setUpdates ] = useState( {} );
const [ fetchingPluginSuppression, setFetchingPluginSuppression ] = useState( false );
const [ fetchingOptions, setFetchingOptions ] = useState( null );
const [ savedOptions, setSavedOptions ] = useState( {} );
const [ savingOptions, setSavingOptions ] = useState( false );
const [ didSaveOptions, setDidSaveOptions ] = useState( false );
const [ originalOptions, setOriginalOptions ] = useState( {} );
Expand Down Expand Up @@ -177,7 +178,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef

// Ensure this promise lasts at least a second so that the "Saving Options" load screen is
// visible long enough for the user to see it is happening.
const [ savedOptions ] = await Promise.all(
const [ retrievedOptions ] = await Promise.all(
[
apiFetch(
{
Expand All @@ -194,7 +195,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
return;
}

setOriginalOptions( savedOptions );
setOriginalOptions( retrievedOptions );
setError( null );
} catch ( e ) {
if ( true === hasUnmounted.current ) {
Expand All @@ -212,6 +213,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
}

setModifiedOptions( { ...modifiedOptions, ...updates } );
setSavedOptions( updates );

setUpdates( {} );
setDidSaveOptions( true );
Expand Down Expand Up @@ -246,6 +248,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
updates,
originalOptions,
saveOptions,
savedOptions,
savingOptions,
unsetOption,
updateOptions,
Expand Down
26 changes: 19 additions & 7 deletions assets/src/components/site-scan-context-provider/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
} from '@wordpress/element';
import apiFetch from '@wordpress/api-fetch';
import { addQueryArgs } from '@wordpress/url';
import { usePrevious } from '@wordpress/compose';

/**
* External dependencies
Expand Down Expand Up @@ -209,10 +208,10 @@ export function SiteScanContextProvider( {
validateNonce,
} ) {
const {
didSaveOptions,
originalOptions: {
theme_support: themeSupport,
},
savedOptions,
refetchPluginSuppression,
} = useContext( Options );
const { setAsyncError } = useAsyncError();
Expand Down Expand Up @@ -292,13 +291,26 @@ export function SiteScanContextProvider( {
* Whenever options change, cancel the current scan (if in progress) and
* refetch the scannable URLs.
*/
const previousDidSaveOptions = usePrevious( didSaveOptions );
useEffect( () => {
if ( ! previousDidSaveOptions && didSaveOptions ) {
cancelSiteScan();
fetchScannableUrls();
if ( Object.keys( savedOptions ).length > 0 ) {
dispatch( { type: ACTION_SCAN_CANCEL } );
dispatch( { type: ACTION_SCANNABLE_URLS_REQUEST } );
}
}, [ cancelSiteScan, didSaveOptions, fetchScannableUrls, previousDidSaveOptions ] );
}, [ savedOptions ] );

/**
* Trigger site scan if the suppressed plugins list has changed and the
* scanner is ready to start a scan.
*/
useEffect( () => {
if (
status === STATUS_READY &&
savedOptions?.suppressed_plugins &&
Object.keys( savedOptions?.suppressed_plugins ).length > 0
) {
dispatch( { type: ACTION_SCAN_INITIALIZE } );
}
}, [ savedOptions, status ] );

/**
* Delay concurrent validation requests.
Expand Down
46 changes: 2 additions & 44 deletions src/PluginSuppression.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,8 @@ public function sanitize_options( $options, $new_options ) {
$option = $options[ Option::SUPPRESSED_PLUGINS ];
$posted_suppressed_plugins = $new_options[ Option::SUPPRESSED_PLUGINS ];

$plugins = $this->plugin_registry->get_plugins( true );
$errors_by_source = AMP_Validated_URL_Post_Type::get_recent_validation_errors_by_source();
$urls_by_frequency = [];
$changes = 0;
$plugins = $this->plugin_registry->get_plugins( true );
$errors_by_source = AMP_Validated_URL_Post_Type::get_recent_validation_errors_by_source();
foreach ( $posted_suppressed_plugins as $plugin_slug => $suppressed ) {
if ( ! isset( $plugins[ $plugin_slug ] ) ) {
unset( $option[ $plugin_slug ] );
Expand All @@ -211,20 +209,8 @@ public function sanitize_options( $options, $new_options ) {
$suppressed = rest_sanitize_boolean( $suppressed );
if ( isset( $option[ $plugin_slug ] ) && ! $suppressed ) {

// Gather the URLs on which the error occurred, keeping track of the frequency so that we can use the URL with the most errors to re-validate.
if ( ! empty( $option[ $plugin_slug ][ Option::SUPPRESSED_PLUGINS_ERRORING_URLS ] ) ) {
foreach ( $option[ $plugin_slug ][ Option::SUPPRESSED_PLUGINS_ERRORING_URLS ] as $url ) {
if ( ! isset( $urls_by_frequency[ $url ] ) ) {
$urls_by_frequency[ $url ] = 0;
}
$urls_by_frequency[ $url ]++;
}
}

// Remove the plugin from being suppressed.
unset( $option[ $plugin_slug ] );

$changes++;
} elseif ( ! isset( $option[ $plugin_slug ] ) && $suppressed && array_key_exists( $plugin_slug, $plugins ) ) {

// Capture the URLs that the error occurred on so we can check them again when the plugin is re-activated.
Expand All @@ -250,37 +236,9 @@ public function sanitize_options( $options, $new_options ) {
Option::SUPPRESSED_PLUGINS_USERNAME => $user instanceof WP_User ? $user->user_nicename : null,
Option::SUPPRESSED_PLUGINS_ERRORING_URLS => array_unique( array_filter( $urls ) ),
];
$changes++;
}
}

// When the suppressed plugins changed, re-validate so validation errors can be re-computed with the plugins newly-suppressed or un-suppressed.
if ( $changes > 0 ) {
add_action(
'update_option_' . AMP_Options_Manager::OPTION_NAME,
static function () use ( $urls_by_frequency ) {
$url = null;
if ( count( $urls_by_frequency ) > 0 ) {
arsort( $urls_by_frequency );
$url = key( $urls_by_frequency );
} else {
$validated_url_posts = get_posts(
[
'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,
'posts_per_page' => 1,
]
);
if ( count( $validated_url_posts ) > 0 ) {
$url = AMP_Validated_URL_Post_Type::get_url_from_post( $validated_url_posts[0] );
}
}
if ( $url ) {
AMP_Validation_Manager::validate_url_and_store( $url );
}
}
);
}

$options[ Option::SUPPRESSED_PLUGINS ] = $option;

return $options;
Expand Down
24 changes: 24 additions & 0 deletions tests/e2e/specs/admin/site-scan-panel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,28 @@ describe( 'AMP settings screen Site Scan panel', () => {
// Clean up.
await installPlugin( 'autoptimize' );
} );

it( 'automatically triggers a scan if Plugin Suppression option has changed', async () => {
await activatePlugin( 'autoptimize' );

await visitAdminPage( 'admin.php', 'page=amp-options' );

// Suppress the plugin.
await scrollToElement( { selector: '#plugin-suppression .components-panel__body-toggle', click: true } );
await expect( page ).toSelect( '#suppressed-plugins-table tbody tr:first-child .column-status select', 'Suppressed' );
await scrollToElement( { selector: '#site-scan' } );

// Save options.
await Promise.all( [
scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ),
page.waitForResponse( ( response ) => response.url().includes( '/wp-json/amp/v1/options' ) ),
] );

await testSiteScanning( {
statusElementClassName: 'settings-site-scan__status',
isAmpFirst: false,
} );

await deactivatePlugin( 'autoptimize' );
} );
} );
4 changes: 1 addition & 3 deletions tests/e2e/utils/site-scan-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ export async function testSiteScanning( { statusElementClassName, isAmpFirst } )

expect( statusText ).toMatch( statusTextRegex );

const currentlyScannedIndex = Number( statusText.match( statusTextRegex )[ 1 ] ) - 1;
const scannableUrlsCount = Number( statusText.match( statusTextRegex )[ 2 ] );
const urls = [ ...Array( scannableUrlsCount - currentlyScannedIndex ) ];

const expectedParams = [
'amp_validate[cache]',
Expand All @@ -21,7 +19,7 @@ export async function testSiteScanning( { statusElementClassName, isAmpFirst } )
const timeout = 20000;

await Promise.all( [
...urls.map( ( url, index ) => page.waitForXPath( `//p[@class='${ statusElementClassName }'][contains(text(), 'Scanning ${ index + 1 }/${ scannableUrlsCount } URLs')]`, { timeout } ) ),
page.waitForXPath( `//p[@class='${ statusElementClassName }'][contains(text(), 'Scanning ${ scannableUrlsCount }/${ scannableUrlsCount } URLs')]`, { timeout } ),
page.waitForResponse( ( response ) => isAmpFirst === response.url().includes( encodeURI( 'amp_validate[force_standard_mode]' ) ) && expectedParams.every( ( param ) => response.url().includes( param ) ), { timeout } ),
page.waitForXPath( `//p[@class='${ statusElementClassName }'][contains(text(), 'Scan complete')]`, { timeout } ),
] );
Expand Down
31 changes: 0 additions & 31 deletions tests/php/src/PluginSuppressionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
use AMP_Options_Manager;
use AMP_Theme_Support;
use AMP_Validated_URL_Post_Type;
use AMP_Validation_Manager;
use AmpProject\AmpWP\Admin\ReaderThemes;
use AmpProject\AmpWP\Tests\Helpers\WithoutBlockPreRendering;
use Exception;
Expand All @@ -29,8 +28,6 @@ final class PluginSuppressionTest extends DependencyInjectedTestCase {
setUp as public prevent_block_pre_render;
}

private $attempted_validate_request_urls = [];

/** @var PluginSuppression */
private $instance;

Expand All @@ -42,27 +39,6 @@ public function setUp() {
$this->add_reader_themes_request_filter();

$this->reset_widgets();
add_filter(
'pre_http_request',
function( $r, /** @noinspection PhpUnusedParameterInspection */ $args, $url ) {
$url_query_vars = [];
parse_str( wp_parse_url( $url, PHP_URL_QUERY ), $url_query_vars );
if ( ! array_key_exists( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url_query_vars ) ) {
return $r;
}

$this->attempted_validate_request_urls[] = remove_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url );
return [
'body' => '',
'response' => [
'code' => 503,
'message' => 'Service Unavailable',
],
];
},
10,
3
);

$plugin_suppression = $this->injector->make( PluginSuppression::class );
$plugin_registry = $this->get_private_property( $plugin_suppression, 'plugin_registry' );
Expand Down Expand Up @@ -92,7 +68,6 @@ function( $r, /** @noinspection PhpUnusedParameterInspection */ $args, $url ) {
*/
public function tearDown() {
parent::tearDown();
$this->attempted_validate_request_urls = [];

$GLOBALS['wp_settings_fields'] = [];
$GLOBALS['wp_registered_settings'] = [];
Expand Down Expand Up @@ -414,7 +389,6 @@ public function test_sanitize_options() {
wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
AMP_Options_Manager::register_settings(); // Adds validate_options as filter.
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
AMP_Validation_Manager::init();

$bad_plugin_file_slugs = $this->get_bad_plugin_file_slugs();
// Test initial state.
Expand All @@ -424,7 +398,6 @@ public function test_sanitize_options() {
AMP_Options_Manager::update_option( Option::SUPPRESSED_PLUGINS, [] );
$this->assertEquals( [], AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS ) );

$this->assertCount( 0, $this->attempted_validate_request_urls );
$this->assertEmpty( AMP_Validated_URL_Post_Type::get_recent_validation_errors_by_source() );

$this->populate_validation_errors( home_url( '/' ), $bad_plugin_file_slugs );
Expand All @@ -442,7 +415,6 @@ public function test_sanitize_options() {
);

$this->assertEquals( [], AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS ) );
$this->assertCount( 0, $this->attempted_validate_request_urls );

// When updating option but both plugins are not suppressed, then no change is made.
$this->update_suppressed_plugins_option(
Expand All @@ -452,7 +424,6 @@ public function test_sanitize_options() {
)
);
$this->assertEquals( [], AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS ) );
$this->assertCount( 0, $this->attempted_validate_request_urls, 'Expected no validation request to have been made since no changes were made (as both plugins are still unsuppressed).' );

// When updating option and both are now suppressed, then a change is made.
$this->update_suppressed_plugins_option(
Expand All @@ -461,7 +432,6 @@ public function test_sanitize_options() {
'1'
)
);
$this->assertCount( 1, $this->attempted_validate_request_urls, 'Expected one validation request to have been made since no changes were made (as both plugins are still unsuppressed).' );
$suppressed_plugins = AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS );
$this->assertEqualSets( $bad_plugin_file_slugs, array_keys( $suppressed_plugins ) );
foreach ( $suppressed_plugins as $slug => $suppressed_plugin ) {
Expand All @@ -483,7 +453,6 @@ public function test_sanitize_options() {
array_fill_keys( $unsuppressed_plugins, '0' )
)
);
$this->assertCount( 2, $this->attempted_validate_request_urls, 'Expected one validation request to have been made since no changes were made (as both plugins are still unsuppressed).' );
$this->assertEqualSets( $suppressed_plugins, array_keys( AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS ) ) );
}

Expand Down

0 comments on commit 87e609c

Please sign in to comment.