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.

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

Dashboard sharing confirmation appears in settings when no roles are shared for module #6633

Closed
aaemnnosttv opened this issue Feb 23, 2023 · 4 comments
Labels
Exp: SP P1 Medium priority Type: Bug Something isn't working

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Feb 23, 2023

Bug Description

When a module is shared via dashboard sharing, if another admin with access to that module's entity attempts to change its configured entity, a notice is shown in the footer of the settings edit view to inform the user that by saving they will be sharing the data for this module through their Google account since they will become the new module owner.

This is only relevant for a module which is currently being shared with one or more roles, and of course if the current user is not already the module owner. Currently this notice is being shown even when no shared roles are configured for a module.

Steps to reproduce

  1. Ensure dashboard sharing is enabled, reset any existing sharing settings so that nothing is shared
  2. Connect Analytics
  3. With a different admin, go to the GA settings, and change the selected UA or GA4 property
  4. See the notice

Screenshots

image


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

Acceptance criteria

  • The notice about granting access to data from {service} via your Google account should only be shown to users who are changing the connected entity of a module when that module is currently being shared with one or more roles

Implementation Brief

  • Within the /assets/js/components/settings/EntityOwnershipChangeNotice.js:
    • In addition to getModuleStoreNames, destructure the getSharedRoles selector as well.
    • When computing storeNames, call the getSharedRoles selector passing the currentSlug.
    • Only add the storeName to the acc if there are sharedRoles.
  • The getSharedRoles selector above relies on global._googlesitekitDashboardSharingData which is not available on the Settings pages. So in Assets.php, in the get_assets() method, when registering googlesitekit-settings, pass the dashboard-sharing context instead of the dashboard context to the get_asset_dependencies() method.

Test Coverage

  • Fix any failing tests.

QA Brief

  • Ensure dashboard sharing is enabled, reset any existing sharing settings so that nothing is shared
  • Connect Analytics
  • With a different admin, sign in to Site Kit as an Admin using an account that ALSO has access to this GA account, and go to the GA settings, and change the selected UA or GA4 property
  • No "By clicking confirm…" notice should appear.

Changelog entry

  • Fix notices about granting view-only access when changing module settings for modules that are not shared with any roles.
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working P1 Medium priority labels Feb 23, 2023
@sashadoes sashadoes self-assigned this Mar 6, 2023
@sashadoes sashadoes removed their assignment Apr 4, 2023
@techanvil techanvil self-assigned this Apr 5, 2023
@techanvil
Copy link
Collaborator

Hi @sashadoes, thanks for drafting this IB, however the approach taken is not quite correct.

The concept of a "shared ownership" module is separate to whether the module has been shared with other roles. Shared ownership modules are covered in the Design Doc, and specifically only PageSpeed Insights is a shared ownership module now that Idea Hub has been removed.

Instead, in order to check if a module has been shared with any roles, the getSharedRoles() selector should be called with the module slug as its argument. If the selector returns a non-empty array then the module has been shared.

There's an additional detail that needs to be considered for the implementation, which is that Dashboard Sharing data is not currently available on the Settings page. This is because the googlesitekit-dashboard-sharing-data asset is not presently a dependency of the googlesitekit-settings asset which is the main bundle for the Settings Screen.

So in order for getSharedRoles() to work on the Settings page, googlesitekit-dashboard-sharing-data needs to be defined as a dependency of googlesitekit-settings. This is probably best managed by updating get_asset_dependencies() to allow multiple contexts to be passed in, and then passing dashboard-sharing as well as dashboard in the call to it that sets up the googlesitekit-settings dependencies.

@aaemnnosttv
Copy link
Collaborator Author

@bethanylang let's try to include this in 1.102, it shouldn't be a big effort and we shouldn't delay it anymore.

@mxbclang mxbclang removed the P1 High label May 18, 2023
@jimmymadon jimmymadon removed their assignment Jun 6, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 6, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Jun 6, 2023

IB ✅

@tofumatt tofumatt self-assigned this Jun 7, 2023
@tofumatt tofumatt removed their assignment Jun 14, 2023
@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Jun 22, 2023
@aaemnnosttv aaemnnosttv removed their assignment Jun 23, 2023
@mohitwp mohitwp self-assigned this Jun 25, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 26, 2023

QA Update ✅

  • Tested on dev environment.
  • Verified The notice about granting access to data from {service} via your Google account showing to users who are changing the connected entity of a module when that module is currently being shared with one or more roles.
  • Tested on both UA and GA4 dashboard.
  • Verified when only GA4 is connected.
  • Verified when both GA4 and UA are connected.
Recording.415.mp4

@mohitwp mohitwp removed their assignment Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P1 Medium priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants