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.

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

Jump to bottom

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
+338 −95
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ffb3238
api: Add `realmWaitingPeriodThreshold` to `InitialSnapshot`
sm-sayedi Aug 6, 2024
9616005
store: Add `realmWaitingPeriodThreshold` to `PerAccountStore`
sm-sayedi Aug 6, 2024
6cf3dd9
model: Add `hasPassedWaitingPeriod` helper method to `User`
sm-sayedi Aug 10, 2024
d1c16a9
model: Add `hasPostingPermission` helper method to `ZulipStream`
sm-sayedi Aug 10, 2024
5aaad2d
test [nfc]: Have `eg.user` accept `dateJoined` and `role`
sm-sayedi Aug 12, 2024
dc9a330
compose_box test [nfc]: Have `prepareComposeBox` accept additional pa…
sm-sayedi Aug 12, 2024
58276c3
test [nfc]: Add `stream`s and `eg.selfUser` to `store` in several tests
sm-sayedi Aug 19, 2024
be0d74d
compose_box: Replace compose box with a banner when cannot post in a …
sm-sayedi Aug 19, 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 @@ -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
47 changes: 29 additions & 18 deletions lib/widgets/compose_box.dart
View file
Open in desktop
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
276 changes: 215 additions & 61 deletions test/widgets/compose_box_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,10 @@ void main() {
});
});

group('compose box in DMs with deactivated users', () {
Finder contentFieldFinder() => find.descendant(
group('compose box replacing with error banner', () {
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;

Finder inputFieldFinder() => find.descendant(
of: find.byType(ComposeBox),
matching: find.byType(TextField));

Expand All @@ -405,97 +407,249 @@ void main() {
matching: find.widgetWithIcon(IconButton, icon));

void checkComposeBoxParts({required bool areShown}) {
check(contentFieldFinder().evaluate().length).equals(areShown ? 1 : 0);
final inputFieldCount = inputFieldFinder().evaluate().length;
areShown ? check(inputFieldCount).isGreaterThan(0) : check(inputFieldCount).equals(0);
check(attachButtonFinder(Icons.attach_file).evaluate().length).equals(areShown ? 1 : 0);
check(attachButtonFinder(Icons.image).evaluate().length).equals(areShown ? 1 : 0);
check(attachButtonFinder(Icons.camera_alt).evaluate().length).equals(areShown ? 1 : 0);
}

void checkBanner({required bool isShown}) {
final bannerTextFinder = find.text(GlobalLocalizations.zulipLocalizations
.errorBannerDeactivatedDmLabel);
check(bannerTextFinder.evaluate().length).equals(isShown ? 1 : 0);
void checkBannerWithLabel(String label, {required bool isShown}) {
check(find.text(label).evaluate().length).equals(isShown ? 1 : 0);
}

void checkComposeBox({required bool isShown}) {
void checkComposeBoxIsShown(bool isShown, {required String bannerLabel}) {
checkComposeBoxParts(areShown: isShown);
checkBanner(isShown: !isShown);
checkBannerWithLabel(bannerLabel, isShown: !isShown);
}

Future<void> changeUserStatus(WidgetTester tester,
{required User user, required bool isActive}) async {
await store.handleEvent(RealmUserUpdateEvent(id: 1,
userId: user.userId, isActive: isActive));
await tester.pump();
}
group('in DMs with deactivated users', () {
void checkComposeBox({required bool isShown}) => checkComposeBoxIsShown(isShown,
bannerLabel: zulipLocalizations.errorBannerDeactivatedDmLabel);

DmNarrow dmNarrowWith(User otherUser) => DmNarrow.withUser(otherUser.userId,
selfUserId: eg.selfUser.userId);
Future<void> changeUserStatus(WidgetTester tester,
{required User user, required bool isActive}) async {
await store.handleEvent(RealmUserUpdateEvent(id: 1,
userId: user.userId, isActive: isActive));
await tester.pump();
}

DmNarrow groupDmNarrowWith(List<User> otherUsers) => DmNarrow.withOtherUsers(
otherUsers.map((u) => u.userId), selfUserId: eg.selfUser.userId);
DmNarrow dmNarrowWith(User otherUser) => DmNarrow.withUser(otherUser.userId,
selfUserId: eg.selfUser.userId);

group('1:1 DMs', () {
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 we are both moving tests to groups and adding the new tests.
The grouping part can be done in a prep commit so the diffs will be easier to read.

testWidgets('compose box replaced with a banner', (tester) async {
final deactivatedUser = eg.user(isActive: false);
await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser),
users: [deactivatedUser]);
checkComposeBox(isShown: false);
});
DmNarrow groupDmNarrowWith(List<User> otherUsers) => DmNarrow.withOtherUsers(
otherUsers.map((u) => u.userId), selfUserId: eg.selfUser.userId);

testWidgets('active user becomes deactivated -> '
'compose box is replaced with a banner', (tester) async {
final activeUser = eg.user(isActive: true);
await prepareComposeBox(tester, narrow: dmNarrowWith(activeUser),
users: [activeUser]);
checkComposeBox(isShown: true);
group('1:1 DMs', () {
testWidgets('compose box replaced with a banner', (tester) async {
final deactivatedUser = eg.user(isActive: false);
await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser),
users: [deactivatedUser]);
checkComposeBox(isShown: false);
});

await changeUserStatus(tester, user: activeUser, isActive: false);
checkComposeBox(isShown: false);
testWidgets('active user becomes deactivated -> '
'compose box is replaced with a banner', (tester) async {
final activeUser = eg.user(isActive: true);
await prepareComposeBox(tester, narrow: dmNarrowWith(activeUser),
users: [activeUser]);
checkComposeBox(isShown: true);

await changeUserStatus(tester, user: activeUser, isActive: false);
checkComposeBox(isShown: false);
});

testWidgets('deactivated user becomes active -> '
'banner is replaced with the compose box', (tester) async {
final deactivatedUser = eg.user(isActive: false);
await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser),
users: [deactivatedUser]);
checkComposeBox(isShown: false);

await changeUserStatus(tester, user: deactivatedUser, isActive: true);
checkComposeBox(isShown: true);
});
});

testWidgets('deactivated user becomes active -> '
'banner is replaced with the compose box', (tester) async {
final deactivatedUser = eg.user(isActive: false);
await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser),
users: [deactivatedUser]);
checkComposeBox(isShown: false);
group('group DMs', () {
testWidgets('compose box replaced with a banner', (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: deactivatedUser, isActive: true);
checkComposeBox(isShown: true);
testWidgets('at least one user becomes deactivated -> '
'compose box is replaced with a banner', (tester) async {
final activeUsers = [eg.user(isActive: true), eg.user(isActive: true)];
await prepareComposeBox(tester, narrow: groupDmNarrowWith(activeUsers),
users: activeUsers);
checkComposeBox(isShown: true);

await changeUserStatus(tester, user: activeUsers[0], isActive: false);
checkComposeBox(isShown: false);
});

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);
});
});
});

group('group DMs', () {
testWidgets('compose box replaced with a banner', (tester) async {
final deactivatedUsers = [eg.user(isActive: false), eg.user(isActive: false)];
await prepareComposeBox(tester, narrow: groupDmNarrowWith(deactivatedUsers),
users: deactivatedUsers);
checkComposeBox(isShown: false);
group('in topic/channel narrow according to channel post policy', () {
void checkComposeBox({required bool isShown}) => checkComposeBoxIsShown(isShown,
bannerLabel: zulipLocalizations.errorBannerCannotPostInChannelLabel);

final testCases = [
(ChannelPostPolicy.unknown, UserRole.unknown, true),
(ChannelPostPolicy.unknown, UserRole.guest, true),
(ChannelPostPolicy.unknown, UserRole.member, true),
(ChannelPostPolicy.unknown, UserRole.moderator, true),
(ChannelPostPolicy.unknown, UserRole.administrator, true),
(ChannelPostPolicy.unknown, UserRole.owner, true),
(ChannelPostPolicy.any, UserRole.unknown, true),
(ChannelPostPolicy.any, UserRole.guest, true),
(ChannelPostPolicy.any, UserRole.member, true),
(ChannelPostPolicy.any, UserRole.moderator, true),
(ChannelPostPolicy.any, UserRole.administrator, true),
(ChannelPostPolicy.any, UserRole.owner, true),
(ChannelPostPolicy.fullMembers, UserRole.unknown, true),
(ChannelPostPolicy.fullMembers, UserRole.guest, false),
(ChannelPostPolicy.fullMembers, UserRole.member, true),
(ChannelPostPolicy.fullMembers, UserRole.moderator, true),
(ChannelPostPolicy.fullMembers, UserRole.administrator, true),
(ChannelPostPolicy.fullMembers, UserRole.owner, true),
(ChannelPostPolicy.moderators, UserRole.unknown, true),
(ChannelPostPolicy.moderators, UserRole.guest, false),
(ChannelPostPolicy.moderators, UserRole.member, false),
(ChannelPostPolicy.moderators, UserRole.moderator, true),
(ChannelPostPolicy.moderators, UserRole.administrator, true),
(ChannelPostPolicy.moderators, UserRole.owner, true),
(ChannelPostPolicy.administrators, UserRole.unknown, true),
(ChannelPostPolicy.administrators, UserRole.guest, false),
(ChannelPostPolicy.administrators, UserRole.member, false),
(ChannelPostPolicy.administrators, UserRole.moderator, false),
(ChannelPostPolicy.administrators, UserRole.administrator, true),
(ChannelPostPolicy.administrators, UserRole.owner, true),
];

for (final testCase in testCases) {
final (ChannelPostPolicy policy, UserRole role, bool canPost) = testCase;
Comment on lines +546 to +547
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
for (final testCase in testCases) {
final (ChannelPostPolicy policy, UserRole role, bool canPost) = testCase;
for (final (ChannelPostPolicy policy, UserRole role, bool canPost) in testCases) {

Comment on lines +546 to +547
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
for (final testCase in testCases) {
final (ChannelPostPolicy policy, UserRole role, bool canPost) = testCase;
for (final (ChannelPostPolicy policy, UserRole role, bool canPost) in testCases) {


testWidgets('"${role.name}" user ${canPost ? 'can' : "can't"} post in channel with "${policy.name}" policy', (tester) async {
final selfUser = eg.user(role: role);
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
streams: [eg.stream(streamId: 1, channelPostPolicy: policy)]);
checkComposeBox(isShown: canPost);
});

testWidgets('"${role.name}" user ${canPost ? 'can' : "can't"} post in topic with "${policy.name}" channel policy', (tester) async {
final selfUser = eg.user(role: role);
await prepareComposeBox(tester,
narrow: const TopicNarrow(1, 'topic'),
selfUser: selfUser,
streams: [eg.stream(streamId: 1, channelPostPolicy: policy)]);
checkComposeBox(isShown: canPost);
});
}

group('only "full member" user can post in channel with "fullMembers" policy', () {
testWidgets('"full member" -> can post in channel', (tester) async {
final selfUser = eg.user(role: UserRole.member,
dateJoined: DateTime.now().subtract(const Duration(days: 30)).toIso8601String());
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
realmWaitingPeriodThreshold: 30,
streams: [eg.stream(streamId: 1, channelPostPolicy: ChannelPostPolicy.fullMembers)]);
checkComposeBox(isShown: true);
});

testWidgets('not a "full member" -> cannot post in channel', (tester) async {
final selfUser = eg.user(role: UserRole.member,
dateJoined: DateTime.now().subtract(const Duration(days: 29)).toIso8601String());
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
realmWaitingPeriodThreshold: 30,
streams: [eg.stream(streamId: 1, channelPostPolicy: ChannelPostPolicy.fullMembers)]);
checkComposeBox(isShown: false);
});
});

testWidgets('at least one user becomes deactivated -> '
'compose box is replaced with a banner', (tester) async {
final activeUsers = [eg.user(isActive: true), eg.user(isActive: true)];
await prepareComposeBox(tester, narrow: groupDmNarrowWith(activeUsers),
users: activeUsers);
Future<void> changeUserRole(WidgetTester tester,
{required User user, required UserRole role}) async {
await store.handleEvent(RealmUserUpdateEvent(id: 1,
userId: user.userId, role: role));
await tester.pump();
}

Future<void> changeChannelPolicy(WidgetTester tester,
{required ZulipStream channel, required ChannelPostPolicy policy}) async {
await store.handleEvent(eg.channelUpdateEvent(channel,
property: ChannelPropertyName.channelPostPolicy, value: policy));
await tester.pump();
}

testWidgets('user role decreases -> compose box is replaced with the banner', (tester) async {
final selfUser = eg.user(role: UserRole.administrator);
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
streams: [eg.stream(streamId: 1, channelPostPolicy: ChannelPostPolicy.administrators)]);
checkComposeBox(isShown: true);

await changeUserStatus(tester, user: activeUsers[0], isActive: false);
await changeUserRole(tester, user: selfUser, role: UserRole.moderator);
checkComposeBox(isShown: false);
});

testWidgets('all deactivated users become active -> '
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the deletion is a bit far apart from where it was re-added.

A goal of restructuring the change would be that we have one commit that moves the old tests, and one that only adds the new ones.

'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);
testWidgets('user role increases -> banner is replaced with the compose box', (tester) async {
final selfUser = eg.user(role: UserRole.guest);
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
streams: [eg.stream(streamId: 1, channelPostPolicy: ChannelPostPolicy.moderators)]);
checkComposeBox(isShown: false);

await changeUserStatus(tester, user: deactivatedUsers[0], isActive: true);
await changeUserRole(tester, user: selfUser, role: UserRole.administrator);
checkComposeBox(isShown: true);
});

testWidgets('channel policy becomes stricter -> compose box is replaced with the banner', (tester) async {
final selfUser = eg.user(role: UserRole.guest);
final channel = eg.stream(streamId: 1, channelPostPolicy: ChannelPostPolicy.any);
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
streams: [channel]);
checkComposeBox(isShown: true);

await changeChannelPolicy(tester, channel: channel, policy: ChannelPostPolicy.fullMembers);
checkComposeBox(isShown: false);
});

testWidgets('channel policy becomes less strict -> banner is replaced with the compose box', (tester) async {
final selfUser = eg.user(role: UserRole.moderator);
final channel = eg.stream(streamId: 1, channelPostPolicy: ChannelPostPolicy.administrators);
await prepareComposeBox(tester,
narrow: const ChannelNarrow(1),
selfUser: selfUser,
streams: [channel]);
checkComposeBox(isShown: false);

await changeUserStatus(tester, user: deactivatedUsers[1], isActive: true);
await changeChannelPolicy(tester, channel: channel, policy: ChannelPostPolicy.moderators);
checkComposeBox(isShown: true);
});
});
Expand Down