-
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
Update to XHB adapter #2536
Update to XHB adapter #2536
Conversation
@daniel-hoffmann Can you take a look at the Travis errors and resolve them when you get the chance? I'll start to look-over the adapter changes in the meantime. Also, would you be able to open a docs PR in the docs repo and make an update in your bidders page to add the following variable?
This can go directly below the variable for your 1.0 compliance. This will allow your adapter to appear in a table that shows GDPR compliant adapters. Thanks. |
@jsnellbaker Both things have been done - let me know if you need anything else to be 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.
Hi @daniel-hoffmann Sorry for the small delay, I have some points to review - see in-line below.
Additionally we need to have a unit-test file included for a new adapter submissions. The following page provides some direction on setting up the unit test file: http://prebid.org/dev-docs/bidder-adaptor.html#adding-unit-tests
Can you please put this together and add it to the PR?
function newBid(serverBid, rtbBid, bidderRequest) { | ||
const bid = { | ||
requestId: serverBid.uuid, | ||
cpm: 0.00, |
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.
To confirm should be 0 all the time? Shouldn't it pull in the CPM from the returned bid?
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 what Xaxis wants to do since they have pre-arranged deals.
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.
As per Matt's comment, yes, that is exactly what we need and together with the hardcoded dealId kinda the whole reason we have our own adapter. ;)
requestId: serverBid.uuid, | ||
cpm: 0.00, | ||
creativeId: rtbBid.creative_id, | ||
dealId: 99999999, |
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.
The dealId
field is an optional field and doesn't have to be explicitly set. If you're not planning on using it, it could be removed instead of hard-coding to a set value. If you are using that set value for a particular use-case, please let me know.
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 necessary for our setup.
modules/xhbBidAdapter.md
Outdated
required: true | ||
}, | ||
brand: { | ||
required: true |
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 the test placement - the brand
is not included which is preventing the bid from being accepted by the native param checks. If you want the brand
to be a required field in general for your publishers, could you update the test ad unit to include this field? If the field isn't strictly required, could you update this .md file?
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.
Actually, I just copied this from the AN adapter, I assume that one has the same issue? I'll look into it and will change it in ours. :)
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, we don't need that to be required.
Can I just set it to false in the md file?
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.
That's fine, or you could just remove the brand
object entirely.
@daniel-hoffmann |
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.
LGTM
* xhb adapter added - use AppNexus test ad unit * adjusted adapter to set responseCPM to 0 and add in dealId * implemented suggested changes * implemented suggested changes * ran jscs fixer for xhb.js and added it to adapters.json * Xhb adapter: adding alias support * minor changes as per request * re-added default bidder settings * Bugfix: getBidRequest was missing utils * minor fix to _defaultBidderSettings * added size to defaultBidderSettings * XHB adapter updated to work with prebid 1.0 * XHB adapter updated to work with prebid 1.0 * updated xhb adapter to incorporate latest AN adapter changes * further fixes for 1.0 adapter * updated Adapter to latest version * indentation fixed * unit tests for xhb Adapter * xhbBidAdapter: removed brand from md file
* xhb adapter added - use AppNexus test ad unit * adjusted adapter to set responseCPM to 0 and add in dealId * implemented suggested changes * implemented suggested changes * ran jscs fixer for xhb.js and added it to adapters.json * Xhb adapter: adding alias support * minor changes as per request * re-added default bidder settings * Bugfix: getBidRequest was missing utils * minor fix to _defaultBidderSettings * added size to defaultBidderSettings * XHB adapter updated to work with prebid 1.0 * XHB adapter updated to work with prebid 1.0 * updated xhb adapter to incorporate latest AN adapter changes * further fixes for 1.0 adapter * updated Adapter to latest version * indentation fixed * unit tests for xhb Adapter * xhbBidAdapter: removed brand from md file
* xhb adapter added - use AppNexus test ad unit * adjusted adapter to set responseCPM to 0 and add in dealId * implemented suggested changes * implemented suggested changes * ran jscs fixer for xhb.js and added it to adapters.json * Xhb adapter: adding alias support * minor changes as per request * re-added default bidder settings * Bugfix: getBidRequest was missing utils * minor fix to _defaultBidderSettings * added size to defaultBidderSettings * XHB adapter updated to work with prebid 1.0 * XHB adapter updated to work with prebid 1.0 * updated xhb adapter to incorporate latest AN adapter changes * further fixes for 1.0 adapter * updated Adapter to latest version * indentation fixed * unit tests for xhb Adapter * xhbBidAdapter: removed brand from md file
* xhb adapter added - use AppNexus test ad unit * adjusted adapter to set responseCPM to 0 and add in dealId * implemented suggested changes * implemented suggested changes * ran jscs fixer for xhb.js and added it to adapters.json * Xhb adapter: adding alias support * minor changes as per request * re-added default bidder settings * Bugfix: getBidRequest was missing utils * minor fix to _defaultBidderSettings * added size to defaultBidderSettings * XHB adapter updated to work with prebid 1.0 * XHB adapter updated to work with prebid 1.0 * updated xhb adapter to incorporate latest AN adapter changes * further fixes for 1.0 adapter * updated Adapter to latest version * indentation fixed * unit tests for xhb Adapter * xhbBidAdapter: removed brand from md file
* xhb adapter added - use AppNexus test ad unit * adjusted adapter to set responseCPM to 0 and add in dealId * implemented suggested changes * implemented suggested changes * ran jscs fixer for xhb.js and added it to adapters.json * Xhb adapter: adding alias support * minor changes as per request * re-added default bidder settings * Bugfix: getBidRequest was missing utils * minor fix to _defaultBidderSettings * added size to defaultBidderSettings * XHB adapter updated to work with prebid 1.0 * XHB adapter updated to work with prebid 1.0 * updated xhb adapter to incorporate latest AN adapter changes * further fixes for 1.0 adapter * updated Adapter to latest version * indentation fixed * unit tests for xhb Adapter * xhbBidAdapter: removed brand from md file
* xhb adapter added - use AppNexus test ad unit * adjusted adapter to set responseCPM to 0 and add in dealId * implemented suggested changes * implemented suggested changes * ran jscs fixer for xhb.js and added it to adapters.json * Xhb adapter: adding alias support * minor changes as per request * re-added default bidder settings * Bugfix: getBidRequest was missing utils * minor fix to _defaultBidderSettings * added size to defaultBidderSettings * XHB adapter updated to work with prebid 1.0 * XHB adapter updated to work with prebid 1.0 * updated xhb adapter to incorporate latest AN adapter changes * further fixes for 1.0 adapter * updated Adapter to latest version * indentation fixed * unit tests for xhb Adapter * xhbBidAdapter: removed brand from md file
Update to XHB adapter for GDPR and v1