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

Jump to bottom

Add support for backupOnly option in mediaType video renderer #5972

Merged
merged 2 commits into from
Nov 25, 2020
Merged

Add support for backupOnly option in mediaType video renderer #5972

merged 2 commits into from
Nov 25, 2020

Conversation

osazos
Copy link
Collaborator

@osazos osazos commented Nov 13, 2020

Type of change

  • Bugfix

Description of change

When a publisher set a Renderer for an adUnit (outstream context), the official documentation explains the publisher can set backupOnly: true to prefer the buyer or adapter renderer.

This options only works when the renderer is defined at the "adUnit" level.

This PR just add the support for the backupOnly option at the "mediaTypes.video" level.

FilipStamenkovic
FilipStamenkovic suggested changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

MR looks ok, can you just refactor condition I already mention in one of the comments?

Also, do we have some example page to test this, or some documentation? I tried testing it on my local by creating some custom html pages, but I couldn't do it.

src/Renderer.js Outdated
Comment on lines 118 to 134
return !!(adUnit &&
(
(adUnit.renderer && adUnit.renderer.url && adUnit.renderer.render && !(utils.isBoolean(adUnit.renderer.backupOnly) && adUnit.renderer.backupOnly)) ||
(adUnit.mediaTypes && adUnit.mediaTypes.video && adUnit.mediaTypes.video.renderer.url && adUnit.mediaTypes.video.renderer.render &&
!(utils.isBoolean(adUnit.mediaTypes.video.renderer.backupOnly) && adUnit.mediaTypes.video.renderer.backupOnly))
)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break down this?

It is not readable, I think breaking it down in multiple ifs would help. It took me a lot of time to decode this entire condition.

Just a small hint, you can replace:

utils.isBoolean(adUnit.renderer.backupOnly) && adUnit.renderer.backupOnly

with:

adUnit.renderer.backupOnly === true

src/auction.js Outdated
@@ -533,7 +533,7 @@ function getPreparedBidForAuction({adUnitCode, bid, bidderRequest, auctionId}) {
var renderer = null;

// the renderer for the mediaType takes precendence
if (mediaTypeRenderer && mediaTypeRenderer.url && mediaTypeRenderer.render) {
if (mediaTypeRenderer && mediaTypeRenderer.url && !(mediaTypeRenderer.backupOnly && isBoolean(mediaTypeRenderer.backupOnly) && mediaTypeRenderer.render)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same hint here, you can replace:

mediaTypeRenderer.backupOnly && isBoolean(mediaTypeRenderer.backupOnly)

with:

mediaTypeRenderer.backupOnly === true

You don't have to change it, I'm just making a suggestion how to make this if condition a bit shorter.

@osazos osazos force-pushed the renderer-backuponly-fix branch 2 times, most recently from 2a20b4b to 993ad37 Compare November 17, 2020 15:50
@osazos
Copy link
Collaborator Author

osazos commented Nov 17, 2020

@FilipStamenkovic, regarding your comment I tried to improve readability especially in the isRendererPreferredFromAdUnit function which was really unreadable you right.

Please let me know.

@osazos
Copy link
Collaborator Author

osazos commented Nov 18, 2020

I didn't notice about your ask for documentation. In fact the documentation already exists : https://docs.prebid.org/dev-docs/show-outstream-video-ads.html#renderers

Renderers are associated with adUnits in two ways. Primarily through the adUnit.renderer object. But also, especially for multiFormat adUnits, through the specified mediaType adUnit.mediaTypes.video.renderer. This object contains these fields:
[...]
backupOnly – Optional field, if set to true, buyer or adapter renderer will be preferred

About testing, I didn't found a test page for an outstream video adUnit that I could extends. If you really need it I can try to build a full page for it, but I'm not sure it is really usefull as it depends of adapters specificities.

FilipStamenkovic
FilipStamenkovic approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

LGTM

@FilipStamenkovic FilipStamenkovic added the needs 2nd review Core module updates require two approvals from the core team label Nov 18, 2020
@osazos osazos force-pushed the renderer-backuponly-fix branch from 993ad37 to 2ffe71c Compare November 25, 2020 07:28
@Fawke Fawke requested review from Fawke and removed request for jsnellbaker November 25, 2020 15:32
Fawke
Fawke approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@Fawke Fawke left a comment

Choose a reason for hiding this comment

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

Thanks for this bugfix. This change looks good.

@Fawke Fawke added LGTM and removed needs 2nd review Core module updates require two approvals from the core team labels Nov 25, 2020
@Fawke Fawke merged commit 939c455 into prebid:master Nov 25, 2020
@osazos osazos deleted the renderer-backuponly-fix branch November 26, 2020 09:56
@aleksatr aleksatr mentioned this pull request Mar 15, 2021
Merged
8 tasks
stsepelin pushed a commit to cointraffic/Prebid.js that referenced this pull request May 28, 2021
@osazos @stsepelin
Add support for backupOnly option in mediaType video renderer (preb…
bfc1760 
…id#5972)

* Add support for `backupOnly` option in mediaType video renderer

* Improve readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fawke Fawke Fawke approved these changes

@FilipStamenkovic FilipStamenkovic FilipStamenkovic approved these changes

Assignees

@FilipStamenkovic FilipStamenkovic

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@osazos @FilipStamenkovic @Fawke