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

updated userid module to stop caching the entire consent object #5641

Merged

Conversation

smenzer
Copy link
Collaborator

@smenzer smenzer commented Aug 20, 2020

Type of change

  • Bugfix

Description of change

after the discussion at the end of #5451, we agreed that we should store just a hash of the consent object rather than the entire object, since all we need it for is comparison purposes between the previous consent and the current consent for the user.

Other information

#5451 (comment)

…ather just a hash of it, since all we need it for is comparison purposes.
@smenzer
Copy link
Collaborator Author

smenzer commented Aug 20, 2020

the CI fail seems unrelated to this PR, it's from of terceptAnalyticsAdapter_spec.js:771

modules/userId/index.js Outdated Show resolved Hide resolved
@jsnellbaker jsnellbaker added the needs 2nd review Core module updates require two approvals from the core team label Aug 20, 2020
src/utils.js Outdated
// IE doesn't support imul
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/imul#Polyfill
if (!Math.imul) {
Math.imul = function(opA, opB) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was under the impression that we were not allowing polyfills.

I could be wrong but I thought I read / heard that somewhere...

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

This looks good, but I left a comment regarding polyfills.

I thought I heard or read somewhere that we were not going to allow them...

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Alright sweet looks good, thanks Scott

@smenzer smenzer merged commit 0d8d9bf into prebid:master Aug 20, 2020
@smenzer smenzer deleted the 5-userid-hash-consent-string-before-caching branch August 20, 2020 16:30
* @param seed (optional)
* @returns {string}
*/
export function simpleHash(str, seed = 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

any chancce you can call this cyrb53Hash or something describing what it is; the source you chose has many hashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants