Show "(guest)" by guest users' names, when setting enabled #5809

chrisbobbe · 2023-12-22T20:25:51Z

Stacked on my current revision for #5806.

I've put "(guest)" in italics where it's rendered with React components, since it was possible and convenient to do it there.

DM compose content input placeholder text before / after:

Before	After

Other "after" screenshots:

Fixes: #5804

chrisbobbe · 2023-12-22T20:33:53Z

Here's a before / after for this commit. With, in both, an uncommitted patch to declare the user_list_incomplete client capability (see #5808):

CustomProfileFields: Use styled UserItem for (unknown user)

Before	After

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 · 2023-12-22T20:46:08Z

Oh, and a whole-PR before-and-after showing the changes to AvatarItem, i.e., the avatar with an overlaid slashed-circle "remove" button, that shows you which users you've selected in the new-group-DM form. In particular this shows the effect of this commit:

AvatarItem: Stop carelessly amputating parts of people's names

Before	After

gnprice

Thanks @chrisbobbe for building this! I agree with the strategy of starting by centralizing how we compute the name text. This looks to be quite thorough.

Several comments below, all of them small.

gnprice · 2023-12-29T00:29:13Z

src/webview/generateInboundEventEditSequence.js

+      const oldSubsequentMessage = oldElement.subsequentMessage;
+      const newSubsequentMessage = newElement.subsequentMessage;


Suggested change

const oldSubsequentMessage = oldElement.subsequentMessage;

const newSubsequentMessage = newElement.subsequentMessage;

const subsequentMessage = oldElement.subsequentMessage;

Right? From the condition above, we already know oldElement and newElement compare equal on a deep equality comparison, so just one subsequentMessage should cover it.

I think that simplifies some of the following logic, too.

gnprice · 2023-12-29T00:30:33Z

src/webview/generateInboundEventEditSequence.js

+        const oldMessageRecipients = pmUiRecipientsFromMessage(
+          oldSubsequentMessage,
+          oldBackgroundData.ownUser.user_id,
+        );
+        const newMessageRecipients = pmUiRecipientsFromMessage(
+          newSubsequentMessage,
+          newBackgroundData.ownUser.user_id,
+        );


In particular the self user ID shouldn't change either (that can be an invariant), so this is just one messageRecipients.

gnprice · 2023-12-29T00:43:11Z

src/api/modelTypes.js

-  // TODO(server-4.0): New in FL 59
-  +role?: Role,
+  +role: Role,


52f253a api [nfc]: Make UserOrBot.role required, since we require Server 4.0

20 files changed, 94 insertions(+), 180 deletions(-)

I think my ideal version of this change would have been several commits:

make role always present, propagate that as far as the types require;

switch various code to rely on that instead of the legacy properties, in one or several commits;

remove the legacy properties from the Redux state and the API, as either one or two commits.

But given that we hope to be done with this codebase soon, this is OK; no need to go back and split it now.

Ah OK, makes sense. Thanks for the feedback!

gnprice · 2023-12-29T00:46:43Z

src/reduxTypes.js

@@ -310,23 +310,14 @@ export type RealmState = {|
  +waitingPeriodThreshold: number,
  +allowEditHistory: boolean,
  +enableReadReceipts: boolean,
-  +emailAddressVisibility: EmailAddressVisibility,
+  +emailAddressVisibility: EmailAddressVisibility | null,
+  +enableGuestUserIndicator: boolean,


6307a8a realm state: Add enableGuestUserIndicator

This needs a dropCache migration.

gnprice · 2023-12-29T01:18:25Z

src/users/userSelectors.js

+          // This ESLint rule is very confused here -- it thinks
+          // getFullNameReactText is a React function component, and it
+          // wants its "props" (`args`) to be $ReadOnly. And for some reason
+          // it thinks this is the place to complain about it. Ignore.
+          // eslint-disable-next-line ft-flow/require-readonly-react-props
+          <ZulipText inheritColor inheritFontSize style={italicTextStyle}>


That is puzzling. But ideally the type of args should have all its properties read-only anyway — in particular, that makes it possible to pass an object where the type of user is more specific, like UserOrBot or User | null. This doesn't typically bother us because we almost always pass an object literal that's used only for the function call, and Flow has special behavior to take advantage of that fact, but it would show itself if e.g. we had some wrapper that took an args object of a more specific type.

I think in general the same reasoning pretty much always applies when we have a function taking as a parameter an object whose purpose is really to serve as a bundle of named parameters: those named parameters should be read-only properties of the object type.

It looks like in general we don't observe that rule in places where the lack of read-only markers hasn't yet happened to get in our way, e.g. in the functions found in src/action-sheets/index.js. But since that lack is getting mildly in our way here thanks to this buggy lint rule, probably the simplest fix is to just add the + signs rather than need this comment explaining why we're ignoring the rule.

gnprice · 2023-12-29T01:29:42Z

src/compose/ComposeBox.js

-  const placeholder = getComposeInputPlaceholder(narrow, ownUserId, allUsersById, streamsById);
+  const placeholder = getComposeInputPlaceholder(
+    narrow,
+    ownUserId,
+    allUsersById,
+    streamsById,
+    enableGuestUserIndicator,
+    _,
+  );


This parameter list has gotten increasingly ridiculous and should probably become getComposeInputPlaceholder(narrow, backgroundData). But shrug probably not worth changing in this legacy codebase.

gnprice · 2023-12-29T01:33:59Z

src/webview/html/header.js

+    .map(r => {
+      const user = allUsersById.get(r.id);
+      return user != null
+        ? _(getFullNameOrMutedUserText({ user, mutedUsers, enableGuestUserIndicator }))


Suggested change

? _(getFullNameOrMutedUserText({ user, mutedUsers, enableGuestUserIndicator }))

// TODO use italics for "(guest)"

? _(getFullNameOrMutedUserText({ user, mutedUsers, enableGuestUserIndicator }))

? Same as for the other two appearances in HTML, in message.js below.

gnprice · 2023-12-29T01:50:23Z

src/lightbox/LightboxHeader.js

+          <ZulipText style={styles.name} numberOfLines={1}>
+            {sender != null ? (
+              <ZulipTextIntl
+                inheritColor
+                inheritFontSize


It looks like this outer ZulipText has just one child, which is itself either a ZulipTextIntl or a ZulipText. Can the outer ZulipText be merged into the inner one? Like this:

{sender != null ? ( <ZulipTextIntl style={styles.name} numberOfLines={1} text={getFullNameReactText({ user: sender, enableGuestUserIndicator })} /> ) : ( <ZulipText style={styles.name} numberOfLines={1} text={message.sender_full_name} /> )}

(I'll include that in the extra commits I push, since it simplifies the main added commit. But this part probably is best squashed into the relevant change earlier in the branch.)

gnprice · 2023-12-29T02:54:32Z

src/webview/html/header.js

+      return user != null
+        ? _(getFullNameOrMutedUserText({ user, mutedUsers, enableGuestUserIndicator }))
+        : r.full_name;


This conditional logic is tightly related to what happens inside getFullNameOrMutedUserText, so I think it'd be cleaner for it to happen inside those helpers.

I'll push a commit on top that makes that refactor. (Actually two commits, starting with a cross-cutting prep commit.) PTAL; no need to squash the change in, though, it's fine for it to happen at the end.

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 · 2024-01-04T01:03:34Z

Thanks for the review! Revision pushed, and I fixed the failing tests. I also added a TODO in a commit at the end for a place we don't yet have "(guest)" indicators: @-mentions in message content. Hmm.

But if it's not in user data, just fall back to the user's name on the message. Soon we'll use the user data to show a "(guest)" marker if it applies; this is zulip#5804. The header doesn't actually live-update when a recipient user's name changes (zulip#5208), because we don't yet use InboundEventEditSequence. But I've added logic in the code that builds that inbound event, so that the live-updating will happen if we ever do start using it. Like we did for the followed-topic marker on stream recipient headers, in 4ef0a0a.

…able But if it's not in user data, just fall back to sender_full_name on the message, since that seems better than showing "(unknown user)". Like in the previous commit, the main motivation for this is that our new helper functions that will power the upcoming "Firstname Lastname (guest)" feature, zulip#5804, need a User object. In the tests for the "content" WebView inbound event, tweak the example message so that its sender is the self-user. That's so the tested code can access the sender's User object, since the self-user is already included in the test's background data.

But if it's not in user data, just fall back to the user's name on the message. Soon we'll use the user data to show a "(guest)" marker if it applies; this is zulip#5804. But as a bonus, this means that as long as we have a User object (which we usually will), we'll live-update the sender's name, here, if it changes.

gnprice · 2024-01-04T01:39:49Z

Thanks! All LGTM now, except there are some rebase conflicts to resolve.

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 · 2024-01-04T02:03:05Z

Thanks! Revision pushed, resolving those conflicts.

gnprice

Thanks! Looks good except one small glitch below. Please go ahead and merge after that.

gnprice · 2024-01-04T03:02:17Z

src/storage/__tests__/migrations-test.js

+  const base62 = {
    ...base52,
-    migrations: { version: 62 },


This migrations line should stay — it makes the description of this "base" value accurate, and it's what we do with the other "baseN" values.

And do some nice cleanups that this enables. See kMinAllowedServerVersion, which is 4.0. Now, we can implement the "guest indicator" feature (zulip#5804) without having to write unused fallback code for a missing `.role`.

There's plenty of room to show the user's whole name, even if it has to go on multiple lines.

…name I very much doubt we permit the empty string for a user's full_name field.

@tim

… web In web, the compose box for 1:1 DMs has a placeholder like "Message Tim Abbott", not "Message @tim Abbott". Update mobile to match.

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)

… file

I don't think the "{_}" ever comes up when we run our tests -- this codebase is extremely light on UI tests -- but it's about to, and in such a way that it'll be nice if our mock handled it.

… user" Like we do in messages in the message list, and items in the inbox and in the recent DM conversations screen.

…n Text NFC because the wrapper's behavior of setting `color` and `fontSize` is overridden by those attributes being specified in the `style` prop in both cases.

…props Thankfully we have a convenient value of this type. Now, when the code under test starts using the value, soon, it won't run into a runtime type error.

Making sure that it chooses '(unknown user)' or 'Muted user', as appropriate. We're about to add another of that kind of thing -- 'Firstname Lastname (guest)' -- for zulip#5804.

Soon we'll have a version of the "full name or muted user text" that's LocalizableReactText, not LocalizableText, because we want to offer an italicized "(guest)" marker, for zulip#5804. Prepare for that by using ZulipTextIntl for the names, since it can handle LocalizableReactText, and our GetText function `_` cannot.

We've so far hesitated to convert this to a function component because of its animation logic, which would take a bit of thought to convert correctly. But we want to use `useSelector`, and for that it needs to be a function component. So, get rid of the animation. A small sacrifice in this page, which has really long loading times and pretty extreme jank when you tap a user.

…g on Fixes: zulip#5804

…d friends

…kers A quick test showed that the web app does this, so we should to. It would be kind of annoying to get it to work here, so it might be right to just not bother in this legacy codebase, and be sure to do it in zulip-flutter.

chrisbobbe · 2024-01-05T04:48:09Z

Thanks! Done.

chrisbobbe force-pushed the pr-guest-indicator branch from f77b836 to 01c538c Compare December 22, 2023 20:36

chrisbobbe requested a review from gnprice December 22, 2023 20:38

chrisbobbe force-pushed the pr-guest-indicator branch from 01c538c to c46625f Compare December 27, 2023 20:01

This comment was marked as off-topic.

Sign in to view

chrisbobbe force-pushed the pr-guest-indicator branch from c46625f to 01c538c Compare December 27, 2023 20:02

This comment was marked as resolved.

Sign in to view

gnprice reviewed Dec 29, 2023

View reviewed changes

chrisbobbe force-pushed the pr-guest-indicator branch from d3f2447 to 8742cbc Compare January 4, 2024 01:02

chrisbobbe added 3 commits January 3, 2024 17:12

chrisbobbe force-pushed the pr-guest-indicator branch from 8742cbc to 8768e61 Compare January 4, 2024 02:02

gnprice reviewed Jan 4, 2024

View reviewed changes

chrisbobbe added 9 commits January 4, 2024 20:45

realm state: Add enableGuestUserIndicator

b032519

AvatarItem: Stop carelessly amputating parts of people's names

a4a26f1

There's plenty of room to show the user's whole name, even if it has to go on multiple lines.

AccountDetailsScreen: Remove unneeded fallback for empty-string full_…

d33f134

…name I very much doubt we permit the empty string for a user's full_name field.

compose: In content-input placeholder, remove @ for 1:1 DMs, to match…

f35cc29

… web In web, the compose box for 1:1 DMs has a placeholder like "Message Tim Abbott", not "Message @tim Abbott". Update mobile to match.

test [nfc]: Pull out mock_, to share with another caller in different…

9b46739

… file

test: Have our mock GetText mock_ handle our special "{_}" message

17eb634

I don't think the "{_}" ever comes up when we run our tests -- this codebase is extremely light on UI tests -- but it's about to, and in such a way that it'll be nice if our mock handled it.

msglist: Respect muted users in DM recipient headers by saying "Muted…

7cc4d9f

… user" Like we do in messages in the message list, and items in the inbox and in the recent DM conversations screen.

chrisbobbe and others added 10 commits January 4, 2024 20:45

LightboxHeader [nfc]: Use our usual ZulipText wrapper instead of plai…

47c2d94

…n Text NFC because the wrapper's behavior of setting `color` and `fontSize` is overridden by those attributes being specified in the `style` prop in both cases.

generateInboundEvents tests: Fix type of our _ GetText function in …

889827b

…props Thankfully we have a convenient value of this type. Now, when the code under test starts using the value, soon, it won't run into a runtime type error.

ui [nfc]: Centralize how we choose display text for a user's name

1c0266d

Making sure that it chooses '(unknown user)' or 'Muted user', as appropriate. We're about to add another of that kind of thing -- 'Firstname Lastname (guest)' -- for zulip#5804.

AvatarItem [nfc]: Convert to a function component

ee39d54

ui: Show "(guest)" indicator on guest users' names, when realm settin…

9171d7d

…g on Fixes: zulip#5804

i18n [nfc]: Factor out noTranslation helper

0c29df4

user [nfc]: Handle name fallback to message within getFullNameText an…

eb3fb95

…d friends

chrisbobbe force-pushed the pr-guest-indicator branch from 8768e61 to e729dbc Compare January 5, 2024 04:47

chrisbobbe merged commit e729dbc into zulip:main Jan 5, 2024
1 check passed

chrisbobbe deleted the pr-guest-indicator branch January 5, 2024 04:48

neiljp mentioned this pull request Jan 6, 2024

Add guest user suffix, depending on organization setting zulip/zulip-terminal#1444

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show "(guest)" by guest users' names, when setting enabled #5809

Show "(guest)" by guest users' names, when setting enabled #5809

chrisbobbe commented Dec 22, 2023 •

edited

Loading

chrisbobbe commented Dec 22, 2023

chrisbobbe commented Dec 22, 2023

This comment was marked as off-topic.

This comment was marked as resolved.

gnprice left a comment

gnprice Dec 29, 2023

gnprice Dec 29, 2023

gnprice Dec 29, 2023

chrisbobbe Jan 3, 2024

gnprice Dec 29, 2023

gnprice Dec 29, 2023

gnprice Dec 29, 2023

chrisbobbe Jan 3, 2024

gnprice Dec 29, 2023

gnprice Dec 29, 2023

gnprice Dec 29, 2023

gnprice Dec 29, 2023

chrisbobbe commented Jan 4, 2024

gnprice commented Jan 4, 2024

chrisbobbe commented Jan 4, 2024

gnprice left a comment

gnprice Jan 4, 2024

chrisbobbe commented Jan 5, 2024

		const oldSubsequentMessage = oldElement.subsequentMessage;
		const newSubsequentMessage = newElement.subsequentMessage;

	const oldSubsequentMessage = oldElement.subsequentMessage;
	const newSubsequentMessage = newElement.subsequentMessage;
	const subsequentMessage = oldElement.subsequentMessage;

	? _(getFullNameOrMutedUserText({ user, mutedUsers, enableGuestUserIndicator }))
	// TODO use italics for "(guest)"
	? _(getFullNameOrMutedUserText({ user, mutedUsers, enableGuestUserIndicator }))

Show "(guest)" by guest users' names, when setting enabled #5809

Show "(guest)" by guest users' names, when setting enabled #5809

Conversation

chrisbobbe commented Dec 22, 2023 • edited Loading

DM compose content input placeholder text before / after:

Other "after" screenshots:

chrisbobbe commented Dec 22, 2023

chrisbobbe commented Dec 22, 2023

This comment was marked as off-topic.

This comment was marked as resolved.

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 4, 2024

gnprice commented Jan 4, 2024

chrisbobbe commented Jan 4, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jan 5, 2024

chrisbobbe commented Dec 22, 2023 •

edited

Loading