Any change to sharing settings triggers sharing notices

[eclarke1]

Bug Description

Bug bash issue: https://app.asana.com/0/1202258919887896/1202436389160088 please refer to Asana issue for background

The notice in the bottom of the sharing settings modal is currently being triggered by what seems to be any change to sharing settings. Since the notice is about informing the user that data will be shared through their account, it should probably only be shown under more limited conditions:

• Sharing with additional user roles
  • Subtracting roles should not prompt a notice
• Changing sharing management to "All admins" – changing to "Only me" restricts permissions so we don't need to "warn" the user about it as it doesn't affect data sharing
• Any change to roles for shared ownership modules which would result in the ownership changing – if the current user is already the owner, it should only show if additional roles are being shared with, as above

---

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

Acceptance criteria

• The notice in the footer of the dashboard sharing settings appears only in the following conditions:
  • When a module is shared with additional roles;
  • When a module sharing management is changed to "All Admins";
  • When a non-owner admin changes module sharing settings for a shared ownership module
• The notice shouldn't appear when the sharing settings are restricted in any way (a module is shared with less roles or sharing management is changed back to owner only).

Implementation Brief

• In assets/js/googlesitekit/modules/datastore/sharing-settings.js, create a new selector haveSharingSettingsExpanded with the following changes:
  • Receives key as a required param.
  • Returns a boolean value, return false by default.
  • Get sharingSettings and savedSharingSettings from the state.
  • Return undefined if either one is not loaded yet.
  • To compare the management changes, have a condition to check if key is equal to management.
  • Return true if the management setting for any module has been changed from owner to all_admins.
  • Similarly, to compare the additional sharedRoles changes, have a condition to check if key is equal to sharedRoles.
  • Return true if sharing settings for any module contain roles that haven't been previously selected.
• In assets/js/components/dashboard-sharing/DashboardSharingSettings/Notice.js:
• Using the haveSharingSettingsExpanded selector, create two calls; one is by passing management as an argument, and the other is sharedRoles.
• Updated the notice conditions with the above instead of isEditingManagement and editingUserRolesSlug.
• Remove all the unused code.
• In assets/js/components/dashboard-sharing/DashboardSharingSettings/constants.js, remove EDITING_MANAGEMENT_KEY.
• Remove all the usages for EDITING_MANAGEMENT_KEY in the DS related components.

Test Coverage

• In assets/js/components/dashboard-sharing/DashboardSharingSettings/index.test.js, add test cases to cover the above scenarios.
• Write unit tests for the new selector haveSharingSettingsExpanded

QA Brief

• Ensure the Sharing Settings Notice in the footer SHOULD appear after updating a module's management from owner to all_admins.
• Ensure the Sharing Settings Notice in the footer SHOULDN'T appear after updating a module's management from all_admins to owner.
• Ensure the Sharing Settings Notice in the footer SHOULDN'T appear after removing some of the module's existing user roles.
• Ensure the Sharing Settings Notice in the footer SHOULDN'T appear after adding back the removed existing role to the module's user roles.
• Ensure the Sharing Settings Notice in the footer SHOULD appear after adding a new non-existing role to the module's user roles.
• Ensure the Sharing Settings Notice in the footer SHOULD appear when adding a role when the module's user roles are empty.

Note: The IB states to add test cases to cover the scenarios; however, the DashboardSharingSettings component isn't responsible for rendering the Footer component. Rather, DashboardSharingSettingsButton renders the Footer. However, we don't have tests for that component.

Changelog entry

• Fix conditions for showing the notice in the bottom of the Dashboard Sharing modal when sharing settings are changed.

[aaemnnosttv]

@hussain-t I just tweaked one point of the AC specifically around "a shared ownership module".

[eugene-manuilov]

@hussain-t, thanks for the initial approach, but I think it would be better if we implement this functionality as a new selector in the sharing settings datastore. We already have original and changed versions of sharing settings in the store state, so we can compare them and find if changes grand access permissions to more users or not. We can call the new selector something like haveSharingSettingsExpanded. Could you please update the IB, please?

[hussain-t]

Thanks, @eugene-manuilov. I have updated the IB.

[eugene-manuilov]

@hussain-t

• create a new selector haveSharingSettingsChangedComparison with the following changes:

Let's call it haveSharingSettingsExpanded because "expanded" meaning is more appropriate than "changed comparison" for this selector.

Similarly, to compare the additional sharedRoles changes, have a condition to check if key is equal to sharedRoles with the following logic:

• Get the keys of sharingSettings and loop through it.
• Return the compared value of sharingSettings[ module ].sharedRoles.length is greater than savedSharingSettings[ module ].sharedRoles.length.

I think this is not sufficient to check just length of shared roles arrays because I can deselect previously selected role and select a new one to grant view permissions to new users. So we can rephrase it to say something like Return true if sharing settings for any module contain roles that hasn't been previously selected.

To compare the management changes, have a condition to check if key is equal to management with the following logic:

• Get the keys of sharingSettings and loop through it.
• Create a variable to check savedSharingSettings[ module ].management is not equal to sharingSettings[ module ].management.
• Return the above compared value AND sharingSettings[ module ].management value is equal to all_admins.

Same here, we can just say Return true if the management setting for any module has been changed from owned to all_admins instead of explaining which fields we need to compare.

• In assets/js/components/dashboard-sharing/DashboardSharingSettings/constants.js, remove EDITING_MANAGEMENT_KEY.
• Remove all the usages for EDITING_MANAGEMENT_KEY in the DS related components.

Don't we need to remove EDITING_USER_ROLE_SELECT_SLUG_KEY as well?

[hussain-t]

Thanks, @eugene-manuilov 👍 I have updated the IB.

Don't we need to remove EDITING_USER_ROLE_SELECT_SLUG_KEY as well?

No, it's been used in other places for conditionally applying class, etc.

[eugene-manuilov]

Thanks, @hussain-t. IB ✔️

[wpdarren]

QA Update: ✅

Verified:

• The Sharing Settings Notice in the footer does appear after updating a management from owner to all_admins.
• The Sharing Settings Notice in the footer does not appear after updating a management from all_admins to owner.
• The Sharing Settings Notice in the footer does not appear after removing some of the module's existing user roles.
• The Sharing Settings Notice in the footer does not appear after adding back the removed role to the module's user roles.
• The Sharing Settings Notice in the footer does appear after adding a new non-existing role to the module's user roles.
• The Sharing Settings Notice in the footer does appear when adding a role when the module's user roles are empty.

Screenshots

[aaemnnosttv]

I just raised #5487 which is somewhat related to this – I thought that problem was caused by this issue, but now that this one is fixed it appears the other still remains.