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.

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

Yieldlab Bid Adapter: add support for native mediatype #7609

Merged
merged 16 commits into from
Oct 29, 2021

Conversation

alex-ylb
Copy link
Contributor

@alex-ylb alex-ylb commented Oct 21, 2021

Type of change

  • [ x] Feature

Description of change

In this change the yieldlabBidAdapter adapter adds support for Native mediatype

The second PR on the docs repo at: prebid/prebid.github.io#3363

@muuki88
Copy link
Collaborator

muuki88 commented Oct 24, 2021

This is awesome news. No workarounds anymore.

What I don't understand is if this supports Multiformat?

@alex-ylb
Copy link
Contributor Author

Hi @muuki88 , for now it supports native image.

@muuki88
Copy link
Collaborator

muuki88 commented Oct 25, 2021

Thanks for your feedback 🤗 @alex-ylb

My question was what happens if banner and native are requested at the same time.

@kippsterr
Copy link
Contributor

Hi @muuki88

I wanna try to give you an answer to your questions.

As far as I can understand it from the docs, by multi-format you mean that there is more than one media type defined per ad unit. That is possible for the Yieldlab adapter.

However, to make a configuration like really support different media types it is additionally required to set up the bids section of the ad unit accordingly. At Yieldlab, an Adslot can only have one type which would be e.g. banner or native. Users would have to configure as many different bids (Adslots) with their respective type as there are media types in that unit.

This brings me to the second question. E.g. banner and native can be requested "at the same time", if the bids section is configured as described above. However, one bid cannot be banner or native at the same time as we would either return a result for either of that. Of course it would work to have multiple results but one bid would always only be of one certain type.

Does that answer your questions? Please ping us again if anything doesn't make sense.

@muuki88
Copy link
Collaborator

muuki88 commented Oct 26, 2021

That's super helpful @kippsterr 🙇‍♂️ 💡
If I would put this into practice

However, to make a configuration like really support different media types it is additionally required to set up the bids section of the ad unit accordingly

This could look something like this

const adUnit = {
  code: 'ad-slot-1',
  mediaTypes: {
    banner: {
      sizes: [ [ 300, 250 ] ]
    },
    native: {
      // native config
    }
  },
  bids: [
    // banner ad slot
    { bidder: 'yieldlab', params: { adslotId: '1234', supplyId: '42' } },
    // native ad slot
    { bidder: 'yieldlab', params: { adslotId: '2345', supplyId: '42' } }
  ]
};

It would be amazing if this information could be made available in the docs PR as well ❤️

@kippsterr
Copy link
Contributor

Yes, that looks perfectly right.

To be honest, we do have revisiting the whole documentation part - for the adapter and for the website - on our list anyway. But I agree, that we should at least put the bare minimum for the multi-format part right now and can then still add more detail info later.

@kippsterr
Copy link
Contributor

Hi @muuki88,

we've added the multi-format documentation and also fixed the not so cross browser direct use of .find.

And can I ask you a question? Do you know why there's this constant amount of randomly failing Browserstack tests? It seems like whenever we fetch the latest upstream changes something else goes wrong and this PR is never going to get green.

@muuki88
Copy link
Collaborator

muuki88 commented Oct 28, 2021

Amazing 🎉 🎉 Thanks a lot.

And can I ask you a question? Do you know why there's this constant amount of randomly failing Browserstack tests?

I'm not too familiar with the browserstack tests, but the failing test seems that is has absolutely nothing to do with your changes. @ChrisHuie can you shed some light on this?

Chrome 79.0.3945.79 (Windows 10): Executed 3317 of 8618 (1 FAILED) (skipped 8) (11.985 secs / 4.627 secs)
Firefox 72.0 (Windows 10): Executed 8611 of 8618 (skipped 11) SUCCESS (30.35 secs / 16.112 secs)
Safari 14.1.2 (Mac OS 10.15.7): Executed 6765 of 8618 (skipped 11) SUCCESS (3.047 secs / 2.141 secs)
Safari 12.1.2 (Mac OS 10.14.6): Executed 8611 of 8618 (skipped 11) SUCCESS (22.991 secs / 15.787 secs)
TOTAL: 1 FAILED, 61747 SUCCESS


1) builds and sends data
     finteza analytics adapter track bid timeout
     AssertionError: expected 'POST' to equal 'GET'
    at Context.<anonymous> (webpack:///test/spec/modules/fintezaAnalyticsAdapter_spec.js:215:46 <- test/test_index.js:317509:143)

@aleksatr
Copy link
Contributor

@kippsterr rerunning the circleCI pipeline usually helps in this situation.

@kippsterr
Copy link
Contributor

Thanks! @aleksatr

@kippsterr rerunning the circleCI pipeline usually helps in this situation.

It seems like we cannot do that on our own, can we? We would just ping you in such cases then. Or did we miss something in the usual workflow which lead to the fact that we're not able to do it?

Alright, so whenever you're fine with the PR and all necessary approvals are given we're also fine with having the PR merged at any time.

Thank you guys for your support 🙇‍♂️

@aleksatr
Copy link
Contributor

@kippsterr to be honest I'm not sure, try to access https://app.circleci.com/pipelines/github/prebid/Prebid.js/8842/workflows/9d731327-92b0-42c9-a0e8-4eb7dd073f25/jobs/19097 (assuming you can login to circleCI with your github account)

@aleksatr aleksatr merged commit 388ddee into prebid:master Oct 29, 2021
@kippsterr
Copy link
Contributor

Yes, login to circleCI is working fine. Triggering reruns somehow doesn't because it's grayed out.
rerun grayed out
Well, for now the PR is merged and we're good. Let's try to figure out again next time. 🤷‍♂️

cpabst pushed a commit to sovrn/Prebid.js that referenced this pull request Jan 10, 2022
* YL-3989: Accept NATIVE yieldprobe response (#2)

* YL-3989: Accept NATIVE response

* Fix: 'utils' is not defined  no-undef

* trigger GitHub actions

* Add multi-format example to the Yieldlab bidder documentation

* Reformat code

* Fix: Object doesn't support 'find'

Object doesn't support property or method 'find' in IE 11

* trigger GitHub actions

* Chore:Replace `filter` by `find` from ..array/find.js

* Fix typo

Co-authored-by: Christoph <29540638+kippsterr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants