-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Debugging Module: Bid responses for various media types (+ TestBidder) #12720
Conversation
Tread carefully! This PR adds 1 linter error and 1 linter warning (possibly disabled through directives):
|
bidsBackHandler: function() { | ||
const bids = pbjs.getHighestCpmBids(adUnitCode); | ||
const winningBid = bids[0]; | ||
jwplayer("player").setup({ |
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.
could we put this in a renderer? https://docs.prebid.org/dev-docs/show-outstream-video-ads.html
if bid.renderer.render
can load jwplayer + do what you have here, it would/should work without additional setup.
modules/debugging/responses.js
Outdated
[VIDEO]: () => '<?xml version=\"1.0\" encoding=\"UTF-8\"?><VAST version=\"3.0\"><Ad><InLine><AdSystem>GDFP</AdSystem><AdTitle>Demo</AdTitle><Description><![CDATA[Demo]]></Description><Creatives><Creative><Linear ><Duration>00:00:11</Duration><VideoClicks><ClickThrough><![CDATA[https://prebid.org/]]></ClickThrough></VideoClicks><MediaFiles><MediaFile delivery=\"progressive\" width=\"640\" height=\"360\" type=\"video/mp4\" scalable=\"true\" maintainAspectRatio=\"true\"><![CDATA[https://s3.amazonaws.com/files.prebid.org/creatives/PrebidLogo.mp4]]></MediaFile></MediaFiles></Linear></Creative></Creatives></InLine></Ad></VAST>', | ||
[NATIVE]: () => ({ | ||
ortb: { | ||
link: { |
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.
your approach here is to fill every possible asset type, which works for "legacy" native.
We recommend using ortb now though, and in it you're allowed for example to ask for more than one image. Also, templates written for ORTB native won't necessarily work with this.
What this should do IMO is loop over bidRequest.nativeOrtbRequest.assets
and provide a default value for each asset. Here's a reference (look for "Asset Response").
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 code changes LGTM. Some minor comments about the integration examples. My preference would be to extract the "bid response simulation" to a separate javascript file (maybe testBidder.js
) but I'm not sure if that'd make things more or less clear.
* This section handles simulating a bidder | ||
* that will always respond with bid responses. | ||
* | ||
* This part should not be present in production. |
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.
Almost nothing in any integration example should be left in production.
bidder: 'testBidder', | ||
}, | ||
then: { | ||
creativeId: 'testCreativeId', |
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.
is this just to have something here? I think it should work with an empty object.
Type of change
Description of change
Other information
Closes #12512
Closes #7365