Skip to content

Commit 87e609c

Browse files
delawskidelawski
committed
Trigger async scan when updating suppressed plugins instead of doing synchronous validation
1 parent 253e5fd commit 87e609c

File tree

6 files changed

+51
-87
lines changed

6 files changed

+51
-87
lines changed

‎assets/src/components/options-context-provider/index.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
4141
const [ updates, setUpdates ] = useState( {} );
4242
const [ fetchingPluginSuppression, setFetchingPluginSuppression ] = useState( false );
4343
const [ fetchingOptions, setFetchingOptions ] = useState( null );
44+
const [ savedOptions, setSavedOptions ] = useState( {} );
4445
const [ savingOptions, setSavingOptions ] = useState( false );
4546
const [ didSaveOptions, setDidSaveOptions ] = useState( false );
4647
const [ originalOptions, setOriginalOptions ] = useState( {} );
@@ -177,7 +178,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
177178

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

197-
setOriginalOptions( savedOptions );
198+
setOriginalOptions( retrievedOptions );
198199
setError( null );
199200
} catch ( e ) {
200201
if ( true === hasUnmounted.current ) {
@@ -212,6 +213,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
212213
}
213214

214215
setModifiedOptions( { ...modifiedOptions, ...updates } );
216+
setSavedOptions( updates );
215217

216218
setUpdates( {} );
217219
setDidSaveOptions( true );
@@ -246,6 +248,7 @@ export function OptionsContextProvider( { children, optionsRestPath, populateDef
246248
updates,
247249
originalOptions,
248250
saveOptions,
251+
savedOptions,
249252
savingOptions,
250253
unsetOption,
251254
updateOptions,

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

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import {
1313
} from '@wordpress/element';
1414
import apiFetch from '@wordpress/api-fetch';
1515
import { addQueryArgs } from '@wordpress/url';
16-
import { usePrevious } from '@wordpress/compose';
1716

1817
/**
1918
* External dependencies
@@ -209,10 +208,10 @@ export function SiteScanContextProvider( {
209208
validateNonce,
210209
} ) {
211210
const {
212-
didSaveOptions,
213211
originalOptions: {
214212
theme_support: themeSupport,
215213
},
214+
savedOptions,
216215
refetchPluginSuppression,
217216
} = useContext( Options );
218217
const { setAsyncError } = useAsyncError();
@@ -292,13 +291,26 @@ export function SiteScanContextProvider( {
292291
* Whenever options change, cancel the current scan (if in progress) and
293292
* refetch the scannable URLs.
294293
*/
295-
const previousDidSaveOptions = usePrevious( didSaveOptions );
296294
useEffect( () => {
297-
if ( ! previousDidSaveOptions && didSaveOptions ) {
298-
cancelSiteScan();
299-
fetchScannableUrls();
295+
if ( Object.keys( savedOptions ).length > 0 ) {
296+
dispatch( { type: ACTION_SCAN_CANCEL } );
297+
dispatch( { type: ACTION_SCANNABLE_URLS_REQUEST } );
300298
}
301-
}, [ cancelSiteScan, didSaveOptions, fetchScannableUrls, previousDidSaveOptions ] );
299+
}, [ savedOptions ] );
300+
301+
/**
302+
* Trigger site scan if the suppressed plugins list has changed and the
303+
* scanner is ready to start a scan.
304+
*/
305+
useEffect( () => {
306+
if (
307+
status === STATUS_READY &&
308+
savedOptions?.suppressed_plugins &&
309+
Object.keys( savedOptions?.suppressed_plugins ).length > 0
310+
) {
311+
dispatch( { type: ACTION_SCAN_INITIALIZE } );
312+
}
313+
}, [ savedOptions, status ] );
302314

303315
/**
304316
* Delay concurrent validation requests.

‎src/PluginSuppression.php

Lines changed: 2 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -198,10 +198,8 @@ public function sanitize_options( $options, $new_options ) {
198198
$option = $options[ Option::SUPPRESSED_PLUGINS ];
199199
$posted_suppressed_plugins = $new_options[ Option::SUPPRESSED_PLUGINS ];
200200

201-
$plugins = $this->plugin_registry->get_plugins( true );
202-
$errors_by_source = AMP_Validated_URL_Post_Type::get_recent_validation_errors_by_source();
203-
$urls_by_frequency = [];
204-
$changes = 0;
201+
$plugins = $this->plugin_registry->get_plugins( true );
202+
$errors_by_source = AMP_Validated_URL_Post_Type::get_recent_validation_errors_by_source();
205203
foreach ( $posted_suppressed_plugins as $plugin_slug => $suppressed ) {
206204
if ( ! isset( $plugins[ $plugin_slug ] ) ) {
207205
unset( $option[ $plugin_slug ] );
@@ -211,20 +209,8 @@ public function sanitize_options( $options, $new_options ) {
211209
$suppressed = rest_sanitize_boolean( $suppressed );
212210
if ( isset( $option[ $plugin_slug ] ) && ! $suppressed ) {
213211

214-
// 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.
215-
if ( ! empty( $option[ $plugin_slug ][ Option::SUPPRESSED_PLUGINS_ERRORING_URLS ] ) ) {
216-
foreach ( $option[ $plugin_slug ][ Option::SUPPRESSED_PLUGINS_ERRORING_URLS ] as $url ) {
217-
if ( ! isset( $urls_by_frequency[ $url ] ) ) {
218-
$urls_by_frequency[ $url ] = 0;
219-
}
220-
$urls_by_frequency[ $url ]++;
221-
}
222-
}
223-
224212
// Remove the plugin from being suppressed.
225213
unset( $option[ $plugin_slug ] );
226-
227-
$changes++;
228214
} elseif ( ! isset( $option[ $plugin_slug ] ) && $suppressed && array_key_exists( $plugin_slug, $plugins ) ) {
229215

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

257-
// When the suppressed plugins changed, re-validate so validation errors can be re-computed with the plugins newly-suppressed or un-suppressed.
258-
if ( $changes > 0 ) {
259-
add_action(
260-
'update_option_' . AMP_Options_Manager::OPTION_NAME,
261-
static function () use ( $urls_by_frequency ) {
262-
$url = null;
263-
if ( count( $urls_by_frequency ) > 0 ) {
264-
arsort( $urls_by_frequency );
265-
$url = key( $urls_by_frequency );
266-
} else {
267-
$validated_url_posts = get_posts(
268-
[
269-
'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,
270-
'posts_per_page' => 1,
271-
]
272-
);
273-
if ( count( $validated_url_posts ) > 0 ) {
274-
$url = AMP_Validated_URL_Post_Type::get_url_from_post( $validated_url_posts[0] );
275-
}
276-
}
277-
if ( $url ) {
278-
AMP_Validation_Manager::validate_url_and_store( $url );
279-
}
280-
}
281-
);
282-
}
283-
284242
$options[ Option::SUPPRESSED_PLUGINS ] = $option;
285243

286244
return $options;

‎tests/e2e/specs/admin/site-scan-panel.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,4 +159,28 @@ describe( 'AMP settings screen Site Scan panel', () => {
159159
// Clean up.
160160
await installPlugin( 'autoptimize' );
161161
} );
162+
163+
it( 'automatically triggers a scan if Plugin Suppression option has changed', async () => {
164+
await activatePlugin( 'autoptimize' );
165+
166+
await visitAdminPage( 'admin.php', 'page=amp-options' );
167+
168+
// Suppress the plugin.
169+
await scrollToElement( { selector: '#plugin-suppression .components-panel__body-toggle', click: true } );
170+
await expect( page ).toSelect( '#suppressed-plugins-table tbody tr:first-child .column-status select', 'Suppressed' );
171+
await scrollToElement( { selector: '#site-scan' } );
172+
173+
// Save options.
174+
await Promise.all( [
175+
scrollToElement( { selector: '.amp-settings-nav button[type="submit"]', click: true } ),
176+
page.waitForResponse( ( response ) => response.url().includes( '/wp-json/amp/v1/options' ) ),
177+
] );
178+
179+
await testSiteScanning( {
180+
statusElementClassName: 'settings-site-scan__status',
181+
isAmpFirst: false,
182+
} );
183+
184+
await deactivatePlugin( 'autoptimize' );
185+
} );
162186
} );

‎tests/e2e/utils/site-scan-utils.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,7 @@ export async function testSiteScanning( { statusElementClassName, isAmpFirst } )
66

77
expect( statusText ).toMatch( statusTextRegex );
88

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

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

2321
await Promise.all( [
24-
...urls.map( ( url, index ) => page.waitForXPath( `//p[@class='${ statusElementClassName }'][contains(text(), 'Scanning ${ index + 1 }/${ scannableUrlsCount } URLs')]`, { timeout } ) ),
22+
page.waitForXPath( `//p[@class='${ statusElementClassName }'][contains(text(), 'Scanning ${ scannableUrlsCount }/${ scannableUrlsCount } URLs')]`, { timeout } ),
2523
page.waitForResponse( ( response ) => isAmpFirst === response.url().includes( encodeURI( 'amp_validate[force_standard_mode]' ) ) && expectedParams.every( ( param ) => response.url().includes( param ) ), { timeout } ),
2624
page.waitForXPath( `//p[@class='${ statusElementClassName }'][contains(text(), 'Scan complete')]`, { timeout } ),
2725
] );

‎tests/php/src/PluginSuppressionTest.php

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
use AMP_Options_Manager;
1515
use AMP_Theme_Support;
1616
use AMP_Validated_URL_Post_Type;
17-
use AMP_Validation_Manager;
1817
use AmpProject\AmpWP\Admin\ReaderThemes;
1918
use AmpProject\AmpWP\Tests\Helpers\WithoutBlockPreRendering;
2019
use Exception;
@@ -29,8 +28,6 @@ final class PluginSuppressionTest extends DependencyInjectedTestCase {
2928
setUp as public prevent_block_pre_render;
3029
}
3130

32-
private $attempted_validate_request_urls = [];
33-
3431
/** @var PluginSuppression */
3532
private $instance;
3633

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

4441
$this->reset_widgets();
45-
add_filter(
46-
'pre_http_request',
47-
function( $r, /** @noinspection PhpUnusedParameterInspection */ $args, $url ) {
48-
$url_query_vars = [];
49-
parse_str( wp_parse_url( $url, PHP_URL_QUERY ), $url_query_vars );
50-
if ( ! array_key_exists( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url_query_vars ) ) {
51-
return $r;
52-
}
53-
54-
$this->attempted_validate_request_urls[] = remove_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, $url );
55-
return [
56-
'body' => '',
57-
'response' => [
58-
'code' => 503,
59-
'message' => 'Service Unavailable',
60-
],
61-
];
62-
},
63-
10,
64-
3
65-
);
6642

6743
$plugin_suppression = $this->injector->make( PluginSuppression::class );
6844
$plugin_registry = $this->get_private_property( $plugin_suppression, 'plugin_registry' );
@@ -92,7 +68,6 @@ function( $r, /** @noinspection PhpUnusedParameterInspection */ $args, $url ) {
9268
*/
9369
public function tearDown() {
9470
parent::tearDown();
95-
$this->attempted_validate_request_urls = [];
9671

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

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

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

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

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

447419
// When updating option but both plugins are not suppressed, then no change is made.
448420
$this->update_suppressed_plugins_option(
@@ -452,7 +424,6 @@ public function test_sanitize_options() {
452424
)
453425
);
454426
$this->assertEquals( [], AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS ) );
455-
$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).' );
456427

457428
// When updating option and both are now suppressed, then a change is made.
458429
$this->update_suppressed_plugins_option(
@@ -461,7 +432,6 @@ public function test_sanitize_options() {
461432
'1'
462433
)
463434
);
464-
$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).' );
465435
$suppressed_plugins = AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS );
466436
$this->assertEqualSets( $bad_plugin_file_slugs, array_keys( $suppressed_plugins ) );
467437
foreach ( $suppressed_plugins as $slug => $suppressed_plugin ) {
@@ -483,7 +453,6 @@ public function test_sanitize_options() {
483453
array_fill_keys( $unsuppressed_plugins, '0' )
484454
)
485455
);
486-
$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).' );
487456
$this->assertEqualSets( $suppressed_plugins, array_keys( AMP_Options_Manager::get_option( Option::SUPPRESSED_PLUGINS ) ) );
488457
}
489458

0 commit comments

Comments
 (0)