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

Allow ability to select what user roles are excluded #1891

Closed
scottshefler opened this issue Aug 7, 2020 · 26 comments
Closed

Allow ability to select what user roles are excluded #1891

scottshefler opened this issue Aug 7, 2020 · 26 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
Milestone

Comments

@scottshefler
Copy link

scottshefler commented Aug 7, 2020

Feature Description

Currently only allowed to enable/disable excluding logged-in users. If you enable it this will also exclude front-end user logins for membership type sites. It's obviously very important to be able to track these types of logged in users.

The plugin should loop through and return all user roles that are setup on the site with a checkbox option to select which roles to exclude.

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

Acceptance criteria

  • Site Kit should allow to disable tracking for only users that can create content (even if they are logged in with a WordPress account), e.g. to satisfy the tracking requirements for membership sites:
    • The Analytics trackingDisabled option needs to support a new possible value contentCreators. If that value is enabled but loggedinUsers is not, tracking should be disabled if the current user can edit_posts (which is a WordPress capability every role except subscriber has).
    • The respective Analytics UI in the module settings panel should be updated for the new possible value:
      • The toggle for contentCreators should appear right of the existing toggle for loggedinUsers.
      • The label for the new toggle should be Users that can write posts. The label for the existing toggle should be rephrased to All logged-in users.
      • If the loggedinUsers toggle is enabled, the contentCreators toggle should not be visible. Only if it is disabled, the contentCreators toggle should be visible.
    • Keep in mind the option is an array. While that is not really applicable right now, it should be maintained as such. Even though loggedinUsers technically includes contentCreators, that is not a big deal since it should simply take precedence.

Implementation Brief

  • Edit includes/Modules/Analytics.php and update the is_tracking_enabled method to also set $disabled to true if 'contentCreators' is in the option array and the user can edit posts (current_user_can( 'edit_posts' );)
  • Edit assets/js/modules/analytics/components/common/TrackingExclusionSwitches.js
  • Add a constant of TRACKING_CONTENT_CREATORS with the value contentCreators
  • Add to and update the trackingExclusionLabels values
    • TRACKING_LOGGED_IN_USERS : All logged-in users
    • TRACKING_CONTENT_CREATORS : Users that can write posts
  • Duplicate the <Switch> component and update the code of the new one to use TRACKING_CONTENT_CREATORS
  • Update the onClick of the first <Switch> to onChangeTrackLoggedInUsers and the second one to onChangeTrackContentCreators
  • Rename the onChange function to onChangeTrackLoggedInUsers and modify it so that it adds and removes TRACKING_LOGGED_IN_USERS depending on whether the checkbox is checked. e.g:
        if ( checked ) {
		trackingDisabledArray = trackingDisabled.concat( TRACKING_LOGGED_IN_USERS );
	} else {
		trackingDisabledArray = trackingDisabled.filter( ( item ) => item !== TRACKING_LOGGED_IN_USERS );
	}
	setTrackingDisabled( trackingDisabledArray );
  • Create a similar function onChangeTrackContentCreators for the second <Switch> component's onClick
  • Only display the second switch if the trackingDisabled array does not contain TRACKING_LOGGED_IN_USERS
  • Add mdc-layout-grid tags and classes to position the switches side by side on desktop and tablet and vertically on mobile https://material.io/develop/web/supporting/layout-grid
  • Add stories to the following for the new switch
    • stories/module-analytics-stories.js
    • stories/module-analytics-settings-stories.js

Test Coverage

  • Update AnalyticsTest.php to test the change to includes/Modules/Analytics.php

Visual Regression Changes

  • Run visual regression tests and update as necessary

QA Brief

You will need to connect Analytics to your Site Kit install.

  • Go to the Analytics settings, and hit 'edit'
  • Go to the section entitled Exclude from Analytics
  • Verify that when
    • All logged-in users is NOT checked, an additional Users that can write posts switch is visible
    • All logged-in users is checked, the Users that can write posts is hidden
  • Verify that these options are saved and displayed in the settings panel correctly
  • Note the edge case discussed below: it's possible to have both switched on, in which case All logged-in users will take precedence.
  • Verify that tracking exclusions are working as in the settings by checking for the following script in the page HTML:
    <script type="text/javascript">window["_gaUserPrefs"] = { ioo : function() { return true; } }</script>
    • if tracking is disabled, the script SHOULD be present
    • if tracking is enabled, the script should NOT be present

Changelog entry

  • Add the ability to exclude users who can write posts from being tracked by Google Analytics
@jamesozzie
Copy link
Collaborator

@scottshefler Thanks for your suggestion. Note that all features requested are periodically reviewed by our product team, however there is no ETA for it.

@jamesozzie jamesozzie added the Type: Enhancement Improvement of an existing feature label Aug 7, 2020
@felixarntz felixarntz added the P2 Low priority label Mar 18, 2021
@felixarntz felixarntz self-assigned this Mar 18, 2021
@felixarntz felixarntz removed their assignment Mar 30, 2021
@felixarntz felixarntz added the Module: Analytics Google Analytics module related issues label Mar 30, 2021
@felixarntz felixarntz added P1 Medium priority and removed P2 Low priority labels Apr 13, 2021
@Hazlitte Hazlitte assigned Hazlitte and unassigned Hazlitte Apr 14, 2021
@gerardreches
Copy link

Hi!

Are there any news about this feature implementation? Do we at least have any action or filter available to hook into for now?

@felixarntz
Copy link
Member

@gerardreches This is currently being worked on and should be included in one of the upcoming plugin releases. In the meantime, you can always override the default behavior using the googlesitekit_analytics_tracking_disabled filter, which expects a boolean.

@fhollis fhollis added this to the Sprint 48 milestone Apr 21, 2021
@eugene-manuilov eugene-manuilov self-assigned this Apr 26, 2021
@eugene-manuilov
Copy link
Collaborator

@Hazlitte there are two things we still need to add to the IB: changes required on the PHP side and a note that we need to hide the second switch if the first one is enabled as it is described in the AC:

If the loggedinUsers toggle is enabled, the contentCreators toggle should not be visible. Only if it is disabled, the contentCreators toggle should be visible.

@Hazlitte
Copy link
Contributor

Hi @eugene-manuilov, on reviewing the IB I spotted that I hadn't included the requirement to add and update the switch labels. I have now done that. I have also added the requirements to:

  • only display the second switch if the first one is disabled
  • update the Analytics.php file
  • update the corresponding phpunit tests

@Hazlitte Hazlitte assigned eugene-manuilov and unassigned Hazlitte Apr 26, 2021
@eugene-manuilov
Copy link
Collaborator

Thanks, @Hazlitte! IB ✔️

@eugene-manuilov
Copy link
Collaborator

I have also updated the estimate to 7 since 11 seems to be too much.

@johnPhillips
Copy link
Contributor

@Hazlitte / @eugene-manuilov I have a question about an edge case here, and another about the UI.

The edge case is this:

  • Both switches are rendered and unchecked (i.e. because the logged in user switch is off, the content creator switch is rendered too)
  • User checks the content creator switch, meaning the array is now ["contentCreators"]
  • User then checks the logged in user switch, which hides the content creator switch

At this point, should the (hidden) content creator switch be unchecked because it is now overridden? I.e. the array is now just [ "loggedinUsers" ].
Or should we persist the checked state, i.e. the array is now ["contentCreators", "loggedinUsers"]?
Hope that makes sense. I understand that "loggedinUsers" probably overrides "contentCreators" so perhaps it doesn't matter, but I wanted to check.

Also, just a quick UI question regarding the text underneath the component - are we ok with something like this?:

if ( trackingDisabled.includes( TRACKING_LOGGED_IN_USERS ) ) {
	message = __( 'All logged-in users will be excluded from Analytics tracking.', 'google-site-kit' );
} else if ( trackingDisabled.includes( TRACKING_CONTENT_CREATORS ) ) {
	message = __( 'Users that can write posts will be excluded from Analytics tracking.', 'google-site-kit' );
} else {
	message = __( 'All logged-in users will be included in Analytics tracking.', 'google-site-kit' );
}

@eugene-manuilov
Copy link
Collaborator

Or should we persist the checked state, i.e. the array is now ["contentCreators", "loggedinUsers"]?

@johnPhillips Yes, let's keep both values. The loggedinUsers value will take precedence and contentCreators will be ignored.

Also, just a quick UI question regarding the text underneath the component - are we ok with something like this?

Yes, looks good to me.

@johnPhillips
Copy link
Contributor

@eugene-manuilov I wasn't sure how to actually test that the exclusion is working on my local environment - looking at the PHP tests, I think an opt-out script should be added to the HTML?
Let me know the specifics and I'll update the QAB. For now I've just put Verify that tracking exclusions are working as in the settings.

@johnPhillips johnPhillips removed their assignment Apr 30, 2021
@eclarke1 eclarke1 added this to the Sprint 49 milestone May 10, 2021
@wpdarren wpdarren self-assigned this May 10, 2021
@wpdarren
Copy link
Collaborator

QA Update: Fail ❌

@johnPhillips the styling is a little off I've noticed. As you can see from the screenshot, there's a lot of white space around the Exclude from Analytics text. In the latest release, the text and toggle are aligned to the left and have no white space.

image

This is more of a personal preference, but when the All logged-in users toggle is disabled, and the Users that can write posts toggle appears, it would be good for that to appear underneath, rather than to the side. This would mean the setting toggles are all displayed to the left. Not sure what @Pratheep-lab feels about that though?

image

Verified:

  • When all logged-in users is NOT checked, an additional Users that can write posts switch is visible.
  • When all logged-in users is checked, the Users that can write posts is hidden
  • The options are saved and displayed in the settings panel correctly.

image

  • When Users that can write posts is enabled, then the script is active on the website.
  • When Users that can write posts is disabled, then the script is not active on the website.
  • Tested to make sure that the script appeared as expected for an Editor and Admin user.

image

@wpdarren wpdarren assigned johnPhillips and unassigned wpdarren May 10, 2021
@johnPhillips
Copy link
Contributor

Thanks @wpdarren

I just followed the ACs and the IB:

The toggle for contentCreators should appear right of the existing toggle for loggedinUsers.

Add mdc-layout-grid tags and classes to position the switches side by side on desktop and tablet and vertically on mobile

But obviously the ACs were written a while ago, so if there was a particular reason for requiring the toggle on the right, that might not be the case now (CC @felixarntz).

Anyway, I'm more than happy to adjust it if needed, just let me know what needs doing. 👍

@Pratheep-lab
Copy link
Collaborator

That's a good point, I agree with your suggestion, @wpdarren. There is a slight chance of this control to be not noticed by the user if it is on the right with so much space in between. Unless there is a valid reason for this, it is better to keep the 2nd switch close to the 1st one.

@felixarntz
Copy link
Member

@johnPhillips I think the toggle should still be displayed on the right, but I agree that the spacing is off, it should be fixed, so that they are next to each other, instead of percentage-based or using a grid or so.

@Pratheep-lab
Copy link
Collaborator

@felixarntz Yes, no problem, as long as they both are close enough to be considered as one group of controls :)

@johnPhillips
Copy link
Contributor

Thanks for the feedback all. I've added a new PR which attempts to keep the layout (inline on desktop / tablet; stacked on mobile) but to reduce the gap between the switches. I hope this is ok - let me know if any further changes required.

Mobile Tablet, desktop
Screenshot 2021-05-11 at 12 10 28 Screenshot 2021-05-11 at 12 10 47
Screenshot 2021-05-11 at 12 10 14 Screenshot 2021-05-11 at 12 11 11

@wpdarren
Copy link
Collaborator

@eugene-manuilov @johnPhillips is this merged into develop branch yet? I'm not seeing the changes :)

@eugene-manuilov
Copy link
Collaborator

@wpdarren just merged it. Please, try again 😄

@wpdarren
Copy link
Collaborator

QA Update: Pass ✅

Verified:

  • When all logged-in users is NOT checked, an additional Users that can write posts switch is visible.
  • When all logged-in users is checked, the Users that can write posts is hidden
  • The options are saved and displayed in the settings panel correctly.

image

  • When Users that can write posts is enabled, then the script is active on the website.
  • When Users that can write posts is disabled, then the script is not active on the website.
  • Tested to make sure that the script appeared as expected for an Editor and Admin user.

image

@wpdarren wpdarren removed their assignment May 12, 2021
@gerardreches
Copy link

When will this be released?

@wpdarren
Copy link
Collaborator

@gerardreches it is planned to go into the next release. We are currently smoke testing and will hopefully push it for release next week.

@limcolin
Copy link

Hi team, thanks for adding this feature. I just wanted to ask whether 'Posts' include custom post types when excluding "Users that can write posts"?

The concern I have is whether we are excluding public, non-creator users that can write (e.g.) reviews which currently sit as CPTs.

Thanks!

@jamesozzie
Copy link
Collaborator

@limcolin Posts do include custom post types, meaning any such configurations would apply to a "Reviews" CPT. For queries such as this going forward you can reach out in the plugin support forums. Likewise if you have any further questions on this, feel free to open a support topic.

@limcolin
Copy link

Much appreciated for the info @jamesozzie thanks for responding - will check out the forums!

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