allow outstream video, remove parsePosition method, simplify code #2683

moonshells · 2018-06-05T22:17:44Z

Type of change

Feature
Refactoring (no functional changes, no api changes)

Description of change

For outstream video, Rubicon adapter will return video ads response instead of marking the bid request as invalid, so publishers can serve outstream ads with Rubicon when they provide ad unit level renderer. This PR also removes parsePosition method and simplifies code.

snapwich · 2018-06-06T18:19:31Z

modules/rubiconBidAdapter.js

-    }
-
-    let parsedSizes = parseSizes(bid);
-    if (parsedSizes.length < 1) {


@idettman can you review this? not sure if this is undoing the work you've done recently with the mediaTypes.

snapwich · 2018-06-06T18:21:28Z

modules/rubiconBidAdapter.js

@@ -150,14 +139,14 @@ export const spec = {
      let slotData = {
        site_id: params.siteId,
        zone_id: params.zoneId,
-        position: parsePosition(params.position),
+        position: params.position === 'atf' || params.position === 'btf' ? params.position : 'unknown',


why undo this? I think this logic was purposely moved into parsePosition since its shared with the p_pos logic below.

I'm trying to follow openRTB standard and remove parsePosition for video: after the video endpoint migrated to PBS Java openRTB, the new endpoint runs auction with OpenRTB 2.5 bid request, which uses integer values for ads position.

idettman

LGTM, with the caveat that we should have Bobby run his tests to make sure it behaves the same.

robertrmartinez · 2018-06-11T21:40:43Z

modules/rubiconBidAdapter.js

+    if (hasVideoMediaType(bid)) {
+      // Log warning if mediaTypes contains both 'banner' and 'video'
+      if (typeof utils.deepAccess(bid, `mediaTypes.${BANNER}`) !== 'undefined') {
+        utils.logWarn('Warning: instream video and banner requested for same ad unit, continuing with video instream request');


Remove instream -> As of now this is logged when it has outstream as well, so is misleading.

** Actually, instead, we may want to alter this check actually. **

We could just add a check for if the video is instream with typeof utils.deepAccess(bid, mediaTypes.${BANNER}) !== 'undefined' && utils.deepAccess(bid, mediaTypes.${VIDEO}.context) === 'instream'

robertrmartinez · 2018-06-11T21:49:06Z

modules/rubiconBidAdapter.js

        floor: parseFloat(params.floor) > 0.01 ? params.floor : 0.01,
        element_id: bidRequest.adUnitCode,
        name: bidRequest.adUnitCode,
        language: params.video.language,
        width: size[0],
        height: size[1],
-        size_id: params.video.size_id
+        size_id: utils.deepAccess(bidRequest, `mediaTypes.${VIDEO}.context`) === 'outstream' ? 203 : params.video.size_id


We are setting the size_id to be 203 no matter what if it is outstream,

Above at line 102 we reject a video bid request if it does not contain params.video.size_id

So are we requiring that anyone who uses outstream with us place size_id in the video params field?

Looks to me like if we are just always changing it to 203 ourselves, we should not have this strict check above?

robertrmartinez · 2018-06-11T22:17:07Z

modules/rubiconBidAdapter.js

+        utils.logWarn('Warning: instream video and banner requested for same ad unit, continuing with video instream request');
+      }
+      // Bid is invalid if legacy video is set but params video is missing size_id
+      if (typeof utils.deepAccess(bid, 'params.video.size_id') === 'undefined') {


If the bid is outstream we are later defaulting it to be size_id 203 later anyways.

Let's make this check strictly for instream, and if it is outstream without a size_id, we can just let it defualt itself to 203 like it does on line 149

robertrmartinez · 2018-06-14T00:36:58Z

modules/rubiconBidAdapter.js

+ * @returns {boolean}
+ */
+export function hasVideoMediaType(bidRequest) {
+  return bidRequest.mediaType === VIDEO || typeof utils.deepAccess(bidRequest, `mediaTypes.${VIDEO}`) !== 'undefined';


So bidRequest.mediaType === VIDEO is supporting legacy way of declaring video bid.

But in the Validations we return false if mediaTypes.video.context is neither instream or outstream? (Line 115)

Am I missing something or would a legacy video bid where mediaType == 'video' no longer ever work because of this?

robertrmartinez · 2018-06-14T00:40:34Z

modules/rubiconBidAdapter.js

-    let parsedSizes = parseSizes(bid);
-    if (parsedSizes.length < 1) {
+    } else if (bid.mediaTypes && typeof utils.deepAccess(bid, `mediaTypes.${BANNER}`) === 'undefined') {
+      // Bid is invalid if mediaTypes video is invalid and a mediaTypes banner property is not defined


mediaTypes.banner is not required, if there are no mediaTypes objects then we want to just default to banner. Not exit out of the bid request.

Let me know if I am missing something, but I would think we want to still continue with banner if mediaTypes is declared but banner is not.

adUnit.sizes will eventually be deprecated, but we still are allowing it for now, and should continue to do so as long as Prebid Core does.

…ebid#2683) * allow outstream video, remove parsePosition method, simplify code * update media type validation rule * update media type validation rule * video objetc bug fix * cover no instream no outstream case * add 'Rubicon bid adapter' as the prefix for log messages

allow outstream video, remove parsePosition method, simplify code

0ad6cfd

jsnellbaker assigned ndhimehta Jun 6, 2018

jsnellbaker added the needs review label Jun 6, 2018

ndhimehta requested a review from snapwich June 6, 2018 18:13

ndhimehta assigned snapwich and unassigned ndhimehta Jun 6, 2018

snapwich requested a review from idettman June 6, 2018 18:18

snapwich reviewed Jun 6, 2018

View reviewed changes

snapwich requested changes Jun 6, 2018

View reviewed changes

idettman approved these changes Jun 11, 2018

View reviewed changes

robertrmartinez reviewed Jun 11, 2018

View reviewed changes

update media type validation rule

f05e790

robertrmartinez reviewed Jun 14, 2018

View reviewed changes

zma and others added 3 commits June 18, 2018 12:54

update media type validation rule

1c872b6

video objetc bug fix

94e337b

cover no instream no outstream case

71c486c

snapwich approved these changes Jun 25, 2018

View reviewed changes

moonshells and others added 2 commits June 25, 2018 12:18

add 'Rubicon bid adapter' as the prefix for log messages

6200e66

Merge branch 'master' into outstream

2c8a204

snapwich merged commit 0c4b815 into prebid:master Jun 26, 2018

moonshells deleted the outstream branch June 26, 2018 16:58

lanceloti mentioned this pull request Aug 9, 2018

Rubicon Outstream: Ignoring bid: Video bid does not have required vastUrl or renderer property #2957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow outstream video, remove parsePosition method, simplify code #2683

allow outstream video, remove parsePosition method, simplify code #2683

moonshells commented Jun 5, 2018

snapwich Jun 6, 2018

snapwich Jun 6, 2018

moonshells Jun 6, 2018

idettman left a comment

robertrmartinez Jun 11, 2018

robertrmartinez Jun 11, 2018 •

edited

Loading

robertrmartinez Jun 11, 2018

robertrmartinez Jun 11, 2018

robertrmartinez Jun 14, 2018

robertrmartinez Jun 14, 2018 •

edited

Loading

robertrmartinez Jun 14, 2018

allow outstream video, remove parsePosition method, simplify code #2683

allow outstream video, remove parsePosition method, simplify code #2683

Conversation

moonshells commented Jun 5, 2018

Type of change

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idettman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez Jun 11, 2018 • 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

robertrmartinez Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez Jun 11, 2018 •

edited

Loading

robertrmartinez Jun 14, 2018 •

edited

Loading