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

CM-1260 Implement LiveIntent Prebid User Id Module Based On The Hub #42

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

peixunzhang
Copy link

@peixunzhang peixunzhang commented Aug 2, 2024

CM-1260
Implement LiveIntent Prebid User Id Module Based On The Hub

Type of change

  • Feature

Description of change

Other information

Related PR: LiveIntent/prebid.github.io#25

Sorry, something went wrong.

@peixunzhang peixunzhang marked this pull request as ready for review August 9, 2024 09:15
@3link 3link requested review from a team and 3link August 9, 2024 09:32
Copy link

@mschuwalow mschuwalow left a comment

Choose a reason for hiding this comment

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

Looks good from my side, would only recommend splitting out the shared code.

not sure about the naming of the files in /libraries, but also no good ideas.
@3link do you have a better suggestion?

libraries/liveIntentIdSystem/liveIntentId3System.js Outdated Show resolved Hide resolved
libraries/liveIntentIdSystem/liveIntentId3System.js Outdated Show resolved Hide resolved
libraries/liveIntentIdSystem/liveIntentId3System.js Outdated Show resolved Hide resolved
libraries/liveIntentIdSystem/liveIntentId3System.js Outdated Show resolved Hide resolved
@@ -1,387 +1,14 @@
/**

Choose a reason for hiding this comment

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

hmm, it's a bit unfortunate that it doesn't show the diff between this one and /libraries/liveIntentIdSystem/liveIntentIdSystem.js (treating it as a move), as that makes this a bit more difficult to review. No suggestions how to fix this though.

@3link
Copy link

3link commented Aug 13, 2024

Looks good from my side, would only recommend splitting out the shared code.

not sure about the naming of the files in /libraries, but also no good ideas. @3link do you have a better suggestion?

Hmm. maybe we can call the new module liveIntentIdHubSystem.js?

@peixunzhang peixunzhang requested a review from 3link August 14, 2024 09:39
@wi101
Copy link

wi101 commented Aug 15, 2024

will we get rid of the live-connect dependency in the package.js or is it still needed?
I saw it is still needed

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

const PUBLISHER_ID = '89899';
const defaultConfigParams = { params: {publisherId: PUBLISHER_ID, fireEventDelay: 1} };

describe('LiveIntentIdHub', function() {
Copy link

Choose a reason for hiding this comment

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

I did not find a test that checks for example if appId and distributorId are provided, only one of them will be used. Can we add it if it is really missing?

Copy link
Author

Choose a reason for hiding this comment

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

Added here

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Viktor Dreiling <34981284+3link@users.noreply.github.com>
Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

@@ -0,0 +1,201 @@
import {UID1_EIDS} from '../uid1Eids/uid1Eids.js';

Choose a reason for hiding this comment

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

I think we can drop the liveIntentId prefix from all files in libraries. They are in a folder called liveintentId anyway. So I would do:

  • externalIdSystem.js
  • idSystem.js
  • shared.js

Copy link
Author

Choose a reason for hiding this comment

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

Okie

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentIdSystem/liveIntentIdSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 8 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/externalIdSystem.js (+1 error)
  • libraries/liveIntentId/idSystem.js (+3 errors)
  • modules/liveIntentIdSystem.js (+4 errors)

Copy link

Tread carefully! This PR adds 6 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/externalIdSystem.js (+1 error)
  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+4 errors)

Copy link

Tread carefully! This PR adds 5 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+4 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Copy link

Tread carefully! This PR adds 4 linter errors (possibly disabled through directives):

  • libraries/liveIntentId/idSystem.js (+1 error)
  • modules/liveIntentIdSystem.js (+3 errors)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants