feat: Identify if Provided Email in Rokt SelectPlacements#351
Conversation
BrandonStalnaker
requested review from
Mansi-mParticle,
alexs-mparticle and
rmi22186
BrandonStalnaker
changed the title
|
|
||
| - (void)confirmEmail:(NSString * _Nullable)email user:(MParticleUser * _Nullable)user completion:(void (^)(MParticleUser *_Nullable))completion { | ||
| if (email && email != user.identities[@(MPIdentityEmail)]) { | ||
| MPIdentityApiRequest *identityRequest = [MPIdentityApiRequest requestWithUser:user]; |
There was a problem hiding this comment.
I think we should log here a log that says something more explicit about the current email vs the passed in email. Something like:
The email sent to selectPlacements (foo@gmail.com) is different from what is on the current user (bar@gmail.com). Double check your implementation to confirm this is your intended behavior
There was a problem hiding this comment.
That kinda reads like an error message to me and might lead customers to think they are doing something wrong. Is that your intent? Maybe the second sentence could be more like. We will attempt to update the current user with this email but to receive a Rokt Layout as quickly as possible, sync the email identity to mParticle as soon as you receive it.
There was a problem hiding this comment.
I think actually there are 2 scenarios here that should be considered for messaging purposes. both would result in calling identify, but one i think is a bug
- if (selectPlacements email && !currentUser.email) // this is not a bug, as there's a chance that someone is checking out as guest, and so they'd only get the email once
- if (selectPlacements email && currentUser.email && selectPlacements email !== currentUser.email) --> this i think is a bug. if the user has an email, then the selectPlacements email should match it. we should log that as a warning at a minimum to the user to let them know
the first scenario doesn't need any logging since it's a valid case, the 2nd one does need a log
mParticle-Apple-SDK/MPRokt.m
Outdated
| NSLog(@"Failed to automatically update email from selectPlacement: %@", error); | ||
| completion(user); | ||
| } else { | ||
| NSLog(@"Updated email identity based off selectPlacement's attributes: %@", apiResult.user.identities.description); |
There was a problem hiding this comment.
Does this provide a description of all the identities that are on the user? Cursor is telling me something like the following would come back:
{
7 = "user@example.com"; // MPIdentityEmail
1 = "customer123"; // MPIdentityCustomerId
2 = "fb123456"; // MPIdentityFacebook
4 = "google123"; // MPIdentityGoogle
3 = "twitter123"; // MPIdentityTwitter
5 = "ms123456"; // MPIdentityMicrosoft
6 = "yahoo123"; // MPIdentityYahoo
19 = "+1234567890"; // MPIdentityMobileNumber
}
Is that accurate? If so, i think having it be by number is confusing.
I think just confirming which email was used is fine here also. **_Or_** if you add the above suggestion to give more detailed emails in the above log I recommended, then it would be redundant here and could be removed.
There was a problem hiding this comment.
yeah unfortunately identities are stored with the key being an integer. I could change it to just be the email.
| XCTAssertEqualObjects(testResult, expectedResult, @"Mapping does not match ."); | ||
| } | ||
|
|
||
| - (void)testSelectPlacementsIdentifyUser { |
There was a problem hiding this comment.
Can you add a test where there is an email (foo@gmail.com) and (bar@gmail.com) is set on selectPlacements - then assert that identify was called with bar@gmail.com?
There was a problem hiding this comment.
I'll try. Identity is really tricky to mock well but it should be doable
BrandonStalnaker
deleted the
feat/SQDSDKS-7247-Identify-If-Provided-Email-In-Rokt
branch
|
🎉 This PR is included in version 8.31.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)