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

Prevent users from connecting GA4 without access to UA #5886

Closed
aaemnnosttv opened this issue Sep 23, 2022 · 19 comments
Closed

Prevent users from connecting GA4 without access to UA #5886

aaemnnosttv opened this issue Sep 23, 2022 · 19 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Sep 23, 2022

Feature Description

Analytics is a bit of a special case when it comes to modules as it is essentially one but technically implemented as two under the hood. In #4825 we added guardrails to the module settings edit views for modules with service entities to prevent an admin without access to the configured entity to change it.

For GA4, we do the same thing – prevent the user from changing the connection without access. Since older configurations could be setup without GA4, we should additionally require that only admins that have access to the current configured UA entity can connect or change the GA4 connection.

At the same time, we should evaluate if it should even be possible for UA and GA4 to have different module owners, which is currently possible.


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

Acceptance criteria

  • The Settings Edit screen for Analytics should be modified to prevent an admin from connecting or changing the connected GA4 property without having access to the connected UA entity

    • The loading state should wait for access to be checked for both UA + GA4 (if connected) before showing the main settings form interface
  • If GA4 is not connected, the toggle to enable GA4 (not the snippet) should be disabled with an added notice explaining that user needs access to UA in order to connect GA4 (exact copy below). The style here should largely follow what is already in place for the insufficient access restrictions in the settings view. The notice should specifically indicate the owner of the main/UA Analytics module as the point of contact for access.

    {username} configured Analytics and you don’t have access to its configured property. Contact them to share access or setup Google Analytics 4.

  • If GA4 is connected, the existing logic in place around insufficient access restrictions for analytics-4 should be amended to also require the user has access to the current UA analytics entity in order to change the connected GA4 entity. In this case, the notice shown should use alternate copy (see below) and have a higher priority over the GA4 notice. E.g. if the user didn't have access to the UA or the GA4 entities, they would only see the notice about UA

    {username} configured Analytics and you don’t have access to its configured property. Contact them to share access or change the configured Google Analytics 4 property.

  • The current messages which are shown when a user does not have access to the current configured entity should be amended for consistency

    {username} configured {Analytics} and you don’t have access to its configured property. Contact them to share access or change the configured property.

    Where {Analytics} is either Analytics or Analytics 4 respectively.

Implementation Brief

  • In assets/js/modules/analytics/components/settings/SettingsForm.js:
    • Locate the <GA4SettingsControls /> component.
      • Remove the hasModuleAccess prop passed to it.
      • Pass both hasAnalyticsAccess and hasAnalytics4Access props to it.
  • In assets/js/modules/analytics/components/common/GA4ActivateSwitch.js:
    • Accept a new prop named disabled.
    • Pass the disabled prop to the <Switch /> component.
  • In assets/js/modules/analytics/components/settings/GA4SettingsControls.js:
    • Accept hasAnalyticsAccess and hasAnalytics4Access props to the component instead of hasModuleAccess. Do not replace the usages of hasModuleAccess because we will create a new variable with the same name.
    • Locate the <GA4ActivateSwitch /> component and add the disabled prop to it. The disabled prop should be truthy only if hasAnalyticsAccess is falsey.
    • Check if the analytics-4 module is connected by running the isModuleConnected selector from the core/modules store.
    • Create a new boolean variable named hasModuleAccess. It should be true if analytics-4 is connected, and both hasAnalyticsAccess & hasAnalytics4Access are truthy.
    • Update the <SettingsNotice /> component:
      • Display it if any of the following conditions are met, replacing current condition(s):
        • If analytics-4 is not connected and hasAnalyticsAccess is falsey.
        • If analytics-4 is connected and hasAnalyticsAccess is falsey.
        • If analytics-4 is connected and hasAnalyticsAccess is truthy, but hasAnalytics4Access is falsey.
      • Leaving all the other props in it intact, update the notice prop depending on the above conditions.
        • If the first condition is met, display the first message quoted in the ACs.
        • Otherwise, if the second condition is met, display the second message quoted in the ACs.
        • Otherwise, if the third condition is met, display the third message quoted in the ACs with Analytics 4 replacing {Analytics}.
  • In assets/js/components/settings/SettingsActiveModule/Header.js:
    • Display the Connect Google Analytics 4 CTA only if the current user has access to Analytics (UA).
    • In order to check if the current user has access to Analytics (UA), a useSelect hook usage similar to hasAnalyticsAccess from assets/js/modules/analytics/components/settings/SettingsEdit.js can be used.
  • In assets/js/modules/analytics/components/settings/SettingsControls.js:
    • Update the notice prop of the <SettingsNotice /> component to match the third message quoted in the ACs with Analytics replacing {Analytics}.
  • In assets/js/modules/analytics/components/settings/SettingsForm.stories.js:
    • Replace the Settings w/o module access story with two distinct stories, one with no access to Analytics and the other with no access to Analytics 4.

Test Coverage

  • Fix any failing tests. A potential test that could fail is the should render a button to connect GA4 if Analytics is connected but GA4 is not test case in assets/js/components/settings/SettingsActiveModule/Header.test.js.
  • Update VRT references if required.

QA Brief

  • Connect Analytics (UA) but not GA4 as an Admin1.
  • To mimic disconnecting GA4, delete googlesitekit_analytics-4_settings from the wp_options table.
  • Go to the settings page and ensure the Connect Google Analytics 4 CTA is visible on the Module header for the Admin1.
  • Click on the CTA and ensure no access required notices are displayed as per the AC.
  • Login as an Admin2 using another Google account and ensure the Connect Google Analytics 4 CTA is not visible since the Admin1 initially set up Analytics.
  • Expand the Analytics header and click on the Edit link to go to the /edit page. This is the same page that will be navigated when we click the Connect Google Analytics 4 CTA.
  • Ensure the Activate Google Analytics 4 and place code on your site. toggle switch is disabled when the user doesn't have access to the Analytics (UA).
  • Ensure the notices are displayed as per the AC.
  • In the Modules -> Analytics -> Settings -> SettingsEdit storybook, ensure the following newly added stories are as expected:
    • Settings w/o UA and GA4 access, GA4 not connected
    • Settings w/o UA and GA4 access, GA4 connected
    • Settings w/o Analytics access, GA4 connected
    • Settings w/o Analytics-4 access, GA4 connected
    • Settings w/o UA and GA4 access, fallback owner name

Update

  • Follow the above steps and ensure the Connect Google Analytics 4 CTA is still visible for the second admin who doesn’t have access to Analytics.

Update

  • Ensure the Connect Google Analytics 4 CTA is hidden for the users who don't have Analytics access; instead shows a status Google Analytics 4 is not connected. Please refer to this comment for more details.

Update

  • Verify that the flicker reported here doesn't occur anymore and a progress bar is appropriately shown when access is being checked.

Changelog entry

  • Prevent users from changing the Google Analytics 4 configuration when they don't have access to the currently configured Universal Analytics property.
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Sep 23, 2022
@aaemnnosttv aaemnnosttv removed their assignment Oct 12, 2022
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Oct 13, 2022
@techanvil techanvil self-assigned this Oct 17, 2022
@techanvil
Copy link
Collaborator

IB ✅

@nfmohit I made a minor adjustment and fixed a typo which you can see in the edit history, I didn't think these merited the round trip!

@jimmymadon
Copy link
Collaborator

@aaemnnosttv Partially assigned this to you only to get your thoughts on this CR comment. Thanks!

@hussain-t hussain-t assigned jimmymadon and unassigned hussain-t Nov 11, 2022
@aaemnnosttv aaemnnosttv removed their assignment Nov 11, 2022
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Nov 21, 2022
@jimmymadon jimmymadon assigned hussain-t and unassigned jimmymadon Nov 22, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Dec 2, 2022

@aaemnnosttv I've created #6270 to address this. Thank you!

@nfmohit nfmohit assigned nfmohit and aaemnnosttv and unassigned aaemnnosttv and nfmohit Dec 2, 2022
@aaemnnosttv aaemnnosttv assigned nfmohit and unassigned aaemnnosttv Dec 3, 2022
@aaemnnosttv
Copy link
Collaborator Author

Thanks @nfmohit 👍 there's a little more to do on that one but not much, I've sent it back with you.

More importantly, this issue seems to have introduced a regression (currently in main) from the current release regarding the behavior of the "Activate Google Analytics 4 and place code on your site." toggle for users who don't have GA4 enabled yet. This could have been missed due to another bug that I discovered which was introduced in 1.88.0 which has the effect of automatically enabling/skipping this toggle. See #6271.

The issue here however is that once GA4 is enabled/toggled-on, the property select remains disabled.

Both of these should really be fixed for the release.

@aaemnnosttv
Copy link
Collaborator Author

@wpdarren this is ready for review again – please note also that your observation #6271 (comment) should be resolved as well.

Just to note for QA as well, the cases we want to ensure are checked here are:

  • Module owner (always has access)
    • never sees progress bar, always sees button to Connect GA4
  • Non-module owner, with access
    • sees progress bar with warning, then button to Connect GA4
  • Non-module owner, without access
    • sees progress bar with warning, then status text that GA4 is not connected

In all cases, if GA4 is connected, there should be no progress bar and it should always show as "Connected" as other modules.

We also want to see that the height of the collapsed module settings row does not change between loading and loaded states in each case. This is somewhat less important than all the other functional details but is something we paid attention to addressing in CR.

@aaemnnosttv aaemnnosttv assigned wpdarren and unassigned aaemnnosttv Dec 5, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Dec 5, 2022

QA Update: ✅

Verified:

  • Module owner (always has access) - never sees progress bar, always sees button to Connect GA4
  • Non-module owner, with access - sees progress bar with warning, then button to Connect GA4
  • Non-module owner, without access - sees progress bar with warning, then status text that GA4 is not connected
  • In all cases, if GA4 is connected, there is no progress bar and it always shows as "Connected" as other modules.
  • The height of the collapsed module settings row does not change between loading and loaded states in each case.
  • The issue reported on 6271 is fixed. I am able to create a new property.
ga-5.mp4
Screenshots

image
image
image
image

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 Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants