Combine dashboard and splash screens into a single screen in PHP

[aaemnnosttv]

Feature Description

As noted in #4811 (comment), the current logic behind our VIEW_SHARED_DASHBOARD capability makes it difficult to implement a conditional button on the splash screen due to the specific requirement of the shared_dashboard_splash dismissal as part of the criteria for granting the capability.

We should decouple the logic related to the splash screen dismissal from the capability for viewing the shared dashboard. This may mean a user may be redirected back to the splash from the dashboard if they don't have the dismissal yet, but that's okay.

---

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

Acceptance criteria

• The VIEW_SHARED_DASHBOARD capability should no longer require the shared_dashboard_splash dismissed item in its resolution
  • Going forward, this capability will be only the following criteria:
    • The user has a role which is included in the sharing settings for at least one module in the dashboard sharing settings (as today)
    • The user cannot view the authenticated dashboard
• The relevant behavior around the requirement of the dismissal to view the shared dashboard should be preserved as follows:
  • If a user can VIEW_SHARED_DASHBOARD but does not have the dismissal, they should be redirected to the splash page when attempting to access the dashboard
  • If a user can VIEW_SHARED_DASHBOARD but does have the dismissal, they should be redirected to the dashboard when attempting to access the splash page
  • If a user can VIEW_SHARED_DASHBOARD but does not have the dismissal, the splash page Screen should appear in the admin menu instead of the dashboard, the same as if an admin was not authenticated without dashboard sharing today
• VIEW_SHARED_DASHBOARD should continue to only be granted when the dashboardSharing feature is enabled (i.e. explicit checks for the feature flag are not needed when logic requires the capability)

Implementation Brief

• In Permissions.php, within the check_view_shared_dashboard_capability() method:

  • Remove the call to is_shared_dashboard_splash_dismissed().
  • Add a check if the given $user_id has the VIEW_AUTHENTICATED_DASHBOARD capability - use WP_User::has_capability(). If it returns true, fail the check, i.e. return array( 'do_not_allow' ).

• In Screens.php, within the get_screens() method:

  • When registering the googlesitekit-dashboard Screen, add an initialize_callback function. It should:
    • Check if the current user can VIEW_SHARED_DASHBOARD.
    • Also check if the shared_dashboard_splash item is dismissed using the Dismissed_Items class and its is_dismissed method.
    • If the user can VIEW_SHARED_DASHBOARD and does not have the dismissal, redirect the user to the splash page.
  • When registering the googlesitekit-splash Screen, update the initialize_callback function:
    • After the splash context revoked logic, check if the current user can VIEW_SHARED_DASHBOARD and if they have the shared_dashboard_splash dismissal as above. If both are true, redirect them to the dashboard (similar to how it's done when the user has been authenticated).
    • Modify the current_user_can() check to VIEW_AUTHENTICATED_DASHBOARD instead of VIEW_DASHBOARD. Essentially, only redirect the user to the dashboard if the user can view the authenticated dashboard and if the user is authenticated.
  • The updates do not require the dashboardSharing feature flag to be enabled since the capabilities checked above are granted only if the feature flag is enabled.

Test Coverage

• Add new tests in ScreensTest.php:
  • add tests to check if the admin menu is constructed appropriately depending on a user's capabilities.
  • add tests to check if the user is redirected correctly between the dashboard and splash screen depending on their capabilities.

QA Brief

• While the ACs in this issue focus on users who can view the shared dashboard (i.e. non-authenticated users whose role is shared within dashboard sharing settings), there are significant changes which also affect authenticated users. Changes have been made to both these types of users landing on the "Splash screen" or the "Dashboard" pages, and being redirected to the other depending on the conditions in the ACs. As such, their combinations should be tested, ensuring that there are no "infinite redirect" errors in the browser. Tests should include these scenarios:
• With dashboardSharing disabled, verify existing behaviour is not affected. Administrators can verify, authenticate, and setup the plugin. They can then also disconnect or reset the plugin as before.
  • Non-admins should not be able to see links to the plugin in the Admin menu.
• With dashboardSharing enabled, verify that an administrator can authenticate themselves and setup the plugin, as well as disconnect or reset. Also verify that without adding any shared roles, an editor/non-admin cannot see the SK Dashboard link in the admin menu.
• Using the new Dashboard Sharing Settings plugin, allow some roles, especially "administrators" and say, "editors" to be shared roles for some modules like search-console and pagespeed-insights. The management setting is not very relevant here and can be left to their default values. Create users with these shared roles, including another administrator. Since they have roles which are shared, these users are able to VIEW_SHARED_DASHBOARD and can be used to verify the ACs. To navigate directly to the dashboard, append the following to your test domain: /wp-admin/admin.php?page=googlesitekit-dashboard. To navigate directly to the splash, append: /wp-admin/admin.php?page=googlesitekit-splash
• Make sure to use the second administrator to click on "Skip Sign in and View Limited Dashboard" link on the splash page and verify the user is directed to the dashboard.

Changelog entry

[aaemnnosttv]

@felixarntz I've added initial ACs here, let me know what you think. As I mentioned before, one thing to consider regarding these changes are places where we check VIEW_DASHBOARD which would be true going forward for users who don't have the dismissal.

Most notably, users who could see the shared dashboard would get the SK WP dashboard widget + admin bar integration without going through the splash. If we want to require that action first, then we'd need to check for the dismissal in those places as well. Does that sound right? There may even be other places like the post list screen (if Idea Hub was shared).

[felixarntz]

@aaemnnosttv ACs SGTM!

I think getting access to the WP dashboard widget and admin bar without going through the splash screen is not too big of a deal. I'll move this to IB.

That raises another important point I just thought about though: We may want to be a bit more strict about WP dashboard widget and admin bar menu in another way. I don't think the dismissal matters here much, but for example it would not make any sense to show these two areas in case a user e.g. had only access to shared data from AdSense or PageSpeed Insights since these don't surface there at all.

That would definitely not be part of this issue, but maybe we should open an issue to introduce dedicated capabilities for viewing the WP dashboard widget and admin bar menu? Since those rely on "hard-coded" module data, the capabilities should probably check under the hood for shared access only to the modules that are relevant there, in order to make a decision whether to grant the capability. Does that make sense? In that case, could you open an issue for that?

[aaemnnosttv]

SGTM @felixarntz – I'll open an issue for those new capabilities 👍

Edit: see #5202

[tofumatt]

Looks good to me, IB ✅

[felixarntz]

@jimmymadon After a long review & exploration session with @aaemnnosttv, we are thinking that the cleanest solution is combining the googlesitekit-dashboard and googlesitekit-splash screens into a single screen on the PHP side only. We can do that in a way that the JS side is barely affected, by simply rendering the same target div and enqueuing the same JS entry point file from the server-side. This is then relatively similar for how it already works for the main dashboard (googlesitekit-dashboard) and entity dashboard (googlesitekit-dashboard-details), both of which already come only from the single googlesitekit-dashboard screen in PHP.

If we don't do this, we think it will be unavoidable to not have a capability that includes the splash dismissal in its logic, which is what we wanted to avoid here. The key problem we can't easily circumvent is that we cannot affect the WordPress core-invoked permission checks for the screens, so we wouldn't be able to e.g. ensure the dismissal is considered for the googlesitekit-dashboard screen unless it's part of a capability.

We've created two alternative PRs #5293 and #5294 that both pursue this approach, only in slightly different ways. I think we should move forward with one of them. While the changes look quite significant, most of them are actually renamings from googlesitekit-splash to googlesitekit-dashboard in many areas of the plugin. Both PRs furthermore include a redirect in case an old link from somewhere points the user to the old splash URL. We've already tested this approach and it appears to work reliably so far.

This is of course a relatively significant change, which can even affect production sites in principle, so we would need to do thorough testing and expand the QA Brief accordingly. On the positive side, we now have almost 4 weeks until the next Site Kit release, which should give us enough time.

Please take a look at the PRs and share your thoughts on this. In any case, we should try to finalize this issue by Monday.

[aaemnnosttv]

@felixarntz after thinking about this some more, I think there is an opportunity to fix things as they are with much less effort than the approach to merge the splash and dashboard screens. To be clear, I still think this is worth doing, but I feel it is too substantial a change to introduce at this point in the cycle for DS.

I opened a new issue #5299 as an alternative to #5188 (for now) which solves the same problem in a simpler way. This preserves the dismissal in the permissions logic but this should be something we can refactor out later and is more of an enhancement than critical functionality at this point. I've included a PR with the IB which is only a few dozen lines of diff (most of it are some blocks which moved).

I think we should go with #5299 for the sake of time right now and revisit #5188 and screen merging and as soon as we have time to do so.

[FlicHollis]

Thanks @aaemnnosttv based on your comment above do we want to change the priority of this to a P1?
cc @eclarke1

[felixarntz]

@aaemnnosttv Going with #5299 for now works for me (though I left one question on the already merged #5300).

I don't feel that this is a too massive change though, most changes in the PR are changed URLs. The main caveat is that this will require a lot of testing, which from my perspective would maybe actually justify doing it before dashboardSharing launch, giving us the additional time to test, rather than changing it when dashboardSharing is already live and we only have the sprint to test. On the other hand, the change affects non-dashboardSharing code as well, so it would already potentially affect end users today. But with the 2 sprints timeline we have right now, I'd say it may still be worth doing this now.

To be clear, #5299 is good from my end, but I think it would still be useful to also complete this issue within this sprint (e.g. by early next week) because we have the extra time to test. Otherwise, okay to make this a P1, but then we should still tackle it some time shortly after dashboardSharing launch. Feel free to make that call.

[FlicHollis]

Hi @felixarntz and @aaemnnosttv I have updated this ticket to be a P1 as per Evan's update on Slack.
Noted on your point above Felix and if we will get this in as the next priorities after the P0s

[felixarntz]

I'll also move this back to ACs for now, since the scope of this issue has changed. Will provide new ACs soon.

[FlicHollis]

Following on from a conversation on our stand up, we are pausing this ticket until after the launch of Dashboard Sharing. At which point this nice to have improvement can be made.
We have removed the epic to keep the bugs from the bug bash next week clear, we can add this back later.
We have also removed this from the current sprint.
cc @jimmymadon @felixarntz @eclarke1 @aaemnnosttv

[mxbclang]

@felixarntz @aaemnnosttv FYI, adding this to the next Open Bucket agenda now that Dashboard Sharing is done.

[aaemnnosttv]

Closing in favor of #6650