-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add consent opt out #1063
Add consent opt out #1063
Conversation
🦋 Changeset detectedLatest commit: f294612 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8aa69b9
to
989d37e
Compare
9edef47
to
f505ae7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally think this looks good! Comments are relatively minor.
packages/consent/consent-tools-integration-tests/public/consent-tools-vanilla.html
Show resolved
Hide resolved
packages/consent/consent-tools/src/domain/analytics/analytics-service.ts
Show resolved
Hide resolved
export enum OtConsentModel { | ||
optIn = 'opt-in', | ||
optOut = 'opt-out', | ||
implicit = 'implied consent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is implicit
needed now? Doesn't look like it's exposed in OneTrustSettings
and is currently an alias for optIn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that since we only have unique behavior for opt-in and opt-out, I felt like it would be good to leave it out of the public API, but document that it exists in the enum, since this enum is meant to be an Internal DTO that maps to the actual OT API. I don't feel super strongly either way! There are a few other OneTrust consent models that I left out, since everything but implicit defaults to opt-out. coerceConsentModel
is where that logic lives.
* - opt-in (default) - load segment and all destinations without waiting for explicit consent. | ||
* - opt-out (strict/GDPR) - wait for explicit consent before loading segment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these flipped? opt-in
is the one where we wait for you to opt-in before loading, and opt-out is where we load until you explicitly opt out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, yep, the README is correct but these comments are flipped. Good catch.
shouldLoadSegment?: ( | ||
ctx: LoadContext | ||
) => Categories | void | Promise<Categories | void> | ||
shouldLoadSegment?: (ctx: LoadContext) => void | Promise<void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a breaking change - are we doing a major version bump (totally cool with it if we are!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, was going to do a major bump!
'on' in analytics && | ||
'addSourceMiddleware' in analytics | ||
) { | ||
if ('load' in analytics && 'addSourceMiddleware' in analytics) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for addDestinationMiddleware
now too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, yeah -- you got me, this typeguard is incomplete (was being a bit lazy here). 😄
packages/consent/consent-tools/src/domain/blocking-middleware.ts
Outdated
Show resolved
Hide resolved
7d5117f
to
5aeeb11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved! Only one outstanding comment (missing closing HTML tag in a file)
e163a7f
to
18e78cc
Compare
Add support for opt-out consent.
Note: to override the default behavior for OneTrust, the following setting(s) have been added:
yarn changeset
. Read about changesets here).