diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index c7ae2fb6ac..bd9969e678 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -1113,7 +1113,10 @@ sealed class Message extends MessageBase { @override final int id; bool isMeMessage; + @JsonKey(readValue: _readLastEditTimestamp) int? lastEditTimestamp; + @JsonKey(readValue: _readLastMovedTimestamp) + int? lastMovedTimestamp; @JsonKey(fromJson: _reactionsFromJson, toJson: _reactionsToJson) Reactions? reactions; // null is equivalent to an empty [Reactions] @@ -1164,6 +1167,21 @@ sealed class Message extends MessageBase { ); } + /// Reads `last_edit_timestamp` from JSON. + /// + /// This field is present when the message's content has been edited. + static int? _readLastEditTimestamp(Map json, String key) { + return json['last_edit_timestamp'] as int?; + } + + /// Reads `last_moved_timestamp` from JSON. + /// + /// This field is present when the message has been moved to a different + /// channel or topic (excluding resolve/unresolve operations). + static int? _readLastMovedTimestamp(Map json, String key) { + return json['last_moved_timestamp'] as int?; + } + Message({ required this.client, required this.content, @@ -1172,6 +1190,7 @@ sealed class Message extends MessageBase { required this.id, required this.isMeMessage, required this.lastEditTimestamp, + required this.lastMovedTimestamp, required this.reactions, required this.recipientId, required this.senderEmail, @@ -1257,6 +1276,7 @@ class StreamMessage extends Message { required super.id, required super.isMeMessage, required super.lastEditTimestamp, + required super.lastMovedTimestamp, required super.reactions, required super.recipientId, required super.senderEmail, @@ -1319,6 +1339,7 @@ class DmMessage extends Message { required super.id, required super.isMeMessage, required super.lastEditTimestamp, + required super.lastMovedTimestamp, required super.reactions, required super.recipientId, required super.senderEmail, @@ -1373,41 +1394,11 @@ enum MessageEditState { } static MessageEditState _readFromMessage(Map json, String key) { - // Adapted from `analyze_edit_history` in the web app: - // https://github.com/zulip/zulip/blob/c31cebbf68a93927d41e9947427c2dd4d46503e3/web/src/message_list_view.js#L68-L118 - final editHistory = json['edit_history'] as List?; final lastEditTimestamp = json['last_edit_timestamp'] as int?; - if (editHistory == null) { - return (lastEditTimestamp != null) - ? MessageEditState.edited - : MessageEditState.none; - } - - // Edit history should never be empty whenever it is present - assert(editHistory.isNotEmpty); - - bool hasMoved = false; - for (final entry in editHistory) { - if (entry['prev_content'] != null) { - return MessageEditState.edited; - } - - if (entry['prev_stream'] != null) { - hasMoved = true; - continue; - } - - final prevTopicStr = entry['prev_topic'] as String?; - if (prevTopicStr != null) { - final prevTopic = TopicName.fromJson(prevTopicStr); - final topic = TopicName.fromJson(entry['topic'] as String); - hasMoved |= !topicMoveWasResolveOrUnresolve(topic, prevTopic); - } - } - - if (hasMoved) return MessageEditState.moved; + final lastMovedTimestamp = json['last_moved_timestamp'] as int?; - // This can happen when a topic is resolved but nothing else has been edited + if (lastEditTimestamp != null) return MessageEditState.edited; + if (lastMovedTimestamp != null) return MessageEditState.moved; return MessageEditState.none; } } diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index 95c5588392..c7a5899801 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -426,7 +426,13 @@ StreamMessage _$StreamMessageFromJson(Map json) => ), id: (json['id'] as num).toInt(), isMeMessage: json['is_me_message'] as bool, - lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), + lastEditTimestamp: + (Message._readLastEditTimestamp(json, 'last_edit_timestamp') as num?) + ?.toInt(), + lastMovedTimestamp: + (Message._readLastMovedTimestamp(json, 'last_moved_timestamp') + as num?) + ?.toInt(), reactions: Message._reactionsFromJson(json['reactions']), recipientId: (json['recipient_id'] as num).toInt(), senderEmail: json['sender_email'] as String, @@ -454,6 +460,7 @@ Map _$StreamMessageToJson(StreamMessage instance) => 'id': instance.id, 'is_me_message': instance.isMeMessage, 'last_edit_timestamp': instance.lastEditTimestamp, + 'last_moved_timestamp': instance.lastMovedTimestamp, 'reactions': Message._reactionsToJson(instance.reactions), 'recipient_id': instance.recipientId, 'sender_email': instance.senderEmail, @@ -484,7 +491,12 @@ DmMessage _$DmMessageFromJson(Map json) => DmMessage( ), id: (json['id'] as num).toInt(), isMeMessage: json['is_me_message'] as bool, - lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), + lastEditTimestamp: + (Message._readLastEditTimestamp(json, 'last_edit_timestamp') as num?) + ?.toInt(), + lastMovedTimestamp: + (Message._readLastMovedTimestamp(json, 'last_moved_timestamp') as num?) + ?.toInt(), reactions: Message._reactionsFromJson(json['reactions']), recipientId: (json['recipient_id'] as num).toInt(), senderEmail: json['sender_email'] as String, @@ -510,6 +522,7 @@ Map _$DmMessageToJson(DmMessage instance) => { 'id': instance.id, 'is_me_message': instance.isMeMessage, 'last_edit_timestamp': instance.lastEditTimestamp, + 'last_moved_timestamp': instance.lastMovedTimestamp, 'reactions': Message._reactionsToJson(instance.reactions), 'recipient_id': instance.recipientId, 'sender_email': instance.senderEmail, diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 5cb6df7c24..f04bd37192 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -99,6 +99,7 @@ extension MessageChecks on Subject { Subject get id => has((e) => e.id, 'id'); Subject get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage'); Subject get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp'); + Subject get lastMovedTimestamp => has((e) => e.lastMovedTimestamp, 'lastMovedTimestamp'); Subject get editState => has((e) => e.editState, 'editState'); Subject get reactions => has((e) => e.reactions, 'reactions'); Subject get recipientId => has((e) => e.recipientId, 'recipientId'); diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index f04bbd6d4b..8aef5ed356 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -276,121 +276,32 @@ void main() { group('MessageEditState', () { Map baseJson() => deepToJson(eg.streamMessage()) as Map; - group('Edit history is absent', () { - test('Message with no evidence of an edit history -> none', () { - check(Message.fromJson(baseJson()..['edit_history'] = null)) - .editState.equals(MessageEditState.none); - }); - - test('Message without edit history has last edit timestamp -> edited', () { - check(Message.fromJson(baseJson() - ..['edit_history'] = null - ..['last_edit_timestamp'] = 1678139636)) - .editState.equals(MessageEditState.edited); - }); + test('No edits or moves -> none', () { + check(Message.fromJson(baseJson() + ..['last_edit_timestamp'] = null + ..['last_moved_timestamp'] = null)) + .editState.equals(MessageEditState.none); }); - void checkEditState(MessageEditState editState, List> editHistory){ - check(Message.fromJson(baseJson()..['edit_history'] = editHistory)) - .editState.equals(editState); - } + test('Content edited only -> edited', () { + check(Message.fromJson(baseJson() + ..['last_edit_timestamp'] = 1678139636 + ..['last_moved_timestamp'] = null)) + .editState.equals(MessageEditState.edited); + }); - group('edit history exists', () { - test('Moved message has last edit timestamp but no actual edits -> moved', () { - check(Message.fromJson(baseJson() - ..['edit_history'] = [{'prev_stream': 5, 'stream': 7}] - ..['last_edit_timestamp'] = 1678139636)) - .editState.equals(MessageEditState.moved); - }); - - test('Channel change only -> moved', () { - checkEditState(MessageEditState.moved, - [{'prev_stream': 5, 'stream': 7}]); - }); - - test('Topic name change only -> moved', () { - checkEditState(MessageEditState.moved, - [{'prev_topic': 'old_topic', 'topic': 'new_topic'}]); - }); - - test('Both topic and content changed -> edited', () { - checkEditState(MessageEditState.edited, [ - {'prev_topic': 'old_topic', 'topic': 'new_topic'}, - {'prev_content': 'old_content'}, - ]); - checkEditState(MessageEditState.edited, [ - {'prev_content': 'old_content'}, - {'prev_topic': 'old_topic', 'topic': 'new_topic'}, - ]); - }); - - test('Both topic and content changed in a single edit -> edited', () { - checkEditState(MessageEditState.edited, - [{'prev_topic': 'old_topic', 'topic': 'new_topic', 'prev_content': 'old_content'}]); - }); - - test('Content change only -> edited', () { - checkEditState(MessageEditState.edited, - [{'prev_content': 'old_content'}]); - }); + test('Moved only -> moved', () { + check(Message.fromJson(baseJson() + ..['last_edit_timestamp'] = null + ..['last_moved_timestamp'] = 1678139636)) + .editState.equals(MessageEditState.moved); }); - group('topic resolved in edit history', () { - test('Topic was only resolved -> none', () { - checkEditState(MessageEditState.none, - [{'prev_topic': 'old_topic', 'topic': '✔ old_topic'}]); - }); - - test('Topic was resolved but the content changed in the history -> edited', () { - checkEditState(MessageEditState.edited, [ - {'prev_topic': 'old_topic', 'topic': '✔ old_topic'}, - {'prev_content': 'old_content'}, - ]); - }); - - test('Topic was resolved but it also moved in the history -> moved', () { - checkEditState(MessageEditState.moved, [ - {'prev_topic': 'old_topic', 'topic': 'new_topic'}, - {'prev_topic': '✔ old_topic', 'topic': 'old_topic'}, - ]); - }); - - test('Topic was moved but it also was resolved in the history -> moved', () { - checkEditState(MessageEditState.moved, [ - {'prev_topic': '✔ old_topic', 'topic': 'old_topic'}, - {'prev_topic': 'old_topic', 'topic': 'new_topic'}, - ]); - }); - - // Technically the topic *was* unresolved, so MessageEditState.none - // would be valid and preferable -- if it didn't need more intense - // computation than we're comfortable with in a hot codepath, i.e., - // a regex test instead of a simple `startsWith` / `substring` check. - // See comment on the implementation, and discussion: - // https://github.com/zulip/zulip-flutter/pull/1242#discussion_r1917592157 - test('Unresolving topic with a weird prefix -> moved', () { - checkEditState(MessageEditState.moved, - [{'prev_topic': '✔ ✔old_topic', 'topic': 'old_topic'}]); - }); - - // Similar reasoning as in the previous test. - // Also, Zulip doesn't produce topics with a weird resolved-topic prefix, - // so this case can only be produced by unusual input in an - // edit/move-topic UI. A "moved" marker seems like a fine response - // in that circumstance. - test('Resolving topic with a weird prefix -> moved', () { - checkEditState(MessageEditState.moved, - [{'prev_topic': 'old_topic', 'topic': '✔ ✔old_topic'}]); - }); - - // Similar reasoning as the previous test, including that this case had to - // involve unusual input in an edit/move-topic UI. - // Here the computation burden would have come from calling - // [TopicName.canonicalize]. - test('Topic was resolved but with changed case -> moved', () { - checkEditState(MessageEditState.moved, - [{'prev_topic': 'old ToPiC', 'topic': '✔ OLD tOpIc'}]); - }); + test('Both edited and moved -> edited', () { + check(Message.fromJson(baseJson() + ..['last_edit_timestamp'] = 1678139700 + ..['last_moved_timestamp'] = 1678139636)) + .editState.equals(MessageEditState.edited); }); }); } diff --git a/test/example_data.dart b/test/example_data.dart index c9beaef72f..6d564f1bb7 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -720,6 +720,7 @@ StreamMessage streamMessage({ String? content, String? contentMarkdown, int? lastEditTimestamp, + int? lastMovedTimestamp, List? reactions, int? timestamp, List? flags, @@ -744,6 +745,7 @@ StreamMessage streamMessage({ 'flags': flags ?? [], 'id': id ?? _nextMessageId(), 'last_edit_timestamp': lastEditTimestamp, + 'last_moved_timestamp': lastMovedTimestamp, 'subject': topic ?? _defaultTopic, 'submessages': submessages ?? [], 'timestamp': timestamp ?? 1678139636, @@ -770,6 +772,7 @@ DmMessage dmMessage({ String? content, String? contentMarkdown, int? lastEditTimestamp, + int? lastMovedTimestamp, int? timestamp, List? flags, List? submessages, @@ -787,6 +790,7 @@ DmMessage dmMessage({ 'flags': flags ?? [], 'id': id ?? _nextMessageId(), 'last_edit_timestamp': lastEditTimestamp, + 'last_moved_timestamp': lastMovedTimestamp, 'subject': '', 'submessages': submessages ?? [], 'timestamp': timestamp ?? 1678139636,