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

Jump to bottom

Rubicon Bid Adapter FPD Update #6122

Merged
merged 8 commits into from
Feb 7, 2021
Merged

Rubicon Bid Adapter FPD Update #6122

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7680ffc
Update to consolidate applying FPD to both banner and video requests.…
mmoschovas Dec 11, 2020
924fc81
Refectored last push to:
mmoschovas Dec 16, 2020
5780fd4
Consolidated other FPD data logic into new function
mmoschovas Dec 16, 2020
c8bd004
Merge remote-tracking branch 'upstream/master' into rubicon-fpd-update
mmoschovas Jan 20, 2021
030da98
1. Update to move pbadslot and adserver data into imp[] as opposed to…
mmoschovas Jan 21, 2021
330dbe3
Removed unnecessary conditional
mmoschovas Jan 21, 2021
151b55b
Changed conditional to check for undefined type
mmoschovas Jan 21, 2021
21a2471
Update to consolidate several lines of duplicate code into one location
mmoschovas Feb 3, 2021
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
157 changes: 78 additions & 79 deletions modules/rubiconBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,46 +254,14 @@ export const spec = {
utils.deepSetValue(data, 'source.ext.schain', bidRequest.schain);
}

const siteData = Object.assign({}, bidRequest.params.inventory, config.getConfig('fpd.context'));
const userData = Object.assign({}, bidRequest.params.visitor, config.getConfig('fpd.user'));
if (!utils.isEmpty(siteData) || !utils.isEmpty(userData)) {
const bidderData = {
bidders: [ bidderRequest.bidderCode ],
config: {
fpd: {}
}
};

if (!utils.isEmpty(siteData)) {
bidderData.config.fpd.site = siteData;
}

if (!utils.isEmpty(userData)) {
bidderData.config.fpd.user = userData;
}
const bidFpd = {
user: {...bidRequest.params.visitor} || {},
context: {...bidRequest.params.inventory} || {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this will work as expected.

Why did you change it from the previous

      const bidFpd = {
        user: bidRequest.params.visitor || {},
        context: bidRequest.params.inventory || {}
      }

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could just leave it as:

{...bidRequest.params.visitor}
{...bidRequest.params.inventory}

And it will do what is intended (default to {})

The || {} never will get executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I removed this already no? I pushed twice this morning

};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also place lines 257 - 262 into applyFpd function? save a couple duplicate names.


utils.deepSetValue(data, 'ext.prebid.bidderconfig.0', bidderData);
}
if (bidRequest.params.keywords) bidFpd.context.keywords = (utils.isArray(bidRequest.params.keywords)) ? bidRequest.params.keywords.join(',') : bidRequest.params.keywords;

/**
* Prebid AdSlot
* @type {(string|undefined)}
*/
const pbAdSlot = utils.deepAccess(bidRequest, 'fpd.context.pbAdSlot');
if (typeof pbAdSlot === 'string' && pbAdSlot) {
utils.deepSetValue(data.imp[0].ext, 'context.data.pbadslot', pbAdSlot);
}

/**
* Copy GAM AdUnit and Name to imp
*/
['name', 'adSlot'].forEach(name => {
/** @type {(string|undefined)} */
const value = utils.deepAccess(bidRequest, `fpd.context.adserver.${name}`);
if (typeof value === 'string' && value) {
utils.deepSetValue(data.imp[0].ext, `context.data.adserver.${name.toLowerCase()}`, value);
}
});
applyFPD(utils.mergeDeep({}, config.getConfig('fpd') || {}, bidRequest.fpd || {}, bidFpd), VIDEO, data);

// if storedAuctionResponse has been set, pass SRID
if (bidRequest.storedAuctionResponse) {
Expand Down Expand Up @@ -547,49 +515,14 @@ export const spec = {
data['us_privacy'] = encodeURIComponent(bidderRequest.uspConsent);
}

// visitor properties
const visitorData = Object.assign({}, params.visitor, config.getConfig('fpd.user'));
Object.keys(visitorData).forEach((key) => {
if (visitorData[key] != null && key !== 'keywords') {
data[`tg_v.${key}`] = typeof visitorData[key] === 'object' && !Array.isArray(visitorData[key])
? JSON.stringify(visitorData[key])
: visitorData[key].toString(); // initialize array;
}
});

// inventory properties
const inventoryData = Object.assign({}, params.inventory, config.getConfig('fpd.context'));
Object.keys(inventoryData).forEach((key) => {
if (inventoryData[key] != null && key !== 'keywords') {
data[`tg_i.${key}`] = typeof inventoryData[key] === 'object' && !Array.isArray(inventoryData[key])
? JSON.stringify(inventoryData[key])
: inventoryData[key].toString();
}
});
const bidFpd = {
user: {...bidRequest.params.visitor} || {},
context: {...bidRequest.params.inventory} || {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same 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.

Same as above

};

// keywords
const keywords = (params.keywords || []).concat(
utils.deepAccess(config.getConfig('fpd.user'), 'keywords') || [],
utils.deepAccess(config.getConfig('fpd.context'), 'keywords') || []);
data.kw = Array.isArray(keywords) && keywords.length ? keywords.join(',') : '';

/**
* Prebid AdSlot
* @type {(string|undefined)}
*/
const pbAdSlot = utils.deepAccess(bidRequest, 'fpd.context.pbAdSlot');
if (typeof pbAdSlot === 'string' && pbAdSlot) {
data['tg_i.pbadslot'] = pbAdSlot.replace(/^\/+/, '');
}
if (bidRequest.params.keywords) bidFpd.context.keywords = (utils.isArray(bidRequest.params.keywords)) ? bidRequest.params.keywords.join(',') : bidRequest.params.keywords;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the plan was to not support the string or non array case in bidParams.

if (utils.isArray(bidRequest.params.keywords)) bidFpd.context.keywords = bidRequest.params.keywords.join(',');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was but then I reviewed the existing tests and one test explicitly states that it should not error if set as a string. I dont like it but will assume there is a reason for the test that could break things in production. So for the time being I made sure this allows for both


/**
* GAM Ad Unit
* @type {(string|undefined)}
*/
const gamAdUnit = utils.deepAccess(bidRequest, 'fpd.context.adServer.adSlot');
if (typeof gamAdUnit === 'string' && gamAdUnit) {
data['tg_i.dfp_ad_unit_code'] = gamAdUnit.replace(/^\/+/, '');
}
applyFPD(utils.mergeDeep({}, config.getConfig('fpd') || {}, bidRequest.fpd || {}, bidFpd), BANNER, data);

if (config.getConfig('coppa') === true) {
data['coppa'] = 1;
Expand Down Expand Up @@ -949,6 +882,72 @@ function addVideoParameters(data, bidRequest) {
data.imp[0].video.h = size[1]
}

function applyFPD(fpd, mediaType, data) {
const map = {user: {banner: 'tg_v.', code: 'user'}, context: {banner: 'tg_i.', code: 'site'}, adserver: 'dfp_ad_unit_code'};
let obj = {};
let impData = {};
let keywords = [];
const validate = function(prop, key) {
if (typeof prop === 'object' && !Array.isArray(prop)) {
utils.logWarn('Rubicon: Filtered FPD key: ', key, ': Expected value to be string, integer, or an array of strings/ints');
} else if (utils.isNumber(prop) || prop) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a simple

prop || prop === 0

would be a little more efficient. Most of the time the prop will not be falsey, so we should check that first regardless.

And we know it will only hit the other side of the || if it is falsey, and only falsey number is zero so better to strict check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is outdated as well

return (Array.isArray(prop)) ? prop.filter(value => {
if (typeof value !== 'object' && (utils.isNumber(value) || value)) return value.toString();

utils.logWarn('Rubicon: Filtered value: ', value, 'for key', key, ': Expected value to be string, integer, or an array of strings/ints');
}).toString() : prop.toString();
}
};

Object.keys(fpd).filter(value => fpd[value] && map[value] && typeof fpd[value] === 'object').forEach((type) => {
obj[map[type].code] = Object.keys(fpd[type]).filter(value => utils.isNumber(fpd[type][value]) || fpd[type][value]).reduce((result, key) => {
if (key === 'keywords') {
if (!Array.isArray(fpd[type][key]) && mediaType === BANNER) fpd[type][key] = [fpd[type][key]]

result[key] = fpd[type][key];

if (mediaType === BANNER) keywords = keywords.concat(fpd[type][key]);
ncolletti marked this conversation as resolved.
Show resolved Hide resolved
} else if (key === 'data') {
utils.mergeDeep(result, {ext: {data: fpd[type][key]}});
} else if (key === 'adServer' || key === 'pbAdSlot') {
(key === 'adServer') ? ['name', 'adSlot'].forEach(name => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These go inside of the imp array. Not at top level of the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to imp[0]

const value = validate(fpd[type][key][name]);
if (value) utils.deepSetValue(impData, `adserver.${name.toLowerCase()}`, value.replace(/^\/+/, ''))
}) : impData[key.toLowerCase()] = fpd[type][key].replace(/^\/+/, '')
} else {
utils.mergeDeep(result, {ext: {data: {[key]: fpd[type][key]}}});
}

return result;
}, {});

if (mediaType === BANNER) {
let duplicate = (typeof obj[map[type].code].ext === 'object' && obj[map[type].code].ext.data) || {};

Object.keys(duplicate).forEach((key) => {
const val = (key === 'adserver') ? duplicate.adserver.adslot : validate(duplicate[key], key);

if (val) data[(map[key]) ? `${map[type][BANNER]}${map[key]}` : `${map[type][BANNER]}${key}`] = val;
});
}
});

Object.keys(impData).forEach((key) => {
if (mediaType === BANNER) {
(map[key]) ? data[`tg_i.${map[key]}`] = impData[key].adslot : data[`tg_i.${key.toLowerCase()}`] = impData[key];
} else {
utils.mergeDeep(data.imp[0], {ext: {context: {data: {[key]: impData[key]}}}});
}
});

if (mediaType === BANNER) {
let kw = validate(keywords, 'keywords');
if (kw) data.kw = kw;
} else {
utils.mergeDeep(data, obj);
}
}

/**
* @param sizes
* @returns {*}
Expand Down
63 changes: 40 additions & 23 deletions test/spec/modules/rubiconBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,16 +826,22 @@ describe('the rubicon adapter', function () {
});
});

it('should use first party data from getConfig over the bid params, if present', () => {
it('should merge first party data from getConfig with the bid params, if present', () => {
const context = {
keywords: ['e', 'f'],
rating: '4-star'
keywords: 'e,f',
rating: '4-star',
data: {
page: 'home'
}
};
const user = {
keywords: ['d'],
gender: 'M',
yob: '1984',
geo: {country: 'ca'}
geo: {country: 'ca'},
keywords: 'd',
data: {
age: 40
}
};

sandbox.stub(config, 'getConfig').callsFake(key => {
Expand All @@ -849,14 +855,15 @@ describe('the rubicon adapter', function () {
});

const expectedQuery = {
'kw': 'a,b,c,d,e,f',
'kw': 'a,b,c,d',
'tg_v.ucat': 'new',
'tg_v.lastsearch': 'iphone',
'tg_v.likes': 'sports,video games',
'tg_v.gender': 'M',
'tg_v.age': '40',
'tg_v.yob': '1984',
'tg_v.geo': '{"country":"ca"}',
'tg_i.rating': '4-star',
'tg_i.rating': '5-star',
'tg_i.page': 'home',
'tg_i.prodtype': 'tech,mobile',
};

Expand Down Expand Up @@ -1865,11 +1872,17 @@ describe('the rubicon adapter', function () {
createVideoBidderRequest();

const context = {
keywords: ['e', 'f'],
data: {
page: 'home'
},
keywords: 'e,f',
rating: '4-star'
};
const user = {
keywords: ['d'],
data: {
age: 31
},
keywords: 'd',
gender: 'M',
yob: '1984',
geo: {country: 'ca'}
Expand All @@ -1885,18 +1898,22 @@ describe('the rubicon adapter', function () {
return utils.deepAccess(config, key);
});

const expected = [{
bidders: ['rubicon'],
config: {
fpd: {
site: Object.assign({}, bidderRequest.bids[0].params.inventory, context),
user: Object.assign({}, bidderRequest.bids[0].params.visitor, user)
}
}
}];

const [request] = spec.buildRequests(bidderRequest.bids, bidderRequest);
expect(request.data.ext.prebid.bidderconfig).to.deep.equal(expected);

const expected = {
site: Object.assign({}, context, context.data, bidderRequest.bids[0].params.inventory),
user: Object.assign({}, user, user.data, bidderRequest.bids[0].params.visitor)
};

delete expected.site.data;
delete expected.user.data;
delete expected.site.keywords;
delete expected.user.keywords;

expect(request.data.site.keywords).to.deep.equal('a,b,c');
expect(request.data.user.keywords).to.deep.equal('d');
expect(request.data.site.ext.data).to.deep.equal(expected.site);
expect(request.data.user.ext.data).to.deep.equal(expected.user);
});

it('should include storedAuctionResponse in video bid request', function () {
Expand Down Expand Up @@ -1935,7 +1952,7 @@ describe('the rubicon adapter', function () {
createVideoBidderRequest();
bidderRequest.bids[0].fpd = {
context: {
adserver: {
adServer: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this a previous bug being fixed?

Were we looking at all lowercase when it should have been camelCase the whole time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this should have been camelCase

adSlot: '1234567890',
name: 'adServerName1'
}
Expand Down Expand Up @@ -2079,7 +2096,7 @@ describe('the rubicon adapter', function () {
it('should not fail if keywords param is not an array', function () {
bidderRequest.bids[0].params.keywords = 'a,b,c';
const slotParams = spec.createSlotParams(bidderRequest.bids[0], bidderRequest);
expect(slotParams.kw).to.equal('');
expect(slotParams.kw).to.equal('a,b,c');
});
});

Expand Down