-
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
CPMStar Bid Adapter #3820
CPMStar Bid Adapter #3820
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.
Thanks for putting this adapter together. There are a few items that should be reviewed/updated. Please see the comments below for details.
Additionally, do you happen to have any test params for a video bid? If so - can you please update the PR description and update the md file? I verified your adapter is working fine for banner bids based on the test params you've already provided, but I'd like to ensure the video type works as well since your adapter is supporting this mediaType.
Please let me know if you have any questions and thanks!
modules/cpmstarBidAdapter.js
Outdated
@@ -0,0 +1,121 @@ | |||
import * as utils from 'src/utils'; | |||
import { registerBidder } from 'src/adapters/bidderFactory'; |
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.
In lieu of this PR #3435, any imported files within a modules file needs to use the relative style path.
Can you please update these imports accordingly?
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.
We updated the imports.
modules/cpmstarBidAdapter.js
Outdated
|
||
onTimeout(timeout) { | ||
}, | ||
onBidWon(winningBid) { |
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 can remove these functions from the adapter if you're not planning on using them.
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.
Thanks, these have been removed.
var raw = serverResponse.body[i]; | ||
var rawBid = raw.creatives[0]; | ||
if (!rawBid) { | ||
return; |
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.
As a suggestion, it may nice to add some log messages here (such as utils.logWarn()
) to indicate something was missing from the bid response. It would make it easier to debug by seeing the message in the browser console.
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.
We added a few log messages, thanks!
modules/cpmstarBidAdapter.js
Outdated
netRevenue: rawBid.netRevenue ? rawBid.netRevenue : true, | ||
ttl: rawBid.ttl ? rawBid.ttl : DEFAULT_TTL, | ||
creativeId: rawBid.creativeid || 0, | ||
ad: '' |
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.
Given you're supporting both video and banner bids, I'd recommend to remove this default value for the ad
property. The ad
is only used in the banner
type, so it would be left equal to an empty string when you just serve back a video bid.
Having both fields present in the bid may cause some confusion, so it'd be better to only assign the property when its needed.
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 has been removed -- thanks for the tip.
expect(requests[0]).to.have.property('method'); | ||
expect(requests[0]).to.have.property('url'); | ||
expect(requests[0]).to.have.property('bidRequest'); | ||
expect(requests[0].url.startsWith('//server.cpmstar.com/view.aspx')).to.equal(true); |
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.
The startsWith
property is not compatible with IE browsers - which is why the browserstack tests are failing for this PR. Can you use an alternate assertion for this part of the tests?
I think you could use chai's includes
instead to get the same effect (see example below).
expect(requests[0].url).to.include('//dev.server.cpmstar.com/view.aspx');
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 test has been updated for IE compatibility.
…ded warnings to erroneous server responses, and removed the default value for ad in bid response.
We tested the banner placement with the helloworld example, which works fine. But the only working video example we could find (in the prebid.org docs repo, under: examples/video/jwPlayerPrebid.html) uses pbjs.adServers.dfp.buildVideoUrl. DFP does not seem to like the vastUrl/vastXml/videoCacheKey returned by bidsBackHandler from our bidder, and DFP will not provide our vastXml at the url provided by dfp.buildVideoUrl() However, in the above referenced example, calling invokeVideoPlayer() with our bid's vastUrl directly as follows: invokeVideoPlayer(bids.video1.bids[0].vastUrl) ..works just fine. The vastUrl/vastXml contains a proper VAST 2.0 response from our ad server. Please advise. Thank you! |
That jwPlayerPrebid.html test page should provide a good idea on how it could be setup (I'll see if we can upload a dedicated test page in the repo for easier access). The key would be to use the This whole process is similar with other types of ads (such as banner), where we call the equivalent I hope the above makes sense. If so, do you have any test bid params for a video bid that I could to run a check? |
We added the test parameters for video, above in the PR description: { The prebid cache is indeed loading the vast XML properly, and the bid response contains a xmlCacheKey and the xmlVastUrl on the prebid cache. However, DFP's resulting URL does not appear to mirror the VAST creative effectively in the example cited. Actually, our banner code works fine with both hello world and the example here But not with the DFP video examples. The url generated by pbjs.adServers.dfp.buildVideoUrl() returns an empty response. We examined the docs provided by DFP, but it appears this is some kind of issue relating to the cust_params= section of the URL, which DFP does not document except to say they are custom key-value pairs. |
Ok, we tested 3 other video adapters using their test placements and none of them worked in the jwPlayerPrebid.html example. Adapters tested: lkqd, vuble, oneVideo. The only adapter which seems to currently work with this is example is appnexus |
@JoshuaMGoldstein I found the likely issue; the DFP campaign that was setup for those test pages has a keyword targeting on the I added the following code to dynamically adjust the CPM of your test bid to the ideal value and I saw the preroll ad play through the player (the video.js one).
This should work on your end as well if you want to give it a try. From my end, everything looks good now and we can merge this PR. |
* Added CPMStar Bid Adapter * Updated getPlayerSize for cpmstarBidAdapter * Improved cpmstarBidAdapter code coverage * updated test spec, removed empty functions, made imports relative, added warnings to erroneous server responses, and removed the default value for ad in bid response. * added test video ad unit
Type of change
Description of change
Added CPMStar Bid Adapter
contact email of the adapter’s maintainer: josh@cpmstar.com
official adapter submission
A link to a PR on the docs repo: Added cpmstarBidAdapter docs prebid.github.io#1309
Other information
Contact Person:
Joshua Goldstein
General Manager, CPMStar
josh@cpmstar.com