Allow ability to select what user roles are excluded

scottshefler commented 4 years ago

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: https://github.com/google/site-kit-wp/blob/b043daa84e6a4f5166f52022e7a1c6d37d1feabb/includes/Modules/Analytics.php#L1185
- 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 commented 4 years ago

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

gerardreches commented 3 years ago

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 commented 3 years ago

@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.

eugene-manuilov commented 3 years ago

@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 commented 3 years ago

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

eugene-manuilov commented 3 years ago

Thanks, @Hazlitte! IB :heavy_check_mark:

eugene-manuilov commented 3 years ago

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

johnPhillips commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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.

eugene-manuilov commented 3 years ago

@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?

If you disable tracking for let's say content creators and your user can edit posts, then you ~~shouldn't~~ should see this line in the page source HTML code when you open a page on the frontend: https://github.com/google/site-kit-wp/blob/b043daa84e6a4f5166f52022e7a1c6d37d1feabb/includes/Modules/Analytics.php#L1185 Otherwise, this line ~~should~~ shouldn't be added to the page HTML markup.

johnPhillips commented 3 years ago

Thanks @eugene-manuilov - QAB updated 👍

wpdarren commented 3 years ago

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.

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?

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.

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.

johnPhillips commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

johnPhillips commented 3 years ago

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

wpdarren commented 3 years ago

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

eugene-manuilov commented 3 years ago

@wpdarren just merged it. Please, try again :smile:

wpdarren commented 3 years ago

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.

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.

gerardreches commented 3 years ago

When will this be released?

wpdarren commented 3 years ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

google / site-kit-wp