-
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c780e00
Master merge issues
mikael-lundin ea14b3a
Adnuntius Bid Adapter: Added tests for gdpr and segments
mikael-lundin 998caa8
Merge with upstream master
mikael-lundin a788d37
Merge remote-tracking branch 'upstream/master'
mikael-lundin b7910c6
Moved segments to read from ortb2 instead of a custom value.
mikael-lundin 24c4230
Merge remote-tracking branch 'upstream/master'
mikael-lundin 21a1cb1
Changed bidder to read segments from ortb2.
mikael-lundin ee72517
fixing lgtm alert
mikael-lundin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I would suggest either setting a filter for the name so you only match on those segments like:
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:
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:
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.