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

DeepIntent Id Module : fix user ids not being passed on page reload due to getValue func miss #11383

Merged
merged 12 commits into from May 6, 2024

Conversation

parthshah51999
Copy link
Contributor

@parthshah51999 parthshah51999 commented Apr 23, 2024

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

Other information

On page reload, getId is not called from Id system submodule and it expects function extendId to provide the logic to read the id from storage. This is not implemented in deepintentDpesSubmodule which causes response Object to be blank. Because this is blank storedId doesn't get updated to response.id

local storage format: key : {"id": "123123"}

@smenzer
Copy link
Collaborator

smenzer commented Apr 23, 2024

the user id module should pull the ID from cache automatically if using the core id module to save it (as opposed to doing it manually); extendId should not be required unless you're trying to do something extra with the cached value on every page, which in this PR, it doesn't look like you're doing. so it seems like there's an issue with the page setup or something else is going on to cause the issue you're seeing.

@parthshah51999
Copy link
Contributor Author

parthshah51999 commented Apr 24, 2024

the user id module should pull the ID from cache automatically if using the core id module to save it (as opposed to doing it manually); extendId should not be required unless you're trying to do something extra with the cached value on every page, which in this PR, it doesn't look like you're doing. so it seems like there's an issue with the page setup or something else is going on to cause the issue you're seeing.

Thanks for your response, @smenzer. Unfortunately this does not look like a setup issue. I am able to replicate this locally as well. Here's how the flow works:

  1. DeepIntent script is responsible for setting the token in local storage. This is done in the format: key : {"id": "123123123"}.
  2. In the prebid code, "populateSubmoduleId" method is called in the init flow. In the intial flow idSystem.getId gets called. This returns the {id: "123123123"} object.
  3. Post this we check the response is a plainObject and update the local storage to key: "123123123". Could you please let me know why this is done as this creates an inconsistency. On page reload, getId is no longer called but our script updates the value back to {id: "123123123"}. Hence storedId gets 2 different values. Initially it gets the string value, later it starts getting the object.
  4. When we get an object, the createEidObject in file eids.js no longer is able to read the id as it expects a string. Implemented the getValue function to ensure that if we get an object read from it otherwise if it is a string (which prebid updates in local storage) return it as it is.
  5. For now I have added the conditional check, and it resolves our problem of eids not being read. But why would we make an update to the local storage ? And if we need to do this can we not ensure that storedId is always a string in populateSubmoduleId ?

@parthshah51999 parthshah51999 changed the title fix user ids not being passed on page reload due to extendId func miss fix user ids not being passed on page reload due to getValue func miss Apr 24, 2024
@smenzer smenzer requested a review from dgirardi April 25, 2024 10:35
@smenzer
Copy link
Collaborator

smenzer commented Apr 25, 2024

i think we probably need an engineer that knows the details of the module a bit better than myself (product manager) to help out here, @parthshah51999. i've requested that @dgirardi take a look to see if he can add some value.

@dgirardi
Copy link
Collaborator

The issue is that storage is meant to be managed by Prebid. On the other hand, I am not able to find a developer's guide for userId modules, so it's not surprising that this generates confusion.

This is the relevant piece of logic:

let storedId = getStoredValue(submodule);
let response;
let refreshNeeded = false;
if (typeof submodule.config.storage.refreshInSeconds === 'number') {
const storedDate = new Date(getStoredValue(submodule, 'last'));
refreshNeeded = storedDate && (Date.now() - storedDate.getTime() > submodule.config.storage.refreshInSeconds * 1000);
}
if (!storedId || refreshNeeded || forceRefresh || consentChanged(submodule)) {
// 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, gdprConsent, storedId);
} else if (typeof submodule.submodule.extendId === 'function') {
// If the id exists already, give submodule a chance to decide additional actions that need to be taken
response = submodule.submodule.extendId(submodule.config, gdprConsent, storedId);
}
if (isPlainObject(response)) {
if (response.id) {
// A getId/extendId result assumed to be valid user id data, which should be saved to users local storage or cookies
setStoredValue(submodule, response.id);
storedId = response.id;
}

In the "normal" flow, line 769 does not find anything in storage, getId is called, and the return value's .id is then stored in line 789. On refresh, it branches off to extendId if present.

In your case, an external script populates the cookie, but in a different way (stringified JSON.stringify({id}) instead of just the id). getId is still called because some other pieces of metadata are missing (consentChanged). Then the cookie is overwritten in line 789, and on refresh it's not what you expect.

TL;DR how to fix?

The "correct" way is to not use storage configuration, if storage is not managed by Prebid, and instead read the cookie directly from the module. The issue with this mixed setup is that the userSync.userIds.storage configuration is meant to let the publisher choose when and how the ID is stored; but in your case, I don't think they have a choice, it needs to agree with what your script does, and as it is it just gives them an opportunity to get it wrong.

The easy fix is to update

getId(config, consentData, cacheIdObj) {
return cacheIdObj;
},

to

    if (cacheIdObj?.id) return cacheIdObj;
    return cacheIdObj ? {id: cacheIdObj} : cacheIdObj;

but note that this will still overwrite your cookie, so if you store anything else, or want to manage expiration, you'll still have problems.

@parthshah51999
Copy link
Contributor Author

The issue is that storage is meant to be managed by Prebid. On the other hand, I am not able to find a developer's guide for userId modules, so it's not surprising that this generates confusion.

This is the relevant piece of logic:

let storedId = getStoredValue(submodule);
let response;
let refreshNeeded = false;
if (typeof submodule.config.storage.refreshInSeconds === 'number') {
const storedDate = new Date(getStoredValue(submodule, 'last'));
refreshNeeded = storedDate && (Date.now() - storedDate.getTime() > submodule.config.storage.refreshInSeconds * 1000);
}
if (!storedId || refreshNeeded || forceRefresh || consentChanged(submodule)) {
// 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, gdprConsent, storedId);
} else if (typeof submodule.submodule.extendId === 'function') {
// If the id exists already, give submodule a chance to decide additional actions that need to be taken
response = submodule.submodule.extendId(submodule.config, gdprConsent, storedId);
}
if (isPlainObject(response)) {
if (response.id) {
// A getId/extendId result assumed to be valid user id data, which should be saved to users local storage or cookies
setStoredValue(submodule, response.id);
storedId = response.id;
}

In the "normal" flow, line 769 does not find anything in storage, getId is called, and the return value's .id is then stored in line 789. On refresh, it branches off to extendId if present.

In your case, an external script populates the cookie, but in a different way (stringified JSON.stringify({id}) instead of just the id). getId is still called because some other pieces of metadata are missing (consentChanged). Then the cookie is overwritten in line 789, and on refresh it's not what you expect.

TL;DR how to fix?

The "correct" way is to not use storage configuration, if storage is not managed by Prebid, and instead read the cookie directly from the module. The issue with this mixed setup is that the userSync.userIds.storage configuration is meant to let the publisher choose when and how the ID is stored; but in your case, I don't think they have a choice, it needs to agree with what your script does, and as it is it just gives them an opportunity to get it wrong.

The easy fix is to update

getId(config, consentData, cacheIdObj) {
return cacheIdObj;
},

to

    if (cacheIdObj?.id) return cacheIdObj;
    return cacheIdObj ? {id: cacheIdObj} : cacheIdObj;

but note that this will still overwrite your cookie, so if you store anything else, or want to manage expiration, you'll still have problems.

Hey @dgiradi, thanks this works. Also noted that the cookie value is consistently overwritten post the fix. We are okay with this for now and will follow up if we need to make additional updates.

Quick question just for my understanding:
I noticed that after the recommended change the populateSubmoduleId function gets called twice and in the second call, forceRefresh param is passed as true which ensures that the flow works as expected. I did not understand the flow for this could you point me to the relevant piece of code so I can understand this better.

@dgirardi
Copy link
Collaborator

I noticed that after the recommended change the populateSubmoduleId function gets called twice and in the second call, forceRefresh param is passed as true which ensures that the flow works as expected

forceRefresh should not be needed for the flow to work, and whether it's set depends on your setup (in my case it isn't and the call happens once).

It's true when:

  • the user explicitly asks for a refresh with refreshUserIds
    /**
    * Force (re)initialization of ID submodules.
    *
    * This will force a refresh of the specified ID submodules regardless of `auctionDelay` / `syncDelay` settings, and
    * return a promise that resolves to the same value as `getUserIds()` when the refresh is complete.
    * If a refresh is already in progress, it will be canceled (rejecting promises returned by previous calls to `refreshUserIds`).
    *
    * @param submoduleNames? submodules to refresh. If omitted, refresh all submodules.
    * @param callback? called when the refresh is complete
    */
    function refreshUserIds({submoduleNames} = {}, callback) {
    return initIdSystem({refresh: true, submoduleNames})
    .then(() => {
    if (callback && isFn(callback)) {
    callback();
    }
    return getUserIds();
    });
    }
  • a userId module is installed after it has already been configured
    export function attachIdSystem(submodule) {
    if (!find(submoduleRegistry, i => i.name === submodule.name)) {
    submoduleRegistry.push(submodule);
    GDPR_GVLIDS.register(MODULE_TYPE_UID, submodule.name, submodule.gvlid)
    updateSubmodules();
    // TODO: a test case wants this to work even if called after init (the setConfig({userId}))
    // so we trigger a refresh. But is that even possible outside of tests?
    initIdSystem({refresh: true, submoduleNames: [submodule.name]});
    }
    }

@dgirardi
Copy link
Collaborator

Do you mind updating the unit tests? they are passing for all the wrong reasons:

it('Wrong config should fail the tests', () => {
// no config
expect(deepintentDpesSubmodule.getId()).to.be.eq(undefined);
expect(deepintentDpesSubmodule.getId({ })).to.be.eq(undefined);
expect(deepintentDpesSubmodule.getId({params: {}, storage: {}})).to.be.eq(undefined);
expect(deepintentDpesSubmodule.getId({params: {}, storage: {type: 'cookie'}})).to.be.eq(undefined);
expect(deepintentDpesSubmodule.getId({params: {}, storage: {name: '_dpes_id'}})).to.be.eq(undefined);
});
it('Get value stored in cookie for getId', () => {
getCookieStub.withArgs(DI_COOKIE_NAME).returns(DI_COOKIE_STORED);
let diId = deepintentDpesSubmodule.getId(cookieConfig, undefined, DI_COOKIE_OBJECT);
expect(diId).to.deep.equal(DI_COOKIE_OBJECT);
});
it('provides the stored deepintentId if cookie is absent but present in local storage', () => {
getDataFromLocalStorageStub.withArgs(DI_COOKIE_NAME).returns(DI_COOKIE_STORED);
let idx = deepintentDpesSubmodule.getId(html5Config, undefined, DI_COOKIE_OBJECT);
expect(idx).to.deep.equal(DI_COOKIE_OBJECT);
});

getId is not affected by any of the preconditions (all the parameters provided by the tests are ignored, and the storage mocks also have no effect).

@parthshah51999
Copy link
Contributor Author

parthshah51999 commented Apr 25, 2024

I noticed that after the recommended change the populateSubmoduleId function gets called twice and in the second call, forceRefresh param is passed as true which ensures that the flow works as expected

forceRefresh should not be needed for the flow to work, and whether it's set depends on your setup (in my case it isn't and the call happens once).

It's true when:

  • the user explicitly asks for a refresh with refreshUserIds
    /**
    * Force (re)initialization of ID submodules.
    *
    * This will force a refresh of the specified ID submodules regardless of `auctionDelay` / `syncDelay` settings, and
    * return a promise that resolves to the same value as `getUserIds()` when the refresh is complete.
    * If a refresh is already in progress, it will be canceled (rejecting promises returned by previous calls to `refreshUserIds`).
    *
    * @param submoduleNames? submodules to refresh. If omitted, refresh all submodules.
    * @param callback? called when the refresh is complete
    */
    function refreshUserIds({submoduleNames} = {}, callback) {
    return initIdSystem({refresh: true, submoduleNames})
    .then(() => {
    if (callback && isFn(callback)) {
    callback();
    }
    return getUserIds();
    });
    }
  • a userId module is installed after it has already been configured
    export function attachIdSystem(submodule) {
    if (!find(submoduleRegistry, i => i.name === submodule.name)) {
    submoduleRegistry.push(submodule);
    GDPR_GVLIDS.register(MODULE_TYPE_UID, submodule.name, submodule.gvlid)
    updateSubmodules();
    // TODO: a test case wants this to work even if called after init (the setConfig({userId}))
    // so we trigger a refresh. But is that even possible outside of tests?
    initIdSystem({refresh: true, submoduleNames: [submodule.name]});
    }
    }

Hey @dgirardi,

Yes the call happens only once. This led to more debugging from my end and it turns out to test a few things out I had explicitly called the refreshUserIds() function before the setConfig call from my test page.

Apologies for the miss on my end, but the solution you suggested is not working out.

  1. In our case the client is trying to set this up to read from local storage instead of cookies.
  2. After the storage gets updated due to line 789 like you mentioned, getId is no longer called and the code starts branching out to extendId.
  3. Due to this, any updates we make to the getId function (like you suggested) are no longer useful.
  4. This results in the eids not being populated on page refresh because of this check -
    if (isStr(value)) {

    (as our script updates the Id back to (stringified JSON.stringify({id}))
  5. Accepting the fact that prebid will continue to update the localStorage values there are 3 possible fixes that I saw:
    a. Implement the extendId method which will start returning a valid response so that storedId always gets a string value.
    b. Update the decode function called in line 801 and implemented in deepintentIdSystem to check if the the type of 'value' is a string/object and take appropriate action conditionally.
    c. What I have implemented to make sure that whenever we are trying to build eids, make sure the getValue function is implemented in config which will ensure that a string value is always read from the localStorage instead of an object. Something like this ref-
    getValue: function(data) {

Does that make sense? Let me know in case I am missing something.
Let me know if you want me to raise a separate PR for this considering the number of commits that have gone on this PR unnecessarily :p

@dgirardi
Copy link
Collaborator

You are right - my suggestion only works as long as you don't already have a cookie set.

I think (a) technically the best solution, but what you have should also work. (I was worried that it would only work for eids and not the "legacy" userId object, but reading through the code I think it happens to not matter).

@parthshah51999
Copy link
Contributor Author

You are right - my suggestion only works as long as you don't already have a cookie set.

I think (a) technically the best solution, but what you have should also work. (I was worried that it would only work for eids and not the "legacy" userId object, but reading through the code I think it happens to not matter).

I had implemented the extendId in the first iteration of this PR but this was suggested - #11383 (comment)

I can update the PR if necessary to implement the extendId function. But if there is no downside you foresee with this change this seems okay to me as well. Let me know how to proceed here.

@dgirardi
Copy link
Collaborator

This is OK, except for the tests - as I noted above, think most of the suite is not actually testing anything.

@parthshah51999
Copy link
Contributor Author

This is OK, except for the tests - as I noted above, think most of the suite is not actually testing anything.

Cool I will update the tests, thanks for your help!

@patmmccann patmmccann changed the title fix user ids not being passed on page reload due to getValue func miss DeepIntent Id Module: fix user ids not being passed on page reload due to getValue func miss Apr 26, 2024
@ChrisHuie ChrisHuie removed the request for review from dgirardi April 29, 2024 00:46
@parthshah51999
Copy link
Contributor Author

@dgirardi @ChrisHuie any specific reason this was not picked for the 8.47.0 release ?

@ChrisHuie ChrisHuie changed the title DeepIntent Id Module: fix user ids not being passed on page reload due to getValue func miss DeepIntent Id Module : fix user ids not being passed on page reload due to getValue func miss May 6, 2024
@ChrisHuie
Copy link
Collaborator

@dgirardi @ChrisHuie any specific reason this was not picked for the 8.47.0 release ?

Don't believe this pr was approved when I released Wednesday morning but merging now so it is included with this week's release.

@ChrisHuie ChrisHuie merged commit 2bb9f97 into prebid:master May 6, 2024
4 checks passed
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.

None yet

4 participants