-
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
Adding AdButler Adapter #707
Conversation
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.
var spyLoadScript; | ||
|
||
beforeEach(function () { | ||
spyLoadScript = sinon.spy(adLoader, 'loadScript'); |
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 use .stub
here rather than .spy
. A spy
on adLoader.loadScript
can potentially result in script errors and cause other tests to fail. See this commit for context
CPM,minCPM,maxCPM; | ||
|
||
//Ensure that response ad matches one of the placement sizes. | ||
utils._each(callbackData[callbackID].sizes,function(size){ |
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.
bidResponse.bidderCode = 'adbutler'; | ||
} | ||
|
||
bidmanager.addBidResponse(callbackData[callbackID].placementCode, bidResponse); |
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.
Accessing placementCode
when callbackData[callbackID]
is undefined
could also cause TypeError
s. These particularly pop up when Karma is left in debug mode, which you can check with gulp serve --watch
and then opening the developer console
Only attempt to build a bid response if we have the information of which bid to respond to.
Now stubbing adLoader instead of spying. Additional changes to ensure all tests still passed.
@matthewlane Thanks for the feedback! I've committed changes to clear up those issues. I provided you with invalid test parameters by mistake, and that's why you were seeing the SyntaxError on the adapter test. I've updated the parameters, and that should be fixed up now, as well. Please let me know if there is anything else you need from me. |
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.
@dharton Changes look good and I'm able to validate bids with the update parameters. Left a few more comments for your review, when those are addressed I'll review again as soon as possible and we should be good to merge
isCorrectSize = false, | ||
isCorrectCPM = true, | ||
CPM,minCPM,maxCPM, | ||
bidObj = utils.getBidRequest(callbackData[callbackID].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.
This can cause another TypeError
which occasionally fails unit tests. Checking callbackData[callbackID]
before accessing bidId
will fix this. Something like bidObj = callbackData[callbackID] ? utils.getBidRequest(callbackData[callbackID].bidId) : null;
will work as a one-liner but feel free to do the check however you prefer
|
||
} else { | ||
|
||
bidResponse = bidfactory.createBid(2); |
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.
Pass bidObj
as second param to createBid
} | ||
} else { | ||
|
||
bidResponse = bidfactory.createBid(2); |
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.
Pass bidObj
as second param to createBid
|
||
if(isCorrectCPM && isCorrectSize){ | ||
|
||
bidResponse = bidfactory.createBid(1); |
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.
createBid
can take bidObj
as a second parameter here to set the response ID to the request ID. See #509 for background info
Prevent AdButler TypeErrors and pass bid request object into the bid response.
@matthewlane I've updated the code to reflect your comments. Thanks for the heads up about passing the bid request object into createBid(). |
@dharton Thanks, merged. Can you also please create a PR to add the AdButler bidder params to the docs repo? If you create a file in the bidders directory (can copy an existing file and modify the params for AdButler) that'll get added to the Prebid bidders page. Thanks |
Type of change
Description of change
This change will add the AdButler bidder adapter.
[email protected]