-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adds support for additional consent #5600
Conversation
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.
thanks for the updates, Patrick
docs PR prebid/prebid.github.io#2219 |
One example of a cmp supporting this already is https://support.cookiebot.com/hc/en-us/articles/360007652694-Cookiebot-and-the-IAB-Consent-Framework |
From Prebid.js slack, for reviewer: https://prebid.slack.com/archives/C7TLBVAH3/p1597348755293600?thread_ts=1597345850.288600&cid=C7TLBVAH3 |
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.
Looks good, minor nitpick,
why use utils.isString in one module, but regular typeof check in the other?
Not a big deal of course haha!
I think I was probably just matching line 639
…On Mon, Aug 17, 2020, 1:31 PM Robert Ray Martinez III < ***@***.***> wrote:
***@***.**** approved this pull request.
Looks good, minor nitpick,
why use utils.isString in one module, but regular typeof check in the
other?
Not a big deal of course haha!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5600 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM25Z6PN5WOYSUDJUW7OZLSBFSQ7ANCNFSM4P5DFJPQ>
.
|
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.
I think this all looks fine; I did have a question though. See below.
@@ -641,6 +641,9 @@ const OPEN_RTB_PROTOCOL = { | |||
} | |||
utils.deepSetValue(request, 'regs.ext.gdpr', gdprApplies); | |||
utils.deepSetValue(request, 'user.ext.consent', firstBidRequest.gdprConsent.consentString); | |||
if (firstBidRequest.gdprConsent.addtlConsent && typeof firstBidRequest.gdprConsent.addtlConsent === 'string') { | |||
utils.deepSetValue(request, 'user.ext.ConsentedProvidersSettings.consented_providers', firstBidRequest.gdprConsent.addtlConsent); |
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.
Just to confirm - are we sure that this field is supported by PBS?
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.
that was the field @bretg proposed on #5389 (comment) bullet point 2
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.
Thanks for highlighting that spot @patmmccann
LGTM
* add addtlConsent consent to consent object * add unit test for additional consent * Update consentManagement.js * Update index.js * Update prebidServerBidAdapter_spec.js * condense else if
This reverts commit e2dcb8b.
Type of change
Description of change
Solves for #5389 on js following the extension to the TCData object proposed by Google here https://support.google.com/admanager/answer/9681920?hl=en