Teads adapter: handle video playerSize #3423

valsouche · 2019-01-07T16:47:28Z

Type of change

Bugfix

Description of change

Change the way to get sizes. Now we check if playerSize is available before others.
Add tests about sizes.

test parameters for validating bids

{
  bidder: 'teads',
  params: {
    pageId: 89489,
    placementId: 96888
  }
}

muuki88 · 2019-01-08T08:32:55Z

Fixes #3373

muuki88

Awesome! Thanks @valsouche ❤️

muuki88 · 2019-01-08T08:37:34Z

modules/teadsBidAdapter.js

+  let sizes;
+  if (isVideoBid(bid)) {
+    sizes = utils.deepAccess(bid, 'mediaTypes.video.playerSize') ||
+      utils.deepAccess(bid, 'mediaTypes.video.sizes') ||


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

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...

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.

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 😘

robertrmartinez

If logic is fine with multi-format comment below, then LGTM

robertrmartinez · 2019-01-10T21:02:39Z

modules/teadsBidAdapter.js

+  let sizes;
+  if (isVideoBid(bid)) {
+    sizes = utils.deepAccess(bid, 'mediaTypes.video.playerSize') ||
+      utils.deepAccess(bid, 'mediaTypes.video.sizes') ||


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.

robertrmartinez · 2019-01-10T22:03:57Z

test/spec/modules/teadsBidAdapter_spec.js

+    });
+
+    it('should use good mediaTypes video playerSizes', function() {
+      const mediaTypesPlayerSize = {


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!

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.

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?

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!

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

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

robertrmartinez

@valsouche

As it stands this works, but take a look at the suggestions and let me know what you think

robertrmartinez · 2019-01-14T19:49:53Z

modules/teadsBidAdapter.js

-  if (isVideoBid(bid)) {
+  if (isHybridBid(bid)) {
+    sizes = concatHybridBidSizes(bid);
+  } else if (isVideoBid(bid)) {


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.

CC @muuki88 for some input as well

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

Makes sense 😃

robertrmartinez

Awesome!

LGTM!

* Teads adapter: handle playerSize * Handle multiple mediatypes sizes * Clean sizes logic

Teads adapter: handle playerSize

6c715ab

muuki88 approved these changes Jan 8, 2019

View reviewed changes

mkendall07 requested a review from robertrmartinez January 9, 2019 22:26

mkendall07 assigned robertrmartinez Jan 9, 2019

robertrmartinez approved these changes Jan 10, 2019

View reviewed changes

Handle multiple mediatypes sizes

e189b64

robertrmartinez reviewed Jan 14, 2019

View reviewed changes

Clean sizes logic

9cd8787

muuki88 approved these changes Jan 15, 2019

View reviewed changes

robertrmartinez approved these changes Jan 15, 2019

View reviewed changes

robertrmartinez added the LGTM label Jan 15, 2019

robertrmartinez merged commit 3a44263 into prebid:master Jan 15, 2019

pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019

Teads adapter: handle video playerSize (prebid#3423)

55c9c4a

* Teads adapter: handle playerSize * Handle multiple mediatypes sizes * Clean sizes logic

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teads adapter: handle video playerSize #3423

Teads adapter: handle video playerSize #3423

valsouche commented Jan 7, 2019

muuki88 commented Jan 8, 2019 •

edited

Loading

muuki88 left a comment

muuki88 Jan 8, 2019

valsouche Jan 8, 2019

robertrmartinez Jan 10, 2019 •

edited

Loading

muuki88 Jan 11, 2019

robertrmartinez left a comment •

edited

Loading

robertrmartinez Jan 10, 2019 •

edited

Loading

robertrmartinez Jan 10, 2019

robertrmartinez Jan 10, 2019

muuki88 Jan 11, 2019

robertrmartinez Jan 11, 2019

valsouche Jan 14, 2019

valsouche Jan 14, 2019

robertrmartinez left a comment

robertrmartinez Jan 14, 2019

robertrmartinez Jan 14, 2019

valsouche Jan 15, 2019

muuki88 Jan 15, 2019

robertrmartinez left a comment

Teads adapter: handle video playerSize #3423

Teads adapter: handle video playerSize #3423

Conversation

valsouche commented Jan 7, 2019

Type of change

Description of change

muuki88 commented Jan 8, 2019 • edited Loading

muuki88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez left a comment • edited Loading

Choose a reason for hiding this comment

robertrmartinez Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez left a comment

Choose a reason for hiding this comment

muuki88 commented Jan 8, 2019 •

edited

Loading

robertrmartinez Jan 10, 2019 •

edited

Loading

robertrmartinez left a comment •

edited

Loading

robertrmartinez Jan 10, 2019 •

edited

Loading