Add an option to "Reset sharing permissions"

[eclarke1]

Feature Description

Bug bash issue: https://app.asana.com/0/1202258919887896/1202443541768582 please see Asana for background

Consider adding an option to "Reset sharing permissions" within the "Sharing and permissions" screen. This would allow users to easily revert to the default configurations, should they feel there were modifications made.

Suggestion:

---

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

Acceptance criteria

• A new button in the Dashboard Sharing UI should be added, that should appear to the left of the other options (see mockup image above) that reads "Reset sharing permissions". On mobile, this can be shown as Reset<VisuallyHidden> sharing permissions</VisuallyHidden>.
• When this button is clicked, the existing Dashboard Sharing modal should be replaced with a screen asking the user to confirm their decision and clarify what the reset will do, eg:
  • A heading with "Reset Dashboard Sharing permissions"
  • Body text reading "Warning: resetting these permissions will remove view-only access for all users. Are you sure you want to reset all Dashboard Sharing permissions?"
  • A "Reset" button that performs the reset action
    • The reset action means Dashboard Sharing permissions should all be reset; both which roles can view the Shared Dashboard and who can manage view-only access, for every module.
  • A "Cancel" button that returns to the main Dashboard Sharing modal UI.

Implementation Brief

POC PR

A proof-of-concept PR has been added with most of the implementation mentioned below here. This can be used as a starting point, ensuring these next steps:

• There are no console errors.
• All relevant strings are translatable.
• Stying of the action should match the AC (e.g. left aligned).
• Proper error handling should be done for the API request (i.e. the value of errorNotice state of the <ResetSharingSettings /> component should be displayed appropriately.
• Add necessary tests and stories.
• Double-check all the file headers, DocBlocks, and comments.

Summary of the implementation is still mentioned below:

• In includes/Core/Modules/Modules.php:
  • We will create a new method that runs delete_option( 'googlesitekit_dashboard_sharing' ).
  • Create a new public method named delete_dashboard_sharing_settings.
  • Inside this method, delete the option named googlesitekit_dashboard_sharing using the delete method of the Google\Site_Kit\Core\Storage\Options class (already assigned to $this->options of the current Modules class).
• In includes/Core/Modules/REST_Dashboard_Sharing_Controller.php:
  • We will extend the rest route defined here so that when the request is made with a DELETE method, it calls the delete_dashboard_sharing_settings method that we created above.
  • Locate the REST_ROUTE definition core/modules/data/sharing-settings.
  • Create a new array for this definition so that we can define different callbacks for two different methods.
  • Insert the existing WP_REST_Server::EDITABLE method and callback to the new array.
  • Duplicate this array to create a new method/callback array.
  • Update the methods property to WP_REST_Server::DELETABLE.
  • Inside the callback function, call the new delete_dashboard_sharing_settings method of the Google\Site_Kit\Core\Modules class (already assigned to $this->modules of the current REST_Dashboard_Sharing_Controller class).
  • From the callback function, return a WP_REST_Response with true.
  • Remove the args property from this array.
• In assets/js/googlesitekit/modules/datastore/sharing-settings.js:
  • We will create a new fetch store for reset and add a new action that mimics the save action but does a reset instead.
  • Duplicate the fetchSaveSharingSettingsStore fetch store creation call and rename it to fetchResetSharingSettingsStore. In the duplicate fetch store creation call:
    • Change the baseName to resetSharingSettings.
    • In the controlCallback function, remove the savedSharingSettings parameter, in the returned API.set call, set the fourth parameter to an empty object to pass no data with the request, and add an object as the fifth parameter with a method key and DELETE value.
    • In the reducerCallback function, set the savedSharingSettings and sharingSettings properties to empty objects.
    • Remove the argsToParams and validateParams properties.
  • Duplicate the *saveSharingSettings action and rename it to *resetSharingSettings. In the duplicated action:
    • Remove the getRegistry() and getSharingSettings() calls.
    • Instead of calling fetchSaveSharingSettings, call fetchResetSharingSettings of fetchResetSharingSettingsStore.actions.
    • Remove the entire part of setting module owner IDs.
  • Add fetchResetSharingSettingsStore to the Data.combineStores function in the store definition.
• Within the assets/js/components/dashboard-sharing folder:
  • We want the sharing settings dialog and the reset settings dialog to open within the same <Dialog /> component, which is why we'll create a new component DashboardSharingDialog which will contain both the sharing settings dialog and the reset settings dialog, moved away from DashboardSharingSettingsButton.js.
  • Create a new folder named DashboardSharingDialog. Inside this folder:
    • Create three files - index.js which will contain the <Dialog /> component, SharingSettings.js which will contain the existing part of the dialog, and ResetSharingSettings.js which will contain the reset settings part of the dialog.
  • Locate the <Portal /> component in DashboardSharingSettingsButton.js and move it to DashboardSharingDialog/index.js. Update imports accordingly.
  • In DashboardSharingSettings/constants.js, create two new constants SETTINGS_DIALOG with the value of dashboardSharingDialogOpen and RESET_SETTINGS_DIALOG with the value of resetSharingDialogOpen.
    • Update all the usages of UI_KEY_DIALOG_OPEN across the codebase with SETTINGS_DIALOG imported from this file.
  • In DashboardSharingDialog/index.js:
    • Create a variable named settingsDialogOpen which runs the getValue selector in the core/ui store for the SETTINGS_DIALOG constant.
    • Create a variable named resetDialogOpen which runs the getValue selector in the core/ui store for the RESET_SETTINGS_DIALOG constant.
    • Locate the <Dialog /> component in DashboardSharingDialog/index.js and:
      • Change the open prop to be truthy for either settingsDialogOpen or resetDialogOpen.
      • Move the existing content inside it to a new component named SharingSettings exported from DashboardSharingDialog/SharingSettings.js.
  • In DashboardSharingDialog/ResetSharingSettings.js:
    • We will create a part of the dialog which displays the reset settings content and actions.
• In assets/js/components/dashboard-sharing/DashboardSharingSettings/Footer.js:
  • We will add an action link that would open the reset sharing settings dialog.
  • Add a new prop function named openResetDialog.
  • Create two separate containers inside .googlesitekit-dashboard-sharing-settings__footer-actions, one for actions in the left and the other for actions in the right.
  • The existing actions should be left in the right container.
  • Create a new action link in the left container that says "Reset sharing permissions" and on click, calls the openResetDialog prop function.
• In assets/js/components/dashboard-sharing/DashboardSharingSettingsButton.js:
  • We will display the <DashboardSharingDialog /> component replacing the <Portal />.

Test Coverage

• Add a PHP unit test for delete_dashboard_sharing_settings of the Module class in tests/phpunit/integration/Core/Modules/ModuleTest.php.
• Add test cases for resetSharingSettings in assets/js/googlesitekit/modules/datastore/sharing-settings.test.js.
• Add story for the <ResetSharingSettings /> component.

QA Brief

Changelog entry

[tofumatt]

The IB here looks good, but instead of calling the method inside Modules.php, we can probably just name it delete_dashboard_sharing_settings. We aren't really "resetting" anything—it's just that removing the settings causes the plugin to load the default settings, which functions like a reset. In the UI that makes sense, but in our code/function names, we should be explicit and call out that we're actually deleting/removing the saved values. Especially when we're wiring it up to the REST API with a DELETE HTTP method.

That way there isn't any confusion in the future if someone expects the reset to "restore" the values to some sort of default ones but finds them removed instead.

I suppose in the Redux code we might want to do that as well, though I'm more okay with it being called reset because it will reflect what the UI shows. I think coupling things on the frontend is fine, but the REST API feels like it should be marked as "delete".

It's subtle, but unless we already do the "reset means delete" thing elsewhere, let's not start now 🙂

Other than that this looks good, let me know what you think. Hopefully I'm not being too pedantic 😉

[nfmohit]

The IB here looks good, but instead of calling the method inside Modules.php, we can probably just name it delete_dashboard_sharing_settings. We aren't really "resetting" anything—it's just that removing the settings causes the plugin to load the default settings, which functions like a reset. In the UI that makes sense, but in our code/function names, we should be explicit and call out that we're actually deleting/removing the saved values. Especially when we're wiring it up to the REST API with a DELETE HTTP method.

That way there isn't any confusion in the future if someone expects the reset to "restore" the values to some sort of default ones but finds them removed instead.

I suppose in the Redux code we might want to do that as well, though I'm more okay with it being called reset because it will reflect what the UI shows. I think coupling things on the frontend is fine, but the REST API feels like it should be marked as "delete".

It's subtle, but unless we already do the "reset means delete" thing elsewhere, let's not start now 🙂

Other than that this looks good, let me know what you think. Hopefully I'm not being too pedantic 😉

Thank you so much for the detailed review @tofumatt! You are absolutely correct and it was clearly an oversight from my end. I have updated both the IB and the PoC PR as suggested so that the method says delete_dashboard_sharing_settings in the backend/REST API.

Thank you!

[aaemnnosttv]

This is the first I've seen of this issue. A few concerns here:

• We need to make sure it's very clear that this will essentially remove access to the view only dashboard for everyone; I'm don't think the current language is quite clear enough about this (we should probably run any proposed wording by support folks as well for a second opinion).
• Modal on a modal – this is something that we have done well to avoid so far and shouldn't be necessary here (it will probably be problematic with competing z-indexes as well). Can we use the existing sharing settings modal body to display the reset confirmation instead?

[tofumatt]

@aaemnnosttv Good call on both, I bet we can re-purpose the existing modal to show a confirmation screen, and add more context to the action.

I'll update the ACs and then send this back over to @nfmohit to add that part to the IB 🙂

[tofumatt]

Hi @bethanylang—just assigned you to get your thoughts on the language used in this prompt from a support perspective for resetting the dashboard sharing permissions.

The proposed language in the ACs is:

• A heading with "Reset Dashboard Sharing permissions"
• Body text reading "Warning: resetting these permissions will remove view-only access for all users. Are you sure you want to reset all Dashboard Sharing permissions?"
• A "Reset" button that performs the reset action
• A "Cancel" button that returns to the main Dashboard Sharing modal UI

Is that cool or do you think we should change it at all? 🙂

(If it's fine feel free to move this back to IB and assign in to @nfmohit. 👍🏻)

[mxbclang]

@tofumatt Thank you for checking in! The only tiny little change I have is I would make the "r" in "resetting" capitalized, so:

"Warning: Resetting these permissions will remove view-only access for all users. Are you sure you want to reset all Dashboard Sharing permissions?"

Reassigning to @nfmohit and moving back to IB!

[nfmohit]

@tofumatt I've updated the IB and the PoC PR to reflect on the suggestions. In order to not over-crowd assets/js/components/dashboard-sharing/DashboardSharingSettingsButton.js, I decided to create a new component DashboardSharingDialog which would contain SharingSettings and ResetSharingSettings. It involves a fair bit of restructuring but I think it makes things more organised and readable. Also, I have not included every minute detail in the IB as most of it is covered in the PoC PR. Let me know what you think.

CC: @aaemnnosttv

[tofumatt]

@nfmohit For the future, please don't include involved PRs like this as part of the IB. Very basic (one or a few line) PRs that make sense to proof-of-concept first can be used as (part of) an IB, and larger proof-of-concept draft PRs are fine, but using large ones as part of the IB can conflate IB review and code review—it just means sometimes the IB review can get too technical, or it can be hard to separate approach from actual code.

For instance, lines like "This is already laid out in the PoC PR." confuse the IB a bit and the review—is it totally implemented? Or partially? I'd need to verify this reviewing the IB, so in this case I've just removed these lines as they:

• Clutter the IB
• Are difficult to verify without code review

Also, this is a pretty-big proof-of-concept PR. It's probably better in the future to more vaguely outline the idea in the IB as I don't think it was needed to draft this PR yet. The approach looks right, but the IB is much closer to code instructions than an outline of the approach, which is what the IB should be (an outline).

In general this approach looks right, but the IB is less a brief and reads more like instructions on exactly what to do. In this particular case, because the ACs are clear and there's a proof-of-concept PR, I don't think it makes sense to drastically change this IB, but please note these points for the future 🙂

IB ✅ , but assigning to @nfmohit since he already did the proof-of-concept so is familiar with the approach and what's still needed.

[nfmohit]

That completely makes sense and thank you so very much for the kind explanation of what's wrong with the IB here. I'll definitely keep them in mind for future IBs that I get to work on. Thank you, @tofumatt!

[nfmohit]

@tofumatt I have a question regarding the placement of the Reset sharing settings action in the dialog. If we place it according to the mockup image in the issue description, it works.

But that spot is currently responsible for displaying errors or notices, and when they appear, the action is then wrapped and pushed to the right. See:

It works quite well on mobile devices though:

Do we need to involve the UX team here?

Thank you!

[tofumatt]

I don't think we need UX involvement here… I almost wonder about not showing the "Reset sharing permissions" button if that notice is there as it's a lot of actions and could be a bit confusing after a permissions change. The wording is a bit vague in this context and might imply to a user that clicking the "reset" button will reset their changes. But really it resets all settings…

That said, I don't love disappearing action buttons like that.

My understanding is having the "reset" button appear to the left of the notice only on desktop might be a bit of a pain because of how this is built. If that's the case, I think we can leave it as-is and that's okay.

If it's possible to move the "Reset sharing permissions" to the left on the notice but ONLY on non-mobile viewports: let's do that.

[nfmohit]

Thank you for the insights, @tofumatt!

If it's possible to move the "Reset sharing permissions" to the left on the notice but ONLY on non-mobile viewports: let's do that.

I'm open to doing that, but to do so, I believe we'll need to create two instances of the notice element for desktop and mobile viewports respectively and show/hide them depending on viewports. Are we okay to do that, or do you possibly have a better idea?

Thank you!

[tofumatt]

Let's skip that for now then and just go with how it currently works. We can change it in a follow-up issue/PR, but I think it's fine and I prefer the simpler approach for now, at least to start with 😄

[mohitwp]

QA Update ❌

@nfmohit

• In AC it is mentioned that we need to implement a button for 'Reset sharing settings' but we have implemented action link.
• Text font size under 'Reset Sharing Settings dialog' is very small and difficult to read in desktop and in mobile.
• Position of 'Reset sharing settings' action in the dialog along with notices not look good and it also gets cut in iPad portrait mode.
• (Suggestion) If sharing settings already in default mode that time also user can click on 'Reset sharing settings' and it will perform it action. Although, it will not have any effect. Can we disable or hide 'Reset sharing settings' action when sharing settings are set to default ?

[tofumatt]

In AC it is mentioned that we need to implement a button for 'Reset sharing settings' but we have implemented action link.

This is confusing, but that's a button that looks like a link. All secondary buttons are styled as links, but this part is correct.

Text font size under 'Reset Sharing Settings dialog' is very small and difficult to read in desktop and in mobile.

Agreed, let's increase this font sizing. I don't think we need to involve UX here, just a larger font-size would be good here. Going from 12px to 14px seems fine.

Position of 'Reset sharing settings' action in the dialog along with notices not look good and it also gets cut in iPad portrait mode.

Odd, it's not cut off for me on an iPad Air-sized viewport:

(Suggestion) If sharing settings already in default mode that time also user can click on 'Reset sharing settings' and it will perform it action. Although, it will not have any effect. Can we disable or hide 'Reset sharing settings' action when sharing settings are set to default ?

Hmm, that's a good call. I've filed this as a follow-up issue: #5840.

[nfmohit]

Thank you for chiming in with the confirmations, @tofumatt!

Position of 'Reset sharing settings' action in the dialog along with notices not look good and it also gets cut in iPad portrait mode.

@mohitwp I couldn't replicate it getting cut off in all iPad viewports on both portrait and landscape orientations as well. I also tried the viewport mentioned in your screenshot but it doesn't get cut off. Here's a screenshot:

Text font size under 'Reset Sharing Settings dialog' is very small and difficult to read in desktop and in mobile.

I have opened a new PR #5842 to address this, thank you!

[mohitwp]

@tofumatt @nfmohit

Thank you !

-Regarding iPad issue. I verified this using BrowserStack and action title is getting cut in BrowserStack. Verified it using browser -iPad viewports, and it's looking fine there. It can be due to some glitch in BrowserStack so need to be verified in actual device. But action design when it appears with Notice can be improved. If require, we can take help from UX.

[nfmohit]

@mohitwp We've decided to remove the Reset sharing permissions action when there is a notice/error visible. This change is added to my new PR.

[mohitwp]

QA Update ✅

Verified -

• Issues mentioned here are now resolved.
• Text font size under 'Reset Sharing Settings dialog' now increases to 14px from 12px.
• We've removed the Reset sharing permissions action when there is a notice/error visible so design issue in i-Pad will not come now.
• For - If sharing settings already in default mode that time also user can click on 'Reset sharing settings' and it will perform it action. Although, it will not have any effect. Can we disable or hide 'Reset sharing settings' action when sharing settings are set to default ? - We have separate PR https://github.com/google/site-kit-wp/pull/5842 to address this.
• Verified updated design in mobile and desktop.
• Verified 'Reset sharing permissions' functionality. It is working as expected.