Skip to content
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

Merged
merged 4 commits into from
Oct 24, 2016
Merged

Adding AdButler Adapter #707

merged 4 commits into from
Oct 24, 2016

Conversation

dharton
Copy link
Contributor

@dharton dharton commented Oct 14, 2016

Type of change

  • New bidder adapter

Description of change

This change will add the AdButler bidder adapter.

  • test parameters for validating bids
{
  bidder: 'adbutler',
  params: {
    accountID : '167283', //required
    zoneID : '210093', //required       
    minCPM: '1.00', //optional
    maxCPM: '5.00', //optional
    keyword : 'green' //optional
  }
}
  • contact email of the adapter’s maintainer:
    dan@sparklit.com
  • official adapter submission

Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dharton Thank you for the new adapter PR. I left a few notes for your review below. Also when testing this adapter on a page with the provided test params, I'm running into the error Uncaught SyntaxError: missing ) after argument list originating from here, wanted to check if you see that too

var spyLoadScript;

beforeEach(function () {
spyLoadScript = sinon.spy(adLoader, 'loadScript');
Copy link
Collaborator

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){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This occasionally causes Uncaught TypeError: Cannot read property 'sizes' of undefined when running unit tests. Please check that callbackData[callbackId] exists before access the sizes property. See #662, #663, and #664 for PRs where we caught similar TypeErrors

bidResponse.bidderCode = 'adbutler';
}

bidmanager.addBidResponse(callbackData[callbackID].placementCode, bidResponse);
Copy link
Collaborator

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 TypeErrors. 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.
@dharton
Copy link
Contributor Author

dharton commented Oct 21, 2016

@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.

Copy link
Collaborator

@matthewlane matthewlane left a 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);
Copy link
Collaborator

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);
Copy link
Collaborator

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);
Copy link
Collaborator

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);
Copy link
Collaborator

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.
@dharton
Copy link
Contributor Author

dharton commented Oct 24, 2016

@matthewlane I've updated the code to reflect your comments.

Thanks for the heads up about passing the bid request object into createBid().

@matthewlane matthewlane merged commit defa7f0 into prebid:master Oct 24, 2016
@matthewlane
Copy link
Collaborator

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants