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

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

Merged
merged 7 commits into from
Jun 26, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0ad6cfd
allow outstream video, remove parsePosition method, simplify code
moonshells Jun 5, 2018
f05e790
update media type validation rule
moonshells Jun 13, 2018
1c872b6
update media type validation rule
Jun 18, 2018
94e337b
video objetc bug fix
moonshells Jun 19, 2018
71c486c
cover no instream no outstream case
moonshells Jun 19, 2018
6200e66
add 'Rubicon bid adapter' as the prefix for log messages
moonshells Jun 25, 2018
2c8a204
Merge branch 'master' into outstream
moonshells Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 40 additions & 47 deletions modules/rubiconBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,32 +93,33 @@ export const spec = {
return false;
}

// Log warning if context is 'outstream', is not currently supported
if (utils.deepAccess(bid, `mediaTypes.${VIDEO}.context`) === 'outstream') {
utils.logWarn('Warning: outstream video for Rubicon Client Adapter is not supported yet');
}

// Log warning if mediaTypes contains both 'banner' and 'video'
if (spec.hasVideoMediaType(bid) && typeof utils.deepAccess(bid, `mediaTypes.${BANNER}`) !== 'undefined') {
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 (bid.mediaType === 'video' && typeof utils.deepAccess(bid, 'params.video.size_id') === 'undefined') {
return false;
}

// Bid is invalid if mediaTypes video is invalid and a mediaTypes banner property is not defined
if (bid.mediaTypes && !spec.hasVideoMediaType(bid) && typeof bid.mediaTypes.banner === 'undefined') {
if (hasVideoMediaType(bid)) {
// Log warning if mediaTypes contains both 'banner' and 'video'
if (utils.deepAccess(bid, `mediaTypes.${VIDEO}.context`) === 'instream') {
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');
}
// Bid is invalid if video is set but params video is missing size_id
if (typeof utils.deepAccess(bid, 'params.video.size_id') === 'undefined') {
utils.logError('Error: size id is missing for instream video request.');
return false;
}
return true;
} else if (utils.deepAccess(bid, `mediaTypes.${VIDEO}.context`) === 'outstream') {
if (utils.deepAccess(bid, 'params.video.size_id') !== 203) {
utils.logWarn('Warning: outstream video is sending invalid size id, converting size id to 203.');
}
return true;
}
// video context is neither instream nor outstream
utils.logError('Error: video context is neither instream nor outstream.');
return false;
}

let parsedSizes = parseSizes(bid);
if (parsedSizes.length < 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

} 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
Copy link
Collaborator

@robertrmartinez robertrmartinez Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

return false;
}

return true;
return parseSizes(bid).length > 0;
},
/**
* @param {BidRequest[]} bidRequests
Expand All @@ -128,7 +129,7 @@ export const spec = {
buildRequests: function (bidRequests, bidderRequest) {
// separate video bids because the requests are structured differently
let requests = [];
const videoRequests = bidRequests.filter(spec.hasVideoMediaType).map(bidRequest => {
const videoRequests = bidRequests.filter(hasVideoMediaType).map(bidRequest => {
bidRequest.startTime = new Date().getTime();

let params = bidRequest.params;
Expand All @@ -150,14 +151,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',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

};

if (params.inventory && typeof params.inventory === 'object') {
Expand Down Expand Up @@ -192,7 +193,7 @@ export const spec = {

if (config.getConfig('rubicon.singleRequest') !== true) {
// bids are not grouped if single request mode is not enabled
requests = videoRequests.concat(bidRequests.filter(bidRequest => !spec.hasVideoMediaType(bidRequest)).map(bidRequest => {
requests = videoRequests.concat(bidRequests.filter(bidRequest => !hasVideoMediaType(bidRequest)).map(bidRequest => {
const bidParams = spec.createSlotParams(bidRequest, bidderRequest);
return {
method: 'GET',
Expand All @@ -207,7 +208,7 @@ export const spec = {
} else {
// single request requires bids to be grouped by site id into a single request
// note: utils.groupBy wasn't used because deep property access was needed
const nonVideoRequests = bidRequests.filter(bidRequest => !spec.hasVideoMediaType(bidRequest));
const nonVideoRequests = bidRequests.filter(bidRequest => !hasVideoMediaType(bidRequest));
const groupedBidRequests = nonVideoRequests.reduce((groupedBids, bid) => {
(groupedBids[bid.params['siteId']] = groupedBids[bid.params['siteId']] || []).push(bid);
return groupedBids;
Expand Down Expand Up @@ -297,7 +298,7 @@ export const spec = {
'zone_id': params.zoneId,
'size_id': parsedSizes[0],
'alt_size_ids': parsedSizes.slice(1).join(',') || undefined,
'p_pos': parsePosition(params.position),
'p_pos': params.position === 'atf' || params.position === 'btf' ? params.position : 'unknown',
'rp_floor': (params.floor = parseFloat(params.floor)) > 0.01 ? params.floor : 0.01,
'rp_secure': isSecure() ? '1' : '0',
'tk_flint': INTEGRATION,
Expand Down Expand Up @@ -340,17 +341,6 @@ export const spec = {
return data;
},

/**
* Test if bid has mediaType or mediaTypes set for video.
* note: 'mediaType' has been deprecated, however support will remain for a transitional period
* @param {BidRequest} bidRequest
* @returns {boolean}
*/
hasVideoMediaType: function (bidRequest) {
return (typeof utils.deepAccess(bidRequest, 'params.video.size_id') !== 'undefined' &&
(bidRequest.mediaType === VIDEO || utils.deepAccess(bidRequest, `mediaTypes.${VIDEO}.context`) === 'instream'));
},

/**
* @param {*} responseObj
* @param {BidRequest|Object.<string, BidRequest[]>} bidRequest - if request was SRA the bidRequest argument will be a keyed BidRequest array object,
Expand All @@ -368,7 +358,7 @@ export const spec = {
let ads = responseObj.ads;

// video ads array is wrapped in an object
if (typeof bidRequest === 'object' && !Array.isArray(bidRequest) && spec.hasVideoMediaType(bidRequest) && typeof ads === 'object') {
if (typeof bidRequest === 'object' && !Array.isArray(bidRequest) && hasVideoMediaType(bidRequest) && typeof ads === 'object') {
ads = ads[bidRequest.adUnitCode];
}

Expand Down Expand Up @@ -504,7 +494,7 @@ function _renderCreative(script, impId) {

function parseSizes(bid) {
let params = bid.params;
if (spec.hasVideoMediaType(bid)) {
if (hasVideoMediaType(bid)) {
let size = [];
if (params.video && params.video.playerWidth && params.video.playerHeight) {
size = [
Expand All @@ -518,7 +508,7 @@ function parseSizes(bid) {
}

// deprecated: temp legacy support
let sizes = Array.isArray(params.sizes) ? params.sizes : mapSizes(bid.sizes)
let sizes = Array.isArray(params.sizes) ? params.sizes : mapSizes(bid.sizes);

return masSizeOrdering(sizes);
}
Expand All @@ -535,11 +525,14 @@ function mapSizes(sizes) {
}, []);
}

function parsePosition(position) {
if (position === 'atf' || position === 'btf') {
return position;
}
return 'unknown';
/**
* Test if bid has mediaType or mediaTypes set for video.
* note: 'mediaType' has been deprecated, however support will remain for a transitional period
* @param {BidRequest} bidRequest
* @returns {boolean}
*/
export function hasVideoMediaType(bidRequest) {
return bidRequest.mediaType === VIDEO || typeof utils.deepAccess(bidRequest, `mediaTypes.${VIDEO}`) !== 'undefined';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

}

export function masSizeOrdering(sizes) {
Expand Down
28 changes: 17 additions & 11 deletions test/spec/modules/rubiconBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {expect} from 'chai';
import adapterManager from 'src/adaptermanager';
import {spec, masSizeOrdering, resetUserSync} from 'modules/rubiconBidAdapter';
import {spec, masSizeOrdering, resetUserSync, hasVideoMediaType} from 'modules/rubiconBidAdapter';
import {parse as parseQuery} from 'querystring';
import {newBidder} from 'src/adapters/bidderFactory';
import {userSync} from 'src/userSync';
Expand Down Expand Up @@ -215,7 +215,7 @@ describe('the rubicon adapter', () => {
'p_aso.video.ext.skipdelay': 15,
'playerHeight': 320,
'playerWidth': 640,
'size_id': 201,
'size_id': 203,
'aeParams': {
'p_aso.video.ext.skip': '1',
'p_aso.video.ext.skipdelay': '15'
Expand Down Expand Up @@ -1197,7 +1197,9 @@ describe('the rubicon adapter', () => {
},
params: {
accountId: 1001,
video: {}
video: {
size_id: 201
}
},
sizes: [[300, 250]]
}
Expand Down Expand Up @@ -1254,13 +1256,17 @@ describe('the rubicon adapter', () => {
expect(spec.isBidRequestValid(bidderRequestCopy.bids[0])).to.equal(false);
});

it('should not validate bid request when video is outstream', () => {
it('bid request is valid when video context is outstream', () => {
createVideoBidderRequestOutstream();
sandbox.stub(Date, 'now').callsFake(() =>
bidderRequest.auctionStart + 100
);

expect(spec.isBidRequestValid(bidderRequest.bids[0])).to.equal(false);
const bidRequestCopy = clone(bidderRequest);

let [request] = spec.buildRequests(bidRequestCopy.bids, bidRequestCopy);
expect(spec.isBidRequestValid(bidderRequest.bids[0])).to.equal(true);
expect(request.data.slots[0].size_id).to.equal(203);
});

it('should get size from bid.sizes too', () => {
Expand Down Expand Up @@ -1359,12 +1365,12 @@ describe('the rubicon adapter', () => {
describe('hasVideoMediaType', () => {
it('should return true if mediaType is video and size_id is set', () => {
createVideoBidderRequest();
const legacyVideoTypeBidRequest = spec.hasVideoMediaType(bidderRequest.bids[0]);
const legacyVideoTypeBidRequest = hasVideoMediaType(bidderRequest.bids[0]);
expect(legacyVideoTypeBidRequest).is.equal(true);
});

it('should return false if mediaType is video and size_id is not defined', () => {
expect(spec.hasVideoMediaType({
expect(spec.isBidRequestValid({
bid: 99,
mediaType: 'video',
params: {
Expand All @@ -1374,17 +1380,17 @@ describe('the rubicon adapter', () => {
});

it('should return false if bidRequest.mediaType is not equal to video', () => {
expect(spec.hasVideoMediaType({
expect(hasVideoMediaType({
mediaType: 'banner'
})).is.equal(false);
});

it('should return false if bidRequest.mediaType is not defined', () => {
expect(spec.hasVideoMediaType({})).is.equal(false);
expect(hasVideoMediaType({})).is.equal(false);
});

it('should return true if bidRequest.mediaTypes.video.context is instream and size_id is defined', () => {
expect(spec.hasVideoMediaType({
expect(hasVideoMediaType({
mediaTypes: {
video: {
context: 'instream'
Expand All @@ -1399,7 +1405,7 @@ describe('the rubicon adapter', () => {
});

it('should return false if bidRequest.mediaTypes.video.context is instream but size_id is not defined', () => {
expect(spec.hasVideoMediaType({
expect(spec.isBidRequestValid({
mediaTypes: {
video: {
context: 'instream'
Expand Down