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

Duplicate messages creating odd UX/UI when 2nd Admin user does not have access to Analytics #7004

Closed
wpdarren opened this issue May 3, 2023 · 6 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Type: Bug Something isn't working UX Issues that require UX input

Comments

@wpdarren
Copy link
Collaborator

wpdarren commented May 3, 2023

Bug Description

Bug found during testing #6224. When you edit Analytics settings as a second admin user who does not have access to the Analytics account/property, a notice appears underneath the drop downs. We now have red error messages appearing within the UI which in my opinion makes for an odd user experience.

From chatting with Tom.

I think the new errors in red are appearing as a result of the work in #6831. It doesn't seem problematic to me, although we could discuss it on a separate issue to see if we'd like to prevent certain types of error messages from displaying when we're also showing the grey notices.

image

In my opinion, we should remove the red error messages when there are grey warning notices on the settings.


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

Acceptance criteria

  • When a user without access to an Analytics account (like the 2nd user in this case) edits Analytics settings, requests that cause the red permissions error to appear (like in the screenshot) should not be made. Instead, only the messages in grey, below the account select, should appear.
  • The red error messages in the GA4 and UA settings should not appear when a 2nd admin edits Analytics setting, with and without the GA4 Reporting feature flag enabled.

Implementation Brief

  • In assets/js/modules/analytics/components/settings/SettingsForm.js:
    • When fetching properties using getProperties, check if hasAnalyticsAccess OR hasAnalytics4Access are falsy in addition to the accountID check.
  • In assets/js/modules/analytics/components/common/AccountSelect.js:
    • When fetching properties using getProperties, check if hasModuleAccess is falsy in addition to the accountID check.
  • In WebDataStreamSelect.js and PropertyOrWebDataStreamNotAvailableError.js:
    • When fetching webDataStreams using getMatchingWebDataStreamsByPropertyID, check if hasModuleAccess is not false in addition to the isValidPropertyID( propertyID ) check.

Test Coverage

  • No new tests required.

QA Brief

Changelog entry

  • Prevent duplicate error messages from appearing in Analytics settings when another user does not have access to the Analytics account.
@wpdarren wpdarren added Type: Bug Something isn't working Module: Analytics Google Analytics module related issues UX Issues that require UX input labels May 3, 2023
@techanvil techanvil added P1 Medium priority P1 Med labels May 3, 2023
@tofumatt tofumatt assigned tofumatt and jimmymadon and unassigned tofumatt May 5, 2023
@jimmymadon jimmymadon removed their assignment May 5, 2023
@tofumatt tofumatt assigned tofumatt and jimmymadon and unassigned tofumatt May 5, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented May 5, 2023

IB ✅

Assigning to @jimmymadon since he's working on #7005 and they're closely related.

@jimmymadon
Copy link
Collaborator

@nfmohit @techanvil @aaemnnosttv While working on preventing requests to fetch properties when the current logged in user doesn't have access to the Analytics account - I realised there are a few places in the code which completely hide the "Universal Analytics" toggle and settings if there are no properties available. I am not sure if this is desirable. I believe we would still want to show the toggle and the settings but they should be disabled like the rest of the settings. I will create a separate issue to fix this. We simply have to differentiate between when the properties aren't available due to no access or because there really aren't any properties available for the selected account. Some small tweaks will be required below:

useEffect( () => {
if ( isUAEnabled && properties.length === 0 ) {
setValues( FORM_SETUP, { enableUA: false } );
}
}, [ isUAEnabled, properties, setValues ] );

{ ga4ReportingEnabled && properties.length > 0 && (
<div className="googlesitekit-settings-module__fields-group">
<h4 className="googlesitekit-settings-module__fields-group-title">
{ __( 'Universal Analytics', 'google-site-kit' ) }
</h4>
<EnableUniversalAnalytics
hasModuleAccess={ hasAnalyticsAccess }
showErrors
>
<SettingsUseSnippetSwitch />
</EnableUniversalAnalytics>
</div>
) }

if ( properties.length === 0 ) {
return null;
}

@jimmymadon jimmymadon removed their assignment May 8, 2023
@tofumatt tofumatt self-assigned this May 8, 2023
@aaemnnosttv
Copy link
Collaborator

I realised there are a few places in the code which completely hide the "Universal Analytics" toggle and settings if there are no properties available. I am not sure if this is desirable. I believe we would still want to show the toggle and the settings but they should be disabled like the rest of the settings.

@jimmymadon If the interface would otherwise include the toggle, then that means that UA wasn't already connected, so it wouldn't be possible to enable UA in this situation and the only enhancement to make here would be to show the toggle but in a disabled state. I don't think it would make sense to allow enabling it only to not be able to do anything (no point in toggling it only to reveal disabled controls).

I suppose rather than hiding the toggle entirely, we could render it in a disabled OFF state so that it wouldn't introduce loading-related layout shifts. Does that make sense?

@jimmymadon
Copy link
Collaborator

jimmymadon commented May 9, 2023

If the interface would otherwise include the toggle, then that means that UA wasn't already connected

@aaemnnosttv In my testing, if admin1 has enabled UA then the UA toggle and settings are visible on their "settings edit" page. Now if admin2 does not have access to this UA property, then when they edit the settings, they should at least be able to see the current UA settings in a disabled state along with the toggle + the usual "no access notice" (rather than have the whole component hidden).

Screen.Recording.2023-05-09.at.15.59.32.mov

@mohitwp
Copy link
Collaborator

mohitwp commented May 12, 2023

QA Update ✅

Verified

  • The red error messages in the GA4 and UA settings are not appearing when a 2nd admin edits Analytics setting, with and without the GA4 Reporting feature flag enabled.
  • Tested on dev environment.
  • When 'ga4Reporting' feature flag is enabled and only GA4 is connected.
  • When 'ga4Reporting' feature flag is enabled and both GA4 and UA is connected.
  • Verified for both ga4 and UA dashboard view when 'Ga4Reporting' feature flag is enabled.
  • Also when 'ga4Reporting' feature flag is not enabled.
  • When 'ga4Reporting' feature flag is not enabled and only UA is connected.
  • Also, done regression testing of analytics setup when 'ga4Reporting' feature flag is not enabled.

When 'ga4Reporting' feature flag is enabled and only GA4 is connected.

Recording.332.mp4

When 'ga4Reporting' feature flag is enabled and both GA4 and UA is connected.

Recording.333.mp4

When dashboard view is UA.

Recording.334.mp4

When 'ga4Reporting' feature flag is not enabled.

Recording.335.mp4

When 'ga4Reporting' feature flag is not enabled and only GA4 is connected.

Recording.336.mp4

@mohitwp mohitwp removed their assignment May 12, 2023
@aaemnnosttv
Copy link
Collaborator

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Type: Bug Something isn't working UX Issues that require UX input
Projects
None yet
Development

No branches or pull requests

6 participants