-
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
FTRACK USER ID MODULE: tweaking the createEidsArray() method to accept two schema patterns #9123
FTRACK USER ID MODULE: tweaking the createEidsArray() method to accept two schema patterns #9123
Conversation
modules/userId/eids.js
Outdated
}; | ||
const IDS = bidRequestUserId.ftrackId.hasOwnProperty('ext') ? bidRequestUserId.ftrackId.ext : bidRequestUserId.ftrackId; | ||
|
||
for (let id in IDS) { |
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.
To whom it may review.
- The original logic expected
bidRequestUserId.ftrackId
to be the following schema.
ftrackId: {
HHID: ['household_test_id'],
DeviceID: ['device_test_id'],
SingleDeviceID: ['single_device_test_id']
}
- But during testing, we actually were seeing:
ftrackId: {
uid: 'device_test_id',
ext: {
HHID: 'household_test_id',
DeviceID: 'device_test_id',
SingleDeviceID: 'single_device_test_id'
}
}
This code change allows both JSON schema's to work correctly, but its not clear to me if both are needed. Do we know why the schema's are different and/or if one is incorrect?
Thanks!
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 ftrack ID module is what wraps the result in ext
:
Prebid.js/modules/ftrackIdSystem.js
Lines 59 to 64 in d8f25b9
return { | |
ftrackId: { | |
uid: value.DeviceID && value.DeviceID[0], | |
ext | |
} | |
} |
#8678
To me that looks odd, maybe unintentional - it'd make more sense to return {ftrackId: {hhid, deviceId, singleDeviceId}}
; but in the end what matters is that the callers (including the eid conversion logic) know what to expect.
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.
Thanks, that is intentional. Thanks for connecting those dots for me.
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.
then I think you can simplify this to E_ID.ext = bidRequestUserId.ftrackId.ext
; I don't see a need to handle other cases.
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.
Thanks @dgirardi , I concur. Our decode method is already doing that work so its def redundant at this step. I'll make those changes and ping when they are up. Thanks
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.
@dgirardi @mmoschovas This PR has been updated. Thanks
…should be used to prepare the data
modules/ftrackIdSystem.js
Outdated
@@ -48,20 +48,36 @@ export const ftrackIdSubmodule = { | |||
* similar to the module name and ending in id or Id | |||
*/ | |||
decode (value, config) { | |||
if (!value) { return } | |||
const ext = {} | |||
value = 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.
I think it's better to keep the short circuit (if (!value) return
). From what I understand value
should always be present here, but if it isn't, it's not correct to default to {}
: it will cause this logic to return an incomplete object, which would then be translated to essentially gibberish in eids
.
If this returns undefined, the caller will filter it out from the userID map.
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.
Thanks @dgirardi reverted this code
@patmmccann @mmoschovas @dgirardi I forget, do I need to do something now or is the next step on your side? Thanks |
Thanks @patmmccann @mmoschovas @dgirardi |
…t two schema patterns (prebid#9123) * JDB-563: tweaking the createEidsArray() method to accept two schema patterns * JDB-563: further cleanup now that I understand the the decode method should be used to prepare the data * JDB-563: adding shortcircuit back into decode method Co-authored-by: Jason Lydon <jason.lydon@flashtalking.com>
…t two schema patterns (prebid#9123) * JDB-563: tweaking the createEidsArray() method to accept two schema patterns * JDB-563: further cleanup now that I understand the the decode method should be used to prepare the data * JDB-563: adding shortcircuit back into decode method Co-authored-by: Jason Lydon <jason.lydon@flashtalking.com>
Type of change
Description of change
A partner was attempting to use the FTRACK User ID module and they were getting errors in the
createEidsArray
method. The issue was the JSON schema being passed into the method did not match the schema we wrote the code for. The change this PR addresses is the JSON parsing logic was tweaked to accept both schema patterns while also checking value type (string|array) before pushing to theeids
array that is returned