From 020bfcb9bd6dbd94e45a0cf8274e1a46891a2793 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 26 Jul 2024 00:32:21 -0400 Subject: [PATCH 1/9] message: Expect the absence of newTopic for certain moves. As noted in: https://github.com/zulip/zulip-flutter/pull/787#pullrequestreview-2197930076 There is an discrepency between the API docs and the actual server behavior on the presence of the "new_topic" field on the update message event. While it was claimed to be always present when a move occurs, it is actually not the case when the message moves across channels without chaning its topic. Signed-off-by: Zixuan James Li --- lib/model/message.dart | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index e58049710e..2830e5572a 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -167,9 +167,10 @@ class MessageStoreImpl with MessageStore { return; } - if (newTopic == null) { - // The `subject` field (aka newTopic) is documented to be present on moves. - assert(debugLog('Malformed UpdateMessageEvent: move but no newTopic')); // TODO(log) + if (newStreamId == null && newTopic == null) { + // If neither the channel nor topic name changed, nothing moved. + // In that case `orig_subject` (aka origTopic) should have been null. + assert(debugLog('Malformed UpdateMessageEvent: move but no newStreamId or newTopic')); // TODO(log) return; } if (origStreamId == null) { @@ -179,7 +180,7 @@ class MessageStoreImpl with MessageStore { } if (newStreamId == null - && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic)) { + && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!)) { // The topic was only resolved/unresolved. // No change to the messages' editState. return; From b662e1e3239e53eb7cefe78f4aca7855bd2e0105 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 26 Jul 2024 00:20:41 -0400 Subject: [PATCH 2/9] message test [nfc]: Extract prepareOrigMessages. We still repeat origTopic and etc when creating the UpdateMessageEvents. This is necessary because our example data do not fallback to the topic of the provided messages yet. Signed-off-by: Zixuan James Li --- test/model/message_test.dart | 66 +++++++++++++++++++++++++----------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 482d77b2c1..20ccbc970c 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -273,75 +273,101 @@ void main() { }); group('Handle message edit state update', () { - final message = eg.streamMessage(); - final otherMessage = eg.streamMessage(); - - Future sendEvent(Message message, UpdateMessageEvent event) async { + late List origMessages; + + Future prepareOrigMessages({ + required String origTopic, + String? origContent, + }) async { + origMessages = List.generate(2, (i) => eg.streamMessage( + topic: origTopic, + content: origContent, + )); await prepare(); - await prepareMessages([message, otherMessage]); - - await store.handleEvent(event); - checkNotifiedOnce(); + await prepareMessages(origMessages); } test('message not moved update', () async { - await sendEvent(message, eg.updateMessageEditEvent(message)); - check(store).messages[message.id].editState.equals(MessageEditState.edited); - check(store).messages[otherMessage.id].editState.equals(MessageEditState.none); + await prepareOrigMessages(origTopic: 'origTopic'); + await store.handleEvent(eg.updateMessageEditEvent(origMessages[0])); + checkNotifiedOnce(); + check(store).messages[origMessages[0].id].editState.equals(MessageEditState.edited); + check(store).messages[origMessages[1].id].editState.equals(MessageEditState.none); }); test('message topic moved update', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: 'old topic'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: 'old topic', newTopic: 'new topic')); + checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); test('message topic resolved update', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: 'new topic'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: 'new topic', newTopic: '✔ new topic')); + checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); }); test('message topic unresolved update', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: '✔ new topic'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: '✔ new topic', newTopic: 'new topic')); + checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); }); test('message topic both resolved and edited update', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: 'new topic'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: 'new topic', newTopic: '✔ new topic 2')); + checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); test('message topic both unresolved and edited update', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: '✔ new topic'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: '✔ new topic', newTopic: 'new topic 2')); + checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); test('message stream moved update', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: 'topic'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: 'topic', newTopic: 'topic', newStreamId: 20)); + checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); test('message is both moved and updated', () async { - await sendEvent(message, eg.updateMessageMoveEvent([message, otherMessage], + await prepareOrigMessages(origTopic: 'topic', origContent: 'old content'); + await store.handleEvent(eg.updateMessageMoveEvent( + origMessages, origTopic: 'topic', newTopic: 'topic', newStreamId: 20, origContent: 'old content', newContent: 'new content')); - check(store).messages[message.id].editState.equals(MessageEditState.edited); - check(store).messages[otherMessage.id].editState.equals(MessageEditState.moved); + checkNotifiedOnce(); + check(store).messages[origMessages[0].id].editState.equals(MessageEditState.edited); + check(store).messages[origMessages[1].id].editState.equals(MessageEditState.moved); }); }); }); From 31b5c19d4bf2d751b809cdf610faff2dabe3a4d0 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 29 Jul 2024 18:42:56 -0400 Subject: [PATCH 3/9] message test [nfc]: Rename message move test for accuracy. Signed-off-by: Zixuan James Li --- test/model/message_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 20ccbc970c..70f0b14822 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -345,7 +345,7 @@ void main() { check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); - test('message stream moved update', () async { + test('message stream moved without topic change', () async { await prepareOrigMessages(origTopic: 'topic'); await store.handleEvent(eg.updateMessageMoveEvent( origMessages, From 89bba9bda38e4b303cc2c3f6c1a7161d6f01014b Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 9 Jul 2024 16:53:18 -0400 Subject: [PATCH 4/9] test [nfc]: Split eg.updateMessageMoveEvent helper. When testing, in some cases it is more convenient to prepare the original messages with the topic/stream prior to the move or the other way around. To keep the fallback behavior consistent in either cases, we introduce two variants that produce an UpdateMessageEvent suitable for different contexts. Signed-off-by: Zixuan James Li --- test/example_data.dart | 70 ++++++++++++++++++++++++----- test/model/message_test.dart | 42 +++++++---------- test/widgets/message_list_test.dart | 13 +++--- 3 files changed, 83 insertions(+), 42 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index c191aaf8b8..8b385eb4f6 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -463,26 +463,28 @@ UpdateMessageEvent updateMessageEditEvent( ); } -UpdateMessageEvent updateMessageMoveEvent( - List messages, { +UpdateMessageEvent _updateMessageMoveEvent( + List messageIds, { + required int origStreamId, int? newStreamId, - String? origTopic, + required String origTopic, String? newTopic, String? origContent, String? newContent, + required List flags, }) { - assert(messages.isNotEmpty); - final origMessage = messages[0]; - final messageId = origMessage.id; + assert(newTopic != origTopic + || (newStreamId != null && newStreamId != origStreamId)); + assert(messageIds.isNotEmpty); return UpdateMessageEvent( id: 0, userId: selfUser.userId, renderingOnly: false, - messageId: messageId, - messageIds: messages.map((message) => message.id).toList(), - flags: origMessage.flags, + messageId: messageIds.first, + messageIds: messageIds, + flags: flags, editTimestamp: 1234567890, // TODO generate timestamp - origStreamId: origMessage is StreamMessage ? origMessage.streamId : null, + origStreamId: origStreamId, newStreamId: newStreamId, propagateMode: null, origTopic: origTopic, @@ -495,6 +497,54 @@ UpdateMessageEvent updateMessageMoveEvent( ); } +/// An [UpdateMessageEvent] where [origMessages] are moved to somewhere else. +UpdateMessageEvent updateMessageEventMoveFrom({ + required List origMessages, + int? newStreamId, + String? newTopic, + String? newContent, +}) { + assert(origMessages.isNotEmpty); + final origMessage = origMessages.first; + // Only present on content change. + final origContent = (newContent != null) ? origMessage.content : null; + return _updateMessageMoveEvent(origMessages.map((e) => e.id).toList(), + origStreamId: origMessage.streamId, + newStreamId: newStreamId, + origTopic: origMessage.topic, + newTopic: newTopic, + origContent: origContent, + newContent: newContent, + flags: origMessage.flags, + ); +} + +/// An [UpdateMessageEvent] where [newMessages] are moved from somewhere. +UpdateMessageEvent updateMessageEventMoveTo({ + required List newMessages, + int? origStreamId, + String? origTopic, + String? origContent, +}) { + assert(newMessages.isNotEmpty); + final newMessage = newMessages.first; + // Only present on topic move. + final newTopic = (origTopic != null) ? newMessage.topic : null; + // Only present on channel move. + final newStreamId = (origStreamId != null) ? newMessage.streamId : null; + // Only present on content change. + final newContent = (origContent != null) ? newMessage.content : null; + return _updateMessageMoveEvent(newMessages.map((e) => e.id).toList(), + origStreamId: origStreamId ?? newMessage.streamId, + newStreamId: newStreamId, + origTopic: origTopic ?? newMessage.topic, + newTopic: newTopic, + origContent: origContent, + newContent: newContent, + flags: newMessage.flags, + ); +} + UpdateMessageFlagsRemoveEvent updateMessageFlagsRemoveEvent( MessageFlag flag, Iterable messages, { diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 70f0b14822..f179587358 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -297,9 +297,8 @@ void main() { test('message topic moved update', () async { await prepareOrigMessages(origTopic: 'old topic'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: 'old topic', + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, newTopic: 'new topic')); checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); @@ -307,9 +306,8 @@ void main() { test('message topic resolved update', () async { await prepareOrigMessages(origTopic: 'new topic'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: 'new topic', + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, newTopic: '✔ new topic')); checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); @@ -317,9 +315,8 @@ void main() { test('message topic unresolved update', () async { await prepareOrigMessages(origTopic: '✔ new topic'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: '✔ new topic', + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, newTopic: 'new topic')); checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); @@ -327,9 +324,8 @@ void main() { test('message topic both resolved and edited update', () async { await prepareOrigMessages(origTopic: 'new topic'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: 'new topic', + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, newTopic: '✔ new topic 2')); checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); @@ -337,9 +333,8 @@ void main() { test('message topic both unresolved and edited update', () async { await prepareOrigMessages(origTopic: '✔ new topic'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: '✔ new topic', + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, newTopic: 'new topic 2')); checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); @@ -347,23 +342,18 @@ void main() { test('message stream moved without topic change', () async { await prepareOrigMessages(origTopic: 'topic'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: 'topic', - newTopic: 'topic', - newStreamId: 20)); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, + newStreamId: 20)); checkNotifiedOnce(); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); test('message is both moved and updated', () async { await prepareOrigMessages(origTopic: 'topic', origContent: 'old content'); - await store.handleEvent(eg.updateMessageMoveEvent( - origMessages, - origTopic: 'topic', - newTopic: 'topic', - newStreamId: 20, - origContent: 'old content', + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: origMessages, + newStreamId: 20, newContent: 'new content')); checkNotifiedOnce(); check(store).messages[origMessages[0].id].editState.equals(MessageEditState.edited); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 3b13c158e4..d4f77a4491 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -965,8 +965,8 @@ void main() { }); testWidgets('edited and moved messages from events', (WidgetTester tester) async { - final message = eg.streamMessage(); - final message2 = eg.streamMessage(); + final message = eg.streamMessage(topic: 'old'); + final message2 = eg.streamMessage(topic: 'old'); await setupMessageListPage(tester, messages: [message, message2]); checkMarkersCount(edited: 0, moved: 0); @@ -974,8 +974,8 @@ void main() { await tester.pump(); checkMarkersCount(edited: 1, moved: 0); - await store.handleEvent(eg.updateMessageMoveEvent( - [message, message2], origTopic: 'old', newTopic: 'new')); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [message, message2], newTopic: 'new')); await tester.pump(); checkMarkersCount(edited: 1, moved: 1); @@ -1030,8 +1030,9 @@ void main() { final rectsBefore = captureMessageRects(tester, messages, messageWithMarker); checkMarkersCount(edited: 0, moved: 0); - await store.handleEvent(eg.updateMessageMoveEvent( - [messageWithMarker], origTopic: 'old', newTopic: messageWithMarker.topic)); + // TODO(#150): [messageWithMarker]'s topic in store is inconsistent with the event. This will be fixed soon. + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: 'old', newMessages: [messageWithMarker])); await tester.pump(); check(captureMessageRects(tester, messages, messageWithMarker)) .deepEquals(rectsBefore); From a25020fbe8420f2403b9eb60ae2d8a65a42cc92e Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 10 Jul 2024 02:10:38 -0400 Subject: [PATCH 5/9] message [nfc]: Make displayRecipient nullable. We want to only allow it to be nullable after deserialization, because the nullability does not belong to the API, and it exists only for later implementing invalidation of the field when a stream message is moved to a different stream. Signed-off-by: Zixuan James Li --- lib/api/model/model.dart | 6 +- lib/api/model/model.g.dart | 113 +++++++++++++++++-------------- lib/widgets/message_list.dart | 4 +- test/api/model/model_checks.dart | 4 ++ test/api/model/model_test.dart | 9 +++ 5 files changed, 85 insertions(+), 51 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 1a4e35253e..5992c0546d 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -576,7 +576,11 @@ class StreamMessage extends Message { @JsonKey(includeToJson: true) String get type => 'stream'; - final String displayRecipient; + // This is not nullable API-wise, but if the message moves across channels, + // [displayRecipient] still refers to the original channel and it has to be + // invalidated. + @JsonKey(required: true, disallowNullValue: true) + String? displayRecipient; final int streamId; StreamMessage({ diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index a824abba9b..10c1c6b00c 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -260,55 +260,70 @@ Map _$SubscriptionToJson(Subscription instance) => 'color': instance.color, }; -StreamMessage _$StreamMessageFromJson(Map json) => - StreamMessage( - client: json['client'] as String, - content: json['content'] as String, - contentType: json['content_type'] as String, - editState: Message._messageEditStateFromJson( - MessageEditState._readFromMessage(json, 'edit_state')), - id: (json['id'] as num).toInt(), - isMeMessage: json['is_me_message'] as bool, - lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), - reactions: Message._reactionsFromJson(json['reactions']), - recipientId: (json['recipient_id'] as num).toInt(), - senderEmail: json['sender_email'] as String, - senderFullName: json['sender_full_name'] as String, - senderId: (json['sender_id'] as num).toInt(), - senderRealmStr: json['sender_realm_str'] as String, - topic: json['subject'] as String, - timestamp: (json['timestamp'] as num).toInt(), - flags: Message._flagsFromJson(json['flags']), - matchContent: json['match_content'] as String?, - matchTopic: json['match_subject'] as String?, - displayRecipient: json['display_recipient'] as String, - streamId: (json['stream_id'] as num).toInt(), - ); - -Map _$StreamMessageToJson(StreamMessage instance) => - { - 'client': instance.client, - 'content': instance.content, - 'content_type': instance.contentType, - 'edit_state': _$MessageEditStateEnumMap[instance.editState]!, - 'id': instance.id, - 'is_me_message': instance.isMeMessage, - 'last_edit_timestamp': instance.lastEditTimestamp, - 'reactions': Message._reactionsToJson(instance.reactions), - 'recipient_id': instance.recipientId, - 'sender_email': instance.senderEmail, - 'sender_full_name': instance.senderFullName, - 'sender_id': instance.senderId, - 'sender_realm_str': instance.senderRealmStr, - 'subject': instance.topic, - 'timestamp': instance.timestamp, - 'flags': instance.flags, - 'match_content': instance.matchContent, - 'match_subject': instance.matchTopic, - 'type': instance.type, - 'display_recipient': instance.displayRecipient, - 'stream_id': instance.streamId, - }; +StreamMessage _$StreamMessageFromJson(Map json) { + $checkKeys( + json, + requiredKeys: const ['display_recipient'], + disallowNullValues: const ['display_recipient'], + ); + return StreamMessage( + client: json['client'] as String, + content: json['content'] as String, + contentType: json['content_type'] as String, + editState: Message._messageEditStateFromJson( + MessageEditState._readFromMessage(json, 'edit_state')), + id: (json['id'] as num).toInt(), + isMeMessage: json['is_me_message'] as bool, + lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), + reactions: Message._reactionsFromJson(json['reactions']), + recipientId: (json['recipient_id'] as num).toInt(), + senderEmail: json['sender_email'] as String, + senderFullName: json['sender_full_name'] as String, + senderId: (json['sender_id'] as num).toInt(), + senderRealmStr: json['sender_realm_str'] as String, + topic: json['subject'] as String, + timestamp: (json['timestamp'] as num).toInt(), + flags: Message._flagsFromJson(json['flags']), + matchContent: json['match_content'] as String?, + matchTopic: json['match_subject'] as String?, + displayRecipient: json['display_recipient'] as String?, + streamId: (json['stream_id'] as num).toInt(), + ); +} + +Map _$StreamMessageToJson(StreamMessage instance) { + final val = { + 'client': instance.client, + 'content': instance.content, + 'content_type': instance.contentType, + 'edit_state': _$MessageEditStateEnumMap[instance.editState]!, + 'id': instance.id, + 'is_me_message': instance.isMeMessage, + 'last_edit_timestamp': instance.lastEditTimestamp, + 'reactions': Message._reactionsToJson(instance.reactions), + 'recipient_id': instance.recipientId, + 'sender_email': instance.senderEmail, + 'sender_full_name': instance.senderFullName, + 'sender_id': instance.senderId, + 'sender_realm_str': instance.senderRealmStr, + 'subject': instance.topic, + 'timestamp': instance.timestamp, + 'flags': instance.flags, + 'match_content': instance.matchContent, + 'match_subject': instance.matchTopic, + 'type': instance.type, + }; + + void writeNotNull(String key, dynamic value) { + if (value != null) { + val[key] = value; + } + } + + writeNotNull('display_recipient', instance.displayRecipient); + val['stream_id'] = instance.streamId; + return val; +} const _$MessageEditStateEnumMap = { MessageEditState.none: 'none', diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index fca5db5664..b884cd582a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -978,7 +978,9 @@ class StreamMessageRecipientHeader extends StatelessWidget { streamWidget = const SizedBox(width: 16); } else { final stream = store.streams[message.streamId]; - final streamName = stream?.name ?? message.displayRecipient; // TODO(log) if missing + final streamName = stream?.name + ?? message.displayRecipient + ?? '(unknown channel)'; // TODO(log) streamWidget = GestureDetector( onTap: () => Navigator.push(context, diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 436e3569c6..c8261ba78f 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -49,6 +49,10 @@ extension MessageChecks on Subject { Subject get matchTopic => has((e) => e.matchTopic, 'matchTopic'); } +extension StreamMessageChecks on Subject { + Subject get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient'); +} + extension ReactionsChecks on Subject { Subject get total => has((e) => e.total, 'total'); Subject> get aggregated => has((e) => e.aggregated, 'aggregated'); diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 3370fa2a42..351c3f77d8 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -1,6 +1,7 @@ import 'dart:convert'; import 'package:checks/checks.dart'; +import 'package:json_annotation/json_annotation.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/model.dart'; @@ -149,6 +150,14 @@ void main() { check(m2).flags.deepEquals([MessageFlag.read, MessageFlag.unknown]); }); + test('require displayRecipient on parse', () { + check(() => StreamMessage.fromJson(baseStreamJson()..['display_recipient'] = null)) + .throws(); + + check(() => StreamMessage.fromJson(baseStreamJson()..remove('display_recipient'))) + .throws(); + }); + // Code relevant to messageEditState is tested separately in the // MessageEditState group. }); From ce999c49670eebfc6ad350d0a48a7023ee525470 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 3 Jul 2024 16:21:14 -0700 Subject: [PATCH 6/9] message: Handle moved messages from UpdateMessageEvent. We already handle the case where only a message's content is edited. This handles the case where messages are moved too, between topics and/or channels. This introduces more notifyListener calls, and the listeners can be notified more than once per UpdateMessage event. This is expected. --- If the `generation += 1` line is commented out, the message list has a race bug where a fetchOlder starts; we reset (because messages were moved into the narrow); and then the fetch returns and appends in the wrong spot. These races are detailed in the "fetch races" test group. --- StreamMessage.displayRecipient no longer carries the up-to-date display name of the stream when the message has been moved to another stream. We invalidate it by setting it to null. The only time StreamMessage.displayRecipient is useful is when we don't have data on that stream. This is the reason why we don't go ahead and lookup the stream store when such a stream move happen, because the stream store likely will not have that information if we ever need to use displayRecipient as the fallback. --- We have relatively more tests for the topic moves, because there are more cases that make a difference to consider. There is some overlap between the tests like "(channel, topic) -> (new channel, new topic)" and other tests where only one of topic/channel is changed. We include both because the parameterized test cases are easy to modify and it doesn't hurt to cover another realistic scenario handled by the same code path. Co-authored-by: Zixuan James Li Signed-off-by: Zixuan James Li --- lib/api/model/model.dart | 4 +- lib/model/message.dart | 45 +++- lib/model/message_list.dart | 93 ++++++- test/model/message_list_test.dart | 407 ++++++++++++++++++++++++++++++ test/model/message_test.dart | 25 +- 5 files changed, 548 insertions(+), 26 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 5992c0546d..614a784511 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -480,7 +480,7 @@ sealed class Message { final int senderId; final String senderRealmStr; @JsonKey(name: 'subject') - final String topic; + String topic; // final List submessages; // TODO handle final int timestamp; String get type; @@ -581,7 +581,7 @@ class StreamMessage extends Message { // invalidated. @JsonKey(required: true, disallowNullValue: true) String? displayRecipient; - final int streamId; + int streamId; StreamMessage({ required super.client, diff --git a/lib/model/message.dart b/lib/model/message.dart index 2830e5572a..5d30a17ce9 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -179,22 +179,43 @@ class MessageStoreImpl with MessageStore { return; } - if (newStreamId == null - && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!)) { - // The topic was only resolved/unresolved. - // No change to the messages' editState. - return; - } + final wasResolveOrUnresolve = (newStreamId == null + && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!)); - // TODO(#150): Handle message moves. The views' recipient headers - // may need updating, and consequently showSender too. - // Currently only editState gets updated. for (final messageId in event.messageIds) { final message = messages[messageId]; if (message == null) continue; - // Do not override the edited marker if the message has also been moved. - if (message.editState == MessageEditState.edited) continue; - message.editState = MessageEditState.moved; + + if (message is! StreamMessage) { + assert(debugLog('Bad UpdateMessageEvent: stream/topic move on a DM')); // TODO(log) + continue; + } + + if (newStreamId != null) { + message.streamId = newStreamId; + // See [StreamMessage.displayRecipient] on why the invalidation is + // needed. + message.displayRecipient = null; + } + + if (newTopic != null) { + message.topic = newTopic; + } + + if (!wasResolveOrUnresolve + && message.editState == MessageEditState.none) { + message.editState = MessageEditState.moved; + } + } + + for (final view in _messageListViews) { + view.messagesMoved( + origStreamId: origStreamId, + newStreamId: newStreamId ?? origStreamId, + origTopic: origTopic, + newTopic: newTopic ?? origTopic, + messageIds: event.messageIds, + ); } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index fa9dc026c1..29453c2778 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -65,6 +65,9 @@ class MessageListHistoryStartItem extends MessageListItem { /// /// This comprises much of the guts of [MessageListView]. mixin _MessageSequence { + /// A sequence number for invalidating stale fetches. + int generation = 0; + /// The messages. /// /// See also [contents] and [items]. @@ -192,6 +195,17 @@ mixin _MessageSequence { _reprocessAll(); } + /// Reset all [_MessageSequence] data, and cancel any active fetches. + void _reset() { + generation += 1; + messages.clear(); + _fetched = false; + _haveOldest = false; + _fetchingOlder = false; + contents.clear(); + items.clear(); + } + /// Redo all computations from scratch, based on [messages]. void _recompute() { assert(contents.length == messages.length); @@ -398,12 +412,14 @@ class MessageListView with ChangeNotifier, _MessageSequence { assert(!fetched && !haveOldest && !fetchingOlder); assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate + final generation = this.generation; final result = await getMessages(store.connection, narrow: narrow.apiEncode(), anchor: AnchorCode.newest, numBefore: kMessageListFetchBatchSize, numAfter: 0, ); + if (this.generation > generation) return; store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) for (final message in result.messages) { @@ -426,6 +442,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { _fetchingOlder = true; _updateEndMarkers(); notifyListeners(); + final generation = this.generation; try { final result = await getMessages(store.connection, narrow: narrow.apiEncode(), @@ -434,6 +451,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { numBefore: kMessageListFetchBatchSize, numAfter: 0, ); + if (this.generation > generation) return; if (result.messages.isNotEmpty && result.messages.last.id == messages[0].id) { @@ -451,9 +469,11 @@ class MessageListView with ChangeNotifier, _MessageSequence { _insertAllMessages(0, fetchedMessages); _haveOldest = result.foundOldest; } finally { - _fetchingOlder = false; - _updateEndMarkers(); - notifyListeners(); + if (this.generation == generation) { + _fetchingOlder = false; + _updateEndMarkers(); + notifyListeners(); + } } } @@ -489,6 +509,73 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + void _messagesMovedInternally(List messageIds) { + for (final messageId in messageIds) { + if (_findMessageWithId(messageId) != -1) { + _reprocessAll(); + notifyListeners(); + return; + } + } + } + + void _messagesMovedIntoNarrow() { + // If there are some messages we don't have in [MessageStore], and they + // occur later than the messages we have here, then we just have to + // re-fetch from scratch. That's always valid, so just do that always. + // TODO in cases where we do have data to do better, do better. + _reset(); + notifyListeners(); + fetchInitial(); + } + + void _messagesMovedFromNarrow(List messageIds) { + if (_removeMessagesById(messageIds)) { + notifyListeners(); + } + } + + void messagesMoved({ + required int origStreamId, + required int newStreamId, + required String origTopic, + required String newTopic, + required List messageIds, + }) { + switch (narrow) { + case DmNarrow(): + // DMs can't be moved (nor created by moves), + // so the messages weren't in this narrow and still aren't. + return; + + case CombinedFeedNarrow(): + case MentionsNarrow(): + // The messages were and remain in this narrow. + // TODO(#421): … except they may have become muted or not. + // We'll handle that at the same time as we handle muting itself changing. + // Recipient headers, and downstream of those, may change, though. + _messagesMovedInternally(messageIds); + + case ChannelNarrow(:final streamId): + switch ((origStreamId == streamId, newStreamId == streamId)) { + case (false, false): return; + case (true, true ): _messagesMovedInternally(messageIds); + case (false, true ): _messagesMovedIntoNarrow(); + case (true, false): _messagesMovedFromNarrow(messageIds); + } + + case TopicNarrow(:final streamId, :final topic): + final oldMatch = (origStreamId == streamId && origTopic == topic); + final newMatch = (newStreamId == streamId && newTopic == topic); + switch ((oldMatch, newMatch)) { + case (false, false): return; + case (true, true ): return; // TODO(log) no-op move + case (false, true ): _messagesMovedIntoNarrow(); + case (true, false): _messagesMovedFromNarrow(messageIds); // TODO handle propagateMode + } + } + } + // Repeal the `@protected` annotation that applies on the base implementation, // so we can call this method from [MessageStoreImpl]. @override diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 545074a97c..bfcf87a62e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -16,6 +16,7 @@ import 'package:zulip/model/store.dart'; import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; +import '../fake_async.dart'; import '../stdlib_checks.dart'; import 'content_checks.dart'; import 'recent_senders_test.dart' as recent_senders_test; @@ -465,6 +466,412 @@ void main() { }); }); + group('messagesMoved', () { + final stream = eg.stream(streamId: 1, name: 'test stream'); + final otherStream = eg.stream(streamId: 2, name: 'other stream'); + + void checkHasMessages(Iterable messages) { + check(model.messages.map((e) => e.id)).deepEquals(messages.map((e) => e.id)); + } + + Future prepareNarrow(Narrow narrow, List? messages) async { + await prepare(narrow: narrow); + for (final streamToAdd in [stream, otherStream]) { + final subscription = eg.subscription(streamToAdd); + await store.addStream(streamToAdd); + await store.addSubscription(subscription); + } + if (messages != null) { + await prepareMessages(foundOldest: false, messages: messages); + } + checkHasMessages(messages ?? []); + } + + group('in combined feed narrow', () { + const narrow = CombinedFeedNarrow(); + final initialMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + final movedMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + + test('internal move between channels', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: initialMessages, + newTopic: initialMessages[0].topic, + newStreamId: otherStream.streamId, + )); + checkHasMessages(initialMessages); + checkNotified(count: 2); + })); + + test('internal move between topics', () async { + await prepareNarrow(narrow, initialMessages + movedMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopic: 'new', + )); + checkHasMessages(initialMessages + movedMessages); + checkNotified(count: 2); + }); + }); + + group('in channel narrow', () { + final narrow = ChannelNarrow(stream.streamId); + final initialMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + final movedMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + final otherChannelMovedMessages = List.generate(5, (i) => eg.streamMessage(stream: otherStream, topic: 'topic')); + + test('channel -> channel: internal move', () async { + await prepareNarrow(narrow, initialMessages + movedMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopic: 'new', + )); + checkHasMessages(initialMessages + movedMessages); + checkNotified(count: 2); + }); + + test('old channel -> channel: refetch', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: 'orig topic', + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + check(model).fetched.isFalse(); + checkHasMessages([]); + checkNotifiedOnce(); + + async.elapse(const Duration(seconds: 2)); + checkHasMessages(initialMessages + movedMessages); + checkNotifiedOnce(); + })); + + test('channel -> new channel: remove moved messages', () async { + await prepareNarrow(narrow, initialMessages + movedMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopic: 'new', + newStreamId: otherStream.streamId, + )); + checkHasMessages(initialMessages); + checkNotifiedOnce(); + }); + + test('unrelated channel -> new channel: unaffected', () async { + await prepareNarrow(narrow, initialMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: otherChannelMovedMessages, + newStreamId: otherStream.streamId, + )); + checkHasMessages(initialMessages); + checkNotNotified(); + }); + + test('unrelated channel -> unrelated channel: unaffected', () async { + await prepareNarrow(narrow, initialMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: otherChannelMovedMessages, + newTopic: 'new', + )); + checkHasMessages(initialMessages); + checkNotNotified(); + }); + }); + + group('in topic narrow', () { + final narrow = TopicNarrow(stream.streamId, 'topic'); + final initialMessages = List.generate(5, (i) => eg.streamMessage(stream: stream, topic: 'topic')); + final movedMessages = List.generate(5, (i) => eg.streamMessage(stream: stream, topic: 'topic')); + final otherTopicMovedMessages = List.generate(5, (i) => eg.streamMessage(stream: stream, topic: 'other topic')); + final otherChannelMovedMessages = List.generate(5, (i) => eg.streamMessage(stream: otherStream, topic: 'topic')); + + group('moved into narrow: should refetch messages', () { + final testCases = [ + ('(old channel, topic) -> (channel, topic)', 200, null), + ('(channel, old topic) -> (channel, topic)', null, 'other'), + ('(old channel, old topic) -> (channel, topic)', 200, 'other'), + ]; + + for (final (description, origStreamId, origTopic) in testCases) { + test(description, () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origStreamId: origStreamId, + origTopic: origTopic, + newMessages: movedMessages, + )); + check(model).fetched.isFalse(); + checkHasMessages([]); + checkNotifiedOnce(); + + async.elapse(const Duration(seconds: 2)); + checkHasMessages(initialMessages + movedMessages); + checkNotifiedOnce(); + })); + } + }); + + group('moved from narrow: should remove moved messages', () { + final testCases = [ + ('(channel, topic) -> (new channel, topic)', 200, null), + ('(channel, topic) -> (channel, new topic)', null, 'new'), + ('(channel, topic) -> (new channel, new topic)', 200, 'new'), + ]; + + for (final (description, newStreamId, newTopic) in testCases) { + test(description, () async { + await prepareNarrow(narrow, initialMessages + movedMessages); + + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newStreamId: newStreamId, + newTopic: newTopic, + )); + checkHasMessages(initialMessages); + checkNotifiedOnce(); + }); + } + }); + + group('irrelevant moves', () { + test('(channel, old topic) -> (channel, unrelated topic)', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: 'other', + newMessages: otherTopicMovedMessages, + )); + check(model).fetched.isTrue(); + checkHasMessages(initialMessages); + checkNotNotified(); + })); + + test('(old channel, topic) - > (unrelated channel, topic)', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + await store.handleEvent(eg.updateMessageEventMoveTo( + origStreamId: 200, + newMessages: otherChannelMovedMessages, + )); + check(model).fetched.isTrue(); + checkHasMessages(initialMessages); + checkNotNotified(); + })); + }); + }); + + group('fetch races', () { + final narrow = ChannelNarrow(stream.streamId); + final olderMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + final initialMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + final movedMessages = List.generate(5, (i) => eg.streamMessage(stream: stream)); + + test('fetchOlder, _reset, fetchOlder returns, move fetch finishes', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + connection.prepare(delay: const Duration(seconds: 1), json: olderResult( + anchor: model.messages[0].id, + foundOldest: true, + messages: olderMessages, + ).toJson()); + final fetchFuture = model.fetchOlder(); + check(model).fetchingOlder.isTrue(); + checkHasMessages(initialMessages); + checkNotifiedOnce(); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: movedMessages[0].topic, + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + check(model).fetchingOlder.isFalse(); + checkHasMessages([]); + checkNotifiedOnce(); + + await fetchFuture; + checkHasMessages([]); + checkNotNotified(); + + async.elapse(const Duration(seconds: 1)); + checkHasMessages(initialMessages + movedMessages); + checkNotifiedOnce(); + })); + + test('fetchOlder, _reset, move fetch finishes, fetchOlder returns', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + connection.prepare(delay: const Duration(seconds: 2), json: olderResult( + anchor: model.messages[0].id, + foundOldest: true, + messages: olderMessages, + ).toJson()); + final fetchFuture = model.fetchOlder(); + checkHasMessages(initialMessages); + check(model).fetchingOlder.isTrue(); + checkNotifiedOnce(); + + connection.prepare(delay: const Duration(seconds: 1), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: movedMessages[0].topic, + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + checkHasMessages([]); + check(model).fetchingOlder.isFalse(); + checkNotifiedOnce(); + + async.elapse(const Duration(seconds: 1)); + checkHasMessages(initialMessages + movedMessages); + checkNotifiedOnce(); + + await fetchFuture; + checkHasMessages(initialMessages + movedMessages); + checkNotNotified(); + })); + + test('fetchInitial, _reset, initial fetch finishes, move fetch finishes', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, null); + + connection.prepare(delay: const Duration(seconds: 1), json: newestResult( + foundOldest: false, + messages: initialMessages, + ).toJson()); + final fetchFuture = model.fetchInitial(); + checkHasMessages([]); + check(model).fetched.isFalse(); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: movedMessages[0].topic, + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + checkHasMessages([]); + check(model).fetched.isFalse(); + checkNotifiedOnce(); + + await fetchFuture; + checkHasMessages([]); + check(model).fetched.isFalse(); + checkNotNotified(); + + async.elapse(const Duration(seconds: 1)); + checkHasMessages(initialMessages + movedMessages); + checkNotifiedOnce(); + })); + + test('fetchInitial, _reset, move fetch finishes, initial fetch finishes', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, null); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: false, + messages: initialMessages, + ).toJson()); + final fetchFuture = model.fetchInitial(); + checkHasMessages([]); + check(model).fetched.isFalse(); + + connection.prepare(delay: const Duration(seconds: 1), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: movedMessages[0].topic, + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + checkHasMessages([]); + check(model).fetched.isFalse(); + + async.elapse(const Duration(seconds: 1)); + checkHasMessages(initialMessages + movedMessages); + check(model).fetched.isTrue(); + + await fetchFuture; + checkHasMessages(initialMessages + movedMessages); + })); + + test('fetchOlder #1, _reset, move fetch finishes, fetchOlder #2, ' + 'fetchOlder #1 finishes, fetchOlder #2 finishes', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + connection.prepare(delay: const Duration(seconds: 2), json: olderResult( + anchor: model.messages[0].id, + foundOldest: true, + messages: olderMessages, + ).toJson()); + final fetchFuture1 = model.fetchOlder(); + checkHasMessages(initialMessages); + check(model).fetchingOlder.isTrue(); + checkNotifiedOnce(); + + connection.prepare(delay: const Duration(seconds: 1), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: movedMessages[0].topic, + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + checkHasMessages([]); + check(model).fetchingOlder.isFalse(); + checkNotifiedOnce(); + + async.elapse(const Duration(seconds: 1)); + checkNotifiedOnce(); + + connection.prepare(delay: const Duration(seconds: 2), json: olderResult( + anchor: model.messages[0].id, + foundOldest: true, + messages: olderMessages + ).toJson()); + final fetchFuture2 = model.fetchOlder(); + checkHasMessages(initialMessages + movedMessages); + check(model).fetchingOlder.isTrue(); + checkNotifiedOnce(); + + await fetchFuture1; + checkHasMessages(initialMessages + movedMessages); + // The older fetchOlder call should not override fetchingOlder set by + // the new fetchOlder call, nor should it notify the listeners. + check(model).fetchingOlder.isTrue(); + checkNotNotified(); + + await fetchFuture2; + checkHasMessages(olderMessages + initialMessages + movedMessages); + check(model).fetchingOlder.isFalse(); + checkNotifiedOnce(); + })); + }); + }); + group('regression tests for #455', () { test('reaction events handled once, even when message is in two message lists', () async { final stream = eg.stream(); diff --git a/test/model/message_test.dart b/test/model/message_test.dart index f179587358..249317310c 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -297,11 +297,15 @@ void main() { test('message topic moved update', () async { await prepareOrigMessages(origTopic: 'old topic'); + final originalDisplayRecipient = origMessages[0].displayRecipient!; await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: origMessages, newTopic: 'new topic')); - checkNotifiedOnce(); - check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); + checkNotified(count: 2); + check(store).messages.values.every(((message) => + message.isA() + ..editState.equals(MessageEditState.moved) + ..displayRecipient.equals(originalDisplayRecipient))); }); test('message topic resolved update', () async { @@ -309,7 +313,7 @@ void main() { await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: origMessages, newTopic: '✔ new topic')); - checkNotifiedOnce(); + checkNotified(count: 2); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); }); @@ -318,7 +322,7 @@ void main() { await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: origMessages, newTopic: 'new topic')); - checkNotifiedOnce(); + checkNotified(count: 2); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.none))); }); @@ -327,7 +331,7 @@ void main() { await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: origMessages, newTopic: '✔ new topic 2')); - checkNotifiedOnce(); + checkNotified(count: 2); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); @@ -336,7 +340,7 @@ void main() { await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: origMessages, newTopic: 'new topic 2')); - checkNotifiedOnce(); + checkNotified(count: 2); check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); }); @@ -345,8 +349,11 @@ void main() { await store.handleEvent(eg.updateMessageEventMoveFrom( origMessages: origMessages, newStreamId: 20)); - checkNotifiedOnce(); - check(store).messages.values.every(((message) => message.editState.equals(MessageEditState.moved))); + checkNotified(count: 2); + check(store).messages.values.every(((message) => + message.isA() + ..editState.equals(MessageEditState.moved) + ..displayRecipient.equals(null))); }); test('message is both moved and updated', () async { @@ -355,7 +362,7 @@ void main() { origMessages: origMessages, newStreamId: 20, newContent: 'new content')); - checkNotifiedOnce(); + checkNotified(count: 2); check(store).messages[origMessages[0].id].editState.equals(MessageEditState.edited); check(store).messages[origMessages[1].id].editState.equals(MessageEditState.moved); }); From a8fc00d22d2635880445e93bba082d74da9f3458 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 29 Jul 2024 18:27:51 -0400 Subject: [PATCH 7/9] msglist test: Move marker message back and forth. Previously, messageWithMarker already has its new topic in the message list when initialized. A subsequent event that moves the message from elsewhere to said topic does not logically make sense. This fixed that by moving the message back and forth. We can't simply move the message from elsewhere because we need to record the initial rects of the messages. Signed-off-by: Zixuan James Li --- test/widgets/message_list_test.dart | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index d4f77a4491..b2251ab39a 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -1019,20 +1019,25 @@ void main() { testWidgets('edit state updates do not affect layout', (WidgetTester tester) async { final messages = [ - eg.streamMessage(), + eg.streamMessage(topic: 'orig'), eg.streamMessage( + topic: 'orig', reactions: [eg.unicodeEmojiReaction, eg.realmEmojiReaction], flags: [MessageFlag.starred]), - eg.streamMessage(), + eg.streamMessage(topic: 'orig'), ]; final StreamMessage messageWithMarker = messages[1]; await setupMessageListPage(tester, messages: messages); final rectsBefore = captureMessageRects(tester, messages, messageWithMarker); checkMarkersCount(edited: 0, moved: 0); - // TODO(#150): [messageWithMarker]'s topic in store is inconsistent with the event. This will be fixed soon. - await store.handleEvent(eg.updateMessageEventMoveTo( - origTopic: 'old', newMessages: [messageWithMarker])); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [store.messages[messageWithMarker.id] as StreamMessage], + newTopic: 'new')); + await tester.pump(); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: [store.messages[messageWithMarker.id] as StreamMessage], + newTopic: 'orig')); await tester.pump(); check(captureMessageRects(tester, messages, messageWithMarker)) .deepEquals(rectsBefore); From 838d5a75e94848a30323ad30d9cde8fa5c69c079 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 11 Jul 2024 13:48:58 -0400 Subject: [PATCH 8/9] msglist [nfc]: Make narrow mutable for _MessageListPageState. This allows us to switch narrow when a move happens. Signed-off-by: Zixuan James Li --- integration_test/unreadmarker_test.dart | 2 +- lib/notifications/display.dart | 2 +- lib/widgets/message_list.dart | 24 ++++++++++++------- test/notifications/display_test.dart | 2 +- test/widgets/action_sheet_test.dart | 2 +- test/widgets/autocomplete_test.dart | 2 +- test/widgets/content_test.dart | 2 +- test/widgets/message_list_checks.dart | 2 +- test/widgets/message_list_test.dart | 2 +- test/widgets/profile_test.dart | 2 +- .../widgets/recent_dm_conversations_test.dart | 2 +- 11 files changed, 25 insertions(+), 19 deletions(-) diff --git a/integration_test/unreadmarker_test.dart b/integration_test/unreadmarker_test.dart index babc6cd3e8..548a446841 100644 --- a/integration_test/unreadmarker_test.dart +++ b/integration_test/unreadmarker_test.dart @@ -32,7 +32,7 @@ void main() { newestResult(foundOldest: true, messages: messages).toJson()); await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, - child: const MessageListPage(narrow: CombinedFeedNarrow()))); + child: const MessageListPage(initNarrow: CombinedFeedNarrow()))); await tester.pumpAndSettle(); return messages; } diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 0787a61bee..c0aa0ca831 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -272,7 +272,7 @@ class NotificationDisplayManager { // TODO(nav): Better interact with existing nav stack on notif open navigator.push(MaterialAccountWidgetRoute(accountId: account.id, // TODO(#82): Open at specific message, not just conversation - page: MessageListPage(narrow: narrow))); + page: MessageListPage(initNarrow: narrow))); return; } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b884cd582a..0e88b4a9ff 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -191,12 +191,12 @@ abstract class MessageListPageState { } class MessageListPage extends StatefulWidget { - const MessageListPage({super.key, required this.narrow}); + const MessageListPage({super.key, required this.initNarrow}); static Route buildRoute({int? accountId, BuildContext? context, required Narrow narrow}) { return MaterialAccountWidgetRoute(accountId: accountId, context: context, - page: MessageListPage(narrow: narrow)); + page: MessageListPage(initNarrow: narrow)); } /// The [MessageListPageState] above this context in the tree. @@ -211,7 +211,7 @@ class MessageListPage extends StatefulWidget { return state!; } - final Narrow narrow; + final Narrow initNarrow; @override State createState() => _MessageListPageState(); @@ -219,13 +219,19 @@ class MessageListPage extends StatefulWidget { class _MessageListPageState extends State implements MessageListPageState { @override - Narrow get narrow => widget.narrow; + late Narrow narrow; @override ComposeBoxController? get composeBoxController => _composeBoxKey.currentState; final GlobalKey _composeBoxKey = GlobalKey(); + @override + void initState() { + super.initState(); + narrow = widget.initNarrow; + } + @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); @@ -233,7 +239,7 @@ class _MessageListPageState extends State implements MessageLis final Color? appBarBackgroundColor; bool removeAppBarBottomBorder = false; - switch(widget.narrow) { + switch(narrow) { case CombinedFeedNarrow(): case MentionsNarrow(): appBarBackgroundColor = null; // i.e., inherit @@ -256,7 +262,7 @@ class _MessageListPageState extends State implements MessageLis } return Scaffold( - appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow), + appBar: AppBar(title: MessageListAppBarTitle(narrow: narrow), backgroundColor: appBarBackgroundColor, shape: removeAppBarBottomBorder ? const Border() @@ -280,11 +286,11 @@ class _MessageListPageState extends State implements MessageLis // The compose box, when present, pads the bottom inset. // TODO(#311) If we have a bottom nav, it will pad the bottom // inset, and this should always be true. - removeBottom: ComposeBox.hasComposeBox(widget.narrow), + removeBottom: ComposeBox.hasComposeBox(narrow), child: Expanded( - child: MessageList(narrow: widget.narrow))), - ComposeBox(controllerKey: _composeBoxKey, narrow: widget.narrow), + child: MessageList(narrow: narrow))), + ComposeBox(controllerKey: _composeBoxKey, narrow: narrow), ])))); } } diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index d65eb2b558..16f50fdc63 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -546,7 +546,7 @@ void main() { route.isA() ..accountId.equals(account.id) ..page.isA() - .narrow.equals(SendableNarrow.ofMessage(message, + .initNarrow.equals(SendableNarrow.ofMessage(message, selfUserId: account.userId)); } diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 3b61192f0a..a2e561e477 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -59,7 +59,7 @@ Future setupToMessageActionSheet(WidgetTester tester, { ).toJson()); await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, - child: MessageListPage(narrow: narrow))); + child: MessageListPage(initNarrow: narrow))); // global store, per-account store, and message list get loaded await tester.pumpAndSettle(); diff --git a/test/widgets/autocomplete_test.dart b/test/widgets/autocomplete_test.dart index b52eafb10f..dcf3fe8516 100644 --- a/test/widgets/autocomplete_test.dart +++ b/test/widgets/autocomplete_test.dart @@ -48,7 +48,7 @@ Future setupToComposeInput(WidgetTester tester, { prepareBoringImageHttpClient(); await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, - child: MessageListPage(narrow: DmNarrow( + child: MessageListPage(initNarrow: DmNarrow( allRecipientIds: [eg.selfUser.userId, eg.otherUser.userId], selfUserId: eg.selfUser.userId)))); diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index b62dd7502d..3fe1151b25 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -815,7 +815,7 @@ void main() { await tapText(tester, find.text('stream')); check(testBinding.takeLaunchUrlCalls()).isEmpty(); check(pushedRoutes).single.isA() - .page.isA().narrow.equals(const ChannelNarrow(1)); + .page.isA().initNarrow.equals(const ChannelNarrow(1)); }); testWidgets('invalid internal links are opened in browser', (tester) async { diff --git a/test/widgets/message_list_checks.dart b/test/widgets/message_list_checks.dart index 5daa91a1f4..6ce43a2d43 100644 --- a/test/widgets/message_list_checks.dart +++ b/test/widgets/message_list_checks.dart @@ -3,5 +3,5 @@ import 'package:zulip/model/narrow.dart'; import 'package:zulip/widgets/message_list.dart'; extension MessageListPageChecks on Subject { - Subject get narrow => has((x) => x.narrow, 'narrow'); + Subject get initNarrow => has((x) => x.initNarrow, 'initNarrow'); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index b2251ab39a..3f89a10878 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -69,7 +69,7 @@ void main() { newestResult(foundOldest: foundOldest, messages: messages).toJson()); await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, - child: MessageListPage(narrow: narrow))); + child: MessageListPage(initNarrow: narrow))); // global store, per-account store, and message list get loaded await tester.pumpAndSettle(); diff --git a/test/widgets/profile_test.dart b/test/widgets/profile_test.dart index 76435d32ab..651104498a 100644 --- a/test/widgets/profile_test.dart +++ b/test/widgets/profile_test.dart @@ -253,7 +253,7 @@ void main() { await tester.tap(targetWidget); check(pushedRoutes).last.isA().page .isA() - .narrow.equals(DmNarrow.withUser(1, selfUserId: eg.selfUser.userId)); + .initNarrow.equals(DmNarrow.withUser(1, selfUserId: eg.selfUser.userId)); }); testWidgets('page builds; user links render multiple avatars', (WidgetTester tester) async { diff --git a/test/widgets/recent_dm_conversations_test.dart b/test/widgets/recent_dm_conversations_test.dart index 69817ec5ce..77ace1d7e7 100644 --- a/test/widgets/recent_dm_conversations_test.dart +++ b/test/widgets/recent_dm_conversations_test.dart @@ -322,7 +322,7 @@ void main() { check(pushedRoutes).last.isA().page .isA() - .narrow.equals(expectedNarrow); + .initNarrow.equals(expectedNarrow); } testWidgets('1:1', (WidgetTester tester) async { From e9b5f6f5bc6716793b46bba69ccac395963426cb Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 11 Jul 2024 13:57:21 -0400 Subject: [PATCH 9/9] msglist: Update narrow when propagateMode is changeAll or changeLater. Navigation change is only intended for TopicNarrow. For StreamNarrow, propagateMode is unused. See also: - https://github.com/zulip/zulip-mobile/issues/5251 - https://github.com/zulip/zulip-flutter/pull/787#discussion_r1702307866 Fixes: #150 Signed-off-by: Zixuan James Li --- lib/model/message.dart | 7 +++ lib/model/message_list.dart | 18 +++++- lib/widgets/message_list.dart | 16 ++++- test/example_data.dart | 7 ++- test/flutter_checks.dart | 10 ++++ test/model/message_list_test.dart | 93 +++++++++++++++++++++++++++++ test/widgets/message_list_test.dart | 90 ++++++++++++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) diff --git a/lib/model/message.dart b/lib/model/message.dart index 5d30a17ce9..1e3abbbcdb 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -152,6 +152,7 @@ class MessageStoreImpl with MessageStore { final newStreamId = event.newStreamId; // null if topic-only move final origTopic = event.origTopic; final newTopic = event.newTopic; + final propagateMode = event.propagateMode; if (origTopic == null) { // There was no move. @@ -178,6 +179,11 @@ class MessageStoreImpl with MessageStore { assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log) return; } + if (propagateMode == null) { + // The `propagate_mode` field (aka propagateMode) is documented to be present on moves. + assert(debugLog('Malformed UpdateMessageEvent: move but no propagateMode')); // TODO(log) + return; + } final wasResolveOrUnresolve = (newStreamId == null && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic, newTopic!)); @@ -215,6 +221,7 @@ class MessageStoreImpl with MessageStore { origTopic: origTopic, newTopic: newTopic ?? origTopic, messageIds: event.messageIds, + propagateMode: propagateMode, ); } } diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 29453c2778..f20960e221 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -359,7 +359,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { } final PerAccountStore store; - final Narrow narrow; + Narrow narrow; /// Whether [message] should actually appear in this message list, /// given that it does belong to the narrow. @@ -535,12 +535,24 @@ class MessageListView with ChangeNotifier, _MessageSequence { } } + void _handlePropagateMode(PropagateMode propagateMode, Narrow newNarrow) { + switch (propagateMode) { + case PropagateMode.changeAll: + case PropagateMode.changeLater: + narrow = newNarrow; + _reset(); + fetchInitial(); + case PropagateMode.changeOne: + } + } + void messagesMoved({ required int origStreamId, required int newStreamId, required String origTopic, required String newTopic, required List messageIds, + required PropagateMode propagateMode, }) { switch (narrow) { case DmNarrow(): @@ -571,7 +583,9 @@ class MessageListView with ChangeNotifier, _MessageSequence { case (false, false): return; case (true, true ): return; // TODO(log) no-op move case (false, true ): _messagesMovedIntoNarrow(); - case (true, false): _messagesMovedFromNarrow(messageIds); // TODO handle propagateMode + case (true, false): + _messagesMovedFromNarrow(messageIds); + _handlePropagateMode(propagateMode, TopicNarrow(newStreamId, newTopic)); } } } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 0e88b4a9ff..4df50ccd37 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -232,6 +232,12 @@ class _MessageListPageState extends State implements MessageLis narrow = widget.initNarrow; } + void _narrowChanged(Narrow newNarrow) { + setState(() { + narrow = newNarrow; + }); + } + @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); @@ -289,7 +295,7 @@ class _MessageListPageState extends State implements MessageLis removeBottom: ComposeBox.hasComposeBox(narrow), child: Expanded( - child: MessageList(narrow: narrow))), + child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))), ComposeBox(controllerKey: _composeBoxKey, narrow: narrow), ])))); } @@ -368,9 +374,10 @@ const _kShortMessageHeight = 80; const kFetchMessagesBufferPixels = (kMessageListFetchBatchSize / 2) * _kShortMessageHeight; class MessageList extends StatefulWidget { - const MessageList({super.key, required this.narrow}); + const MessageList({super.key, required this.narrow, required this.onNarrowChanged}); final Narrow narrow; + final void Function(Narrow newNarrow) onNarrowChanged; @override State createState() => _MessageListState(); @@ -407,6 +414,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]. + widget.onNarrowChanged(model!.narrow); + } setState(() { // The actual state lives in the [MessageListView] model. // This method was called because that just changed. diff --git a/test/example_data.dart b/test/example_data.dart index 8b385eb4f6..41abc88bc1 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -472,6 +472,7 @@ UpdateMessageEvent _updateMessageMoveEvent( String? origContent, String? newContent, required List flags, + PropagateMode propagateMode = PropagateMode.changeOne, }) { assert(newTopic != origTopic || (newStreamId != null && newStreamId != origStreamId)); @@ -486,7 +487,7 @@ UpdateMessageEvent _updateMessageMoveEvent( editTimestamp: 1234567890, // TODO generate timestamp origStreamId: origStreamId, newStreamId: newStreamId, - propagateMode: null, + propagateMode: propagateMode, origTopic: origTopic, newTopic: newTopic, origContent: origContent, @@ -503,6 +504,7 @@ UpdateMessageEvent updateMessageEventMoveFrom({ int? newStreamId, String? newTopic, String? newContent, + PropagateMode propagateMode = PropagateMode.changeOne, }) { assert(origMessages.isNotEmpty); final origMessage = origMessages.first; @@ -516,6 +518,7 @@ UpdateMessageEvent updateMessageEventMoveFrom({ origContent: origContent, newContent: newContent, flags: origMessage.flags, + propagateMode: propagateMode, ); } @@ -525,6 +528,7 @@ UpdateMessageEvent updateMessageEventMoveTo({ int? origStreamId, String? origTopic, String? origContent, + PropagateMode propagateMode = PropagateMode.changeOne, }) { assert(newMessages.isNotEmpty); final newMessage = newMessages.first; @@ -542,6 +546,7 @@ UpdateMessageEvent updateMessageEventMoveTo({ origContent: origContent, newContent: newContent, flags: newMessage.flags, + propagateMode: propagateMode, ); } diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index d0906d849b..641a2d8de1 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -60,8 +60,14 @@ extension TextChecks on Subject { Subject get style => has((t) => t.style, 'style'); } +extension TextEditingControllerChecks on Subject { + Subject get text => has((t) => t.text, 'text'); +} + extension TextFieldChecks on Subject { Subject get textCapitalization => has((t) => t.textCapitalization, 'textCapitalization'); + Subject get decoration => has((t) => t.decoration, 'decoration'); + Subject get controller => has((t) => t.controller, 'controller'); } extension TextStyleChecks on Subject { @@ -131,3 +137,7 @@ extension MaterialChecks on Subject { Subject get color => has((x) => x.color, 'color'); // TODO more } + +extension InputDecorationChecks on Subject { + Subject get hintText => has((x) => x.hintText, 'hintText'); +} diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index bfcf87a62e..ce1f582b1e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -587,6 +587,37 @@ void main() { checkHasMessages(initialMessages); checkNotNotified(); }); + + void testMessageMove(PropagateMode propagateMode) => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages + movedMessages); + connection.prepare(delay: const Duration(seconds: 1), json: newestResult( + foundOldest: false, + messages: movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopic: 'new', + newStreamId: otherStream.streamId, + propagateMode: propagateMode, + )); + checkNotifiedOnce(); + async.elapse(const Duration(seconds: 1)); + checkHasMessages(initialMessages); + check(model).narrow.equals(ChannelNarrow(stream.streamId)); + checkNotNotified(); + }); + + test('do not follow when propagateMode = changeOne', () { + testMessageMove(PropagateMode.changeOne); + }); + + test('do not follow when propagateMode = changeLater', () { + testMessageMove(PropagateMode.changeLater); + }); + + test('do not follow when propagateMode = changeAll', () { + testMessageMove(PropagateMode.changeAll); + }); }); group('in topic narrow', () { @@ -674,6 +705,68 @@ void main() { checkNotNotified(); })); }); + + void handleMoveEvent(PropagateMode propagateMode) => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages + movedMessages); + connection.prepare(delay: const Duration(seconds: 1), json: newestResult( + foundOldest: false, + messages: movedMessages, + ).toJson()); + await store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: movedMessages, + newTopic: 'new', + newStreamId: otherStream.streamId, + propagateMode: propagateMode, + )); + checkNotifiedOnce(); + async.elapse(const Duration(seconds: 1)); + }); + + test('do not follow to the new narrow when propagateMode = changeOne', () { + handleMoveEvent(PropagateMode.changeOne); + checkNotNotified(); + checkHasMessages(initialMessages); + check(model).narrow.equals(TopicNarrow(stream.streamId, 'topic')); + }); + + test('follow to the new narrow when propagateMode = changeLater', () { + handleMoveEvent(PropagateMode.changeLater); + checkNotifiedOnce(); + checkHasMessages(movedMessages); + check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new')); + }); + + test('follow to the new narrow when propagateMode = changeAll', () { + handleMoveEvent(PropagateMode.changeAll); + checkNotifiedOnce(); + checkHasMessages(movedMessages); + check(model).narrow.equals(TopicNarrow(otherStream.streamId, 'new')); + }); + + test('handle move event before initial fetch', () => awaitFakeAsync((async) async { + await prepare(narrow: narrow); + final subscription = eg.subscription(stream); + await store.addStream(stream); + await store.addSubscription(subscription); + final followedMessage = eg.streamMessage(stream: stream, topic: 'new'); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: true, + messages: [followedMessage], + ).toJson()); + + check(model).fetched.isFalse(); + checkHasMessages([]); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: 'topic', + newMessages: [followedMessage], + propagateMode: PropagateMode.changeAll, + )); + check(model).narrow.equals(TopicNarrow(stream.streamId, 'new')); + + async.elapse(const Duration(seconds: 2)); + checkHasMessages([followedMessage]); + })); }); group('fetch races', () { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 3f89a10878..071823bf65 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -14,6 +14,7 @@ import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; +import 'package:zulip/widgets/autocomplete.dart'; import 'package:zulip/widgets/content.dart'; import 'package:zulip/widgets/emoji_reaction.dart'; import 'package:zulip/widgets/icons.dart'; @@ -620,6 +621,95 @@ void main() { }); }); + group('Update Narrow on message move', () { + const topic = 'foo'; + final channel = eg.stream(name: 'move test stream'); + final otherChannel = eg.stream(name: 'other move test stream'); + final narrow = TopicNarrow(channel.streamId, topic); + + void prepareGetMessageResponse(List messages) { + connection.prepare(json: newestResult( + foundOldest: false, messages: messages).toJson()); + } + + void handleMessageMoveEvent(List messages, String newTopic, {int? newChannelId}) { + store.handleEvent(eg.updateMessageEventMoveFrom( + origMessages: messages, + newTopic: newTopic, + newStreamId: newChannelId, + propagateMode: PropagateMode.changeAll)); + } + + testWidgets('compose box send message after move', (WidgetTester tester) async { + final message = eg.streamMessage(stream: channel, topic: topic, content: 'Message to move'); + await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel, otherChannel]); + + final channelContentInputFinder = find.descendant( + of: find.byType(ComposeAutocomplete), + matching: find.byType(TextField)); + + await tester.enterText(channelContentInputFinder, 'Some text'); + check(tester.widget(channelContentInputFinder)) + ..decoration.isNotNull().hintText.equals('Message #${channel.name} > $topic') + ..controller.isNotNull().text.equals('Some text'); + + prepareGetMessageResponse([message]); + handleMessageMoveEvent([message], 'new topic', newChannelId: otherChannel.streamId); + await tester.pump(const Duration(seconds: 1)); + check(tester.widget(channelContentInputFinder)) + ..decoration.isNotNull().hintText.equals('Message #${otherChannel.name} > new topic') + ..controller.isNotNull().text.equals('Some text'); + + connection.prepare(json: SendMessageResult(id: 1).toJson()); + await tester.tap(find.byIcon(Icons.send)); + await tester.pump(); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages') + ..bodyFields.deepEquals({ + 'type': 'stream', + 'to': '${otherChannel.streamId}', + 'topic': 'new topic', + 'content': 'Some text', + 'read_by_sender': 'true'}); + await tester.pumpAndSettle(); + }); + + testWidgets('Move to narrow with existing messages', (WidgetTester tester) async { + final message = eg.streamMessage(stream: channel, topic: topic, content: 'Message to move'); + await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]); + check(find.textContaining('Existing message').evaluate()).length.equals(0); + check(find.textContaining('Message to move').evaluate()).length.equals(1); + + final existingMessage = eg.streamMessage( + stream: eg.stream(), topic: 'new topic', content: 'Existing message'); + prepareGetMessageResponse([existingMessage, message]); + handleMessageMoveEvent([message], 'new topic'); + await tester.pump(const Duration(seconds: 1)); + + check(find.textContaining('Existing message').evaluate()).length.equals(1); + check(find.textContaining('Message to move').evaluate()).length.equals(1); + }); + + testWidgets('show new topic in TopicNarrow after move', (tester) async { + final message = eg.streamMessage(stream: channel, topic: topic, content: 'Message to move'); + await setupMessageListPage(tester, narrow: narrow, messages: [message], streams: [channel]); + + prepareGetMessageResponse([message]); + handleMessageMoveEvent([message], 'new topic'); + await tester.pump(const Duration(seconds: 1)); + + check(find.descendant( + of: find.byType(RecipientHeader), + matching: find.text('new topic')).evaluate() + ).length.equals(1); + check(find.descendant( + of: find.byType(MessageListAppBarTitle), + matching: find.text('${channel.name} > new topic')).evaluate() + ).length.equals(1); + }); + }); + group('recipient headers', () { group('StreamMessageRecipientHeader', () { final stream = eg.stream(name: 'stream name');