-
Notifications
You must be signed in to change notification settings - Fork 0
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
Xch 2158 support userid #37
Conversation
) | ||
} | ||
|
||
if (uspConsent) { |
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.
Optional feedback...
I am seeing many if
statements checking for the truthyness of the subfields
could it be possible to move that if
statement to setExtension? and probably just keep the one that checks for isArray.
e.g.
function setExtension(obj = {}, key, value='') { // Values such as undefined will default to an empty string
if (!String(value)) { // Values such as 0 or null will be valid for the "ext" assignment
return Object.assign({}, obj);
}
return Object.assign({}, obj, {
ext: Object.assign({}, obj.ext, {
[key]: 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.
Can you elaborate why you are recommending this? In its current form the check to truthyness makes it very clear to the reader when the ORTB field ought to be set or not.
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.
Similar to the feedback I provided in the gdprApplies
suggestion above, I was trying to avoid behavior conditional branching, in my opinion the first if (!String(value))
statement still makes it clear the check for truthyness, but maybe this solution is unnecessarily more elaborated... or maybe the function name setExtension
is not ideal to make the point clear that the extension will be set based on a condition, it could be renamed to setExtensionIfPresent
..
either way if you find it to be more confusing, feel free to ignore this suggestion.
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.
So I want to keep setExtension
simple, it does what it's asked to do i.e. set extension. The caller has the responsibility to figure out if that extension needs to be set or not. Setting an extension fields may not always predicate on the truthyness of a condition, we may want to set it regardless. So I'll keep this code structure as is for the present. Also, I like the fact that branching it makes it clear to the reader what the logic is and also to the developer to add add unit tests accordingly (to test for the outcomes of the condition or not)
bidRequest.userIdAsEids
as an Array. It will not read individual sources underbidRequest
(such asbidRequest.id5id
etc)user
,regs
,source
regardless of whether or not there is the relevant data (CCPA, GDPR, Schain etc). Therefore the request payload sent out to TTX was needlessly bloated with empty ext field values. In this PR, theext
and even the parent fields are conditionally set.Resolves https://jira.internal.33across.com/browse/XCH-2158