Conversation
also changed surce of end-point to prebid-client from prebid-server
PubMatic one
removed method _processFloor and _processPmZoneId added const for undefined
added a small function to find domain from given url
…changed to false, test cases updated
mkendall07
requested review from
jsnellbaker
and removed request for
jaiminpanchal27
|
@PubMatic-OpenWrap |
jsnellbaker
left a comment
jsnellbaker
left a comment
There was a problem hiding this comment.
@PubMatic-OpenWrap Thanks for submitting this adapter. Overall the code looks good. There are some aspects/issues that I wanted to mention so we can be review/clarify them together.
I summarized them below:
- There are some areas with either extra comments or old code that was commented. Can these be reviewed/cleaned up if possible?
- It seems like to test the adapter, the userSync needs to be configured. I know this was highlight in the docs PR, but can we add this note in the md file in this PR as well so that the requirement is stated in a consistent manner?
- When I was testing the code with the test param data on the hello_world.html page, I saw the request to your system was formatted and executed but I wasn't getting any type of response back. The POST request indicated it had a 204 success code. As a result, I was only ever seeing a house ad return. Could you take a look on your end in regards to this request/response?
Thanks.
modules/pubmaticBidAdapter.js
Outdated
| // istanbul ignore else | ||
| if (CUSTOM_PARAMS.hasOwnProperty(key)) { | ||
| value = params[key]; | ||
| // istanbul ignore else |
There was a problem hiding this comment.
Are these types of comments still needed? I assume they were part of the development process and wanted to confirm.
modules/pubmaticBidAdapter.js
Outdated
| return true; | ||
| } | ||
| return false; | ||
| // return !!(bid && bid.params && utils.isStr(bid.params.publisherId) && utils.isStr(bid.params.adSlot)); |
There was a problem hiding this comment.
Can we remove this comment if the earlier code is the planned/definite approach to perform the validation check?
|
We have removed unwanted comments. |
|
@PubMatic-OpenWrap Thanks for making the updates. I used the updated test credentials and I saw the test ad deliver fine. LGTM |
| return conf; | ||
| } | ||
|
|
||
| function _createOrtbTemplate(conf) { |
There was a problem hiding this comment.
nice! Would be great if we had a community ORTB adapter at some point...
* 'master' of https://github.com/prebid/Prebid.js: Prebid 1.2.0 Release Use polyfilled includes method (prebid#2061) RockYou Adapter: Added RockYou Adapter supporting Prebid 1.0 (prebid#1977) Optimera Adapter for 1.0. (prebid#1961) Use cross-browser integer check (prebid#2058) Fix skipped test (prebid#2059) Support multiple media formats within a single ad unit (prebid#1991) pre1api module that allows use of deprecated pre1.0 API in Prebid 1.0 (prebid#1976) Colossus SSP header bidding adapter 1.0.0 (prebid#2029) InSkin Bidder Adapter (prebid#2016) Update adapter to prebid v1.0 (prebid#1908) PubMatic 1.0 adapter (prebid#2011)
8 participants
Type of change
Description of change
Be sure to test the integration with your adserver using the Hello World sample page.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Other information