Tracks: Don't track users in dev mode or when opted out #13698

scruffian · 2019-10-09T16:44:05Z

We shouldn't fire Tracks events when we are in dev mode, or if users haven't accepted our TOS, or if they have opted out of Tracks.

Changes proposed in this Pull Request:

This PR adds a check for these three cases to ensure that we don't fire Tracks events in these cases

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This came up here: Add tracking for the contact form block #13688 (comment), but is a more generic solution

Testing instructions:

Upgrade your Jetpack site to Professional
Add a Jetpack Search widget to the site
Check that you see a jetpack_search_widget_widget_added event in Tracks
Enable development mode (by adding this to wp-config.php):
define( 'JETPACK_DEV_DEBUG', true );
Add a Jetpack Search widget to the site
Check that you don't see another jetpack_search_widget_widget_added event in Tracks

Proposed changelog entry for your changes:

No changelog required?

jetpackbot · 2019-10-09T16:46:22Z

	Warnings
⚠️	`pre-commit` hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 8a14c13

This will avoid having to bypass the pre-commit hook for all commits to this file

matticbot · 2019-10-10T15:03:50Z

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

Called "record_user_event" function.

cc: @pesieminski

kraftbj · 2019-10-10T15:07:37Z

Let's update the legacy file too. I'll push a commit.

jeherve

I added a commit to fix all PHPCS issues, to avoid having to bypass the pre-commit hook for that file in the future.

I also have one remark, I think we could simplify this a bit.

packages/tracking/src/Tracking.php

The TOS check is used for confirming that a site is "Trackable". For sites in development mode or are not active, we should not track them.

packages/tracking/legacy/class.tracks-client.php

Just to be sure. See #13698 (comment)

Don't track users in dev mode or when opted out * Fix all phpcs issues This will avoid having to bypass the pre-commit hook for all commits to this file * [not verified] Opt out of tracking in dev mode in legacy branch. * PHPCS fixes * Include development mode check within Jetpack::jetpack_tos_agreed. The TOS check is used for confirming that a site is "Trackable". For sites in development mode or are not active, we should not track them. * Docblock for is_active_and_not_development function * Bring cookie check back Just to be sure. See Automattic/jetpack#13698 (comment)

enejb · 2019-10-14T12:19:24Z

packages/tracking/src/Tracking.php


 		// We don't want to track user events during unit tests/CI runs.
 		if ( $user instanceof \WP_User && 'wptests_capabilities' === $user->cap_key ) {
 			return false;
 		}

+		// Don't track users who have opted out or not agreed to our TOS, or are not running an active Jetpack.
+		if ( ! \Jetpack::jetpack_tos_agreed() ) {


Since Jetpack is not a package this means that his package is not usable outside of jetpack.
Which kind of defeats the purpose of this a package.

That's a good point. Isn't it already an issue with this already?

jetpack/packages/tracking/legacy/class.tracks-client.php

Line 60 in 8a14c13

if ( ! Jetpack::jetpack_tos_agreed() || ! empty( $_COOKIE['tk_opt-out'] ) ) {

Does this mean we should also refrain from relying on constants like JETPACK__PLUGIN_FILE or JETPACK__VERSION, that are not set anywhere but in Jetpack right now?

To be honest I don't quite know.
but I always assume that the src folder is there code that is supposed to be working also outside of jetpack and the src are classes that could be used outside of Jetpack.

I think using a filter here instead would work well.

@enejb Is this what you had in mind? #13746

The constants are a different thing FWIW, we have the constants package and we can use it to access any constants inside packages without relying on the core plugin.

See #13698 (comment)

See Automattic/jetpack#13698 (comment)

[not verified] Don't track users in dev mode or when opted out

39a6d0f

scruffian added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Feature] Tracks labels Oct 9, 2019

scruffian requested a review from a team as a code owner October 9, 2019 16:44

scruffian self-assigned this Oct 9, 2019

scruffian mentioned this pull request Oct 9, 2019

Add tracking for the contact form block #13688

Merged

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 9, 2019

jeherve added this to the 7.9 milestone Oct 10, 2019

Fix all phpcs issues

b21b20b

This will avoid having to bypass the pre-commit hook for all commits to this file

matticbot added the [Status] Needs GDPR Review label Oct 10, 2019

jeherve requested changes Oct 10, 2019

View reviewed changes

packages/tracking/src/Tracking.php Outdated Show resolved Hide resolved

packages/tracking/src/Tracking.php Outdated Show resolved Hide resolved

packages/tracking/src/Tracking.php Outdated Show resolved Hide resolved

[not verified] Opt out of tracking in dev mode in legacy branch.

90ec446

kraftbj added 2 commits October 10, 2019 10:21

PHPCS fixes

fa2a08c

Include development mode check within Jetpack::jetpack_tos_agreed.

7adc285

The TOS check is used for confirming that a site is "Trackable". For sites in development mode or are not active, we should not track them.

kraftbj force-pushed the update/tracking-in-dev-mode branch from 7921945 to 7adc285 Compare October 10, 2019 15:32

kraftbj added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs GDPR Review labels Oct 10, 2019

kraftbj mentioned this pull request Oct 10, 2019

Status Package: Add additional checks #13711

Open

Docblock for is_active_and_not_development function

5ec1b0b

kraftbj requested a review from jeherve October 10, 2019 15:42

jeherve requested changes Oct 11, 2019

View reviewed changes

packages/tracking/legacy/class.tracks-client.php Show resolved Hide resolved

packages/tracking/legacy/class.tracks-client.php Outdated Show resolved Hide resolved

Bring cookie check back

8a14c13

Just to be sure. See #13698 (comment)

jeherve approved these changes Oct 11, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 11, 2019

kraftbj merged commit 4accf77 into master Oct 11, 2019

kraftbj deleted the update/tracking-in-dev-mode branch October 11, 2019 13:23

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 11, 2019

jeherve removed the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 11, 2019

enejb reviewed Oct 14, 2019

View reviewed changes

jeherve removed the [Status] Needs Changelog label Oct 14, 2019

jeherve added a commit that referenced this pull request Oct 14, 2019

Tracks: use filter instead of relying on Jetpack class

8f4af2b

See #13698 (comment)

jeherve mentioned this pull request Oct 14, 2019

Tracks: use filter instead of relying on Jetpack class #13746

Merged

kraftbj pushed a commit that referenced this pull request Oct 15, 2019

Tracks: use filter instead of relying on Jetpack class (#13746)

2d21118

See #13698 (comment)

jeherve added a commit to Automattic/jetpack-tracking that referenced this pull request Oct 16, 2019

Tracks: use filter instead of relying on Jetpack class (#13746)

c3ef8d0

See Automattic/jetpack#13698 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracks: Don't track users in dev mode or when opted out #13698

Tracks: Don't track users in dev mode or when opted out #13698

scruffian commented Oct 9, 2019

jetpackbot commented Oct 9, 2019 •

edited

Loading

matticbot commented Oct 10, 2019

kraftbj commented Oct 10, 2019

jeherve left a comment

enejb Oct 14, 2019

jeherve Oct 14, 2019

enejb Oct 14, 2019

jeherve Oct 14, 2019

tyxla Oct 15, 2019

Tracks: Don't track users in dev mode or when opted out #13698

Tracks: Don't track users in dev mode or when opted out #13698

Conversation

scruffian commented Oct 9, 2019

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

jetpackbot commented Oct 9, 2019 • edited Loading

matticbot commented Oct 10, 2019

kraftbj commented Oct 10, 2019

jeherve left a comment

Choose a reason for hiding this comment

enejb Oct 14, 2019

Choose a reason for hiding this comment

jeherve Oct 14, 2019

Choose a reason for hiding this comment

enejb Oct 14, 2019

Choose a reason for hiding this comment

jeherve Oct 14, 2019

Choose a reason for hiding this comment

tyxla Oct 15, 2019

Choose a reason for hiding this comment

jetpackbot commented Oct 9, 2019 •

edited

Loading