Initial checkin of RhythmOne bidder adapter and unit tests #657

jstocker76 · 2016-09-27T00:29:40Z

Type of change

Description of change

Addition of a new bidder adapter that uses RhythmOne's ad exchange with unit tests.

test parameters for validating bids

			{
				bidder: 'rhythmone',
				params: {
					placementId: "549",
					zone: "hb",
					path: "mvo"
				}
			}

contact email of the adapter’s maintainer
astocker@rhythmone.com
official adapter submission

KevinCHill · 2016-09-27T00:32:51Z

Hi, Thank you for contacting me. I'm out of the office for the rest of the day. Though I do check my emails randomly throughout the evening, I will most likely get back to you in the morning

jstocker76 · 2016-09-27T17:10:19Z

oy. my bad. Will fix then submit another pull

jstocker76 · 2016-09-27T17:54:18Z

My unit tests were validating a bid property that wasn't under my direct control (bid.type) and had been removed since my last pull from master. I removed this. Should be good to go

baragona · 2016-10-04T18:46:13Z

Not sure if it's appropriate to explicitly refer to "pbjs" in an adapter anymore, we switched to using "$$PREBID_GLOBAL$$" which may be configured when building the project.

jstocker76 · 2016-10-04T21:02:54Z

Aah, got it. I've replaced uses of "pbjs" with the $$PREBID_GLOBAL$$ macro.

protonate · 2016-10-11T23:40:43Z

@jstocker76 thanks for the PR. I'm not getting the ad to render with the provided test parameters. I am getting bids and ads back. I'm using localhost and prebidapp.com domains for testing. Any ideas?

Also see #509 about matching bid request IDs with bid response IDs.

mkendall07 · 2016-10-12T13:38:05Z

src/adapters/rhythmone.js

+
+  var rhythmBidderUtilities = {
+    template: function(){return "<div id=\"{0}_wrapper\"></div><script type=\"text/javascript\">("+rhythmBidderUtilities.process.toString()+")({1}, {6}, {2}, {3}, \"{0}\", {4}, \"{5}\");</scr"+"ipt>";},
+    process: "function (e,t,r,n,i,a,o){function d(e,t,r,i){var a=[\"bidderCode\",\"size\",\"adUnitCode\",\"cpm\",\"timeToRespond\"],o=[\"b\",\"s\",\"a\",\"c\",\"t\"],d=0,s=[],c=new Image,u=\"\",p=document.location.ancestorOrigins,l=\"//rhythmanalytics.1rx.io/rhythmanalytics/img?tracking_id=prebid_r1&hit_type=event&time_ms=\"+(new Date).getTime()+\"&metric0=allBids&metric1=\"+encodeURIComponent(t)+\"&metric2=\"+encodeURIComponent(n.user.id);for(var f in e)for(d=0;d<e[f].bids.length;d++){for(var g={},m=0;m<a.length;m++)g[o[m]]=e[f].bids[d][a[m]];s.push(g)}try{u=null!==r?i.document.location.href.toString():p&&p.length>0?p[p.length-1]:document.location.href.toString()}catch(b){}for(l+=\"&page_url=\"+encodeURIComponent(u),d=0;d<s.length&&10>d;d++)l+=\"&dimension\"+d+\"=\"+encodeURIComponent(JSON.stringify(s[d]));c.src=l}function s(e,t,n){var i=document.createElement(\"iframe\");i.style.border=\"0\",i.scrolling=\"no\",i.seamless=\"seamless\",i.style.height=r.h+\"px\",i.style.width=r.w+\"px\",e.style.height=r.h+\"px\",e.style.width=r.w+\"px\",e.appendChild(i),t?(i.contentWindow.document.open(),i.contentWindow.document.write('<html><head></head><body style=\"margin:0;padding:0;\">'+t+\"</body></html>\"),/(MSIE|Trident|Edge)/.test(window.navigator.userAgent)===!1&&i.contentWindow.document.close()):n&&(i.src=n)}var c=window,u=function(){try{for(;c;){if(c.$$PREBID_GLOBAL$$||c===window.top)return c.$$PREBID_GLOBAL$$;c=c.parent}}catch(e){}return null}(),p=u?u.getBidResponses():{},l=p[i],f=0;d(p,o,u,c);var g=[n.imp[0].bidfloor];if(l)for(var m=0;m<l.bids.length;m++)g.push(parseFloat(l.bids[m].cpm));g.sort(),f=g[g.length-2]+.01;var b=/\\$\\{AUCTION_([A-Z_]+)\\}/g,h={ID:n.id,BID_ID:r.id,IMP_ID:r.impid,SEAT_ID:a.id,AD_ID:r.adid,PRICE:f,CURRENCY:n.cur[0]};e&&(e=e.replace(b,function(e){var t=b.exec(e)[1];return h[t]?h[t]:e})),t&&(t=t.replace(b,function(e){var t=b.exec(e)[1];return h[t]?h[t]:e})),s(document.getElementById(i+\"_wrapper\"),e,t)}"


Why is the code minified here?

mkendall07 · 2016-10-12T13:39:26Z

src/adapters/rhythmone.js

+
+    logToConsole("posting "+json+" to "+url);
+
+    if(typeof global.XDomainRequest !== "undefined"){


We have utility for making ajax requests - please use https://github.com/prebid/Prebid.js/blob/master/src/ajax.js instead

mkendall07 · 2016-10-12T13:40:35Z

src/adapters/rhythmone.js

+
+  var bidderCode = "rhythmone";
+
+  function GUID() {


pleas use utils.generateUUID() instead.

mkendall07 · 2016-10-12T13:43:13Z

src/adapters/rhythmone.js

+    setIfPresent(o.site, "page", function(){
+      var d = (typeof bid.params === "object" ? bid.params.domain : null),
+        l;
+      try{l = top.document.location.href.toString();}


please use window.top

mkendall07 · 2016-10-12T13:44:01Z

src/adapters/rhythmone.js

+      d = document.location.ancestorOrigins;
+      if(d && d.length > 0)
+        return d[d.length-1];
+      return top.document.location.hostname;


Please use window.top and add try/catch.

mkendall07 · 2016-10-12T13:45:17Z

src/adapters/rhythmone.js

+        o.site.keywords = bid.params.keywords;
+    }
+
+    o.device.devicetype = ((/(ios|ipod|ipad|iphone|android)/i).test(global.navigator.userAgent) ? 1 : ((/(smart[-]?tv|hbbtv|appletv|googletv|hdmi|netcast\.tv|viera|nettv|roku|\bdtv\b|sonydtv|inettvbrowser|\btv\b)/i).test(global.navigator.userAgent) ? 3 : 2));


Consider adding this as a function in utils as it might be useful for others.

mkendall07 · 2016-10-12T13:49:02Z

src/adapters/rhythmone.js

+
+    function finish(){
+
+      var sandbox = new ZTStorage("//d3rim8qxq4v92b.cloudfront.net/ztstorage/bg.htm"),


can you explain what this is used for?

mkendall07 · 2016-10-12T13:49:46Z

adapters.json

@@ -14,6 +14,7 @@
    "piximedia",
    "pubmatic",
    "pulsepoint",
+	"rhythmone",


please fix spacing

jstocker76 · 2016-10-21T20:38:39Z

There have been some unforseen alterations to the ad api that require a lot of changes (mostly reductions) in code for this bidder adapter. I'm going to close this pull request until the time comes when the API is stable, and resubmit with all your feedback that's still relevant. Thanks very much for the code reviews

…request to the ad api, removed dependencies on external IP lookup and browser storage (for persistent user ID). Updated unit tests

jstocker76 · 2016-11-03T21:49:18Z

The bidder object has been updated with new test parameters-

{
bidder: 'rhythmone',
params: {
placementId: "549",
zone: "hb",
path: "mvo"
}
}

Alterations since the last pull request:

The ad API can now handle multiple impressions. I was previously issuing a separate ad request for every combination of ad slot and size due to the limitations of the API. These have been consolidated into 1 request
Removed dependencies on local browser storage. During previous code reviews there were questions about what the file "zstorage.js" did. It was used to store a persistent user id (and a couple other things) in local browser storage via a sandboxed iframe. This is no longer necessary
Added support for a GET request, avoiding the separate OPTIONS request.
Removed the need to ping an API to retrieve the user's IP

Code review/feedback addressed:

mkendall07 - "Why is the code minified here?"
The ad markup is wrapped in a script that applies ORTB macros when the ad is rendered. I had kept this as a template string because none of this code is executed by the bidder adapter directly.

mkendall07 - "We have utility for making ajax requests - please use https://github.com/prebid/Prebid.js/blob/master/src/ajax.js instead"
done.

mkendall07 - "pleas use utils.generateUUID() instead."
done.

mkendall07 - "please use window.top"
done.

mkendall07 - "Please use window.top and add try/catch."
Added window.top. The try/catch is in place in the setIfPresent function. It wraps execution of the callback method.

mkendall07 - "can you explain what this is used for?" (in reference to the zstorage.js file)
dependencies on this file have since been removed. It was a utility that streamlines saving data to local browser storage on a common domain. I was using it to store a semi-permanent user id.

mkendall07 - "please fix spacing" (in adapters.json)
done.

I'm confident there won't be any major changes coming from my end from this point onward. Sorry for the delay.

mkendall07 · 2016-11-03T22:01:03Z

src/adapters/rhythmone.js

+          catch(ex){}
+          return null;
+        })(),
+        responses = (pjs && pjs.getBidResponses ? pjs.getBidResponses() : {}),


adapters should not access other bid responses. Can you please remove this bit

That makes it rather difficult to populate the {AUCTION_PRICE} ortb macro...

I've talked this over with the powers that be, and will remove the ortb macro substitution.

Is there a policy or guideline that states what public objects/data should not be accessed?

No, but we should have one. Essentially no adapter should be looking at bids submitted by other partners or otherwise using the public API's designated for publishers. What this means practically is that you do not need to access any methods/properties on pbjs with the exception of if your adapter needs to expose a JSONP callback handler function.

In regards to {AUCTION_PRICE} macro, the price paid is equal to the bid amount in prebid.js. There is no flooring that happens.

What about registering to pbjs notifications such as auctionEnd, timeout, etc?

@abkbynature
I'll review the code here and let you know if I have any objections.

protonate · 2016-11-04T22:23:00Z

src/adapters/rhythmone.js

@@ -0,0 +1,360 @@
+/*
+  Copyright (c) 2016 RhythmOne, LLC. All rights reserved.


Please remove, all submissions are subject to the Apache 2.0 License.
https://github.com/prebid/Prebid.js/blob/master/LICENSE

protonate · 2016-11-04T22:23:24Z

src/adapters/rhythmone.js

+
+import {ajax as ajax} from '../ajax';
+
+//function setupGA() {


Please remove commented code.

protonate · 2016-11-04T22:29:38Z

src/adapters/rhythmone.js

+  function noBids(params){
+    for(var i=0; i<params.bids.length; i++){
+      if(params.bids[i].success !== 1){
+        var bid = bidfactory.createBid(2);


Adapters can now pass a constant for status and the originating bid request, the first for readability and the second to match bid request / response IDs per #509.

e.g.

var bid = bidfactory.createBid(CONSTANTS.STATUS.NO_BID, bidRequest);

protonate · 2016-11-04T22:30:07Z

src/adapters/rhythmone.js

+
+              slotMap[bid.impid].success = 1;
+
+              var pbResponse = bidfactory.createBid(1),


Adapters can now pass a constant for status and the originating bid request, the first for readability and the second to match bid request / response IDs per #509.

e.g.

var bid = bidfactory.createBid(CONSTANTS.STATUS.GOOD, bidRequest);

…atus

protonate · 2016-11-09T22:24:45Z

Thanks for the updates, reviewed and approved.

aneuway2 · 2016-11-17T17:23:29Z

@jstocker76 are all parameter's required or are there optional inputs?

jstocker76 · 2016-11-17T17:28:08Z

aneuway2 - only placementId is required, the other 2 parameters are only needed until our API hits production sometime today.

mkendall07 · 2016-11-17T18:09:48Z

@jstocker76
Please submit a docs PR here: https://github.com/prebid/prebid.github.io/tree/master/dev-docs/bidders

jstocker76 · 2016-11-17T18:49:08Z

mkendall07 - done

aneuway2 · 2017-01-03T16:54:25Z

@jstocker76 looking at http://prebid.org/dev-docs/bidders.html#rhythmone and the PR for documentation it looks like placementId was specified in the documentation instead of publisherId. is this an error, or did it change to placementId?

Thanks!

jstocker76 · 2017-01-03T21:46:01Z

"placementId" is what the code expects. Whether, in practice, it functions as a placement id or publisher id depends on how the publisher's account is set up in our system.

initial checkin of rhythmone prebid adapter and unit tests

9b30ce0

jstocker76 added 2 commits September 27, 2016 10:03

fixing unit tests

e1f85e6

removing check of deprecated property

5583c14

jstocker76 closed this Sep 27, 2016

reintroducing cpm check to unit tests

ca77a51

jstocker76 reopened this Sep 27, 2016

matthewlane added the needs review label Oct 3, 2016

matthewlane assigned protonate Oct 4, 2016

changing direct use of pbjs to the $$PREBID_GLOBAL$$ build macro

3eb847e

protonate added needs update and removed needs review labels Oct 11, 2016

mkendall07 requested changes Oct 12, 2016

View reviewed changes

mkendall07 reviewed Oct 12, 2016

View reviewed changes

adapters.json

@@ -14,6 +14,7 @@

"piximedia",

"pubmatic",

"pulsepoint",

"rhythmone",

Copy link

Member

mkendall07 Oct 12, 2016

Choose a reason for hiding this comment

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

please fix spacing

jstocker76 added 3 commits October 13, 2016 14:27

prebid feedback implemented, aviv's GA logging added

0e9e49d

altered timeout tracking logic and updated unit tests

9e7449e

additional unit test updates

a9ed8a0

jstocker76 closed this Oct 21, 2016

Altered rhythmone bid adapter to support either a single get or post …

b797e44

…request to the ad api, removed dependencies on external IP lookup and browser storage (for persistent user ID). Updated unit tests

jstocker76 reopened this Nov 3, 2016

mkendall07 added needs review and removed needs update labels Nov 3, 2016

found another use of top without window. Fixed

b0272e0

mkendall07 reviewed Nov 3, 2016

View reviewed changes

Removed ORTB macros dependent on the use of getBidResponses

685689a

protonate suggested changes Nov 4, 2016

View reviewed changes

removing copyright, commented ga code, and used a constant for bid st…

19e1003

…atus

protonate approved these changes Nov 9, 2016

View reviewed changes

mkendall07 approved these changes Nov 9, 2016

View reviewed changes

protonate merged commit 9e1b4e9 into prebid:master Nov 9, 2016


		logToConsole("posting "+json+" to "+url);

		if(typeof global.XDomainRequest !== "undefined"){


		function finish(){

		var sandbox = new ZTStorage("//d3rim8qxq4v92b.cloudfront.net/ztstorage/bg.htm"),

		@@ -0,0 +1,360 @@
		/*
		Copyright (c) 2016 RhythmOne, LLC. All rights reserved.


		slotMap[bid.impid].success = 1;

		var pbResponse = bidfactory.createBid(1),

Initial checkin of RhythmOne bidder adapter and unit tests #657

Initial checkin of RhythmOne bidder adapter and unit tests #657

Conversation

jstocker76 commented Sep 27, 2016 • edited Loading

Type of change

Description of change

KevinCHill commented Sep 27, 2016 via email

jstocker76 commented Sep 27, 2016

jstocker76 commented Sep 27, 2016

baragona commented Oct 4, 2016

jstocker76 commented Oct 4, 2016

protonate commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstocker76 commented Oct 21, 2016

jstocker76 commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protonate commented Nov 9, 2016

aneuway2 commented Nov 17, 2016

jstocker76 commented Nov 17, 2016 • edited Loading

mkendall07 commented Nov 17, 2016

jstocker76 commented Nov 17, 2016

aneuway2 commented Jan 3, 2017

jstocker76 commented Jan 3, 2017

jstocker76 commented Sep 27, 2016 •

edited

Loading

jstocker76 commented Nov 17, 2016 •

edited

Loading