Added global function for refreshing user id's #5752

coryhammon1 · 2020-09-15T19:58:26Z

Type of change

Bugfix
[ x] 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

Added a global function to refresh user id's after initialization on page load. This seems to be the simplest way to get this functionality without adding more complexity into the user id module itself. Essentially, the function triggers a reinitialization of all user id modules, so if there were any changes to the stored values after page load, those new values will be included in the any bid requests.

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
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

#5683

smenzer · 2020-09-16T08:44:35Z

this is a good start, but it doesn't take into account the fact that the user id modules will likely need to call their servers again to get a fresh ID. here's a use case:

user enters site example.com for the first time
userId module runs and each submodule calls their server to generate a fresh ID for the user
user logs in to the site with user@email.com
refreshIds() is called, but since the userId doesn't know it needs to refresh the submodules, it simply pulls the latest ID from localstorage and returns it via extendId() and then decode()

In fact, in step 4 the userId module actually needs to issue a getId() call instead.

TLadd · 2020-09-16T16:31:31Z

To address @smenzer's points, that refresh flag would need to be passed down into initSubmodules and then have that force it go down the getId path regardless of if there is a non-expired stored value or not. I agree this is necessary since the refresh doesn't do a whole lot of good if it just keeps the stored values around.

I would also like if there were a way to only refresh specific submodules. In my particular case, the effect of being logged in only effects a handful of submodules, so it would be better for me to trigger just the ones I know will benefit from the new information instead of having to update them all. I think two reasonable ways to do this would be:

a refreshUserId method that takes a submodule name
an option like refreshUserIds({ submodules: string[] }).

I would probably prefer refreshUserIds({ submodules: string[] }) since it keeps it to one global api method and also makes it easy to filter down to one or multiple submodules.

smenzer · 2020-09-16T16:58:25Z

i like that approach and something i was thinking about too. if you specify a list of submodules, then it's clear which ones are getting refreshed and having that passed in as a condition to call getId() would be perfect.

coryhammon1 · 2020-09-21T21:54:59Z

Refactored for individual submodule refresh. I've done some initial testing and the submodule's getId method is called on refresh. I may still be missing something though (I'm not super familiar with this code). Please let me know if there's more I need to do.

smenzer

a few comments/questions, but overall I like this approach

modules/userId/index.js

coryhammon1 · 2020-09-22T19:07:59Z

I added submodule initialization to the refresh function, so that it won't quit early if they aren't initialized. I think this will work correctly based on both your comments above.

smenzer · 2020-09-23T07:23:13Z

this is looking good. when you add tests, can you make sure that in addition to just calling getId for each refreshed module, that if a submodule's config value changes by the publisher, that the submodule also gets the changed config values (and getId isn't using a cached value for some reason)?

modules/userId/index.js

Refactored refresh user id's parameter to be optional where an empty list will result in all modules being refreshed.

coryhammon1 · 2020-09-28T21:56:49Z

Made the submodule name list parameter optional, and if absent all modules will be refreshed. Also, added a unit test, and verified that the most recently updated config will be read when refreshing.

smenzer

tests feel a bit light, can we add some about which submodules get refreshed based on what you pass in, etc.?

smenzer · 2020-09-29T09:01:27Z

modules/userId/index.js

+
+    if (!storedId || refreshNeeded || !!forceRefresh || !storedConsentDataMatchesConsentData(storedConsentData, consentData)) {
+      // No id previously saved, or a refresh is needed, or consent has changed. Request a new id from the submodule.
+      response = submodule.submodule.getId(submodule.config.params, consentData, storedId);


what do folks think about passing forceRefresh into this function so if a submodule wants to treat a refresh differently for some reason, they can?

@icflournoy do you have thoughts on this?

i've created a new issue to track interest in this: #5818

smenzer · 2020-10-01T07:56:39Z

tests feel a bit light, can we add some about which submodules get refreshed based on what you pass in, etc.?

@TLadd could you add a test (or edit one of the existing ones) to cover the refreshUserIds() callback, please? once that's done I'll merge. thanks!

smenzer · 2020-10-01T07:58:41Z

@TLadd @coryhammon1 The CI build is failing in Edge with this message ... can you look into it and see if you can find anything that broke the Zeotap adapter and attempt to fix, too, please?

1) when a stored Zeotap ID exists it is added to bids
     Zeotap ID System requestBids hook
     expected { Object (bidder, params) } to have deep nested property 'userId.IDP'
Error
   at AssertionError (webpack:///node_modules/assertion-error/index.js:74:1 <- test/test_index.js:24167:7)
   at Assertion.prototype.assert (webpack:///node_modules/chai/lib/chai/assertion.js:141:1 <- test/test_index.js:53648:7)
   at assertProperty (webpack:///node_modules/chai/lib/chai/core/assertions.js:1866:1 <- test/test_index.js:55544:7)
   at methodWrapper (webpack:///node_modules/chai/lib/chai/utils/addMethod.js:57:1 <- test/test_index.js:52778:5)
   at Anonymous function (webpack:///test/spec/modules/zeotapIdPlusIdSystem_spec.js:125:13 <- test/test_index.js:374855:13)
   at Array.prototype.forEach (native code)
   at Anonymous function (webpack:///test/spec/modules/zeotapIdPlusIdSystem_spec.js:124:11 <- test/test_index.js:374854:11)
   at Array.prototype.forEach (native code)
   at Anonymous function (webpack:///test/spec/modules/zeotapIdPlusIdSystem_spec.js:123:9 <- test/test_index.js:374853:9)
   at Anonymous function (webpack:///modules/userId/index.js:449:5 <- test/test_index.js:15160:5)

TLadd · 2020-10-01T16:55:33Z

tests feel a bit light, can we add some about which submodules get refreshed based on what you pass in, etc.?

@TLadd could you add a test (or edit one of the existing ones) to cover the refreshUserIds() callback, please? once that's done I'll merge. thanks!

Added that here: #5815. Am currently using that branch to try and figure out what's going on with the failing test as well. Pushed up some logs and seeing some odd behavior. Notice here Edge 17 fails, but here and here it's Edge 18, so it's not entirely deterministic.

In the build where I added logs, I get this output:

Edge 18.18363.0 (Windows 10.0.0) LOG LOG: 'STARTING FAILING TEST'
Edge 18.18363.0 (Windows 10.0.0) LOG LOG: 'GETTING ZEOTAPID COOKIE', ''
Edge 18.18363.0 (Windows 10.0.0) LOG LOG: 'CALLED getId', undefined
Edge 18.18363.0 (Windows 10.0.0) LOG LOG: '{"bidder":"sampleBidder","params":{"placementId":"banner-only-bidder"}}'

Compared to one of the browsers that succeeds:

Chrome 79.0.3945 (Windows 10.0.0) LOG LOG: 'STARTING FAILING TEST'
Chrome 79.0.3945 (Windows 10.0.0) LOG LOG: 'GETTING ZEOTAPID COOKIE', 'IlRISVMtSVMtQS1EVU1NWS1DT09LSUUi'
Chrome 79.0.3945 (Windows 10.0.0) LOG LOG: 'CALLED getId', '{"id":"IlRISVMtSVMtQS1EVU1NWS1DT09LSUUi"}'
Chrome 79.0.3945 (Windows 10.0.0) LOG LOG: 'SET idObj', '{"IDP":"THIS-IS-A-DUMMY-COOKIE"}'
Chrome 79.0.3945 (Windows 10.0.0) LOG LOG: '{"bidder":"sampleBidder","params":{"placementId":"banner-only-bidder"},"userId":{"IDP":"THIS-IS-A-DUMMY-COOKIE"},"userIdAsEids":[{"source":"zeotap.com","uids":[{"id":"THIS-IS-A-DUMMY-COOKIE","atype":1}]}]}'

So it fails because somehow the cookie isn't set properly in the beforeEach hook.

Edit, after significantly more logging, it seems that the cookie is set properly (it shows up in document.cookie) but then is oftentimes not present when it comes time to initialize the submodule. I looked quite a while for some place that might be unsetting the cookie but couldn't find anything. I ended up just trying to remove the expiration time being set on the cookie in the beforeEach hook on a whim and that seems to make the tests pass reliably. I have no clue why anything changed in this PR would have caused this.

So @smenzer believe #5815 is ready.

* Test callback in refreshUserIds test * Remove zeotapIdPlus expiration on cookie in test because it caused it to intermittently fail

smenzer · 2020-10-02T07:09:14Z

it looks like you've been working on an outdated version of master that has an old version of the zeotap test. if you look at the version in this PR compared to what is in master (https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/zeotapIdPlusIdSystem_spec.js) you'll see that several of these cookie calls have been removed per #5758. Make sure you do a full merge of master and resolve the conflicts, then this should be working properly. This may have been due to #5815 I'm not sure

TLadd · 2020-10-02T12:33:00Z

@smenzer Yep, sure enough, the same failures were happening on master before that PR that mocked writing to cookies: https://app.circleci.com/pipelines/github/prebid/Prebid.js/2130/workflows/efbe4e02-8090-41ea-a517-adef33a6410a/jobs/9900.

I've created #5819 that merged the latest master and removes the mocking, as it is no longer necessary after removing the expiration time on the cookie. I'm generally of the philosophy that mocking should be avoided unless they make tests easier to write/maintain and don't diminish the value of the test significantly. In this case, the tests aren't easier to write and assuming cookies work in the tests, I think that does provide some value to the tests. There are a lot things that can go wrong with cookies. Trying to write a unit test suite for cookie storage that covers every possible thing that could go wrong within Prebid would be really hard. Even if you did have such a suite, mocks can be set incorrectly and behave differently than reality. Part of the reason to bother running these tests in multiple browsers I assume is to catch browser-specific behavior, which you don't get when mocking browser API's.

Just my 2 cents on the matter; as I said in #5819, I can easily go back to the mocking solution if that's preferred.

smenzer · 2020-10-02T14:14:21Z

closing in favor of #5819

Added global function for refreshing user id's

b3d2df0

coryhammon1 requested a review from smenzer September 15, 2020 19:59

gglas assigned jsnellbaker Sep 16, 2020

jsnellbaker assigned smenzer and unassigned jsnellbaker Sep 16, 2020

Refactored submodule initialization to allow for refresh

e597884

smenzer requested changes Sep 22, 2020

View reviewed changes

modules/userId/index.js Outdated Show resolved Hide resolved

modules/userId/index.js Show resolved Hide resolved

modules/userId/index.js Outdated Show resolved Hide resolved

Added submodule initialization when refreshing user id's

ca064cf

TLadd reviewed Sep 23, 2020

View reviewed changes

modules/userId/index.js Outdated Show resolved Hide resolved

chammon added 2 commits September 23, 2020 11:23

Refactored refresh parameter to be optional

cfe74c2

Refactored refresh user id's parameter to be optional where an empty list will result in all modules being refreshed.

Added unit tests for refresh user id's

c1b091c

smenzer reviewed Sep 29, 2020

View reviewed changes

Added single module refresh test

ede6f1b

TLadd mentioned this pull request Oct 1, 2020

Test callback in refreshUserIds test and fix flakey test #5815

Merged

Test callback in refreshUserIds test and fix flakey test (#5815)

8b48836

* Test callback in refreshUserIds test * Remove zeotapIdPlus expiration on cookie in test because it caused it to intermittently fail

smenzer mentioned this pull request Oct 2, 2020

Should pbjs.refreshUserIds() pass a flag to user id modules that it was refreshed by the publisher? #5818

Closed

smenzer closed this Oct 2, 2020

TLadd mentioned this pull request Oct 7, 2020

Force refresh userId #5819

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added global function for refreshing user id's #5752

Added global function for refreshing user id's #5752

coryhammon1 commented Sep 15, 2020

smenzer commented Sep 16, 2020

TLadd commented Sep 16, 2020 •

edited

Loading

smenzer commented Sep 16, 2020

coryhammon1 commented Sep 21, 2020

smenzer left a comment

coryhammon1 commented Sep 22, 2020

smenzer commented Sep 23, 2020

coryhammon1 commented Sep 28, 2020

smenzer left a comment

smenzer Sep 29, 2020

smenzer Sep 30, 2020

smenzer Oct 2, 2020

smenzer commented Oct 1, 2020

smenzer commented Oct 1, 2020

TLadd commented Oct 1, 2020 •

edited

Loading

smenzer commented Oct 2, 2020

TLadd commented Oct 2, 2020

smenzer commented Oct 2, 2020

Added global function for refreshing user id's #5752

Added global function for refreshing user id's #5752

Conversation

coryhammon1 commented Sep 15, 2020

Type of change

Description of change

Other information

smenzer commented Sep 16, 2020

TLadd commented Sep 16, 2020 • edited Loading

smenzer commented Sep 16, 2020

coryhammon1 commented Sep 21, 2020

smenzer left a comment

Choose a reason for hiding this comment

coryhammon1 commented Sep 22, 2020

smenzer commented Sep 23, 2020

coryhammon1 commented Sep 28, 2020

smenzer left a comment

Choose a reason for hiding this comment

smenzer Sep 29, 2020

Choose a reason for hiding this comment

smenzer Sep 30, 2020

Choose a reason for hiding this comment

smenzer Oct 2, 2020

Choose a reason for hiding this comment

smenzer commented Oct 1, 2020

smenzer commented Oct 1, 2020

TLadd commented Oct 1, 2020 • edited Loading

smenzer commented Oct 2, 2020

TLadd commented Oct 2, 2020

smenzer commented Oct 2, 2020

TLadd commented Sep 16, 2020 •

edited

Loading

TLadd commented Oct 1, 2020 •

edited

Loading