ID Library #5863

SKOCHERI · 2020-10-16T07:22:06Z

Type of change

Bugfix
Feature
New bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other

Description of change

test parameters for validating bids

{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

contact email of the adapter’s maintainer eng-dmp@magnite.com
official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

A link to a PR on the docs repo at https://github.com/prebid/prebid.github.io/

Other information

lgtm-com · 2020-10-16T07:31:40Z

This pull request introduces 2 alerts when merging 7440f9e into 4662f06 - view on LGTM.com

new alerts:

2 for Use of returnless function

lgtm-com · 2020-10-16T17:21:59Z

This pull request introduces 2 alerts when merging 021cd69 into 926ccc6 - view on LGTM.com

new alerts:

2 for Use of returnless function

lgtm-com · 2020-10-16T18:36:42Z

This pull request introduces 2 alerts when merging a44f22a into 926ccc6 - view on LGTM.com

new alerts:

2 for Use of returnless function

lgtm-com · 2020-10-16T18:59:49Z

This pull request introduces 2 alerts when merging 9a20eaf into 926ccc6 - view on LGTM.com

new alerts:

2 for Use of returnless function

lgtm-com · 2020-10-16T19:31:28Z

This pull request introduces 2 alerts when merging 0aec7ea into 926ccc6 - view on LGTM.com

new alerts:

2 for Use of returnless function

jdwieland8282 · 2020-10-16T22:41:04Z

docs pr: prebid/prebid.github.io#2429

smenzer · 2020-10-20T15:03:38Z

@SKOCHERI or @jdwieland8282 can you provide a bit of context as to what this is for, please?

jdwieland8282 · 2020-10-20T15:22:27Z

@smenzer context is in the docs pr, but specifically to answer your question:

"The ID Library module gathers and generates a map of identities present on the page. The primary usecase for this adapter is for Publishers who have included multiple UserId subadapters in their prebid.js implementation, and want to store the resulting user ids server side for modeling or graphing purposes. "

smenzer · 2020-10-20T15:40:32Z

@jdwieland8282 ah thanks...I opened the docs PR looking for some context but didn't actually go read the .md file 🤦

from the docs and code, it looks like this is all tied to a hashed email, is that correct? what if there isn't an email available, can the publisher provide their own UID or something?

jdwieland8282 · 2020-10-20T15:51:58Z

its all tied to a persistent identifier, hashed email is one option but not the only option. If the id library module doesn't find a persistent id nothing happens. The publisher can declare an element id for where they store the users userName for example on the page:

ex. "2. a publisher configured element, for example when configuration target='username', the value is extracted from an element having id 'username'"

robertrmartinez

Still need to do some more review but had a few concerns / comments

robertrmartinez · 2020-10-27T22:23:56Z

modules/idLibrary.js

+    logInfo('Email is found, body observer disconnected');
+  }
+
+  const body = document.body.innerHTML;


We are scanning the entire page with regex for an email?

Can you explain the use case here? This doesn't seem very prebid-y.

Unless the publisher has configured the element from which the email needs to be fetched, we will have to scan the entire page to get email.

why can't the publisher just pass the hashed email directly to the library when they initiate it? i agree that scanning an entire page is not ideal

I recommend we make this scanning of entire document a configurable option via the setConfig call to this module.

We will leave it defaulting to true but I think it is in our best interest to document the behavior of the module thoroughly, and make sure the publisher has the control necessary.

@jdwieland8282 Can we make sure the associated docs PR contains the steps outlined and the fallbacks?

Something that outlines the flow:

How the target is found:

Uses publisher defined target element

Searches for HTML input (text/email) element

Searches entire document for email using regex.

And outline that the latter (3) can be configured off by something like:

pbjs.setConfig({ idLibrary: { fallbackSearch: false // defaults to true (I am bad at naming vars) } });

@SKOCHERI

@robertrmartinez the existing Docs PR describes the following:

A persistent identifier can be extracted in the following ways: 1. From a generic `<div>` element 2. From a publisher configured HTML element id 3. From an `<input>` element of type text/email

Are you suggesting your 1-3 above replace the existing 1-3?

Well just something that outlines exactly the flow of the module.

Could be another flow chart or something.

robertrmartinez · 2020-10-27T22:25:53Z

modules/idLibrary.js

+  const body = document.body.innerHTML;
+
+  if (hasEmail(body)) {
+    email = getEmail(body);


I do not think it is a good idea to call hasEmail which calls getEmail and does a full regex lookup on the innerHtml of the whole page, then call getEmail again. Seems very inefficient.

If you are gonna run getEmail anyways, we should just check the response instead of wrapping it in hasEmail

This has been fixed

jdwieland8282 · 2020-10-29T19:24:20Z

adding some context behind the motivation for creating this module

Our point of view with regard to identity is that a 1st party relationship between publishers and viewers of their content exists. As such, we believe that publishers have a right to anchor all user ids generated by the Prebid userid module, to a persistent (hashed) identifier such as an email address. We believe that it is in the publisher, and by extension the open web’s interest, to create a deterministic id graph composed of the linkage between persistent ids and 1st party ids generated by the user id sub-adapters. As browsers limit our ability to generate cross domain identifiers, we believe a publisher controlled id graph is the right response. We (Magnite) may decide to help publishers build this graph in the future if asked.

lgtm-com · 2020-11-02T17:32:22Z

This pull request introduces 2 alerts when merging 4a6c09c into ae1b4d7 - view on LGTM.com

new alerts:

2 for Use of returnless function

robertrmartinez

There's some changes needed.

robertrmartinez · 2020-11-02T18:39:21Z

modules/idLibrary.js

+function debounce(func, wait, immediate) {
+  let timeout;
+
+  return function (...args) {


So debounce is just returning a function to be called later?

However, anywhere debounce is called, the return is not used.

So this is effectively not doing anything.

I think the real fix here is to add the method bindings as requested for the other methods (first param anytime debounce is called).

And to make this function a regular function that just executes, not one which returns a function.

Also, did you mean to use the keyword arguments instead of args ?

@robertrmartinez The debounce issue is fixed and full body scan config is provided.

robertrmartinez · 2020-11-02T18:44:39Z

modules/idLibrary.js

+function handleTargetElement(conf) {
+  const targetObserver = new MutationObserver(function (mutations, observer) {
+    logInfo('target observer called');
+    debounce(targetAction(conf, mutations, observer), conf.debounce, false);


@SKOCHERI

What is the intent here? Please elaborate on the expectation of things to happen when this line is hit.

debounce takes in a func param that looks like the intent is for it to be called later.

But you are executing the targetAction here with those params, which returns undefined. So really func is always undefined when debounce is run.

Is the intent to not call targetAction here but just pass its reference to debounce?

If so, recommend you use bind.

debounce(targetAction.bind(null, conf, mutations, observer), conf.debounce, false);

The debounce issue is fixed

robertrmartinez · 2020-11-02T18:45:12Z

modules/idLibrary.js

+  addInputElementsElementListner(conf);
+  const bodyObserver = new MutationObserver(function (mutations, observer) {
+    logInfo('Body observer called');
+    debounce(bodyAction(conf, mutations, observer), conf.debounce, false);


Same as debounce change needed here.

smenzer · 2020-11-03T07:51:52Z

@jdwieland8282 is there a use case where the publisher doesn't want this library to scan the page for a hashed email, but has their own identifier already (maybe in a 1P cookie, session, etc) but they still want to collect all of the userId module IDs and post them to themselves? in other words...is it possible to turn off the first 4 steps in "Application Flow" (https://github.com/prebid/prebid.github.io/pull/2429/files) and just capture and post the user IDs someplace?

jdwieland8282 · 2020-11-03T17:00:22Z

Hi @smenzer , Not exactly, the intention here is to make the output deterministic in nature, and not require a bunch of probabilistic ML. If a user wanted to do what you suggested, they could deploy the pubprovided userid module, but they would be limited to page loads where a persistent value was observed.

robertrmartinez

Several more questions / comments

robertrmartinez · 2020-11-03T18:14:48Z

modules/idLibrary.js

+    logError('The required url is not configured');
+    return;
+  }
+  if (!config.debounce) {


is debounce === 0 not a valid use case?

Maybe this should be:

if (typeof config.debounce !== 'number')

robertrmartinez · 2020-11-03T18:15:56Z

modules/idLibrary.js

+    config.debounce = CONF_DEFAULT_OBSERVER_DEBOUNCE_MS;
+    logInfo('Set default observer debounce to ' + CONF_DEFAULT_OBSERVER_DEBOUNCE_MS);
+  }
+  if (config.fullscan == undefined) {


Would reccomend just checking if it is type of boolean here to make sure it is always one.

if (typeof config.fullscan !== 'boolean')

Also when possible, it is best to use strict ===.

Sometimes you want to catch null and undefined so == will do that,

but we know we want it to be a number for sure, so I think it is best to explicitly check if it is a number.

Same with fullscan (boolean).

robertrmartinez · 2020-11-03T18:19:58Z

modules/idLibrary.js

+function postData() {
+  (getGlobal()).refreshUserIds();
+  const userIds = (getGlobal()).getUserIds();
+  if (!userIds || userIds.length === 0) {


Think it may be better to use

!Array.isArray(userIds) instead of !userIds

If for some reason userIds comes back with a prop that does not have .length this will throw an error.

userIds is an object, so updated the check to Object.keys(userIds).length === 0

robertrmartinez · 2020-11-03T18:23:47Z

modules/idLibrary.js

+
+function syncCallback() {
+  return {
+    success: function (responseBody) {


not using responseBody, can remove I guess.

robertrmartinez · 2020-11-03T18:29:24Z

modules/idLibrary.js

+}
+
+function postData() {
+  (getGlobal()).refreshUserIds();


Why are we refreshing userIds every single call?

Not sure that was the intent when pbjs.refreshUserIds was implemented.

@jdwieland8282

CC @bretg

refreshUserIds is called prior to posting data which happens only when a email is found.

robertrmartinez · 2020-11-03T19:23:39Z

modules/idLibrary.js

+  const value = event.target.value;
+  logInfo(`Modified Value of input ${event.target.value}`);
+  email = getEmail(value);
+  if (email != null) {


same, strict check since getEmail explicitly will return null.

robertrmartinez · 2020-11-03T19:25:51Z

modules/idLibrary.js

+    logInfo(` Original Value in Input = ${inputs[i].value}`);
+    inputs[i].addEventListener('change', event => processInputChange(event));
+    inputs[i].addEventListener('blur', event => processInputChange(event));
+  }


Could use a forEach instead of a for loop since you do not need to exit early or anything.

Just a personal preference, can leave it.

using forEach LGTM analysis: JavaScript test failed so used for loop

robertrmartinez · 2020-11-03T19:26:24Z

modules/idLibrary.js

+  return function () {
+    var context = this;
+    var args = arguments;
+    var later = function () {


usually we use let / or const now instead of actual var declaration.

No big deal though.

robertrmartinez · 2020-11-03T19:27:18Z

modules/idLibrary.js

+    for (const node of mutation.addedNodes) {
+      email = node.textContent;
+
+      if (email) {


If we get an email do we need to keep going through all other mutations?

Should there only ever be one email on page?

Should we break here?

Once email is found, data is posted and returned now

robertrmartinez · 2020-11-03T19:27:24Z

modules/idLibrary.js

+
+  const body = document.body.innerHTML;
+  email = getEmail(body);
+  if (email != null) {


SKOCHERI · 2020-11-05T22:00:19Z

@robertrmartinez @smenzer All the review comments has been addressed. Can you take a look and approve if no more comments.

smenzer · 2020-11-06T10:37:59Z

I have no other comments, thanks.

smenzer

LGTM, pending review by @robertrmartinez

robertrmartinez · 2020-11-09T18:59:56Z

modules/idLibrary.js

+      targetObserver.observe(targetElement, OBSERVER_CONFIG);
+    } else {
+      logInfo(' Target found with target ' + email);
+      logInfo(' Post data on email found in target with target');


I think the space is already included in the logInfo function you created, are these needed?

robertrmartinez · 2020-11-09T19:00:36Z

modules/idLibrary.js

+  email = getEmail(document.body.innerHTML);
+  if (email !== null) {
+    logInfo('Email found in body ' + email);
+    logInfo(' Post data on email found in the body without observer');


unnecessary space?

robertrmartinez · 2020-11-09T19:02:22Z

modules/idLibrary.js

+      logInfo(' Data synced successfully.');
+    },
+    error: function () {
+      logInfo(' Data sync failed.');


spaces still

robertrmartinez · 2020-11-09T19:28:49Z

Code looks good now,

I am going to do some on page testing today and if it goes well we should be good to merge for tomorrows release!

* ID Library * build fix * build fix * build fix * build fix * Fixing review comments * Fixing review comments (debounce, full body scan option * Fixing review comments (var declaration, strict check) * Fixing space Co-authored-by: skocheri <skocheri@rubiconproject.com>

ID Library

7440f9e

SKOCHERI closed this Oct 16, 2020

SKOCHERI reopened this Oct 16, 2020

build fix

021cd69

build fix

a44f22a

build fix

9a20eaf

build fix

0aec7ea

jdwieland8282 requested a review from robertrmartinez October 19, 2020 17:06

gglas assigned gglas and robertrmartinez and unassigned gglas Oct 21, 2020

robertrmartinez requested changes Oct 29, 2020

View reviewed changes

Fixing review comments

4a6c09c

robertrmartinez requested changes Nov 2, 2020

View reviewed changes

Fixing review comments (debounce, full body scan option

3221040

robertrmartinez requested changes Nov 3, 2020

View reviewed changes

Fixing review comments (var declaration, strict check)

33f9a77

smenzer approved these changes Nov 6, 2020

View reviewed changes

robertrmartinez reviewed Nov 9, 2020

View reviewed changes

Fixing space

b617a84

robertrmartinez approved these changes Nov 12, 2020

View reviewed changes

robertrmartinez merged commit 954acdb into prebid:master Nov 12, 2020

patmmccann mentioned this pull request Nov 19, 2020

Prebid 5.0 #5966

Closed

ID Library #5863

ID Library #5863

Conversation

SKOCHERI commented Oct 16, 2020 • edited by jdwieland8282 Loading

Type of change

Description of change

Other information

lgtm-com bot commented Oct 16, 2020

lgtm-com bot commented Oct 16, 2020

lgtm-com bot commented Oct 16, 2020

lgtm-com bot commented Oct 16, 2020

lgtm-com bot commented Oct 16, 2020

jdwieland8282 commented Oct 16, 2020

smenzer commented Oct 20, 2020

jdwieland8282 commented Oct 20, 2020

smenzer commented Oct 20, 2020

jdwieland8282 commented Oct 20, 2020 • edited Loading

robertrmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdwieland8282 Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdwieland8282 commented Oct 29, 2020

lgtm-com bot commented Nov 2, 2020

robertrmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smenzer commented Nov 3, 2020

jdwieland8282 commented Nov 3, 2020

robertrmartinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SKOCHERI commented Nov 5, 2020

smenzer commented Nov 6, 2020

smenzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertrmartinez commented Nov 9, 2020

SKOCHERI commented Oct 16, 2020 •

edited by jdwieland8282

Loading

jdwieland8282 commented Oct 20, 2020 •

edited

Loading

jdwieland8282 Nov 2, 2020 •

edited

Loading