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

Implement setOptOut cookie command #9085

Closed
wants to merge 3 commits into from
Closed

Conversation

jaswrks
Copy link
Contributor

@jaswrks jaswrks commented Mar 19, 2018

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
@jeherve jeherve added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Mar 20, 2018
@oskosk
Copy link
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.

Copy link
Member

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

@@ -78,6 +79,7 @@ export const updateSetting = ( updatedOption ) => {
error: error,
updatedOption
} );
maybeSetAnalyticsOptOut( updatedOption );
Copy link
Member

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

Copy link
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
Contributor Author

jaswrks commented Mar 26, 2018

@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
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
Contributor Author

jaswrks commented Mar 26, 2018

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

@jaswrks
Copy link
Contributor Author

jaswrks commented Mar 27, 2018

A potentially better alternative now exists in #9156

@jaswrks
Copy link
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 To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants