From da37ceb3f2bdb2b1d9ccb8d1ad142094ea510afb Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Fri, 25 Aug 2023 22:46:32 +0600 Subject: [PATCH 1/6] Make widgetSlugs optional. --- .../REST_Key_Metrics_Controller.php | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php b/includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php index f81bce2e1c9..f4e1c967b85 100644 --- a/includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php +++ b/includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php @@ -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() ); }, @@ -161,7 +163,7 @@ protected function get_rest_routes() { ), 'widgetSlugs' => array( 'type' => 'array', - 'required' => true, + 'required' => false, 'maxItems' => 4, 'items' => array( 'type' => 'string', From 5da551f6699492f88fcc90dfaea8b618d4bd2a43 Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Fri, 25 Aug 2023 22:47:51 +0600 Subject: [PATCH 2/6] Only set `keyMetricsSetupCompleted` to `true` if there is widgetSlugs. --- assets/js/googlesitekit/datastore/user/key-metrics.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/js/googlesitekit/datastore/user/key-metrics.js b/assets/js/googlesitekit/datastore/user/key-metrics.js index b6317db239e..3bde7aec7a5 100644 --- a/assets/js/googlesitekit/datastore/user/key-metrics.js +++ b/assets/js/googlesitekit/datastore/user/key-metrics.js @@ -133,7 +133,7 @@ const baseActions = { if ( error ) { // Store error manually since saveKeyMetrics signature differs from fetchSaveKeyMetricsStore. yield receiveError( error, 'saveKeyMetricsSettings', [] ); - } else { + } else if ( 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. From 4d9a4b9f4a7f0182b497d8d6b72359f96cae0e2c Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Fri, 25 Aug 2023 22:48:45 +0600 Subject: [PATCH 3/6] Update `handleKeyMetricsToggle` to not pass `widgetSlugs` in request body. --- assets/js/components/settings/SettingsKeyMetrics.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/assets/js/components/settings/SettingsKeyMetrics.js b/assets/js/components/settings/SettingsKeyMetrics.js index 5aade3bfdc2..6d5cd2f7407 100644 --- a/assets/js/components/settings/SettingsKeyMetrics.js +++ b/assets/js/components/settings/SettingsKeyMetrics.js @@ -49,7 +49,9 @@ export default function SettingsKeyMetrics() { 'isWidgetHidden', ! keyMetricsWidgetHidden ); - await saveKeyMetricsSettings(); + await saveKeyMetricsSettings( { + widgetSlugs: undefined, + } ); }, [ keyMetricsWidgetHidden, saveKeyMetricsSettings, From 7fcc11cd5ca74fd2bf94fe1ff1354f6e39ad3c6d Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Sat, 26 Aug 2023 00:45:28 +0600 Subject: [PATCH 4/6] Update the action for non empty argument. --- assets/js/googlesitekit/datastore/user/key-metrics.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/assets/js/googlesitekit/datastore/user/key-metrics.js b/assets/js/googlesitekit/datastore/user/key-metrics.js index 3bde7aec7a5..f5ae0cda933 100644 --- a/assets/js/googlesitekit/datastore/user/key-metrics.js +++ b/assets/js/googlesitekit/datastore/user/key-metrics.js @@ -18,7 +18,7 @@ * External dependencies */ import invariant from 'invariant'; -import { isPlainObject } from 'lodash'; +import { isEmpty, isPlainObject } from 'lodash'; /** * Internal dependencies @@ -133,7 +133,7 @@ const baseActions = { if ( error ) { // Store error manually since saveKeyMetrics signature differs from fetchSaveKeyMetricsStore. yield receiveError( error, 'saveKeyMetricsSettings', [] ); - } else if ( settings?.widgetSlugs ) { + } 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. From 7c421a438c7741bcbe1745fa4da6a9b980fe45bc Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Sun, 27 Aug 2023 08:55:25 +0600 Subject: [PATCH 5/6] Add JS test coverage. --- .../datastore/user/key-metrics.test.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/assets/js/googlesitekit/datastore/user/key-metrics.test.js b/assets/js/googlesitekit/datastore/user/key-metrics.test.js index dc46ae5457d..2af7d4c819a 100644 --- a/assets/js/googlesitekit/datastore/user/key-metrics.test.js +++ b/assets/js/googlesitekit/datastore/user/key-metrics.test.js @@ -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(); + } ); } ); } ); From 836fe8dac293fb4efe7d4ba1fe5277365f49cfdf Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Sun, 27 Aug 2023 09:48:06 +0600 Subject: [PATCH 6/6] Add PHP test coverage. --- .../REST_Key_Metrics_ControllerTest.php | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/tests/phpunit/integration/Core/Key_Metrics/REST_Key_Metrics_ControllerTest.php b/tests/phpunit/integration/Core/Key_Metrics/REST_Key_Metrics_ControllerTest.php index 2c6e3f8dff6..d8d9c78414f 100644 --- a/tests/phpunit/integration/Core/Key_Metrics/REST_Key_Metrics_ControllerTest.php +++ b/tests/phpunit/integration/Core/Key_Metrics/REST_Key_Metrics_ControllerTest.php @@ -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 @@ -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, + ), + ), ); }