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

Jump to bottom

Drop non-video bidders from video ad units #1815

Merged
merged 3 commits into from
Nov 14, 2017
Merged

Drop non-video bidders from video ad units #1815

merged 3 commits into from
Nov 14, 2017

Conversation

matthewlane
Copy link
Collaborator

@matthewlane matthewlane commented Nov 8, 2017 •
edited
Loading

Type of change

  • Bugfix

Description of change

Ad units with mediaType: 'video' that included non-video bidders were preventing calls to all bidders in the ad unit, even if that included video bidders. This change drops non-video bidders and still calls valid video bidders rather than dropping the ad unit entirely. Works for ad units configured with mediaTypes: 'video' or 1.0-compatible meidaTypes: {video: {}}

In this example ad unit, on master appnexusAst will not get called because fakeBidder is not a video bidder, or even real for that matter. With this PR appnexusAst gets called.

pbjs.addAdUnits({
  code: '/19968336/prebid_video_adunit',
  sizes: [640,480],
  mediaType: 'video',
  // or mediaTypes: { video: {context: 'instream'} },
  bids: [

    {
      bidder: 'appnexusAst',
      params: {
        placementId: '9333431',
        video: { skippable: true, playback_method: ['auto_play_sound_off'] }
      }
    },

    {
      bidder: 'fakeBidder',
      params: { tag: 'stopHammerTime' }
    },

  ]
});

Other information

Fixes #1137

mkendall07
mkendall07 reviewed Nov 8, 2017
View reviewed changes
src/prebid.js Outdated
@@ -383,7 +383,7 @@ $$PREBID_GLOBAL$$.requestBids = function ({ bidsBackHandler, timeout, adUnits, a
// for video-enabled adUnits, only request bids if all bidders support video
const invalidVideoAdUnits = adUnits.filter(videoAdUnit).filter(hasNonVideoBidder);
invalidVideoAdUnits.forEach(adUnit => {
utils.logError(`adUnit ${adUnit.code} has 'mediaType' set to 'video' but contains a bidder that doesn't support video. No Prebid demand requests will be triggered for this adUnit.`);
utils.logError(`adUnit ${adUnit.code} is of type 'video' but contains a bidder that doesn't support video. No Prebid demand requests will be triggered for this adUnit.`);
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that we call the demand that is enabled for video instead of just dropping it completely. Is it hard to make that change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I can do it. I'll update the PR to do that when either mediaType or mediaTypes is used to configure 'video'

@matthewlane matthewlane changed the title Check mediaTypes when validating video bidders in ad unit Drop non-video bidders from video ad units Nov 9, 2017
@mkendall07 mkendall07 added the LGTM label Nov 9, 2017
@jaiminpanchal27 jaiminpanchal27 merged commit 8f9df5b into master Nov 14, 2017
@matthewlane matthewlane deleted the bugfix/check-video-mediatypes branch November 14, 2017 15:18
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Nov 21, 2017
Merge remote-tracking branch 'unstream/master'
77e66d3 
* unstream/master: (36 commits)
  + Add Optimatic Bid Adapter (prebid#1837)
  Add Bridgewell adapter (prebid#1825)
  Kumma adapter updated for Prebid 1.0 (prebid#1766)
  Touchup add bid response (prebid#1822)
  Fix skipped test (prebid#1836)
  Added new size in Rubicon pbjs Adapter (prebid#1842)
  HuddledMasses header bidding adapter (prebid#1806)
  Increment pre version
  Prebid 0.33.0 Release
  Update AOL adapter for v1.0  (prebid#1693)
  Sovrn 1.0 compliance (prebid#1796)
  Platform.io Bidder Adapter update (prebid#1817)
  Drop non-video bidders from video ad units (prebid#1815)
  Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795)
  Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823)
  Remove require.ensure entirely (prebid#1816)
  Add custom keyword support for pbs bid adapter (prebid#1763)
  OpenX Video Adapter update to Prebid v1.0 (prebid#1724)
  Fix test that hard-coded pbjs global. (prebid#1786)
  Update Pollux Adapter to v1.0 (prebid#1694)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Dec 28, 2017
@vzhukovsky
Merge pull request prebid#133 in AOLP_ADS_JS/prebid.js from release/1…
e9bf890 
….33.0 to aolgithub-master

* commit '3e9756098bb20ecbe0314f16eed5298c5675b24c': (32 commits)
  Wrapped content type in options object.
  Added partners ids.
  Added changelog entry.
  Prebid 0.33.0 Release
  Update AOL adapter for v1.0  (prebid#1693)
  Sovrn 1.0 compliance (prebid#1796)
  Platform.io Bidder Adapter update (prebid#1817)
  Drop non-video bidders from video ad units (prebid#1815)
  Update renderAd to replace ${AUCTION_PRICE} in adUrl (prebid#1795)
  Pulsepoint adapter: fixing bid rejection due to missing mandatory bid params. (prebid#1823)
  Remove require.ensure entirely (prebid#1816)
  Add custom keyword support for pbs bid adapter (prebid#1763)
  OpenX Video Adapter update to Prebid v1.0 (prebid#1724)
  Fix test that hard-coded pbjs global. (prebid#1786)
  Update Pollux Adapter to v1.0 (prebid#1694)
  PubMatic adapter (prebid#1707)
  Added sizes to Rubicon Adapter (prebid#1818)
  jsonpFunction name should match the namespace (prebid#1785)
  Adding 33Across adapter (prebid#1805)
  Unit test fix (prebid#1812)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@matthewlane @dluxemburg
Drop non-video bidders from video ad units (prebid#1815)
6e86850 
* Check mediaTypes when validating video bidders

* Update error message

* Drop non-compatible bidders rather than entire ad unit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaiminpanchal27 jaiminpanchal27 jaiminpanchal27 approved these changes

@mkendall07 mkendall07 mkendall07 approved these changes

Assignees
No one assigned
Labels
LGTM needs review video
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bids requests failing if bidder missing
3 participants
@matthewlane @mkendall07 @jaiminpanchal27