Skip to content
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.

Sign up for GitHub

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

Teads adapter: handle video playerSize #3423

Merged
merged 3 commits into from
Jan 15, 2019
Merged

Teads adapter: handle video playerSize #3423

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c715ab
Teads adapter: handle playerSize
Jan 7, 2019
e189b64
Handle multiple mediatypes sizes
Jan 14, 2019
9cd8787
Clean sizes logic
Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion modules/teadsBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ function buildRequestObject(bid) {
let placementId = utils.getValue(bid.params, 'placementId');
let pageId = utils.getValue(bid.params, 'pageId');

reqObj.sizes = utils.parseSizesInput(bid.sizes);
reqObj.sizes = getSizes(bid);
reqObj.bidId = utils.getBidIdParameter('bidId', bid);
reqObj.bidderRequestId = utils.getBidIdParameter('bidderRequestId', bid);
reqObj.placementId = parseInt(placementId, 10);
Expand All @@ -127,6 +127,55 @@ function buildRequestObject(bid) {
return reqObj;
}

function getSizes(bid) {
let sizes;
if (isHybridBid(bid)) {
sizes = concatHybridBidSizes(bid);
} else if (isVideoBid(bid)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a couple comments / suggestions:

With this current implementation a multi-format ad unit will add video.sizes and video.playerSize.

But if it is just a video mediaType, then it picks video.playerSize OR video.sizes. Not sure if it is better to remain consistent and just concatenate in this scenario as well?

Or maybe in concatHybridBidSizes add the logic to choose video.playerSize OR video.sizes.

Also, instead of checking the isHybridBid or isVideoBid => else, does it make sense to just use the concatHybridBidSizes and not do any mediaTypes checking?

Since you are using utils.deepAccess to get the mediaTypes Sizes, then if the mediaType is not present, it will do what is needed anyways right?

so getSizes could just be something like:

function getSizes(bid) {
   return utils.parseSizesInput(concatHybridBidSizes(bid));
}

And if needed add the video.playerSize || video.sizes logic, if that is the intended implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @muuki88 for some input as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact ! I did the change, simpler and cleaner :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 😃

sizes = utils.deepAccess(bid, 'mediaTypes.video.playerSize') ||
utils.deepAccess(bid, 'mediaTypes.video.sizes') ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not convinced that mediaTypes.video.sizes is a supported property 😂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PlayerSize is optional, bid.sizes is the default and as you we are not sure about mediaTypes.video.sizes (We didn't find any line code about this property in the sources) but there is an adUnit section in the Prebid documentation which makes us doubt (see my screen). That is the reason why we preferred to add this check anyway...
capture d ecran 2019-01-08 a 14 45 49

Copy link
Collaborator

@robertrmartinez robertrmartinez Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW Prebid will expose whatever the user puts in the adUnit.mediaTypes.video object to the adapters.

So if a publisher is using Teads and has an ad unit like so:

{
			code: 'test_div_1',
			mediaTypes: {
                video: {
                    playerSize: [400, 600],
                    sizes: [400, 600]
                }
            }
   ...... etc

Then both playerSize and sizes will be available to pick from.

The only caveat is that PBJS standardizes the playerSize to be the common array of size arrays if a single size array is passed in. (Like the example I just gave)

What this means is that the adapter would see an object with those two fields like so:

{
   ...
   playerSize: [ [400, 600] ],
   sizes: [400, 600]
   ...
}

We have noticed some publishers are pretty loose with declaring their sizes as a single array, or the recommended array of size arrays.

From what it looks like this scenario works just fine in this PR.

This adapter calls utils.parseSizesInput (which is nice) after this so even if a publisher uses adUnit.mediaTypes.video.sizes instead of playerSize, it will handle accordingly and return the gpt standard array of ['40x60']

Just figured I would bring that up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying this. Even js is untyped, I pretend it is 😉 and try to avoid ambiguous variables. This fix was necessary as some internal behavior of adunit.sizes changed and the playerSize was not used anymore. That's why highlighted this in this PR.

When I searched the code for sizes related functions, I got really confused as there are multiple representations (as you mentioned single-size array and array of size arrays, but also the <witdh>x<height> arrays) and various parsing methods.

Hope this implementation will last ❤️ Thanks @valsouche for taking care 😘

bid.sizes;
} else {
sizes = utils.deepAccess(bid, 'mediaTypes.banner.sizes') || bid.sizes;
}

return utils.parseSizesInput(sizes);
}

function isVideoBid(bid) {
return bid.mediaType === 'video' || utils.deepAccess(bid, 'mediaTypes.video');
}

function isHybridBid(bid) {
return isVideoBid(bid) && utils.deepAccess(bid, 'mediaTypes.banner');
}

function concatHybridBidSizes(bid) {
let playerSize = utils.deepAccess(bid, 'mediaTypes.video.playerSize');
let videoSizes = utils.deepAccess(bid, 'mediaTypes.video.sizes');
let bannerSizes = utils.deepAccess(bid, 'mediaTypes.banner.sizes');
let sizes;

if (utils.isArray(bannerSizes) || utils.isArray(playerSize) || utils.isArray(videoSizes)) {
let mediaTypesSizes = [bannerSizes, videoSizes, playerSize];
sizes = mediaTypesSizes
.reduce(function(acc, currSize) {
if (utils.isArray(currSize)) {
if (utils.isArray(currSize[0])) {
currSize.forEach(function (childSize) { acc.push(childSize) })
} else {
acc.push(currSize);
}
}
return acc;
}, [])
} else {
sizes = bid.sizes;
}

return sizes;
}

function _validateId(id) {
return (parseInt(id) > 0);
}
Expand Down
84 changes: 73 additions & 11 deletions test/spec/modules/teadsBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {spec} from 'modules/teadsBidAdapter';
import {newBidder} from 'src/adapters/bidderFactory';

const ENDPOINT = '//a.teads.tv/hb/bid-request';
const AD_SCRIPT = '<script type="text/javascript" class="teads" async="true" src="http://a.teads.tv/hb/bid-request/hb/getAdSettings"></script>"';
const AD_SCRIPT = '<script type="text/javascript" class="teads" async="true" src="http://a.teads.tv/hb/getAdSettings"></script>"';

describe('teadsBidAdapter', function() {
const adapter = newBidder(spec);
Expand Down Expand Up @@ -96,13 +96,14 @@ describe('teadsBidAdapter', function() {
}
];

let bidderResquestDefault = {
'auctionId': '1d1a030790a475',
'bidderRequestId': '22edbae2733bf6',
'timeout': 3000
};

it('sends bid request to ENDPOINT via POST', function() {
let bidderRequest = {
'auctionId': '1d1a030790a475',
'bidderRequestId': '22edbae2733bf6',
'timeout': 3000
};
const request = spec.buildRequests(bidRequests, bidderRequest);
const request = spec.buildRequests(bidRequests, bidderResquestDefault);

expect(request.url).to.equal(ENDPOINT);
expect(request.method).to.equal('POST');
Expand All @@ -129,7 +130,7 @@ describe('teadsBidAdapter', function() {
expect(payload.gdpr_iab).to.exist;
expect(payload.gdpr_iab.consent).to.equal(consentString);
expect(payload.gdpr_iab.status).to.equal(12);
})
});

it('should send GDPR to endpoint with 11 status', function() {
let consentString = 'JRJ8RKfDeBNsERRDCSAAZ+A==';
Expand All @@ -152,7 +153,7 @@ describe('teadsBidAdapter', function() {
expect(payload.gdpr_iab).to.exist;
expect(payload.gdpr_iab.consent).to.equal(consentString);
expect(payload.gdpr_iab.status).to.equal(11);
})
});

it('should send GDPR to endpoint with 22 status', function() {
let consentString = 'JRJ8RKfDeBNsERRDCSAAZ+A==';
Expand All @@ -173,7 +174,7 @@ describe('teadsBidAdapter', function() {
expect(payload.gdpr_iab).to.exist;
expect(payload.gdpr_iab.consent).to.equal('');
expect(payload.gdpr_iab.status).to.equal(22);
})
});

it('should send GDPR to endpoint with 0 status', function() {
let consentString = 'JRJ8RKfDeBNsERRDCSAAZ+A==';
Expand All @@ -196,7 +197,68 @@ describe('teadsBidAdapter', function() {
expect(payload.gdpr_iab).to.exist;
expect(payload.gdpr_iab.consent).to.equal(consentString);
expect(payload.gdpr_iab.status).to.equal(0);
})
});

it('should use good mediaTypes video playerSizes', function() {
const mediaTypesPlayerSize = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just want to confirm:

If a publisher requests a multi-format adUnit with banner and video, teads will default to Video and try to get the sizes from the video object.

Just want to confirm this is intended and there is no logic necessary for teads' approach to multiformat requests.

From a glance it does not look like mediaType is passed into the Teads Ad request anyways, but want to be sure that in this scenario, the behavior is as expected!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ex:

{
   mediaTypes: {
      video: {
         playerSize: [400, 600]
      }
      banner: {
         sizes: [[300, 250], [728, 90]]
      }
}

Some adapters do a multi-format call with both, some default to just video, some to banner.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point! Teads does offer some special formats like parallax ads, which may come with another size. Should video and banner sizes simply be concatenated?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is hard to say without knowing the Teads Ad Exchange or what they tell customers about their implementation.

I think coming up with what the Teads approach to multi-format adUnits is would be a good start.

Maybe concatenating is the solution, maybe having one of the mediaTypes be higher priority (as video is in this current implementation)

Not sure!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point,
we prefer receive all sizes, so we will concatenate. I'll push the update soon. Thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. :) Don't hesitate to ask if you find something weird.
Thanks !

'mediaTypes': {
'video': {
'playerSize': [32, 34]
}
}
};
checkMediaTypesSizes(mediaTypesPlayerSize, '32x34')
});

it('should use good mediaTypes video sizes', function() {
const mediaTypesVideoSizes = {
'mediaTypes': {
'video': {
'sizes': [12, 14]
}
}
};
checkMediaTypesSizes(mediaTypesVideoSizes, '12x14')
});

it('should use good mediaTypes banner sizes', function() {
const mediaTypesBannerSize = {
'mediaTypes': {
'banner': {
'sizes': [46, 48]
}
}
};
checkMediaTypesSizes(mediaTypesBannerSize, '46x48')
});

it('should use good mediaTypes for both video and banner sizes', function() {
const hybridMediaTypes = {
'mediaTypes': {
'banner': {
'sizes': [46, 48]
},
'video': {
'sizes': [[50, 34], [45, 45]]
}
}
};
checkMediaTypesSizes(hybridMediaTypes, ['46x48', '50x34', '45x45'])
});

function checkMediaTypesSizes(mediaTypes, expectedSizes) {
const bidRequestWithBannerSizes = Object.assign(bidRequests[0], mediaTypes);
const requestWithBannerSizes = spec.buildRequests([bidRequestWithBannerSizes], bidderResquestDefault);
const payloadWithBannerSizes = JSON.parse(requestWithBannerSizes.data);

return payloadWithBannerSizes.data.forEach(bid => {
if (Array.isArray(expectedSizes)) {
expect(JSON.stringify(bid.sizes)).to.equal(JSON.stringify(expectedSizes));
} else {
expect(bid.sizes[0]).to.equal(expectedSizes);
}
});
}
});

describe('interpretResponse', function() {
Expand Down