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

Convert from emails to user IDs to identify users #3764

Open
gnprice opened this issue Jan 7, 2020 · 2 comments
Open

Convert from emails to user IDs to identify users #3764

gnprice opened this issue Jan 7, 2020 · 2 comments

Comments

@gnprice
Copy link
Member

gnprice commented Jan 7, 2020

This is a meta-issue which covers a large number of independent tasks. Filing it in order to have an identifier (ha) we can use to refer to the overall effort.

In general, Zulip originally used a user's email address as the primary way of identifying the user, throughout the API and the client data structures. This isn't good -- among other reasons, because people change their email addresses. So, years ago, we shifted toward using numeric user IDs. This was a slow migration, but the server and the webapp are now pretty far along; just about the whole API can now operate in terms of user IDs.

In the mobile app, though, we still pass around emails to identify users in lots of places. This often complicates the code, as different layers look up the email for a user ID and vice versa in order to communicate with other layers; it also leads to bugs when a user changes their email address. So we should fix it.

In general, this means:

  • Almost everywhere that we pass an email in an API request, it should instead be a user ID.
  • Almost everywhere that a React component takes a prop like email, it should instead be a userId.
    • Possibly everywhere. Anyplace where we have a good reason to take an email prop -- other than the boring reason that we haven't gotten around to converting it yet -- there should be a comment explaining why.
  • Almost everywhere that we have an object or Map data structure where the keys are emails, they should instead be user IDs.
    • The main exception is the data structures, like the one returned by getAllUsersByEmail, that we use only within code we haven't converted yet. Once other code is all converted, those should disappear too, or be used only in rare exceptional spots. (E.g., sending email addresses in API requests to older servers.)

Tasks for specific instances of this migration include #3501 and #3525; #3196 was related. There are also a lot of parts of the code where the conversion is straightforward; in those cases we don't need to file specific issues, and can just refer to this one.

@agrawal-d
Copy link
Member

agrawal-d commented Mar 10, 2020

Hi!
I'm going to work on migrating navigateToAccountDetails, AccountDetailsScreen, and getUserIsActive to user IDs, to make pull #3878 mergeable, as per #3878 (comment).

agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 12, 2020
We are moving towards user Ids to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
Ids, and other non functional changes required to enable that.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 12, 2020
This commit refactors all call sites of 'navigateToAccoutDetails'
to use user Ids, and other non functional changes required to enable
that.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 12, 2020
This commit refactors all call sites of 'navigateToAccoutDetails'
to use user Ids, and other non functional changes required to enable
that.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 19, 2020
This commit refactors all call sites of 'navigateToAccoutDetails'
to use user IDs, and other non functional changes required to enable
that.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 23, 2020
We are moving towards user Ids to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
Ids.

Also, if parsing the userId string to an integer fails in the
avatar message list event handler, a toast is shown instead of
navigating to the account details screen.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 23, 2020
We are moving towards user Ids to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
Ids.

Also, if parsing the userId string to an integer fails in the
avatar message list event handler, a toast is shown instead of
navigating to the account details screen.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 23, 2020
We are moving towards user IDs to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
IDs.

Also, if parsing the 'userId' string to an integer fails in the
avatar message list event handler, a toast is shown instead of
navigating to the account details screen.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 23, 2020
We are moving towards user IDs to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
IDs.

Also, if parsing the 'userId' string to an integer fails in the
avatar message list event handler, a toast is shown instead of
navigating to the account details screen.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 23, 2020
We are moving towards user IDs to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
IDs.

Also, if parsing the 'userId' string to an integer fails in the
avatar message list event handler, a toast is shown instead of
navigating to the account details screen.

Part of zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 24, 2020
We are moving towards user IDs to indentify users. This commit
refactors all call sites of navigateToAccoutDetails to use user
IDs, and other small changes required for it.

Part of zulip#3764.
gnprice pushed a commit to agrawal-d/zulip-mobile that referenced this issue Mar 26, 2020
Currently, AccountDetailsScreen uses 'getAccountDetailsUserForEmail'
to find the user object for the given email. But this selector is
deprecated - it involves the use of NULL_USER. Replace it with
the new selector 'getUserForEmail' instead.

This can behave differently in the case where the given email
doesn't belong to any known user.  The existing behavior is we'll
show the screen with some dummy data -- like showing the email as
the user's name; trying to use the `realm_uri` as their avatar URL;
and skipping presence information.

That's a pretty broken-looking experience if it can happen; and if
it can happen, it's a bug in the app.  (There's one known case that
can cause it, which is if the user in question changes their email
address.  This is basically part of why zulip#3764 is a bug.)  Instead of
showing some broken-looking bogus data, better to just show nothing.
The simplest version of that is to throw an error; we could do a
softer version of "show nothing", but because this case should be
rare the simple thing is fine for now.

[greg: expanded commit message]
gnprice pushed a commit to agrawal-d/zulip-mobile that referenced this issue Mar 26, 2020
We are moving towards user IDs to identify users.  This commit
refactors AccountDetailsScreen to take a user ID in its nav params,
all call sites of navigateToAccoutDetails to pass a user ID, and makes
other small changes required for that.

Part of zulip#3764.
gnprice added a commit to agrawal-d/zulip-mobile that referenced this issue Mar 26, 2020
This will hopefully help us use user IDs instead of emails everywhere
we can easily choose, as part of using user IDs everywhere, i.e. zulip#3764.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Apr 18, 2020
We are migrating from emails to user IDs to identify users, and
this is a move in that direction (zulip#3764).

Changes `TypingState` keys to use user IDs rather than emails.
Consequently, changes the consumers of TypingState to handle
this change. This also required replacing ownEmail with ownUserId
in the 'EventTyping' action.

Closes zulip#3930.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Apr 18, 2020
We are migrating from emails to user IDs to identify users, and
this is a move in that direction (zulip#3764).

Changes `TypingState` keys to use user IDs rather than emails.
Consequently, changes the consumers of TypingState to handle
this change. This also required replacing ownEmail with ownUserId
in the 'EventTyping' action.

Closes zulip#3930.
agrawal-d added a commit to agrawal-d/zulip-mobile that referenced this issue Apr 21, 2020
We are migrating from emails to user IDs to identify users, and
this is a move in that direction (zulip#3764).

Changes `TypingState` keys to use user IDs rather than emails.
Consequently, changes the consumers of TypingState to handle
this change. This also required replacing ownEmail with ownUserId
in the 'EventTyping' action.

Closes zulip#3930.
gnprice pushed a commit to agrawal-d/zulip-mobile that referenced this issue Apr 21, 2020
We are migrating from emails to user IDs to identify users, and
this is a move in that direction (zulip#3764).

Changes `TypingState` keys to use user IDs rather than emails.
Consequently, changes the consumers of TypingState to handle
this change. This also required replacing ownEmail with ownUserId
in the 'EventTyping' action.

Closes zulip#3930.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this issue May 12, 2020
... in support of a reducer for new state, shortly to be added.

(This is also in support of zulip#3764, although it takes no additional
stpes towards it.)
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this issue May 20, 2020
... in support of a reducer for new state, shortly to be added.

(This is also in support of zulip#3764, although it takes no additional
stpes towards it.)
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jan 6, 2021
This completes a major objective of the long string of refactoring
that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342,
then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368.  After this
change, the portion of zulip#4333 that's about PMs, emails, and user IDs --
aka the portion of zulip#3764 that's about narrows -- is complete.

Still open from zulip#4333 is to convert stream and topic narrows from
stream names to stream IDs.  I'm hopeful that that will be simpler:
(a) unlike the situation for PMs, there's just one stream mentioned
at a time, so there's no question of sorting, and (b) there isn't
the "include self or not" complication that's bedeviled much of our
code related to PMs.  In other words, stream and topic narrows
don't suffer from zulip#4035.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 15, 2021
…ties

When we added this function in 056930c, we may have specifically
chosen Identity objects over Account objects, to avoid adding a
place where we pass around auth credentials. Hmm.

Still, we've recently started storing the user ID in Account objects
(4fdefb0), and we have a TODO in this function for using it.

The Identity type doesn't have the user ID, and it's not currently
convenient to add it there. That's because the Identity type is
based on the Auth type (and we make Identity objects from Auth
objects), and the Auth type doesn't have the user ID. It's not super
convenient to add the user ID to the Auth type because there are
places where we make Auth objects but we don't have a user ID, like
`authFromCallbackUrl` in src/start/webAuth.js. As Greg points
out [1], those places are obstacles to zulip#3764, "Convert from emails
to user IDs to identify users".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 17, 2021
…ties

When we added this function in 056930c, we may have specifically
chosen Identity objects over Account objects, to avoid adding a
place where we pass around auth credentials. Hmm.

Still, we've recently started storing the user ID in Account objects
(4fdefb0), and we have a TODO in this function for using it.

The Identity type doesn't have the user ID, and it's not currently
convenient to add it there. That's because the Identity type is
based on the Auth type (and we make Identity objects from Auth
objects), and the Auth type doesn't have the user ID. It's not super
convenient to add the user ID to the Auth type because there are
places where we make Auth objects but we don't have a user ID, like
`authFromCallbackUrl` in src/start/webAuth.js. As Greg points
out [1], those places are obstacles to zulip#3764, "Convert from emails
to user IDs to identify users".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 23, 2021
…ties

When we added this function in 056930c, we may have specifically
chosen Identity objects over Account objects, to avoid adding a
place where we pass around auth credentials. Hmm.

Still, we've recently started storing the user ID in Account objects
(4fdefb0), and we have a TODO in this function for using it.

The Identity type doesn't have the user ID, and it's not currently
convenient to add it there. That's because the Identity type is
based on the Auth type (and we make Identity objects from Auth
objects), and the Auth type doesn't have the user ID. It's not super
convenient to add the user ID to the Auth type because there are
places where we make Auth objects but we don't have a user ID, like
`authFromCallbackUrl` in src/start/webAuth.js. As Greg points
out [1], those places are obstacles to zulip#3764, "Convert from emails
to user IDs to identify users".
chetas411 pushed a commit to chetas411/zulip-mobile that referenced this issue Nov 28, 2021
…ties

When we added this function in 056930c, we may have specifically
chosen Identity objects over Account objects, to avoid adding a
place where we pass around auth credentials. Hmm.

Still, we've recently started storing the user ID in Account objects
(4fdefb0), and we have a TODO in this function for using it.

The Identity type doesn't have the user ID, and it's not currently
convenient to add it there. That's because the Identity type is
based on the Auth type (and we make Identity objects from Auth
objects), and the Auth type doesn't have the user ID. It's not super
convenient to add the user ID to the Auth type because there are
places where we make Auth objects but we don't have a user ID, like
`authFromCallbackUrl` in src/start/webAuth.js. As Greg points
out [1], those places are obstacles to zulip#3764, "Convert from emails
to user IDs to identify users".
@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2021

As an update to see (most of) what remains for this task, I tried commenting out the email property on User:

 export type User = $ReadOnly<{|
   user_id: UserId,
-  email: string,
+  // email: string,

and seeing where Flow reported we're still using it.

  • In several places we still use emails because old server versions didn't accept user IDs:

    • get-messages
    • sending PMs
    • subscribing other users to streams
    • sending typing status

    I think those are all old enough now that we can drop that logic.

  • The one area that's blocked is presence: state.presence is indexed by email, and as a result components like UserAvatarWithPresence need an email. The root cause here is that the users/me/presence endpoint in the server API returns an object indexed by email. There's said to be work in progress on that on the server side.

  • In several places we use emails and it's appropriate to do so:

    • computing Gravatar URLs
    • matching for autocomplete
    • showing a list of users, where we sometimes include their emails

    In the future the server API will make it clearer when we have a real email, and when we have only a fake email like user1234@chat.example; see chat thread (which is what prompted me to make this survey). Then we can have appropriate fallback behavior in these cases:

    • Gravatar: use user.delivery_email ?? user.email, i.e. use the real email if we have it and fall back to the fake (but also, if we don't have a real email then the server should have already sent us a URL)
    • autocomplete: use the real email or skip it (and then match only on the display name)
    • display: show the real email or say something like "(Email not available)"

    That's all out of scope for this issue; we'll make separate issues for it when the server API for it is ready.

The above points also cover the remaining uses of email on PmRecipientUser, and of sender_email on Message.

There are a few other places we use emails:

  • In notifications, in sender_email on a notification for a 1:1 PM. New servers have sent sender_id for quite a while, and since 4773cf4 we assume they do, so we just need to complete the use of that.

  • In authentication, where we use the self-user's own email along with their API key. This should probably also change -- the status quo can't be a good experience when you change your own email address -- but let's call it out of scope for this issue and make it a followup.

sumj25 pushed a commit to sumj25/zulip-mobile that referenced this issue Jan 12, 2022
…ties

When we added this function in 056930c, we may have specifically
chosen Identity objects over Account objects, to avoid adding a
place where we pass around auth credentials. Hmm.

Still, we've recently started storing the user ID in Account objects
(4fdefb0), and we have a TODO in this function for using it.

The Identity type doesn't have the user ID, and it's not currently
convenient to add it there. That's because the Identity type is
based on the Auth type (and we make Identity objects from Auth
objects), and the Auth type doesn't have the user ID. It's not super
convenient to add the user ID to the Auth type because there are
places where we make Auth objects but we don't have a user ID, like
`authFromCallbackUrl` in src/start/webAuth.js. As Greg points
out [1], those places are obstacles to zulip#3764, "Convert from emails
to user IDs to identify users".
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 18, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 18, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants