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

Redirect authenticated users from splash to dashboard #2529

Closed
felixarntz opened this issue Dec 14, 2020 · 11 comments
Closed

Redirect authenticated users from splash to dashboard #2529

felixarntz opened this issue Dec 14, 2020 · 11 comments
Labels
P2 Low priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Dec 14, 2020

Bug Description

The googlesitekit-splash screen currently has an incorrect <title> tag where the main segment is missing. At the moment it looks like the following:

 < example.com — WordPress

However, there's currently no way for a user to reach the splash screen after they're already authenticated – even the activation notice shown after activating the plugin will link to the dashboard. This makes this situation only possible if the user navigated directly to the splash screen (from browser history/back, etc). The splash screen is only a stepping stone to send the user through Google OAuth; if they're already authenticated, there's no reason to go through it again so they should be sent straight to the dashboard.

If required scopes have changed, the user will still need to go through oAuth of course, but we already prompt the user to "Redo Setup" in this case.


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

Acceptance criteria

  • The googlesitekit-splash screen should redirect to googlesitekit-dashboard if the user is already authenticated (Authentication::is_authenticated)
  • The connect and disconnect URLs should be updated to no longer use the base splash screen URL, but instead use their own admin action URLs, like we do for Google Proxy setup/settings URLs

Implementation Brief

  • Update screen definition of googlesitekit-splash to redirect to the dashboard if authenticated (preserving existing logic to ensure permissions first)
  • Move connect/disconnect handlers from googlesitekit-splash URL to dedicated admin_action_ URLs:
    • index.php?action=googlesitekit_connect&nonce=...
    • index.php?action=googlesitekit_disconnect&nonce=...
  • Finish and merge Redirect splash to dashboard if authenticated #3037
    • Update references to these URLs in JS/fixtures
    • Updating tests

Test Coverage

  • Add/update tests for decoupled action handlers
  • Update JS tests as necessary

Visual Regression Changes

  • None.

QA Brief

This issue primarily updates the splash screen to redirect the user to the SK dashboard if they're already authenticated and have sufficient permissions.

The following cases should be tested to work (and continue to work) properly

Unauthenticated / Site Kit not set up yet

  • Splash screen loads without redirecting
  • Navigating directly to SK dashboard redirects to splash screen

Authenticated / Site Kit set up

  • Navigating directly to splash screen redirects to SK dashboard
  • SK dashboard does not redirect to splash

Disconnect via user menu

  • User lands on splash screen with same disconnect messaging as today
  • Session storage should be cleared

Secondarily, the URLs used for beginning the oAuth flow (connect) and disconnecting have been updated to use dedicated action URLs. This change should make no difference for the user.

Changelog entry

  • Redirect users from the splash screen to the dashboard if they are already authenticated.
@felixarntz felixarntz added Type: Bug Something isn't working P1 Medium priority Next Up labels Dec 14, 2020
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Dec 14, 2020
@asvinb asvinb self-assigned this Dec 15, 2020
@aaemnnosttv
Copy link
Collaborator

@felixarntz after digging into this a little with @asvinb it seems that this only happens when navigating to the splash screen directly once Site Kit is already set up. The title shown on the splash screen on the initial set up is consistent with other Site Kit screens for both primary and secondary users.

The problem is related to how WP derives the page title which seems to depend on the page having a parent. When the current user already has permissions for the dashboard (i.e. the main Dashboard menu item is present) the splash page is not registered as a sub-menu of the main page since the titles are the same. Because of this, the $title is blank in this case (which is still odd) but it doesn't seem possible for a user to actually see this without navigating to it directly.

@asvinb asvinb assigned felixarntz and unassigned asvinb Dec 17, 2020
@felixarntz felixarntz added P2 Low priority and removed Next Up P1 Medium priority labels Jan 7, 2021
@felixarntz felixarntz removed their assignment Jan 7, 2021
@felixarntz
Copy link
Member Author

@aaemnnosttv Ah, that makes sense. I'll deprioritize this for now, but still worth fixing in principle.

@benbowler benbowler self-assigned this Feb 26, 2021
@benbowler
Copy link
Collaborator

I tracked this down to, as @aaemnnosttv said, a bug/issue with Wordpress where title of the page is not set if the parent slug passed to add_submenu_page is NULL.

I found a solution and have added an IB.

@benbowler benbowler removed their assignment Feb 26, 2021
@aaemnnosttv aaemnnosttv self-assigned this Mar 1, 2021
@aaemnnosttv
Copy link
Collaborator

The IB is pretty close, with a few things to iterate on:

  • Let's use the current screen (WP_Screen) to for the logic in the conditional rather than query params
  • We can't simply concatenate the title with the value coming through the filter. We should translate this as its done in core to preserve an i18n-friendly result.

@eugene-manuilov
Copy link
Collaborator

@aaemnnosttv @felixarntz I have a question: why do we add the splash screen page when the user already has permissions? What's the point to have it? I think if the user already has permissions, we should redirect them to the dashboard page instead of showing the splash screen again and confuse users. What do you think?

@aaemnnosttv
Copy link
Collaborator

@eugene-manuilov – users shouldn't really land on this page in this state unless they navigated directly to it (from browser history perhaps?). I have found it useful to test things before by being able to navigate directly to it without disconnecting first, but I don't know if that is reason enough to keep this.

I agree that the issue as-is does not fix the the root cause, so a redirect to the dashboard would likely be a better experience. As mentioned previously, we already have a redirect in place from the dashboard to the splash page when the user is not authenticated, so we need to make sure this is well-documented and covered with tests to ensure this never results in a circular redirect loop, both now and in the future.

@felixarntz – if you agree, let me know and one of us can rewrite the issue/ACs accordingly.

@aaemnnosttv aaemnnosttv removed their assignment Mar 5, 2021
@felixarntz
Copy link
Member Author

@aaemnnosttv @eugene-manuilov Redirecting from splash to the dashboard when the user is already authenticated makes sense - let's do that.

@felixarntz felixarntz assigned aaemnnosttv and unassigned felixarntz Mar 19, 2021
@aaemnnosttv aaemnnosttv changed the title Splash screen has missing segment in <title> tag Redirect authenticated users from splash to dashboard Mar 29, 2021
@aaemnnosttv
Copy link
Collaborator

@felixarntz ACs updated + IB which includes moving handlers for connect/disconnect since these don't really belong on the splash URL either and is more consistent with how we're handling other action URLs for proxy setup and settings.

@felixarntz
Copy link
Member Author

IB ✅

@felixarntz felixarntz removed their assignment Mar 29, 2021
@eclarke1 eclarke1 added this to the Sprint 47 milestone Apr 7, 2021
@eugene-manuilov
Copy link
Collaborator

@felixarntz assigning it to you since Evan also wants you to review it.

@fhollis fhollis modified the milestones: Sprint 47, Sprint 48 Apr 26, 2021
@fhollis fhollis added the Rollover Issues which role over to the next sprint label Apr 26, 2021
@felixarntz felixarntz removed their assignment Apr 26, 2021
@wpdarren wpdarren self-assigned this Apr 27, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented May 3, 2021

QA Update: Pass ✅

Unauthenticated / Site Kit not set up yet
Splash screen loads without redirecting
Navigating directly to SK dashboard redirects to splash screen
/wp-admin/admin.php?page=googlesitekit-splash

image

Authenticated / Site Kit set up

  • Navigating directly to splash screen redirects to SK dashboard
  • SK dashboard does not redirect to splash

Disconnect via user menu

  • User lands on splash screen with same disconnect messaging.
  • Session storage is cleared but then new data is entered when the splash loads.

image

@wpdarren wpdarren removed their assignment May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Rollover Issues which role over to the next sprint Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants