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

Audigent HaloID User Id System Module #5524

Merged
merged 33 commits into from
Sep 15, 2020
Merged

Audigent HaloID User Id System Module #5524

merged 33 commits into from
Sep 15, 2020

Conversation

antlauzon
Copy link
Contributor

@antlauzon antlauzon commented Jul 21, 2020

Type of change

  • Feature

Description of change

This changeset adds support for the Audigent HaloID.

Other information

Audigent Segmentation Real-time Data Provider #4834
#4834

@pm-harshad-mane
Copy link
Contributor

Hello @anthonylauzon ,
Could you please add entries for HaloID in https://github.com/prebid/Prebid.js/blob/master/modules/userId/eids.md and https://github.com/prebid/Prebid.js/blob/master/modules/userId/eids.js This will help bidders like PrebidServerBid Adapter and PubMatic bid adapter to support HaloID without making any change in their code.

Also we will need to add entry in https://github.com/prebid/Prebid.js/blob/master/modules/.submodules.json to auto-select the parent module at build time.

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

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

Please use the storageManager.js functions for any local storage or cookie actions

modules/haloIdSystem.js Outdated Show resolved Hide resolved
@patmmccann
Copy link
Collaborator

there are no unit tests added to the user id module unit tests as all other prs have done

modules/haloIdSystem.js Outdated Show resolved Hide resolved
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.

A few comments about how you're retrieving and storing your ID from cache:

Your code is manually retrieving your ID only from localStorage and only from one of two names: haloId or auHaloId. However, are using the userId module to handle writing the ID to cache, which relies on the storage configuration settings, which you allow the publisher to configure (both the storage type and name). It's entirely possible that neither of the locations you hardcoded in your IdSystem contains the ID that was stored by the userId module.

I would suggest you either read the parameters from the publisher configuration to read the ID (or better yet, just add a third parameter to your getId() method which contains the cached ID, if any; or you need to write the ID to cache yourself so you know where it is located, and if you do, you should drop any storage config from the docs so publishers aren't confused.

modules/.submodules.json Outdated Show resolved Hide resolved
modules/haloIdSystem.js Outdated Show resolved Hide resolved
modules/haloIdSystem.js Outdated Show resolved Hide resolved
modules/haloIdSystem.js Outdated Show resolved Hide resolved
@patmmccann
Copy link
Collaborator

Appears to be missing changes to eids spec

@smenzer
Copy link
Collaborator

smenzer commented Aug 27, 2020

i don't see any docs PRs for this, which are required before merging

@patmmccann
Copy link
Collaborator

@anthonylauzon Looks like another merge created conflicts, fyi

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

modules/haloIdSystem.js Outdated Show resolved Hide resolved
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

@pm-harshad-mane
Copy link
Contributor

@anthonylauzon have you also raised PR for the related documentation?
Refer https://github.com/prebid/prebid.github.io/blob/master/dev-docs/modules/userId.md

@antlauzon
Copy link
Contributor Author

@anthonylauzon have you also raised PR for the related documentation?
Refer https://github.com/prebid/prebid.github.io/blob/master/dev-docs/modules/userId.md
The documentation PR is located here:

prebid/prebid.github.io#2320

@antlauzon
Copy link
Contributor Author

@smenzer @pm-harshad-mane this is ready to go. can we please merge this so that we can begin integration with our RTD module?

@pm-harshad-mane
Copy link
Contributor

pm-harshad-mane commented Sep 15, 2020

Hello @jsnellbaker , @bretg , @idettman
Should we merge this PR?
Does everything looks OK to you guys?

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

@smenzer smenzer dismissed idettman’s stale review September 15, 2020 07:23

requested changes have been made

@smenzer smenzer merged commit 30a069c into prebid:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants