handle unknown users everywhere #1012

mofirojean · 2024-10-19T09:05:11Z

As described in the issue, I was able to reference the various instances of the store.users[userId] and most of the reference cases found had the null cases handled already, and what was left to do was to include a placeholder to adhere to Zulip's internalisation standard. Most of this could be traced to other instances that followed a similar pattern. I also performed a test to make sure nothing was broken while I implemented the changes.

fixes: #716

chrisbobbe · 2024-10-21T23:05:01Z

@PIG208 could you take this one? :)

PIG208 · 2024-10-22T19:15:26Z

Hi! Please take a look at the commit-discipline to fix the commit messages. Per this suggestion, I think our goal for this PR is to fix part of #716, before going over all the places that need to be updated.

PIG208

I think the second commit can be squashed into the first one.

One other thing to note is that you can use the NFC tag for your commits if they don't affect the behavior.

This is a change that we would like to have but it is not addressing #716 by finding places where we don't have unknown users. Nevertheless, this should be a good checkpoint before we start working on a PR.

PIG208 · 2024-10-22T19:19:45Z

lib/widgets/inbox.dart


      // TODO(i18n): List formatting, like you can do in JavaScript:
      //   new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Shu'])
      //   // 'Chris、Greg、Alya、Shu'
-      _ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)').join(', '),
+      _ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName).join(', '),


This line was already quite long:

Suggested change

_ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName).join(', '),

_ => narrow.otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName,

).join(', '),

I have updated the code with the suggestions you gave, and also joined the two commits to one.

Moving next to implement the issue at hand.

PIG208

The [nfc] tag is usually used as a suffix. Here are some examples:

402f998 color [nfc]: Fix a straggling reference to Color.value, in a pigeon's doc
2737d4c store [nfc]: Comment in _PerAccountStoreWidgetState.didChangeDependencies
26069bd store [nfc]: Add indirection doLoadPerAccount
fc80be1 store [nfc]: Offer singleton global store on binding

So we should update [nfc]: handle unknown users everywhere to maybe i18n [nfc]: Handle unknown users everywhere (also note the capitalization change).

PIG208 · 2024-10-24T16:21:01Z

lib/widgets/inbox.dart

-      _ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)').join(', '),
+      _ => narrow.otherRecipientIds.map((id) =>
+        store.users[id]?.fullName ?? zulipLocalizations.unknownUserName
+      ).join(', '),


Looks like the updated code is not properly indented. Could you check again?

Okay, i will do that right away

PIG208 · 2024-10-24T16:21:42Z

lib/widgets/message_list.dart

@@ -322,6 +322,8 @@ class MessageListAppBarTitle extends StatelessWidget {
  Widget build(BuildContext context) {
    final zulipLocalizations = ZulipLocalizations.of(context);

+    // TODO(i18n): provide tranlations just as 'zulipLocalizations.unknownUserName'
+    // for 'unknown channel'


nit: tranlations -> translations

It would be better to move this comment to right before any of the lines containing (unknown channel), so it is easier to spot where the translation is applicable.

Okay 👍 , just noticed i didn't spell that right

PIG208 · 2024-10-24T16:24:03Z

lib/widgets/message_list.dart

@@ -349,7 +351,7 @@ class MessageListAppBarTitle extends StatelessWidget {
        if (otherRecipientIds.isEmpty) {
          return const Text("DMs with yourself");
        } else {
-          final names = otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)');
+          final names = otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName);


nit:

Suggested change

final names = otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName);

final names = otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName);

PIG208 · 2024-10-24T16:28:01Z

lib/widgets/recent_dm_conversations.dart

        avatar = AvatarImage(userId: otherUserId, size: _avatarSize);
      default:
        // TODO(i18n): List formatting, like you can do in JavaScript:
        //   new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya'])
        //   // 'Chris、Greg、Alya'
-        title = narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)').join(', ');
+        title = narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName).join(', ');


nit:

Suggested change

title = narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName).join(', ');

title = narrow.otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

).join(', ');

mofirojean · 2024-10-28T07:07:02Z

Hi @PIG208 , I updated the changes you requested. I have some questions with regards to the issue. As I understand it, the app has a map called PerAccountStore.users (also referred to as store.users), which holds information about all known users, and sometimes this list may not include all the users in the realm. Also, in some cases, when you try to look up a user by their userId in store.users, the result might be null (i.e., that user doesn’t exist in the map) and this may result in a crash.

Amongst the solutions, it was said not to use ! or assertions and use the dart's null-safety operator and then provide fallbacks for missing users, along with other solutions. I checked the codebase with the reference store.users and identified a few areas that match the description of the issue.

for the first case i worked on adding the placeholders and changing some translations as well.

I then saw this area with an assertion, models/compose.dart

String quoteAndReplyPlaceholder(PerAccountStore store, {
  required Message message,
}) {
  final sender = store.users[message.senderId];
  // Is this the area i should resolve
  assert(sender != null);

  final url = narrowLink(store,
    SendableNarrow.ofMessage(message, selfUserId: store.selfUserId),
    nearMessageId: message.id);
  ....
}

Is this an area, that requires the changes that address our issue?

PIG208

Thanks for the revision! Just one more comment from me.

Regarding the question on quoteAndReplyPlaceholder, I think that one in particular is one of the places we have access to the message that contains the sender information. #716 mentioned that we should fallback to that if the user is unknown.

When we have a context like a Message that provides its own details on a user (like the message sender), we should fall back to those if the user is unknown. That way we not only don't crash, but provide helpful information, in cases like a guest user looking at older messages sent by someone no longer subscribed to their streams.

PIG208 · 2024-10-29T20:34:51Z

lib/widgets/inbox.dart

+      _ => narrow.otherRecipientIds.map((id) =>
+              store.users[id]?.fullName ?? zulipLocalizations.unknownUserName
+           ).join(', '),


nit: use 2-space indentation

Suggested change

_ => narrow.otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

).join(', '),

_ => narrow.otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

).join(', '),

Okay, thanks for pointing that out
Let me make the changes

PIG208 · 2024-10-29T21:22:04Z

We should also update the commit summary, because we haven' handled unknown users everywhere yet.

mofirojean · 2024-10-29T21:41:51Z

We should also update the commit summary, because we haven' handled unknown users everywhere yet.

About this, I checked other areas that didn't handle the null cases and it seems to me that they have been handled to resolve unknown users. Unless i might be confuse with what is required of me.

PIG208 · 2024-10-29T21:45:17Z

In this change we only start to use the translation string, but there are some other cases where we do not handle unknown users yet.

PIG208 · 2024-10-29T21:47:10Z

lib/widgets/inbox.dart


      // TODO(i18n): List formatting, like you can do in JavaScript:
      //   new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Shu'])
      //   // 'Chris、Greg、Alya、Shu'
-      _ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? '(unknown user)').join(', '),
+      _ => narrow.otherRecipientIds.map((id) =>
+            store.users[id]?.fullName ?? zulipLocalizations.unknownUserName


bump: we use 2 spaces for indentation

I am using android studio as my IDE and i configured it to use 2 spaces, and the changes I provided used this 2 spaces for indentation

I see. That should work for most cases automatically, but looks like at the line (started with store.users[id]?.fullName) I commented, there is only one space indented from the text it aligns to (started with narrow.otherRecipientIds.map) in the previous line.

You can count the number of spaces at the column where the n from narrow.otherRecipientIds.map is at, at line store.users[id]?.fullName to see what I mean.

I would suggest the following change:

before:

_ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

after:

_ => narrow.otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

I was able to add the changes as requested and i updated the PR description.

mofirojean · 2024-10-29T22:03:24Z

In this change we only start to use the translation string, but there are some other cases where we do not handle unknown users yet.

Okay, I get it now. Please can you look this code from the widget/content.dart, which i referenced using store.users[userId],

class AvatarImage extends StatelessWidget {
  ...
  @override
  Widget build(BuildContext context) {
    final store = PerAccountStoreWidget.of(context);
    final user = store.users[userId];
   
   /// In this case we have a condition to check if the user is null, does this mean
   ///  i need to add more logic to this or it handles the case as the issue describes ?
    if (user == null) { // TODO(log)
      return const SizedBox.shrink();
    }
  ...
  }
}

PIG208 · 2024-10-31T20:34:44Z

Thanks for the update! For the question on widget/content.dart, I think in that case the unknown users have been taken care of.

We should also update the commit summary, because we haven' handled unknown users everywhere yet.

We have been focusing on using the translation strings for this PR/commit, so the summary should reflect that.

PIG208 · 2024-11-06T16:16:05Z

Looks good overall! This message i18n [nfc]: Handle unknown users everywhere doesn't seem to reflect on what this is doing though, because this commit is mostly about switching to localized strings.

How about something like i18n: Use translation for "(unknown users)"?
(Looking at the change, I realized that this is not an nfc because this is a behavior change to support languages other than English)

mofirojean · 2024-11-07T06:08:42Z

Looks good overall! This message i18n [nfc]: Handle unknown users everywhere doesn't seem to reflect on what this is doing, though, because this commit is mostly about switching to localized strings.

How about something like i18n: Use translation for "(unknown users)"?

Yes that true, let me make the changes

PIG208 · 2024-11-07T15:26:07Z

Thanks! This looks good to me.

gnprice

Thanks @mofirojean for taking care of this, and @PIG208 for the previous reviews!

Generally this looks good. A few small comments below.

The PR description also needs to be fixed (just like the commit message was earlier) so it doesn't say this will fix #716.

gnprice · 2024-11-19T05:32:36Z

lib/widgets/emoji_reaction.dart

@@ -157,7 +159,7 @@ class ReactionChip extends StatelessWidget {
      ? userIds.map((id) {
          return id == store.selfUserId
            ? 'You'
-            : store.users[id]?.fullName ?? '(unknown user)'; // TODO(i18n)


nit: commit message doesn't quite match actual string:

i18n: Use translation for "(unknown users)"

gnprice · 2024-11-19T05:33:05Z

lib/widgets/message_list.dart

+        // TODO(i18n): provide translations for 'unknown channel'
        final streamName = stream?.name ?? '(unknown channel)';


nit: it's just one translation

Suggested change

// TODO(i18n): provide translations for 'unknown channel'

final streamName = stream?.name ?? '(unknown channel)';

// TODO(i18n): provide translation for 'unknown channel'

final streamName = stream?.name ?? '(unknown channel)';

gnprice · 2024-11-19T05:33:32Z

lib/widgets/message_list.dart

+        // TODO(i18n): provide translations just as
+        //  'zulipLocalizations.unknownUserName' for 'unknown channel'


nit: simplify comment

Suggested change

// TODO(i18n): provide translations just as

// 'zulipLocalizations.unknownUserName' for 'unknown channel'

// TODO(i18n): provide translation for 'unknown channel'

gnprice · 2024-11-19T05:34:28Z

lib/widgets/recent_dm_conversations.dart

+        title = narrow.otherRecipientIds.map((id) =>
+            store.users[id]?.fullName ?? zulipLocalizations.unknownUserName
+          ).join(', ');


nit: indentation

Suggested change

title = narrow.otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

).join(', ');

title = narrow.otherRecipientIds.map((id) =>

store.users[id]?.fullName ?? zulipLocalizations.unknownUserName

).join(', ');

gnprice added the maintainer review PR ready for review by Zulip maintainers label Oct 19, 2024

chrisbobbe assigned PIG208 Oct 21, 2024

PIG208 reviewed Oct 22, 2024

View reviewed changes

mofirojean force-pushed the Handle-unknown-users-everywhere branch from d5f8afe to cc5c85c Compare October 23, 2024 21:25

PIG208 reviewed Oct 24, 2024

View reviewed changes

mofirojean force-pushed the Handle-unknown-users-everywhere branch from cc5c85c to 9a36ab5 Compare October 28, 2024 06:29

mofirojean requested a review from PIG208 October 28, 2024 07:08

PIG208 reviewed Oct 29, 2024

View reviewed changes

mofirojean force-pushed the Handle-unknown-users-everywhere branch from 9a36ab5 to ea6a44b Compare October 29, 2024 21:35

PIG208 reviewed Oct 29, 2024

View reviewed changes

mofirojean force-pushed the Handle-unknown-users-everywhere branch from ea6a44b to a6a0be0 Compare November 6, 2024 06:27

mofirojean requested a review from PIG208 November 6, 2024 06:27

mofirojean force-pushed the Handle-unknown-users-everywhere branch from a6a0be0 to c3de2bb Compare November 7, 2024 11:50

i18n: Use translation for "(unknown users)"

12a4cb6

mofirojean force-pushed the Handle-unknown-users-everywhere branch from c3de2bb to 12a4cb6 Compare November 7, 2024 11:54

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Nov 7, 2024

PIG208 assigned gnprice and unassigned PIG208 Nov 7, 2024

PIG208 removed their request for review November 7, 2024 15:25

PIG208 requested a review from gnprice November 7, 2024 15:25

gnprice reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle unknown users everywhere #1012

handle unknown users everywhere #1012

mofirojean commented Oct 19, 2024 •

edited

Loading

chrisbobbe commented Oct 21, 2024

PIG208 commented Oct 22, 2024

PIG208 left a comment

PIG208 Oct 22, 2024

mofirojean Oct 23, 2024

PIG208 left a comment

PIG208 Oct 24, 2024

mofirojean Oct 28, 2024

PIG208 Oct 24, 2024

PIG208 Oct 24, 2024

mofirojean Oct 28, 2024

PIG208 Oct 24, 2024

PIG208 Oct 24, 2024

mofirojean commented Oct 28, 2024

PIG208 left a comment

PIG208 Oct 29, 2024

mofirojean Oct 29, 2024

PIG208 commented Oct 29, 2024 •

edited

Loading

mofirojean commented Oct 29, 2024

PIG208 commented Oct 29, 2024

PIG208 Oct 29, 2024

mofirojean Oct 29, 2024

PIG208 Oct 31, 2024

mofirojean Nov 6, 2024

mofirojean commented Oct 29, 2024

PIG208 commented Oct 31, 2024 •

edited

Loading

PIG208 commented Nov 6, 2024

mofirojean commented Nov 7, 2024

PIG208 commented Nov 7, 2024

gnprice left a comment

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

gnprice Nov 19, 2024

	final names = otherRecipientIds.map((id) => store.users[id]?.fullName ?? zulipLocalizations.unknownUserName);
	final names = otherRecipientIds.map((id) =>
	store.users[id]?.fullName ?? zulipLocalizations.unknownUserName);

		// TODO(i18n): provide translations for 'unknown channel'
		final streamName = stream?.name ?? '(unknown channel)';

		// TODO(i18n): provide translations just as
		// 'zulipLocalizations.unknownUserName' for 'unknown channel'

	// TODO(i18n): provide translations just as
	// 'zulipLocalizations.unknownUserName' for 'unknown channel'
	// TODO(i18n): provide translation for 'unknown channel'

handle unknown users everywhere #1012

Are you sure you want to change the base?

handle unknown users everywhere #1012

Conversation

mofirojean commented Oct 19, 2024 • edited Loading

chrisbobbe commented Oct 21, 2024

PIG208 commented Oct 22, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 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

mofirojean commented Oct 28, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Oct 29, 2024 • edited Loading

mofirojean commented Oct 29, 2024

PIG208 commented Oct 29, 2024

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

mofirojean commented Oct 29, 2024

PIG208 commented Oct 31, 2024 • edited Loading

PIG208 commented Nov 6, 2024

mofirojean commented Nov 7, 2024

PIG208 commented Nov 7, 2024

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

mofirojean commented Oct 19, 2024 •

edited

Loading

PIG208 commented Oct 29, 2024 •

edited

Loading

PIG208 commented Oct 31, 2024 •

edited

Loading