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.

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

compose_box: Replace compose box with a banner when cannot post in a channel #886

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
4 changes: 4 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@
"@errorBannerDeactivatedDmLabel": {
"description": "Label text for error banner when sending a message to one or multiple deactivated users."
},
"errorBannerCannotPostInChannelLabel": "You do not have permission to post in this channel.",
"@errorBannerCannotPostInChannelLabel": {
"description": "Error-banner text replacing the compose box when you do not have permission to send a message to the channel."
},
"composeBoxAttachFilesTooltip": "Attach files",
"@composeBoxAttachFilesTooltip": {
"description": "Tooltip for compose box icon to attach a file to the message."
Expand Down
9 changes: 9 additions & 0 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ class InitialSnapshot {

final List<UserTopicItem>? userTopics; // TODO(server-6)

/// The number of days until a user's account is treated as a full member.
///
/// Search for "realm_waiting_period_threshold" in https://zulip.com/api/register-queue.
///
/// For how to determine if a user is a full member, see:
/// https://zulip.com/api/roles-and-permissions#determining-if-a-user-is-a-full-member
final int realmWaitingPeriodThreshold;

final Map<String, RealmDefaultExternalAccount> realmDefaultExternalAccounts;

final int maxFileUploadSizeMib;
Expand Down Expand Up @@ -115,6 +123,7 @@ class InitialSnapshot {
required this.streams,
required this.userSettings,
required this.userTopics,
required this.realmWaitingPeriodThreshold,
required this.realmDefaultExternalAccounts,
required this.maxFileUploadSizeMib,
required this.realmUsers,
Expand Down
3 changes: 3 additions & 0 deletions lib/api/model/initial_snapshot.g.dart

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

30 changes: 30 additions & 0 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,18 @@ class User {
factory User.fromJson(Map<String, dynamic> json) => _$UserFromJson(json);

Map<String, dynamic> toJson() => _$UserToJson(this);

/// Whether the user has passed the realm's waiting period to be a full member.
///
/// See:
/// https://zulip.com/api/roles-and-permissions#determining-if-a-user-is-a-full-member
///
/// To determine if a user is a full member, callers must also check that the
/// user's role is at least Role.Member.
bool hasPassedWaitingPeriod(DateTime byDate, int realmWaitingPeriodThreshold) {
final dateJoined = DateTime.parse(this.dateJoined);
Copy link
Member

Choose a reason for hiding this comment

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

We could simplify this by parsing dateJoined as we deserialize the User JSON object.

Copy link
Member

Choose a reason for hiding this comment

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

We could, but I'd prefer to keep that parsing deferred: converting user objects from JSON is a hot spot performance-wise, because there can be so many of them, and parsing a DateTime is the sort of thing that I feel could easily be slow.

And conversely I think there's only a few places like here where we care about the value of dateJoined, so it's not much burden on code complexity for these to have to handle the parsing; and they're all for just one user at a time, so there's no performance concern.

return byDate.difference(dateJoined).inDays >= realmWaitingPeriodThreshold;
}
}

/// As in [User.profileData].
Expand Down Expand Up @@ -303,6 +315,11 @@ enum UserRole{
final int? apiValue;

int? toJson() => apiValue;

bool isAtLeast(UserRole threshold) {
// Roles with more privilege have lower [apiValue].
return (apiValue ?? 0) <= (threshold.apiValue ?? 0);
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this, I was thinking about why 0 is chosen as the default value for both operands. Considering the context this helper is used, I suppose that we want to fallback to the behavior that, when the role has an unknown value, we allow the user to post.

This consideration is specific to the posting permission feature we are implementing here. I think the best thing to do is to assert that both api values are non-null; then we handle the case when either of them is null in hasPostingPermission.

}
}

/// As in `streams` in the initial snapshot.
Expand Down Expand Up @@ -370,6 +387,19 @@ class ZulipStream {
_$ZulipStreamFromJson(json);

Map<String, dynamic> toJson() => _$ZulipStreamToJson(this);

bool hasPostingPermission(User user, {required DateTime byDate, required int realmWaitingPeriodThreshold}) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's break this into multiple lines

final role = user.role;
return switch (channelPostPolicy) {
ChannelPostPolicy.any => true,
ChannelPostPolicy.fullMembers => role.isAtLeast(UserRole.member) && (role == UserRole.member
Copy link
Member

@PIG208 PIG208 Oct 10, 2024

Choose a reason for hiding this comment

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

nit: by turning this into || we can replace the ... ? ... : true clause with ... && ...

? user.hasPassedWaitingPeriod(byDate, realmWaitingPeriodThreshold)
: true),
ChannelPostPolicy.moderators => role.isAtLeast(UserRole.moderator),
ChannelPostPolicy.administrators => role.isAtLeast(UserRole.administrator),
ChannelPostPolicy.unknown => true,
};
}
}

/// The name of a property of [ZulipStream] that gets updated
Expand Down
4 changes: 4 additions & 0 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
connection: connection,
realmUrl: realmUrl,
maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib,
realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should come before maxFileUploadSizeMib.

realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts,
customProfileFields: _sortCustomProfileFields(initialSnapshot.customProfileFields),
emailAddressVisibility: initialSnapshot.emailAddressVisibility,
Expand Down Expand Up @@ -270,6 +271,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess
required this.connection,
required this.realmUrl,
required this.maxFileUploadSizeMib,
required this.realmWaitingPeriodThreshold,
required this.realmDefaultExternalAccounts,
required this.customProfileFields,
required this.emailAddressVisibility,
Expand Down Expand Up @@ -324,6 +326,8 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess

String get zulipVersion => account.zulipVersion;
final int maxFileUploadSizeMib; // No event for this.
/// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold].
final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting
final Map<String, RealmDefaultExternalAccount> realmDefaultExternalAccounts;
List<CustomProfileField> customProfileFields;
/// For docs, please see [InitialSnapshot.emailAddressVisibility].
Expand Down
47 changes: 29 additions & 18 deletions lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1071,26 +1071,8 @@ 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));
if (hasDeactivatedUser) {
return _ErrorBanner(label: ZulipLocalizations.of(context)
.errorBannerDeactivatedDmLabel);
}
}
return null;
}

@override
Widget build(BuildContext context) {
final errorBanner = _errorBanner(context);
if (errorBanner != null) {
return _ComposeBoxContainer(child: errorBanner);
}

return _ComposeBoxLayout(
contentController: _contentController,
contentFocusNode: _contentFocusNode,
Expand Down Expand Up @@ -1128,8 +1110,37 @@ class ComposeBox extends StatelessWidget {
}
}

Widget? _errorBanner(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
final selfUser = store.users[store.selfUserId]!;
switch (narrow) {
case ChannelNarrow narrow:
final channel = store.streams[narrow.streamId]!;
return channel.hasPostingPermission(selfUser, byDate: DateTime.now(), realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)
? null : _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel);
case TopicNarrow narrow:
final channel = store.streams[narrow.streamId]!;
return channel.hasPostingPermission(selfUser, byDate: DateTime.now(), realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)
? null : _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel);
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's extract ZulipLocalizations.of(context) as a variable

Comment on lines +1117 to +1124
Copy link
Member

Choose a reason for hiding this comment

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

Then we can simplify this to:

Suggested change
case ChannelNarrow narrow:
final channel = store.streams[narrow.streamId]!;
return channel.hasPostingPermission(selfUser, byDate: DateTime.now(), realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)
? null : _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel);
case TopicNarrow narrow:
final channel = store.streams[narrow.streamId]!;
return channel.hasPostingPermission(selfUser, byDate: DateTime.now(), realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)
? null : _ErrorBanner(label: ZulipLocalizations.of(context).errorBannerCannotPostInChannelLabel);
case ChannelNarrow(:final streamId):
case TopicNarrow(:final streamId):
final channel = store.streams[streamId]!;
if (channel.hasPostingPermission(selfUser, byDate: DateTime.now(),
realmWaitingPeriodThreshold: store.realmWaitingPeriodThreshold)) {
return _ErrorBanner(
label: localizations.errorBannerCannotPostInChannelLabel);
}

case DmNarrow(:final otherRecipientIds):
final hasDeactivatedUser = otherRecipientIds.any((id) =>
!(store.users[id]?.isActive ?? true));
return hasDeactivatedUser ? _ErrorBanner(label: ZulipLocalizations.of(context)
.errorBannerDeactivatedDmLabel) : null;
Comment on lines +1128 to +1129
Copy link
Member

Choose a reason for hiding this comment

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

nit: to make the lines shorter:

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

case CombinedFeedNarrow():
case MentionsNarrow():
case StarredMessagesNarrow():
return null;
}
}

@override
Widget build(BuildContext context) {
final errorBanner = _errorBanner(context);
if (errorBanner != null) {
return _ComposeBoxContainer(child: errorBanner);
}

final narrow = this.narrow;
switch (narrow) {
case ChannelNarrow():
Expand Down
8 changes: 6 additions & 2 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,10 @@ User user({
String? deliveryEmail,
String? email,
String? fullName,
String? dateJoined,
bool? isActive,
bool? isBot,
UserRole? role,
String? avatarUrl,
Map<int, ProfileFieldUserData>? profileData,
}) {
Expand All @@ -121,13 +123,13 @@ User user({
deliveryEmail: effectiveDeliveryEmail,
email: email ?? effectiveDeliveryEmail,
fullName: fullName ?? 'A user', // TODO generate example names
dateJoined: '2023-04-28',
dateJoined: dateJoined ?? '2023-04-28',
isActive: isActive ?? true,
isBillingAdmin: false,
isBot: isBot ?? false,
botType: null,
botOwnerId: null,
role: UserRole.member,
role: role ?? UserRole.member,
timezone: 'UTC',
avatarUrl: avatarUrl,
avatarVersion: 0,
Expand Down Expand Up @@ -789,6 +791,7 @@ InitialSnapshot initialSnapshot({
List<ZulipStream>? streams,
UserSettings? userSettings,
List<UserTopicItem>? userTopics,
int? realmWaitingPeriodThreshold,
Map<String, RealmDefaultExternalAccount>? realmDefaultExternalAccounts,
int? maxFileUploadSizeMib,
List<User>? realmUsers,
Expand Down Expand Up @@ -821,6 +824,7 @@ InitialSnapshot initialSnapshot({
emojiset: Emojiset.google,
),
userTopics: userTopics,
realmWaitingPeriodThreshold: realmWaitingPeriodThreshold ?? 0,
realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {},
maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25,
realmUsers: realmUsers ?? [],
Expand Down
2 changes: 1 addition & 1 deletion test/widgets/action_sheet_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {

await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
await store.addUser(eg.user(userId: message.senderId));
await store.addUsers([eg.selfUser, eg.user(userId: message.senderId)]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

compose_box: Replace compose box with a banner when cannot post in a channel

This seems like a reasonable change to the action-sheet tests, but why is it needed in this commit, which is about the compose box?

…Ah, I think I understand: it's a boring but necessary bit of setup so that the simulated action sheet doesn't crash in the code that decides whether to show the error banner. Is that right?

There are quite a few other changes in this commit that look like they're made for the same reason. Would you move those to a prep commit before this commit? That should make it easier to focus on the interesting changes here. 🙂

if (message is StreamMessage) {
final stream = eg.stream(streamId: message.streamId);
await store.addStream(stream);
Expand Down
2 changes: 2 additions & 0 deletions test/widgets/autocomplete_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,12 @@ Future<Finder> setupToTopicInput(WidgetTester tester, {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
await store.addUser(eg.selfUser);
final connection = store.connection as FakeApiConnection;

// prepare message list data
final stream = eg.stream();
await store.addStream(stream);
final message = eg.streamMessage(stream: stream, sender: eg.selfUser);
connection.prepare(json: GetMessagesResult(
anchor: message.id,
Expand Down
Loading