Stop using surprising subsets of users; use `getAllUsersBy*` instead

[gnprice]

We have selectors getUsersById and getUsersByEmail that return maps from user IDs or emails to user objects.

Despite their names, they actually return only certain subsets of users:

• no "deactivated" users, i.e. people who've left the organization (but you might be looking at messages they sent, or PM threads with them, etc.)
• none of the "cross-realm bots", which are a handful of special built-in bots -- but OTOH normal bots are included, so it's not like this has a meaning like "only human users"

That particular subset is basically an artifact of the way the Zulip server returns the lists of users in the initial data on /register -- those two categories, and the remaining users, come in three separate lists. It's quite uncommon that this list is actually the data that a piece of code really wants to be working with.

That means that most places we use getUsersById or getUsersByEmail are a bug. One of these we just fixed was #4068 -- see discussion here: #4068 (comment) .

Instead, what we almost always want is getAllUsersById, or getAllUsersByEmail.

Thankfully we have only a handful of call-sites left for the getUsersBy* versions. We should fix each of them to use the getAllUsersBy* form -- or, if there's a reason it really should be the subset form, make that choice explicit with a comment.

(Then we should probably also switch around the names. But that can come after fixing the call sites.)

Here's a list of call sites, from rg getUsersBy:

• src/message/messagesActions.js -- should be getAll..., and I think this may be a live bug
• src/common/PresenceStatusIndicator.js -- should be getAll..., though I think not currently a live bug (because the code bails out if the user isn't found)
• src/notification/notificationActions.js -- should be getAll..., and I think this may be a live bug
• src/outbox/outboxActions.js -- should be getAll..., and I think this may be a live bug
• src/user-picker/UserPickerCard.js -- should be getAll..., though I think not currently a live bug
• src/users/userSelectors.js in getOwnUser and the similar getSelfUserDetail -- hmm, this one might be annoying if a lot of things care because cross-realm bots have a slightly different set of properties. We can do this one last.

The ones that are live bugs all involve a PM thread with a cross-realm bot or a deactivated user (including a group PM thread with one such bot/user).

[timabbott]

The main places we ignore deactivated users in the webapp codebase is for things like typeahead.

[rk-for-zulip]

• src/users/userSelectors.js in getOwnUser and the similar getSelfUserDetail -- hmm, this one might be annoying if a lot of things care because cross-realm bots have a slightly different set of properties. We can do this one last.

I think we can safely assume that the currently-logged-in user is not a cross-realm bot.

[gnprice]

I think we can safely assume that the currently-logged-in user is not a cross-realm bot.

Yes, which is why I didn't label that one (the last on the list) as a bug. But it'll still be good to get rid of it, particularly once the others are all gone -- it's silly to go and construct a giant data structure that's just a copy of most of the full set of users, just in order to get data about the one self-user.

In any event, as I said, we can do that one last. Let's focus on the rest.

[rk-for-zulip]

I think we can safely assume that the currently-logged-in user is not a cross-realm bot.

Yes, which is why I didn't label that one (the last on the list) as a bug. But it'll still be good to get rid of it, particularly once the others are all gone -- it's silly to go and construct a giant data structure that's just a copy of most of the full set of users, just in order to get data about the one self-user.

Ah, I mean that these can just use getAllUsersBy*, but still return User. That way any changes are isolated to their interior – making these potentially the simplest to deal with, rather than the most complicated.

I'm not sure what the initial mention of "cross-realm bots" is supposed to refer to, if not the return type changing.

[gnprice]

Ah, I mean that these can just use getAllUsersBy*, but still return User. That way any changes are isolated to their interior

Ah, got it. Yeah, that could be helpful.

[gnprice]

I believe this is fixed! I made these changes in some recent PRs, but I'd forgotten about having filed this issue.

There's one remaining callsite found by rg getUsersBy:

• src/users/userSelectors.js in getOwnUser […] -- hmm, this one might be annoying if a lot of things care because cross-realm bots have a slightly different set of properties. We can do this one last.

This behaves OK, because the self user will never be deactivated or a cross-realm bot. It'd be good to clean up this remaining call site and remove getUsersById` entirely, but only as a matter of simplicity and efficiency.

Of the others mentioned above:

• src/message/messagesActions.js -- should be getAll..., and I think this may be a live bug

Fixed in 9ba8e44 / #4332.

• src/common/PresenceStatusIndicator.js -- should be getAll..., though I think not currently a live bug (because the code bails out if the user isn't found)

Fixed in 30e2a79 / #4332.

• src/notification/notificationActions.js -- should be getAll..., and I think this may be a live bug

Fixed in 9ba8e44 / #4332.

• src/outbox/outboxActions.js -- should be getAll..., and I think this may be a live bug

Fixed in aff51f5 / #4299.

• src/user-picker/UserPickerCard.js -- should be getAll..., though I think not currently a live bug

Fixed in bdea60d / #4424.

• src/users/userSelectors.js in […] getSelfUserDetail

Removed in 9babf62 / #4424.