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

Tnc id module: initial release #8435

Merged
merged 31 commits into from
Jun 18, 2022
Merged

Tnc id module: initial release #8435

merged 31 commits into from
Jun 18, 2022

Conversation

annavane
Copy link
Contributor

Type of change

  • [x ] Feature
  • [ x] Does this change affect user-facing APIs or examples documented on http://prebid.org?

Description of change

New UserId Module: TNCIDIdSystem

Maintainer contact: [email protected]

@patmmccann patmmccann changed the title Tncid Tnc id module: initial release May 20, 2022
modules/tncIdSystem.md Outdated Show resolved Hide resolved
modules/tncIdSystem.js Outdated Show resolved Hide resolved
source: 'thenewco.it',
atype: 3
},

Copy link
Collaborator

Choose a reason for hiding this comment

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

you appear to be missing changes to eids.md, user id spec and eids spec

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this was resolved without changes

-  modules/tncIdSystem.md: spacing fix
-  added eids.md, user id spec and eids spec
@annavane annavane requested a review from smenzer May 31, 2022 08:34
@annavane
Copy link
Contributor Author

annavane commented Jun 6, 2022

@smenzer @patmmccann Can you please re-review this PR? We would like to know if any other changes are necessary.

@annavane annavane requested a review from patmmccann June 8, 2022 07:12
@annavane
Copy link
Contributor Author

@smenzer @patmmccann Can you please let us know if code is good to go now?

@micheletnc
Copy link
Contributor

Hi all,

sorry for the pressure, is there any update about this release? We have a big client who's waiting for proceeding with media activities with deep intergration with our TNCID.

Thanks for understanding!

@patmmccann
Copy link
Collaborator

Appears to be missing changes to eids spec and userid spec files


const loadRemoteScript = (providerId) => {
return new Promise((resolve) => {
loadExternalScript('https://js.tncid.app/remote.js?ns=' + FALLBACK_TNC_INSTANCE + '&providerId=' + providerId, MODULE_NAME, resolve);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for converting to load external script, please also do either of the below:

  1. open source and lock this script to a version
  2. change its location to a configuration parameter with no default ( but feel free to mention the usual location in an integration example or documentation )

Thanks!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Thank you for your feedback. We followed your second suggestion and now the script URL is an input parameter configurable by module user.

const loadRemoteScript = () => {
return new Promise((resolve) => {
loadExternalScript(url, MODULE_NAME, resolve);
})
}

We also added missing tests in eids_spec and userid_spec files and update documentation.

Thnak you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you!

@annavane
Copy link
Contributor Author

Hi @smenzer and @jlukas79, do you have any additional requests or comments?

Thank you!

@annavane annavane requested a review from patmmccann June 17, 2022 11:10
@patmmccann
Copy link
Collaborator

@jlukas79 could you take another look? Ty!

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

@patmmccann patmmccann merged commit 4d0b063 into prebid:master Jun 18, 2022
@annavane annavane deleted the TNCID branch June 20, 2022 07:29
renebaudisch pushed a commit to renebaudisch/Prebid.js that referenced this pull request Jun 28, 2022
* - TNCID UserID Module

* revert

* - TNCID UserID Module

* - TNCID UserID Module

* Converted loadTNCScript to Promise

* Added CacheID

* - TNCID UserID module

* - Documentation updated

* - File naming fix

* - type fix

* -no change

* - space removed

* new line removed

* - lint fix

* - eids fix

* - .submodules.json: module name moved in alphabetical order
-  modules/tncIdSystem.md: spacing fix
-  added eids.md, user id spec and eids spec

* - tnc script loaded by loadExternalScript util function

* -resolved lint errors

* - module name refactoring

* - input parameter added to disable load of fallback script

* - TNC fallback script is now input param

* - Added missing tests

Co-authored-by: AzureAD\AnnaVane <[email protected]>
Co-authored-by: micheletnc <[email protected]>
bwhisp pushed a commit to bwhisp/Prebid.js that referenced this pull request Jul 13, 2022
* - TNCID UserID Module

* revert

* - TNCID UserID Module

* - TNCID UserID Module

* Converted loadTNCScript to Promise

* Added CacheID

* - TNCID UserID module

* - Documentation updated

* - File naming fix

* - type fix

* -no change

* - space removed

* new line removed

* - lint fix

* - eids fix

* - .submodules.json: module name moved in alphabetical order
-  modules/tncIdSystem.md: spacing fix
-  added eids.md, user id spec and eids spec

* - tnc script loaded by loadExternalScript util function

* -resolved lint errors

* - module name refactoring

* - input parameter added to disable load of fallback script

* - TNC fallback script is now input param

* - Added missing tests

Co-authored-by: AzureAD\AnnaVane <[email protected]>
Co-authored-by: micheletnc <[email protected]>
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.

5 participants