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.

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

Option to use a configurable vendor preset for s2sConfig #2073

Merged
merged 8 commits into from
Feb 12, 2018
Merged

Option to use a configurable vendor preset for s2sConfig #2073

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1832bdc
initial commit to support pbs vendor config options
jsnellbaker Jan 25, 2018
b1f3823
correcting a value in a test case
jsnellbaker Jan 25, 2018
c1c6462
adding rubicon config info and updated var names
jsnellbaker Jan 30, 2018
ee6bdb4
made changes to unit tests, changed cookieSet default, and fixed smal…
jsnellbaker Jan 31, 2018
5488050
tweaking logic of main check to read it better
jsnellbaker Feb 2, 2018
7e8ec2c
adjusting rubicon vendor values and default config to fix issue
jsnellbaker Feb 2, 2018
6bc107d
undoing default config change
jsnellbaker Feb 2, 2018
b901fc1
additional changes to rubicon defaults
jsnellbaker Feb 9, 2018
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
52 changes: 46 additions & 6 deletions modules/prebidServerBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,40 @@ const DEFAULT_S2S_CURRENCY = 'USD';
const DEFAULT_S2S_NETREVENUE = true;

let _s2sConfig;

const s2sDefaultConfig = {
enabled: false,
timeout: 1000,
maxBids: 1,
adapter: 'prebidServer'
};

config.setDefaults({
's2sConfig': {
enabled: false,
timeout: 1000,
maxBids: 1,
adapter: 'prebidServer'
}
's2sConfig': s2sDefaultConfig
});

// accountId and bidders params are not included here, should be configured by end-user
const availVendorDefaults = {
'appnexus': {
adapter: 'prebidServer',
cookieSet: true,
cookieSetUrl: '//acdn.adnxs.com/cookieset/cs.js',
enabled: true,
endpoint: '//prebid.adnxs.com/pbs/v1/auction',
syncEndpoint: '//prebid.adnxs.com/pbs/v1/cookie_sync',
timeout: 1000
},
'rubicon': {
adapter: 'prebidServer',
cookieSet: true,
cookieSetUrl: 'https://secure-assets.rubiconproject.com/utils/cookieset/cs.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed -- please remove the cookieSetUrl for Rubicon and turn cookieSet to false. Thanks. Also make "endpoint" protocol-relative. Thanks.

enabled: true,
endpoint: 'https://prebid-server.rubiconproject.com/auction',
Copy link
Member

Choose a reason for hiding this comment

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

@bretg
You want these hardcoded to HTTPS right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We've been avoiding the protocol-relative URLs due to articles like

https://stackoverflow.com/questions/28446314/why-use-protocol-relative-urls-at-all
https://jeremywagner.me/blog/stop-using-the-protocol-relative-url/

You haven't seen issues with protocol-relative URLs?

In any case, go ahead with this for now and we'll start the conversation internally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Talked to @mkendall07 -- the issue here is that non-secure demand is left on the table for HTTPS.

So, yes, let's switch the endpoints to protocol-relative. Thanks Matt. We'll take this discussion up internally as well.

syncEndpoint: 'https://prebid-server.rubiconproject.com/cookie_sync',
timeout: 500
}
};

/**
* Set config for server to server header bidding
* @typedef {Object} options - required
Expand All @@ -42,6 +67,21 @@ config.setDefaults({
* @property {string} [cookieSetUrl] url for cookie set library, if passed then cookieSet is enabled
*/
function setS2sConfig(options) {
if (options.defaultVendor) {
let vendor = options.defaultVendor

if (availVendorDefaults.hasOwnProperty(vendor)) {
Object.keys(availVendorDefaults[vendor]).forEach(function(vendorKey) {
if (s2sDefaultConfig[vendorKey] === options[vendorKey]) {
Copy link
Member

@mkendall07 mkendall07 Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I read somewhere that you shouldn't write code as clever as you are, because debugging code is 2x more difficult than writing it. So if you write clever code, you by definition cannot debug it... :)

Ok it's probably an exaggeration but the point remains. It took me awhile to figure out why this works - it's because its compare undefined to undefined (in the case of a missing key) and so it's actually proceeding to copy to undefined key over. It's not necessarily wrong, it's just hard to read. At minimum it needs a comment, or ideally a nicer way to understand what's going on in code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback @mkendall07, I've just finished making a slight change to hopefully infer the logic better. I also put in a comment as a backup. Please let me know your thoughts.

options[vendorKey] = availVendorDefaults[vendor][vendorKey]
}
});
} else {
utils.logError('Incorrect or unavailable prebid server default vendor option: ' + vendor);
return false;
}
}

let keys = Object.keys(options);

if (['accountId', 'bidders', 'endpoint'].filter(key => {
Expand Down
57 changes: 57 additions & 0 deletions test/spec/modules/prebidServerBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,5 +475,62 @@ describe('S2S Adapter', () => {
config.setConfig({ s2sConfig: options });
sinon.assert.calledOnce(logErrorSpy);
});

it('should log error when vendor does not exist', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a test to show there's a failure when 'defaultVendor' doesn't exist but endpoint isn't specified? This will show there's no default-defaultVendor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @bretg - I'm not sure I follow what you mean. Can you please clarify further when you get the chance?

Copy link
Member

Choose a reason for hiding this comment

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

I think he's saying to add a test case to assert an error when both endpoint & vendor default are not specified in the S2S config (or vendor default doesn't exist)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, thanks for that clarification. I've added a test case in the next commit.

const options = {
accountId: '1',
bidders: ['appnexus'],
defaultVendor: 'mytest'
};

config.setConfig({ s2sConfig: options });
sinon.assert.calledOnce(logErrorSpy);
});

it('should configure the s2sConfig object with appnexus vendor defaults unless specified by user', () => {
const options = {
accountId: '123',
bidders: ['appnexus'],
defaultVendor: 'appnexus',
timeout: 750
};

config.setConfig({ s2sConfig: options });
sinon.assert.notCalled(logErrorSpy);

let vendorConfig = config.getConfig('s2sConfig');
expect(vendorConfig).to.have.property('accountId', '123');
expect(vendorConfig).to.have.property('adapter', 'prebidServer');
expect(vendorConfig.bidders).to.deep.equal(['appnexus']);
expect(vendorConfig.cookieSet).to.be.true;
expect(vendorConfig).to.have.property('cookieSetUrl', '//acdn.adnxs.com/cookieset/cs.js');
expect(vendorConfig.enabled).to.be.true;
expect(vendorConfig).to.have.property('endpoint', '//prebid.adnxs.com/pbs/v1/auction');
expect(vendorConfig).to.have.property('syncEndpoint', '//prebid.adnxs.com/pbs/v1/cookie_sync');
expect(vendorConfig).to.have.property('timeout', 750);
});

it('should configure the s2sConfig object with rubicon vendor defaults unless specified by user', () => {
const options = {
accountId: 'abc',
bidders: ['rubicon'],
defaultVendor: 'rubicon',
timeout: 750
};

config.setConfig({ s2sConfig: options });
sinon.assert.notCalled(logErrorSpy);

let vendorConfig = config.getConfig('s2sConfig');
expect(vendorConfig).to.have.property('accountId', 'abc');
expect(vendorConfig).to.have.property('adapter', 'prebidServer');
expect(vendorConfig.bidders).to.deep.equal(['rubicon']);
expect(vendorConfig.cookieSet).to.be.true;
expect(vendorConfig).to.have.property('cookieSetUrl', 'https://secure-assets.rubiconproject.com/utils/cookieset/cs.js');
expect(vendorConfig.enabled).to.be.true;
expect(vendorConfig).to.have.property('endpoint', 'https://prebid-server.rubiconproject.com/auction');
expect(vendorConfig).to.have.property('syncEndpoint', 'https://prebid-server.rubiconproject.com/cookie_sync');
expect(vendorConfig).to.have.property('timeout', 750);
});
});
});