-
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
EngageBDR New Bid Adapter #2309
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.
@jlzhangdev Thanks for submitting this new bid adapter. There are a few things that need to be updated before it's good to go. I have listed them below:
- there are various cases where
var
,let
, andconst
are used to define variables. I would recommend to be consistent and change thevar
s to their correspondinglet
orconst
(depending on the need). - The adapter indicates that it's meant to support both
banner
andvideo
(via thesupportedMediaTypes
field). However, it seems like there's only support for banner ad units when looking through thebuildRequests
andinterpretResponse
functions. Can you take a look through the information on this page http://prebid.org/dev-docs/bidder-adaptor.html#step-1-register-the-adapter-as-supporting-video if you are planning to support video? If you're not, please update thesupportedMediaTypes
field. - Unit tests are needed for each adapter. Can you please put them together and include them in this PR? The following page has some additional information if needed (http://prebid.org/dev-docs/bidder-adaptor.html#adding-unit-tests).
- When I was trying to test out the adapter with the provided test params using the hello_world page, I was seeing some CORS errors in the console for
http://dsp.bmla.com/hb
. Additionally, the response didn't seem like it came back properly; the JSON response started with anull
. Can you take a look into these issues? Please see below for a copy of the entire response:
null({"id":"11789b1d475b6f","seatbid":[{"bid":[{"id":"20f947c2b44109","impid":"20f947c2b44109","price":9.81,"adid":"abcde-12345","nurl":"http://rex.bnmla.com/pixel?not=1&sid=LRX8nxQ4gX&z=16&rx=1.3392&d=3&dt=3&s=16&px=1.007999","adm":"<div><img src=\"http://cdnin.bnmla.com/0b1c6e85e9376e3092df8c9fc8ab9095.gif\" width=350 height=250 /></div>","adomain":["advertiserdomain.com"],"iurl":"http://rex.bnmla.com/pixel?not=1&sid=LRX8nxQ4gX&z=16&rx=1.3392&d=3&dt=3&s=16&px=1.007999","cid":"campaign1","crid":"abcde-12345"}],"seat":"11789b1d475b6f"}],"bidid":"11789b1d475b6f","cur":"USD"})
modules/ebdrBidAdapter.js
Outdated
cpm: responseCPM, | ||
width: ebdrBid.w, | ||
height: ebdrBid.h, | ||
mediaType: BANNER, |
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 mediaType
field isn't needed in the returned bid object.
modules/ebdrBidAdapter.js
Outdated
var adm = decodeURIComponent(ebdrBid.adm); | ||
ebdrResponseImps.push({ | ||
requestId: ebdrBid.id, | ||
bidderCode: spec.code, |
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 bidderCode
field isn't needed in the returned bid object.
Also we need a docs PR before merge. Please submit a PR to the docs repo to add a file for your adapter to the bidders directory so your adapter's params will appear on the bidders page. Thanks! |
@jsnellbaker We have updated our code base to fix all those things mention in your commend. |
@jlzhangdev Thanks for submitting the changes and the docs PR as well. I'm reviewing over the changes now. As a note, do you have a test ad unit for video available? If so, can you provide me those test params (and perhaps include them in your .md file as well)? I'll let you know what I find. |
modules/ebdrBidAdapter.js
Outdated
} | ||
} | ||
let _mediaTypes = bid.mediaTypes && bid.mediaTypes.video ? VIDEO : BANNER; | ||
if (_mediaTypes == BANNER && bid.mediaTypes[_mediaTypes].sizes && bid.mediaTypes[_mediaTypes].sizes[0] && bid.mediaTypes[_mediaTypes].sizes[0].length === 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.
This if
statement doesn't verify if the bid.mediaTypes
and bid.mediaTypes[_mediaTypes]
objects exist. This could lead to undefined
errors if the page only had adUnit.sizes
defined (which may be the case with some older prebid.js supported sites).
The earlier statement for the _mediaTypes
variable doesn't guarantee the bid.mediaTypes
object actually exists when it assigns BANNER
.
Should alter the logic here (likely in the if
but perhaps in _mediaTypes
) to avoid potential errors.
Fix an issue might caused undefined if it is an old bidder
Hi @jsnellbaker, Let me know if anything else might have a problem. Thanks. |
Hi @mjacobsonny, can you spec what doc do we need? |
@jlzhangdev Thanks for making the change. The banner is showing fine if Do you have test params for a video ad unit? If so, can you share them so I can verify the adapter for this ad type; also can you add them to your md file? If they have any unique params, please include those in your docs PR. Thanks. |
Hi @jsnellbaker |
Thanks @jlzhangdev for providing the video test param and updating your md file. Everything looks to be working fine now. |
Type of change
Description of change
Be sure to test the integration with your adserver using the Hello World sample page.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Other information