Update splash screen to offer the view-only dashboard to admins

[aaemnnosttv]

Feature Description

With dashboard sharing, users who are able to authenticate (admins) will see a different splash screen than users who cannot. This issue is about updating the splash screen for admins to be able to go straight to the shared dashboard if they are able to (if any module is shared with admins).

---

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

Acceptance criteria

• The splash screen shown to users who are able to authenticate (SetupUsingProxyWithSignIn) should be updated to offer a new "Go to dashboard" button as a secondary action to the primary sign in with Google button
  • The "Go to dashboard" button should trigger the same dismissed item dismissal as for non-admins in Implement <ViewOnlySplash /> #4810 before navigating to the dashboard
  • The button should only be shown if the user has the VIEW_SHARED_DASHBOARD capability
  • See the original Figma design

Implementation Brief

• Using assets/js/components/setup/SetupUsingProxyWithSignIn.js,
  • Query the user data store via the hasCapability selector with googlesitekit_view_shared_dashboard as parameter to see if the user has permission to view the shared dashboard.
  • Check if dashboardSharing is enabled via the useFeature hook, passing dashboardSharing as parameter.
  • After inProgressFeedback, add a new Button with the translated Go to dashboard text. The Button should be rendered only if the user has permission to view the shared dashboard and the dashboardSharing feature flag is enabled.
  • Add the onClick prop to Button and the callback function should dismiss the shared_dashboard_splash item and navigate to the dashboard.
    • To dismiss the shared_dashboard_splash item, dispatch the dismissItem action of the core/user data store.
    • To get the dashboard URL, query the core/site data store via the getAdminURL selector, with googlesitekit-dashboard as parameter.
• Update assets/js/components/setup/SetupUsingProxyWithSignIn.stories.js to include a story where the Go to dashboard is rendered.

Test Coverage

• No new tests to be added.

QA Brief

• QAing this issue would require accessing the site as a secondary admin who can view the shared dashboard, then going to "setup" Site Kit as the secondary admin. I'm not sure this is available yet, but if so: setup the site as a secondary admin with the feature flag for dashboardSharing enabled, then ensure you see these buttons at the bottom of the setup page:

• For now, check Storybook for the "Start - with Dashboard Sharing enabled and available" story and ensure the button layout/content matches the figma doc/image above in the QA Brief.

QA Brief Update

As per this comment, the "Go to dashboard" button styling should match the styling for the "Reset Site Kit" button.

Changelog entry

• Allow users with shared dashboard access to navigate directly to the shared dashboard from the splash page.

[eugene-manuilov]

IB ✔️

[wpdarren]

QA Update: ⚠️

@tofumatt When accessing the site as a secondary admin, I do not see what is appearing on the figma design on my test site. This is what I see on the develop branch. Followed information in QAB.

Also when I look at Storybook, what I see is also different than the figma design.

Thoughts? 🤔

[tofumatt]

The view in storybook is accurate as I understand it from the ACs because they only mention adding the button. So the storybook view is correct, but the in-plugin version isn't, so I'll take a look.

[techanvil]

Approval

@tofumatt @techanvil This wasn't clear from the ACs, but seeing the secondary button, the font and size there looks off. Please check the stories above https://google.github.io/site-kit-wp/storybook/main/?path=/story/setup-using-proxy-with-sign-in--shared-dashboard-admin-can-view, the appearance should match the one of the "Reset Site Kit" button. You should be able to achieve that by adding the googlesitekit-cta-link--inherit class to the secondary link.

Can you please open a follow-up PR based on main?

Thanks @felixarntz. @tofumatt was feeling a bit unwell earlier so I've picked this up in case he's not able to get to it any time soon. Have created a followup PR: #5181.

[aaemnnosttv]

The view in storybook is accurate as I understand it from the ACs because they only mention adding the button. So the storybook view is correct, but the in-plugin version isn't, so I'll take a look.

@tofumatt @felixarntz looks like there was a flaw in the ACs which prevents the button from being displayed when relying on the VIEW_SHARED_DASHBOARD capability. This is because the capability requires that the shared_dashboard_splash item is dismissed which is currently set by clicking "View dashboard" from the splash page. This makes it impossible to reach because the button to "grant" the capability essentially requires the same capability to show it 😄

I think this can be solved with a one-line change when checking the shared dashboard capability.

site-kit-wp/includes/Core/Permissions/Permissions.php

Lines 474 to 483 in 7be61fc

	private function check_view_shared_dashboard_capability( $user_id ) {
	if ( ! $this->user_has_shared_role( $user_id ) ) {
	return array( 'do_not_allow' );
	}
	if ( ! $this->is_shared_dashboard_splash_dismissed( $user_id ) ) {
	return array( 'do_not_allow' );
	}
	return array();

If the user has a shared role but hasn't dismissed the splash item, we can simply require AUTHENTICATE rather than do_not_allow. This will grant the permission for admins regardless of the dismissed item, but only if a module is shared with administrators.

I'll open a new issue for addressing this as it should get updated test coverage as well which would likely be too time consuming to fit in here.

[felixarntz]

@aaemnnosttv Good catch. I wonder then, should we even consider the dismissal of the splash screen inside of the capability? I'm thinking now that it may be more appropriate to check explicitly for the dismissal in the few places where it's needed rather than baking it into the capability.

Other than the complexity that baking the check into the capability introduces based on the feedback, I think it even makes sense just from a capability standpoint. There is nothing sensitive about seeing the shared dashboard even without dismissing the splash screen, so maybe this shouldn't have been part of the capability check in the first place. It's just that from a product perspective, we want people to go through it. But it's not critical from a security or functionality perspective, it's just good for more clear UX. Hence I think now this shouldn't be baked into a capability check.

We could for example also include logic to redirect to the splash screen when someone who is not authenticated hasn't dismissed the splash screen.

We could basically say on the server side when adding the screen:

• If the user has VIEW_SHARED_DASHBOARD, check if they have dismissed splash.
• If yes, add the (shared) dashboard screen.
• If not, add the splash screen.

What do you think?

[aaemnnosttv]

I wonder then, should we even consider the dismissal of the splash screen inside of the capability? I'm thinking now that it may be more appropriate to check explicitly for the dismissal in the few places where it's needed rather than baking it into the capability.

@felixarntz I think the reason it was baked in was that we would need to check that in all the same places where the capability is used.

There is nothing sensitive about seeing the shared dashboard even without dismissing the splash screen, so maybe this shouldn't have been part of the capability check in the first place. It's just that from a product perspective, we want people to go through it. But it's not critical from a security or functionality perspective, it's just good for more clear UX. Hence I think now this shouldn't be baked into a capability check.

That makes sense, I think it may just make things a bit more complicated to "unpack" this to check the dismissal explicitly.

We could for example also include logic to redirect to the splash screen when someone who is not authenticated hasn't dismissed the splash screen.

This should already be the case now, isn't it? The dashboard requires VIEW_DASHBOARD and if the dismissal hadn't been set, an unauthenticated user would not be able to view either dashboard so that would fail, so the redirect should happen as part of Screens::no_access_redirect_dashboard_to_splash. Did you mean specifically unathenticated users who can authenticate?

We could basically say on the server side when adding the screen:

• If the user has VIEW_SHARED_DASHBOARD, check if they have dismissed splash.
• If yes, add the (shared) dashboard screen.
• If not, add the splash screen.

This would be incomplete I think because an authenticated admin who can VIEW_AUTHENTICATED_DASHBOARD would also be able to VIEW_SHARED_DASHBOARD by this definition (has a module shared with the administrator role) would not be able to get to the dashboard unless they went through the secondary link to the shared dashboard to get the dismissal.

TL;DR I think the proper solution may be to decouple the two as you're suggesting, however I think it would be simpler to get things working as an intermediate step to make the smaller change suggested above and then the decoupling can be more of a pure refactoring.

[aaemnnosttv]

Maybe a new VIEW_SHARED_POSTS_INSIGHTS capability would make more sense as the requirement for showing the CTA here? This would equate to any user who has a module shared with their role. Then I think the VIEW_SHARED_DASHBOARD logic could stay the same – if a user has that capability it means they can view it (without being redirected to the splash).

@felixarntz are you happy for this all to happen in a separate issue or is there anything else you think is necessary to finalize this one?

[felixarntz]

@aaemnnosttv I think we can definitely address this in a separate issue. It seems like we need to think about the approach a bit more anyway.

Looking back at your original proposal:

If the user has a shared role but hasn't dismissed the splash item, we can simply require AUTHENTICATE rather than do_not_allow. This will grant the permission for admins regardless of the dismissed item, but only if a module is shared with administrators.

I think this would cause problems with users that can authenticate though. If you are an admin that can read shared module data, you can also decide to not authenticate, like a non-admin, and instead go to the shared dashboard. In that case we still need to show them the splash screen and the admin would have to dismiss it the same like a non-admin.

I'm still leaning towards breaking out the dismissal from capabilities. Primarily because of what I mentioned before that it's not security-sensitive in any way, and also because we would only need it in a capability because we have the splash screen as a separate screen technically and in WP we typically use capabilities to determine which screen to show. In a perfect world, maybe we would only have the dashboard, and the splash screen could almost be like a modal on top of the dashboard, all handled in JS. So from that perspective, I think decoupling dismissal from the capabilities makes sense.

Regarding the effort, I think we could make this change by exclusively updating server-side code, and I would assume the amount of places where we check whether the user can access the shared dashboard are fairly limited. Worst case, even if we somewhere accidentally linked to the shared dashboard, the user would get redirected to the splash screen if they hadn't dismissed the splash screen yet.

[aaemnnosttv]

I think we can definitely address this in a separate issue. It seems like we need to think about the approach a bit more anyway.

Sounds good, I'll open a new issue to refine this 👍

Regarding the effort, I think we could make this change by exclusively updating server-side code, and I would assume the amount of places where we check whether the user can access the shared dashboard are fairly limited. Worst case, even if we somewhere accidentally linked to the shared dashboard, the user would get redirected to the splash screen if they hadn't dismissed the splash screen yet.

Aside from this issue, I don't think we have any checks for the shared dashboard outside of Permissions yet, so I'm less worried about specific references. The more complicated part is unpacking it from references to VIEW_DASHBOARD for which we have many more. Maybe that isn't such a problem either but because it's tied into that capability we have to look at that one as well.