narrow: Convert more email usage to user IDs#4361
Merged
chrisbobbe merged 11 commits intozulip:masterfrom Dec 29, 2020
Merged
Conversation
chrisbobbe
reviewed
Dec 29, 2020
Contributor
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! See just a few small comments below.
| /* @flow strict-local */ | ||
|
|
||
| import React, { PureComponent } from 'react'; | ||
| import React, { type ComponentType, PureComponent } from 'react'; |
Contributor
There was a problem hiding this comment.
user: Add "...ById" variant for UserAvatarWithPresence.
We'll temporarily have two variants of this component -- an
implementation in common but with slightly different interfaces -- as a
way to smoothly migrate from callers using one interface to the other.
The new interface keeps callers from having to know or care about
emails, which we're moving away from. It also encapsulates the need
to find the user's avatar URL; the caller only has to be concerned
with identifying which user it wants represented, by user ID. This
improves react-redux hygiene: it means that when that user's avatar
changes, we we no longer have to rerender the calling component,
which fundamentally shouldn't care about the specific avatar URL.
Nit in commit message: "we we"
|
|
||
| type Props = $ReadOnly<{ | ||
| email: string, | ||
| userId: number, |
Contributor
There was a problem hiding this comment.
I see that you've replaced the use of UserAvatarWithPresence with UserAvatarWithPresenceById in TitleGroup, but did you mean to do the same in TitlePrivate and forget? 🙂
Contributor
There was a problem hiding this comment.
It looks like we could also conveniently use UserAvatarWithPresenceById in UserItem without too much fuss, too?
Member
Author
There was a problem hiding this comment.
Sure, added those changes.
| // expected props. Normally that's just how our Props types are in the | ||
| // first place, but here we have to provide it explicitly. | ||
| /* eslint-disable flowtype/generic-spacing */ | ||
| (UserAvatarWithPresence: ComponentType< |
Contributor
There was a problem hiding this comment.
I think using react-redux's connect here is fine and convenient because we want to create a separate thing with a new interface, instead of modifying the original thing.
But when we settle on using this one and removing the old one, I wonder about trying out React Hooks, in particular, react-redux's useReducer (which is already available with the react-redux version we're on). True, we've gotten a pretty good handle on treating the "inner" and "outer" props in most places, with SelectorProps—but we won't have to do even that, when there's no wrapping component involved. 🙂
Member
Author
There was a problem hiding this comment.
Sure, that'd be interesting!
(useSelector, I think.)
Contributor
There was a problem hiding this comment.
(
useSelector, I think.)
Ah of course 🤪.
Member
Author
|
New revision pushed! |
We use this function to avoid trying to load UI for a narrow where the UI is doomed to crash because we're lacking basic data about it: in particular, a PM conversation where one of the putative participants is a user we have no data for. One way that can happen is if a user has changed their email address, and either we learn about the narrow by their old email address, or we still have them by their old email address and learn about the narrow by their new one. In the course of migrating code from using the emails in a PM-conversation narrow to using the user IDs, we're about to start doing so with code that will look the users up in our data structures to get further information like full name or avatar. So, in the spirit of "crunchy shell", we should apply the same checks here to the user IDs. Then at the end of this migration series, when we've converted everything else that consumes a PM narrow so that it uses IDs instead of emails, we can drop the email part here because it'll no longer be possible for it to matter.
This could conceivably affect behavior if we reached the point of having composed, and attempting to send, a PM where we don't actually have data for one of the recipients by email, but do by user ID, or vice versa. That shouldn't be possible, though, because the UI to show the conversation in the first place would have already errored out by that point.
This could conceivably affect behavior if we're showing a 1:1 conversation with a user we lack data for by email, but have it by user ID, or vice versa. We'd switch from showing the generic "Type a message" to a specific "Message Greg Price" or back. That shouldn't be possible, though, because the UI to show the conversation in the first place would have already errored out by that point.
We dropped the last use of this selector in the previous commit.
This isn't used, and makes the code more complex. We always effectively use the default of "rounded", never the alternative of "square"; so just hard-wire the "rounded" behavior. Back in 805d35d, fixing zulip#3398, we took out the old "circle" option here, which produced quite bad results when used. The "square" option could be OK in the right design context; but we don't actually use it, and haven't since 57f45f4 a couple of years ago. If we want that functionality in the future, we can add it back in the relevant places then.
We'll temporarily have two variants of this component -- an implementation in common but with slightly different interfaces -- as a way to smoothly migrate from callers using one interface to the other. The new interface keeps callers from having to know or care about emails, which we're moving away from. It also encapsulates the need to find the user's avatar URL; the caller only has to be concerned with identifying which user it wants represented, by user ID. This improves react-redux hygiene: it means that when that user's avatar changes, we no longer have to rerender the calling component, which fundamentally shouldn't care about the specific avatar URL.
Now this component doesn't need to know or care about the state we have for these users in Redux. After all, it was never really using any of those details -- it's the child components, showing their avatars and presence dots, that actually care.
This removes a few more references to emails.
chrisbobbe
force-pushed
the
pr-userids
branch
from
15a0d8d to
4116f83
Compare
Contributor
|
Great, thanks! Merged. |
This was referenced Dec 30, 2020
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this pull request
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the next tranche of changes in the series of PRs up to #4356. It converts several more of the places we use the emails from a PM narrow to use user IDs instead. Along the way we make some other conversions from user IDs to emails, as well as other cleanups in the code we're operating on.