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

Add lotame id system #5388

Merged
merged 18 commits into from
Jul 9, 2020
Merged

Add lotame id system #5388

merged 18 commits into from
Jul 9, 2020

Conversation

markaconrad
Copy link
Contributor

@markaconrad markaconrad commented Jun 17, 2020 •
edited
Loading

Type of change

  • New user id module
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

Description of change

Adds a new user id module named lotamePanoramaId

  • test parameters for validating bids
{
        name: 'lotamePanoramaId'
}

Tested with
(/integrationExamples/gpt/userId_example.html) sample page.

Maintainer Email Address

@markaconrad
Add a LotameIdSystem as a new User ID module
08ce0ee
@markaconrad
Now using our own caching system
4ee8ccd
@markaconrad
Add more test cases; Switch names to be constants
c27c19a
@markaconrad
Add test cases
9d41c90
@markaconrad
Handle "expiring" local storage like the built in code does
ec826ec
@markaconrad
Switch to using the expiry_ms from the endpoint as the cookie expiration
fc32767
@markaconrad
Update to the official naming; Remove the lastUpdate storage as we do…
864339e 
…n't use it and depend on what comes from the endpoint; Update to use the dev bcp endpoint for now
@markaconrad
Fix tests, numbers vs strings....; Add gdpr_applies query param
f5532ba
@markaconrad
Fix the timestamp becoming an invalid date
8231cc6
@markaconrad
Clear out the panorama id when on optout
627633b
@markaconrad
Add eid support
beba511
@markaconrad
Switch to the prod version of the url
be46374
@markaconrad
Merge branch 'master' into add_lotameIdSystem
b994d26
@markaconrad
Update test wording
1c5e954
smenzer
smenzer reviewed Jun 18, 2020
View reviewed changes
Copy link
Collaborator

@smenzer smenzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple questions but overall lgtm

modules/lotamePanoramaIdSystem.js Show resolved Hide resolved
modules/lotamePanoramaIdSystem.js Outdated
utils.logError(error);
}
}
callback(responseObj);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, i may be missing something, but here you are storing to the prebid cache object the entire responseObj, which seems to contain several keys, including the core_id and profile_id. however, in your decode function on line 157, you are checking if the value is a string, otherwise you return undefined. my understanding is that the value you receive in decode() will be the same thing you pass in the callback() here...which means decode would always return undefined. again, i'm sure i'm missing something here since you're doing some of your own cache management, but wanted to flag this as it doesn't seem correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this part as it was tricky to trace what decode will always get as there are a couple of different paths

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed up a fix, thanks for catching this!

@markaconrad
From PR feedback
74bf928 
Only care about the core_id if a profile_id is also returned so it won't look like we can store a core_id and then promptly delete it
Return just the core_id from getId()
@markaconrad markaconrad mentioned this pull request Jun 18, 2020
Merged
@markaconrad
Merge pull request #2 from Lotame/prebid_Selected_fixes
628cb21 
Only care about the core_id if a profile_id is also returned so it won't look like we can store a core_id and then promptly delete it
Return just the core_id from getId()
smenzer
smenzer reviewed Jun 19, 2020
View reviewed changes
Copy link
Collaborator

@smenzer smenzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. we should still have one of our primary reviewers go through it, though.

modules/lotamePanoramaIdSystem.js Show resolved Hide resolved
@pm-harshad-mane
Copy link
Contributor

Hello @markaconrad
Can you please add a Unit test case in https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/eids_spec.js for Lotame ID?

@markaconrad
Copy link
Contributor Author

@msm0504 Just checking in to see if you need anything else from me before this is good to go. Thanks!

@msm0504
Copy link
Contributor

msm0504 commented Jul 8, 2020

@markaconrad I don't think I need anything right now. Sorry, I haven't had a chance to review this yet. I'll review this week.

@msm0504 msm0504 added the LGTM label Jul 9, 2020
@msm0504 msm0504 merged commit 8f17bb5 into prebid:master Jul 9, 2020
sbrosinski added a commit to smaato/Prebid.js that referenced this pull request Jul 10, 2020
@sbrosinski
Merge branch 'master' of github.com:prebid/Prebid.js into smaato-adapter
4c2509e 
* 'master' of github.com:prebid/Prebid.js: (39 commits)
  Increment pre version
  Prebid 3.25.0 release
  Add lotame id system (prebid#5388)
  Change endpoint (prebid#5459)
  ozone 2.4.0 adapter updates (prebid#5421)
  Update AppNexus usersync script (prebid#5473)
  Eplanning fix: decode parameters (prebid#5448)
  Teads adapter : Support page referrer and network bandwidth (prebid#5430)
  Merge Valueimpression Bid Adapter to Quantumdex Bid Adapter (prebid#5405)
  Criteo - partially restore adapter before PR prebid#4518 following performance issues (prebid#5376)
  onetagBidAdapter: outstream support (prebid#5435)
  Vidazoo Adapter: Feature/bidder-version (prebid#5384)
  adform and adformOpenRTB bid adapters: Added support for userId modules (prebid#5425)
  proxistore bid adapter: delay request to server by 5 min if there were no bids (prebid#5379)
  Vidazoo Adapter: Feature/subdomain (prebid#5446)
  Inskin Bid adapter small changes (prebid#5373)
  Revert "add AMX adapter (prebid#5383)" (prebid#5455)
  ATS-change logError to logInfo type (prebid#5443)
  ATS-identityLinkId - add additional info logging events (prebid#5442)
  Update padsquad for meta.advertiserDomains (prebid#5439)
  ...
sbrosinski added a commit to smaato/Prebid.js that referenced this pull request Jul 10, 2020
@sbrosinski
Merge branch 'smaato-adapter' into smaato-adapter-video
a3a2fbc 
* smaato-adapter: (40 commits)
  Smaato: Fix test data
  Increment pre version
  Prebid 3.25.0 release
  Add lotame id system (prebid#5388)
  Change endpoint (prebid#5459)
  ozone 2.4.0 adapter updates (prebid#5421)
  Update AppNexus usersync script (prebid#5473)
  Eplanning fix: decode parameters (prebid#5448)
  Teads adapter : Support page referrer and network bandwidth (prebid#5430)
  Merge Valueimpression Bid Adapter to Quantumdex Bid Adapter (prebid#5405)
  Criteo - partially restore adapter before PR prebid#4518 following performance issues (prebid#5376)
  onetagBidAdapter: outstream support (prebid#5435)
  Vidazoo Adapter: Feature/bidder-version (prebid#5384)
  adform and adformOpenRTB bid adapters: Added support for userId modules (prebid#5425)
  proxistore bid adapter: delay request to server by 5 min if there were no bids (prebid#5379)
  Vidazoo Adapter: Feature/subdomain (prebid#5446)
  Inskin Bid adapter small changes (prebid#5373)
  Revert "add AMX adapter (prebid#5383)" (prebid#5455)
  ATS-change logError to logInfo type (prebid#5443)
  ATS-identityLinkId - add additional info logging events (prebid#5442)
  ...
@markaconrad markaconrad deleted the add_lotameIdSystem branch January 4, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smenzer smenzer smenzer left review comments

@msm0504 msm0504 msm0504 approved these changes

Assignees

@msm0504 msm0504

Labels
LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@markaconrad @pm-harshad-mane @msm0504 @smenzer