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

Rubicon Bid Adapter: Choose video when banner mediaType is not present #8997

Merged
merged 7 commits into from
Sep 20, 2022

Conversation

spotxslagle
Copy link
Contributor

Type of change

  • [ X ] Bugfix

Description of change

Rubicon's Adapter only classifies a bid as video when mediaTypes.video and params.video are both defined. This change causes the adapter to only check against params.video when mediaTypes.banner is also present, otherwise mediaTypes.video is enough to classify as video

Other information

@robertrmartinez

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Looks good, will you add a unit test covering classifiedAsVideo.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Actually the unit tests are failing, so please fix and add a couple for the new classifiedAsVideo

@@ -1990,6 +1990,19 @@ describe('the rubicon adapter', function () {
expect(spec.isBidRequestValid(bidderRequest.bids[0])).to.equal(false);
});

it('should be invalid if bidRequest.mediaTypes.video.context is instream but size_id is not defined', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is the check for this exactly? I do not see size_id being checked if instream anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test used to be under the hasVideoType tests, but it doesn't actually have anything to do with that function. I just moved it to where a bunch of other spec.isBidRequestValid situations were being tested. No idea if it tests what it is supposed to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah seems like a legacy test that does not do what is intended!

It is failing because we require

mediaTypes.video object to have a couple things.

Which is covered in should enforce the new required mediaTypes.video params

Can you just delete this test please!

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Is there a test for when BOTH banner and VIDEO are present

But also params.video object is defined?

Thus, making it classify as video?

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

LGTM! @ChrisHuie Will you do a non Magnite review on this please?!

@ChrisHuie ChrisHuie merged commit dd0f8ac into prebid:master Sep 20, 2022
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
prebid#8997)

* Choose video when banner mediaType is not present

* Define video.params as needed

* Refactor unit tests for classifiedAsVideo

* Unit test fixes

* Add video+banner+params.video test

* Remove obsolete test
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
prebid#8997)

* Choose video when banner mediaType is not present

* Define video.params as needed

* Refactor unit tests for classifiedAsVideo

* Unit test fixes

* Add video+banner+params.video test

* Remove obsolete test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants