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.

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

Fix uncached video bids triggering callback early #2017

Merged
merged 2 commits into from
Jan 10, 2018

Conversation

matthewlane
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

Fixes a bug where auctions for video bids with cache enabled would execute the prebid callback handler on the first bid response, rather than waiting for all bids to be back. Does this by marking the doneCbCallCount property of cach-enabled video bids only after they've been cached server-side, rather than immediately on bid response.

To test manually, create a test page with the following ad unit ('appnexusAst' is an alias of 'appnexus'). On master, search for auctionEnd in the dev console and notice a bidResponse comes after this event. On this fix, auctionEnd comes after all bidResponses

pbjs.setConfig({
  debug: true,
  cache: {
    url: 'https://prebid.adnxs.com/pbc/v1/cache'
  },
});

pbjs.addAdUnits({
  code: '/19968336/prebid_video_adunit',
  sizes: [[640,480]],
  mediaTypes: { video: {context: 'instream'} },
  bids: [
    {
      bidder: 'appnexusAst',
      params: {
        placementId: '9333431',
        video: { skippable: true, playback_method: ['auto_play_sound_off'] }
      }
    },
    {
      bidder: 'appnexus',
      params: {
        placementId: '9333431',
        video: { skippable: true, playback_method: ['auto_play_sound_off'] }
      }
    },
  ]
});

@ghost ghost assigned matthewlane Jan 9, 2018
@ghost ghost added the in progress label Jan 9, 2018
@matthewlane matthewlane removed their assignment Jan 9, 2018
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@jsnellbaker jsnellbaker merged commit 289cee2 into master Jan 10, 2018
@matthewlane matthewlane deleted the bugfix/video-bid-done branch January 10, 2018 20:22
chanand pushed a commit to chanand/Prebid.js that referenced this pull request Jan 18, 2018
* Mark video bid done after it's been cached

* Add tests
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Jan 23, 2018
* 'master' of https://github.com/prebid/Prebid.js: (23 commits)
  Update Atomx adapter for Prebid v1.0 (prebid#2026)
  Add vi bid adapter (prebid#2020)
  Add eplanningBidAdapter (prebid#2003)
  OpenX Adapter: Update to support mediaTypes field, instead of the deprecated mediaType field (prebid#1974)
  Separate bids & won calls (prebid#2015)
  1.0 adapter support for mantis (prebid#1840)
  Media.net adapter added (prebid#2038)
  GumGum Adapter for Prebid.js 1.0 (prebid#1966)
  Serverbid Bid Adapter: updated docs and ad sizes (prebid#2023)
  Adding districtm as an alias (prebid#2018)
  Use sudo to workaround Travis regression (prebid#2041)
  Fix uncached video bids triggering callback early (prebid#2017)
  Re-implemented RhythmOne audit beacon in prebid 1.0 interface (prebid#1953)
  Add NasmediaAdmixer adapter for Perbid.js 1.0 (prebid#1937)
  Update Adform adapter to Prebid v1.0 (prebid#1947)
  Upgrade Admixer adapter for Prebid 1.0 (prebid#1755)
  multiformat size validation checks (prebid#1964)
  Gjirafa Bidder Adapter (prebid#1944)
  pin gulp-connect at non-broken version (prebid#2008)
  Added dynamic ttl property for One Display and One Mobile. (prebid#2004)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Mark video bid done after it's been cached

* Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants