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.

Sign up for GitHub

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

Jump to bottom

Fix Key Metrics toggle in SK Settings when answer based matrices are used #7503

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Fix Key Metrics toggle in SK Settings when answer based matrices are used #7503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da37ceb
Make widgetSlugs optional.
kuasha420 Aug 25, 2023
5da551f
Only set `keyMetricsSetupCompleted` to `true` if there is widgetSlugs.
kuasha420 Aug 25, 2023
4d9a4b9
Update `handleKeyMetricsToggle` to not pass `widgetSlugs` in request …
kuasha420 Aug 25, 2023
7fcc11c
Update the action for non empty argument.
kuasha420 Aug 25, 2023
7c421a4
Add JS test coverage.
kuasha420 Aug 27, 2023
836fe8d
Add PHP test coverage.
kuasha420 Aug 27, 2023
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
4 changes: 3 additions & 1 deletion assets/js/components/settings/SettingsKeyMetrics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ export default function SettingsKeyMetrics() {
'isWidgetHidden',
! keyMetricsWidgetHidden
);
await saveKeyMetricsSettings();
await saveKeyMetricsSettings( {
widgetSlugs: undefined,
} );
}, [
keyMetricsWidgetHidden,
saveKeyMetricsSettings,
Expand Down
4 changes: 2 additions & 2 deletions assets/js/googlesitekit/datastore/user/key-metrics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* External dependencies
*/
import invariant from 'invariant';
import { isPlainObject } from 'lodash';
import { isEmpty, isPlainObject } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -133,7 +133,7 @@ const baseActions = {
if ( error ) {
// Store error manually since saveKeyMetrics signature differs from fetchSaveKeyMetricsStore.
yield receiveError( error, 'saveKeyMetricsSettings', [] );
} else {
} else if ( isEmpty( settings ) || settings.widgetSlugs ) {
// Update the `keyMetricsSetupCompleted` value to keep it in sync, as it will have been set
// to `true` on the backend when the key metrics settings were successfully saved.
// TODO: We should find a better way of keeping this value synced.
Expand Down
23 changes: 23 additions & 0 deletions assets/js/googlesitekit/datastore/user/key-metrics.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,29 @@ describe( 'core/user key metrics', () => {

expect( console ).toHaveErrored();
} );

it( 'should not set the keyMetricsSetupCompleted site info setting to true if only `isWidgetHidden` is changed', async () => {
fetchMock.postOnce( coreKeyMetricsEndpointRegExp, {
body: coreKeyMetricsExpectedResponse,
status: 200,
} );

// Verify the setting is initially false.
expect(
registry.select( CORE_SITE ).isKeyMetricsSetupCompleted()
).toBe( false );

await registry
.dispatch( CORE_USER )
.saveKeyMetricsSettings( { isWidgetHidden: true } );

// Verify the setting is still false.
expect(
registry.select( CORE_SITE ).isKeyMetricsSetupCompleted()
).toBe( false );

expect( console ).not.toHaveErrored();
} );
} );
} );

Expand Down
42 changes: 22 additions & 20 deletions includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,28 +120,30 @@ protected function get_rest_routes() {
$data = $request->get_param( 'data' );
$settings = $data['settings'];

$num_widgets = count( $settings['widgetSlugs'] );
if ( ! $num_widgets ) {
return new WP_Error(
'rest_invalid_param',
__( 'Selected metrics cannot be empty.', 'google-site-kit' ),
array( 'status' => 400 )
);
}
// Additional check is needed to ensure that we have no more than 4 widget
// slugs provided. This is required until we drop support for WP versions below 5.5.0, after
// which we can solely rely on `maxItems` in the schema validation (see below).
// See https://github.com/WordPress/WordPress/blob/965fcddcf68cf4fd122ae24b992e242dfea1d773/wp-includes/rest-api.php#L1922-L1925.
if ( $num_widgets > 4 ) {
return new WP_Error(
'rest_invalid_param',
__( 'No more than 4 key metrics can be selected.', 'google-site-kit' ),
array( 'status' => 400 )
);
if ( isset( $settings['widgetSlugs'] ) ) {
$num_widgets = count( $settings['widgetSlugs'] );
if ( ! $num_widgets ) {
return new WP_Error(
'rest_invalid_param',
__( 'Selected metrics cannot be empty.', 'google-site-kit' ),
array( 'status' => 400 )
);
}
// Additional check is needed to ensure that we have no more than 4 widget
// slugs provided. This is required until we drop support for WP versions below 5.5.0, after
// which we can solely rely on `maxItems` in the schema validation (see below).
// See https://github.com/WordPress/WordPress/blob/965fcddcf68cf4fd122ae24b992e242dfea1d773/wp-includes/rest-api.php#L1922-L1925.
if ( $num_widgets > 4 ) {
return new WP_Error(
'rest_invalid_param',
__( 'No more than 4 key metrics can be selected.', 'google-site-kit' ),
array( 'status' => 400 )
);
}
$this->key_metrics_setup_completed->set( true );
}

$this->settings->merge( $data['settings'] );
$this->key_metrics_setup_completed->set( true );

return new WP_REST_Response( $this->settings->get() );
},
Expand All @@ -161,7 +163,7 @@ protected function get_rest_routes() {
),
'widgetSlugs' => array(
'type' => 'array',
'required' => true,
'required' => false,
'maxItems' => 4,
'items' => array(
'type' => 'string',
Expand Down
45 changes: 43 additions & 2 deletions tests/phpunit/integration/Core/Key_Metrics/REST_Key_Metrics_ControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,41 @@ public function test_set_settings() {
$this->assertEqualSetsWithIndex( $changed_settings, $response->get_data() );
}

public function test_set_settings__only__isWidgetHidden() {
remove_all_filters( 'googlesitekit_rest_routes' );
$this->controller->register();
$this->register_rest_routes();

$original_settings = array(
'widgetSlugs' => array( 'widgetA' ),
'isWidgetHidden' => false,
);

$changed_settings = array(
'isWidgetHidden' => true,
);

$expected_settings = array(
'widgetSlugs' => array( 'widgetA' ),
'isWidgetHidden' => true,
);

$this->settings->register();
$this->settings->set( $original_settings );

$request = new WP_REST_Request( 'POST', '/' . REST_Routes::REST_ROOT . '/core/user/data/key-metrics' );
$request->set_body_params(
array(
'data' => array(
'settings' => $changed_settings,
),
)
);

$response = rest_get_server()->dispatch( $request );
$this->assertEqualSetsWithIndex( $expected_settings, $response->get_data() );
}

/**
* @dataProvider data_setup_completed
* @param bool $expected
Expand Down Expand Up @@ -154,20 +189,26 @@ public function test_setup_completed( $expected, $settings ) {

public function data_setup_completed() {
return array(
'completed on success' => array(
'completed on success' => array(
true,
array(
'widgetSlugs' => array( 'widgetA' ),
'isWidgetHidden' => false,
),
),
'incomplete on error' => array(
'incomplete on error' => array(
false,
array(
'widgetSlugs' => array(), // Insufficient number of widget slugs.
'isWidgetHidden' => false,
),
),
'incomplete on only isWidgetHidden change' => array(
false,
array(
'isWidgetHidden' => false,
),
),
);
}

Expand Down
Loading