feat: SDKE-123 Support hashedEmailUserIdentityType for "other" identity type #40
Conversation
rmi22186
requested review from
BrandonStalnaker,
alexs-mparticle and
Mansi-mParticle
src/Rokt-Kit.js
Outdated
| var data = mergeObjects({}, _data || {}); | ||
| if (_data.hasOwnProperty(OTHER_IDENTITY)) { | ||
| data[EMAIL_SHA256_IDENTITY] = _data[OTHER_IDENTITY]; | ||
| delete data[OTHER_IDENTITY]; |
There was a problem hiding this comment.
Didn't we agree to only remove email from the data if emailsha256 is set? mparticle-integrations/mparticle-apple-integration-rokt@29fe2db#diff-09961b2837d7729eae6e2c8c4aec512bda7efbc0901b0b59410bd17d5fcc36ccR339-R341
There was a problem hiding this comment.
oh wait this doesn't delete other from the attributes just from the identities data okay. 👍
There was a problem hiding this comment.
correct. I decided to make 2 distinct functions to make it clear. good question though
test/src/tests.js
Outdated
| }); | ||
|
|
||
| it('should prioritize other passed to selectPlacements over other in userIdentities', async () => { | ||
| it('should pass the attribute other in selectPlacements directly to Rokt', async () => { |
There was a problem hiding this comment.
| it('should pass the attribute other in selectPlacements directly to Rokt', async () => { | |
| it('should pass the attribute `other` in selectPlacements directly to Rokt', async () => { |
src/Rokt-Kit.js
Outdated
| return data; | ||
| } | ||
|
|
||
| function sanitizeIdentities(_data) { |
There was a problem hiding this comment.
I think sanitize is too vague and generic.
| function sanitizeIdentities(_data) { | |
| function sanitizeEmailIdentities(_data) { |
rmi22186
changed the title
src/Rokt-Kit.js
Outdated
| var EMAIL_IDENTITY = 'email'; | ||
|
|
||
| // Dynamic identity type for Rokt's emailsha256 identity value which MP doesn't support - will be set during initialization | ||
| var OTHER_IDENTITY; |
There was a problem hiding this comment.
I think its a bit confusing to name this OTHER_IDENTITY I feel like EMAIL_SHA256_IDENTITY makes more sense here and something like EMAIL_KEY and EMAIL_SHA256_KEY would make more sense for the other 2 variables
rmi22186
force-pushed
the
feat/SQDSDKS-7556-hashed-email-for-rokt
branch
from
8504101 to
e4ca1ed
Compare
|
@alexs-mparticle @BrandonStalnaker @Mansi-mParticle good for a final review |
src/Rokt-Kit.js
Outdated
| // Set dynamic OTHER_IDENTITY based on server settings | ||
| // Convert to lowercase since server sends TitleCase (e.g., 'Other' -> 'other') | ||
| if (settings.hashedEmailUserIdentityType) { | ||
| MAPPED_EMAIL_SHA256_IDENTITY = |
There was a problem hiding this comment.
I would make MAPPED_EMAIL_SHA256_IDENTITY a standard variable if you plan on assigning it to a value. This looks like a constant as is, so it shouldn't be assigned a value like this.
Co-authored-by: Alex S <[email protected]>
Summary
This PR adjusts behavior of how we handle
other. Previously,otherfrom selectAttributes was dropped as it was assumed to be the customer passing int a hashed email. However, product decided we should treatotheras any other attribute.This PR also removes
emailin addition tootheras an identity ifotheris passed in.Finally, if selectPlacement passes in
emailsha256, we remove theemailidentity.Testing Plan
Unit tests added.