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

Adding a method pbjs.getUserIds to share userIds in Prebid to external codes #3889

Merged
merged 5 commits into from
Jul 11, 2019

Conversation

pm-harshad-mane
Copy link
Contributor

Type of change

  • Feature

Description of change

Adding a method pbjs.getUserIds to share userIds in Prebid to external codes.

  • Need an API to share the UserIds retrieved by Prebid with external codes
  • Use cases: UserIds can be passed to Wrappers like A9/EB
  • Added pbjs.getUserIds method
  • Moved the code that initializes all sub-modules and calls callbacks into a separate function
  • Moved code to combine submodule IDs in an object into a separate function

… codes

- Need an API to share the UserIds retrieved by Prebid with external codes
- Use casse: UserIds can be passed to Wrappers like A9/EB

- Added pbjs.getUserIds method
- Moved the cide that initalizes all sub-modules and calls passbacks into a separate function
- Moved code to combine submodule ids in an object into a separate function
@pm-harshad-mane
Copy link
Contributor Author

Hello @idettman ,
Could you please review the changes?

@pm-harshad-mane
Copy link
Contributor Author

Example:
pbjs.getUserIds();
// will return following object when tdid is available, object will contain more ids as per availability
{tdid: "cdbe1fea-44a9-4f93-b6a7-11a64eb9f8dc"}

@bretg
Copy link
Collaborator

bretg commented Jun 10, 2019

@pm-harshad-mane - we're going to need a docs update for new external API call. Care to take a cut at a PR for http://prebid.org/dev-docs/publisher-api-reference.html ?

@pm-harshad-mane
Copy link
Contributor Author

Sure @bretg , I will add the documentation PR as well.
I might some help from you, will ping you on Slack.

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.

Waiting for @jaiminpanchal27 to take a look before I approve

@@ -414,6 +443,10 @@ export function init(config) {
updateSubmodules();
}
})

// exposing getUserIds function in global-name-space so that userIds stored in Prebid can be used by external codes.
let theGlobal = getGlobal();
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaiminpanchal27 Can you review this use of getGlobal?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@idettman this looks good. Use case here is valid and user id being a core module we can allow usage of global.
Another approach to not use global here was to import getUserIds function in prebid.js and use their but then it will always import user id module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @jaiminpanchal27 👍

@pm-harshad-mane
Copy link
Contributor Author

Hello @jaiminpanchal27 ,
Can you please review prebid/prebid.github.io#1361 ?

@pm-harshad-mane
Copy link
Contributor Author

Hello @jaiminpanchal27 and @idettman ,
Documentation review is done by @bretg
Can you please review the code?

Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

@pm-harshad-mane Left one comment

* This function will be exposed in global-name-space so that userIds stored by Prebid UserId module can be used by external codes as well.
* Simple use case will be passing these UserIds to A9 wrapper solution
*/
function getUserIds() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pm-harshad-mane Should this function not have a callback function as argument which will be called when user id's are ready. Id systems make async calls to get user ids. This function may return empty object or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @jaiminpanchal27 ,
my expectation from the API is to return whatever data is available at the moment.
It will be a good feature to have a call-back on getting all ids, but for now, it is not a requirement. Also, as userId module is caching the data, maybe on the second iteration of execution the new API will return all ids so it is sufficient for now.

@jaiminpanchal27 jaiminpanchal27 added LGTM needs 2nd review Core module updates require two approvals from the core team labels Jul 2, 2019
@pm-harshad-mane

This comment has been minimized.

@pm-harshad-mane

This comment has been minimized.

@idettman idettman self-requested a review July 9, 2019 17:58
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.

Small changes to line 204 and line 449, the rest looks good

modules/userId/index.js Outdated Show resolved Hide resolved
@idettman idettman added needs update and removed LGTM labels Jul 9, 2019
modules/userId/index.js Outdated Show resolved Hide resolved
@idettman idettman removed the needs 2nd review Core module updates require two approvals from the core team label Jul 9, 2019
@pm-harshad-mane

This comment has been minimized.

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.

LGTM

@idettman idettman merged commit 2be5cc7 into prebid:master Jul 11, 2019
@pm-harshad-mane
Copy link
Contributor Author

Thank you @idettman and @jaiminpanchal27 !!

SublimeLeo pushed a commit to SublimeSkinz/Prebid.js that referenced this pull request Jul 16, 2019
…l codes (prebid#3889)

* Adding a mthod pbjs.getUserIds to share userIds in Prebid to external codes

- Need an API to share the UserIds retrieved by Prebid with external codes
- Use casse: UserIds can be passed to Wrappers like A9/EB

- Added pbjs.getUserIds method
- Moved the cide that initalizes all sub-modules and calls passbacks into a separate function
- Moved code to combine submodule ids in an object into a separate function

* implemented code suggestion chnages

* fixed eslint issue
leonardlabat pushed a commit to criteo-forks/Prebid.js that referenced this pull request Jul 30, 2019
…l codes (prebid#3889)

* Adding a mthod pbjs.getUserIds to share userIds in Prebid to external codes

- Need an API to share the UserIds retrieved by Prebid with external codes
- Use casse: UserIds can be passed to Wrappers like A9/EB

- Added pbjs.getUserIds method
- Moved the cide that initalizes all sub-modules and calls passbacks into a separate function
- Moved code to combine submodule ids in an object into a separate function

* implemented code suggestion chnages

* fixed eslint issue
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
…l codes (prebid#3889)

* Adding a mthod pbjs.getUserIds to share userIds in Prebid to external codes

- Need an API to share the UserIds retrieved by Prebid with external codes
- Use casse: UserIds can be passed to Wrappers like A9/EB

- Added pbjs.getUserIds method
- Moved the cide that initalizes all sub-modules and calls passbacks into a separate function
- Moved code to combine submodule ids in an object into a separate function

* implemented code suggestion chnages

* fixed eslint issue
sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
…l codes (prebid#3889)

* Adding a mthod pbjs.getUserIds to share userIds in Prebid to external codes

- Need an API to share the UserIds retrieved by Prebid with external codes
- Use casse: UserIds can be passed to Wrappers like A9/EB

- Added pbjs.getUserIds method
- Moved the cide that initalizes all sub-modules and calls passbacks into a separate function
- Moved code to combine submodule ids in an object into a separate function

* implemented code suggestion chnages

* fixed eslint issue
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.

4 participants