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 2 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
47 changes: 28 additions & 19 deletions modules/rubiconBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,11 @@ export const spec = {
}

const bidFpd = {
user: bidRequest.params.visitor || {},
context: bidRequest.params.inventory || {}
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.


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

applyFPD(utils.mergeDeep({}, config.getConfig('fpd') || {}, bidRequest.fpd || {}, bidFpd), VIDEO, data);

Expand Down Expand Up @@ -516,11 +516,11 @@ export const spec = {
}

const bidFpd = {
user: bidRequest.params.visitor || {},
context: bidRequest.params.inventory || {}
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

};

if (bidRequest.params.keywords) bidFpd.context.keywords = bidRequest.params.keywords;
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


applyFPD(utils.mergeDeep({}, config.getConfig('fpd') || {}, bidRequest.fpd || {}, bidFpd), BANNER, data);

Expand Down Expand Up @@ -885,21 +885,22 @@ function addVideoParameters(data, bidRequest) {
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(e, t) {
if (typeof e === 'object' && !Array.isArray(e)) {
utils.logWarn('Rubicon: Filtered FPD key: ', t, ': Expected value to be string, integer, or an array of strings/ints');
} else if (e) {
return (Array.isArray(e)) ? e.filter(value => {
if (typeof value !== 'object' && value) return value;

utils.logWarn('Rubicon: Filtered value: ', value, 'for key', t, ': Expected value to be string, integer, or an array of strings/ints');
}).toString() : e.toString();
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 => fpd[type][value]).reduce((result, key) => {
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]]

Expand All @@ -911,9 +912,8 @@ function applyFPD(fpd, mediaType, data) {
} 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.mergeDeep(result, {ext: {data: {adserver: {[name.toLowerCase()]: value.replace(/^\/+/, '')}}}});
}) : utils.mergeDeep(result, {ext: {data: {[key.toLowerCase()]: fpd[type][key].replace(/^\/+/, '')}}});
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]}}});
}
Expand All @@ -923,6 +923,7 @@ function applyFPD(fpd, mediaType, data) {

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);

Expand All @@ -931,6 +932,14 @@ function applyFPD(fpd, mediaType, data) {
}
});

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;
Expand Down
20 changes: 10 additions & 10 deletions test/spec/modules/rubiconBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ describe('the rubicon adapter', function () {

it('should merge first party data from getConfig with the bid params, if present', () => {
const context = {
keywords: ['e', 'f'],
keywords: 'e,f',
rating: '4-star',
data: {
page: 'home'
Expand All @@ -838,7 +838,7 @@ describe('the rubicon adapter', function () {
gender: 'M',
yob: '1984',
geo: {country: 'ca'},
keywords: ['d'],
keywords: 'd',
data: {
age: 40
}
Expand All @@ -855,7 +855,7 @@ describe('the rubicon adapter', function () {
});

const expectedQuery = {
'kw': 'e,f,a,b,c,d',
'kw': 'a,b,c,d',
'tg_v.ucat': 'new',
'tg_v.lastsearch': 'iphone',
'tg_v.likes': 'sports,video games',
Expand Down Expand Up @@ -1875,14 +1875,14 @@ describe('the rubicon adapter', function () {
data: {
page: 'home'
},
keywords: ['e', 'f'],
keywords: 'e,f',
rating: '4-star'
};
const user = {
data: {
age: 31
},
keywords: ['d'],
keywords: 'd',
gender: 'M',
yob: '1984',
geo: {country: 'ca'}
Expand Down Expand Up @@ -1910,8 +1910,8 @@ describe('the rubicon adapter', function () {
delete expected.site.keywords;
delete expected.user.keywords;

expect(request.data.site.keywords).to.deep.equal(['e', 'f', 'a', 'b', 'c']);
expect(request.data.user.keywords).to.deep.equal(['d']);
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);
});
Expand Down Expand Up @@ -1945,7 +1945,7 @@ describe('the rubicon adapter', function () {
);

const [request] = spec.buildRequests(bidderRequest.bids, bidderRequest);
expect(request.data.site.ext.data.pbadslot).to.equal('1234567890');
expect(request.data.imp[0].ext.context.data.pbadslot).to.equal('1234567890');
});

it('should include GAM ad unit in bid request', function () {
Expand All @@ -1964,8 +1964,8 @@ describe('the rubicon adapter', function () {
);

const [request] = spec.buildRequests(bidderRequest.bids, bidderRequest);
expect(request.data.site.ext.data.adserver.adslot).to.equal('1234567890');
expect(request.data.site.ext.data.adserver.name).to.equal('adServerName1');
expect(request.data.imp[0].ext.context.data.adserver.adslot).to.equal('1234567890');
expect(request.data.imp[0].ext.context.data.adserver.name).to.equal('adServerName1');
});

it('should use the integration type provided in the config instead of the default', () => {
Expand Down