Quantcast Bid Adapter: uses video mediatypes to read OpenRTB parameters #6932
Conversation
patmmccann
changed the title
patmmccann
changed the title
|
do you want to fallback to your own parameters so you don't break any existing integrations? |
| video['delivery'] = bid.params.video.delivery; | ||
| video['placement'] = bid.params.video.placement; | ||
| video['api'] = bid.params.video.api; | ||
| } |
There was a problem hiding this comment.
why not keep this block so either place populates it? I think this is why your tests are failing
| @@ -221,7 +225,11 @@ describe('Quantcast adapter', function () { | |||
|
|
|||
| it('overrides video parameters with parameters from adunit', function() { | |||
| setupVideoBidRequest({ | |||
There was a problem hiding this comment.
this test looks good as is, it has two different values for mimes and you're testing the ad unit wins.
modules/quantcastBidAdapter.js
Outdated
| video['h'] = bid.mediaTypes.video.playerSize[0][1]; | ||
| } else { | ||
| video['w'] = bid.mediaTypes.video.playerSize[0]; | ||
| video['h'] = bid.mediaTypes.video.playerSize[1]; |
There was a problem hiding this comment.
you may want to check for h and w existing directly as well
modules/quantcastBidAdapter.js
Outdated
| @@ -25,30 +25,89 @@ export const storage = getStorageManager(QUANTCAST_VENDOR_ID, BIDDER_CODE); | |||
|
|
|||
| function makeVideoImp(bid) { | |||
| const video = {}; | |||
|
|
|||
| if (bid.params.video) { | |||
There was a problem hiding this comment.
should mimes also be retrieved from the mediaTypes object?
modules/quantcastBidAdapter.js
Outdated
| if (bid.params.video) { | ||
| video['mimes'] = bid.params.video.mimes; | ||
| } | ||
|
|
||
| if (bid.mediaTypes && bid.mediaTypes.video && bid.mediaTypes.video.minduration) { |
There was a problem hiding this comment.
you might want to consider using utils.deepAccess for nested parameters.
modules/quantcastBidAdapter.js
Outdated
| video['api'] = bid.params.video.api; | ||
| } | ||
| if (bid.mediaTypes.video.mimes) { | ||
|
|
||
| if (bid.mediaTypes && bid.mediaTypes.video && bid.mediaTypes.video.mimes) { |
There was a problem hiding this comment.
this should be grouped with the bid.param.video.mimes above
|
@patmmccann could you please have another look |
modules/quantcastBidAdapter.js
Outdated
| } else { | ||
| video['w'] = bid.mediaTypes.video.playerSize[0]; | ||
| video['h'] = bid.mediaTypes.video.playerSize[1]; | ||
| for (const prop of VIDEO_PROPS_TO_REMOVE) { |
There was a problem hiding this comment.
a bit of a nit-pick - would it make more sense to have an allow list of properties rather than a block list? otherwise the video object on mediaTypes or params could introduce new properties and we wouldn't filter them.
Type of change
Description of change
Reads video properties from mediaTypes #6512