Add adpod support to AppNexus adapter #3484
Conversation
modules/appnexusBidAdapter.js
Outdated
| @@ -316,6 +327,22 @@ function newBid(serverBid, rtbBid, bidderRequest) { | |||
| vastImpUrl: rtbBid.notify_url, | |||
| ttl: 3600 | |||
| }); | |||
|
|
|||
| const videoContext = utils.deepAccess( | |||
| bidderRequest.bids[0], | |||
There was a problem hiding this comment.
We can't always assume the first bid object will be an adpod; a publisher could be mixing the ad-types together (like banner + adpod) in the adunit.
The local bidRequest variable (here) looks to already pull the matching bid based on the id (if I'm reading that right); could you take a look at this var and see if it meets the needs here?
764e7c0 to
dd2eacc
Compare
There was a problem hiding this comment.
Hi @matthewlane,
This largely looks good, but please take a look at the questions/points listed below. Thanks!
modules/appnexusBidAdapter.js
Outdated
| function getAdPodPlacementNumber(videoParams, requireExactDuration) { | ||
| const { adPodDurationSec, durationRangeSec } = videoParams; | ||
| const minAllowedDuration = utils.getMinValueFromArray(durationRangeSec); | ||
| const numberOfPlacements = adPodDurationSec / minAllowedDuration; |
There was a problem hiding this comment.
do we need to floor/ceil this value?
modules/appnexusBidAdapter.js
Outdated
| data: payloadString, | ||
| bidderRequest | ||
| }; | ||
| const adPodBid = find(bidRequests, hasAdPod); |
There was a problem hiding this comment.
Won't this find function only grab the first bid it matches? What if there are multiple bids in the bidRequests object as a result from multiple adpod adUnits?
modules/appnexusBidAdapter.js
Outdated
|
|
||
| bid.video = { | ||
| context: 'adpod', | ||
| durationSeconds: rtbBid.rtb.video.duration_ms / 1000, |
There was a problem hiding this comment.
Do we need to floor/ceil here - in case the numbers aren't clean?
modules/appnexusBidAdapter.js
Outdated
| let request = utils.fill(...tagToDuplicate, numberOfPlacements); | ||
|
|
||
| if (requireExactDuration) { | ||
| const divider = numberOfPlacements / durationRangeSec.length; |
There was a problem hiding this comment.
need a floor/ceil here?
modules/appnexusBidAdapter.js
Outdated
| return ( | ||
| bid.mediaTypes && | ||
| bid.mediaTypes.video && | ||
| bid.mediaTypes.video.context === 'adpod' |
There was a problem hiding this comment.
We added adpod context to mediaTypes.js file here.
#3523
You can update to use this const.
dd2eacc to
71071c0
Compare
|
This pull request introduces 1 alert when merging 0635c0f into b6c5644 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
…er-support # Conflicts: # modules/appnexusBidAdapter.js
Type of change
Description of change
brandCategoryExclusionOther information
This is for initial review/testing with other PRs related to #3379