Skip to content

Implement setOptOut cookie command#9085

Closed
jaswrks wants to merge 3 commits intomasterfrom
add/set-opt-out-call-check
Closed

Implement setOptOut cookie command#9085
jaswrks wants to merge 3 commits intomasterfrom
add/set-opt-out-call-check

Conversation

@jaswrks
Copy link
Copy Markdown
Contributor

@jaswrks jaswrks commented Mar 19, 2018 •
edited
Loading

NOTE: Please consider merging #9156 instead of this.

PR Objectives

  • Implement the upcoming setOptOut command; i.e., whenever the new privacy option is changed, this PR will set or unset the opt-out cookie using window._tkq.push( [ 'setOptOut', true|false ] )

  • Make Jetpack_Tracks_Client in PHP aware of this cookie too. If the opt-out cookie exists, do not record server-side events either.

How to test:

Blocker

This will not actually set the cookie, it merely logs what would occur if setOptOut existed. Therefore, this PR cannot be tested any further at this time. The work in gh-analytics-38 (setOptOut command) needs to exist in w.js as well, which I am investigating.

Unresolved Questions

  • Will the final option key for opt-outs in Jetpack remain jetpack_event_tracking? This question has been raised in: p7kMJG-4rC-p2 and is pending further discussion. Yes
  • Should setOptOut disable Google Analytics in addition to Tracks? No

@jaswrks jaswrks added this to the GDPR Compliant milestone Mar 19, 2018
@jaswrks jaswrks requested a review from a team as a code owner March 19, 2018 22:37
@jaswrks jaswrks mentioned this pull request Mar 20, 2018
Merged
@jeherve jeherve added the [Status] Needs Review This PR is ready for review. label Mar 20, 2018
@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Mar 26, 2018

@jaswrks this PR is not passing some JS tests. Mostly due to an attempt to access window from the state-handling code (an action). So, when the tests run on the console, window is not defined there.

zinigor
zinigor suggested changes Mar 26, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, looks good, except I have one question.

_inc/client/state/settings/actions.js Outdated
error: error,
updatedOption
} );
maybeSetAnalyticsOptOut( updatedOption );
Copy link
Copy Markdown
Contributor

@zinigor zinigor Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this getting set in the catch block instead of in the success block?

@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 26, 2018

@zinigor Thank you for catching this!

Makes sense to me, looks good, except I have one question.

Fixed in da1219f

oskosk
oskosk reviewed Mar 26, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaswrks moving the tracking behaviour to somewhere on the react side of things (instead of on the redux action creator) would be a preferred solution for the problem I mentioned earlier.

@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 26, 2018 •
edited
Loading

@oskosk writes...

@jaswrks this PR is not passing some JS tests. Mostly due to an attempt to access window from the state-handling code (an action). So, when the tests run on the console, window is not defined there.

Thank you. I see what you mean. I reviewed the Travis output and I see this is a result of me importing /_inc/client/lib/analytics, which depends on window.

Does anyone have a suggestion about how to best approach a fix for this and get tests to pass? Am I doing something really unexpected by importing the analytics code in this PR? Or is this missing window var a separate area of concern as it relates to the test environment?

@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 26, 2018

@jaswrks moving the tracking behaviour to somewhere on the react side of things (instead of on the redux action creator) would be a preferred solution for the problem I mentioned earlier.

Copy that :-) Ty

@jaswrks jaswrks force-pushed the add/set-opt-out-call-check branch from da1219f to bc3d5c4 Compare March 26, 2018 21:23
@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 26, 2018 •
edited
Loading

@oskosk Thank you. I altered my approach in this PR in the last commit: bc3d5c4

@jaswrks jaswrks mentioned this pull request Mar 27, 2018
Merged
@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 27, 2018

A potentially better alternative now exists in #9156

@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 27, 2018

Closing, superseded by #9156

@jaswrks jaswrks closed this Mar 27, 2018
@jeherve jeherve deleted the add/set-opt-out-call-check branch March 28, 2018 11:31
@kraftbj kraftbj removed the [Status] Needs Review This PR is ready for review. label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@oskosk oskosk oskosk left review comments

@zinigor zinigor zinigor requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

[Feature] Tracks [Focus] GDPR [Pri] BLOCKER Touches WP.com Files

Projects

None yet

Milestone

GDPR Compliant

Development

Successfully merging this pull request may close these issues.

7 participants

@jaswrks @oskosk @zinigor @kraftbj @jeherve @dereksmart @matticbot