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.

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

sharedIdSystem: return pubcid instead of sharedId #7099

Merged
merged 2 commits into from Jul 13, 2021

Conversation

philipwatson
Copy link
Contributor

Type of change

  • Bugfix

Description of change

I had a look at sharedIdSystem.js after the changes made in #6808 for issue #6640

I'm confused.

It doesn't make sense to me that the sharedId is passed into the callback. Should it be pubcid instead?

It could do with more tests. I didn't invest much time there because my understanding could be wrong.

From unit testing, I noticed storage.expires in the storeData function does not exist (or does it?). So I changed to config.storage.expires. Not sure if this is correct.

@ChrisHuie ChrisHuie changed the title Make sharedIdSystem return pubcid instead of sharedId sharedIdSystem: return pubcid instead of sharedId Jun 25, 2021
@jdwieland8282
Copy link
Member

@NothingIsMatter Mind taking a look here?

@jdwieland8282
Copy link
Member

jdwieland8282 commented Jun 29, 2021

@ChrisHuie re: "return pubcid instead of sharedId 4 days ago" can you add more here? The user id generated by the modules formally known as pubcommon and sharedid, were just random string generators. The user id output of either shouldn't matter.

The intention was to have a guid set in a 1pc, then added to the eids array using pubcid.org as the source.

@philipwatson
Copy link
Contributor Author

Let me give a scenario to help raise the alarm a little ;-)

Given default auctionDelay of 0. A new user. On page load:

Bidders receive bid.userId as:

{
   pubcid: "0ea08523-0ceb-4ae6-bde3-84b34c2eb893"
}

The value is a UUID created on the page (by sharedIdSystem's getId). All good here.

Then I look at the cookies in developer tools. I see the cookies created via sharedIdSystem:
_pubcid = 01EWC4FHX0QQEV1HGR16HAQGPM
_pubcid_sharedid = 01EWC4FHX0QQEV1HGR16HAQGPM

They are set to the same ULID value from sharedid server. The callback, which is run after the auction, replaces the established UUID with the ULID. Note: this will also happen to existing users (not just new users) when getId is called for a refresh.

So on the next page load, bidders receive bid.userId as:

{
   sharedid: {id: "01EWC4FHX0QQEV1HGR16HAQGPM"},
   pubcid: "01EWC4FHX0QQEV1HGR16HAQGPM"
}

For completeness, bid.userIdAsEids has:

[{
  source: "pubcid.org",
  uids: [{
    id: "01EWC4FHX0QQEV1HGR16HAQGPM",
    atype: 1
  }]
}]

Only pubcid.org here which is expected after reading the code - sharedId is filtered out (or do we want to see two items in the uids array here?)

sharedIdSystem manages the storage of _pubcid_sharedid.. but also gets the user id module to manage the storage of _pubcid with the same value. Obviously there is a problem here right?

@jdwieland8282
Copy link
Member

jdwieland8282 commented Jul 2, 2021

Hey @philipwatson thanks for the example, we are taking a look, it does indeed look like a problem. What we want in the eids array according to you example is indeed.

[{
  source: "pubcid.org",
  uids: [{
    id: "0ea08523-0ceb-4ae6-bde3-84b34c2eb893",
    atype: 1
  }]
}]

@ChrisHuie
Copy link
Collaborator

ChrisHuie commented Jul 7, 2021

@philipwatson Are there more changes coming to this pr? Sounded like possible revisions from the comments. If not though if you could pull in the latest commits from master so tests are passing for this pr 🙏

@ChrisHuie ChrisHuie requested review from ChrisHuie and removed request for osazos July 7, 2021 09:02
@ChrisHuie ChrisHuie assigned ChrisHuie and unassigned osazos Jul 7, 2021
@ChrisHuie ChrisHuie removed the minor label Jul 7, 2021
@philipwatson
Copy link
Contributor Author

No more changes coming in this PR.
I've merged master. Can anyone rerun the build? An unrelated test is suspiciously timing out even though there is not much in it. Same test is failing on another PR.

@SKOCHERI
Copy link
Contributor

SKOCHERI commented Jul 7, 2021

@philipwatson The SharedIdSystem need not call the Sharedid endpoint. The same has been updated in #7149 and the SharedIdSystem module is cleaned up to removed read/write cookie. Please take a look at this PR.

@ChrisHuie ChrisHuie merged commit 7e9e832 into prebid:master Jul 13, 2021
prebidtappx pushed a commit to prebidtappx/Prebid.js that referenced this pull request Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants