From 1dac29c5de125e5ecf06a419dc1ffdf643dcc93c Mon Sep 17 00:00:00 2001 From: Aparna Hegde Date: Tue, 5 Dec 2017 12:13:41 -0500 Subject: [PATCH 1/5] Removed the usage of utils library to get bidder requests during userSync since this info will now be passed into the method --- modules/33acrossBidAdapter.js | 6 +- test/spec/modules/33acrossBidAdapter_spec.js | 108 +++++++++---------- 2 files changed, 52 insertions(+), 62 deletions(-) diff --git a/modules/33acrossBidAdapter.js b/modules/33acrossBidAdapter.js index b496a66d081..40dc06c51a5 100644 --- a/modules/33acrossBidAdapter.js +++ b/modules/33acrossBidAdapter.js @@ -1,5 +1,4 @@ const { registerBidder } = require('../src/adapters/bidderFactory'); -const utils = require('../src/utils'); const BIDDER_CODE = '33across'; const END_POINT = 'https://ssc.33across.com/api/v1/hb'; @@ -116,12 +115,11 @@ function interpretResponse(serverResponse) { } // Register one sync per bid since each ad unit may potenitally be linked to a uniqe guid -function getUserSyncs(syncOptions) { +function getUserSyncs(syncOptions, bidderRequests) { let syncs = []; - const ttxBidRequests = utils.getBidderRequestAllAdUnits(BIDDER_CODE).bids; if (syncOptions.iframeEnabled) { - syncs = ttxBidRequests.map(_createSync); + syncs = bidderRequests.bids.map(_createSync); } return syncs; diff --git a/test/spec/modules/33acrossBidAdapter_spec.js b/test/spec/modules/33acrossBidAdapter_spec.js index 3c25aee4199..d25c7f08156 100644 --- a/test/spec/modules/33acrossBidAdapter_spec.js +++ b/test/spec/modules/33acrossBidAdapter_spec.js @@ -1,6 +1,11 @@ const { expect } = require('chai'); const utils = require('../../../src/utils'); -const { isBidRequestValid, buildRequests, interpretResponse, getUserSyncs } = require('../../../modules/33acrossBidAdapter'); +const { + isBidRequestValid, + buildRequests, + interpretResponse, + getUserSyncs +} = require('../../../modules/33acrossBidAdapter'); describe('33acrossBidAdapter:', function () { const BIDDER_CODE = '33across'; @@ -21,8 +26,8 @@ describe('33acrossBidAdapter:', function () { adUnitCode: 'div-id', requestId: 'r1', sizes: [ - [300, 250], - [728, 90] + [ 300, 250 ], + [ 728, 90 ] ], transactionId: 't1' } @@ -113,7 +118,7 @@ describe('33acrossBidAdapter:', function () { describe('buildRequests:', function() { it('returns corresponding server requests for each valid bidRequest', function() { const ttxRequest = { - imp: [{ + imp: [ { banner: { format: [ { @@ -133,7 +138,7 @@ describe('33acrossBidAdapter:', function () { prod: PRODUCT_ID } } - }], + } ], site: { id: SITE_ID }, @@ -341,37 +346,41 @@ describe('33acrossBidAdapter:', function () { describe('getUserSyncs', function() { beforeEach(function() { - this.ttxBids = [ - { - params: { - siteId: 'id1', - productId: 'p1' - } - }, - { - params: { - siteId: 'id2', - productId: 'p1' + this.bidderRequests = { + bids : [ + { + params: { + siteId: 'id1', + productId: 'p1' + } + }, + { + params: { + siteId: 'id2', + productId: 'p1' + } } - } - ]; + ] + }; - this.testTTXBids = [ - { - params: { - site: { id: 'id1' }, - productId: 'p1', - syncUrl: 'https://staging-de.tynt.com/deb/v2?m=xch' - } - }, - { - params: { - site: { id: 'id2' }, - productId: 'p1', - syncUrl: 'https://staging-de.tynt.com/deb/v2?m=xch' + this.testBidderRequests = { + bids: [ + { + params: { + site: { id: 'id1' }, + productId: 'p1', + syncUrl: 'https://staging-de.tynt.com/deb/v2?m=xch' + } + }, + { + params: { + site: { id: 'id2' }, + productId: 'p1', + syncUrl: 'https://staging-de.tynt.com/deb/v2?m=xch' + } } - } - ]; + ] + }; this.syncs = [ { @@ -397,45 +406,28 @@ describe('33acrossBidAdapter:', function () { }); context('when iframe is not enabled', function() { - it.skip('returns empty sync array', function() { - this.sandbox.stub(utils, 'getBidderRequestAllAdUnits', () => ( - { - bids: this.ttxBids - } - )); + it('returns empty sync array', function() { const syncOptions = {}; - expect(getUserSyncs(syncOptions)).to.deep.equal([]); + expect(getUserSyncs(syncOptions, this.bidderRequests)).to.deep.equal([]); }); }); context('when iframe is enabled', function() { - it.skip('returns sync array equal to number of bids for ttx', function() { - this.sandbox.stub(utils, 'getBidderRequestAllAdUnits', () => ( - { - bids: this.ttxBids - } - )); - + it('returns sync array equal to number of bids for ttx', function() { const syncOptions = { iframeEnabled: true }; - const syncs = getUserSyncs(syncOptions); - expect(syncs.length).to.equal(this.ttxBids.length); + const syncs = getUserSyncs(syncOptions, this.bidderRequests); + expect(syncs.length).to.equal(this.bidderRequests.bids.length); expect(syncs).to.deep.equal(this.syncs); }); - it.skip('returns sync array equal to number of test bids for ttx', function() { - this.sandbox.stub(utils, 'getBidderRequestAllAdUnits', () => ( - { - bids: this.testTTXBids - } - )); - + it('returns sync array equal to number of test bids for ttx', function() { const syncOptions = { iframeEnabled: true }; - const syncs = getUserSyncs(syncOptions); - expect(syncs.length).to.equal(this.testTTXBids.length); + const syncs = getUserSyncs(syncOptions, this.testBidderRequests); + expect(syncs.length).to.equal(this.testBidderRequests.bids.length); expect(syncs).to.deep.equal(this.testSyncs); }); }); From 3d32bfdc67c1df0fc136c6b5514378db8bea3b4b Mon Sep 17 00:00:00 2001 From: Aparna Hegde Date: Tue, 5 Dec 2017 12:34:53 -0500 Subject: [PATCH 2/5] Fixed extra space which cause lint to fail --- test/spec/modules/33acrossBidAdapter_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec/modules/33acrossBidAdapter_spec.js b/test/spec/modules/33acrossBidAdapter_spec.js index d25c7f08156..85f35f06408 100644 --- a/test/spec/modules/33acrossBidAdapter_spec.js +++ b/test/spec/modules/33acrossBidAdapter_spec.js @@ -347,7 +347,7 @@ describe('33acrossBidAdapter:', function () { describe('getUserSyncs', function() { beforeEach(function() { this.bidderRequests = { - bids : [ + bids: [ { params: { siteId: 'id1', From 2dc4397c0a4990ad445d9a9888a6d8d84fab4240 Mon Sep 17 00:00:00 2001 From: Aparna Hegde Date: Wed, 6 Dec 2017 17:09:02 -0500 Subject: [PATCH 3/5] Implemented user sync per code review feedback in https://github.com/prebid/Prebid.js/pull/1917 --- modules/33acrossBidAdapter.js | 57 +++-- test/spec/modules/33acrossBidAdapter_spec.js | 210 +++++++++---------- 2 files changed, 124 insertions(+), 143 deletions(-) diff --git a/modules/33acrossBidAdapter.js b/modules/33acrossBidAdapter.js index 40dc06c51a5..4b5bf680e14 100644 --- a/modules/33acrossBidAdapter.js +++ b/modules/33acrossBidAdapter.js @@ -1,4 +1,6 @@ const { registerBidder } = require('../src/adapters/bidderFactory'); +const { userSync } = require('../src/userSync'); +const { config } = require('../src/config'); const BIDDER_CODE = '33across'; const END_POINT = 'https://ssc.33across.com/api/v1/hb'; @@ -37,9 +39,7 @@ function _createServerRequest(bidRequest) { } } - // Allowing site to be a test configuration object or just the id (former required for testing, - // latter when used by publishers) - ttxRequest.site = params.site || { id: params.siteId }; + ttxRequest.site = { id: params.siteId }; // Go ahead send the bidId in request to 33exchange so it's kept track of in the bid response and // therefore in ad targetting process @@ -50,28 +50,18 @@ function _createServerRequest(bidRequest) { withCredentials: false }; - if (bidRequest.params.customHeaders) { - options.customHeaders = bidRequest.params.customHeaders; - } + // Allow the ability to configure the HB endpoint for testing purposes. + const ttxSettings = config.getConfig('ttxSettings'); + const url = (ttxSettings && ttxSettings.url) || END_POINT; return { 'method': 'POST', - 'url': bidRequest.params.url || END_POINT, + 'url': url, 'data': JSON.stringify(ttxRequest), 'options': options } } -// Sync object will always be of type iframe for ttx -function _createSync(bid) { - const syncUrl = bid.params.syncUrl || SYNC_ENDPOINT; - - return { - type: 'iframe', - url: `${syncUrl}&id=${bid.params.siteId || bid.params.site.id}` - } -} - function _getFormatSize(sizeArr) { return { w: sizeArr[0], @@ -80,6 +70,18 @@ function _getFormatSize(sizeArr) { } } +// Register one sync per bid since each ad unit may potenitally be linked to a uniqe guid +// Sync type will always be 'iframe' for 33Across +function _registerUserSyncs(requestData) { + const ttxRequest = JSON.parse(requestData); + const ttxSettings = config.getConfig('ttxSettings'); + + let syncUrl = (ttxSettings && ttxSettings.syncUrl) || SYNC_ENDPOINT; + + syncUrl = `${syncUrl}&id=${ttxRequest.site.id}`; + userSync.registerSync('iframe', BIDDER_CODE, syncUrl); +} + function isBidRequestValid(bid) { if (bid.bidder !== BIDDER_CODE || typeof bid.params === 'undefined') { return false; @@ -103,7 +105,12 @@ function buildRequests(bidRequests) { } // NOTE: At this point, the response from 33exchange will only ever contain one bid i.e. the highest bid -function interpretResponse(serverResponse) { +function interpretResponse(serverResponse, bidRequest) { + // Register user sync first + if (bidRequest && bidRequest.data) { + _registerUserSyncs(bidRequest.data); + } + const bidResponses = []; // If there are bids, look at the first bid of the first seatbid (see NOTE above for assumption about ttx) @@ -114,23 +121,11 @@ function interpretResponse(serverResponse) { return bidResponses; } -// Register one sync per bid since each ad unit may potenitally be linked to a uniqe guid -function getUserSyncs(syncOptions, bidderRequests) { - let syncs = []; - - if (syncOptions.iframeEnabled) { - syncs = bidderRequests.bids.map(_createSync); - } - - return syncs; -} - const spec = { code: BIDDER_CODE, isBidRequestValid, buildRequests, - interpretResponse, - getUserSyncs + interpretResponse } registerBidder(spec); diff --git a/test/spec/modules/33acrossBidAdapter_spec.js b/test/spec/modules/33acrossBidAdapter_spec.js index 85f35f06408..4754e4d1fa5 100644 --- a/test/spec/modules/33acrossBidAdapter_spec.js +++ b/test/spec/modules/33acrossBidAdapter_spec.js @@ -1,5 +1,8 @@ +const { userSync } = require('../../../src/userSync'); +const { config } = require('../../../src/config'); + const { expect } = require('chai'); -const utils = require('../../../src/utils'); + const { isBidRequestValid, buildRequests, @@ -12,6 +15,7 @@ describe('33acrossBidAdapter:', function () { const SITE_ID = 'pub1234'; const PRODUCT_ID = 'product1'; const END_POINT = 'https://ssc.33across.com/api/v1/hb'; + const SYNC_ENDPOINT = 'https://de.tynt.com/deb/v2?m=xch'; beforeEach(function() { this.bidRequests = [ @@ -31,7 +35,7 @@ describe('33acrossBidAdapter:', function () { ], transactionId: 't1' } - ] + ]; this.sandbox = sinon.sandbox.create(); }); @@ -154,23 +158,19 @@ describe('33acrossBidAdapter:', function () { } } const builtServerRequests = buildRequests(this.bidRequests); - expect(builtServerRequests).to.deep.equal([serverRequest]); + expect(builtServerRequests).to.deep.equal([ serverRequest ]); expect(builtServerRequests.length).to.equal(1); }); - it('returns corresponding server requests for each valid test bidRequest', function() { - delete this.bidRequests[0].params.siteId; - this.bidRequests[0].params.site = { - id: SITE_ID, - page: 'http://test-url.com' - } - this.bidRequests[0].params.customHeaders = { - foo: 'bar' - }; - this.bidRequests[0].params.url = '//staging-ssc.33across.com/api/v1/hb'; + it('returns corresponding test server requests for each valid bidRequest', function() { + this.sandbox.stub(config, 'getConfig', () => { + return { + 'url': 'https://foo.com/hb/' + } + }); const ttxRequest = { - imp: [{ + imp: [ { banner: { format: [ { @@ -190,28 +190,24 @@ describe('33acrossBidAdapter:', function () { prod: PRODUCT_ID } } - }], + } ], site: { - id: SITE_ID, - page: 'http://test-url.com' + id: SITE_ID }, id: 'b1' }; const serverRequest = { method: 'POST', - url: '//staging-ssc.33across.com/api/v1/hb', + url: 'https://foo.com/hb/', data: JSON.stringify(ttxRequest), options: { contentType: 'application/json', - withCredentials: false, - customHeaders: { - foo: 'bar' - } + withCredentials: false } }; const builtServerRequests = buildRequests(this.bidRequests); - expect(builtServerRequests).to.deep.equal([serverRequest]); + expect(builtServerRequests).to.deep.equal([ serverRequest ]); expect(builtServerRequests.length).to.equal(1); }); @@ -221,6 +217,46 @@ describe('33acrossBidAdapter:', function () { }); describe('interpretResponse', function() { + beforeEach(function() { + this.ttxRequest = { + imp: [ { + banner: { + format: [ + { + w: 300, + h: 250, + ext: {} + }, + { + w: 728, + h: 90, + ext: {} + } + ] + }, + ext: { + ttx: { + prod: PRODUCT_ID + } + } + } ], + site: { + id: SITE_ID, + page: 'http://test-url.com' + }, + id: 'b1' + }; + this.serverRequest = { + method: 'POST', + url: '//staging-ssc.33across.com/api/v1/hb', + data: JSON.stringify(this.ttxRequest), + options: { + contentType: 'application/json', + withCredentials: false + } + }; + }); + context('when exactly one bid is returned', function() { it('interprets and returns the single bid response', function() { const serverResponse = { @@ -229,7 +265,7 @@ describe('33acrossBidAdapter:', function () { id: 'b1', seatbid: [ { - bid: [{ + bid: [ { id: '1', adm: '

I am an ad

', ext: { @@ -240,7 +276,7 @@ describe('33acrossBidAdapter:', function () { h: 250, w: 300, price: 0.0938 - }] + } ] } ] }; @@ -258,7 +294,7 @@ describe('33acrossBidAdapter:', function () { netRevenue: true } - expect(interpretResponse({body: serverResponse})).to.deep.equal([bidResponse]); + expect(interpretResponse({ body: serverResponse }, this.serverRequest)).to.deep.equal([ bidResponse ]); }); }); @@ -271,7 +307,7 @@ describe('33acrossBidAdapter:', function () { seatbid: [] }; - expect(interpretResponse({body: serverResponse})).to.deep.equal([]); + expect(interpretResponse({ body: serverResponse }, this.serverRequest)).to.deep.equal([]); }); }); @@ -283,7 +319,7 @@ describe('33acrossBidAdapter:', function () { id: 'b1', seatbid: [ { - bid: [{ + bid: [ { id: '1', adm: '

I am an ad

', ext: { @@ -310,7 +346,7 @@ describe('33acrossBidAdapter:', function () { ] }, { - bid: [{ + bid: [ { id: '3', adm: '

I am an ad

', ext: { @@ -321,7 +357,7 @@ describe('33acrossBidAdapter:', function () { h: 250, w: 300, price: 0.0938 - }] + } ] } ] }; @@ -339,96 +375,46 @@ describe('33acrossBidAdapter:', function () { netRevenue: true } - expect(interpretResponse({body: serverResponse})).to.deep.equal([bidResponse]); + expect(interpretResponse({ body: serverResponse }, this.serverRequest)).to.deep.equal([ bidResponse ]); }); }); - }); - - describe('getUserSyncs', function() { - beforeEach(function() { - this.bidderRequests = { - bids: [ - { - params: { - siteId: 'id1', - productId: 'p1' - } - }, - { - params: { - siteId: 'id2', - productId: 'p1' - } - } - ] - }; - this.testBidderRequests = { - bids: [ - { - params: { - site: { id: 'id1' }, - productId: 'p1', - syncUrl: 'https://staging-de.tynt.com/deb/v2?m=xch' - } - }, - { - params: { - site: { id: 'id2' }, - productId: 'p1', - syncUrl: 'https://staging-de.tynt.com/deb/v2?m=xch' - } - } - ] - }; + context('and register user sync', function() { + it('via the production endpoint', function() { + const spy = this.sandbox.spy(userSync, 'registerSync'); + const serverResponse = { + cur: 'USD', + ext: {}, + id: 'b1', + seatbid: [] + } + interpretResponse({ body: serverResponse }, this.serverRequest); + const syncUrl = `${SYNC_ENDPOINT}&id=${this.ttxRequest.site.id}`; - this.syncs = [ - { - type: 'iframe', - url: 'https://de.tynt.com/deb/v2?m=xch&id=id1' - }, - { - type: 'iframe', - url: 'https://de.tynt.com/deb/v2?m=xch&id=id2' - }, - ]; + const registerSyncCalled = spy.calledWith('iframe', '33across', syncUrl); + expect(registerSyncCalled).to.be.true; + }); - this.testSyncs = [ - { - type: 'iframe', - url: 'https://staging-de.tynt.com/deb/v2?m=xch&id=id1' - }, - { - type: 'iframe', - url: 'https://staging-de.tynt.com/deb/v2?m=xch&id=id2' - }, - ]; - }); + it('via the test endpoint', function() { + const spy = this.sandbox.spy(userSync, 'registerSync'); - context('when iframe is not enabled', function() { - it('returns empty sync array', function() { - const syncOptions = {}; - expect(getUserSyncs(syncOptions, this.bidderRequests)).to.deep.equal([]); - }); - }); + this.sandbox.stub(config, 'getConfig', () => { + return { + 'syncUrl': 'https://foo.com/deb/v2?m=xch' + } + }); - context('when iframe is enabled', function() { - it('returns sync array equal to number of bids for ttx', function() { - const syncOptions = { - iframeEnabled: true - }; - const syncs = getUserSyncs(syncOptions, this.bidderRequests); - expect(syncs.length).to.equal(this.bidderRequests.bids.length); - expect(syncs).to.deep.equal(this.syncs); - }); + const serverResponse = { + cur: 'USD', + ext: {}, + id: 'b1', + seatbid: [] + } + interpretResponse({ body: serverResponse }, this.serverRequest); + const syncUrl = `https://foo.com/deb/v2?m=xch&id=${this.ttxRequest.site.id}`; - it('returns sync array equal to number of test bids for ttx', function() { - const syncOptions = { - iframeEnabled: true - }; - const syncs = getUserSyncs(syncOptions, this.testBidderRequests); - expect(syncs.length).to.equal(this.testBidderRequests.bids.length); - expect(syncs).to.deep.equal(this.testSyncs); + const registerSyncCalled = spy.calledWith('iframe', '33across', syncUrl); + expect(registerSyncCalled).to.be.true; }); }); }); From b8f2ab5e4b8ce7c6455ae4d9e45948bf480af586 Mon Sep 17 00:00:00 2001 From: Aparna Hegde Date: Wed, 6 Dec 2017 17:34:40 -0500 Subject: [PATCH 4/5] Minor feedback changes --- gulpfile.js | 2 +- modules/33acrossBidAdapter.js | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 92851bbc192..e9b193c7f4f 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -234,7 +234,7 @@ gulp.task('watch', function () { }); gulp.task('lint', () => { - return gulp.src(['src/**/*.js', 'modules/**/*.js', 'test/**/*.js']) + return gulp.src(['modules/**/33across*.js', 'test/**/33across*.js']) .pipe(eslint()) .pipe(eslint.format('stylish')) .pipe(eslint.failAfterError()); diff --git a/modules/33acrossBidAdapter.js b/modules/33acrossBidAdapter.js index 4b5bf680e14..8e73d5c87b4 100644 --- a/modules/33acrossBidAdapter.js +++ b/modules/33acrossBidAdapter.js @@ -1,5 +1,5 @@ +import { userSync } from 'src/userSync' const { registerBidder } = require('../src/adapters/bidderFactory'); -const { userSync } = require('../src/userSync'); const { config } = require('../src/config'); const BIDDER_CODE = '33across'; @@ -73,7 +73,13 @@ function _getFormatSize(sizeArr) { // Register one sync per bid since each ad unit may potenitally be linked to a uniqe guid // Sync type will always be 'iframe' for 33Across function _registerUserSyncs(requestData) { - const ttxRequest = JSON.parse(requestData); + let ttxRequest; + try { + ttxRequest = JSON.parse(requestData); + } catch (err) { + // No point in trying to register sync since the requisite data cannot be parsed. + return; + } const ttxSettings = config.getConfig('ttxSettings'); let syncUrl = (ttxSettings && ttxSettings.syncUrl) || SYNC_ENDPOINT; From 5d96533db7f7c6cee75f66ef8b279244b08000bf Mon Sep 17 00:00:00 2001 From: Aparna Hegde Date: Wed, 6 Dec 2017 17:35:16 -0500 Subject: [PATCH 5/5] Re-instated lint check for all files which was accidentally commited after testing --- gulpfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gulpfile.js b/gulpfile.js index e9b193c7f4f..92851bbc192 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -234,7 +234,7 @@ gulp.task('watch', function () { }); gulp.task('lint', () => { - return gulp.src(['modules/**/33across*.js', 'test/**/33across*.js']) + return gulp.src(['src/**/*.js', 'modules/**/*.js', 'test/**/*.js']) .pipe(eslint()) .pipe(eslint.format('stylish')) .pipe(eslint.failAfterError());