Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle unknown users on profile and reactions-list screens #5790

Merged
merged 6 commits into from
Nov 15, 2023
Merged

Handle unknown users on profile and reactions-list screens #5790

merged 6 commits into from
Nov 15, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Nov 10, 2023

This fixes the issues I found just now when investigating #5787, our compatibility with the upcoming "limited guests" feature. Specifically:

  • If you navigate to the profile screen of an unknown user — say because they're the sender of a message you can see, perhaps while they were in a stream they've since left — we would crash. Instead, show an error.
  • If you choose "See who reacted" on a message where an unknown user reacted — perhaps one who's since left the stream, for example — we would crash. Instead, call them "(unknown user)" with a placeholder avatar.

@gnprice
user: Handle unknown user in AccountDetailsScreen
80cf681 
This is modeled on what we do in zulip-flutter (at ProfilePage),
but omitting the icon for expedience in this legacy codebase.
@gnprice
icon [nfc]: Generalize the name IconUserBlank, from IconUserMuted
a240ddb 
The icon itself doesn't convey anything specific about "muted",
and we'll use it in another case shortly.
@gnprice
user [nfc]: Rearrange UserAvatar logic a bit
64a21ac 
Making this into normal statement control flow, rather than
a `?:` operator, gives it more room to comfortably expand.
@gnprice
user: Render placeholder in UserAvatar for unknown user
50afec8
@gnprice
user: Handle unknown user in UserItem
5cde73f
@gnprice
user [nfc]: Document the new preference for tryGetUserForId over thro…
22672cd 
…wing
@gnprice gnprice added P1 high-priority webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. server release goal Things we should try to coordinate with a major Zulip Server release. labels Nov 10, 2023
@gnprice gnprice mentioned this pull request Nov 10, 2023
Closed
chrisbobbe
chrisbobbe approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@chrisbobbe chrisbobbe left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One nit, then please merge at will. Merging.

src/users/UserItem.js
let displayName;
let displayEmail;
if (!user) {
displayName = _('(unknown user)');
Copy link
Contributor

@chrisbobbe chrisbobbe Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new UI string; we should add it to messages_en.json.

Ah, and I see it wasn't added to messages_en.json in this PR for the simple reason that it's already there :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I actually did initially add it there, and then got a warning about a duplicate key :)

@chrisbobbe chrisbobbe merged commit b2919cd into zulip:main Nov 15, 2023
1 check passed
@gnprice gnprice deleted the pr-unknown-user branch November 15, 2023 04:46
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Dec 22, 2023
@chrisbobbe
CustomProfileFields: Use styled UserItem for (unknown user)
be17af8 
Since zulip#5790 last month, UserItem has been gracefully handling users
we don't know about. Use that, instead of special-casing with a
plain ZulipTextIntl, which we'd been doing since 2022-06 (0a10831)
when CustomProfileFields was added.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Dec 22, 2023
@chrisbobbe
CustomProfileFields: Use styled UserItem for (unknown user)
690c1ff 
Since zulip#5790 last month, UserItem has been prepared to gracefully
handle users we don't know about. Use that, instead of
special-casing with a plain ZulipTextIntl, which we'd been doing
since 2022-06 (0a10831) when CustomProfileFields was added.

The change won't be visible unless a given user object is actually
absent; that'll start happening when we do zulip#5808.

See screenshots:
  zulip#5809 (comment)
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 4, 2024
@chrisbobbe
CustomProfileFields: Use styled UserItem for (unknown user)
930d9d7 
Since zulip#5790 last month, UserItem has been prepared to gracefully
handle users we don't know about. Use that, instead of
special-casing with a plain ZulipTextIntl, which we'd been doing
since 2022-06 (0a10831) when CustomProfileFields was added.

The change won't be visible unless a given user object is actually
absent; that'll start happening when we do zulip#5808.

See screenshots:
  zulip#5809 (comment)
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 4, 2024
@chrisbobbe
CustomProfileFields: Use styled UserItem for (unknown user)
6826517 
Since zulip#5790 last month, UserItem has been prepared to gracefully
handle users we don't know about. Use that, instead of
special-casing with a plain ZulipTextIntl, which we'd been doing
since 2022-06 (0a10831) when CustomProfileFields was added.

The change won't be visible unless a given user object is actually
absent; that'll start happening when we do zulip#5808.

See screenshots:
  zulip#5809 (comment)
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 5, 2024
@chrisbobbe
CustomProfileFields: Use styled UserItem for (unknown user)
2273d56 
Since zulip#5790 last month, UserItem has been prepared to gracefully
handle users we don't know about. Use that, instead of
special-casing with a plain ZulipTextIntl, which we'd been doing
since 2022-06 (0a10831) when CustomProfileFields was added.

The change won't be visible unless a given user object is actually
absent; that'll start happening when we do zulip#5808.

See screenshots:
  zulip#5809 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisbobbe chrisbobbe chrisbobbe approved these changes

Assignees
No one assigned
Labels
P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gnprice @chrisbobbe