-
Notifications
You must be signed in to change notification settings - Fork 54
feat: SDKE-221 Support hashedEmailUserIdentityType for "other" identity type #1052
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
base: development
Are you sure you want to change the base?
feat: SDKE-221 Support hashedEmailUserIdentityType for "other" identity type #1052
Conversation
|
src/roktManager.ts
Outdated
this.identityService.identify({ | ||
userIdentities: { | ||
...currentUserIdentities, | ||
other: newEmailSha256 as string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Web you can identify with more than 1 identity at a time right? I just think if you need to identify with either you should just identify with both rather than possibly identifying twice.
src/roktManager.ts
Outdated
newHashedEmail = mappedAttributes['emailsha256'] as string || undefined; | ||
} | ||
|
||
const emailChanged = !!(newEmail && (!currentEmail || currentEmail !== newEmail)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So !! is a double not right so it doesn't invert like not(!), it just converts to a boolean?
So this would be = ((newEmail != nil && currentEmail == nil) || (newEmail != nil && currentEmail !== newEmail))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as long as my understanding of the logic is right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks good to me
src/roktManager.ts
Outdated
): void { | ||
const { userAttributeFilters, settings } = roktConfig || {}; | ||
const { placementAttributesMapping } = settings || {}; | ||
this.configSettings = settings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If configSettings
is just a copy of settings
, we should move it above the extraction of placementAttributesMapping
, and combine the destructuring of placementAttributesMapping
and hashedEmailUserIdentityType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexs-mparticle not sure I follow. Destructuring hashedEmaiLuserIdenittyType
from settings so that I can use it in selectPlacements
would require me to place a new setting on the Rokt class. Is that what you are suggesting? It's not used in the current init
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this here:
const { userAttributeFilters, settings } = roktConfig || {};
const { placementAttributesMapping, hashedEmailUserIdentityType } = settings || {};
Then you can avoid doing this later in the file:
const { hashedEmailUserIdentityType = null } = this.configSettings || {};
And then we don't need to store configSettings
on this
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing the loop here - confirmed with Alex that we shodl set it on the Rokt Class and not just destructure it so it can be used later.
src/roktManager.ts
Outdated
newHashedEmail = mappedAttributes['emailsha256'] as string || undefined; | ||
} | ||
|
||
const emailChanged = !!(newEmail && (!currentEmail || currentEmail !== newEmail)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid using double bang (!!
) in these checks because it makes it hard to read what's going on here. I think we created https://go.mparticle.com/work/SQDSDKS-7338
to refactor this into something readable, so now would be a great time to clean this up.
a729496
to
bd7785f
Compare
@alexs-mparticle please take another look |
Co-authored-by: Alex S <[email protected]>
test/jest/roktManager.spec.ts
Outdated
// Reset mocks | ||
jest.clearAllMocks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need to do this since it's in the beforeEach
?
// Reset mocks | |
jest.clearAllMocks(); |
…ected current identity type
|
Instructions
development
Summary
In this PR, we make an identify call if
emailsha256
passed toselectPlacement
differs from theother
identity value.Testing Plan
Integration tests and tested in an app
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)