message: Enhance message move checks

Because of this simplification, it is easier to make sense of the
constraints that apply to the inter-related fields.

This also allows us to drop the `assert`'s and "TODO(log)"'s, because
the stacktrace we need can be retrieved after throwing these
`FormatException`'s.

This is similar to zulip-mobile code for parsing move data. The main
difference is that we now also check the value of `propagate_mode`,
which is documented to be present on message moves.
See Greg's comment on this:
  zulip#1311 (comment)

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

Author: PIG208

lib/model/message.dart

@@ -173,44 +173,23 @@ class MessageStoreImpl with MessageStore {
     // For reference, see: https://zulip.com/api/get-events#update_message
 
     final origStreamId = event.origStreamId;
-    final newStreamId = event.newStreamId; // null if topic-only move
+    final newStreamId = event.newStreamId ?? origStreamId;
     final origTopic = event.origTopic;
-    final newTopic = event.newTopic;
+    final newTopic = event.newTopic ?? origTopic;
     final propagateMode = event.propagateMode;
 
-    if (origTopic == null) {
+    if (origTopic == newTopic && origStreamId == newStreamId) {
+      if (propagateMode != null) {
+        throw FormatException(
+          'UpdateMessageEvent: incoherent message-move fields; '
+          'propagate_mode present but no new channel or topic');
+      }
       // There was no move.
-      assert(() {
-        if (newStreamId != null && origStreamId != null
-            && newStreamId != origStreamId) {
-          // This should be impossible; `orig_subject` (aka origTopic) is
-          // documented to be present when either the stream or topic changed.
-          debugLog('Malformed UpdateMessageEvent: stream move but no origTopic'); // TODO(log)
-        }
-        return true;
-      }());
-      return;
-    }
-
-    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) {
-      // The `stream_id` field (aka origStreamId) is documented to be present on moves.
-      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!));
+    final wasResolveOrUnresolve = origStreamId == newStreamId
+      && MessageEditState.topicMoveWasResolveOrUnresolve(origTopic!, newTopic!);
 
     for (final messageId in event.messageIds) {
       final message = messages[messageId];
@@ -221,15 +200,15 @@ class MessageStoreImpl with MessageStore {
         continue;
       }
 
-      if (newStreamId != null) {
-        message.streamId = newStreamId;
+      if (origStreamId != newStreamId) {
+        message.streamId = newStreamId!;
         // See [StreamMessage.displayRecipient] on why the invalidation is
         // needed.
         message.displayRecipient = null;
       }
 
-      if (newTopic != null) {
-        message.topic = newTopic;
+      if (origTopic != newTopic) {
+        message.topic = newTopic!;
       }
 
       if (!wasResolveOrUnresolve
@@ -240,12 +219,12 @@ class MessageStoreImpl with MessageStore {
 
     for (final view in _messageListViews) {
       view.messagesMoved(
-        origStreamId: origStreamId,
-        newStreamId: newStreamId ?? origStreamId,
-        origTopic: origTopic,
-        newTopic: newTopic ?? origTopic,
+        origStreamId: origStreamId!,
+        newStreamId: newStreamId!,
+        origTopic: origTopic!,
+        newTopic: newTopic!,
         messageIds: event.messageIds,
-        propagateMode: propagateMode,
+        propagateMode: propagateMode!,
       );
     }
   }

test/api/model/model_checks.dart

@@ -53,6 +53,7 @@ extension TopicNameChecks on Subject<TopicName> {
 
 extension StreamMessageChecks on Subject<StreamMessage> {
   Subject<String?> get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient');
+  Subject<int> get streamId => has((e) => e.streamId, 'streamId');
   Subject<TopicName> get topic => has((e) => e.topic, 'topic');
 }
 

test/model/message_test.dart

@@ -377,6 +377,82 @@ void main() {
     });
   });
 
+  group('message move', () {
+    final message = eg.streamMessage();
+    final baseJson = {
+      'id': 1,
+      'type': 'update_message',
+      'user_id': eg.selfUser.userId,
+      'rendering_only': false,
+      'message_id': message.id,
+      'message_ids': [message.id],
+      'flags': <String>[],
+      'edit_timestamp': 1718741351,
+      'stream_id': eg.stream().streamId,
+    };
+    final baseMoveJson = { ...baseJson,
+      'orig_subject': 'foo',
+      'propagate_mode': 'change_all',
+    };
+
+    Future<void> setupAndHandleEvent(Map<String, Object?> json) async {
+      await prepare();
+      await prepareMessages([message]);
+      await store.handleEvent(Event.fromJson(json) as UpdateMessageEvent);
+    }
+
+    test('smoke', () async {
+      await setupAndHandleEvent({ ...baseMoveJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+      });
+      checkNotified(count: 2);
+      check(store).messages[message.id].isA<StreamMessage>()
+        ..topic.equals(message.topic)
+        ..streamId.equals(2);
+    });
+
+    test('no message move', () async {
+      await setupAndHandleEvent({ ...baseJson,
+        'orig_content': 'foo',
+        'orig_rendered_content': 'foo',
+        'content': 'bar',
+        'rendered_content': 'bar',
+      });
+      checkNotifiedOnce();
+    });
+
+    test('stream move but no orig_subject', () async {
+      await check(setupAndHandleEvent({ ...baseMoveJson,
+        'stream_id': 1,
+        'new_stream_id': 2,
+        'orig_subject': null,
+      })).throws();
+    });
+
+    test('move but no subject or new_stream_id', () async {
+      await check(setupAndHandleEvent({ ...baseMoveJson,
+        'new_stream_id': null,
+        'subject': null,
+      })).throws<FormatException>();
+    });
+
+    test('move but no orig_stream_id', () async {
+      await check(setupAndHandleEvent({ ...baseMoveJson,
+        'stream_id': null,
+        'new_stream_id': 2,
+      })).throws();
+    });
+
+    test('move but no propagate_mode', () async {
+      await check(setupAndHandleEvent({ ...baseMoveJson,
+        'orig_subject': 'foo',
+        'subject': 'bar',
+        'propagate_mode': null,
+      })).throws();
+    });
+  });
+
   group('handleDeleteMessageEvent', () {
     test('delete an unknown message', () async {
       final message1 = eg.streamMessage();