Update dfp.buildVideoUrl to accept adserver url#1663
Conversation
302fcfd to
d4905de
Compare
dbemiller
left a comment
There was a problem hiding this comment.
Looks sensible to me, barring one open question.
modules/dfpAdServerVideo.js
Outdated
| } | ||
|
|
||
| if (options.params && options.url) { | ||
| logError(`Passing both a params object and a url to pbjs.adServers.dfp.buildVideoUrl is invalid.`); |
There was a problem hiding this comment.
Is there any reason we aren't supporting this? It seems legitimate to parse the URL, overwrite any params, stringify the URL, and then return it.
0d97096 to
349792f
Compare
…ns description_url
8a36160 to
9e6d46f
Compare
dbemiller
left a comment
There was a problem hiding this comment.
Looks good overall... few recommendations. Mostly about docs.
| // 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) { |
There was a problem hiding this comment.
very good idea. This makes the cache & no-cache workflows much more smooth... I'm a little embarrassed I never thought of it.
| * demand in DFP. | ||
| */ | ||
| export default function buildDfpVideoUrl(options) { | ||
| if (!options.params && !options.url) { |
There was a problem hiding this comment.
Looks like the JSDoc types need some updating for this. The DfpVideoOptions still says that params is required, and doesn't mention url at all.
modules/dfpAdServerVideo.js
Outdated
| * 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 {Object} bid winning bid object to append parameters from |
There was a problem hiding this comment.
Thanks to @rmloveland's documentation in src/prebid.js, you can call this an AdapterBidResponse now.
modules/dfpAdServerVideo.js
Outdated
| } else { | ||
| logError(`input url cannnot contain description_url`); | ||
| } | ||
| } |
There was a problem hiding this comment.
This description_url validation block is basically copy/paste between buildDfpVideoUrl and buildUrlFromAdserverUrlComponents... would be more maintainable in a helper method.
There was a problem hiding this comment.
Looks good.
There's one more thing I should point out about the Types... but I don't think it should hold up the PR or anything. Almost all the programmers I've worked with in loosely-typed languages do the same thing you did.
If the DfpVideoParams has optional params and url properties, then that type implies that "both" or "neither" are also acceptable. But... what you really mean here is "must have one or the other, but not both."
In this case... it's probably better spec'd out as two types. One requires params, and the other requires url. The buildDfVideoUrl would accept either one (in JSDoc, {type1|type2}).
In any language, the golden standard for good types is "rejects all args which cause this function to error, and accept all args that allow this function to do its job."
* 'master' of https://github.com/prebid/Prebid.js: Increment pre version Prebid 0.32.0 Release Commenting out tests that are failing in IE10 (prebid#1710) Update dfp.buildVideoUrl to accept adserver url (prebid#1663) Update rubicon adapter with new properties and 1.0 changes (prebid#1776) Added adUnitCode for compatibility (prebid#1781) Remove 'supported' from analytics adapter info (prebid#1780) Add TTL parameter to bid (prebid#1784)
* tag '0.32.0' of https://github.com/prebid/Prebid.js: (44 commits) Prebid 0.32.0 Release Commenting out tests that are failing in IE10 (prebid#1710) Update dfp.buildVideoUrl to accept adserver url (prebid#1663) Update rubicon adapter with new properties and 1.0 changes (prebid#1776) Added adUnitCode for compatibility (prebid#1781) Remove 'supported' from analytics adapter info (prebid#1780) Add TTL parameter to bid (prebid#1784) Update GetIntent adapter to 1.0 version (prebid#1721) Add `usePaymentRule` param to AN bidders (prebid#1778) New hooks API (replaces monkey-patching for currency) (prebid#1683) Change prebidServer to call client user syncs if they exist (prebid#1734) Fix Centro adapter to allow requests of the same units (prebid#1746) add vastUrl + media type for video bids Prebid Server (prebid#1739) Update adxcg adapter for prebid 1.0 (prebid#1741) Update yieldmoBid adapter request url (prebid#1771) Upgrade Quantcast adapter for Prebid 1.0 (prebid#1753) Fidelity Media Adapter update. Prebid v1.0 (prebid#1719) Kargo Adapter for Prebid 1.0 (prebid#1729) updated for prebid 1.0 api (prebid#1722) Add AdOcean adapter (prebid#1735) ...
….32.0 to aolgithub-master * commit '7a956351d69ed3a54ec31aaef17a36441be16fbd': (58 commits) Fixed wrong contentType and customHeader options. Fixed aol adapter bad merge (accepted official changes) Fixed issue with invalid webpack module. Added partners ids. Added changelog entry. Refactored interpretResponse tests for Prebid 1.0. Prebid 0.32.0 Release Commenting out tests that are failing in IE10 (prebid#1710) Update dfp.buildVideoUrl to accept adserver url (prebid#1663) Update rubicon adapter with new properties and 1.0 changes (prebid#1776) Added adUnitCode for compatibility (prebid#1781) Remove 'supported' from analytics adapter info (prebid#1780) Add TTL parameter to bid (prebid#1784) Refactored get userSyncs tests for Prebid 1.0. Fixed faining tests for One Mobile endpoint. Refactored One Mobile tests for Prebid 1.0 Refactored Marketplace tests for Prebid 1.0 Implemented AOL user syncs config via setConfig API. Move one mobile post properties to options object. Update GetIntent adapter to 1.0 version (prebid#1721) ...
Updates
pbjs.adServer.dfp.buildVideoUrlto accept a video adserver url (similar to deprecatedpbjs.buildMasterVideoTagFromAdserverTagfunction) with aurlparameter on the input object:Also updates the function to work without prebid cache enabled when the input contains an object of querystring parameters by appending
description_urlto the output url: