user: Convert a lot more emails to user IDs, and other cleanups#4424
user: Convert a lot more emails to user IDs, and other cleanups#4424gnprice merged 25 commits intozulip:masterfrom
Conversation
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, @gnprice, this LGTM! Just one comment about a possible tiny followup.
| import { View } from 'react-native'; | ||
|
|
||
| import type { UserId, UserOrBot } from '../types'; | ||
| import type { UserId } from '../types'; |
There was a problem hiding this comment.
| // TODO cut this `user.email` condition -- it should never trigger, and | ||
| // looks like a fudge for the possibility of data coming from NULL_USER | ||
| // looks like a fudge for the possibility of data coming from | ||
| // the late NULL_USER |
There was a problem hiding this comment.
I do think the user.email condition should probably be removed, maybe as a followup; I agree with the comment here.
It also seems obnoxious for UserItem to sometimes ignore the onPress that a caller has decided to pass—especially since the caller also controls the user that this condition is based on (whether by passing user directly, or by passing userId, which will produce a user that shouldn't have an empty email). An empty email seems like an unusual and special case (and one that I'm skeptical about existing at all, actually), so if it needs to be handled at all, maybe it should be handled in the onPress that individual callers pass.
I think the onPress condition (i.e., checking if onPress exists) can certainly be removed. It's marked as required, and all callers pass it.
There was a problem hiding this comment.
It also seems obnoxious for
UserItemto sometimes ignore theonPressthat a caller has decided to pass—especially since the caller also controls theuserthat this condition is based on
Yeah, agreed.
I think the
onPresscondition (i.e., checking ifonPressexists) can certainly be removed. It's marked as required, and all callers pass it.
Hmm, true. I think the right fix for that is actually in the other direction, though: the onPress should be marked as optional. At least one caller (in ShareToPm) passes a no-op callback for it; even if that weren't the case now, there could be callers in the future that just wanted to display the user and not offer an interaction related to them.
Making the distinction between having a real action to do here, and not having one, is good because among other things it can let us pass that distinction on to the underlying UI widgets, and that can give an important hint to a11y tools that some widgets don't have an interaction and shouldn't be offered to try to interact with. (I think this kind of thing is the cause of at least one of the items listed at #4394: "On the PM-conversations screen, the avatars are selectable. They shouldn't be".)
There was a problem hiding this comment.
Hmm, true. I think the right fix for that is actually in the other direction, though
Ah, yeah, those are good points.
One of many steps in our conversion from emails to user IDs to identify users. Like many of these conversion steps, this potentially fixes some buggy behavior in a case where some user's email has changed, and where our data didn't all agree on the email to use for them.
When the list is empty, that's exactly when there's nothing significant to be gained by returning the identical list rather than proceeding with the `Array#filter`. In fact there's really nothing at all to gain: the one way it might help is by enabling an "identical, therefore unchanged" optimization somewhere downstream, but at the one call site, we never pass the result as a React prop or anywhere else that might try to apply that optimization. Removing it is helpful because it lets us mark this function's input as a read-only array while keeping the output read-write, effectively signalling that this function returns a new copy rather than the original array. We want the output read-write so that the caller can go on to `Array#sort` it (without making an additional, unnecessary copy); and we'll want the input read-only shortly so that we can make it take `UserOrBot` elements and stay compatible with being passed an array that's supposed to contain specifically `User`.
This should have no effect on behavior, because this part of the UI never shows in the first place the users that were left out of the smaller map. But, first, it's simpler to understand the code and be confident it's right if we don't have to worry about checking that; and second, having this special extra smaller map means we have to wastefully construct a second map, duplicating references to all the users that are in it. So switch to looking up users in the normal, complete map that contains all the users we know about. Because the broader `allUsersByEmail` map contains values of the slightly less specific type `UserOrBot` rather than `User`, this also requires propagating that type through a range of other code.
Both of these data structures cause wasteful duplication of the data we have on users; all the same user objects can be found in `getAllUsersById`. One of them moreover has been part of our use of emails to identify users, which we're converting to user IDs. We've just eliminated the last caller of `getUsersByEmail` outside this selectors file itself; so we can convert the remaining users to `getUsersById`, and then delete the selector entirely. We also not long ago (in 9ba8e44) removed the last caller of `getUsersById` outside this file. Converting the remaining uses within the file isn't as trivial, so we'll leave that for a future commit; but we can at least unexport it, except for tests, to avoid adding further uses.
This simplifies the code, and lets us drop a use of `getAllUsersByEmail`.
Calling `setState` with values based on data we've previously read from the state is buggy, and explicitly forbidden by the React API: https://reactjs.org/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous The reason is that the actual change to the state may be applied asynchronously, and so several updates may race each other. Instead, the proper thing to do is to call `setState` with a callback that can take the state and compute the update. Do that. While here, we can simplify the code a bit by inlining trivial array manipulations.
This doesn't have much generalizable value -- its logic really represents part of the UI choices of this particular part of the UI. So move it to that UI code. One sign that this is an improvement is that we no longer need the "WARNING" label; instead, these particular choices can simply be commented as part of the UI choices for this particular UI.
If you're managing to mention the user, we should give the appropriate warning. This also just simplifies reasoning about this code, and avoids forcing us to construct a duplicative data structure of all "active" users.
I'm not sure it's actually possible to get a compose box for sending
to a deactivated user. If it is, then previously we'd fall back to
the generic "Type a message" placeholder, and now we show the same
"Message ${name}" as we do for anyone else.
More importantly this gives us one fewer distinct thing to reason
about, and one fewer dependency on a duplicative large data structure.
We've now eliminated all the uses of this selector outside this file, so we can unexport it to avoid adding more. We can't quite just delete it, because `getUserIsActive` relies on it as a cache.
In a lot of places where we pass around ownUser, we really just want the user ID, in particular to recognize when some other user ID is or isn't equal to it. Passing just the user ID simplifies things because it never changes, eliminating the possibility of being out of date. Many of the places we pass the full ownUser, in fact, formerly had just ownEmail, and we converted to ownUser in order to straddle code that now used the user ID and code that still relied on emails. So converting further to just ownUserId is a way of completing the email-to-user-ID conversion for that code.
Passing around a whole user object, instead of a bag of its properties, helps. We still need the email to pass to UserAvatarWithPresence -- which in fact uses it, because our presence data is still keyed by email.
These all appear inside the app's normal main UI, where we already assume we have our normal app data; so we can just use `getOwnUser`, which throws if it can't find the data it's supposed to return. Initially I thought that `OwnAvatar` might need a workaround similar to the one added in c1acd3a to `UserAvatar`, to deal with the same bug where the UI seems to remain mounted and get updates after the user has begun switching to another account and the data has changed out from under it. But empirically, things work just fine with this straightforward version. I think the point is probably that that bug specifically involves changes to `state.accounts`, and is triggered by the use of selectors like `getAuth` that rely on `state.accounts`. This code doesn't, so isn't affected.
This expresses just the properties that this component actually needs from these objects. A good thing, too, because in the case of showing autocomplete suggestions, these are the only properties it's getting that are at all explicitly chosen to be anything particularly reasonable; the rest are bogus data from the NULL_USER value. For that matter, even out of these three properties we use, one of them is also bogus data from NULL_USER in that situation: namely, `user_id`. That's why we have to explicitly handle its possible bogosity in `UserAvatarWithPresenceById`, the one place we use it.
…eed. Specifically we just need, for each option to present for autocomplete, a user ID, an email, and a name. That means that when making up such an entry that isn't a real user, we can invent just those fields, and don't need a generic "fake user" object to supply dubious values for a bunch of other fields. This still isn't a *great* way to handle these autocomplete suggestions that aren't users; instead we should have some actually appropriate bit of UI design, and data here that explicitly calls for that. But making it explicit like this what's going on here will help make it easier to go about that change, too.
We've converted all the former uses of this so that they instead make explicit choices about how to handle a "no such user" case, based on what's appropriate for their respective UI contexts.
|
Thanks for the review! Merged. Also replied to a followup comment above. I might go and do those quick followups next. |
2 participants
This branch pushes forward several interrelated cleanups to our data on users:
getActiveUsersBy*orgetUsersBy*maps to instead use the full,getAllUsersBy*map. Some of these areas of code have good reasons why they don't ever try to look up a user that won't be in the smaller maps. But it's better to just always use the full map anyway, for two reasons:NULL_USER, and then delete that value. Instead, when we can't find data on a user we wanted to operate on, either skip that user or fail at a higher level, as appropriate; and in general, have each spot of code that was usingNULL_USERinstead make explicit choices appropriate for its own context.At the end of this branch, #3764 is mostly complete; the majority of the remaining references to emails in the codebase are legitimate, mostly for logging in and for displaying in the UI. Of the remaining email-uses that should be eliminated, the most prominent are: