compose_box: Replace compose box with a banner in DMs with deactivated users #816

sm-sayedi · 2024-07-16T19:02:04Z

When DMs have one or multiple deactivated users, all parts of the compose box are replaced with a banner, saying: You cannot send messages to deactivated users.

DMs with active user	DMs with deactivated user	DMs with deactivated user - Dark

dms.with.deactivated.users.mp4

Note: This work is the result of a productive pair programming session with @rajveermalviya.

Fixes: #675

Khader-1

Thanks @sm-sayedi! All looks good, I have one suggestion below PTAL. Over to mentor review with @hackerkid now.

Khader-1 · 2024-07-17T09:13:05Z

lib/widgets/compose_box.dart

+    if (!enabled) {
+      controller.clear();
+    }


optional nit

Suggested change

if (!enabled) {

controller.clear();

}

if (!enabled) controller.clear();

alya · 2024-07-17T22:02:28Z

The text in the screenshot is slightly different from the web app:

I don't have strong feelings on what's better, but we might as well keep it consistent. I would default to just using the web app text, unless others think this is an improvement. If it turns out that folks like this better, we'd want to tweak it in the web app.

gnprice · 2024-07-18T04:45:33Z

lib/widgets/compose_box.dart


  @override
  State<_SendButton> createState() => _SendButtonState();
 }

 class _SendButtonState extends State<_SendButton> {
  void _hasErrorsChanged() {
-    setState(() {
-      // Update disabled/non-disabled state
+    WidgetsBinding.instance.addPostFrameCallback((_) {


I spotted this and wondered about it, and then happened to see something similar in another PR today and looked into it. See thread here:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/addPostFrameCallback/near/1893536

sm-sayedi · 2024-07-18T05:06:43Z

The text in the screenshot is slightly different from the web app:

The image provided shows the text message for a 1:1 conversation; for a group conversation, it is the same as the web app. We thought it would be slightly better to distinguish between 1 vs. many deactivated users.

alya · 2024-07-18T18:22:43Z

I see. I would just use the plural everywhere. It's stated as a rule, and doesn't imply that there are specifically many users in the current conversation.

alya · 2024-07-18T18:23:24Z

Also, as a future note, it would have been very helpful to explicitly flag this decision for discussion in the PR description.

sm-sayedi · 2024-07-18T23:50:10Z

Revision pushed with PR description images updated.

alya · 2024-07-19T18:13:55Z

Looks good to me, thanks!

gnprice

Thanks @sm-sayedi and @rajveermalviya! Comments below.

Taking a step back, though, and this may make some of my detailed code comments moot: the current UI in this PR is that if you're in this error condition where you can't actually send a message, we show some error text saying so… but we do so in the form of a "hint" placeholder in the content input.

That doesn't seem like a conventional use of that piece of UI. For example the other analogous hints we put there are like "Message #general > ideas", not errors or warnings.

It also doesn't match what we do on web, where there's a red banner like Alya showed above.

How about instead:

When there's this error condition, we don't show the content input at all, or the send button, or the attach-files/etc. buttons.
Instead, there's a red banner. It'll look similar to the one on web, though unlike the one on web it will take the place of those other pieces of the compose box.
There'll still be the gray background for the compose box as a whole. The banner will sit inside that, similar to how the content input and the buttons etc. normally sit inside it.

A bonus is that I think that will simplify a lot of the code in this PR: fewer layers to thread the enabled/disabled information down through, and no need for the _contentController.clear call that made the data flow trickier as discussed at #816 (comment) .

gnprice · 2024-07-24T04:24:09Z

lib/api/model/events.dart

@@ -285,6 +285,7 @@ class RealmUserUpdateEvent extends RealmUserEvent {

  @JsonKey(readValue: _readFromPerson) final RealmUserUpdateCustomProfileField? customProfileField;
  @JsonKey(readValue: _readFromPerson) final String? newEmail;
+  @JsonKey(readValue: _readFromPerson) final bool? isActive;


Hmm, seems like a bug we weren't updating this before — good catch!

It looks like this PR is missing code to actually apply the update in handleEvent, though. So that should go in the same commit that adds this field to the event.

… Ah I see, the story is that this event didn't have this field until FL 222, in server-8:
https://zulip.com/api/get-events#realm_user-update

Instead it would send RealmUserAddEvent or RealmUserRemoveEvent. And those we handled already.

Anyway, the same comment about applying the update still applies.

It looks like this PR is missing code to actually apply the update in handleEvent, though.

Thanks for spotting. I was wondering why the tests for the previous revision were failing, and now I know why. When rebasing store [nfc]: Replace else-if's with switch for handleEvent, I forgot to include this part when resolving the conflicts. 😀

gnprice · 2024-07-24T04:43:54Z

lib/widgets/compose_box.dart

+    final store = PerAccountStoreWidget.of(context);
+    enabled = switch (widget.narrow) {
+      DmNarrow(:final otherRecipientIds) => otherRecipientIds.every((id) =>
+        store.users[id]?.isActive ?? true),


Let's move the PerAccountStoreWidget.of inside the DmNarrow case. (That probably means making it a switch statement instead of expression.)

That way when composing to a topic narrow, we don't unnecessarily create a dependency that causes this to get rebuilt on store updates.

gnprice · 2024-07-24T04:52:33Z

lib/widgets/compose_box.dart

-      case DmNarrow(): // A group DM thread.
+      case (DmNarrow(), true): // A group DM thread.


This logic with the switch on a pair gets kind of twisty to follow.

For example this comment no longer matches the code — this case is now only for a group DM thread where enabled is true.

And above, the self-1:1 thread is treated differently from other DM threads. Is that needed? The logic would be simpler if it were treated the same as other DM threads.

gnprice · 2024-07-24T05:01:02Z

lib/widgets/compose_box.dart

+      case (DmNarrow(), false):
+        return zulipLocalizations.composeBoxDeactivatedDmContentHint;


Here we're relying on an assumption that if enabled is false, it's because there's a deactivated user.

That's how the code currently works in this version, as of the tip of this PR. But it's not expressed in the name enabled, nor in any dartdoc on that field. So now that the field is added, it'd be very natural for someone in the future to add some other condition that causes enabled to be false; but that would produce a misleading message here.

One adequate fix would be to give the field a more specific name that matches the semantics this widget is attaching to it. For example, disabledBecauseDmRecipientDeactivated. Then this code can look for that flag, and return this specific error message when it's set, with a clear conscience.

A more complex fix would be to make an enum in the same spirit as TopicValidationError and ContentValidationError, with a name like DestinationError. It'd have just one value at first, named like DestinationError.dmRecipientDeactivated. Then the flag here would be declared like

final DestinationError? destinationError;

and null would mean there was no error.

The advantage of that more complex fix is that it'd be straightforward to add another value to the enum in the future, for a different error condition (like running afoul of the stream post policy, #674).

gnprice · 2024-07-24T05:19:24Z

test/widgets/message_list_test.dart

@@ -981,4 +983,222 @@ void main() {
      });
    });
  });
+
+  group('message list page for DMs with deactivated users', () {


Does the new logic interact with the message list? I don't think it does — it seems like it's all contained in the compose box.

In that case these tests should go in test/widgets/compose_box_test.dart instead, and should be able to use prepareComposeBox there (or something like it) without setting up a MessageListPage.

In general when adding code in a file lib/foo/bar.dart, we try to put the tests for that code in the corresponding file test/foo/bar_test.dart.

sm-sayedi · 2024-07-26T00:16:15Z

Thanks @gnprice for the review. The suggested design looks better and more intuitive. Implemented that. PTAL.

gnprice

Thanks @sm-sayedi for the revision! I agree — this design looks good. Small comments on the code.

gnprice · 2024-07-26T05:28:17Z

lib/widgets/compose_box.dart

+        color: const Color.fromRGBO(238, 222, 221, 1),
+        border: Border.all(color: const Color.fromRGBO(132, 41, 36, 0.4)),


Rather than hard-code colors here, let's put them on DesignVariables so that they each have one value for light mode and another for dark mode. For background context, see #95.

In the class, put them in the group of fields at the end, for variables that didn't come from Vlad.

To test dark mode locally, you can change the brightness local in zulipThemeData and then hot reload.

gnprice · 2024-07-26T05:29:10Z

lib/widgets/compose_box.dart

 }

+class _ErrorBanner extends StatelessWidget {


This is separating a StatefulWidget from its corresponding State; move it above to not separate them.

gnprice · 2024-07-26T05:31:30Z

lib/widgets/compose_box.dart

+      final showPlaceholder = otherRecipientIds.any((id) =>
+        !(store.users[id]?.isActive ?? true));
+      if (showPlaceholder) {


A more descriptive name for this local would be like hasDeactivatedUser.

gnprice · 2024-07-26T05:37:08Z

lib/widgets/compose_box.dart

+    if (widget.narrow case DmNarrow(:final otherRecipientIds)) {
+      final store = PerAccountStoreWidget.of(context);
+      final showPlaceholder = otherRecipientIds.any((id) =>
+        !(store.users[id]?.isActive ?? true));


This ?? true doesn't seem right — it means we'll show this error banner if there's a user we don't have access to see. In that situation it isn't particularly likely the user is deactivated — it's much more likely that our user is a guest and they've somehow navigated to a narrow containing a user they can't see. So the error message would be misleading.

Simplest fix is to just say ?? false. If we can't see the user, then the server probably won't ultimately let us send them a message… but then we'll get an error message at that point, so the effect is only annoying and not catastrophic.

A more complete fix would be to look first for any unknown users in the list, and in that case show a different message. Could be "You cannot send messages to unknown users." — "unknown user" is the term we use for these users in the UI and in the Help Center:
https://zulip.com/help/guest-users#configure-whether-guests-can-see-all-other-users

I think it's the opposite. 🙂 If we don't know about a user, we assume it as an active one. showPlaceholder will be true if there is a deactivated user, so in this case, we don't show the error banner.

Ahhh, I miscounted the ! operator around this. Carry on, then.

gnprice · 2024-07-26T05:38:42Z

lib/widgets/compose_box.dart

  });

  final Widget? topicInput;
  final Widget contentInput;
  final Widget sendButton;
+  final Widget? placeholder;


This seems like more than a placeholder — it's not occupying a space where we lack something else to put there, but rather is displacing the things we would otherwise put here.

Could call it… blockingErrorBanner. That name disambiguates from an alternative UI (which is what web does for this very condition, and we might do in the future for some other conditions) where there's an error banner but the rest of the compose-box UI is still there too.

gnprice · 2024-07-26T05:40:00Z

lib/widgets/message_list.dart

-                child: MessageList(narrow: widget.narrow))),
-            ComposeBox(controllerKey: _composeBoxKey, narrow: widget.narrow),
-          ]))));
+          child: Column(crossAxisAlignment: CrossAxisAlignment.stretch,


Hmm interesting — why is this change needed?

Without .stretch, the error banner would not fill the available width. Another alternative could be to give width: double.infinity to the Container in _ErrorBanner, but I think that is not a good option, as this widget may be reused in other places where it doesn't necessarily need to occupy all the available width; so I thought let the parent decide its width.

Hmm, I see. This still seems like the wrong level in the tree to be handling that, though: in particular, this forces the MessageList widget itself to be wide, which isn't desirable. This also represents the message-list page telling the compose box it must be wide; I think it'd make more sense for the compose box to decide how wide it wants to be, within the width available from its parent.

So that means _ComposeBoxLayout would handle making the banner take all the width available to it. That's the widget that already handles that for the compose box's text inputs.

gnprice · 2024-07-26T05:42:45Z

test/widgets/compose_box_test.dart

+      testWidgets('all deactivated users become active -> '
+          'banner is replaced with the compose box', (tester) async {
+        final deactivatedUsers = [eg.user(isActive: false), eg.user(isActive: false)];
+        await prepareComposeBox(tester, narrow: groupDmNarrowWith(deactivatedUsers),
+          users: deactivatedUsers);
+        checkComposeBox(isShown: false);
+
+        await changeUserStatus(tester, user: deactivatedUsers[0], isActive: true);
+        checkComposeBox(isShown: false);
+
+        await changeUserStatus(tester, user: deactivatedUsers[1], isActive: true);
+        checkComposeBox(isShown: true);


Cool, these test cases are nice and compact.

sm-sayedi · 2024-07-26T18:29:47Z

Thanks @gnprice for the review. Changes pushed. Also #816 comment and #816 comment in the previous sub-thread.

chrisbobbe · 2024-07-29T22:42:44Z

Looks like this has a conflict with the recently merged 7bd5bef; could you resolve that please?

sm-sayedi · 2024-07-30T04:44:51Z

Thanks @chrisbobbe. Conflicts resolved!

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2024-07-31T19:05:18Z

lib/widgets/theme.dart

+      errorBannerLabel: const Color.fromRGBO(133, 42, 35, 1),
+      errorBannerBackground: const Color.fromRGBO(238, 222, 221, 1),
+      errorBannerBorder: const Color.fromRGBO(132, 41, 36, 0.4),


Since the web css uses HSL, let's use HSLColor.fromAHSL(…).toColor(), so it's easier to check against web. (Here and in .dark below.)

chrisbobbe · 2024-07-31T19:11:01Z

lib/widgets/theme.dart

  // Not named variables in Figma; taken from older Figma drafts, or elsewhere.
  final Color star;
+  final Color errorBannerLabel;
+  final Color errorBannerBackground;
+  final Color errorBannerBorder;


I think we'd like the items in this section to be in alphabetical order. When making that change, please also update the other boilerplate where these names appear in the class.

chrisbobbe · 2024-07-31T19:18:07Z

lib/widgets/compose_box.dart

+      child: Text(label,
+        maxLines: 2,
+        overflow: TextOverflow.ellipsis,
+        style: TextStyle(fontSize: 18, color: designVariables.errorBannerLabel),
+      ),


Can we remove the maxLines and overflow, and just let the banner be as tall as it needs to be? It seems unhelpful to cut off some of the message without giving a way to see the whole thing. As long as we control label, we should be able to keep it short enough that it doesn't take up way too much vertical space.

chrisbobbe · 2024-07-31T19:26:18Z

test/widgets/compose_box_test.dart

+      await tester.pump();
+    }
+
+    final selfUser = eg.selfUser;


Let's just inline this; three characters isn't much savings. 🙂

sm-sayedi · 2024-08-01T05:32:19Z

Thanks @chrisbobbe for the review. Revision pushed. PTAL.

chrisbobbe

Thanks! LGTM except one thing below, and I'll mark this for Greg's review.

chrisbobbe · 2024-08-02T00:03:32Z

lib/widgets/theme.dart

      // TODO(#95) unchanged in dark theme?
+      errorBannerBackground: const HSLColor.fromAHSL(1, 0, 0.61, 0.19).toColor(),
+      errorBannerBorder: const HSLColor.fromAHSL(0.4, 3, 0.73, 0.74).toColor(),
+      errorBannerLabel: const HSLColor.fromAHSL(1, 2, 0.73, 0.80).toColor(),
      star: const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor(),


The TODO(#95) has been separated from the line that it's about.

sm-sayedi · 2024-08-02T00:53:56Z

Thanks @chrisbobbe for spotting that TODO. Changes pushed.

chrisbobbe · 2024-08-02T18:21:24Z

Thanks! LGTM; over to Greg, then.

This case was new in feature level 222 (server-8). This will become handy in the next commit(s) where we want to change the UI based on `User.isActive` when having a DM conversation.

This will become handy in the next commit(s) when testing the compose box in DMs with deactivated users.

…d users Fixes: zulip#675 Co-authored-by: Rajesh Malviya <[email protected]>

gnprice · 2024-08-02T19:39:20Z

Thanks again @sm-sayedi and @rajveermalviya, and @chrisbobbe for the previous review!

This looks good — merging, with one commit-message tweak:

$ git range-diff origin pr/816 @
1:  8fe389dfc ! 1:  8c5053791 api: Add `isActive` to `RealmUserUpdateEvent`
    @@ Metadata
      ## Commit message ##
         api: Add `isActive` to `RealmUserUpdateEvent`
     
    +    This case was new in feature level 222 (server-8).
    +
         This will become handy in the next commit(s) where we want to change the UI
         based on `User.isActive` when having a DM conversation.
     
2:  89c8c95ea = 2:  cc451157a compose_box test [nfc]: Make `prepareComposeBox` take a list of users
3:  6e18c99ac = 3:  052f2036e compose_box: Replace compose box with a banner in DMs with deactivated users

to give a bit more context on why that isActive case is being added now and how it happened that it wasn't already there.

I feel like _ComposeBoxLayout is doing two somewhat different jobs now, and could be improved with some refactoring. But rather than block merging this on doing that refactor, I'll try to whip that up and send it as a follow-up PR.

…xhaustive This was prompted by noticing recently that we weren't updating [User.isActive]: zulip#816 (comment) After fixing that, I looked and found there were actually three other fields on User which we weren't updating -- a latent bug, since we never looked at those fields, but a bug. Fixed that in the preceding commit. Now add a comment showing my work on verifying that that's it, and giving us a head start on re-verifying that in the future with whatever's changed in the API between now and then.

This was prompted by noticing recently that we weren't updating [User.isActive]: zulip#816 (comment) After fixing that, I looked and found there were actually three other fields on User which we weren't updating -- a latent bug, since we never looked at those fields, but a bug. Fixed that in the preceding commit. Now the remaining fields that don't get updated are fields that really are immutable on a Zulip user. Mark those as final, and add a comment to help maintain the pattern.

sm-sayedi added the buddy review GSoC buddy review needed. label Jul 16, 2024

sm-sayedi requested a review from Khader-1 July 16, 2024 19:02

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from b6023ad to 616322c Compare July 16, 2024 19:10

sm-sayedi changed the title ~~compose_box: DMs with deactivated users~~ compose_box: Disable the compose box in DMs with deactivated users Jul 17, 2024

Khader-1 approved these changes Jul 17, 2024

View reviewed changes

Khader-1 requested a review from hackerkid July 17, 2024 09:21

Khader-1 added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 17, 2024

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from 616322c to 4d106d6 Compare July 17, 2024 18:12

sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Jul 17, 2024

sm-sayedi requested a review from chrisbobbe July 17, 2024 18:34

gnprice reviewed Jul 18, 2024

View reviewed changes

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from 4d106d6 to 3c48c68 Compare July 18, 2024 23:41

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from 3c48c68 to c7a442f Compare July 22, 2024 18:18

gnprice reviewed Jul 24, 2024

View reviewed changes

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from c7a442f to 2fca6a5 Compare July 25, 2024 23:51

sm-sayedi changed the title ~~compose_box: Disable the compose box in DMs with deactivated users~~ compose_box: Replace compose box with a banner in DMs with deactivated users Jul 25, 2024

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from 2fca6a5 to d06abe7 Compare July 26, 2024 00:02

sm-sayedi requested a review from gnprice July 26, 2024 00:16

gnprice reviewed Jul 26, 2024

View reviewed changes

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch 2 times, most recently from 1000957 to 208ec67 Compare July 26, 2024 18:24

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch 3 times, most recently from e5d9552 to bbfd6e4 Compare July 29, 2024 15:59

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from bbfd6e4 to f4b37c2 Compare July 30, 2024 04:34

sm-sayedi requested a review from chrisbobbe July 31, 2024 05:11

chrisbobbe reviewed Jul 31, 2024

View reviewed changes

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from f4b37c2 to 019a68c Compare August 1, 2024 05:17

sm-sayedi requested a review from chrisbobbe August 1, 2024 05:32

chrisbobbe requested changes Aug 2, 2024

View reviewed changes

chrisbobbe 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 Aug 2, 2024

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from 019a68c to 060313b Compare August 2, 2024 00:42

sm-sayedi force-pushed the issue-675-dms-with-deactivated-users branch from 060313b to 6e18c99 Compare August 2, 2024 03:04

chrisbobbe approved these changes Aug 2, 2024

View reviewed changes

sm-sayedi and others added 3 commits August 2, 2024 12:23

api: Add isActive to RealmUserUpdateEvent

8c50537

This case was new in feature level 222 (server-8). This will become handy in the next commit(s) where we want to change the UI based on `User.isActive` when having a DM conversation.

compose_box test [nfc]: Make prepareComposeBox take a list of users

cc45115

This will become handy in the next commit(s) when testing the compose box in DMs with deactivated users.

compose_box: Replace compose box with a banner in DMs with deactivate…

052f203

…d users Fixes: zulip#675 Co-authored-by: Rajesh Malviya <[email protected]>

gnprice force-pushed the issue-675-dms-with-deactivated-users branch from 6e18c99 to 052f203 Compare August 2, 2024 19:39

gnprice merged commit 052f203 into zulip:main Aug 2, 2024

gnprice mentioned this pull request Aug 2, 2024

Refactor compose-box layout with error banner #861

Merged

gnprice mentioned this pull request Aug 10, 2024

Drop three User fields we aren't keeping up to date; verify that's all #876

Merged

		case DmNarrow(): // A group DM thread.
		case (DmNarrow(), true): // A group DM thread.

		case (DmNarrow(), false):
		return zulipLocalizations.composeBoxDeactivatedDmContentHint;

		color: const Color.fromRGBO(238, 222, 221, 1),
		border: Border.all(color: const Color.fromRGBO(132, 41, 36, 0.4)),

compose_box: Replace compose box with a banner in DMs with deactivated users #816

compose_box: Replace compose box with a banner in DMs with deactivated users #816

Conversation

sm-sayedi commented Jul 16, 2024 • edited Loading

Khader-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alya commented Jul 17, 2024

Choose a reason for hiding this comment

sm-sayedi commented Jul 18, 2024

alya commented Jul 18, 2024

alya commented Jul 18, 2024

sm-sayedi commented Jul 18, 2024

alya commented Jul 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi commented Jul 26, 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

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

sm-sayedi commented Jul 26, 2024 • edited Loading

chrisbobbe commented Jul 29, 2024

sm-sayedi commented Jul 30, 2024

chrisbobbe 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

chrisbobbe Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

sm-sayedi commented Aug 1, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm-sayedi commented Aug 2, 2024

chrisbobbe commented Aug 2, 2024 • edited Loading

gnprice commented Aug 2, 2024

sm-sayedi commented Jul 16, 2024 •

edited

Loading

sm-sayedi commented Jul 26, 2024 •

edited

Loading

chrisbobbe Jul 31, 2024 •

edited

Loading

chrisbobbe commented Aug 2, 2024 •

edited

Loading