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

View only settings. #7633

Merged
merged 6 commits into from
Oct 2, 2023
Merged

View only settings. #7633

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b74495
Add interface and trait for view-only settings.
zutigrm Sep 28, 2023
e92b5cb
Update rest module controller to include view-only settings.
zutigrm Sep 28, 2023
68e7e74
Add php tests.
zutigrm Sep 28, 2023
2bcd3f0
Update per CR feedback.
zutigrm Sep 28, 2023
3d9f195
Fix indentation.
zutigrm Sep 28, 2023
0b87b4c
Simplify REST handler for view-only settings.
aaemnnosttv Oct 2, 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
26 changes: 23 additions & 3 deletions includes/Core/Modules/REST_Modules_Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Google\Site_Kit\Core\REST_API\REST_Routes;
use Google\Site_Kit\Core\REST_API\REST_Route;
use Google\Site_Kit\Core\REST_API\Exception\Invalid_Datapoint_Exception;
use Google\Site_Kit\Core\Storage\Setting_With_ViewOnly_Keys_Interface;
use Google\Site_Kit\Modules\Analytics;
use Google\Site_Kit\Modules\Analytics_4;
use WP_REST_Server;
Expand Down Expand Up @@ -424,7 +425,7 @@ private function get_rest_routes() {
array(
array(
'methods' => WP_REST_Server::READABLE,
'callback' => function( WP_REST_Request $request ) {
'callback' => function( WP_REST_Request $request ) use ( $can_manage_options ) {
$slug = $request['slug'];
try {
$module = $this->modules->get_module( $slug );
Expand All @@ -435,9 +436,28 @@ private function get_rest_routes() {
if ( ! $module instanceof Module_With_Settings ) {
return new WP_Error( 'invalid_module_slug', __( 'Module does not support settings.', 'google-site-kit' ), array( 'status' => 400 ) );
}
return new WP_REST_Response( $module->get_settings()->get() );

if ( $can_manage_options() ) {
return new WP_REST_Response( $module->get_settings()->get() );
}

$settings = $module->get_settings();
if ( $settings instanceof Setting_With_ViewOnly_Keys_Interface ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're doing a bit more within this block than before, let's invert the condition to return the error here instead and then this can all be un-indented below since we'd know that settings implemented the interface. Alternatively, the body of this condition could be extracted to a helper method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On closer inspection, this isn't necessary since the block here can be further simplified a fair bit. See below

$view_only_keys = $settings->get_view_only_keys();
$settings = $settings->get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we should avoid redeclaring $settings as a different type.


if ( empty( $view_only_keys ) || empty( $settings ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This condition is unnecessary because array_intersect_key below will return an empty array in either case anyways, and array_flip is also safe to use with an empty array 👍 $settings will always be an array and never be empty since the full value will always be an array with one or more key-value pairs.

return array();
}

$view_only_settings = array_intersect_key( $settings, array_flip( $view_only_keys ) );

return new WP_REST_Response( $view_only_settings );
}

return new WP_Error( 'no_view_only_settings', __( 'No view-only settings available.', 'google-site-kit' ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

The translatable message shouldn't be necessary here, this is mostly an internal error that we don't expect to encounter, except maybe in tests. Adding this adds a new string to be translated for the plugin, but it shouldn't be possible to trigger, but also, this message probably wouldn't be very useful to the user to see either.

},
'permission_callback' => $can_manage_options,
'permission_callback' => $can_list_data,
),
array(
'methods' => WP_REST_Server::EDITABLE,
Expand Down
31 changes: 31 additions & 0 deletions includes/Core/Storage/Setting_With_ViewOnly_Keys_Interface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php
/**
* Interface Google\Site_Kit\Core\Storage\Setting_With_ViewOnly_Keys_Interface
*
* @package Google\Site_Kit\Core\Storage
* @copyright 2023 Google LLC
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0
* @link https://sitekit.withgoogle.com
*/

namespace Google\Site_Kit\Core\Storage;

/**
* Interface for a settings class that includes view-only settings.
*
* @since n.e.x.t
* @access private
* @ignore
*/
interface Setting_With_ViewOnly_Keys_Interface {

/**
* Returns keys for view-only settings.
*
* @since n.e.x.t
*
* @return array An array of keys for view-only settings.
*/
public function get_view_only_keys();

}
33 changes: 33 additions & 0 deletions tests/phpunit/includes/Core/Modules/FakeModuleSettings_WithViewOnlyKeys.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php
/**
* Class Google\Site_Kit\Tests\Core\Modules\FakeModuleSettings_WithViewOnlyKeys
*
* @package Google\Site_Kit\Tests\Core\Modules
* @copyright 2023 Google LLC
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0
* @link https://sitekit.withgoogle.com
*/

namespace Google\Site_Kit\Tests\Core\Modules;

use Google\Site_Kit\Core\Modules\Module_Settings;
use Google\Site_Kit\Core\Storage\Setting_With_ViewOnly_Keys_Interface;
use Google\Site_Kit\Core\Storage\Setting_With_ViewOnly_Keys_Trait;

class FakeModuleSettings_WithViewOnlyKeys extends Module_Settings implements Setting_With_ViewOnly_Keys_Interface {

const OPTION = 'fake_module_settings_with_view_only';

public function get_view_only_keys() {
return array(
'viewOnlyKey',
);
}

protected function get_default() {
return array(
'defaultKey' => 'default-value',
'viewOnlyKey' => 'default-value',
);
}
}
24 changes: 24 additions & 0 deletions tests/phpunit/includes/Core/Modules/FakeModule_WithViewOnlySettings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
/**
* FakeModule
*
* @package Google\Site_Kit
* @copyright 2023 Google LLC
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0
* @link https://sitekit.withgoogle.com
*/

namespace Google\Site_Kit\Tests\Core\Modules;

class FakeModule_WithViewOnlySettings extends FakeModule {

/**
* Sets up the module's settings instance which implements view-only keys.
*
* @return FakeModuleSettings_WithViewOnlyKeys
*/
protected function setup_settings() {
return new FakeModuleSettings_WithViewOnlyKeys( $this->options );
}

}
135 changes: 133 additions & 2 deletions tests/phpunit/integration/Core/Modules/REST_Modules_ControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@
namespace Google\Site_Kit\Tests\Core\Modules;

use Google\Site_Kit\Context;
use Google\Site_Kit\Core\Authentication\Authentication;
use Google\Site_Kit\Core\Dismissals\Dismissed_Items;
use Google\Site_Kit\Core\Modules\Module_Sharing_Settings;
use Google\Site_Kit\Core\Modules\Modules;
use Google\Site_Kit\Core\Modules\REST_Modules_Controller;
use Google\Site_Kit\Core\Permissions\Permissions;
use Google\Site_Kit\Core\REST_API\REST_Routes;
use Google\Site_Kit\Core\Storage\Options;
use Google\Site_Kit\Core\Storage\User_Options;
Expand Down Expand Up @@ -106,6 +110,44 @@ private function setup_fake_module( $force_active = true ) {
$this->set_available_modules( array( $fake_module ) );
}

private function setup_fake_module_with_view_only_settings( $force_active = true ) {
$fake_module = new FakeModule_WithViewOnlySettings( $this->context );
$fake_module->set_force_active( $force_active );

$fake_module_settings = new FakeModuleSettings_WithViewOnlyKeys( $this->options );
$fake_module_settings->register();

$this->set_available_modules( array( $fake_module ) );
}

private function share_modules_with_user_role( $shared_with_role, $shared_modules = array( 'fake-module' ) ) {
$sharing_settings = new Module_Sharing_Settings( new Options( $this->context ) );

$shared_modules = array_combine(
$shared_modules,
array_fill(
0,
count( $shared_modules ),
array( 'sharedRoles' => array( $shared_with_role ) )
)
);

$sharing_settings->set( $shared_modules );
}

private function set_current_active_user_role( $role ) {
$user = self::factory()->user->create_and_get( array( 'role' => $role ) );

wp_set_current_user( $user->ID );
}

private function request_get_module_setings( $module = 'fake-module' ) {
$request = new WP_REST_Request( 'GET', '/' . REST_Routes::REST_ROOT . '/modules/' . $module . '/data/settings' );
$response = rest_get_server()->dispatch( $request );

return $response;
}

public function test_register() {
remove_all_filters( 'googlesitekit_rest_routes' );
remove_all_filters( 'googlesitekit_apifetch_preload_paths' );
Expand Down Expand Up @@ -548,12 +590,101 @@ public function test_settings_rest_endpoint__post_invalid_slug() {
$this->controller->register();
$this->register_rest_routes();

$request = new WP_REST_Request( 'POST', '/' . REST_Routes::REST_ROOT . '/modules/non-existent-module/data/settings' );
$response = rest_get_server()->dispatch( $request );
$response = $this->request_get_module_setings( 'non-existent-module' );

$this->assertEquals( 404, $response->get_status() );
}

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

$response = $this->request_get_module_setings();

$this->assertEqualSetsWithIndex(
array(
'defaultKey' => 'default-value',
),
$response->get_data()
);
}

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

$shared_with_roles = array( 'editor', 'author', 'contributor' );
foreach ( $shared_with_roles as $shared_with_role ) {
$this->set_current_active_user_role( $shared_with_role );
$this->share_modules_with_user_role( $shared_with_role );

$response = $this->request_get_module_setings();

$this->assertEquals( '500', $response->get_status() );
$this->assertEquals( 'no_view_only_settings', $response->get_data()['code'] );
}
}

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

$response = $this->request_get_module_setings();

$this->assertEqualSetsWithIndex(
array(
'defaultKey' => 'default-value',
'viewOnlyKey' => 'default-value',
),
$response->get_data()
);
}

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

$roles = array( 'editor', 'author', 'contributor' );
foreach ( $roles as $role ) {
$this->set_current_active_user_role( $role );

$response = $this->request_get_module_setings();

$this->assertEquals( '403', $response->get_status() );
}
}

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

$shared_with_roles = array( 'editor', 'author', 'contributor' );
foreach ( $shared_with_roles as $shared_with_role ) {
$this->set_current_active_user_role( $shared_with_role );
$this->share_modules_with_user_role( $shared_with_role );

$response = $this->request_get_module_setings();

$this->assertEquals( '200', $response->get_status() );
$this->assertEqualSetsWithIndex(
array(
'viewOnlyKey' => 'default-value',
),
$response->get_data()
);
}
}

public function test_data_available_rest_endpoint__valid_method__non_implementing_module() {
remove_all_filters( 'googlesitekit_rest_routes' );
$this->controller->register();
Expand Down
Loading