Document pbjs.adServers.dfp.buildVideoUrl update to accept ad server URL
#422
Conversation
There was a problem hiding this comment.
Looks good Rich, left a few comments.
I'm a little confused about the interaction of cache with URL usage - sounds like the big thing is that the bidder must return a vastUrl if cache is disabled, right? Any thoughts on how to make that explanation simpler, or just a comment on how you tend to think about how it works
That's correct, if cache is disabled the bid must contain a vastUrl to work properly. Going to think about the second part of the question and will get back to you on it, wanted to post the first set of comments for now
Update: right -- with prebid-cache enabled, and when a url is passed to pbjs.adServers.dfp.buildVideoUrl, Prebid does not set the description_url to the bid.vastUrl, it only attaches the bid's adserver targeting, rebuilds the url based on the input, and returns that. For url inputs, Prebid sets description_url to the bid.vastUrl when prebid-cache is disabled.
dev-docs/publisher-api-reference.md
Outdated
|
|
||
| The `options.params` object contains the parameters needed to form a URL which hits the [DFP API](https://support.google.com/dfp_premium/answer/1068325?hl=en). It has the following fields: | ||
|
|
||
| <span style="color: rgb(255,0,0);">FIXME</span>: is "arbitraryKey" still supported? not seeing anything explicitly mentioning it in the code at a glance, but may have missed it. It's also not mentioned in the JSDoc. |
There was a problem hiding this comment.
As far as I can tell, no. I hadn't seen usage of this before, and it seems like sending custom key-value pairs is covered by the cust_params field, so possibly cust_params is a replacement for that
dev-docs/publisher-api-reference.md
Outdated
|
|
||
| The table below lists the expected behavior of this method when called with various configurations. | ||
|
|
||
| <span class="FIXME" style="color: rgb(255,0,0);">FIXME</span>: In the PR comment (https://github.com/prebid/Prebid.js/pull/1663) options.params with cache DISABLED is not mentioned. Does it work? |
There was a problem hiding this comment.
Yes, if the correct adserver adunit id is used. In our example case, /19968336/prebid_cache_video_adunit is the adunit id used when prebid-cache is enabled, which is correct. If prebid-cache is disabled, that same adunit id will no longer work, but /19968336/prebid_video_adunit will because it is expecting a vast url rather than a prebid-cache key
dev-docs/publisher-api-reference.md
Outdated
|
|
||
| <span class="FIXME" style="color: rgb(255,0,0);">FIXME</span>: In the PR comment (https://github.com/prebid/Prebid.js/pull/1663) options.params with cache DISABLED is not mentioned. Does it work? | ||
|
|
||
| <span style="color: rgb(255,0,0);">FIXME</span>: are updates to video bidder adapter docs required? e.g., to note that they must always supply a <code>vastUrl</code> as described at https://corpwiki.appnexus.com/x/8jWcBw |
There was a problem hiding this comment.
Video bidders must supply either vastUrl or vastXml. They may supply both. If the video bidder supplies only vastXml and prebid-cache is disabled, this will not work and the bid will be dropped from the auction (if prebid-cache is enabled, this same scenario is fine). If a video bidder supplies neither vastUrl or vastXml, the bid is dropped.
dev-docs/publisher-api-reference.md
Outdated
| |---------------------------------------------+-----------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | `options.params` only | Yes | If you set `description_url`, we use that; otherwise this method will automagically build it for you. | | ||
| | `options.params` only | No | Bidder must respond with **only** `bid.vastUrl` - `bid.vastXml` won't work. | | ||
| | `options.params` and `options.url` together | Yes | DOES NOT WORK (?) according to https://corpwiki.appnexus.com/x/8jWcBw - or is it simply the case that the bidder must provide `bid.vastUrl` (as below)? | |
There was a problem hiding this comment.
Correct, Prebid doesn't set a description_url in this case
dev-docs/publisher-api-reference.md
Outdated
| | Argument | Prebid Cache Enabled? | Notes | | ||
| |---------------------------------------------+-----------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | `options.params` only | Yes | If you set `description_url`, we use that; otherwise this method will automagically build it for you. | | ||
| | `options.params` only | No | Bidder must respond with **only** `bid.vastUrl` - `bid.vastXml` won't work. | |
There was a problem hiding this comment.
The bidder must contain bid.vastUrl, correct, but if there is also a bid.vastXml it will still work, the xml just wouldn't be used.
(Also: some edits for brevity/clarity/etc.)
dev-docs/publisher-api-reference.md
Outdated
| The table below lists the expected behavior of this method when called with various combinations of arguments and cache usage. | ||
|
|
||
| {: .table .table-bordered .table-striped } | ||
| | Argument | Prebid Cache Enabled? | Notes | |
There was a problem hiding this comment.
@matthewlane based on your comment update re: how/when description_url gets built, I added some more info to this table. I also added two more lines for the "only using options.url cases with cache on/off.
Are the descriptions of what happens in each of these cases correct?
There was a problem hiding this comment.
@rmloveland One update for the notes in options.url only, cache enabled
Prebid attaches the bid’s adserver targeting and rebuilds the URL based on the input by merging publisher input with default values. It does not set description_url to bid.vastUrl.
default values aren't merged in this case, the resulting url is just based off the input url with adserver targeting attached.
All other descriptions look good to me
dev-docs/publisher-api-reference.md
Outdated
| |--------+--------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | adUnit | object | (Required) The Prebid adUnit to which the returned URL will map. | | ||
| | bid | object | (Optional) The Prebid bid for which targeting will be set. If this is not defined, Prebid will use the bid with the highest CPM for the adUnit. | | ||
| | params | object | (Required) Querystring parameters that will be used to construct the DFP video ad tag URL. Publisher-supplied values will override values set by Prebid.js. See below for fields. | |
There was a problem hiding this comment.
I don't think that Options.params is required. It should be acceptable to invoke buildVideoUrl with an Options object that contains either the "url" or "params" field.
@matthewlane can we confirm the expected behavior if both Options.params and Options.url are passed to buildVideoUrl?
There was a problem hiding this comment.
Right, options.params isn't required, the pub can pass either options.params, options.url, or both, but not neither.
If both params and url are passed in, the url is parsed, and any equivalent params that are present will overwrite those that were present in the url (in other words, in the event of collisions, params takes precedence). Then everything else that happens when just params were passed in still happens (adding defaults, adserver targeting, etc.), everything is stringified back to a url, and this is returned
dev-docs/publisher-api-reference.md
Outdated
|
|
||
| #### Usage scenarios and expected behavior | ||
|
|
||
| How you call `dfp.buildVideoUrl` may change depending on: |
There was a problem hiding this comment.
I think we need to rework or possibly eliminate this section. Options.params and Options.url are not really intended for distinct sets of use-cases. We allow users to either pass an existing adserver URL or an object containing adserver URL querystring parameters for convenience.
dev-docs/publisher-api-reference.md
Outdated
|
|
||
| {: .alert.alert-info :} | ||
| **Bid Responses and Prebid Cache** | ||
| Video bidders must supply either `bid.vastUrl` or `bid.vastXml`. They may supply both. If the video bidder supplies *only* `bid.vastXml` and Prebid Cache is disabled, this will not work and the bid will be dropped from the auction (if Prebid Cache is enabled, this same scenario is fine). If a video bidder supplies neither `bid.vastUrl` or `bid.vastXml`, the bid is dropped. |
There was a problem hiding this comment.
I think we can get rid of the last sentence: "If a video bidder supplies neither bid.vastUrl or bid.vastXml, the bid is dropped.", as it seems repetitive.
dev-docs/publisher-api-reference.md
Outdated
|
|
||
| The table below lists the expected behavior of this method when called with various combinations of arguments and cache usage. | ||
|
|
||
| {: .table .table-bordered .table-striped } |
There was a problem hiding this comment.
I think we need to figure out how to represent more clearly the information in the table below, as it seems a bit confusing to me at the moment.
Was originally going to add this info in #422 but it fits better here.
Was originally going to add this info in #422 but it fits better here.
Relates to prebid/Prebid.js#1663
@matthewlane Would you mind taking a look at these docs? They still need some refinement and shortening. I'm trying to figure out how to make them shorter given the interaction of
options.paramsvs.options.urlvs. Prebid Cache being enabled or not.In the meantime, I'd appreciate if you could take a look! Specifically:
vastUrlif cache is disabled, right? Any thoughts on how to make that explanation simpler, or just a comment on how you tend to think about how it works, would be really appreciated :-)Thanks!