Zeotap ID+ submodule #5640

shikharsharma-zeotap · 2020-08-20T09:47:11Z

Type of change

Feature

Description of change

This pr adds the zeotapIdPlusIdSystem submodule to add Zeotap IDP id to the bid requests.

Documentation PR Link: prebid/prebid.github.io#2260

smenzer

a few changes requested based on not seeing the need for storage configuration or configParams mostly

smenzer · 2020-08-20T16:40:00Z

integrationExamples/gpt/userId_example.html

+                    },
+                    {
+                      name: "zeotapIdPlus",
+                      storage: {
+                        type: "cookie",
+                        name: "IDP"
+                      }


it looks like you are defining your own name for the cookie and you're not leveraging the userId module storage (which is what this bit is used for). so I think you can just ignore this, like how Parrable does.

based on your module code, I think you can just do this:

Suggested change

},

{

name: "zeotapIdPlus",

storage: {

type: "cookie",

name: "IDP"

}

},

{

name: "zeotapIdPlus"

Hey @smenzer, thanks for the suggestion, have made changes to the module and files to remove storage configuration.

smenzer · 2020-08-20T16:40:48Z

modules/zeotapIdPlusIdSystem.js

+function isValidConfig(configParams) {
+  if (!configParams) {
+    utils.logError('User ID - zeotapId submodule requires configParams');
+    return false;
+  }
+  if (!configParams.name) {
+    utils.logError('User ID - zeotapId submodule requires valid config params: name');
+    return false;
+  }
+  return true;
+}


in your example you don't list any params, but you are requiring "name" here. what is the name param and how are you using it? i don't see it anywhere in your code

This is removed, not required.

smenzer · 2020-08-20T16:41:11Z

modules/zeotapIdPlusIdSystem.js

+}
+
+function fetchId(configParams) {
+  if (!isValidConfig(configParams)) return undefined;


per the above, i don't think you need this since you're not using any configParams

Suggested change

if (!isValidConfig(configParams)) return undefined;

made changes.

smenzer · 2020-08-20T16:42:33Z

test/spec/modules/userId_spec.js

+            name: 'zeotapIdPlus',
+            storage: { name: 'IDP', type: 'cookie' }


per above, this can probably be dropped

Suggested change

name: 'zeotapIdPlus',

storage: { name: 'IDP', type: 'cookie' }

name: 'zeotapIdPlus'

smenzer · 2020-08-20T16:44:37Z

test/spec/modules/userId_spec.js

@@ -1314,6 +1357,8 @@ describe('User ID', function() {
            name: 'sharedId', storage: {name: 'sharedid', type: 'cookie'}
          }, {
            name: 'intentIqId', storage: { name: 'intentIqId', type: 'cookie' }
+          }, {
+            name: 'zeotapIdPlus', storage: { name: 'IDP', type: 'cookie' }


per above, this can probably just be your name

Suggested change

name: 'zeotapIdPlus', storage: { name: 'IDP', type: 'cookie' }

name: 'zeotapIdPlus'

made changes.

smenzer · 2020-08-20T16:45:07Z

test/spec/modules/zeotapIdPlusIdSystem_spec.js

+  name: 'zeotapIdPlus',
+  storage: { name: 'IDP', type: 'cookie' }


probably can drop storage

Suggested change

name: 'zeotapIdPlus',

storage: { name: 'IDP', type: 'cookie' }

name: 'zeotapIdPlus'

smenzer · 2020-08-20T16:45:30Z

test/spec/modules/zeotapIdPlusIdSystem_spec.js

+        name: 'zeotapIdPlus',
+        storage: {
+          name: 'IDP',
+          type: 'cookie'
+        }


probably can drop storage

Suggested change

name: 'zeotapIdPlus',

storage: {

name: 'IDP',

type: 'cookie'

}

name: 'zeotapIdPlus'

test/spec/modules/zeotapIdPlusIdSystem_spec.js

smenzer

LGTM

test/spec/modules/zeotapIdPlusIdSystem_spec.js

shikharsharma-zeotap · 2020-08-25T20:39:51Z

@smenzer anything else required from my side for this to be merged?

smenzer · 2020-08-26T09:45:33Z

@shikharsharma-zeotap where are you actually generating the ID? if there's no ID in cookie/local storage...right now you just return undefined...is that what you want? No calls to your servers are required?

shikharsharma-zeotap · 2020-08-26T11:59:30Z

@smenzer the ID will be generated on the publisher page by our JS. No calls to our server is required from prebid's end.

patmmccann · 2020-08-26T12:43:18Z

Are modifications to the eids spec and userid spec required parts of adding a user id module? Most user id commits seem to modify them but not all. You seem to be missing a unit test on eids spec

smenzer · 2020-08-26T13:25:46Z

Are modifications to the eids spec and userid spec required parts of adding a user id module? Most user id commits seem to modify them but not all. You seem to be missing a unit test on eids spec

there are a couple tests in those two specs that look at basic testing for any modules that want to. eids seems required although the tests in userId_spec can be covered by a separate test suite; but better to have both than nothing if you ask me.

smenzer · 2020-08-26T13:26:24Z

@smenzer the ID will be generated on the publisher page by our JS. No calls to our server is required from prebid's end.

so if the publisher doesn't have your JS on page from a direct integration with Zeotap, you aren't expecting to be able to provide an ID, right?

shikharsharma-zeotap · 2020-08-26T13:29:11Z

Yes @smenzer, we expect publisher integration with zeotap for ID to be present, so generation/server calls from prebid are not required. Also, thanks @patmmccann , will be adding the Eid spec tests too.

smenzer · 2020-08-26T13:32:15Z

@idettman can you do a quick review and make sure you're ok with this before we merge? thanks!

patmmccann · 2020-08-26T16:41:49Z

@idettman can you do a quick review and make sure you're ok with this before we merge? thanks!

I don't think this can be merged until eids.js is covered with a test

patmmccann · 2020-08-26T16:44:42Z

modules/zeotapIdPlusIdSystem.js

+import {submodule} from '../src/hook.js';
+import { getStorageManager } from '../src/storageManager.js';
+
+const ZEOTAP_COOKIE_NAME = 'IDP';


should this be configurable by the pub? can you read if this is set in config and fallback to 'IDP' ?

@patmmccann I have added the tests to eid spec file. The cookie name 'IDP' is not configurable by the pub. It gets added to the storage by Zeotap JS after publisher integration. I didn't get the other question.

shikharsharma-zeotap · 2020-09-02T07:58:45Z

@idettman waiting for your comments. @smenzer @patmmccann have resolved conflicts with base branch. Can we merge this now?

smenzer

thanks for the additional test for eids.

CI is failing, though, so that needs to be fixed before we can merge. @idettman can you do a quick review, too, please?

shikharsharma-zeotap · 2020-09-03T09:08:40Z

@smenzer the circleCI shows 2 tests breaking during the BrowserStack testing.
One is related to Parrable Id Module, other is for Zeotap Id module. Is there something that I require to fix from my end, so that CI looks green?

smenzer · 2020-09-04T13:21:32Z

@shikharsharma-zeotap i'm not really sure why the builds are failing...locally they run fine for me. i've reached out to the prebid community in slack and perhaps someone else here can take a look at things. once that is fixed this is good to go from my end

smenzer · 2020-09-10T16:28:48Z

@shikharsharma-zeotap can you try pulling in the latest master to see if that helps at all?

shikharsharma-zeotap · 2020-09-11T09:27:42Z

Hey @smenzer , have rebased with the latest master, looks like the circleCI has stopped complaining for us now. Thanks!

smenzer · 2020-09-11T09:33:28Z

Hey @smenzer , have rebased with the latest master, looks like the circleCI has stopped complaining for us now. Thanks!

great! I'd like to get a review from @jlukas79 and then this can be merged.

robertrmartinez requested a review from smenzer August 20, 2020 16:12

robertrmartinez assigned smenzer Aug 20, 2020

robertrmartinez requested a review from idettman August 20, 2020 16:12

robertrmartinez assigned idettman Aug 20, 2020

smenzer requested changes Aug 20, 2020

View reviewed changes

idettman added needs review needs update needs 2nd review Core module updates require two approvals from the core team labels Aug 20, 2020

shikharsharma-zeotap requested a review from smenzer August 23, 2020 21:24

smenzer approved these changes Aug 24, 2020

View reviewed changes

test/spec/modules/zeotapIdPlusIdSystem_spec.js Show resolved Hide resolved

patmmccann reviewed Aug 26, 2020

View reviewed changes

shikharsharma-zeotap force-pushed the IDU-117 branch from 5e0b415 to 36dcf11 Compare August 29, 2020 09:23

smenzer reviewed Sep 2, 2020

View reviewed changes

smenzer assigned jsnellbaker Sep 7, 2020

shikharsharma-zeotap added 3 commits September 11, 2020 14:33

IDU-117 IDU-119 Add zeotap submodule

9334591

IDU-117 IDU-119 Add tests for zeotapId+ module

699c53a

add zeotapId+ module spec

8bf0bc7

shikharsharma-zeotap added 7 commits September 11, 2020 14:33

Add IDP base64 decode logic

5e055e8

remove unwanted file changes

9b52f84

rename zeotapId+ to zeotapIdPlus

435f3c7

add zeotapIdPlus submodule to submodules.json

f587d2a

refactor code for requested changes: remove storage from configParams

20f152e

add tests to eids_spec

4fe5411

rebase n resolve conflicts

3b56ee6

shikharsharma-zeotap force-pushed the IDU-117 branch from 36dcf11 to 3b56ee6 Compare September 11, 2020 09:16

smenzer assigned jlukas79 and unassigned idettman and jsnellbaker Sep 11, 2020

smenzer requested review from jlukas79 and removed request for idettman September 11, 2020 09:32

jlukas79 approved these changes Sep 11, 2020

View reviewed changes

smenzer merged commit 1e9be73 into prebid:master Sep 11, 2020

		name: 'zeotapIdPlus',
		storage: { name: 'IDP', type: 'cookie' }

	name: 'zeotapIdPlus',
	storage: { name: 'IDP', type: 'cookie' }
	name: 'zeotapIdPlus'

	name: 'zeotapIdPlus', storage: { name: 'IDP', type: 'cookie' }
	name: 'zeotapIdPlus'

Zeotap ID+ submodule #5640

Zeotap ID+ submodule #5640

Conversation

shikharsharma-zeotap commented Aug 20, 2020

Type of change

Description of change

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

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

smenzer left a comment

Choose a reason for hiding this comment

shikharsharma-zeotap commented Aug 25, 2020

smenzer commented Aug 26, 2020

shikharsharma-zeotap commented Aug 26, 2020

patmmccann commented Aug 26, 2020 • edited Loading

smenzer commented Aug 26, 2020

smenzer commented Aug 26, 2020

shikharsharma-zeotap commented Aug 26, 2020

smenzer commented Aug 26, 2020

patmmccann commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikharsharma-zeotap commented Sep 2, 2020

smenzer left a comment

Choose a reason for hiding this comment

shikharsharma-zeotap commented Sep 3, 2020

smenzer commented Sep 4, 2020

smenzer commented Sep 10, 2020

shikharsharma-zeotap commented Sep 11, 2020

smenzer commented Sep 11, 2020

patmmccann commented Aug 26, 2020 •

edited

Loading