-
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 adapter to prebid v1.0 #1908
Conversation
modules/smartadserverBidAdapter.js
Outdated
adloader.loadScript(url.format(adCall)); | ||
} | ||
// pull requested transaction ID from bidderRequest.bids[].transactionId | ||
if (!Array.isArray(validBidRequests)) { |
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 will always be an array, so this is unnecessary
modules/smartadserverBidAdapter.js
Outdated
siteid: bid.params.siteId, | ||
pageid: bid.params.pageId, | ||
formatid: bid.params.formatId, | ||
currencyCode: config.getConfig('currency'), |
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.
'currency'
is an object, do you mean 'currency.adServerCurrency'
?
modules/smartadserverBidAdapter.js
Outdated
*/ | ||
interpretResponse: function (serverResponse, bidRequest) { | ||
const bidResponses = []; | ||
if (serverResponse) { |
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.
serverResponse
is now split into two properties. serverResponse.body
and serverResponse.headers
. I think you're wanting to get all the stuff off serverResponse.body
here.
modules/smartadserverBidAdapter.js
Outdated
}, | ||
getUserSyncs: function (syncOptions) { | ||
// iframe || image | ||
return undefined; |
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.
If you're not using userSyncs, just don't include this function definition rather than returning undefined
.
modules/smartadserverBidAdapter.js
Outdated
var bid = validBidRequests[0]; | ||
const payload = { | ||
siteid: bid.params.siteId, | ||
pageid: bid.params.pageId, | ||
formatid: bid.params.formatId, | ||
currencyCode: config.getConfig('currency'), | ||
currencyCode: config.getConfig('currency').adServerCurrency, |
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.
You should use config.getConfig('currency.adServerCurency')
since the config module does safe deep access. The way you have it here if currency hasn't been set then it will throw "can't read adServerCurrency of undefined" error.
Hello, |
@Spacedragoon you have conflicts that need to be resolved |
Hello, |
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
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.
Are the test parameters in smartadserverBidAdapter.md
still valid? Currently getting a 200 OK
but empty response when using those for testing. Also one change requested below
modules/smartadserverBidAdapter.js
Outdated
if (response) { | ||
const bidResponse = { | ||
requestId: bidRequest.bidId, | ||
bidderCode: spec.code, |
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.
bidderCode
will be set automatically by bidderFactory
now, this line can be dropped
@Spacedragoon Updates look, but still getting an empty response with the updated test parameters. The way I'm testing is taking those params and putting them in the hello_world test page bids array, running Once bid responses are verified this should be good for merge. |
Alright, it didn't work because i assumed the code here would work just by filling the blanks. But the line It may be a good idea to review this part of the documentation |
Unfortunately the location the The documentation doesn't describe that process very well, I'll see if I can update it to clarify what that id is and how to get it. |
@Spacedragoon still getting no response data with the test parameters and updated code, with the setup described here, checking on this again so we can validate bid responses and get this merged |
Hello @matthewlane I tested it again here and it seems to work : you can see the response from our side, plus the bidResponses in pbjs. |
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.
@Spacedragoon Ok, thanks. One more required change then will merge
modules/smartadserverBidAdapter.js
Outdated
var response = serverResponse.body; | ||
if (response) { | ||
const bidResponse = { | ||
requestId: JSON.parse(bidRequest.data).bidId, |
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.
Any JSON.parse
calls should be wrapped in a try/catch
block
@matthewlane Alright, done. |
Thanks, this is merged. Please also submit a PR to update your bidder file in the docs repo, to add the line |
@matthewlane Thank you, the PR for the docs repo is now done here |
* '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)
* Update adapter to prebid v1.0 * corrected some things following pullrequest review * slight change to avoid potential error * added the maintainer email * Updated the tests to fit changes * Updated the doc, removed the bidderCode, added adUrl * fix adapter * Added try catch around JSON.parse
Type of change
Description of change
Version 1.0 of the Smart Bidder Adapter, every mandatory params and optional params are the same as previous version
Be sure to test the integration with your adserver using the Hello World sample page.
Other information