diff --git a/lib/api/core.dart b/lib/api/core.dart index 8a9ce60d43..5d792ac272 100644 --- a/lib/api/core.dart +++ b/lib/api/core.dart @@ -24,6 +24,7 @@ class ApiConnection { /// For talking to a live server, use [ApiConnection.live]. ApiConnection({ required this.realmUrl, + required this.zulipFeatureLevel, // required even though nullable; see field doc String? email, String? apiKey, required http.Client client, @@ -34,11 +35,31 @@ class ApiConnection { _client = client; /// Construct an API connection that talks to a live Zulip server over the real network. - ApiConnection.live({required Uri realmUrl, String? email, String? apiKey}) - : this(realmUrl: realmUrl, email: email, apiKey: apiKey, client: http.Client()); + ApiConnection.live({ + required Uri realmUrl, + required int? zulipFeatureLevel, // required even though nullable; see field doc + String? email, + String? apiKey, + }) : this(client: http.Client(), + realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel, + email: email, apiKey: apiKey); final Uri realmUrl; + /// The server's last known Zulip feature level, if any. + /// + /// Individual route/endpoint bindings may use this to adapt + /// for compatibility with older servers. + /// + /// If this is null, this [ApiConnection] may be used only for the + /// [getServerSettings] route. Calls to other routes may throw an exception. + /// Constructors therefore require this as a parameter, so that a null value + /// must be passed explicitly. + /// + /// See: + /// * API docs at . + int? zulipFeatureLevel; + final String? _authValue; void addAuth(http.BaseRequest request) { diff --git a/lib/api/model/narrow.dart b/lib/api/model/narrow.dart index 2ff19fd865..9da7827618 100644 --- a/lib/api/model/narrow.dart +++ b/lib/api/model/narrow.dart @@ -47,15 +47,53 @@ class ApiNarrowTopic extends ApiNarrowElement { } /// An [ApiNarrowElement] with the 'dm', or legacy 'pm-with', operator. +/// +/// An instance directly of this class must not be serialized with [jsonEncode], +/// and more generally its [operator] getter must not be called. +/// Instead, call [resolve] and use the object it returns. class ApiNarrowDm extends ApiNarrowElement { - @override String get operator => 'pm-with'; // TODO(#146): use 'dm' where possible + @override String get operator { + assert(false, + "The [operator] getter was called on a plain [ApiNarrowDm]. " + "Before passing to [jsonEncode] or otherwise getting [operator], " + "the [ApiNarrowDm] must be replaced by the result of [ApiNarrowDm.resolve]." + ); + return "dm"; + } @override final List operand; ApiNarrowDm(this.operand, {super.negated}); - factory ApiNarrowDm.fromJson(Map json) => ApiNarrowDm( - (json['operand'] as List).map((e) => e as int).toList(), - negated: json['negated'] as bool? ?? false, - ); + factory ApiNarrowDm.fromJson(Map json) { + var operand = (json['operand'] as List).map((e) => e as int).toList(); + var negated = json['negated'] as bool? ?? false; + return (json['operator'] == 'pm-with') + ? ApiNarrowPmWith._(operand, negated: negated) + : ApiNarrowDmModern._(operand, negated: negated); + } + + /// This element resolved, as either an [ApiNarrowDmModern] or an [ApiNarrowPmWith]. + ApiNarrowDm resolve({required bool legacy}) { + return legacy ? ApiNarrowPmWith._(operand, negated: negated) + : ApiNarrowDmModern._(operand, negated: negated); + } +} + +/// An [ApiNarrowElement] with the 'dm' operator (and not the legacy 'pm-with'). +/// +/// To construct one of these, use [ApiNarrowDm.resolve]. +class ApiNarrowDmModern extends ApiNarrowDm { + @override String get operator => 'dm'; + + ApiNarrowDmModern._(super.operand, {super.negated}); +} + +/// An [ApiNarrowElement] with the legacy 'pm-with' operator. +/// +/// To construct one of these, use [ApiNarrowDm.resolve]. +class ApiNarrowPmWith extends ApiNarrowDm { + @override String get operator => 'pm-with'; + + ApiNarrowPmWith._(super.operand, {super.negated}); } diff --git a/lib/api/route/account.dart b/lib/api/route/account.dart index 75536324b3..e678932485 100644 --- a/lib/api/route/account.dart +++ b/lib/api/route/account.dart @@ -7,11 +7,12 @@ part 'account.g.dart'; /// https://zulip.com/api/fetch-api-key Future fetchApiKey({ required Uri realmUrl, + required int? zulipFeatureLevel, required String username, required String password, }) async { // TODO make this function testable by taking ApiConnection from caller - final connection = ApiConnection.live(realmUrl: realmUrl); + final connection = ApiConnection.live(realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel); try { return await connection.post('fetchApiKey', FetchApiKeyResult.fromJson, 'fetch_api_key', { 'username': RawParameter(username), diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index 31b293d299..b81d9ac015 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -17,6 +17,13 @@ Future getMessages(ApiConnection connection, { bool? applyMarkdown, // bool? useFirstUnreadAnchor // omitted because deprecated }) { + if (narrow.any((element) => element is ApiNarrowDm)) { + final supportsOperatorDm = connection.zulipFeatureLevel! >= 177; // TODO(server-7) + narrow = narrow.map((element) => switch (element) { + ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm), + _ => element, + }).toList(); + } return connection.get('getMessages', GetMessagesResult.fromJson, 'messages', { 'narrow': narrow, 'anchor': switch (anchor) { @@ -99,13 +106,14 @@ Future sendMessage( String? queueId, String? localId, }) { + final supportsTypeDirect = connection.zulipFeatureLevel! >= 174; // TODO(server-7) return connection.post('sendMessage', SendMessageResult.fromJson, 'messages', { if (destination is StreamDestination) ...{ 'type': RawParameter('stream'), 'to': destination.streamId, 'topic': RawParameter(destination.topic), } else if (destination is DmDestination) ...{ - 'type': RawParameter('private'), // TODO(#146): use 'direct' where possible + 'type': supportsTypeDirect ? RawParameter('direct') : RawParameter('private'), 'to': destination.userIds, } else ...( throw Exception('impossible destination') // TODO(dart-3) show this statically diff --git a/lib/api/route/realm.dart b/lib/api/route/realm.dart index 34d349a382..890709316e 100644 --- a/lib/api/route/realm.dart +++ b/lib/api/route/realm.dart @@ -17,9 +17,10 @@ part 'realm.g.dart'; // https://github.com/zulip/zulip-flutter/pull/55#discussion_r1160267577 Future getServerSettings({ required Uri realmUrl, + required int? zulipFeatureLevel, }) async { // TODO make this function testable by taking ApiConnection from caller - final connection = ApiConnection.live(realmUrl: realmUrl); + final connection = ApiConnection.live(realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel); try { return await connection.get('getServerSettings', GetServerSettingsResult.fromJson, 'server_settings', null); } finally { diff --git a/lib/model/store.dart b/lib/model/store.dart index b975fd06fd..1b194db433 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -161,10 +161,10 @@ class PerAccountStore extends ChangeNotifier { maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib; final Account account; - final ApiConnection connection; + final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events // TODO(#135): Keep all this data updated by handling Zulip events from the server. - final String zulipVersion; + final String zulipVersion; // TODO get from account; update there on initial snapshot final Map users; final Map streams; final Map subscriptions; @@ -344,7 +344,8 @@ class LivePerAccountStore extends PerAccountStore { /// In the future this might load an old snapshot from local storage first. static Future load(Account account) async { final connection = ApiConnection.live( - realmUrl: account.realmUrl, email: account.email, apiKey: account.apiKey); + realmUrl: account.realmUrl, zulipFeatureLevel: account.zulipFeatureLevel, + email: account.email, apiKey: account.apiKey); final stopwatch = Stopwatch()..start(); final initialSnapshot = await registerQueue(connection); // TODO retry diff --git a/lib/widgets/login.dart b/lib/widgets/login.dart index f090223b4e..9af391e9d1 100644 --- a/lib/widgets/login.dart +++ b/lib/widgets/login.dart @@ -150,7 +150,7 @@ class _AddAccountPageState extends State { try { final GetServerSettingsResult serverSettings; try { - serverSettings = await getServerSettings(realmUrl: url!); + serverSettings = await getServerSettings(realmUrl: url!, zulipFeatureLevel: null); } catch (e) { if (!context.mounted) { return; @@ -255,7 +255,9 @@ class _PasswordLoginPageState extends State { Future _getUserId(FetchApiKeyResult fetchApiKeyResult) async { final FetchApiKeyResult(:email, :apiKey) = fetchApiKeyResult; final connection = ApiConnection.live( // TODO make this widget testable - realmUrl: widget.serverSettings.realmUri, email: email, apiKey: apiKey); + realmUrl: widget.serverSettings.realmUri, + zulipFeatureLevel: widget.serverSettings.zulipFeatureLevel, + email: email, apiKey: apiKey); return (await getOwnUser(connection)).userId; } @@ -279,7 +281,9 @@ class _PasswordLoginPageState extends State { final FetchApiKeyResult result; try { result = await fetchApiKey( - realmUrl: realmUrl, username: username, password: password); + realmUrl: realmUrl, + zulipFeatureLevel: widget.serverSettings.zulipFeatureLevel, + username: username, password: password); } on ApiRequestException catch (e) { if (!context.mounted) return; // TODO(#105) give more helpful feedback. The RN app is diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 20b31c9ed1..11a8fc6432 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -84,18 +84,30 @@ class FakeHttpClient extends http.BaseClient { /// An [ApiConnection] that accepts and replays canned responses, for testing. class FakeApiConnection extends ApiConnection { - FakeApiConnection({Uri? realmUrl}) - : this._(realmUrl: realmUrl ?? eg.realmUrl, client: FakeHttpClient()); + /// Construct an [ApiConnection] that accepts and replays canned responses, for testing. + /// + /// If `zulipFeatureLevel` is omitted, it defaults to [eg.futureZulipFeatureLevel], + /// which causes route bindings to behave as they would for the + /// latest Zulip server versions. + /// To set `zulipFeatureLevel` to null, pass null explicitly. + FakeApiConnection({Uri? realmUrl, int? zulipFeatureLevel = eg.futureZulipFeatureLevel}) + : this._( + realmUrl: realmUrl ?? eg.realmUrl, + zulipFeatureLevel: zulipFeatureLevel, + client: FakeHttpClient(), + ); FakeApiConnection.fromAccount(Account account) : this._( realmUrl: account.realmUrl, + zulipFeatureLevel: account.zulipFeatureLevel, email: account.email, apiKey: account.apiKey, client: FakeHttpClient()); FakeApiConnection._({ required super.realmUrl, + required super.zulipFeatureLevel, super.email, super.apiKey, required this.client, @@ -105,17 +117,24 @@ class FakeApiConnection extends ApiConnection { /// Run the given callback on a fresh [FakeApiConnection], then close it, /// using try/finally. + /// + /// If `zulipFeatureLevel` is omitted, it defaults to [eg.futureZulipFeatureLevel], + /// which causes route bindings to behave as they would for the + /// latest Zulip server versions. + /// To set `zulipFeatureLevel` to null, pass null explicitly. static Future with_( Future Function(FakeApiConnection connection) fn, { Uri? realmUrl, + int? zulipFeatureLevel = eg.futureZulipFeatureLevel, Account? account, }) async { - assert((account == null) || (realmUrl == null)); + assert((account == null) + || (realmUrl == null && zulipFeatureLevel == eg.futureZulipFeatureLevel)); final connection = (account != null) ? FakeApiConnection.fromAccount(account) - : FakeApiConnection(realmUrl: realmUrl); + : FakeApiConnection(realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel); try { - return fn(connection); + return await fn(connection); } finally { connection.close(); } diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index f1c608ae95..f218464ea5 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -3,50 +3,176 @@ import 'dart:convert'; import 'package:checks/checks.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/model/narrow.dart'; import 'package:zulip/api/route/messages.dart'; +import 'package:zulip/model/narrow.dart'; +import '../../example_data.dart' as eg; import '../../stdlib_checks.dart'; import '../fake_api.dart'; import 'route_checks.dart'; void main() { - test('sendMessage to stream', () { - return FakeApiConnection.with_((connection) async { - const streamId = 123; - const topic = 'world'; - const content = 'hello'; - connection.prepare(json: SendMessageResult(id: 42).toJson()); - final result = await sendMessage(connection, - destination: StreamDestination(streamId, topic), content: content); - check(result).id.equals(42); + group('getMessages', () { + Future checkGetMessages( + FakeApiConnection connection, { + required ApiNarrow narrow, + required Anchor anchor, + bool? includeAnchor, + required int numBefore, + required int numAfter, + bool? clientGravatar, + bool? applyMarkdown, + required Map expected, + }) async { + final result = await getMessages(connection, + narrow: narrow, anchor: anchor, includeAnchor: includeAnchor, + numBefore: numBefore, numAfter: numAfter, + clientGravatar: clientGravatar, applyMarkdown: applyMarkdown, + ); check(connection.lastRequest).isNotNull().isA() - ..method.equals('POST') + ..method.equals('GET') ..url.path.equals('/api/v1/messages') - ..bodyFields.deepEquals({ - 'type': 'stream', - 'to': streamId.toString(), - 'topic': topic, - 'content': content, - }); + ..url.queryParameters.deepEquals(expected); + return result; + } + + final fakeResult = GetMessagesResult( + anchor: 12345, foundNewest: false, foundOldest: false, foundAnchor: false, + historyLimited: false, messages: []); + + test('smoke', () { + return FakeApiConnection.with_((connection) async { + connection.prepare(json: fakeResult.toJson()); + await checkGetMessages(connection, + narrow: const AllMessagesNarrow().apiEncode(), + anchor: AnchorCode.newest, numBefore: 10, numAfter: 20, + expected: { + 'narrow': jsonEncode([]), + 'anchor': 'newest', + 'num_before': '10', + 'num_after': '20', + }); + }); + }); + + test('narrow', () { + return FakeApiConnection.with_((connection) async { + Future checkNarrow(ApiNarrow narrow, String expected) async { + connection.prepare(json: fakeResult.toJson()); + await checkGetMessages(connection, + narrow: narrow, + anchor: AnchorCode.newest, numBefore: 10, numAfter: 20, + expected: { + 'narrow': expected, + 'anchor': 'newest', + 'num_before': '10', + 'num_after': '20', + }); + } + + await checkNarrow(const AllMessagesNarrow().apiEncode(), jsonEncode([])); + await checkNarrow(const StreamNarrow(12).apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + ])); + await checkNarrow(const TopicNarrow(12, 'stuff').apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + {'operator': 'topic', 'operand': 'stuff'}, + ])); + + await checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ + {'operator': 'dm', 'operand': [123, 234]}, + ])); + connection.zulipFeatureLevel = 176; + await checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ + {'operator': 'pm-with', 'operand': [123, 234]}, + ])); + connection.zulipFeatureLevel = eg.futureZulipFeatureLevel; + }); + }); + + test('anchor', () { + return FakeApiConnection.with_((connection) async { + Future checkAnchor(Anchor anchor, String expected) async { + connection.prepare(json: fakeResult.toJson()); + await checkGetMessages(connection, + narrow: const AllMessagesNarrow().apiEncode(), + anchor: anchor, numBefore: 10, numAfter: 20, + expected: { + 'narrow': jsonEncode([]), + 'anchor': expected, + 'num_before': '10', + 'num_after': '20', + }); + } + + await checkAnchor(AnchorCode.newest, 'newest'); + await checkAnchor(AnchorCode.oldest, 'oldest'); + await checkAnchor(AnchorCode.firstUnread, 'first_unread'); + await checkAnchor(const NumericAnchor(1), '1'); + await checkAnchor(const NumericAnchor(999999999), '999999999'); + await checkAnchor(const NumericAnchor(10000000000000000), '10000000000000000'); + }); }); }); - test('sendMessage to DM conversation', () { - return FakeApiConnection.with_((connection) async { - const userIds = [23, 34]; - const content = 'hi there'; + group('sendMessage', () { + const streamId = 123; + const content = 'hello'; + const topic = 'world'; + const userIds = [23, 34]; + + Future checkSendMessage( + FakeApiConnection connection, { + required MessageDestination destination, + required String content, + required Map expectedBodyFields, + }) async { connection.prepare(json: SendMessageResult(id: 42).toJson()); final result = await sendMessage(connection, - destination: DmDestination(userIds: userIds), content: content); + destination: destination, content: content); check(result).id.equals(42); check(connection.lastRequest).isNotNull().isA() ..method.equals('POST') ..url.path.equals('/api/v1/messages') - ..bodyFields.deepEquals({ - 'type': 'private', - 'to': jsonEncode(userIds), - 'content': content, - }); + ..bodyFields.deepEquals(expectedBodyFields); + } + + test('to stream', () { + return FakeApiConnection.with_((connection) async { + await checkSendMessage(connection, + destination: StreamDestination(streamId, topic), content: content, + expectedBodyFields: { + 'type': 'stream', + 'to': streamId.toString(), + 'topic': topic, + 'content': content, + }); + }); + }); + + test('to DM conversation', () { + return FakeApiConnection.with_((connection) async { + await checkSendMessage(connection, + destination: DmDestination(userIds: userIds), content: content, + expectedBodyFields: { + 'type': 'direct', + 'to': jsonEncode(userIds), + 'content': content, + }); + }); + }); + + test('to DM conversation, with legacy type "private"', () { + return FakeApiConnection.with_(zulipFeatureLevel: 173, (connection) async { + await checkSendMessage(connection, + destination: DmDestination(userIds: userIds), content: content, + expectedBodyFields: { + 'type': 'private', + 'to': jsonEncode(userIds), + 'content': content, + }); + }); }); }); } diff --git a/test/example_data.dart b/test/example_data.dart index 2fb88d9e04..75834edd7d 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -8,6 +8,7 @@ final Uri realmUrl = Uri.parse('https://chat.example/'); const String recentZulipVersion = '6.1'; const int recentZulipFeatureLevel = 164; +const int futureZulipFeatureLevel = 9999; User user({int? userId, String? email, String? fullName}) { return User(