Check mobile compatibility for limited guests

[alya]

We should make sure the mobile app is compatible with the changes being made in zulip/zulip#10970.

[gnprice]

I've just taken a look at this, and I've sent #5790 to fix the two issues I found.

I think neither of those is a showstopper. So we should get those fixes out promptly, but not worry about some users still having old clients while encountering the new feature.

I was testing on an approximation of the real feature, though: I tried running zulip/zulip#27544 in my dev server, but AFAICT that PR doesn't yet actually make it possible to enable the feature — it only lays the groundwork. So it'd be good to re-check compatibility when there's something more like a working version of the server feature which can be tried out.

---

Details on the severity of the two issues

• They're crashes, which is bad.
• But they happen only on relatively uncommon actions (tapping a message sender's name or avatar to go to their profile, or selecting "See who reacted" on a message), and only if the user involved is one you're not able to see, even though they sent or reacted to a message you're looking at.
• And I believe even getting into the latter situation is a fairly uncommon edge case: IIUC you can see any user you're in a stream with, and any user you have DM history with, so it'd have to be something like that the user has since left the stream.

Details on how I approximated the "limited guests" feature

I applied the following ad-hoc diff:

--- a/src/users/usersReducer.js
+++ b/src/users/usersReducer.js
@@ -14,2 +14,4 @@ const initialState: UsersState = NULL_ARRAY;
 
+const allowedUsers = new Set(["Cordelia, Lear's daughter", 'Polonius']);
+
 export default (
@@ -23,3 +25,3 @@ export default (
     case REGISTER_COMPLETE:
-      return action.data.realm_users;
+      return action.data.realm_users.filter(u => allowedUsers.has(u.full_name));
 

This causes the app, when it's first loading server data, to drop on the floor any users other than the two named. (And other than deactivated users and system bots, which this diff doesn't affect.) Then I logged into my dev server as Polonius, a guest user.

Details on what I tested

I explored various parts of the UI in the presence of unknown users, trying to think of all the various places we show information on a user:

• The inbox screen, and the DM-conversations screen. These behaved fine — any conversations involving unknown users were just omitted.

  IIUC any users we have DM history with should not be unknown, so this shouldn't even come up.

• The message list. This was fine — the name and avatar of a message sender shows up even if the user is unknown, because we get it from the message object anyway.

• Lists of all users: specifically the "New DM" screen and the "Add subscribers" (to a stream) screen. (There are a couple of other places these lists can appear, but they use the same code so I stopped after those two.) These worked fine — any unknown users just don't show up.

• Autocomplete for @-mentions. This worked fine — any unknown users just don't show up.

• The profile screen, and the "See who reacted" screen. These crashed as described above.

I also looked through the code:

• We have a getUserForId function which looks up a user by their user ID and throws if not found. (After all, until this feature, in the bulk of the app that could only be a sign of a bug.) This was the source of both of the crashes above. I looked at all call sites; the remaining two are harmless.
• I looked around the code for places where rather than showing UI, we're handling an event from the server, and where we might look up data on a user. All the ones I found are already using tryGetUserForId (the non-throwing version of getUserForId, and are doing something reasonable if they don't find the user.
• I also looked at where we handle opening a notification (and trying to navigate to the conversation). There too, if we don't find the user then we just fail gracefully rather than crash.

[alya]

Awesome, thank you for the investigation and the detailed notes!

@sahil839 FYI

[gnprice]

OK, with zulip/zulip#27544 and then zulip/zulip#27873, it's now possible to test the actual implementation of this using a dev server. Here's what I found:

• The user list, for e.g. the "New DM" button: The unknown users appear here, named "Unknown user". This is a bit funny-looking, but doesn't break anything.

  This is a consequence of our compatibility design for this feature, where the server gives the client user objects full of fake data for all the users it's not allowed to know about. In the future we intend to have a way for the client to tell the server to skip those fake users, but my reading of https://chat.zulip.org/#narrow/stream/378-api-design/topic/client.20capability.20for.20sending.20unknown.20users.20data/near/1687602 is that that isn't implemented yet. Once the server has that flag, we can have the client start sending it.

  • The avatars are missing here; they show as a blank space, rather than the "unknown user" avatar we chose. Haven't debugged that yet; it'd be good to fix.

• DM message list: This is a situation that ideally should be hard to reach, but is currently easy to get to via "New DM" because of the point above. (It'll still be possible in principle because you could be looking at such a message list and then lose access to the user.)

  This works fine; the app bar even shows the "unknown user" avatar, unlike the user list.

  • If you try to send a message, the outbound message will sit there never succeeding. Ideal behavior would be that instead we'd disable the send button, and give you a nice error message if you try to tap it. But it doesn't seem worth doing anything about that given that this screen should be hard to reach.

• User profile: Works fine except the avatar is again missing.

• Message list, for unknown sender: Like before, works fine and even shows the name and avatar, because we get those from the message itself.

• Message list, for person who left an emoji reaction: Works fine, saying "Unknown user".

• "See who reacted" screen: Works fine except the avatar is missing.

• Autocomplete for @-mentions: Much like the user list, this shows the unknown users as "Unknown user", which we can avoid in the future when the server has implemented the capability to skip sending the fake user objects. This one does show the "unknown user" avatar, though.

---

In summary:

• No crashes; nothing that breaks other functionality of the app.
• Lists of users (the "user list" component found in several places including "New DM", and the autocomplete for @-mentions) include the unknown users and shouldn't. We can fix that once a planned server feature is added.
• Several places (profile screen, "See who reacted" screen, also the "user list" component though as mentioned above we'll be able to make this not arise there) show a blank instead of the intended avatar, while other places successfully show it. This calls for some debugging.
• Sending a DM to an unknown user, or a group including one, would ideally have a clearer error behavior. But once it's possible to make that other fix, it'll be hard to even reach this point, so not worth working on.

[gnprice]

• Several places […] show a blank instead of the intended avatar, while other places successfully show it. This calls for some debugging.

Aha, here's the issue:

The "unknown user" avatar doesn't exist on the server in all the places that a normal avatar image would.

• It's present at /static/images/unknown-user-avatar.png.
• But there's nothing at /static/images/unknown-user-avatar-medium.png.
• For other kinds of avatars that are specified by URL and the URL isn't on the Gravatar domain, there's a -medium variant as well as the image at the base URL. So that's what the code in existing mobile clients looks for when it needs something bigger than 100x100 physical pixels.

I'll follow up on the #api design thread.

[chrisbobbe]

I tested and confirmed that the avatar issue (zulip/zulip#28071) has been fixed. So from Greg's message above, I think the following is the only remaining item, and fixing it would close this issue?

• Lists of users (the "user list" component found in several places including "New DM", and the autocomplete for @-mentions) include the unknown users and shouldn't. We can fix that once a planned server feature is added.

[sahil839]

965869d3f8e added client capability to not receive data for unknown users.

[gnprice]

Thanks @chrisbobbe and @sahil839 for those updates! I've filed #5808 for the followup based on those. The compatibility is now thoroughly checked, so closing this issue.