-
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
Concert Bid Adapter: adding referer (Resubmit of Pr #8580) #8676
Conversation
@andrew-a-dev resubmitted this pr under my account to hopefully get this merged in this week. |
modules/concertBidAdapter.js
Outdated
* `consent-string` npm module; so will have to rely on that | ||
* happening on the bid-server. */ | ||
return !(bidderRequest.uspConsent === 'string' && | ||
bidderRequest.uspConsent.toUpperCase().substring(0, 2) === '1YY') | ||
return !(bidderRequest.uspConsent === 'string' && bidderRequest.uspConsent.toUpperCase().substring(0, 2) === '1YY'); |
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.
why are you checking for the second character https://github.com/InteractiveAdvertisingBureau/USPrivacy/blob/f8d659f58665b4e797b99b59079fd787a386eaaa/CCPA/US%20Privacy%20String.md?plain=1#L123
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 is checking the first 3 characters of the string; we do not mark as consenting to PPID usage unless: (1) USPrivacy string version matches what we expect, and (2) both Notice to Opt-out
and Opt-out Sale
are marked as consenting. (And we don't care about LSPA.)
(Also, similarly to below: this line only shows in the diff because it looks like our linter took out the line-break; nothing consequential was changed.)
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.
If the string is '1YY' or '1NY', the user is opted 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.
If I understand what you're saying, we should consider the user "opted-out" if the CCPA string starts 1NY
? If so, I suspect we didn't realise that was a possible value (i.e. can they have opted-out if they weren't notified of the opt-out option?). I guess we assumed that a Y
in the third character implied a Y
in the second character. If that's not the case, I'll happily update the PR to ignore the second character then. (And also, your original comment makes more sense to me now; thank you for that.)
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.
some people have started pushing '1NY' if the GPC headers in the browser indicate opt out.
@@ -203,9 +204,8 @@ function hasOptedOutOfPersonalization() { | |||
* @param {BidderRequest} bidderRequest Object which contains any data consent signals | |||
*/ | |||
function consentAllowsPpid(bidderRequest) { | |||
/* NOTE: We cannot easily test GDPR consent, without the | |||
/* NOTE: We can't easily test GDPR consent, without the |
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.
what does this comment mean? why can't you look at the tcf consent string without this npm module?
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.
We can view the consent string, yes, but if parsing it is non-trivial enough that there exists an official, specialised, and sanctioned npm
module to do it, it does not seem appropriate that we would try to add that functionality ourselves to our bid adapter; especially since we do that exact step (using consent-string
) in our bid server.
(For the record, this change only exists in this PR because it was a NOP
commit, to try to activate the CircleCI on a fresh push.)
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.
we provide a utility function, eg
Prebid.js/modules/appnexusBidAdapter.js
Line 559 in 0be4560
if (!hasPurpose1Consent(bidderRequest?.gdprConsent)) { |
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.
fwiw, this is not a change request, just trying to be helpful
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 appreciate this, @patmmccann , thank you! (I don't think those functions existed when we first wrote our adapter, so it's good to know that so we can update it.) I've updated our use of consent strings, as per these comments, and will ask Chris if they can merge these changes to this branch (which I can't do). 🙌
Thank you, @ChrisHuie ! 🙌 |
@ChrisHuie Thanks again for creating this PR, would you be able to pull in from this commit? voxmedia@c48c1d4 ; this includes further changes to the consent-string handling, as per the feedback/suggestions above. Many thanks, again! 🙌 |
Sure can 👍 Will update shortly |
…bid#8676) * Concert Bid Adapter refererInfo update * fix linting * update with purpose1consent
…bid#8676) * Concert Bid Adapter refererInfo update * fix linting * update with purpose1consent
…bid#8676) * Concert Bid Adapter refererInfo update * fix linting * update with purpose1consent
#8580 CircleCI tests didn't kickoff so resubmitted pr under my account to keep this code moving.