Skip to content

Aliasbidder fix #1652

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 6, 2017
Merged

Aliasbidder fix #1652

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b7ab1c1
aliasBidder did not work for bidderFactory
jaiminpanchal27 Oct 3, 2017
57c6167
Merge branch 'master' of github.com:prebid/Prebid.js into aliasbidder…
jaiminpanchal27 Oct 3, 2017
3d9d2dc
Merge branch 'master' of github.com:prebid/Prebid.js into aliasbidder…
jaiminpanchal27 Oct 3, 2017
9352eb7
Merge branch 'master' of github.com:prebid/Prebid.js into aliasbidder…
jaiminpanchal27 Oct 4, 2017
df9a5c0
added function to get adapter spec
jaiminpanchal27 Oct 4, 2017
e9acb26
Freezing spec and moved unit tests
jaiminpanchal27 Oct 6, 2017
049a8a5
Updated test case to not import adapter
jaiminpanchal27 Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/adaptermanager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import { flatten, getBidderCodes, getDefinedParams, shuffle } from './utils';
import { mapSizes } from './sizeMapping';
import { processNativeAdUnitParams, nativeAdapters } from './native';
import { newBidder } from './adapters/bidderFactory';

var utils = require('./utils.js');
var CONSTANTS = require('./constants.json');
Expand Down Expand Up @@ -192,6 +193,13 @@ function transformHeightWidth(adUnit) {
return sizesObj;
}

function getSupportedMediaTypes(bidderCode) {
let result = [];
if (exports.videoAdapters.includes(bidderCode)) result.push('video');
if (nativeAdapters.includes(bidderCode)) result.push('native');
return result;
}

exports.videoAdapters = []; // added by adapterLoader for now

exports.registerBidAdapter = function (bidAdaptor, bidderCode, {supportedMediaTypes = []} = {}) {
Expand All @@ -218,14 +226,24 @@ exports.aliasBidAdapter = function (bidderCode, alias) {

if (typeof existingAlias === 'undefined') {
var bidAdaptor = _bidderRegistry[bidderCode];

if (typeof bidAdaptor === 'undefined') {
utils.logError('bidderCode "' + bidderCode + '" is not an existing bidder.', 'adaptermanager.aliasBidAdapter');
} else {
try {
let newAdapter = new bidAdaptor.constructor();
newAdapter.setBidderCode(alias);
this.registerBidAdapter(newAdapter, alias);
let newAdapter;
let supportedMediaTypes = getSupportedMediaTypes(bidderCode);
// Have kept old code to support backward compatibilitiy.
// Remove this if loop when all adapters are supporting bidderFactory. i.e When Prebid.js is 1.0
if (bidAdaptor.constructor.prototype != Object.prototype) {
newAdapter = new bidAdaptor.constructor();
newAdapter.setBidderCode(alias);
} else {
let spec = Object.assign({}, bidAdaptor, { code: alias });
newAdapter = newBidder(spec);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little buggy.

The bidderFactory accepts a spec, but only returns an object like:

{
  callBids: function() { ... }
}

So in this code, you're using objects of the shape:

newAdapter = newBidder({
  code: alias,
  callBids: function() { ... }
});

...but newBIdder needs a spec. The typedef doesn't have callBids, but does require several other things which you're not giving here (interpretResponse, buildRequests, etc).

So... if callBids gets called on an aliased bidder, there'll be all sorts of errors about missing functions.

}
this.registerBidAdapter(newAdapter, alias, {
supportedMediaTypes
});
} catch (e) {
utils.logError(bidderCode + ' bidder does not currently support aliasing.', 'adaptermanager.aliasBidAdapter');
}
Expand Down
29 changes: 29 additions & 0 deletions test/spec/unit/pbjs_api_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
getAdUnits
} from 'test/fixtures/fixtures';
import { config as configObj } from 'src/config';
import { newBidder, registerBidder } from 'src/adapters/bidderFactory';

var assert = require('chai').assert;
var expect = require('chai').expect;
Expand Down Expand Up @@ -1288,6 +1289,18 @@ describe('Unit: Prebid Module', function () {
});

describe('aliasBidder', () => {
let spec;
const CODE = 'sampleBidder';
before(() => {
spec = {
code: CODE,
isBidRequestValid: () => {},
buildRequests: () => {},
interpretResponse: () => {},
getUserSyncs: () => {}
};
});

it('should call adaptermanager.aliasBidder', () => {
const aliasBidAdapterSpy = sinon.spy(adaptermanager, 'aliasBidAdapter');
const bidderCode = 'testcode';
Expand All @@ -1306,6 +1319,22 @@ describe('Unit: Prebid Module', function () {
assert.ok(logErrorSpy.calledWith(error), 'expected error was logged');
utils.logError.restore();
});

it('should add alias to registry', () => {
const bidderCode = 'appnexusAst';
const alias = 'testalias';
$$PREBID_GLOBAL$$.aliasBidder(bidderCode, alias);
expect(adaptermanager.bidderRegistry).to.have.property(alias);
});

it('should add alias to registry when original adapter is using bidderFactory', function() {
const thisSpec = Object.assign(spec, { supportedMediaTypes: ['video'] });
registerBidder(thisSpec);
const alias = 'aliasBidder';
$$PREBID_GLOBAL$$.aliasBidder(CODE, alias);
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to avoid $$PREBID_GLOBAL$$ in tests (see #1508).

Prebid-specifics aside... strong unit tests cover the smallest amount of code possible. All your changes are in adapterManager... so it's much better if your tests import the adapterManager and call the functions directly. The $$PREBID_GLOBAL$$.aliasBidder function is at a higher level, and would be better tested by stubbing the adapterManager with sinon and making sure the methods get called with the right args.

On a less important note... you could probably write these in test/spec/unit/adapterManager_spec.js. This file is much larger than it should be already.

expect(adaptermanager.bidderRegistry).to.have.property(alias);
expect(adaptermanager.videoAdapters).to.include(alias);
});
});

describe('setPriceGranularity', () => {
Expand Down