diff --git a/lib/api/model/narrow.dart b/lib/api/model/narrow.dart index 8082ac64ff..641e78de49 100644 --- a/lib/api/model/narrow.dart +++ b/lib/api/model/narrow.dart @@ -6,18 +6,41 @@ part 'narrow.g.dart'; typedef ApiNarrow = List; -/// Resolve any [ApiNarrowDm] elements appropriately. +/// Adapt the given narrow to be sent to the given Zulip server version. /// -/// This encapsulates a server-feature check. -ApiNarrow resolveDmElements(ApiNarrow narrow, int zulipFeatureLevel) { - if (!narrow.any((element) => element is ApiNarrowDm)) { +/// Any elements that take a different name on old vs. new servers +/// will be resolved to the specific name to use. +/// Any elements that are unknown to old servers and can +/// reasonably be omitted will be omitted. +ApiNarrow resolveApiNarrowForServer(ApiNarrow narrow, int zulipFeatureLevel) { + final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7) + final supportsOperatorWith = zulipFeatureLevel >= 271; // TODO(server-9) + + bool hasDmElement = false; + bool hasWithElement = false; + for (final element in narrow) { + switch (element) { + case ApiNarrowDm(): hasDmElement = true; + case ApiNarrowWith(): hasWithElement = true; + default: + } + } + if (!(hasDmElement || (hasWithElement && !supportsOperatorWith))) { return narrow; } - final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7) - return narrow.map((element) => switch (element) { - ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm), - _ => element, - }).toList(); + + final result = []; + for (final element in narrow) { + switch (element) { + case ApiNarrowDm(): + result.add(element.resolve(legacy: !supportsOperatorDm)); + case ApiNarrowWith() when !supportsOperatorWith: + break; // drop unsupported element + default: + result.add(element); + } + } + return result; } /// An element in the list representing a narrow in the Zulip API. @@ -102,7 +125,7 @@ class ApiNarrowTopic extends ApiNarrowElement { /// and more generally its [operator] getter must not be called. /// Instead, call [resolve] and use the object it returns. /// -/// If part of [ApiNarrow] use [resolveDmElements]. +/// If part of [ApiNarrow] use [resolveApiNarrowForServer]. class ApiNarrowDm extends ApiNarrowElement { @override String get operator { assert(false, @@ -150,6 +173,22 @@ class ApiNarrowPmWith extends ApiNarrowDm { ApiNarrowPmWith._(super.operand, {super.negated}); } +/// An [ApiNarrowElement] with the 'with' operator. +/// +/// If part of [ApiNarrow] use [resolveApiNarrowForServer]. +class ApiNarrowWith extends ApiNarrowElement { + @override String get operator => 'with'; + + @override final int operand; + + ApiNarrowWith(this.operand, {super.negated}); + + factory ApiNarrowWith.fromJson(Map json) => ApiNarrowWith( + json['operand'] as int, + negated: json['negated'] as bool? ?? false, + ); +} + class ApiNarrowIs extends ApiNarrowElement { @override String get operator => 'is'; diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index be6728c790..5af312ce45 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -91,7 +91,7 @@ Future getMessages(ApiConnection connection, { // bool? useFirstUnreadAnchor // omitted because deprecated }) { return connection.get('getMessages', GetMessagesResult.fromJson, 'messages', { - 'narrow': resolveDmElements(narrow, connection.zulipFeatureLevel!), + 'narrow': resolveApiNarrowForServer(narrow, connection.zulipFeatureLevel!), 'anchor': RawParameter(anchor.toJson()), if (includeAnchor != null) 'include_anchor': includeAnchor, 'num_before': numBefore, @@ -400,7 +400,7 @@ Future updateMessageFlagsForNarrow(ApiConnect if (includeAnchor != null) 'include_anchor': includeAnchor, 'num_before': numBefore, 'num_after': numAfter, - 'narrow': resolveDmElements(narrow, connection.zulipFeatureLevel!), + 'narrow': resolveApiNarrowForServer(narrow, connection.zulipFeatureLevel!), 'op': RawParameter(op.toJson()), 'flag': RawParameter(flag.toJson()), }); diff --git a/lib/model/internal_link.dart b/lib/model/internal_link.dart index db11115cf3..1290259033 100644 --- a/lib/model/internal_link.dart +++ b/lib/model/internal_link.dart @@ -59,7 +59,7 @@ String? decodeHashComponent(String str) { // you do so by passing the `anchor` param. Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { // TODO(server-7) - final apiNarrow = resolveDmElements( + final apiNarrow = resolveApiNarrowForServer( narrow.apiEncode(), store.connection.zulipFeatureLevel!); final fragment = StringBuffer('narrow'); for (ApiNarrowElement element in apiNarrow) { @@ -86,6 +86,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { fragment.write('${element.operand.join(',')}-$suffix'); case ApiNarrowDm(): assert(false, 'ApiNarrowDm should have been resolved'); + case ApiNarrowWith(): + fragment.write(element.operand.toString()); case ApiNarrowIs(): fragment.write(element.operand.toString()); case ApiNarrowMessageId(): @@ -160,6 +162,7 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { ApiNarrowStream? streamElement; ApiNarrowTopic? topicElement; ApiNarrowDm? dmElement; + ApiNarrowWith? withElement; Set isElementOperands = {}; for (var i = 0; i < segments.length; i += 2) { @@ -188,12 +191,17 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { if (dmIds == null) return null; dmElement = ApiNarrowDm(dmIds, negated: negated); + case _NarrowOperator.with_: + if (withElement != null) return null; + final messageId = int.tryParse(operand, radix: 10); + if (messageId == null) return null; + withElement = ApiNarrowWith(messageId); + case _NarrowOperator.is_: // It is fine to have duplicates of the same [IsOperand]. isElementOperands.add(IsOperand.fromRawString(operand)); case _NarrowOperator.near: // TODO(#82): support for near - case _NarrowOperator.with_: // TODO(#683): support for with continue; case _NarrowOperator.unknown: @@ -202,7 +210,9 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { } if (isElementOperands.isNotEmpty) { - if (streamElement != null || topicElement != null || dmElement != null) return null; + if (streamElement != null || topicElement != null || dmElement != null || withElement != null) { + return null; + } if (isElementOperands.length > 1) return null; switch (isElementOperands.single) { case IsOperand.mentioned: @@ -219,13 +229,14 @@ Narrow? _interpretNarrowSegments(List segments, PerAccountStore store) { return null; } } else if (dmElement != null) { - if (streamElement != null || topicElement != null) return null; + if (streamElement != null || topicElement != null || withElement != null) return null; return DmNarrow.withUsers(dmElement.operand, selfUserId: store.selfUserId); } else if (streamElement != null) { final streamId = streamElement.operand; if (topicElement != null) { - return TopicNarrow(streamId, topicElement.operand); + return TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand); } else { + if (withElement != null) return null; return ChannelNarrow(streamId); } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 670785ac4e..ea120cd72e 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -511,6 +511,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { numAfter: 0, ); if (this.generation > generation) return; + _adjustNarrowForTopicPermalink(result.messages.firstOrNull); store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) for (final message in result.messages) { @@ -524,12 +525,47 @@ class MessageListView with ChangeNotifier, _MessageSequence { notifyListeners(); } + /// Update [narrow] for the result of a "with" narrow (topic permalink) fetch. + /// + /// To avoid an extra round trip, the server handles [ApiNarrowWith] + /// by returning results from the indicated message's current stream/topic + /// (if the user has access), + /// even if that differs from the narrow's stream/topic filters + /// because the message was moved. + /// + /// If such a "redirect" happened, this helper updates the stream and topic + /// in [narrow] to match the message's current conversation. + /// It also removes the "with" component from [narrow] + /// whether or not a redirect happened. + /// + /// See API doc: + /// https://zulip.com/api/construct-narrow#message-ids + void _adjustNarrowForTopicPermalink(Message? someFetchedMessageOrNull) { + final narrow = this.narrow; + if (narrow is! TopicNarrow || narrow.with_ == null) return; + + switch (someFetchedMessageOrNull) { + case null: + // This can't be a redirect; a redirect can't produce an empty result. + // (The server only redirects if the message is accessible to the user, + // and if it is, it'll appear in the result, making it non-empty.) + this.narrow = narrow.sansWith(); + case StreamMessage(): + this.narrow = TopicNarrow.ofMessage(someFetchedMessageOrNull); + case DmMessage(): // TODO(log) + assert(false); + } + } + /// Fetch the next batch of older messages, if applicable. Future fetchOlder() async { if (haveOldest) return; if (fetchingOlder) return; if (fetchOlderCoolingDown) return; assert(fetched); + assert(narrow is! TopicNarrow + // We only intend to send "with" in [fetchInitial]; see there. + || (narrow as TopicNarrow).with_ == null); assert(messages.isNotEmpty); _fetchingOlder = true; _updateEndMarkers(); diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index 9e29808ceb..25b14ff980 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -92,7 +92,7 @@ class ChannelNarrow extends Narrow { } class TopicNarrow extends Narrow implements SendableNarrow { - const TopicNarrow(this.streamId, this.topic); + const TopicNarrow(this.streamId, this.topic, {this.with_}); factory TopicNarrow.ofMessage(StreamMessage message) { return TopicNarrow(message.streamId, message.topic); @@ -100,6 +100,9 @@ class TopicNarrow extends Narrow implements SendableNarrow { final int streamId; final TopicName topic; + final int? with_; + + TopicNarrow sansWith() => TopicNarrow(streamId, topic); @override bool containsMessage(Message message) { @@ -108,22 +111,33 @@ class TopicNarrow extends Narrow implements SendableNarrow { } @override - ApiNarrow apiEncode() => [ApiNarrowStream(streamId), ApiNarrowTopic(topic)]; + ApiNarrow apiEncode() => [ + ApiNarrowStream(streamId), + ApiNarrowTopic(topic), + if (with_ != null) ApiNarrowWith(with_!), + ]; @override StreamDestination get destination => StreamDestination(streamId, topic); @override - String toString() => 'TopicNarrow($streamId, ${topic.displayName})'; + String toString() { + final fields = [ + streamId.toString(), + topic.displayName, + if (with_ != null) 'with: ${with_!}', + ]; + return 'TopicNarrow(${fields.join(', ')})'; + } @override bool operator ==(Object other) { if (other is! TopicNarrow) return false; - return other.streamId == streamId && other.topic == topic; + return other.streamId == streamId && other.topic == topic && other.with_ == with_; } @override - int get hashCode => Object.hash('TopicNarrow', streamId, topic); + int get hashCode => Object.hash('TopicNarrow', streamId, topic, with_); } /// The narrow for a direct-message conversation. diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 48c0d70b59..127bd65c61 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -503,8 +503,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat void _modelChanged() { if (model!.narrow != widget.narrow) { - // A message move event occurred, where propagate mode is - // [PropagateMode.changeAll] or [PropagateMode.changeLater]. + // Either: + // - A message move event occurred, where propagate mode is + // [PropagateMode.changeAll] or [PropagateMode.changeLater]. Or: + // - We fetched a "with" / topic-permalink narrow, and the response + // redirected us to the new location of the operand message ID. widget.onNarrowChanged(model!.narrow); } setState(() { diff --git a/test/api/model/narrow_test.dart b/test/api/model/narrow_test.dart new file mode 100644 index 0000000000..42c991c6e4 --- /dev/null +++ b/test/api/model/narrow_test.dart @@ -0,0 +1,4 @@ +void main() { + // resolveApiNarrowForServer is covered in test/api/route/messages_test.dart, + // in the ApiNarrow.toJson test. +} diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index 0d969f0531..a29a87e24b 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -169,13 +169,20 @@ void main() { }); }); - test('Narrow.toJson', () { + test('ApiNarrow.toJson', () { return FakeApiConnection.with_((connection) async { void checkNarrow(ApiNarrow narrow, String expected) { - narrow = resolveDmElements(narrow, connection.zulipFeatureLevel!); + narrow = resolveApiNarrowForServer(narrow, connection.zulipFeatureLevel!); check(jsonEncode(narrow)).equals(expected); } + checkNarrow(const MentionsNarrow().apiEncode(), jsonEncode([ + {'operator': 'is', 'operand': 'mentioned'}, + ])); + checkNarrow(const StarredMessagesNarrow().apiEncode(), jsonEncode([ + {'operator': 'is', 'operand': 'starred'}, + ])); + checkNarrow(const CombinedFeedNarrow().apiEncode(), jsonEncode([])); checkNarrow(const ChannelNarrow(12).apiEncode(), jsonEncode([ {'operator': 'stream', 'operand': 12}, @@ -184,21 +191,43 @@ void main() { {'operator': 'stream', 'operand': 12}, {'operator': 'topic', 'operand': 'stuff'}, ])); - checkNarrow(const MentionsNarrow().apiEncode(), jsonEncode([ - {'operator': 'is', 'operand': 'mentioned'}, + checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + {'operator': 'topic', 'operand': 'stuff'}, + {'operator': 'with', 'operand': 1}, ])); - checkNarrow(const StarredMessagesNarrow().apiEncode(), jsonEncode([ - {'operator': 'is', 'operand': 'starred'}, + checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ + {'operator': 'dm', 'operand': [123, 234]}, + ])); + checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([ + {'operator': 'dm', 'operand': [123, 234]}, + {'operator': 'with', 'operand': 1}, ])); + connection.zulipFeatureLevel = 270; + checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + {'operator': 'topic', 'operand': 'stuff'}, + ])); checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ {'operator': 'dm', 'operand': [123, 234]}, ])); + checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([ + {'operator': 'dm', 'operand': [123, 234]}, + ])); connection.zulipFeatureLevel = 176; + checkNarrow(eg.topicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([ + {'operator': 'stream', 'operand': 12}, + {'operator': 'topic', 'operand': 'stuff'}, + ])); checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([ {'operator': 'pm-with', 'operand': [123, 234]}, ])); + checkNarrow([ApiNarrowDm([123, 234]), ApiNarrowWith(1)], jsonEncode([ + {'operator': 'pm-with', 'operand': [123, 234]}, + ])); + connection.zulipFeatureLevel = eg.futureZulipFeatureLevel; }); }); @@ -259,7 +288,7 @@ void main() { }); }); - test('narrow uses resolveDmElements to encode', () { + test('narrow uses resolveApiNarrowForServer to encode', () { return FakeApiConnection.with_(zulipFeatureLevel: 176, (connection) async { connection.prepare(json: fakeResult.toJson()); await checkGetMessages(connection, @@ -707,7 +736,7 @@ void main() { }); }); - test('narrow uses resolveDmElements to encode', () { + test('narrow uses resolveApiNarrowForServer to encode', () { return FakeApiConnection.with_(zulipFeatureLevel: 176, (connection) async { connection.prepare(json: mkResult(foundOldest: true).toJson()); await checkUpdateMessageFlagsForNarrow(connection, diff --git a/test/example_data.dart b/test/example_data.dart index a758481f52..04d6723b7a 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -332,8 +332,8 @@ Subscription subscription( /// Useful in test code that mentions a lot of topics in a compact format. TopicName t(String apiName) => TopicName(apiName); -TopicNarrow topicNarrow(int channelId, String topicName) { - return TopicNarrow(channelId, TopicName(topicName)); +TopicNarrow topicNarrow(int channelId, String topicName, {int? with_}) { + return TopicNarrow(channelId, TopicName(topicName), with_: with_); } UserTopicItem userTopicItem( diff --git a/test/model/internal_link_test.dart b/test/model/internal_link_test.dart index eb91e35855..611cc3ece0 100644 --- a/test/model/internal_link_test.dart +++ b/test/model/internal_link_test.dart @@ -284,13 +284,14 @@ void main() { final testCases = [ ('/#narrow/stream/check/topic/test', eg.topicNarrow(1, 'test')), ('/#narrow/stream/mobile/subject/topic/near/378333', eg.topicNarrow(3, 'topic')), - ('/#narrow/stream/mobile/subject/topic/with/1', eg.topicNarrow(3, 'topic')), + ('/#narrow/stream/mobile/subject/topic/with/1', eg.topicNarrow(3, 'topic', with_: 1)), ('/#narrow/stream/mobile/topic/topic/', eg.topicNarrow(3, 'topic')), ('/#narrow/stream/stream/topic/topic/near/1', eg.topicNarrow(5, 'topic')), - ('/#narrow/stream/stream/topic/topic/with/22', eg.topicNarrow(5, 'topic')), + ('/#narrow/stream/stream/topic/topic/with/22', eg.topicNarrow(5, 'topic', with_: 22)), ('/#narrow/stream/stream/subject/topic/near/1', eg.topicNarrow(5, 'topic')), - ('/#narrow/stream/stream/subject/topic/with/333', eg.topicNarrow(5, 'topic')), + ('/#narrow/stream/stream/subject/topic/with/333', eg.topicNarrow(5, 'topic', with_: 333)), ('/#narrow/stream/stream/subject/topic', eg.topicNarrow(5, 'topic')), + ('/#narrow/stream/stream/subject/topic/with/asdf', null), // invalid `with` ]; testExpectedNarrows(testCases, streams: streams); }); @@ -313,7 +314,7 @@ void main() { final testCases = [ ('/#narrow/dm/1,2-group', expectedNarrow), ('/#narrow/dm/1,2-group/near/1', expectedNarrow), - ('/#narrow/dm/1,2-group/with/2', expectedNarrow), + ('/#narrow/dm/1,2-group/with/2', null), ('/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', null), ('/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/4', null), ]; @@ -326,7 +327,7 @@ void main() { final testCases = [ ('/#narrow/pm-with/1,2-group', expectedNarrow), ('/#narrow/pm-with/1,2-group/near/1', expectedNarrow), - ('/#narrow/pm-with/1,2-group/with/2', expectedNarrow), + ('/#narrow/pm-with/1,2-group/with/2', null), ('/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', null), ('/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/3', null), ]; @@ -342,7 +343,7 @@ void main() { ('/#narrow/is/$operand', narrow), ('/#narrow/is/$operand/is/$operand', narrow), ('/#narrow/is/$operand/near/1', narrow), - ('/#narrow/is/$operand/with/2', narrow), + ('/#narrow/is/$operand/with/2', null), ('/#narrow/channel/7-test-here/is/$operand', null), ('/#narrow/channel/check/topic/test/is/$operand', null), ('/#narrow/topic/test/is/$operand', null), diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 2fa97fd5e3..da286f3ef5 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -95,221 +95,269 @@ void main() { }); } - test('fetchInitial', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - connection.prepare(json: newestResult( - foundOldest: false, - messages: List.generate(kMessageListFetchBatchSize, - (i) => eg.streamMessage()), - ).toJson()); - final fetchFuture = model.fetchInitial(); - check(model).fetched.isFalse(); + group('fetchInitial', () { + final someChannel = eg.stream(); + const someTopic = 'some topic'; + + final otherChannel = eg.stream(); + const otherTopic = 'other topic'; + + group('smoke', () { + Future smoke( + Narrow narrow, + Message Function(int i) generateMessages, + ) async { + await prepare(narrow: narrow); + connection.prepare(json: newestResult( + foundOldest: false, + messages: List.generate(kMessageListFetchBatchSize, generateMessages), + ).toJson()); + final fetchFuture = model.fetchInitial(); + check(model).fetched.isFalse(); - checkNotNotified(); - await fetchFuture; - checkNotifiedOnce(); - check(model) - ..messages.length.equals(kMessageListFetchBatchSize) - ..haveOldest.isFalse(); - checkLastRequest( - narrow: narrow.apiEncode(), - anchor: 'newest', - numBefore: kMessageListFetchBatchSize, - numAfter: 0, - ); - }); + checkNotNotified(); + await fetchFuture; + checkNotifiedOnce(); + check(model) + ..messages.length.equals(kMessageListFetchBatchSize) + ..haveOldest.isFalse(); + checkLastRequest( + narrow: narrow.apiEncode(), + anchor: 'newest', + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + ); + } - test('fetchInitial, short history', () async { - await prepare(); - connection.prepare(json: newestResult( - foundOldest: true, - messages: List.generate(30, (i) => eg.streamMessage()), - ).toJson()); - await model.fetchInitial(); - checkNotifiedOnce(); - check(model) - ..messages.length.equals(30) - ..haveOldest.isTrue(); - }); + test('CombinedFeedNarrow', () async { + await smoke(const CombinedFeedNarrow(), (i) => eg.streamMessage()); + }); - test('fetchInitial, no messages found', () async { - await prepare(); - connection.prepare(json: newestResult( - foundOldest: true, - messages: [], - ).toJson()); - await model.fetchInitial(); - checkNotifiedOnce(); - check(model) - ..fetched.isTrue() - ..messages.isEmpty() - ..haveOldest.isTrue(); - }); + test('TopicNarrow', () async { + await smoke(TopicNarrow(someChannel.streamId, eg.t(someTopic)), + (i) => eg.streamMessage(stream: someChannel, topic: someTopic)); + }); + }); - // TODO(#824): move this test - test('fetchInitial, recent senders track all the messages', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - final messages = [ - eg.streamMessage(), - // Not subscribed to the stream with id 10. - eg.streamMessage(stream: eg.stream(streamId: 10)), - ]; - connection.prepare(json: newestResult( - foundOldest: false, - messages: messages, - ).toJson()); - await model.fetchInitial(); + test('short history', () async { + await prepare(); + connection.prepare(json: newestResult( + foundOldest: true, + messages: List.generate(30, (i) => eg.streamMessage()), + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..messages.length.equals(30) + ..haveOldest.isTrue(); + }); - check(model).messages.length.equals(1); - recent_senders_test.checkMatchesMessages(store.recentSenders, messages); - }); + test('no messages found', () async { + await prepare(); + connection.prepare(json: newestResult( + foundOldest: true, + messages: [], + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model) + ..fetched.isTrue() + ..messages.isEmpty() + ..haveOldest.isTrue(); + }); - test('fetchOlder', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - await prepareMessages(foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + // TODO(#824): move this test + test('recent senders track all the messages', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + final messages = [ + eg.streamMessage(), + // Not subscribed to the stream with id 10. + eg.streamMessage(stream: eg.stream(streamId: 10)), + ]; + connection.prepare(json: newestResult( + foundOldest: false, + messages: messages, + ).toJson()); + await model.fetchInitial(); - connection.prepare(json: olderResult( - anchor: 1000, foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), - ).toJson()); - final fetchFuture = model.fetchOlder(); - checkNotifiedOnce(); - check(model).fetchingOlder.isTrue(); + check(model).messages.length.equals(1); + recent_senders_test.checkMatchesMessages(store.recentSenders, messages); + }); - await fetchFuture; - checkNotifiedOnce(); - check(model) - ..fetchingOlder.isFalse() - ..messages.length.equals(200); - checkLastRequest( - narrow: narrow.apiEncode(), - anchor: '1000', - includeAnchor: false, - numBefore: kMessageListFetchBatchSize, - numAfter: 0, - ); + group('topic permalinks', () { + test('if redirect, we follow it and remove "with" element', () async { + await prepare(narrow: TopicNarrow(someChannel.streamId, eg.t(someTopic), with_: 1)); + connection.prepare(json: newestResult( + foundOldest: false, + messages: [eg.streamMessage(id: 1, stream: otherChannel, topic: otherTopic)], + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model).narrow + .equals(TopicNarrow(otherChannel.streamId, eg.t(otherTopic))); + }); + + test('if no redirect, we still remove "with" element', () async { + await prepare(narrow: TopicNarrow(someChannel.streamId, eg.t(someTopic), with_: 1)); + connection.prepare(json: newestResult( + foundOldest: false, + messages: [eg.streamMessage(id: 1, stream: someChannel, topic: someTopic)], + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + check(model).narrow + .equals(TopicNarrow(someChannel.streamId, eg.t(someTopic))); + }); + }); }); - test('fetchOlder nop when already fetching', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - await prepareMessages(foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + group('fetchOlder', () { + test('smoke', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); - connection.prepare(json: olderResult( - anchor: 1000, foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), - ).toJson()); - final fetchFuture = model.fetchOlder(); - checkNotifiedOnce(); - check(model).fetchingOlder.isTrue(); + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), + ).toJson()); + final fetchFuture = model.fetchOlder(); + checkNotifiedOnce(); + check(model).fetchingOlder.isTrue(); - // Don't prepare another response. - final fetchFuture2 = model.fetchOlder(); - checkNotNotified(); - check(model).fetchingOlder.isTrue(); + await fetchFuture; + checkNotifiedOnce(); + check(model) + ..fetchingOlder.isFalse() + ..messages.length.equals(200); + checkLastRequest( + narrow: narrow.apiEncode(), + anchor: '1000', + includeAnchor: false, + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + ); + }); - await fetchFuture; - await fetchFuture2; - // We must not have made another request, because we didn't - // prepare another response and didn't get an exception. - checkNotifiedOnce(); - check(model) - ..fetchingOlder.isFalse() - ..messages.length.equals(200); - }); + test('nop when already fetching', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); - test('fetchOlder nop when already haveOldest true', () async { - await prepare(narrow: const CombinedFeedNarrow()); - await prepareMessages(foundOldest: true, messages: - List.generate(30, (i) => eg.streamMessage())); - check(model) - ..haveOldest.isTrue() - ..messages.length.equals(30); + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 900 + i)), + ).toJson()); + final fetchFuture = model.fetchOlder(); + checkNotifiedOnce(); + check(model).fetchingOlder.isTrue(); - await model.fetchOlder(); - // We must not have made a request, because we didn't - // prepare a response and didn't get an exception. - checkNotNotified(); - check(model) - ..haveOldest.isTrue() - ..messages.length.equals(30); - }); + // Don't prepare another response. + final fetchFuture2 = model.fetchOlder(); + checkNotNotified(); + check(model).fetchingOlder.isTrue(); - test('fetchOlder nop during backoff', () => awaitFakeAsync((async) async { - final olderMessages = List.generate(5, (i) => eg.streamMessage()); - final initialMessages = List.generate(5, (i) => eg.streamMessage()); - await prepare(narrow: const CombinedFeedNarrow()); - await prepareMessages(foundOldest: false, messages: initialMessages); - check(connection.takeRequests()).single; + await fetchFuture; + await fetchFuture2; + // We must not have made another request, because we didn't + // prepare another response and didn't get an exception. + checkNotifiedOnce(); + check(model) + ..fetchingOlder.isFalse() + ..messages.length.equals(200); + }); - connection.prepare(apiException: eg.apiBadRequest()); - check(async.pendingTimers).isEmpty(); - await check(model.fetchOlder()).throws(); - checkNotified(count: 2); - check(model).fetchOlderCoolingDown.isTrue(); - check(connection.takeRequests()).single; + test('nop when already haveOldest true', () async { + await prepare(narrow: const CombinedFeedNarrow()); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage())); + check(model) + ..haveOldest.isTrue() + ..messages.length.equals(30); - await model.fetchOlder(); - checkNotNotified(); - check(model).fetchOlderCoolingDown.isTrue(); - check(model).fetchingOlder.isFalse(); - check(connection.lastRequest).isNull(); + await model.fetchOlder(); + // We must not have made a request, because we didn't + // prepare a response and didn't get an exception. + checkNotNotified(); + check(model) + ..haveOldest.isTrue() + ..messages.length.equals(30); + }); - // Wait long enough that a first backoff is sure to finish. - async.elapse(const Duration(seconds: 1)); - check(model).fetchOlderCoolingDown.isFalse(); - checkNotifiedOnce(); - check(connection.lastRequest).isNull(); + test('nop during backoff', () => awaitFakeAsync((async) async { + final olderMessages = List.generate(5, (i) => eg.streamMessage()); + final initialMessages = List.generate(5, (i) => eg.streamMessage()); + await prepare(narrow: const CombinedFeedNarrow()); + await prepareMessages(foundOldest: false, messages: initialMessages); + check(connection.takeRequests()).single; - connection.prepare(json: olderResult( - anchor: 1000, foundOldest: false, messages: olderMessages).toJson()); - await model.fetchOlder(); - checkNotified(count: 2); - check(connection.takeRequests()).single; - })); + connection.prepare(apiException: eg.apiBadRequest()); + check(async.pendingTimers).isEmpty(); + await check(model.fetchOlder()).throws(); + checkNotified(count: 2); + check(model).fetchOlderCoolingDown.isTrue(); + check(connection.takeRequests()).single; - test('fetchOlder handles servers not understanding includeAnchor', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - await prepareMessages(foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); + await model.fetchOlder(); + checkNotNotified(); + check(model).fetchOlderCoolingDown.isTrue(); + check(model).fetchingOlder.isFalse(); + check(connection.lastRequest).isNull(); - // The old behavior is to include the anchor message regardless of includeAnchor. - connection.prepare(json: olderResult( - anchor: 1000, foundOldest: false, foundAnchor: true, - messages: List.generate(101, (i) => eg.streamMessage(id: 900 + i)), - ).toJson()); - await model.fetchOlder(); - checkNotified(count: 2); - check(model) - ..fetchingOlder.isFalse() - ..messages.length.equals(200); - }); + // Wait long enough that a first backoff is sure to finish. + async.elapse(const Duration(seconds: 1)); + check(model).fetchOlderCoolingDown.isFalse(); + checkNotifiedOnce(); + check(connection.lastRequest).isNull(); + + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, messages: olderMessages).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(connection.takeRequests()).single; + })); - // TODO(#824): move this test - test('fetchOlder, recent senders track all the messages', () async { - const narrow = CombinedFeedNarrow(); - await prepare(narrow: narrow); - final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); - await prepareMessages(foundOldest: false, messages: initialMessages); + test('handles servers not understanding includeAnchor', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + await prepareMessages(foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 1000 + i))); - final oldMessages = List.generate(10, (i) => eg.streamMessage(id: 89 + i)) - // Not subscribed to the stream with id 10. - ..add(eg.streamMessage(id: 99, stream: eg.stream(streamId: 10))); - connection.prepare(json: olderResult( - anchor: 100, foundOldest: false, - messages: oldMessages, - ).toJson()); - await model.fetchOlder(); + // The old behavior is to include the anchor message regardless of includeAnchor. + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, foundAnchor: true, + messages: List.generate(101, (i) => eg.streamMessage(id: 900 + i)), + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(model) + ..fetchingOlder.isFalse() + ..messages.length.equals(200); + }); - check(model).messages.length.equals(20); - recent_senders_test.checkMatchesMessages(store.recentSenders, - [...initialMessages, ...oldMessages]); + // TODO(#824): move this test + test('recent senders track all the messages', () async { + const narrow = CombinedFeedNarrow(); + await prepare(narrow: narrow); + final initialMessages = List.generate(10, (i) => eg.streamMessage(id: 100 + i)); + await prepareMessages(foundOldest: false, messages: initialMessages); + + final oldMessages = List.generate(10, (i) => eg.streamMessage(id: 89 + i)) + // Not subscribed to the stream with id 10. + ..add(eg.streamMessage(id: 99, stream: eg.stream(streamId: 10))); + connection.prepare(json: olderResult( + anchor: 100, foundOldest: false, + messages: oldMessages, + ).toJson()); + await model.fetchOlder(); + + check(model).messages.length.equals(20); + recent_senders_test.checkMatchesMessages(store.recentSenders, + [...initialMessages, ...oldMessages]); + }); }); test('MessageEvent', () async { diff --git a/test/model/narrow_checks.dart b/test/model/narrow_checks.dart index ce65de854d..df141654dd 100644 --- a/test/model/narrow_checks.dart +++ b/test/model/narrow_checks.dart @@ -16,4 +16,5 @@ extension DmNarrowChecks on Subject { extension TopicNarrowChecks on Subject { Subject get streamId => has((x) => x.streamId, 'streamId'); Subject get topic => has((x) => x.topic, 'topic'); + Subject get with_ => has((x) => x.with_, 'with_'); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 1aa1af81c5..b7384824fd 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -14,6 +14,7 @@ import 'package:zulip/api/model/narrow.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/actions.dart'; import 'package:zulip/model/localizations.dart'; +import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/typing_status.dart'; @@ -56,16 +57,21 @@ void main() { List? users, List? subscriptions, UnreadMessagesSnapshot? unreadMsgs, + int? zulipFeatureLevel, List navObservers = const [], bool skipAssertAccountExists = false, + bool skipPumpAndSettle = false, }) async { TypingNotifier.debugEnable = false; addTearDown(TypingNotifier.debugReset); addTearDown(testBinding.reset); streams ??= subscriptions ??= [eg.subscription(eg.stream(streamId: eg.defaultStreamMessageStreamId))]; - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot( + zulipFeatureLevel ??= eg.recentZulipFeatureLevel; + final selfAccount = eg.selfAccount.copyWith(zulipFeatureLevel: zulipFeatureLevel); + await testBinding.globalStore.add(selfAccount, eg.initialSnapshot( + zulipFeatureLevel: zulipFeatureLevel, streams: streams, subscriptions: subscriptions, unreadMsgs: unreadMsgs)); - store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + store = await testBinding.globalStore.perAccount(selfAccount.id); connection = store.connection as FakeApiConnection; // prepare message list data @@ -78,15 +84,25 @@ void main() { connection.prepare(json: eg.newestGetMessagesResult(foundOldest: foundOldest, messages: messages).toJson()); - await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + await tester.pumpWidget(TestZulipApp(accountId: selfAccount.id, skipAssertAccountExists: skipAssertAccountExists, navigatorObservers: navObservers, child: MessageListPage(initNarrow: narrow))); + if (skipPumpAndSettle) return; // global store, per-account store, and message list get loaded await tester.pumpAndSettle(); } + void checkAppBarChannelTopic(String channelName, String topic) { + final appBarFinder = find.byType(MessageListAppBarTitle); + check(appBarFinder).findsOne(); + check(find.descendant(of: appBarFinder, matching: find.text(channelName))) + .findsOne(); + check(find.descendant(of: appBarFinder, matching: find.text(topic))) + .findsOne(); + } + ScrollController? findMessageListScrollController(WidgetTester tester) { final scrollView = tester.widget(find.byType(CustomScrollView)); return scrollView.controller; @@ -265,6 +281,81 @@ void main() { check(backgroundColor()).isSameColorAs(MessageListTheme.dark.streamMessageBgDefault); }); + group('fetch initial batch of messages', () { + group('topic permalink', () { + final someStream = eg.stream(); + const someTopic = 'some topic'; + + final otherStream = eg.stream(); + const otherTopic = 'other topic'; + + testWidgets('with message move', (tester) async { + final narrow = TopicNarrow(someStream.streamId, eg.t(someTopic), with_: 1); + await setupMessageListPage(tester, + narrow: narrow, + // server sends the /with/ message in its current, different location + messages: [eg.streamMessage(id: 1, stream: otherStream, topic: otherTopic)], + streams: [someStream, otherStream], + skipPumpAndSettle: true); + await tester.pump(); // global store loaded + await tester.pump(); // per-account store loaded + + // Until we learn the conversation was moved, + // we put the link's stream/topic in the app bar. + checkAppBarChannelTopic(someStream.name, someTopic); + + await tester.pumpAndSettle(); // initial message fetch plus anything else + + // When we learn the conversation was moved, + // we put the new stream/topic in the app bar. + checkAppBarChannelTopic(otherStream.name, otherTopic); + + // We followed the move in just one fetch. + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode(narrow.apiEncode()), + 'anchor': AnchorCode.newest.toJson(), + 'num_before': kMessageListFetchBatchSize.toString(), + 'num_after': '0', + }); + }); + + testWidgets('without message move', (tester) async { + final narrow = TopicNarrow(someStream.streamId, eg.t(someTopic), with_: 1); + await setupMessageListPage(tester, + narrow: narrow, + // server sends the /with/ message in its current, different location + messages: [eg.streamMessage(id: 1, stream: someStream, topic: someTopic)], + streams: [someStream], + skipPumpAndSettle: true); + await tester.pump(); // global store loaded + await tester.pump(); // per-account store loaded + + // Until we learn if the conversation was moved, + // we put the link's stream/topic in the app bar. + checkAppBarChannelTopic(someStream.name, someTopic); + + await tester.pumpAndSettle(); // initial message fetch plus anything else + + // There was no move, so we're still showing the same stream/topic. + checkAppBarChannelTopic(someStream.name, someTopic); + + // We only made one fetch. + check(connection.takeRequests()).single.isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode(narrow.apiEncode()), + 'anchor': AnchorCode.newest.toJson(), + 'num_before': kMessageListFetchBatchSize.toString(), + 'num_after': '0', + }); + }); + }); + }); + group('fetch older messages on scroll', () { int? itemCount(WidgetTester tester) => tester.widget(find.byType(CustomScrollView)).semanticChildCount; @@ -779,10 +870,7 @@ void main() { of: find.byType(RecipientHeader), matching: find.text('new topic')).evaluate() ).length.equals(1); - check(find.descendant( - of: find.byType(MessageListAppBarTitle), - matching: find.text('new topic')).evaluate() - ).length.equals(1); + checkAppBarChannelTopic(channel.name, 'new topic'); }); }); @@ -1100,7 +1188,7 @@ void main() { .initNarrow.equals(DmNarrow.withUser(eg.otherUser.userId, selfUserId: eg.selfUser.userId)); await tester.pumpAndSettle(); }); - + testWidgets('does not navigate on tapping recipient header in DmNarrow', (tester) async { final pushedRoutes = >[]; final navObserver = TestNavigatorObserver()