From 834c2d52fd180ab63360a9f7c5ef64a1fa260fc6 Mon Sep 17 00:00:00 2001 From: Matt Lane Date: Tue, 31 Oct 2017 11:48:37 -0700 Subject: [PATCH] Update dfp.buildVideoUrl to accept adserver url (#1663) * Update dfp.buildVideoUrl to accept adserver url * Reject invalid param usage * Don't overwrite description_url if cache is disabled and input contains description_url * Reject xml-only bids when cache is disabled * Accept both url and params object in function call * Fix conflict and nobid condition * Update docs and refactor based on code review --- modules/dfpAdServerVideo.js | 71 ++++++++++++++++++++-- src/utils.js | 2 +- src/video.js | 12 +++- test/spec/modules/dfpAdServerVideo_spec.js | 58 ++++++++++++++++++ test/spec/video_spec.js | 17 +++++- 5 files changed, 153 insertions(+), 7 deletions(-) diff --git a/modules/dfpAdServerVideo.js b/modules/dfpAdServerVideo.js index 4f56355a70c7..591b8f7baf32 100644 --- a/modules/dfpAdServerVideo.js +++ b/modules/dfpAdServerVideo.js @@ -4,8 +4,9 @@ import { registerVideoSupport } from '../src/adServerManager'; import { getWinningBids } from '../src/targeting'; -import { formatQS, format as buildUrl } from '../src/url'; -import { parseSizesInput } from '../src/utils'; +import { formatQS, format as buildUrl, parse } from '../src/url'; +import { deepAccess, isEmpty, logError, parseSizesInput } from '../src/utils'; +import { config } from '../src/config'; /** * @typedef {Object} DfpVideoParams @@ -31,8 +32,9 @@ import { parseSizesInput } from '../src/utils'; * @param [Object] bid The bid which should be considered alongside the rest of the adserver's demand. * If this isn't defined, then we'll use the winning bid for the adUnit. * - * @param {DfpVideoParams} params Query params which should be set on the DFP request. + * @param {DfpVideoParams} [params] Query params which should be set on the DFP request. * These will override this module's defaults whenever they conflict. + * @param {string} [url] video adserver url */ /** Safe defaults which work on pretty much all video calls. */ @@ -55,9 +57,26 @@ const defaultParamConstants = { * demand in DFP. */ export default function buildDfpVideoUrl(options) { + if (!options.params && !options.url) { + logError(`A params object or a url is required to use pbjs.adServers.dfp.buildVideoUrl`); + return; + } + const adUnit = options.adUnit; const bid = options.bid || getWinningBids(adUnit.code)[0]; + let urlComponents = {}; + + if (options.url) { + // when both `url` and `params` are given, parsed url will be overwriten + // with any matching param components + urlComponents = parse(options.url); + + if (isEmpty(options.params)) { + return buildUrlFromAdserverUrlComponents(urlComponents, bid); + } + } + const derivedParams = { correlator: Date.now(), sz: parseSizesInput(adUnit.sizes).join('|'), @@ -73,9 +92,14 @@ export default function buildDfpVideoUrl(options) { const queryParams = Object.assign({}, defaultParamConstants, + urlComponents.search, derivedParams, options.params, - { cust_params: encodeURIComponent(formatQS(customParams)) }); + { cust_params: encodeURIComponent(formatQS(customParams)) } + ); + + const descriptionUrl = getDescriptionUrl(bid, options, 'params'); + if (descriptionUrl) { queryParams.description_url = descriptionUrl; } return buildUrl({ protocol: 'https', @@ -85,6 +109,45 @@ export default function buildDfpVideoUrl(options) { }); } +/** + * Builds a video url from a base dfp video url and a winning bid, appending + * Prebid-specific key-values. + * @param {Object} components base video adserver url parsed into components object + * @param {AdapterBidResponse} bid winning bid object to append parameters from + * @return {string} video url + */ +function buildUrlFromAdserverUrlComponents(components, bid) { + const descriptionUrl = getDescriptionUrl(bid, components, 'search'); + if (descriptionUrl) { components.search.description_url = descriptionUrl; } + + const adserverTargeting = (bid && bid.adserverTargeting) || {}; + const customParams = Object.assign({}, + adserverTargeting, + ); + components.search.cust_params = encodeURIComponent(formatQS(customParams)); + + return buildUrl(components); +} + +/** + * Returns the encoded vast url if it exists on a bid object, only if prebid-cache + * is disabled, and description_url is not already set on a given input + * @param {AdapterBidResponse} bid object to check for vast url + * @param {Object} components the object to check that description_url is NOT set on + * @param {string} prop the property of components that would contain description_url + * @return {string | undefined} The encoded vast url if it exists, or undefined + */ +function getDescriptionUrl(bid, components, prop) { + if (config.getConfig('usePrebidCache')) { return; } + + if (!deepAccess(components, `${prop}.description_url`)) { + const vastUrl = bid && bid.vastUrl; + if (vastUrl) { return encodeURIComponent(vastUrl); } + } else { + logError(`input cannnot contain description_url`); + } +} + registerVideoSupport('dfp', { buildVideoUrl: buildDfpVideoUrl }); diff --git a/src/utils.js b/src/utils.js index 00a06fcb091e..9efa4f53c578 100644 --- a/src/utils.js +++ b/src/utils.js @@ -339,7 +339,7 @@ exports.isNumber = function(object) { */ exports.isEmpty = function (object) { if (!object) return true; - if (this.isArray(object) || this.isStr(object)) { + if (exports.isArray(object) || exports.isStr(object)) { return !(object.length > 0); } diff --git a/src/video.js b/src/video.js index 386b6b692e99..f5203e4b198d 100644 --- a/src/video.js +++ b/src/video.js @@ -1,5 +1,6 @@ import { videoAdapters } from './adaptermanager'; -import { getBidRequest, deepAccess } from './utils'; +import { getBidRequest, deepAccess, logError } from './utils'; +import { config } from '../src/config'; const VIDEO_MEDIA_TYPE = 'video'; const OUTSTREAM = 'outstream'; @@ -32,6 +33,15 @@ export function isValidVideoBid(bid) { // if context not defined assume default 'instream' for video bids // instream bids require a vast url or vast xml content if (!bidRequest || (videoMediaType && context !== OUTSTREAM)) { + // xml-only video bids require prebid-cache to be enabled + if (!config.getConfig('usePrebidCache') && bid.vastXml && !bid.vastUrl) { + logError(` + This bid contains only vastXml and will not work when prebid-cache is disabled. + Try enabling prebid-cache with pbjs.setConfig({ usePrebidCache: true }); + `); + return false; + } + return !!(bid.vastUrl || bid.vastXml); } diff --git a/test/spec/modules/dfpAdServerVideo_spec.js b/test/spec/modules/dfpAdServerVideo_spec.js index 3156c628abdf..07439be126ce 100644 --- a/test/spec/modules/dfpAdServerVideo_spec.js +++ b/test/spec/modules/dfpAdServerVideo_spec.js @@ -4,6 +4,7 @@ import parse from 'url-parse'; import buildDfpVideoUrl from 'modules/dfpAdServerVideo'; import { parseQS } from 'src/url'; import adUnit from 'test/fixtures/video/adUnit'; +import { newConfig } from 'src/config'; const bid = { videoCacheKey: 'abc', @@ -36,6 +37,43 @@ describe('The DFP video support module', () => { expect(queryParams).to.have.property('url'); }); + it('can take an adserver url as a parameter', () => { + const bidCopy = Object.assign({ }, bid); + bidCopy.vastUrl = 'vastUrl.example'; + + const url = parse(buildDfpVideoUrl({ + adUnit: adUnit, + bid: bidCopy, + url: 'https://video.adserver.example/', + })); + + expect(url.host).to.equal('video.adserver.example'); + + const queryObject = parseQS(url.query); + expect(queryObject.description_url).to.equal('vastUrl.example'); + }); + + it('requires a params object or url', () => { + const url = buildDfpVideoUrl({ + adUnit: adUnit, + bid: bid, + }); + + expect(url).to.be.undefined; + }); + + it('overwrites url params when both url and params object are given', () => { + const url = parse(buildDfpVideoUrl({ + adUnit: adUnit, + bid: bid, + url: 'https://video.adserver.example/ads?sz=640x480&iu=/123/aduniturl&impl=s', + params: { iu: 'my/adUnit' } + })); + + const queryObject = parseQS(url.query); + expect(queryObject.iu).to.equal('my/adUnit'); + }); + it('should override param defaults with user-provided ones', () => { const url = parse(buildDfpVideoUrl({ adUnit: adUnit, @@ -92,6 +130,26 @@ describe('The DFP video support module', () => { expect(customParams).to.have.property('my_targeting', 'foo'); }); + it('should not overwrite an existing description_url for object input and cache disabled', () => { + const config = newConfig(); + config.setConfig({ usePrebidCache: true }); + + const bidCopy = Object.assign({}, bid); + bidCopy.vastUrl = 'vastUrl.example'; + + const url = parse(buildDfpVideoUrl({ + adUnit: adUnit, + bid: bidCopy, + params: { + iu: 'my/adUnit', + description_url: 'descriptionurl.example' + } + })); + + const queryObject = parseQS(url.query); + expect(queryObject.description_url).to.equal('descriptionurl.example'); + }); + it('should work with nobid responses', () => { const url = buildDfpVideoUrl({ adUnit: adUnit, diff --git a/test/spec/video_spec.js b/test/spec/video_spec.js index 57a7f7a127e8..512b56c334f7 100644 --- a/test/spec/video_spec.js +++ b/test/spec/video_spec.js @@ -1,5 +1,6 @@ import { isValidVideoBid } from 'src/video'; -const utils = require('src/utils'); +import { newConfig } from 'src/config'; +import * as utils from 'src/utils'; describe('video.js', () => { afterEach(() => { @@ -34,6 +35,20 @@ describe('video.js', () => { expect(valid).to.be(false); }); + it('catches invalid bids when prebid-cache is disabled', () => { + sinon.stub(utils, 'getBidRequest', () => ({ + bidder: 'vastOnlyVideoBidder', + mediaTypes: { video: {} }, + })); + + const config = newConfig(); + config.setConfig({ usePrebidCache: false }); + + const valid = isValidVideoBid({ vastXml: 'vast' }); + + expect(valid).to.be(false); + }); + it('validates valid outstream bids', () => { sinon.stub(utils, 'getBidRequest', () => ({ bidder: 'appnexusAst',