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

Video header bidding support to RhythmOne bidder adapter #1052

Merged
merged 3 commits into from
Mar 28, 2017
Merged

Video header bidding support to RhythmOne bidder adapter #1052

merged 3 commits into from
Mar 28, 2017

Conversation

jstocker76
Copy link
Contributor

@jstocker76 jstocker76 commented Mar 16, 2017 •
edited
Loading

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

Added video header bidding support to RhythmOne bidder adapter

  • test parameters for validating bids
{
	bidder: 'rhythmone',
	params: {
		endpoint: "//1r-qa-ads.rhythmxchange.com/rmp/{placementId}/0/{path}?z={zone}",
		placementId: "389633",
		zone: "csmith5",
		path: "mvo",
		debug: true,
		method:"get"
	}
}
  • contact email of the adapter’s maintainer
  • official adapter submission

Other information

@matthewlane matthewlane self-assigned this Mar 20, 2017
@matthewlane matthewlane self-requested a review March 20, 2017 18:24
matthewlane
matthewlane reviewed Mar 24, 2017
View reviewed changes
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

The test params return bids for a regular ad unit, but I'm not getting bids when used with an ad unit configured with mediaType: 'video', should that be expected? Also a few notes below for your review

src/adapters/rhythmone.js
CONSTANTS = require('../constants.json');

import {ajax as ajax} from '../ajax';

function track(debug, p1, p2, p3) {
function track(debug) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function doesn't seem to do anything now, is it still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use google analytics to test this bidder on some of our own sites, and use this function for that purpose. Having to add it and all its references back, only to remove it before a push, repeatedly, would be cumbersome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks for clarifying

src/adapters/rhythmone.js
var d = global.document.location.ancestorOrigins;
if(d && d.length > 0)
return d[d.length-1];
return global.top.document.location.hostname;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap any top references in a try/catch block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attempt function wraps this callback in a try/catch - somewhere around line 78

src/adapters/rhythmone.js
return d[d.length-1];
return global.top.document.location.hostname;
},""));
p("title", attempt(function(){return global.top.document.title;},""));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another top reference to wrap in try/catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attempt function wraps this callback in a try/catch - somewhere around line 78

@jstocker76
Copy link
Contributor Author

My bad, I've updated the bidder parameters to an endpoint and placement id combo that reliably return video bids

@matthewlane matthewlane requested a review from mkendall07 March 27, 2017 22:00
@protonate protonate merged commit 2e6d865 into prebid:master Mar 28, 2017
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request May 19, 2017
@vzhukovsky
Merge pull request prebid#90 in AOLP_ADS_JS/prebid.js from release/1.…
8b44d83 
…19.0 to aolgithub-master

* commit '5109273bd4317535b23e26ff609345101a3d038d': (49 commits)
  Disable unit tests that fails on PhantomJs.
  Fixed unit tests for adapter loader.
  Changed way of including analytic adapters.
  Added adapters in aolPartnersIds.json.
  Added changelog entry.
  Replace removed utils.extend functionality by object.assign.
  Add Inneractive adapter (prebid#1048)
  Add alias freewheel-ssp to stickyadstv bidder adapter  (prebid#1043)
  Add Facebook Audience Network adapter (prebid#1068)
  Add Atomx support (prebid#1056)
  Updated rubicon video bid endpoint in source and test files (prebid#1097)
  Pass through params to server (prebid#1084)
  Reset the list of slots to be requested between each action for pubmatic (prebid#1079)
  Support for downloading Analytics Adapters via http://prebid.org/download.html (prebid#1021)
  update PR template to include link to dev docs page (prebid#1075)
  Prebid 0.21.0
  Lifestreet adapter: ignore unnecessary events from creative. (prebid#1054)
  Video header bidding support to RhythmOne bidder adapter (prebid#1052)
  Add rubicon targeting to rubicon bid responses for bidderSettings use (prebid#1045)
  Fix adapter getSize (prebid#1064)
  ...
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request May 22, 2017
@vzhukovsky
Merge pull request prebid#91 in AOLP_ADS_JS/prebid.js from release/1.…
ab3f32e 
…19.0 to master

* commit '5109273bd4317535b23e26ff609345101a3d038d': (49 commits)
  Disable unit tests that fails on PhantomJs.
  Fixed unit tests for adapter loader.
  Changed way of including analytic adapters.
  Added adapters in aolPartnersIds.json.
  Added changelog entry.
  Replace removed utils.extend functionality by object.assign.
  Add Inneractive adapter (prebid#1048)
  Add alias freewheel-ssp to stickyadstv bidder adapter  (prebid#1043)
  Add Facebook Audience Network adapter (prebid#1068)
  Add Atomx support (prebid#1056)
  Updated rubicon video bid endpoint in source and test files (prebid#1097)
  Pass through params to server (prebid#1084)
  Reset the list of slots to be requested between each action for pubmatic (prebid#1079)
  Support for downloading Analytics Adapters via http://prebid.org/download.html (prebid#1021)
  update PR template to include link to dev docs page (prebid#1075)
  Prebid 0.21.0
  Lifestreet adapter: ignore unnecessary events from creative. (prebid#1054)
  Video header bidding support to RhythmOne bidder adapter (prebid#1052)
  Add rubicon targeting to rubicon bid responses for bidderSettings use (prebid#1045)
  Fix adapter getSize (prebid#1064)
  ...
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
@jstocker76 @dluxemburg
Video header bidding support to RhythmOne bidder adapter (prebid#1052)
486e283 
* adding video header bidding support to rhythmone adapter

* improving code coverage of unit tests

* fixing unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@protonate protonate protonate approved these changes

@matthewlane matthewlane matthewlane approved these changes

@mkendall07 mkendall07 Awaiting requested review from mkendall07

Assignees

@matthewlane matthewlane

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jstocker76 @matthewlane @protonate