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

Update Roxot prebid analytic adapter #1034

Merged
merged 30 commits into from
Apr 13, 2017

Conversation

kir-roxot
Copy link
Contributor

@kir-roxot kir-roxot commented Mar 7, 2017

  • Feature

Description of change

Hi Guys,

We have changed our analytics adapter logics. AUCTION_END and REQUEST_BIDS events were also added in AnalyticsAdapter to get it full.

@kir-roxot
Copy link
Contributor Author

@matthewlane @mkendall07 Hi guys, i've added test publisher for you. You can test analytic that sent at our endpoint with this config, and you will get 200 ok response:

pbjs.que.push(function () {
  pbjs.enableAnalytics({
    provider: 'roxot',
    options: {
      publisherIds: ['bea17352-fb94-490d-b206-45096c96dbbc']
    }
  });
});

@kir-roxot kir-roxot changed the title Update roxot prebid analytic adapter Update Roxot prebid analytic adapter Mar 14, 2017
@matthewlane matthewlane self-requested a review March 24, 2017 16:05
package.json Outdated
@@ -17,7 +17,7 @@
"prebid"
],
"globalVarName": "pbjs",
"analytics": [],
"analytics": ["roxot_prebid_analytic"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

The analytics array should still be empty by default

eventStack.events = [];
}

let roxotAdapter = utils.extend(adapter({url, analyticsType}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Object.assign should work here as utils.extend was recently removed in #1055

@@ -0,0 +1,35 @@
let assert = require('assert');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assertions aren't used in this test?

return;
}

args.ad = "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably best to check for the existence of args first, if args is undefined this causes a TypeError originating in event.js to be thrown

@@ -1,4 +1,3 @@
let assert = require('assert');
let analytic = require('../../src/adapters/analytics/roxot');
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the assertions, was meaning that the test doesn't check for anything in it's current state. As it's structured could check that the track function was called in response to the emitted events. This can be done by setting up a sinon spy before the emits with sinon.spy(analytic, 'track');, then after the emits something like sinon.assert.callCount(analytic.track, 7);. To make this work the analytic import here would need to change to use ES2015 import statement, or to let analytic = require('../../src/adapters/analytics/roxot').default; because of how the default export is compiling. Feel free to test this according to your preferences though

Also please move this into the analytics test directory at test/spec/adapters/analytics, thanks

@kir-roxot
Copy link
Contributor Author

Hey @matthewlane !

We added the tests you described.
Is it possible to jump the queue and release our updated analytics adapter as soon as possible?

Currently, the adapter on http://prebid.org/download.html is outdated and doesn't collect data as it is supposed to. We are concerned that publishers might be confused as the old version of our adapter doesn't work as described.

@matthewlane
Copy link
Collaborator

Thanks for the tests. We'll get the PR merged as soon as possible. There's not currently a plan to do a patch release but it would be included whenever the next release is

package.json Outdated
"array.prototype.find": "^2.0.3"
"array.prototype.find": "^2.0.3",
"babel-cli": "^6.24.1",
"babel-preset-node6": "^11.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were babel-cli and babel-preset-node6 added as dependencies, I don't think they're used anywhere?

@kir-roxot
Copy link
Contributor Author

@matthewlane Sorry, fixed.

Copy link
Collaborator

@protonate protonate left a comment

Choose a reason for hiding this comment

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

Looks good, adapter is sending analytics data, though the POST response is 403, perhaps the test publisher id is stale.

Ship it!

@matthewlane matthewlane merged commit 2a7298f into prebid:master Apr 13, 2017
outoftime added a commit to Genius/Prebid.js that referenced this pull request Apr 19, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (38 commits)
  Add optional domain parameter to AdButler adapter (prebid#1078)
  Send transactionID to Criteo Services (prebid#1113)
  Fix `buildMasterVideoTagFromAdserverTag()` not selecting winning bid (prebid#1106)
  Remove placement size selection and filtering (prebid#1107)
  revert `srcdoc` change (prebid#1130)
  Add new Adapter- Beachfront Media (prebid#1062)
  Fixes SpringServe adapter (prebid#1101)
  Update Widespace request param (prebid#1098)
  - New Adapter: Innity (prebid#1074)
  Update Roxot prebid analytic adapter (prebid#1034)
  Yarn Package Manager (prebid#1109)
  allow writing into current document if prebid is loaded inside an iframe (prebid#1066)
  Adapter bug fix (prebid#1096)
  fix typo
  added pr review process and governance model (prebid#1103)
  added support for sampling in ga and base adapter, fixed up some tests (prebid#1011)
  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)
  ...
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.

5 participants