-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
appnexus adapter support empty keyvalues in bidder params #3257
appnexus adapter support empty keyvalues in bidder params #3257
Conversation
modules/appnexusBidAdapter.js
Outdated
@@ -178,6 +178,14 @@ export const spec = { | |||
params.use_pmt_rule = (typeof params.usePaymentRule === 'boolean') ? params.usePaymentRule : false; | |||
if (params.usePaymentRule) { delete params.usePaymentRule; } | |||
|
|||
if (utils.isArray(params.keywords) && params.keywords.length > 0) { | |||
params.keywords.forEach(function(keyPairObj) { | |||
if (utils.isArray(keyPairObj.value) && keyPairObj.value.length > 0 && keyPairObj.value[0] === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if statement here and on line 360 are same. Please move in some function.
@@ -261,6 +261,8 @@ describe('AppNexusAdapter', function () { | |||
singleArrNum: [5], | |||
multiValMixed: ['value1', 2, 'value3'], | |||
singleValNum: 123, | |||
emptyStr: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is valid use case but what will happen if someone adds emptyStr: ' '
(space is added as value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the space character is treated as the value for the key-pair and is preserved in the ut request.
eg "keywords":[{"key":"color","value":["red"]},{"key":"mytest"},{"key":"myspacetest","value":[" "]}]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up to the above, the treatment of the space character has not been changed through this PR. Using a space character as a keyvalue in master
would result in the same type of object for the ut request as noted above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return !!(utils.isArray(arr) && arr.length > 0); | ||
} | ||
|
||
function deleteValues(keyPairObj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; please rename to deleteEmptyValues
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just found one nitpick
* appnexus adapter support empty keyvalues in bidder params * create function to check if array is populated * refactor forEach functions
Type of change
Description of change
This change allows the appenxus bidder param
keywords
to accept an empty keyvalue and subsequently pass just thekey
param to the adserver.In terms of setup, the bidder param would just use an empty string as the value; examples below:
or
Based on the above, the
tags[...].keywords
object in the ut request would look like the following:A similar structure would be used in the request to PBS for the
imp[...].ext.appnexus.keywords
object.