-
Notifications
You must be signed in to change notification settings - Fork 369
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
chore: Identify by id #3898
chore: Identify by id #3898
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Ephemeral Environment
|
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.
@@ -91,7 +91,7 @@ global.API = { | |||
} | |||
|
|||
flagsmith | |||
.identify(user.email, { | |||
.identify(user.id, { |
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'd argue maybe this should be some concatenated string to avoid interspersion with the previous email based identities. Something like the following perhaps?
.identify(user.id, { | |
.identify(`user-${user.id}`, { |
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 was thinking we should delete them tbh @matthewelwell, it won't affect anything
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.
Yep, fair enough. I do still think that using a string concatenation like this is still a good idea, even if just for aesthetics.
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 think your suggestion around having a user guid would be the best, if we want to do that I'll hold off on this PR until we do that @matthewelwell ?
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
We're currently calling flagsmith.identify with email as the identity. This changes the behaviour to identify by user id.
How did I test
Validated we are identifying by uid, checked identity existed in Staging.