View only settings. #7633

zutigrm · 2023-09-28T10:30:51Z

Summary

Addresses issue:

Allow specific module settings to be available in a view-only context #7114

Relevant technical choices

Implemented per IB. With addition of VIEW_SPLASH permission in readable settings route, as it is needed for proper access for the roles.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2023-09-28T10:39:36Z

Build files for 0b87b4c are ready:

Download development build
Download production build

aaemnnosttv

Thanks @zutigrm – this is almost there, a few minor things to address, mostly to simplify the implementation a bit.

includes/Core/Modules/REST_Modules_Controller.php

aaemnnosttv · 2023-09-28T13:28:17Z

includes/Core/Modules/REST_Modules_Controller.php

+							if ( $settings instanceof Setting_With_ViewOnly_Keys_Interface && ! $can_manage_options() ) {
+								return new WP_REST_Response( $settings->get_view_only_settings() );
+							}
+
+							if ( $can_manage_options() ) {
+								return new WP_REST_Response( $module->get_settings()->get() );
+							}


Let's check can_manage_options first, that way we only need to check it once. If the user can't, we'd then only check if it supports view-only keys.

If it doesn't support view-only keys, we should probably return an error rather than an empty array. Let's go with a default WP_Error with a code of no_view_only_settings. The HTTP status is 500 by default which is good for this case too.

includes/Core/Storage/Setting_With_ViewOnly_Keys_Trait.php

tests/phpunit/includes/Core/Modules/FakeModule_WithViewOnlySettings.php

aaemnnosttv · 2023-09-28T13:51:18Z

tests/phpunit/integration/Core/Modules/REST_Modules_ControllerTest.php

+	private function share_fake_module_with_user_role( $shared_with_role ) {
+		$user = self::factory()->user->create_and_get( array( 'role' => $shared_with_role ) );
+		wp_set_current_user( $user->ID );


This method does quite a bit more than one might expect by the name which also makes it less reusable in other tests. Please keep helper methods to be focused in their purpose and limited in their side-effects. This method should only be concerned with adding a module to the shared roles (it could be more useful if it wasn't specific to fake-module). Instantiating a few classes where needed is okay, but it's better to use those that are shared throughout as you are.

tests/phpunit/integration/Core/Modules/REST_Modules_ControllerTest.php

tests/phpunit/integration/Modules/Analytics_4/Settings_ViewOnly_TraitTest.php

aaemnnosttv

Thanks @zutigrm ! This is a nice improvement over the first iteration and almost there, just a few small points left to address!

Edit: these are quite minor so I'll push these now to unblock this important issue.

aaemnnosttv · 2023-10-02T16:46:15Z

includes/Core/Modules/REST_Modules_Controller.php

+								return new WP_REST_Response( $view_only_settings );
+							}
+
+							return new WP_Error( 'no_view_only_settings', __( 'No view-only settings available.', 'google-site-kit' ) );


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.

aaemnnosttv · 2023-10-02T16:48:56Z

includes/Core/Modules/REST_Modules_Controller.php

+							}
+
+							$settings = $module->get_settings();
+							if ( $settings instanceof Setting_With_ViewOnly_Keys_Interface ) {


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.

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

aaemnnosttv

Left a few more thoughts about the changes I'm making which are just about simplifying what we have. Thanks @zutigrm !

aaemnnosttv · 2023-10-02T17:05:24Z

includes/Core/Modules/REST_Modules_Controller.php

+							$settings = $module->get_settings();
+							if ( $settings instanceof Setting_With_ViewOnly_Keys_Interface ) {
+								$view_only_keys = $settings->get_view_only_keys();
+								$settings       = $settings->get();


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

aaemnnosttv · 2023-10-02T17:07:42Z

includes/Core/Modules/REST_Modules_Controller.php

+								$view_only_keys = $settings->get_view_only_keys();
+								$settings       = $settings->get();
+
+								if ( empty( $view_only_keys ) || empty( $settings ) ) {


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.

aaemnnosttv

This looks good and works well in my testing. I gave it a quick try using GA4 just to make sure it works as intended (as if measurementID was a view only setting) and it does!

zutigrm added 3 commits September 28, 2023 12:00

Add interface and trait for view-only settings.

3b74495

Update rest module controller to include view-only settings.

e92b5cb

Add php tests.

68e7e74

zutigrm changed the title ~~Enhancement/7114 view only settings~~ View only settings. Sep 28, 2023

aaemnnosttv requested changes Sep 28, 2023

View reviewed changes

zutigrm added 2 commits September 28, 2023 18:43

Update per CR feedback.

2bcd3f0

Fix indentation.

3d9f195

aaemnnosttv reviewed Oct 2, 2023

View reviewed changes

Simplify REST handler for view-only settings.

0b87b4c

aaemnnosttv reviewed Oct 2, 2023

View reviewed changes

aaemnnosttv approved these changes Oct 2, 2023

View reviewed changes

aaemnnosttv merged commit 92fedb0 into develop Oct 2, 2023
14 of 17 checks passed

aaemnnosttv deleted the enhancement/7114-view-only-settings branch October 2, 2023 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View only settings. #7633

View only settings. #7633

zutigrm commented Sep 28, 2023

github-actions bot commented Sep 28, 2023 •

edited

Loading

aaemnnosttv left a comment

aaemnnosttv Sep 28, 2023

aaemnnosttv Sep 28, 2023

aaemnnosttv left a comment •

edited

Loading

aaemnnosttv Oct 2, 2023

aaemnnosttv Oct 2, 2023

aaemnnosttv Oct 2, 2023

aaemnnosttv left a comment

aaemnnosttv Oct 2, 2023

aaemnnosttv Oct 2, 2023

aaemnnosttv left a comment

View only settings. #7633

View only settings. #7633

Conversation

zutigrm commented Sep 28, 2023

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Sep 28, 2023 • edited Loading

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv Sep 28, 2023

Choose a reason for hiding this comment

aaemnnosttv Sep 28, 2023

Choose a reason for hiding this comment

aaemnnosttv left a comment • edited Loading

Choose a reason for hiding this comment

aaemnnosttv Oct 2, 2023

Choose a reason for hiding this comment

aaemnnosttv Oct 2, 2023

Choose a reason for hiding this comment

aaemnnosttv Oct 2, 2023

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv Oct 2, 2023

Choose a reason for hiding this comment

aaemnnosttv Oct 2, 2023

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2023 •

edited

Loading

aaemnnosttv left a comment •

edited

Loading