-
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
Added 'ceh' config property in Criteo bid adapter #3969
Conversation
sync from head fork
Added 'ceh' config property and mapped it to CDB request
Hi @leonardlabat, Instead of passing this new value through the Thanks, |
Hi @jsnellbaker, Yes that was our first thought. But that would require the publisher to repeat the hashed email on each ad unit. However, this is some kind of global parameter about the user (a bit like gdpr), something that we'll send at the root of the http request to our bidder, and not along each bid request. Do you see what I mean ? Regards, |
Hi @leonardlabat There is discussion about establishing a defined way to pass bidder unique data through the config (#3687) which might be in line with what you wanted to setup here. But until that process is decided and implemented, would you be able to pivot the changes here? |
Hi @jsnellbaker , |
ping @jsnellbaker |
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.
Through further internal discussion on the setConfig topic, it was decided that it would be okay for you to proceed with this implementation.
I have ran some tests with the updates in the adapter and found they appear to be working as expected. There was one suggestion that I would like you to make in the unit tests before we merge this PR. Can you please take a look at the in-line comment below when you have the chance?
Thanks.
}, | ||
}, | ||
]; | ||
config.setConfig({ |
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.
Can you either add a after
, afterEach
or beforeEach
function in this describe
section to run the config.resetConfig();
statement? This will ensure that some other tests don't have a setConfig
that includes your bidder property when they're all running together.
…e an impact on any other test
Hi @jsnellbaker, Thanks for the review. I've just pushed a commit to add this call to config.resetConfig. Leonard |
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 update; LGTM
* Added 'ceh' config property and mapped it to CDB request Added 'ceh' config property and mapped it to CDB request * Added config.resetConfig() to ensure that call to setConfig won't have an impact on any other test * Missed the ';'
* Added 'ceh' config property and mapped it to CDB request Added 'ceh' config property and mapped it to CDB request * Added config.resetConfig() to ensure that call to setConfig won't have an impact on any other test * Missed the ';'
* Added 'ceh' config property and mapped it to CDB request Added 'ceh' config property and mapped it to CDB request * Added config.resetConfig() to ensure that call to setConfig won't have an impact on any other test * Missed the ';'
Type of change
Description of change
We'd like to add the ability for publishers to give to our adapter the hashed email of the current user.
Syntax for this will be the following.