From a98a3496c2ee67047dc34d6278fcbcae808e36ae Mon Sep 17 00:00:00 2001 From: Jaimin Panchal Date: Fri, 9 Nov 2018 10:22:50 -0500 Subject: [PATCH 1/2] do not load external js if renderer defined on adUnit --- modules/appnexusBidAdapter.js | 3 ++ src/Renderer.js | 23 +++++++++++---- test/spec/modules/appnexusBidAdapter_spec.js | 30 +++++++++++++++++--- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/modules/appnexusBidAdapter.js b/modules/appnexusBidAdapter.js index cc0cac579da..1e77dfce564 100644 --- a/modules/appnexusBidAdapter.js +++ b/modules/appnexusBidAdapter.js @@ -211,6 +211,7 @@ function newRenderer(adUnitCode, rtbBid, rendererOptions = {}) { url: rtbBid.renderer_url, config: rendererOptions, loaded: false, + adUnitCode }); try { @@ -238,6 +239,7 @@ function newRenderer(adUnitCode, rtbBid, rendererOptions = {}) { * @return Bid */ function newBid(serverBid, rtbBid, bidderRequest) { + const bidRequest = utils.getBidRequest(serverBid.uuid, [bidderRequest]); const bid = { requestId: serverBid.uuid, cpm: rtbBid.cpm, @@ -246,6 +248,7 @@ function newBid(serverBid, rtbBid, bidderRequest) { currency: 'USD', netRevenue: true, ttl: 300, + adUnitCode: bidRequest.adUnitCode, appnexus: { buyerMemberId: rtbBid.buyer_member_id, dealPriority: rtbBid.deal_priority, diff --git a/src/Renderer.js b/src/Renderer.js index 3a156b2b86e..87f60e65668 100644 --- a/src/Renderer.js +++ b/src/Renderer.js @@ -1,5 +1,6 @@ import { loadScript } from './adloader'; import * as utils from './utils'; +import find from 'core-js/library/fn/array/find'; /** * @typedef {object} Renderer @@ -10,7 +11,7 @@ import * as utils from './utils'; */ export function Renderer(options) { - const { url, config, id, callback, loaded } = options; + const { url, config, id, callback, loaded, adUnitCode } = options; this.url = url; this.config = config; this.handlers = {}; @@ -35,12 +36,16 @@ export function Renderer(options) { this.process(); }); - // we expect to load a renderer url once only so cache the request to load script - loadScript(url, this.callback, true); + if (!isRendererDefinedOnAdUnit(adUnitCode)) { + // we expect to load a renderer url once only so cache the request to load script + loadScript(url, this.callback, true); + } else { + utils.logWarn(`External Js not loaded by Renderer since renderer url and callback is already defined on adUnit ${adUnitCode}`); + } } -Renderer.install = function({ url, config, id, callback, loaded }) { - return new Renderer({ url, config, id, callback, loaded }); +Renderer.install = function({ url, config, id, callback, loaded, adUnitCode }) { + return new Renderer({ url, config, id, callback, loaded, adUnitCode }); }; Renderer.prototype.getConfig = function() { @@ -94,3 +99,11 @@ export function isRendererRequired(renderer) { export function executeRenderer(renderer, bid) { renderer.render(bid); } + +function isRendererDefinedOnAdUnit(adUnitCode) { + const adUnits = $$PREBID_GLOBAL$$.adUnits; + const adUnit = find(adUnits, adUnit => { + return adUnit.code === adUnitCode; + }); + return !!(adUnit && adUnit.renderer && adUnit.renderer.url && adUnit.renderer.render); +} diff --git a/test/spec/modules/appnexusBidAdapter_spec.js b/test/spec/modules/appnexusBidAdapter_spec.js index e6afc8561b6..08e5f35cb8d 100644 --- a/test/spec/modules/appnexusBidAdapter_spec.js +++ b/test/spec/modules/appnexusBidAdapter_spec.js @@ -463,12 +463,18 @@ describe('AppNexusAdapter', function () { 'currency': 'USD', 'ttl': 300, 'netRevenue': true, + 'adUnitCode': 'code', 'appnexus': { 'buyerMemberId': 958 } } ]; - let bidderRequest; + let bidderRequest = { + bids: [{ + bidId: '3db3773286ee59', + adUnitCode: 'code' + }] + } let result = spec.interpretResponse({ body: response }, {bidderRequest}); expect(Object.keys(result[0])).to.have.members(Object.keys(expectedResponse[0])); }); @@ -505,7 +511,12 @@ describe('AppNexusAdapter', function () { }] }] }; - let bidderRequest; + let bidderRequest = { + bids: [{ + bidId: '84ab500420319d', + adUnitCode: 'code' + }] + } let result = spec.interpretResponse({ body: response }, {bidderRequest}); expect(result[0]).to.have.property('vastUrl'); @@ -538,7 +549,12 @@ describe('AppNexusAdapter', function () { }, 'impression_trackers': ['http://example.com'], }; - let bidderRequest; + let bidderRequest = { + bids: [{ + bidId: '3db3773286ee59', + adUnitCode: 'code' + }] + } let result = spec.interpretResponse({ body: response1 }, {bidderRequest}); expect(result[0].native.title).to.equal('Native Creative'); @@ -554,6 +570,7 @@ describe('AppNexusAdapter', function () { const bidderRequest = { bids: [{ + bidId: '3db3773286ee59', renderer: { options: { adText: 'configured' @@ -573,7 +590,12 @@ describe('AppNexusAdapter', function () { responseWithDeal.tags[0].ads[0].deal_priority = 'high'; responseWithDeal.tags[0].ads[0].deal_code = '123'; - let bidderRequest; + let bidderRequest = { + bids: [{ + bidId: '3db3773286ee59', + adUnitCode: 'code' + }] + } let result = spec.interpretResponse({ body: responseWithDeal }, {bidderRequest}); expect(Object.keys(result[0].appnexus)).to.include.members(['buyerMemberId', 'dealPriority', 'dealCode']); }); From c270f9cc7afa7e4303c7ed51cda32585c3a9e21b Mon Sep 17 00:00:00 2001 From: Jaimin Panchal Date: Tue, 20 Nov 2018 16:39:39 -0500 Subject: [PATCH 2/2] Added unit test --- test/spec/renderer_spec.js | 175 ++++++++++++++++++++++--------------- 1 file changed, 105 insertions(+), 70 deletions(-) diff --git a/test/spec/renderer_spec.js b/test/spec/renderer_spec.js index d4e90245ea5..7a7354add31 100644 --- a/test/spec/renderer_spec.js +++ b/test/spec/renderer_spec.js @@ -1,96 +1,131 @@ import { expect } from 'chai'; import { Renderer } from 'src/Renderer'; - -describe('Renderer: A renderer installed on a bid response', function () { - let testRenderer1; - let testRenderer2; - let spyRenderFn; - let spyEventHandler; - - beforeEach(function () { - testRenderer1 = Renderer.install({ - url: 'https://httpbin.org/post', - config: { test: 'config1' }, - id: 1 +import * as utils from 'src/utils'; + +describe('Renderer', function () { + describe('Renderer: A renderer installed on a bid response', function () { + let testRenderer1; + let testRenderer2; + let spyRenderFn; + let spyEventHandler; + + beforeEach(function () { + testRenderer1 = Renderer.install({ + url: 'https://httpbin.org/post', + config: { test: 'config1' }, + id: 1 + }); + testRenderer2 = Renderer.install({ + url: 'https://httpbin.org/post', + config: { test: 'config2' }, + id: 2 + }); + + spyRenderFn = sinon.spy(); + spyEventHandler = sinon.spy(); }); - testRenderer2 = Renderer.install({ - url: 'https://httpbin.org/post', - config: { test: 'config2' }, - id: 2 + + it('is an instance of Renderer', function () { + expect(testRenderer1 instanceof Renderer).to.equal(true); }); - spyRenderFn = sinon.spy(); - spyEventHandler = sinon.spy(); - }); + it('has expected properties ', function () { + expect(testRenderer1.url).to.equal('https://httpbin.org/post'); + expect(testRenderer1.config).to.deep.equal({ test: 'config1' }); + expect(testRenderer1.id).to.equal(1); + }); - it('is an instance of Renderer', function () { - expect(testRenderer1 instanceof Renderer).to.equal(true); - }); + it('returns config from getConfig method', function () { + expect(testRenderer1.getConfig()).to.deep.equal({ test: 'config1' }); + expect(testRenderer2.getConfig()).to.deep.equal({ test: 'config2' }); + }); - it('has expected properties ', function () { - expect(testRenderer1.url).to.equal('https://httpbin.org/post'); - expect(testRenderer1.config).to.deep.equal({ test: 'config1' }); - expect(testRenderer1.id).to.equal(1); - }); + it('sets a render function with setRender method', function () { + testRenderer1.setRender(spyRenderFn); + expect(typeof testRenderer1.render).to.equal('function'); + testRenderer1.render(); + expect(spyRenderFn.called).to.equal(true); + }); - it('returns config from getConfig method', function () { - expect(testRenderer1.getConfig()).to.deep.equal({ test: 'config1' }); - expect(testRenderer2.getConfig()).to.deep.equal({ test: 'config2' }); - }); + it('sets event handlers with setEventHandlers method and handles events with installed handlers', function () { + testRenderer1.setEventHandlers({ + testEvent: spyEventHandler + }); - it('sets a render function with setRender method', function () { - testRenderer1.setRender(spyRenderFn); - expect(typeof testRenderer1.render).to.equal('function'); - testRenderer1.render(); - expect(spyRenderFn.called).to.equal(true); - }); + expect(testRenderer1.handlers).to.deep.equal({ + testEvent: spyEventHandler + }); - it('sets event handlers with setEventHandlers method and handles events with installed handlers', function () { - testRenderer1.setEventHandlers({ - testEvent: spyEventHandler + testRenderer1.handleVideoEvent({ id: 1, eventName: 'testEvent' }); + expect(spyEventHandler.called).to.equal(true); }); - expect(testRenderer1.handlers).to.deep.equal({ - testEvent: spyEventHandler + it('pushes commands to queue if renderer is not loaded', function () { + testRenderer1.loaded = false; + testRenderer1.push(spyRenderFn); + expect(testRenderer1.cmd.length).to.equal(1); + + // clear queue for next tests + testRenderer1.cmd = []; }); - testRenderer1.handleVideoEvent({ id: 1, eventName: 'testEvent' }); - expect(spyEventHandler.called).to.equal(true); - }); + it('fires commands immediately if the renderer is loaded', function () { + const func = sinon.spy(); - it('pushes commands to queue if renderer is not loaded', function () { - testRenderer1.loaded = false; - testRenderer1.push(spyRenderFn); - expect(testRenderer1.cmd.length).to.equal(1); + testRenderer1.loaded = true; + testRenderer1.push(func); - // clear queue for next tests - testRenderer1.cmd = []; - }); + expect(testRenderer1.cmd.length).to.equal(0); - it('fires commands immediately if the renderer is loaded', function () { - const func = sinon.spy(); + sinon.assert.calledOnce(func); + }); - testRenderer1.loaded = true; - testRenderer1.push(func); + it('processes queue by calling each function in queue', function () { + testRenderer1.loaded = false; + const func1 = sinon.spy(); + const func2 = sinon.spy(); - expect(testRenderer1.cmd.length).to.equal(0); + testRenderer1.push(func1); + testRenderer1.push(func2); + expect(testRenderer1.cmd.length).to.equal(2); - sinon.assert.calledOnce(func); - }); + testRenderer1.process(); - it('processes queue by calling each function in queue', function () { - testRenderer1.loaded = false; - const func1 = sinon.spy(); - const func2 = sinon.spy(); + sinon.assert.calledOnce(func1); + sinon.assert.calledOnce(func2); + expect(testRenderer1.cmd.length).to.equal(0); + }); + }); - testRenderer1.push(func1); - testRenderer1.push(func2); - expect(testRenderer1.cmd.length).to.equal(2); + describe('3rd party renderer', function () { + let adUnitsOld; + let utilsSpy; + before(function () { + adUnitsOld = $$PREBID_GLOBAL$$.adUnits; + utilsSpy = sinon.spy(utils, 'logWarn'); + }); - testRenderer1.process(); + after(function() { + $$PREBID_GLOBAL$$.adUnits = adUnitsOld; + utilsSpy.restore(); + }); - sinon.assert.calledOnce(func1); - sinon.assert.calledOnce(func2); - expect(testRenderer1.cmd.length).to.equal(0); + it('should not load renderer and log warn message', function() { + $$PREBID_GLOBAL$$.adUnits = [{ + code: 'video1', + renderer: { + url: 'http://cdn.adnxs.com/renderer/video/ANOutstreamVideo.js', + render: sinon.spy() + } + }] + + let testRenderer = Renderer.install({ + url: 'https://httpbin.org/post', + config: { test: 'config1' }, + id: 1, + adUnitCode: 'video1' + }); + expect(utilsSpy.callCount).to.equal(1); + }); }); });