-
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
PAIR: Support for Generic TechLab Version #12146 #12599
base: master
Are you sure you want to change the base?
PAIR: Support for Generic TechLab Version #12146 #12599
Conversation
return undefined; | ||
} | ||
|
||
return {'id': 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.
I think this requires some more logic to be converted into an EID - the userId module expects a string either from decode
, or from eids.openPairId.getValue(id)
eids.pairId.getValue(id)
(because that's what referenced in decode
- see other comment below)
From my testing (putting some random data in local storage), this does not generate any EID.
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.
Seems that other modules return a hash in this format, including the original pairId and a few other examples:
Prebid.js/modules/pairIdSystem.js
Line 101 in 2f713dd
return {'id': ids}; |
Prebid.js/modules/pubProvidedIdSystem.js
Line 56 in 2f713dd
return {id: res}; |
Prebid.js/modules/sharedIdSystem.js
Line 141 in 2f713dd
return {id: newId, callback: getIdCallback(newId, pixelUrl)}; |
Prebid.js/modules/publinkIdSystem.js
Line 155 in 2f713dd
return {id: localValue}; |
I think your test was failing because I was not properly exporting the correct module. Do you mind testing again?
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 also recommend merging or rebasing with master, there's been quite a few changes to userId since this PR's parent.
modules/openPairIdSystem.js
Outdated
* @returns {{pairId:string} | undefined } | ||
*/ | ||
decode(value) { | ||
return value && Array.isArray(value) ? {'pairId': value} : undefined |
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.
This should be openPairId
- if it's meant to be distinct from the other module - or if they're meant to be mutually exclusive (clashes are not a problem), the EID configuration in this should also use pairId
.
3ff3e51
to
73e6c68
Compare
@dgirardi I've gone ahead and rebased and addressed your comments. Please review again. |
@therevoltingx your unit tests are failing; this isnt yet ready for review |
@patmmccann my unit tests are passing on my end:
What is failing for you? |
|
If you click details next to the red x below, it has details on the failed tests |
Submodules.json |
@patmmccann ok i have rebased with latest master and also tests are passing now! |
ad7bd83
to
c5f58b9
Compare
c5f58b9
to
6a6f7c1
Compare
can you add changes to .submodules.json? |
import { storage, openPairIdSubmodule } from 'modules/openPairIdSystem.js'; | ||
import * as utils from 'src/utils.js'; | ||
|
||
describe('openPairId', function () { |
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.
There are currently no tests on the eid being formed
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.
please add coverage on the eid you expect getting formed
Documentation PR
prebid/prebid.github.io#5785
Clean Room Configuration
Bid Request