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

chore: Identify by id #3898

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: Identify by id #3898

wants to merge 1 commit into from

Conversation

kyle-ssg
Copy link
Member

@kyle-ssg kyle-ssg commented May 8, 2024

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

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.

Copy link

vercel bot commented May 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2024 1:31pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2024 1:31pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2024 1:31pm

@github-actions github-actions bot added the front-end Issue related to the React Front End Dashboard label May 8, 2024
Copy link
Contributor

github-actions bot commented May 8, 2024

Uffizzi Ephemeral Environment deployment-51442

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/3898

📄 View Application Logs etc.

What is Uffizzi? Learn more!

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

This seems fine, but it really makes me want to implement this or this (but that's much harder!).

@@ -91,7 +91,7 @@ global.API = {
}

flagsmith
.identify(user.email, {
.identify(user.id, {
Copy link
Contributor

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?

Suggested change
.identify(user.id, {
.identify(`user-${user.id}`, {

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end Issue related to the React Front End Dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants