Closed alya closed 10 months ago
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 https://github.com/zulip/zulip/pull/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.
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.
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:
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.tryGetUserForId
(the non-throwing version of getUserForId
, and are doing something reasonable if they don't find the user.Awesome, thank you for the investigation and the detailed notes!
@sahil839 FYI
OK, with https://github.com/zulip/zulip/pull/27544 and then https://github.com/zulip/zulip/pull/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.
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.
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:
- 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.
/static/images/unknown-user-avatar.png
./static/images/unknown-user-avatar-medium.png
.-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.
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.
965869d3f8e added client capability to not receive data for unknown users.
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.
We should make sure the mobile app is compatible with the changes being made in zulip/zulip#10970.