-
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
Adbookpsp Bid Adapter: add new bid adapter #6712
Conversation
Please fix the lint errors in this PR, so that the Circle CI tests will run. Double quotes need to be replaced with single quotes. |
I've been able to reproduce the test failures locally, will now try to fix them. I'll mark this PR ready to for review once I'm done to not bother you unnecessarily. |
01074e4
to
5ad491a
Compare
d6fb751
to
5e14968
Compare
modules/adbookpspBidAdapter.md
Outdated
params: { | ||
placementId: 'example-placement-id', | ||
video: { | ||
protocols: [2, 3], |
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.
Please move this to the ad unit
5e14968
to
22460fd
Compare
138208c
to
6a8cb8a
Compare
I've updated this branch with the latest upstream changes but still some tests are not passing. Do you have any advice? |
Tests have passed |
|
||
for (const id of impIds) { | ||
targetingMap[id] = { | ||
hb_liid_adbookpsp: values[id].lineItemIds.join(TARGETING_VALUE_SEPARATOR), |
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 is happening here
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.
In short, we use custom targeting keys to pass additional information to GAM which is then used by our custom creative script to select an ad independently from Prebid.
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.
Do you want to check said script into integration examples?
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 meant the script rendered by GAM, I'm not sure we're interested in adding an example right now. Is this something you require?
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.
No, but please add prominent disclosure in your doc repo pull and your .md file that custom gam creatives are required and that this bidder only works in its own gam order and that the prebid universal creative does not work etc. Please be very explicit and redundant.
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've expanded the note about the adapter requirements in the doc pull request and brought the note to the readme file here.
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.
a few minor requests
@patmmccann Are there any changes that you're still waiting on or can this be approved? |
* Added adbookpspBidAdapter * Fixed video parameter placement in readme * Simplified video parameters handling * Improved video dimensions handling * Updated readme with note about custom GAM setup
Type of change
Description of change
Added adapter that connects to the AdbookPSP demand sources