-
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
adnuntias Bid Adapter: Added GDPR support and segment passing #6796
Conversation
@mikael-lundin can you please add some test coverage to this pr please 🙏 |
I will since you asked so friendly 😄 |
Tests are now added. 🥇 <- I give this to myself 😃 |
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.
@mikael-lundin a few comments here
modules/adnuntiusBidAdapter.js
Outdated
const networks = {}; | ||
const bidRequests = {}; | ||
const requests = []; | ||
const segments = config.getConfig('segments'); | ||
utils.logMessage('CONF', segments) |
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 line should be removed entirely, as this does not seem like a necessary log to me. I believe that we would rather not inflate code with messaging that is not pertinent as this is neither a user warning/error or really clear in label as to what is being logged - also this can be achieved in the console via a pbjs.getConfig('segments').
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.
yeah, that was just a blunder from me, will remove that.
modules/adnuntiusBidAdapter.js
Outdated
const networks = {}; | ||
const bidRequests = {}; | ||
const requests = []; | ||
const segments = config.getConfig('segments'); |
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 not so much of a change request, but more curious about this config setting. I am not aware of segments being set under the config at the top level, so I am curious if this is something specific to your adapter and if so, should it be in your docs? I also want to make sure that this is not meant to be first party segment data that is defined to be passed via ortb2.user.data and ortb2.site.content.data respectively
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 was strugling to find where to put it, and yes it's just meant to be for our adapter only, but if ortb2.user.data is the place to put it I will go for it. Should I also prefix our segments (like adn-segments
) or are there any other rules for how to structure it?
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 can offer suggestions, but Im not sure where you mean to put the prefix. Maybe reviewing this will offer a better understanding of first party segment data: https://docs.prebid.org/features/firstPartyData.html. You can see under ortb2.user.data, this is an array of objects that "should" include at the very least name and segment (with segment as an array of objects), as well as ext property that can contain other data. Let me know if this helps in any way. Also, the pubs will have the option to utilize setBidderConfig to set config data meant only for specific adapters, meaning that only your adapter will receive those values if set using your bidder.
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 should now be solved and the documentation is filed under this pull request:
prebid/prebid.github.io#2975
modules/adnuntiusBidAdapter.js
Outdated
let segments; | ||
if (userData) { | ||
userData.forEach(userdat => { | ||
segments = (userdat.segment) ? userdat.segment.map(segment => segment.id) : undefined |
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.
@mikael-lundin thanks for updating. Do you only want to pass those segments defined with the name 'adnuntius'? Currently there is no filter setup for this here, meaning that if any segment data defined after this type will overwrite the data I believe you are intended to leverage. i.e. if the below is defined in the config, you will only receive the segment 3 and not 1 and 2.
ortb2: {
user: {
data: [{
name: "adnuntius",
segment: [ { id: "1" }, { id: "2" } ]
},{
name: "other",
segment: [ { id: "3" }]
}]
}
}
I would suggest either setting a filter for the name so you only match on those segments like:
userData.filter(data => data.name === 'adnuntius').forEach(userdat => {...});
Or if you want to collect all segment data, I would suggest something along the lines of this seeing as you check for segment length and then you can also remove the first two parts of the condition since segments will always be an array:
let segments = [];
if (userData) {
userData.forEach(userdat => {
if (userdat.segment) segments.push(...userdat.segment.map(segment => segment.id));
});
}
A bunch of ways you can do this, but I wanted to make sure that this is corrected so you receive the data you are expecting. Also, I may suggest adding a filter prior to the segment map function to ensure the index of the array contains a property called id - the reason being is that this data structure is fairly new and if a pub were to create it using an array like ['1234','5678'] instead of [{id: '1234'}, {id:'5678'}] you will receive empty comma separated values. Could be something as simple as the following I think would work:
userdat.segment.filter(seg => seg.id).map(...);
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 that, you kind of saved my ass a bit! :D I've added some changes to it so that it works more as you say, and for now we will just pass all the segments that we see to the adserver.
This pull request introduces 1 alert when merging 21a1cb1 into 8183e85 - view on LGTM.com new alerts:
|
@mikael-lundin can you fix this LGTM alert? Since the getSegmentsFromOrtb always returns an array there is no need for those two checks - as the array will always exist and be of type array. I think you should be fine simply checking the array length in your condition, which I think should remove this LGTM alert |
I think I've done what you asked for :D Saw no alerts on my end while testing. |
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 making these updates! LGTM
…#6796) * Master merge issues * Adnuntius Bid Adapter: Added tests for gdpr and segments * Moved segments to read from ortb2 instead of a custom value. * Changed bidder to read segments from ortb2. * fixing lgtm alert
Type of change
Description of change
Adding GDPR support for the bid adapter and allowing customers to pass segments through the bidder config. If a user has accepted our GLV ID we will use the segments passed on to the adserver. If not, they will not be matched against targeted line items by the adserver.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Adnuntius Bid Adapter - Updated documentation for passing on segments. prebid.github.io#2975
Other information