From 8decc4e84cd2fde910d0ed42b2cd951c4cccdc42 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 21 Jan 2025 17:15:10 -0500 Subject: [PATCH 1/5] store test: Check for data consistency when adding account This covers some of the fields that can be inconsistent (`zulipFeatureLevel` in particular) when a test is not set up properly Signed-off-by: Zixuan James Li --- test/model/test_store.dart | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/model/test_store.dart b/test/model/test_store.dart index 2322e78d7a..b6887cbed7 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -85,6 +85,9 @@ class TestGlobalStore extends GlobalStore { /// [PerAccountStore] when [perAccount] is subsequently called for this /// account, in particular when a [PerAccountStoreWidget] is mounted. Future add(Account account, InitialSnapshot initialSnapshot) async { + assert(initialSnapshot.zulipVersion == account.zulipVersion); + assert(initialSnapshot.zulipMergeBase == account.zulipMergeBase); + assert(initialSnapshot.zulipFeatureLevel == account.zulipFeatureLevel); await insertAccount(account.toCompanion(false)); assert(!_initialSnapshots.containsKey(account.id)); _initialSnapshots[account.id] = initialSnapshot; From 3098349e931085f675a683c55753374481d2247a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 21 Jan 2025 15:36:37 -0500 Subject: [PATCH 2/5] store: Add realmMandatoryTopics realm setting Signed-off-by: Zixuan James Li --- lib/api/model/initial_snapshot.dart | 3 +++ lib/api/model/initial_snapshot.g.dart | 2 ++ lib/model/store.dart | 3 +++ test/example_data.dart | 2 ++ test/model/store_checks.dart | 1 + 5 files changed, 11 insertions(+) diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index efe7a79ebc..0d5e0b50c2 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -64,6 +64,8 @@ class InitialSnapshot { final List? userTopics; // TODO(server-6) + final bool realmMandatoryTopics; + /// 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. @@ -125,6 +127,7 @@ class InitialSnapshot { required this.streams, required this.userSettings, required this.userTopics, + required this.realmMandatoryTopics, required this.realmWaitingPeriodThreshold, required this.realmDefaultExternalAccounts, required this.maxFileUploadSizeMib, diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 445bb8fdb6..454282844c 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -58,6 +58,7 @@ InitialSnapshot _$InitialSnapshotFromJson(Map json) => userTopics: (json['user_topics'] as List?) ?.map((e) => UserTopicItem.fromJson(e as Map)) .toList(), + realmMandatoryTopics: json['realm_mandatory_topics'] as bool, realmWaitingPeriodThreshold: (json['realm_waiting_period_threshold'] as num).toInt(), realmDefaultExternalAccounts: @@ -108,6 +109,7 @@ Map _$InitialSnapshotToJson(InitialSnapshot instance) => 'streams': instance.streams, 'user_settings': instance.userSettings, 'user_topics': instance.userTopics, + 'realm_mandatory_topics': instance.realmMandatoryTopics, 'realm_waiting_period_threshold': instance.realmWaitingPeriodThreshold, 'realm_default_external_accounts': instance.realmDefaultExternalAccounts, 'max_file_upload_size_mib': instance.maxFileUploadSizeMib, diff --git a/lib/model/store.dart b/lib/model/store.dart index 496e373c7f..17629074ac 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -268,6 +268,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess globalStore: globalStore, connection: connection, realmUrl: realmUrl, + realmMandatoryTopics: initialSnapshot.realmMandatoryTopics, realmWaitingPeriodThreshold: initialSnapshot.realmWaitingPeriodThreshold, maxFileUploadSizeMib: initialSnapshot.maxFileUploadSizeMib, realmDefaultExternalAccounts: initialSnapshot.realmDefaultExternalAccounts, @@ -311,6 +312,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess required GlobalStore globalStore, required this.connection, required this.realmUrl, + required this.realmMandatoryTopics, required this.realmWaitingPeriodThreshold, required this.maxFileUploadSizeMib, required this.realmDefaultExternalAccounts, @@ -375,6 +377,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, ChannelStore, Mess Uri? tryResolveUrl(String reference) => _tryResolveUrl(realmUrl, reference); String get zulipVersion => account.zulipVersion; + final bool realmMandatoryTopics; // TODO(#668): update this realm setting /// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold]. final int realmWaitingPeriodThreshold; // TODO(#668): update this realm setting final int maxFileUploadSizeMib; // No event for this. diff --git a/test/example_data.dart b/test/example_data.dart index 0e45321632..2ec9c17dee 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -856,6 +856,7 @@ InitialSnapshot initialSnapshot({ List? streams, UserSettings? userSettings, List? userTopics, + bool? realmMandatoryTopics, int? realmWaitingPeriodThreshold, Map? realmDefaultExternalAccounts, int? maxFileUploadSizeMib, @@ -890,6 +891,7 @@ InitialSnapshot initialSnapshot({ emojiset: Emojiset.google, ), userTopics: userTopics, + realmMandatoryTopics: realmMandatoryTopics ?? true, realmWaitingPeriodThreshold: realmWaitingPeriodThreshold ?? 0, realmDefaultExternalAccounts: realmDefaultExternalAccounts ?? {}, maxFileUploadSizeMib: maxFileUploadSizeMib ?? 25, diff --git a/test/model/store_checks.dart b/test/model/store_checks.dart index 3495aa0359..f2ef63cd4b 100644 --- a/test/model/store_checks.dart +++ b/test/model/store_checks.dart @@ -31,6 +31,7 @@ extension PerAccountStoreChecks on Subject { Subject get isLoading => has((x) => x.isLoading, 'isLoading'); Subject get realmUrl => has((x) => x.realmUrl, 'realmUrl'); Subject get zulipVersion => has((x) => x.zulipVersion, 'zulipVersion'); + Subject get realmMandatoryTopics => has((x) => x.realmMandatoryTopics, 'realmMandatoryTopics'); Subject get maxFileUploadSizeMib => has((x) => x.maxFileUploadSizeMib, 'maxFileUploadSizeMib'); Subject> get realmDefaultExternalAccounts => has((x) => x.realmDefaultExternalAccounts, 'realmDefaultExternalAccounts'); Subject> get customProfileFields => has((x) => x.customProfileFields, 'customProfileFields'); From 9e5f8036fcf1945343ed51b52fe1b50e86260076 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 21 Jan 2025 14:12:43 -0500 Subject: [PATCH 3/5] compose test [nfc]: Remove unused prepareComposeBox param The parameter `realmWaitingPeriodThreshold` was added in commit 157418c26 but it was never used. Signed-off-by: Zixuan James Li --- test/widgets/compose_box_test.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 96bc6cb837..e4e4ffbb3b 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -44,7 +44,6 @@ void main() { Future prepareComposeBox(WidgetTester tester, { required Narrow narrow, User? selfUser, - int? realmWaitingPeriodThreshold, List users = const [], List streams = const [], }) async { @@ -55,8 +54,7 @@ void main() { addTearDown(testBinding.reset); selfUser ??= eg.selfUser; final selfAccount = eg.account(user: selfUser); - await testBinding.globalStore.add(selfAccount, eg.initialSnapshot( - realmWaitingPeriodThreshold: realmWaitingPeriodThreshold)); + await testBinding.globalStore.add(selfAccount, eg.initialSnapshot()); store = await testBinding.globalStore.perAccount(selfAccount.id); From f76b0030e86be13713c6e4016f4c7914779f9423 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 27 Jan 2025 14:59:21 -0500 Subject: [PATCH 4/5] compose test [nfc]: Rename param from user to otherUsers Signed-off-by: Zixuan James Li --- test/widgets/compose_box_test.dart | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index e4e4ffbb3b..4a7d0fd3c9 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -44,7 +44,7 @@ void main() { Future prepareComposeBox(WidgetTester tester, { required Narrow narrow, User? selfUser, - List users = const [], + List otherUsers = const [], List streams = const [], }) async { if (narrow case ChannelNarrow(:var streamId) || TopicNarrow(: var streamId)) { @@ -58,7 +58,7 @@ void main() { store = await testBinding.globalStore.perAccount(selfAccount.id); - await store.addUsers([selfUser, ...users]); + await store.addUsers([selfUser, ...otherUsers]); await store.addStreams(streams); connection = store.connection as FakeApiConnection; @@ -746,7 +746,7 @@ void main() { testWidgets('compose box replaced with a banner', (tester) async { final deactivatedUser = eg.user(isActive: false); await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser), - users: [deactivatedUser]); + otherUsers: [deactivatedUser]); checkComposeBox(isShown: false); }); @@ -754,7 +754,7 @@ void main() { 'compose box is replaced with a banner', (tester) async { final activeUser = eg.user(isActive: true); await prepareComposeBox(tester, narrow: dmNarrowWith(activeUser), - users: [activeUser]); + otherUsers: [activeUser]); checkComposeBox(isShown: true); await changeUserStatus(tester, user: activeUser, isActive: false); @@ -765,7 +765,7 @@ void main() { 'banner is replaced with the compose box', (tester) async { final deactivatedUser = eg.user(isActive: false); await prepareComposeBox(tester, narrow: dmNarrowWith(deactivatedUser), - users: [deactivatedUser]); + otherUsers: [deactivatedUser]); checkComposeBox(isShown: false); await changeUserStatus(tester, user: deactivatedUser, isActive: true); @@ -777,7 +777,7 @@ void main() { 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); + otherUsers: deactivatedUsers); checkComposeBox(isShown: false); }); @@ -785,7 +785,7 @@ void main() { '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); + otherUsers: activeUsers); checkComposeBox(isShown: true); await changeUserStatus(tester, user: activeUsers[0], isActive: false); @@ -796,7 +796,7 @@ void main() { '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); + otherUsers: deactivatedUsers); checkComposeBox(isShown: false); await changeUserStatus(tester, user: deactivatedUsers[0], isActive: true); From f1646c4939685c617e55b82bc7e8badd7b7dd037 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 21 Jan 2025 15:39:26 -0500 Subject: [PATCH 5/5] compose: Respect realm setting for mandatory topics Signed-off-by: Zixuan James Li --- lib/widgets/compose_box.dart | 39 ++++++++++++-------- test/model/autocomplete_test.dart | 3 +- test/widgets/compose_box_test.dart | 59 +++++++++++++++++++++++++++++- 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 750c703085..1233964afb 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -89,13 +89,14 @@ enum TopicValidationError { } class ComposeTopicController extends ComposeController { - ComposeTopicController() { + ComposeTopicController({required this.store}) { _update(); } - // TODO: subscribe to this value: - // https://zulip.com/help/require-topics - final mandatory = true; + PerAccountStore store; + + // TODO(#668): listen to [PerAccountStore] once we subscribe to this value + bool get mandatory => store.realmMandatoryTopics; // TODO(#307) use `max_topic_length` instead of hardcoded limit @override final maxLengthUnicodeCodePoints = kMaxTopicLengthCodePoints; @@ -1227,7 +1228,10 @@ sealed class ComposeBoxController { } class StreamComposeBoxController extends ComposeBoxController { - final topic = ComposeTopicController(); + StreamComposeBoxController({required PerAccountStore store}) + : topic = ComposeTopicController(store: store); + + final ComposeTopicController topic; final topicFocusNode = FocusNode(); @override @@ -1308,16 +1312,20 @@ abstract class ComposeBoxState extends State { ComposeBoxController get controller; } -class _ComposeBoxState extends State implements ComposeBoxState { - @override ComposeBoxController get controller => _controller; - late final ComposeBoxController _controller; +class _ComposeBoxState extends State with PerAccountStoreAwareStateMixin implements ComposeBoxState { + @override ComposeBoxController get controller => _controller!; + ComposeBoxController? _controller; @override - void initState() { - super.initState(); + void onNewStore() { switch (widget.narrow) { case ChannelNarrow(): - _controller = StreamComposeBoxController(); + final store = PerAccountStoreWidget.of(context); + if (_controller == null) { + _controller = StreamComposeBoxController(store: store); + } else { + (controller as StreamComposeBoxController).topic.store = store; + } case TopicNarrow(): case DmNarrow(): _controller = FixedDestinationComposeBoxController(); @@ -1330,7 +1338,7 @@ class _ComposeBoxState extends State implements ComposeBoxState { @override void dispose() { - _controller.dispose(); + controller.dispose(); super.dispose(); } @@ -1370,15 +1378,16 @@ class _ComposeBoxState extends State implements ComposeBoxState { return _ComposeBoxContainer(body: null, errorBanner: errorBanner); } + final controller = this.controller; final narrow = widget.narrow; - switch (_controller) { + switch (controller) { case StreamComposeBoxController(): { narrow as ChannelNarrow; - body = _StreamComposeBoxBody(controller: _controller, narrow: narrow); + body = _StreamComposeBoxBody(controller: controller, narrow: narrow); } case FixedDestinationComposeBoxController(): { narrow as SendableNarrow; - body = _FixedDestinationComposeBoxBody(controller: _controller, narrow: narrow); + body = _FixedDestinationComposeBoxBody(controller: controller, narrow: narrow); } } diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index d6ed9574e0..3d680aca0e 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -835,7 +835,8 @@ void main() { final description = 'topic-input with text: $markedText produces: ${expectedQuery?.raw ?? 'No Query!'}'; test(description, () { - final controller = ComposeTopicController(); + final store = eg.store(); + final controller = ComposeTopicController(store: store); controller.value = parsed.value; if (expectedQuery == null) { check(controller).autocompleteIntent.isNull(); diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 4a7d0fd3c9..a5032f5705 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -46,6 +46,7 @@ void main() { User? selfUser, List otherUsers = const [], List streams = const [], + bool? mandatoryTopics, }) async { if (narrow case ChannelNarrow(:var streamId) || TopicNarrow(: var streamId)) { assert(streams.any((stream) => stream.streamId == streamId), @@ -54,7 +55,9 @@ void main() { addTearDown(testBinding.reset); selfUser ??= eg.selfUser; final selfAccount = eg.account(user: selfUser); - await testBinding.globalStore.add(selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.add(selfAccount, eg.initialSnapshot( + realmMandatoryTopics: mandatoryTopics, + )); store = await testBinding.globalStore.perAccount(selfAccount.id); @@ -558,6 +561,60 @@ void main() { }); }); + group('sending to empty topic', () { + late ZulipStream channel; + + Future setupAndTapSend(WidgetTester tester, { + required String topicInputText, + required bool mandatoryTopics, + }) async { + TypingNotifier.debugEnable = false; + addTearDown(TypingNotifier.debugReset); + + channel = eg.stream(); + final narrow = ChannelNarrow(channel.streamId); + await prepareComposeBox(tester, + narrow: narrow, streams: [channel], + mandatoryTopics: mandatoryTopics); + + await enterTopic(tester, narrow: narrow, topic: topicInputText); + await tester.enterText(contentInputFinder, 'test content'); + await tester.tap(find.byIcon(ZulipIcons.send)); + await tester.pump(); + } + + void checkMessageNotSent(WidgetTester tester) { + check(connection.takeRequests()).isEmpty(); + checkErrorDialog(tester, + expectedTitle: 'Message not sent', + expectedMessage: 'Topics are required in this organization.'); + } + + testWidgets('empty topic -> "(no topic)"', (tester) async { + await setupAndTapSend(tester, + topicInputText: '', + mandatoryTopics: false); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages') + ..bodyFields['topic'].equals('(no topic)'); + }); + + testWidgets('if topics are mandatory, reject empty topic', (tester) async { + await setupAndTapSend(tester, + topicInputText: '', + mandatoryTopics: true); + checkMessageNotSent(tester); + }); + + testWidgets('if topics are mandatory, reject "(no topic)"', (tester) async { + await setupAndTapSend(tester, + topicInputText: '(no topic)', + mandatoryTopics: true); + checkMessageNotSent(tester); + }); + }); + group('uploads', () { void checkAppearsLoading(WidgetTester tester, bool expected) { final sendButtonElement = tester.element(find.ancestor(