do not merge; api: Indicate support for handling empty topics

Look for `allow_empty_topic_name` and `empty_topic_name` under "Feature level 334" in the API Changelog to verify the affected routes: https://zulip.com/api/changelog To keep the API bindings thin, instead of setting `allow_empty_topic_name` for the callers, we require the callers to pass the appropriate values instead. Instead of making this parameter a `bool` that defaults to `false` (and have the bindings remove the field when it's `false`), we type it as `bool?` and only drop it when it is `null`. This is also for making the API binding thin. Fixes: zulip#1250 Signed-off-by: Zixuan James Li <[email protected]>
PIG208 · Feb 26, 2025 · cb1334d · cb1334d
1 parent d67aa53
commit cb1334d
Show file tree

Hide file tree

Showing 10 changed files with 129 additions and 7 deletions.
diff --git a/lib/api/route/channels.dart b/lib/api/route/channels.dart
@@ -7,8 +7,12 @@ part 'channels.g.dart';
 /// https://zulip.com/api/get-stream-topics
 Future<GetStreamTopicsResult> getStreamTopics(ApiConnection connection, {
   required int streamId,
+  bool? allowEmptyTopicName,
 }) {
-  return connection.get('getStreamTopics', GetStreamTopicsResult.fromJson, 'users/me/$streamId/topics', {});
+  assert(allowEmptyTopicName != false, '`allowEmptyTopicName` should only be true or null');
+  return connection.get('getStreamTopics', GetStreamTopicsResult.fromJson, 'users/me/$streamId/topics', {
+    if (allowEmptyTopicName != null) 'allow_empty_topic_name': allowEmptyTopicName,
+  });
 }
 
 @JsonSerializable(fieldRename: FieldRename.snake)

diff --git a/lib/api/route/events.dart b/lib/api/route/events.dart
@@ -18,6 +18,7 @@ Future<InitialSnapshot> registerQueue(ApiConnection connection) {
       'user_avatar_url_field_optional': false, // TODO(#254): turn on
       'stream_typing_notifications': true,
       'user_settings_object': true,
+      'empty_topic_name': true,
     },
   });
 }

diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart
@@ -16,9 +16,11 @@ part 'messages.g.dart';
 Future<Message?> getMessageCompat(ApiConnection connection, {
   required int messageId,
   bool? applyMarkdown,
+  bool? allowEmptyTopicName,
 }) async {
   final useLegacyApi = connection.zulipFeatureLevel! < 120;
   if (useLegacyApi) {
+    assert(allowEmptyTopicName == null);
     final response = await getMessages(connection,
       narrow: [ApiNarrowMessageId(messageId)],
       anchor: NumericAnchor(messageId),
@@ -37,6 +39,7 @@ Future<Message?> getMessageCompat(ApiConnection connection, {
       final response = await getMessage(connection,
         messageId: messageId,
         applyMarkdown: applyMarkdown,
+        allowEmptyTopicName: allowEmptyTopicName,
       );
       return response.message;
     } on ZulipApiException catch (e) {
@@ -57,10 +60,13 @@ Future<Message?> getMessageCompat(ApiConnection connection, {
 Future<GetMessageResult> getMessage(ApiConnection connection, {
   required int messageId,
   bool? applyMarkdown,
+  bool? allowEmptyTopicName,
 }) {
+  assert(allowEmptyTopicName != false, '`allowEmptyTopicName` should only be true or null');
   assert(connection.zulipFeatureLevel! >= 120);
   return connection.get('getMessage', GetMessageResult.fromJson, 'messages/$messageId', {
     if (applyMarkdown != null) 'apply_markdown': applyMarkdown,
+    if (allowEmptyTopicName != null) 'allow_empty_topic_name': allowEmptyTopicName,
   });
 }
 
@@ -88,8 +94,10 @@ Future<GetMessagesResult> getMessages(ApiConnection connection, {
   required int numAfter,
   bool? clientGravatar,
   bool? applyMarkdown,
+  bool? allowEmptyTopicName,
   // bool? useFirstUnreadAnchor // omitted because deprecated
 }) {
+  assert(allowEmptyTopicName != false, '`allowEmptyTopicName` should only be true or null');
   return connection.get('getMessages', GetMessagesResult.fromJson, 'messages', {
     'narrow': resolveApiNarrowForServer(narrow, connection.zulipFeatureLevel!),
     'anchor': RawParameter(anchor.toJson()),
@@ -98,6 +106,7 @@ Future<GetMessagesResult> getMessages(ApiConnection connection, {
     'num_after': numAfter,
     if (clientGravatar != null) 'client_gravatar': clientGravatar,
     if (applyMarkdown != null) 'apply_markdown': applyMarkdown,
+    if (allowEmptyTopicName != null) 'allow_empty_topic_name': allowEmptyTopicName,
   });
 }
 

diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart
@@ -904,7 +904,11 @@ class TopicAutocompleteView extends AutocompleteView<TopicAutocompleteQuery, Top
   Future<void> _fetch() async {
      assert(!_isFetching);
     _isFetching = true;
-    final result = await getStreamTopics(store.connection, streamId: streamId);
+    final result = await getStreamTopics(store.connection, streamId: streamId,
+      allowEmptyTopicName:
+        // TODO(server-10): simplify this condition away
+        store.zulipFeatureLevel >= 334 ? true : null,
+    );
     _topics = result.topics.map((e) => e.name);
     _isFetching = false;
     return _startSearch();

diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart
@@ -509,6 +509,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       anchor: AnchorCode.newest,
       numBefore: kMessageListFetchBatchSize,
       numAfter: 0,
+      allowEmptyTopicName:
+        // TODO(server-10): simplify this condition away
+        store.zulipFeatureLevel >= 334 ? true : null,
     );
     if (this.generation > generation) return;
     _adjustNarrowForTopicPermalink(result.messages.firstOrNull);
@@ -581,6 +584,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
           includeAnchor: false,
           numBefore: kMessageListFetchBatchSize,
           numAfter: 0,
+          allowEmptyTopicName:
+            // TODO(server-10): simplify this condition away
+            store.zulipFeatureLevel >= 334 ? true : null,
         );
       } catch (e) {
         hasFetchError = true;

diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart
@@ -693,9 +693,12 @@ Future<String?> fetchRawContentWithFeedback({
     //   On final failure or success, auto-dismiss the snackbar.
     final zulipLocalizations = ZulipLocalizations.of(context);
     try {
-      fetchedMessage = await getMessageCompat(PerAccountStoreWidget.of(context).connection,
+      final store = PerAccountStoreWidget.of(context);
+      fetchedMessage = await getMessageCompat(store.connection,
         messageId: messageId,
         applyMarkdown: false,
+        // TODO(server-10): simplify this condition away
+        allowEmptyTopicName: store.zulipFeatureLevel >= 334 ? true : null,
       );
       if (fetchedMessage == null) {
         errorMessage = zulipLocalizations.errorMessageDoesNotSeemToExist;

diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart
@@ -20,10 +20,12 @@ void main() {
       required bool expectLegacy,
       required int messageId,
       bool? applyMarkdown,
+      bool? allowEmptyTopicName,
     }) async {
       final result = await getMessageCompat(connection,
         messageId: messageId,
         applyMarkdown: applyMarkdown,
+        allowEmptyTopicName: allowEmptyTopicName,
       );
       if (expectLegacy) {
         check(connection.lastRequest).isA<http.Request>()
@@ -43,6 +45,8 @@ void main() {
           ..url.path.equals('/api/v1/messages/$messageId')
           ..url.queryParameters.deepEquals({
             if (applyMarkdown != null) 'apply_markdown': applyMarkdown.toString(),
+            if (allowEmptyTopicName != null)
+              'allow_empty_topic_name': allowEmptyTopicName.toString(),
           });
       }
       return result;
@@ -57,6 +61,7 @@ void main() {
           expectLegacy: false,
           messageId: message.id,
           applyMarkdown: true,
+          allowEmptyTopicName: true,
         );
         check(result).isNotNull().jsonEquals(message);
       });
@@ -71,6 +76,7 @@ void main() {
           expectLegacy: false,
           messageId: message.id,
           applyMarkdown: true,
+          allowEmptyTopicName: true,
         );
         check(result).isNull();
       });
@@ -92,6 +98,7 @@ void main() {
           expectLegacy: true,
           messageId: message.id,
           applyMarkdown: true,
+          allowEmptyTopicName: null,
         );
         check(result).isNotNull().jsonEquals(message);
       });
@@ -113,6 +120,7 @@ void main() {
           expectLegacy: true,
           messageId: message.id,
           applyMarkdown: true,
+          allowEmptyTopicName: null,
         );
         check(result).isNull();
       });
@@ -124,11 +132,13 @@ void main() {
       FakeApiConnection connection, {
       required int messageId,
       bool? applyMarkdown,
+      bool? allowEmptyTopicName,
       required Map<String, String> expected,
     }) async {
       final result = await getMessage(connection,
         messageId: messageId,
         applyMarkdown: applyMarkdown,
+        allowEmptyTopicName: allowEmptyTopicName,
       );
       check(connection.lastRequest).isA<http.Request>()
         ..method.equals('GET')
@@ -159,6 +169,16 @@ void main() {
       });
     });
 
+    test('allow empty topic name', () {
+      return FakeApiConnection.with_((connection) async {
+        connection.prepare(json: fakeResult.toJson());
+        await checkGetMessage(connection,
+          messageId: 1,
+          allowEmptyTopicName: true,
+          expected: {'allow_empty_topic_name': 'true'});
+      });
+    });
+
     test('Throws assertion error when FL <120', () {
       return FakeApiConnection.with_(zulipFeatureLevel: 119, (connection) async {
         connection.prepare(json: fakeResult.toJson());
@@ -255,12 +275,14 @@ void main() {
       required int numAfter,
       bool? clientGravatar,
       bool? applyMarkdown,
+      bool? allowEmptyTopicName,
       required Map<String, String> expected,
     }) async {
       final result = await getMessages(connection,
         narrow: narrow, anchor: anchor, includeAnchor: includeAnchor,
         numBefore: numBefore, numAfter: numAfter,
         clientGravatar: clientGravatar, applyMarkdown: applyMarkdown,
+        allowEmptyTopicName: allowEmptyTopicName,
       );
       check(connection.lastRequest).isA<http.Request>()
         ..method.equals('GET')
@@ -279,11 +301,13 @@ void main() {
         await checkGetMessages(connection,
           narrow: const CombinedFeedNarrow().apiEncode(),
           anchor: AnchorCode.newest, numBefore: 10, numAfter: 20,
+          allowEmptyTopicName: true,
           expected: {
             'narrow': jsonEncode([]),
             'anchor': 'newest',
             'num_before': '10',
             'num_after': '20',
+            'allow_empty_topic_name': 'true',
           });
       });
     });

diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart
@@ -3,6 +3,7 @@ import 'dart:convert';
 
 import 'package:checks/checks.dart';
 import 'package:flutter/widgets.dart';
+import 'package:http/http.dart' as http;
 import 'package:test/scaffolding.dart';
 import 'package:zulip/api/model/initial_snapshot.dart';
 import 'package:zulip/api/model/model.dart';
@@ -19,6 +20,7 @@ import 'package:zulip/widgets/compose_box.dart';
 import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
 import '../fake_async.dart';
+import '../stdlib_checks.dart';
 import 'test_store.dart';
 import 'autocomplete_checks.dart';
 
@@ -1026,6 +1028,38 @@ void main() {
     check(done).isTrue();
   });
 
+  test('TopicAutocompleteView allow empty topic name on modern servers', () async {
+    final account = eg.account(user: eg.selfUser, zulipFeatureLevel: 334);
+    final store = eg.store(account: account, initialSnapshot: eg.initialSnapshot(
+      zulipFeatureLevel: 334));
+    final connection = store.connection as FakeApiConnection;
+
+    connection.prepare(json: GetStreamTopicsResult(
+      topics: [eg.getStreamTopicsEntry(name: '')],
+    ).toJson());
+    TopicAutocompleteView.init(store: store, streamId: 1000,
+      query: TopicAutocompleteQuery('foo'));
+    check(connection.lastRequest).isA<http.Request>()
+      ..url.path.equals('/api/v1/users/me/1000/topics')
+      ..url.queryParameters['allow_empty_topic_name'].equals('true');
+  });
+
+  test('TopicAutocompleteView disallow empty topic name on legacy servers', () async {
+    final account = eg.account(user: eg.selfUser, zulipFeatureLevel: 333);
+    final store = eg.store(account: account, initialSnapshot: eg.initialSnapshot(
+      zulipFeatureLevel: 333));
+    final connection = store.connection as FakeApiConnection;
+
+    connection.prepare(json: GetStreamTopicsResult(
+      topics: [eg.getStreamTopicsEntry(name: 'foo')],
+    ).toJson());
+    TopicAutocompleteView.init(store: store, streamId: 1000,
+      query: TopicAutocompleteQuery('foo'));
+    check(connection.lastRequest).isA<http.Request>()
+      ..url.path.equals('/api/v1/users/me/1000/topics')
+      ..url.queryParameters.isEmpty();
+  });
+
   group('TopicAutocompleteQuery.testTopic', () {
     final store = eg.store();
     void doCheck(String rawQuery, String topic, bool expected) {

diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart
@@ -49,7 +49,10 @@ void main() {
   Future<void> prepare({Narrow narrow = const CombinedFeedNarrow()}) async {
     final stream = eg.stream(streamId: eg.defaultStreamMessageStreamId);
     subscription = eg.subscription(stream);
-    store = eg.store();
+    final account = eg.account(user: eg.selfUser,
+      zulipFeatureLevel: eg.futureZulipFeatureLevel);
+    store = eg.store(account: account, initialSnapshot: eg.initialSnapshot(
+      zulipFeatureLevel: eg.futureZulipFeatureLevel));
     await store.addStream(stream);
     await store.addSubscription(subscription);
     connection = store.connection as FakeApiConnection;
@@ -82,6 +85,7 @@ void main() {
     bool? includeAnchor,
     required int numBefore,
     required int numAfter,
+    bool? allowEmptyTopicName,
   }) {
     check(connection.lastRequest).isA<http.Request>()
       ..method.equals('GET')
@@ -92,6 +96,8 @@ void main() {
         if (includeAnchor != null) 'include_anchor': includeAnchor.toString(),
         'num_before': numBefore.toString(),
         'num_after': numAfter.toString(),
+        if (allowEmptyTopicName != null)
+          'allow_empty_topic_name': allowEmptyTopicName.toString(),
       });
   }
 
@@ -126,6 +132,7 @@ void main() {
           anchor: 'newest',
           numBefore: kMessageListFetchBatchSize,
           numAfter: 0,
+          allowEmptyTopicName: true,
         );
       }
 
@@ -238,6 +245,7 @@ void main() {
         includeAnchor: false,
         numBefore: kMessageListFetchBatchSize,
         numAfter: 0,
+        allowEmptyTopicName: true,
       );
     });
 

diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart
@@ -50,12 +50,16 @@ late FakeApiConnection connection;
 Future<void> setupToMessageActionSheet(WidgetTester tester, {
   required Message message,
   required Narrow narrow,
+  int? zulipFeatureLevel,
 }) async {
   addTearDown(testBinding.reset);
   assert(narrow.containsMessage(message));
 
-  await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
-  store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+  final account = eg.account(user: eg.selfUser,
+    zulipFeatureLevel: zulipFeatureLevel);
+  await testBinding.globalStore.add(account, eg.initialSnapshot(
+    zulipFeatureLevel: zulipFeatureLevel));
+  store = await testBinding.globalStore.perAccount(account.id);
   await store.addUsers([
     eg.selfUser,
     eg.user(userId: message.senderId),
@@ -71,7 +75,7 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
 
   connection.prepare(json: eg.newestGetMessagesResult(
     foundOldest: true, messages: [message]).toJson());
-  await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
+  await tester.pumpWidget(TestZulipApp(accountId: account.id,
     child: MessageListPage(initNarrow: narrow)));
 
   // global store, per-account store, and message list get loaded
@@ -911,6 +915,31 @@ void main() {
         await setupToMessageActionSheet(tester, message: message, narrow: const StarredMessagesNarrow());
         check(findQuoteAndReplyButton(tester)).isNull();
       });
+
+      testWidgets('handle empty topic', (tester) async {
+        final message = eg.streamMessage();
+        await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message),
+          zulipFeatureLevel: 334);
+
+        prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world');
+        await tapQuoteAndReplyButton(tester);
+        check(connection.lastRequest).isA<http.Request>()
+          .url.queryParameters['allow_empty_topic_name'].equals('true');
+        await tester.pump(Duration.zero);
+      });
+
+      testWidgets('legacy: handle empty topic', (tester) async {
+        final message = eg.streamMessage();
+        await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message),
+          zulipFeatureLevel: 333);
+
+        prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world');
+        await tapQuoteAndReplyButton(tester);
+        check(connection.lastRequest).isA<http.Request>()
+          .url.queryParameters
+            .not((it) => it.containsKey('allow_empty_topic_name'));
+        await tester.pump(Duration.zero);
+      });
     });
 
     group('MarkAsUnread', () {