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

Add Sharethrough adapter #865

Merged
merged 16 commits into from Jan 12, 2017
Merged

Add Sharethrough adapter #865

merged 16 commits into from Jan 12, 2017

Conversation

rizhang
Copy link
Contributor

@rizhang rizhang commented Dec 8, 2016

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

  • test parameters for validating bids
{
  bidder: 'sharethrough',
  params: {
      pkey: 'DfFKxpkRGPMS7A9f71CquBgZ'
  }
}

Other information

Added Sharethrough Adapter

bidmanager.addBidResponse(bidObj.placementCode, bid);
}

str.bidWon = function() {
Copy link
Member

Choose a reason for hiding this comment

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

We currently do not allow bidders to listen to other bids. If a bidder wants to do this, they should add an analytics adapter. This is a publisher opt in only process. https://github.com/prebid/Prebid.js/blob/master/src/adapters/analytics/example2.js


const str = {};
str.STR_BTLR_HOST = document.location.protocol + "//btlr.sharethrough.com";
str.STR_TEST_HOST = document.location.protocol + "//btlr-prebid-test.sharethrough.com";
Copy link
Member

Choose a reason for hiding this comment

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

Using a different endpoint for test is not ideal as real issues are discovered in prod. Is there a way to not use a test endpoint here?

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 have to use a test endpoint here to guarantee that a creative is always return from our exchange. If we used the real endpoint there's no way to guarantee that a DSP will always bid on the impression request.

Copy link
Member

Choose a reason for hiding this comment

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

Usually other bidders setup a test creative /placement on the live exchange. Is this possible?

const bidRequest = bids[i];
str.placementCodeSet[bidRequest.placementCode] = bidRequest;
const scriptUrl = _buildSharethroughCall(bidRequest);
str.loadIFrame(scriptUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Don't load JSONP inside an iframe. Use ajax.js or adLoader.loadScript() instead

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 need to put the url inside an iframe so we can drop tracking cookies on publisher's page. This method work for iOS Safari since they block 3rd party cookies.

Copy link
Member

Choose a reason for hiding this comment

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

Ok

sfp_js.src = "//native.sharethrough.com/assets/sfp.js";
sfp_js.type = 'text/javascript';
sfp_js.charset = 'utf-8';
window.top.document.getElementsByTagName('body')[0].appendChild(sfp_js);
Copy link
Member

Choose a reason for hiding this comment

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

window.top isn't safe to call directly, it should be in a try/catch. Additionally it should be avoided if possible to support SafeFrame in the future.

@@ -0,0 +1,269 @@
import { expect } from 'chai';
import Adapter from '../../../src/adapters/sharethrough';
import adloader from '../../../src/adloader';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere, is it needed?


function _receiveMessage(event) {
if(event.origin === str.STR_BTLR_HOST || event.origin === str.STR_TEST_HOST) {
$$PREBID_GLOBAL$$.strcallback(JSON.parse(event.data).response);
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 JSON.parse in a try/catch block

try {
const bid = bidfactory.createBid(1, bidObj);

bid.bidderCode = 'sharethrough';
Copy link
Collaborator

Choose a reason for hiding this comment

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

'sharethrough' is defined in STR_BIDDER_CODE, could use that here to avoid duplicating the string

function _callBids(params) {
const bids = params.bids;

$$PREBID_GLOBAL$$.onEvent('bidWon', str.bidWon);
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed now that you added your analytics adapter.

function _receiveMessage(event) {
if(event.origin === str.STR_BTLR_HOST || event.origin === str.STR_TEST_HOST) {
try {
$$PREBID_GLOBAL$$.strcallback(JSON.parse(event.data).response);
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't need to a public function because it's scoped within the adapter itself. Only use a public function if you use JSONP.

import adapter from 'AnalyticsAdapter';
const utils = require('../../utils');

const url = 'https://httpbin.org/post';
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't want to be sending data to httpbin.org

@@ -17,7 +17,7 @@
"prebid"
],
"globalVarName": "pbjs",
"analytics": [],
"analytics": ["sharethrough_analytics"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Analytics is explicitly turned on by the publisher so this array should be blank by default

@matthewlane matthewlane added this to the Prebid 0.18.0 milestone Jan 6, 2017
@rizhang
Copy link
Contributor Author

rizhang commented Jan 10, 2017

I updated the test pkey parameter

@mkendall07
Copy link
Member

@matthewlane just need final review for bids back on this one.

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.

Validated bids come back

@matthewlane matthewlane merged commit a42655b into prebid:master Jan 12, 2017
protonate pushed a commit that referenced this pull request Jan 18, 2017
* Rz/submit to prebid (#4)

Added Sharethrough Adapter

* fix warnings

* added beacons

* made compatible with chrome 37. other minor changes

* win beacon fired in analytics adapter

* specs for new analytics adapter

* add try catch blocks. misc refactor

* removed test page

* remove debugger

* refactor analytics adapter

* removed test endpoint

* analytics url parameter is empty

* removed bidwon listener on adapter

* removed analytics from package.json
matthewlane pushed a commit that referenced this pull request Jan 19, 2017
* refactored the way hb_deal is handled in adserverTargeting

* Add Sharethrough adapter (#865)

* Rz/submit to prebid (#4)

Added Sharethrough Adapter

* fix warnings

* added beacons

* made compatible with chrome 37. other minor changes

* win beacon fired in analytics adapter

* specs for new analytics adapter

* add try catch blocks. misc refactor

* removed test page

* remove debugger

* refactor analytics adapter

* removed test endpoint

* analytics url parameter is empty

* removed bidwon listener on adapter

* removed analytics from package.json

* refactor hb_deal targeting key as a standard key

* rollback errant style fixes

* more fixes
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…ebid-official-0.18.0 to release/1.13.0

* commit 'e145489bc5dd6d292cf16e7fe80adfdf991562ac': (27 commits)
  Add changelog entry.
  Prebid 0.18.0 Release
  Add Criteo adapter (prebid#928)
  add an event that fires when requestBids is called (prebid#939)
  Xaxis adapter submitted by Daniel hoffmann (prebid#938)
  Add flash detection to TripleLift adapter (prebid#855)
  OpenX Adapter: Fixed bug regarding cross-domain iframe support (prebid#931)
  Emit event when setTargetingForGPTAsync is called (prebid#873)
  Maintenance/refactor hb deal (prebid#935)
  Reset hb_* keys only for registered aduniits (prebid#934)
  update code style - smartyads adapter
  Catch errors in bidsBackHandler.  Also fix test cleanup in pbjs api spec. (prebid#905)
  Smartyads Adapter (prebid#895)
  Appnexus targeting function (prebid#920)
  There are 2 changes- (prebid#913)
  Adding support for all AST parameters (prebid#923)
  GumGum adapter - include the bid timeout as `tmax` (prebid#908)
  Add pixel size (prebid#892)
  enable postMessage listener for cross-domain iframe support (prebid#885)
  Add Sharethrough adapter (prebid#865)
  ...
mp-12301 pushed a commit to aol/Prebid.js that referenced this pull request Apr 10, 2017
…13.0 to master

* commit '7d32ed18c8636d9241ef8b299b6abd885536db69': (27 commits)
  Add changelog entry.
  Prebid 0.18.0 Release
  Add Criteo adapter (prebid#928)
  add an event that fires when requestBids is called (prebid#939)
  Xaxis adapter submitted by Daniel hoffmann (prebid#938)
  Add flash detection to TripleLift adapter (prebid#855)
  OpenX Adapter: Fixed bug regarding cross-domain iframe support (prebid#931)
  Emit event when setTargetingForGPTAsync is called (prebid#873)
  Maintenance/refactor hb deal (prebid#935)
  Reset hb_* keys only for registered aduniits (prebid#934)
  update code style - smartyads adapter
  Catch errors in bidsBackHandler.  Also fix test cleanup in pbjs api spec. (prebid#905)
  Smartyads Adapter (prebid#895)
  Appnexus targeting function (prebid#920)
  There are 2 changes- (prebid#913)
  Adding support for all AST parameters (prebid#923)
  GumGum adapter - include the bid timeout as `tmax` (prebid#908)
  Add pixel size (prebid#892)
  enable postMessage listener for cross-domain iframe support (prebid#885)
  Add Sharethrough adapter (prebid#865)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants