Skip to content

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

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

Merged
merged 3 commits into from
Aug 2, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c50537
api: Add `isActive` to `RealmUserUpdateEvent`
sm-sayedi Jul 16, 2024
cc45115
compose_box test [nfc]: Make `prepareComposeBox` take a list of users
sm-sayedi Jul 25, 2024
052f203
compose_box: Replace compose box with a banner in DMs with deactivate…
sm-sayedi Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/l10n/app_en.arb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@
"@successMessageLinkCopied": {
"description": "Message when link of a message was copied to the user's system clipboard."
},
"errorBannerDeactivatedDmLabel": "You cannot send messages to deactivated users.",
"@errorBannerDeactivatedDmLabel": {
"description": "Label text for error banner when sending a message to one or multiple deactivated users."
},
"composeBoxAttachFilesTooltip": "Attach files",
"@composeBoxAttachFilesTooltip": {
"description": "Tooltip for compose box icon to attach a file to the message."
Expand Down
2 changes: 2 additions & 0 deletions lib/api/model/events.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ class RealmUserUpdateEvent extends RealmUserEvent {

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

… 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
image

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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. 😀


static Object? _readFromPerson(Map<dynamic, dynamic> json, String key) {
return (json['person'] as Map<String, dynamic>)[key];
Expand Down Expand Up @@ -319,6 +320,7 @@ class RealmUserUpdateEvent extends RealmUserEvent {
this.deliveryEmail,
this.customProfileField,
this.newEmail,
this.isActive,
});

factory RealmUserUpdateEvent.fromJson(Map<String, dynamic> json) =>
Expand Down
3 changes: 3 additions & 0 deletions lib/api/model/events.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ class PerAccountStore extends ChangeNotifier with ChannelStore, MessageStore {
if (event.isBillingAdmin != null) user.isBillingAdmin = event.isBillingAdmin!;
if (event.deliveryEmail != null) user.deliveryEmail = event.deliveryEmail!.value;
if (event.newEmail != null) user.email = event.newEmail!;
if (event.isActive != null) user.isActive = event.isActive!;
if (event.customProfileField != null) {
final profileData = (user.profileData ??= {});
final update = event.customProfileField!;
Expand Down
86 changes: 63 additions & 23 deletions lib/widgets/compose_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import '../model/store.dart';
import 'autocomplete.dart';
import 'dialog.dart';
import 'store.dart';
import 'theme.dart';

const double _inputVerticalPadding = 8;
const double _sendButtonSize = 36;
Expand Down Expand Up @@ -850,11 +851,13 @@ class _ComposeBoxLayout extends StatelessWidget {
required this.sendButton,
required this.contentController,
required this.contentFocusNode,
this.blockingErrorBanner,
});

final Widget? topicInput;
final Widget contentInput;
final Widget sendButton;
final Widget? blockingErrorBanner;
final ComposeContentController contentController;
final FocusNode contentFocusNode;

Expand Down Expand Up @@ -883,28 +886,30 @@ class _ComposeBoxLayout extends StatelessWidget {
minimum: const EdgeInsets.fromLTRB(8, 0, 8, 8),
child: Padding(
padding: const EdgeInsets.only(top: 8.0),
child: Column(children: [
Row(crossAxisAlignment: CrossAxisAlignment.end, children: [
Expanded(
child: Theme(
data: inputThemeData,
child: Column(children: [
if (topicInput != null) topicInput!,
if (topicInput != null) const SizedBox(height: 8),
contentInput,
]))),
const SizedBox(width: 8),
sendButton,
]),
Theme(
data: themeData.copyWith(
iconTheme: themeData.iconTheme.copyWith(color: colorScheme.onSurfaceVariant)),
child: Row(children: [
_AttachFileButton(contentController: contentController, contentFocusNode: contentFocusNode),
_AttachMediaButton(contentController: contentController, contentFocusNode: contentFocusNode),
_AttachFromCameraButton(contentController: contentController, contentFocusNode: contentFocusNode),
])),
])))); }
child: blockingErrorBanner != null
? SizedBox(width: double.infinity, child: blockingErrorBanner)
: Column(children: [
Row(crossAxisAlignment: CrossAxisAlignment.end, children: [
Expanded(
child: Theme(
data: inputThemeData,
child: Column(children: [
if (topicInput != null) topicInput!,
if (topicInput != null) const SizedBox(height: 8),
contentInput,
]))),
const SizedBox(width: 8),
sendButton,
]),
Theme(
data: themeData.copyWith(
iconTheme: themeData.iconTheme.copyWith(color: colorScheme.onSurfaceVariant)),
child: Row(children: [
_AttachFileButton(contentController: contentController, contentFocusNode: contentFocusNode),
_AttachMediaButton(contentController: contentController, contentFocusNode: contentFocusNode),
_AttachFromCameraButton(contentController: contentController, contentFocusNode: contentFocusNode),
])),
])))); }
}

abstract class ComposeBoxController<T extends StatefulWidget> extends State<T> {
Expand Down Expand Up @@ -973,6 +978,27 @@ class _StreamComposeBoxState extends State<_StreamComposeBox> implements Compose
}
}

class _ErrorBanner extends StatelessWidget {
Comment on lines 979 to +981
Copy link
Member

Choose a reason for hiding this comment

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

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

const _ErrorBanner({required this.label});

final String label;

@override
Widget build(BuildContext context) {
final designVariables = DesignVariables.of(context);
return Container(
padding: const EdgeInsets.all(8),
decoration: BoxDecoration(
color: designVariables.errorBannerBackground,
border: Border.all(color: designVariables.errorBannerBorder),
borderRadius: BorderRadius.circular(5)),
child: Text(label,
style: TextStyle(fontSize: 18, color: designVariables.errorBannerLabel),
),
);
}
}

class _FixedDestinationComposeBox extends StatefulWidget {
const _FixedDestinationComposeBox({super.key, required this.narrow});

Expand All @@ -998,6 +1024,19 @@ class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox
super.dispose();
}

Widget? _errorBanner(BuildContext context) {
if (widget.narrow case DmNarrow(:final otherRecipientIds)) {
final store = PerAccountStoreWidget.of(context);
final hasDeactivatedUser = otherRecipientIds.any((id) =>
!(store.users[id]?.isActive ?? true));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

if (hasDeactivatedUser) {
return _ErrorBanner(label: ZulipLocalizations.of(context)
.errorBannerDeactivatedDmLabel);
}
}
return null;
}

@override
Widget build(BuildContext context) {
return _ComposeBoxLayout(
Expand All @@ -1013,7 +1052,8 @@ class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox
topicController: null,
contentController: _contentController,
getDestination: () => widget.narrow.destination,
));
),
blockingErrorBanner: _errorBanner(context));
}
}

Expand Down
21 changes: 21 additions & 0 deletions lib/widgets/theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
channelColorSwatches: ChannelColorSwatches.light,
atMentionMarker: const HSLColor.fromAHSL(0.5, 0, 0, 0.2).toColor(),
dmHeaderBg: const HSLColor.fromAHSL(1, 46, 0.35, 0.93).toColor(),
errorBannerBackground: const HSLColor.fromAHSL(1, 4, 0.33, 0.90).toColor(),
errorBannerBorder: const HSLColor.fromAHSL(0.4, 3, 0.57, 0.33).toColor(),
errorBannerLabel: const HSLColor.fromAHSL(1, 4, 0.58, 0.33).toColor(),
loginOrDivider: const Color(0xffdedede),
loginOrDividerText: const Color(0xff575757),
sectionCollapseIcon: const Color(0x7f1e2e48),
Expand All @@ -163,6 +166,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
// TODO(#95) need proper dark-theme color (this is ad hoc)
atMentionMarker: const HSLColor.fromAHSL(0.4, 0, 0, 1).toColor(),
dmHeaderBg: const HSLColor.fromAHSL(1, 46, 0.15, 0.2).toColor(),
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(),
loginOrDivider: const Color(0xff424242),
loginOrDividerText: const Color(0xffa8a8a8),
// TODO(#95) need proper dark-theme color (this is ad hoc)
Expand All @@ -185,6 +191,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
required this.channelColorSwatches,
required this.atMentionMarker,
required this.dmHeaderBg,
required this.errorBannerBackground,
required this.errorBannerBorder,
required this.errorBannerLabel,
required this.loginOrDivider,
required this.loginOrDividerText,
required this.sectionCollapseIcon,
Expand Down Expand Up @@ -218,6 +227,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
// Not named variables in Figma; taken from older Figma drafts, or elsewhere.
final Color atMentionMarker;
final Color dmHeaderBg;
final Color errorBannerBackground;
final Color errorBannerBorder;
final Color errorBannerLabel;
final Color loginOrDivider; // TODO(#95) need proper dark-theme color (this is ad hoc)
final Color loginOrDividerText; // TODO(#95) need proper dark-theme color (this is ad hoc)
final Color sectionCollapseIcon;
Expand All @@ -238,6 +250,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
ChannelColorSwatches? channelColorSwatches,
Color? atMentionMarker,
Color? dmHeaderBg,
Color? errorBannerBackground,
Color? errorBannerBorder,
Color? errorBannerLabel,
Color? loginOrDivider,
Color? loginOrDividerText,
Color? sectionCollapseIcon,
Expand All @@ -257,6 +272,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
channelColorSwatches: channelColorSwatches ?? this.channelColorSwatches,
atMentionMarker: atMentionMarker ?? this.atMentionMarker,
dmHeaderBg: dmHeaderBg ?? this.dmHeaderBg,
errorBannerBackground: errorBannerBackground ?? this.errorBannerBackground,
errorBannerBorder: errorBannerBorder ?? this.errorBannerBorder,
errorBannerLabel: errorBannerLabel ?? this.errorBannerLabel,
loginOrDivider: loginOrDivider ?? this.loginOrDivider,
loginOrDividerText: loginOrDividerText ?? this.loginOrDividerText,
sectionCollapseIcon: sectionCollapseIcon ?? this.sectionCollapseIcon,
Expand All @@ -283,6 +301,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
channelColorSwatches: ChannelColorSwatches.lerp(channelColorSwatches, other.channelColorSwatches, t),
atMentionMarker: Color.lerp(atMentionMarker, other.atMentionMarker, t)!,
dmHeaderBg: Color.lerp(dmHeaderBg, other.dmHeaderBg, t)!,
errorBannerBackground: Color.lerp(errorBannerBackground, other.errorBannerBackground, t)!,
errorBannerBorder: Color.lerp(errorBannerBorder, other.errorBannerBorder, t)!,
errorBannerLabel: Color.lerp(errorBannerLabel, other.errorBannerLabel, t)!,
loginOrDivider: Color.lerp(loginOrDivider, other.loginOrDivider, t)!,
loginOrDividerText: Color.lerp(loginOrDividerText, other.loginOrDividerText, t)!,
sectionCollapseIcon: Color.lerp(sectionCollapseIcon, other.sectionCollapseIcon, t)!,
Expand Down
Loading