[EXCH-10877] Add support for native to OpenX bid adapter #59
Conversation
…e-imps' into EXCH-10877-add-support-for-native-imps
modules/openxBidAdapter.js
Outdated
| function isBannerBid(bid) { | ||
| return utils.deepAccess(bid, 'mediaTypes.banner') || !isVideoBid(bid); | ||
| return utils.deepAccess(bid, 'mediaTypes.banner') || (!isVideoBid(bid) && !isNativeBid(bid)); |
There was a problem hiding this comment.
can we move this condition (!isVideoBid(bid) && !isNativeBid(bid)) to const as this is getting less readable
modules/openxBidAdapter.js
Outdated
| // In case of multi-format bids remove `video` from mediaTypes as for video a separate bid request is built | ||
| .map(bid => ({...bid, mediaTypes: {...bid.mediaTypes, video: undefined}})); | ||
|
|
||
| let requests = bannerAndNativeBids.length ? [createRequest(bannerAndNativeBids, bidderRequest, undefined)] : []; |
There was a problem hiding this comment.
are we fine now, all of our banner bids with end up with
{
context: {
mediaType: undefined
}
}
Does undefined means explicitly that is some kind of mixed? (or it can be also uknown type)
What about null rather than undefined?
There was a problem hiding this comment.
Yes, undefined in this case means that this request can contain both - banner and native imps. So we cannot say what is the media type of whole bid request.
I can change it to null - it should work the same as undefined.
| let bannerAndNativeBids = bids.filter(bid => isBannerBid(bid) || isNativeBid(bid)) | ||
| // In case of multi-format bids remove `video` from mediaTypes as for video a separate bid request is built | ||
| .map(bid => ({...bid, mediaTypes: {...bid.mediaTypes, video: undefined}})); |
There was a problem hiding this comment.
in case of this map, we dont need something similar for video?
What i do understand for multi video we will end up with
{native: [value], video: [value]} - not marking other types as undefined
There was a problem hiding this comment.
Actually for video we don't need to do something similar because for each element of videoBids we specify VIDEO mediaType when we build bid request. Because of this, converter will skip all other mediaTypes (banner, native) when converting request to openRTB. So at the end we will have only video format in such bid request.
I added assertions to unit tests which verify it.
…e-imps' into EXCH-10877-add-support-for-native-imps
| expect(requests[1].data.imp).to.have.length(1); | ||
| expect(requests[1].data.imp[0].banner).to.not.exist; | ||
| expect(requests[1].data.imp[0].native).to.not.exist; | ||
| if (FEATURES.VIDEO) { |
There was a problem hiding this comment.
Wondering what is FEATURES.VIDEO and FEATURES.NATIVE.
Are those if's necessary?
There was a problem hiding this comment.
Our adapter utilizes ortbConverter library and there are some checks there which verifies if video/native feature is enabled.
When all tests are executed with gulp test command, test-all-features-disabled task will be executed firstly, and then test-only.
I enclosed native-related tests in this if, so that they won't be run when native feature is disabled (test-all-features-disabled). However those tests should be run in test-only task.
I've also seen similar pattern in other adapters tests: example.
There was a problem hiding this comment.
That makes sense. Thanks for clarification :).
| @@ -333,7 +488,7 @@ describe('OpenxRtbAdapter', function () { | |||
|
|
|||
| const request = spec.buildRequests(bidRequestsWithMediaTypes, mockBidderRequest); | |||
| expect(request[0].data.ext.platform).to.equal(bidRequestsWithMediaTypes[0].params.platform); | |||
| expect(request[1].data.ext.platform).to.equal(bidRequestsWithMediaTypes[0].params.platform); | |||
| expect(request[1].data.ext.platform).to.equal(bidRequestsWithMediaTypes[1].params.platform); | |||
There was a problem hiding this comment.
If we are changing this line, maybe it would be a good idea to give bidRequestsWithMediaTypes[1].params.platform different value than for bidRequestsWithMediaTypes[0].params.platform
There was a problem hiding this comment.
Good idea. Updated, thanks!
|
Changes were merged to upstream repo: prebid#12625 |
No description provided.