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.

Sign up for GitHub

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

Jump to bottom

New Cox adapter #1228

Merged
merged 5 commits into from
Jun 13, 2017
Merged

New Cox adapter #1228

merged 5 commits into from
Jun 13, 2017

Conversation

reynold-cox
Copy link
Contributor

Type of change

New bidder adapter

Description of change

test parameters for validating bids:

{
  bidder: 'cox',
  params: {
    size: '300x250',
    id: 2000005991707,
    siteId: 2000100948180,
  }
}
  • contact email of the adapter’s maintainer: reynold@coxmt.com
  • [x ] official adapter submission

matthewlane
matthewlane reviewed Jun 1, 2017
View reviewed changes
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.

Thanks for the PR, I was able to verify bid responses and tests are passing

src/adapters/cox.js
// function finalizeAd(price) {
// this.ad = W.CMT.Service.setAuctionPrice(this.ad, price);
// return this;
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't need the code that is commented out here you may want to remove this

src/adapters/cox.js
bidObj.width = data.w;
bidObj.height = data.h;
// bidObj.floor = W.CMT.Service.getSecondPrice(adZoneKey);
// bidObj.finalizeAd = finalizeAd;
Copy link
Collaborator

Choose a reason for hiding this comment

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

More commented code for potential removal

src/adapters/cox.js

// setAuctionPrice: function setAuctionPrice(ad, bid) {
// return ad ? ad.replace('${AUCTION_PRICE}', bid) : ad;
// },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this all required at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a partner that utilizes the commented-out code. We'd rather keep it in there for completeness' sake. But if y'all would prefer it be removed, we'll be happy to do so. Please let me know. Thanks!

@dbemiller
Copy link
Contributor

@reynold-cox What exactly does it mean if someone configures your bidder with a size param?

Not that I want to go policing bidders' params or anything... but size already comes in on the bid, and nobody reviewing this could think of a reason why a bidder would have a size... so I wanted to make sure it wasn't an oversight & duplicate info.

Or if there's motivation for the duplicate info, then it'd be very helpful to know why, so we can factor the use case into future prebid designs.

@protonate
Copy link
Collaborator

I'm not able to get bids back, are the test params still operational?

@reynold-cox
Copy link
Contributor Author

Hi David
Our ad spaces only support a single size each, and our ad servers expect the size to be present in the request. Since the ad unit config can have multiple sizes defined, we can't always deduce the size for a given Cox ad space...so that's why we have the "size" key in the bidder params. Thanks!

@reynold-cox
Copy link
Contributor Author

@protonate:
The test params should be ok. I just used the following and I'm getting a non-zero bid back:

{
  bidder: 'cox',
    params: {
      size: '300x250',
      id: 2000005991707,
      siteId: 2000100948180,
  }
}

Are you not getting a response, or are you getting a zero bid?

@protonate
Copy link
Collaborator

Ah, I see, thanks, it works if running on localhost, confirmed bids back, reviewed.

@protonate protonate merged commit 89c6300 into prebid:master Jun 13, 2017
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Sep 6, 2017
@vzhukovsky
Merge pull request prebid#110 in AOLP_ADS_JS/prebid.js from release/1…
711ffb3 
….24.0 to aolgithub-master

* commit '7a81386579a3c129617323ef32e1c8be0897c3c7': (31 commits)
  Added changelog entry.
  Added records in aolPartnersIds.json.
  Prebid 0.25.0 Release
  Added Unruly adapter (prebid#1264)
  Fixing Adyoulike adapter for Safari iOS7 (prebid#1296)
  hide overflow to prevent scrolling (prebid#1294)
  fix protocol detection in iframe (prebid#1293)
  Add eplanning adapter (prebid#1245)
  Use Renderer command queue to render outstream (prebid#1283)
  Generate no-bid response for ech bid request not matched by a bid (prebid#1216)
  New Cox adapter (prebid#1228)
  Carambola Adapter (prebid#1221)
  Pass through transactionId and set fd=1 (prebid#1259)
  Integration test page for adapters (prebid#1182)
  Getting DigiTrustID in GumGum adapter (prebid#1256)
  Trion adapter (prebid#1254)
  use request bidder code as default bidderCode for createBid (prebid#1235)
  Add drop pixels once feature to AOL Adapter (prebid#1224)
  Rubicon Adapter - Adding 320x150 (prebid#1279)
  Prevent 'about:' protocol in _publisherTagUrl (prebid#1170)
  ...
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
@reynold-cox @jbAdyoulike
New Cox adapter (prebid#1228)
c42d3fb 
* Cox adapter (new)

* Unit test for cox adapter (new)

* Added cox entry

* Styling for ESLint

* Styling for ESLint
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@reynold-cox @dluxemburg
New Cox adapter (prebid#1228)
5b9258a 
* Cox adapter (new)

* Unit test for cox adapter (new)

* Added cox entry

* Styling for ESLint

* Styling for ESLint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewlane matthewlane matthewlane approved these changes

Assignees

@matthewlane matthewlane

Labels
LGTM needs update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reynold-cox @dbemiller @protonate @matthewlane