Skip to content

Update/centralize user privacy + cookie (PR combo)#9156

Merged
mattwiebe merged 4 commits intomasterfrom
update/centralize-user-privacy-cookie
Mar 27, 2018
Merged

Update/centralize user privacy + cookie (PR combo)#9156
mattwiebe merged 4 commits intomasterfrom
update/centralize-user-privacy-cookie

Conversation

@jaswrks
Copy link
Copy Markdown
Contributor

@jaswrks jaswrks commented Mar 27, 2018

Depends on #9142 and #9154


This PR is a fork of (and alternative to) #9155 (props @mattwiebe) that brings it together with the work I did previously in #9085 to implement window._tkq.push( [ 'setOptOut', true|false ] )

This PR adds the following to #9155

  • Implement the 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 the setOptOut cookie too. If the opt-out cookie exists, do not record server-side events either.

How to test:


Some Tests Failing at Travis

It's to be expected at this time, because this depends on #9142 and #9154

@jaswrks jaswrks added [Status] Needs Review This PR is ready for review. [Focus] GDPR labels Mar 27, 2018
@jaswrks jaswrks added this to the GDPR Compliant milestone Mar 27, 2018
@jaswrks jaswrks requested a review from a team as a code owner March 27, 2018 04:29
@jaswrks jaswrks changed the title Update/centralize user privacy cookie Update/centralize user privacy cookie (PR combo) Mar 27, 2018
@jaswrks jaswrks changed the title Update/centralize user privacy cookie (PR combo) Update/centralize user privacy + cookie (PR combo) Mar 27, 2018
@jaswrks jaswrks requested a review from mattwiebe March 27, 2018 04:55
@dereksmart
Copy link
Copy Markdown
Contributor

@jaswrks can you rebase this onto master to get the changes merged in #9142 and #9154?

@dereksmart
Copy link
Copy Markdown
Contributor

This PR is a fork of (and alternative to) #9155

Can we just close that one then?

mattwiebe and others added 2 commits March 27, 2018 10:34
@mattwiebe mattwiebe force-pushed the update/centralize-user-privacy-cookie branch from 84ad8be to 4bc7448 Compare March 27, 2018 15:37
Maintain the `this.togglePrivacy` function for toggling `onChange` rather than an inline function

Use `this.props.isUpdating()` to disable the checkbox during an update.
@mattwiebe mattwiebe requested review from dereksmart and zinigor March 27, 2018 15:50
@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 27, 2018
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

Works as expected!

toggling={ isSavingAnyOption( 'jetpack_event_tracking' ) }
toggleModule={ this.togglePrivacy }>
checked={ ! this.props.tracking.tracks_opt_out }
disabled={ this.props.isFetching || this.props.isUpdating() }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The isUpdating prop should be a boolean value, not a function. However, it's currently a function because there is a conflict with the isUpdating prop in ModuleSettingsForm. I'm taking a look at a fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 89408c6

- Reference tracking settings in full form; e.g., ... trackingSettings.
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 27, 2018
@dereksmart dereksmart added the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 27, 2018
@jaswrks
Copy link
Copy Markdown
Contributor Author

jaswrks commented Mar 27, 2018

Can we just close that one then?

Thanks for the review. Yes, 9155 has been closed.

@mattwiebe mattwiebe merged commit 217b8e1 into master Mar 27, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 27, 2018
@mattwiebe mattwiebe deleted the update/centralize-user-privacy-cookie branch March 27, 2018 20:34
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.

4 participants