site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

User menu does not show which account is connected

Open aaemnnosttv opened this issue 1 year ago • 13 comments

Feature Description

Before the unified dashboard, the SK user menu used to have the user's email address in the header which showed the address of the connected Google account. This is no longer shown which makes for a more consistent header with other Google products, although it makes it hard to tell which user is connected as this isn't even surfaced in the site health info.

Most Google products show the email address of the connected account on hover or otherwise in the user menu, so we should do the same.

Otherwise, for users who use the same photo for multiple Google accounts, it can be almost impossible to tell which one is connected.


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

Acceptance criteria

  • The tooltip for the user menu should be updated to show the user's Google account for consistency with other Google services
  • The tooltip text for the user menu should be updated to:

    Google Account {Full Name} {google account email address}

  • The googlesitekit_profile user option should be updated to store a new full_name property using the first displayName from the user's names returned by the People API (see OAuth_Client::refresh_profile_data)

Implementation Brief

  • In OAuth_Client::refresh_profile_data(): Add names to the personFields' value when fetching people data within the people.get() method. This will add a names array to the response. Add this property within the profile->set() params as per the AC, i.e. 'full_name' => $response['names'][0]['displayName'].
  • In assets/js/googlesitekit/datastore/user/user-info.js: Create a getFullName selector similar to getPicture. (Return the user.full_name property if it is defined.)
  • Currently, our <Button> component uses the aria-label (or title) of the Button as the Tooltip text. Modify assets/js/components/Button.js and create a new customizedTooltip prop which can accept a React element and renders a Customised MUI Tooltip (which allows text on multiple lines) if that prop is passed.
  • In assets/js/components/UserMenu.js:
    • Use the new getFullName selector to fetch the user's full name.
    • Pass a react element to the new customizedTooltip prop of the <Button> component to render the tooltip as per the AC.
  • Port/refactor the simple stories/user-menu.stories.js into a new Storybook v6 story and ensure the customized tooltip shows correctly. Ensure the other Storybook v5 plugin header stories (stories/header.stories.js) render the new tooltip correctly.

Test Coverage

  • Update OAuth_ClientTest::test_refresh_profile_data()
  • Add tests for the new selector in user-info.test.js.

QA Brief

  • Navigate to the Dashboard and hover over the User menu.
  • Verify the tooltip appears as per the AC.
  • Check the tooltip appears in the Header and UserMenu stories.
    • Note, the UserMenu story will now appear under Components rather than Global.
  • A note for QA:
    • The full name will currently only appear on a new Site or after you reset Site Kit. This is not the long-term plan, see: #6003
  • Test that old sites without the user's full name do not error:
    • Sign in to a site using Site Kit before using a branch with this change (eg. sign in using version 1.85.0).
    • Replace your Site Kit plugin with develop
    • Ensure no errors appear when accessing Site Kit.
    • Note that your name will not appear as it was not requested in previous versions of Site Kit, but no error should appear.

Changelog entry

  • Show user account info in user menu.

aaemnnosttv avatar Aug 22 '22 18:08 aaemnnosttv

@felixarntz do you think we need UX input here? Also, most (if not all?) Google services show the user's name as well (not just the email) but we only have the email. Should we update our Profile to include the name too and show this if it's available? That way it would work for existing users too.

aaemnnosttv avatar Aug 29 '22 22:08 aaemnnosttv

@aaemnnosttv I think it would be great to look up the name then too so we can display fully consistent information.

Regarding the user's avatar, I don't think we need to enforce it to be 80x80, IMO the way it currently is is fine. I would suggest we make this issue only about the tooltip content, to match the regular Google experience there.

Just to confirm, the ACs otherwise are only about the tooltip correct? That's a bit unclear to me now when I read them.

felixarntz avatar Aug 30 '22 20:08 felixarntz

Regarding the user's avatar, I don't think we need to enforce it to be 80x80, IMO the way it currently is is fine. I would suggest we make this issue only about the tooltip content, to match the regular Google experience there.

@felixarntz this isn't suggesting we change the size in the header but what is shown in the dropdown.

image

So as a whole the suggestion here is to update the tooltip content, and add the larger avatar, name, and email to the top of the body of the dropdown as above (without the ability to edit your avatar of course).

aaemnnosttv avatar Aug 31 '22 17:08 aaemnnosttv

@aaemnnosttv Ah that makes potentially more sense, but I think it has a bit more UX implications. We don't 100% mirror the Google UI anyway, so I don't know if we really need to have that information within the dropdown content.

I'd prefer if we focused this issue only on the tooltip content, and this way we can build the foundation here to also store the account name and then display it in the tooltip. We could open a follow-up issue to further evaluate what you're suggesting above regarding the dropdown content. WDYT?

felixarntz avatar Aug 31 '22 18:08 felixarntz

We don't 100% mirror the Google UI anyway

True, but I think this portion of it makes sense to mirror (as it's rather small) but also relevant to the connected user which isn't currently shown.

I don't know if we really need to have that information within the dropdown content.

IMO, the email should be there at the least – we used to have it in the SK header! I would say following established UX for this type of thing used across products would feel more intuitive than deviating from it as we are now (even if somewhat accidentally perhaps).

I'd prefer if we focused this issue only on the tooltip content, and this way we can build the foundation here to also store the account name and then display it in the tooltip. We could open a follow-up issue to further evaluate what you're suggesting above regarding the dropdown content. WDYT?

Sure, although it might be a bit easy to miss if only in the tooltip. I'll update the ACs here to be limited to the tooltip and open a follow up issue as you suggested.

aaemnnosttv avatar Aug 31 '22 19:08 aaemnnosttv

Thanks @jimmymadon – just a few points:

  • Since any authenticated user won't have the name set yet, let's only render that part if there is one
  • There are some relevant tests for test_refresh_profile_data which should get updated here as well, so let's note this in the test coverage and perhaps bump the estimate

aaemnnosttv avatar Sep 16 '22 21:09 aaemnnosttv

Thanks @aaemnnosttv - have updated the IB!

jimmymadon avatar Sep 17 '22 09:09 jimmymadon

IB ✅

aaemnnosttv avatar Sep 20 '22 18:09 aaemnnosttv

(@techanvil won't be able to look at this for a bit, but there's an issue with the PR—I left a review. If someone else can pick it up that'd be great and I can review again, otherwise I'll take a look at this Monday or Tuesday.)

tofumatt avatar Sep 30 '22 21:09 tofumatt

Hi @tofumatt just noticed this is in execution but not assigned, are you still planning on picking this up as Tom is o.o.o? Thanks!

FlicHollis avatar Oct 07 '22 10:10 FlicHollis

I'll give this a look since I did the IB and have some context. Will go through @tofumatt 's comments on the open PR.

jimmymadon avatar Oct 08 '22 00:10 jimmymadon

My comment here is affecting any site that was set up before this change: https://github.com/google/site-kit-wp/pull/5946#pullrequestreview-1139827695

I'm guessing when you tested this you were using a site that had been reset and pulling this "full name" data, but for sites that haven't retrieved this data yet there will be an error:

image

It's because this line is new: https://github.com/google/site-kit-wp/blob/0def4c239441e78f2eef88da46891a30d119fe8e/includes/Core/Authentication/Clients/OAuth_Client.php#L516

So we'll want to check if $profile_data[ 'full_name' ] is empty before accessing it, and account for a possible null value throughout the code for sites that haven't retrieved it. I suppose we could also just set it as an empty string until it's updated, or if the value is empty force a profile refresh so it's immediately available. 🤔

tofumatt avatar Oct 13 '22 11:10 tofumatt

QA Update ✅

  • Verified on dev.
  • Verified for new site as well as for old site.
  • On older site full name not display as per QAB but it display when user reset plugin or connect any module.
  • No errors showing on the dev branch.
  • The tooltip text for the user menu should be updated to:

Google Account {Full Name} {google account email address}

Older site :

image

After connecting new module or resetting the plugin

image

mohitwp avatar Oct 18 '22 07:10 mohitwp

⚠️ Approval

On review, the tooltip looks off with an empty space where the full name would be. In this case, there are 3 consecutive line breaks where the last one should only be there if there is a full name.

This should be very quick to correct with a follow up PR.

aaemnnosttv avatar Oct 20 '22 15:10 aaemnnosttv

All good now 👍

Screenshots

image image

aaemnnosttv avatar Oct 20 '22 18:10 aaemnnosttv