-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Unruly Bid Adapter: consolidated adapter with RhythmOne #7073
Unruly Bid Adapter: consolidated adapter with RhythmOne #7073
Conversation
* Finished the request and bid validation side * removed the user sync since now the backend is responsible for that.
* Finished handling the response now the adapter supports banner/instream/outstream
* fixed the unit tests for the outstream response part (still missing new unit tests for instream and banner)
* added new unit tests for instream and banner
* changed the endpoint to the new endpoint * unitTests indentations
* Added missing semicolons
* fixes after code review
* added the ability to overwrite the end point for testing reasons. * removed the floorInfo object and changed it to floor * added more unit tests * added unmissable configuration to md file
* added check in instream if the response has vastUrl or vastXml + unit test
* Updated the md file to product request
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 @opeledtremor, looks good overall, just a couple things you can take a look at.
modules/unrulyBidAdapter.js
Outdated
}, | ||
|
||
buildRequests: function (validBidRequests, bidderRequest) { | ||
let endPoint = '//targeting.unrulymedia.com/unruly_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.
Please hardcode to https://
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.
Pushed the requested change (adding https to the endpoint).
options | ||
Object.keys(requestBySiteId).forEach((key) => { | ||
let data = { | ||
bidderRequest: Object.assign({}, {bids: requestBySiteId[key], invalidBidsCount, ...bidderRequestData}) |
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 am all for using the object spread syntax but just letting you know it won't work in IE11.
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.
Looks like it does get transpiled to es5, so I guess, it's fine.
* added https for the endpoint as requested from prebid reviewer.
* Unruly and RhythmOne consolidated adapter - first commit * Finished the request and bid validation side * removed the user sync since now the backend is responsible for that. * Unruly and RhythmOne consolidated adapter - second commit * Finished handling the response now the adapter supports banner/instream/outstream * Unruly and RhythmOne consolidated adapter - third commit * fixed the unit tests for the outstream response part (still missing new unit tests for instream and banner) * Unruly and RhythmOne consolidated adapter - * added new unit tests for instream and banner * Unruly and RhythmOne consolidated adapter * changed the endpoint to the new endpoint * unitTests indentations * Unruly and RhythmOne consolidated adapter * Added missing semicolons * Unruly and RhythmOne consolidated adapter * fixes after code review * Unruly and RhythmOne consolidated adapter * added the ability to overwrite the end point for testing reasons. * removed the floorInfo object and changed it to floor * added more unit tests * added unmissable configuration to md file * Unruly and RhythmOne consolidated adapter * added check in instream if the response has vastUrl or vastXml + unit test * Unruly and RhythmOne consolidated adapter * Updated the md file to product request * Unruly and RhythmOne consolidated adapter - * added https for the endpoint as requested from prebid reviewer. Co-authored-by: Tal Lavon <tlavon@tremorvideo.com>
Type of change
Description of change
This is a consolidated adapter between the Unruly adapter and the RhythmOne Adapter.
now the unruly adapter support instream, outstream and banner.
list of changes:
contact email of the adapter’s maintainer: opeled@tremorvideo.com
official adapter submission
A link to a PR on the docs repo: Unruly r1 consolidated adapter prebid.github.io#3054