message_edit: Don't display resolved topics as MOVED.

We loop through edit history entries and see if any of them
are more interesting than a (un)resolve topic edit, extending
the existing loop we had.

We also update the associated node tests.

Fixes #19919.

Co-authored by: Lauryn Menard <lauryn@zulip.com>

frontend_tests/node_tests/message_list_view.js

@@ -73,10 +73,16 @@ test("msg_moved_var", () => {
         message_context = {
             ...message_context,
         };
-        message_context.msg = {
+        if ("edit_history" in message) {
-            last_edit_timestamp: (next_timestamp += 1),
+            message_context.msg = {
-            ...message,
+                last_edit_timestamp: (next_timestamp += 1),
-        };
+                ...message,
+            };
+        } else {
+            message_context.msg = {
+                ...message,
+            };
+        }
         return message_context;
     }
 
@@ -96,50 +102,80 @@ test("msg_moved_var", () => {
     function assert_moved_false(message_container) {
         assert.equal(message_container.moved, false);
     }
+    function assert_moved_undefined(message_container) {
+        assert.equal(message_container.moved, undefined);
+    }
 
     (function test_msg_moved_var() {
         const messages = [
-            // no edits: Not moved.
+            // no edit history: NO LABEL
-            build_message_context(),
+            build_message_context({}),
-            // stream changed: Move
+            // stream changed: MOVED
             build_message_context({
-                edit_history: [{prev_stream: "test_stream", timestamp: 1000, user_id: 1}],
+                edit_history: [{prev_stream: 1, timestamp: 1000, user_id: 1}],
             }),
-            // topic changed: Move
+            // topic changed (not resolved/unresolved): MOVED
             build_message_context({
-                edit_history: [{prev_topic: "test_topic", timestamp: 1000, user_id: 1}],
+                edit_history: [
+                    {prev_topic: "test_topic", topic: "new_topic", timestamp: 1000, user_id: 1},
+                ],
             }),
-            // content edited: Edit
+            // content edited: EDITED
             build_message_context({
                 edit_history: [{prev_content: "test_content", timestamp: 1000, user_id: 1}],
             }),
-            // stream and topic edited: Move
+            // stream and topic edited: MOVED
             build_message_context({
                 edit_history: [
-                    {prev_stream: "test_stream", timestamp: 1000, user_id: 1},
+                    {
-                    {prev_topic: "test_topic", timestamp: 1000, user_id: 1},
+                        prev_stream: 1,
+                        prev_topic: "test_topic",
+                        topic: "new_topic",
+                        timestamp: 1000,
+                        user_id: 1,
+                    },
                 ],
             }),
-            // topic and content changed: Edit
+            // topic and content changed: EDITED
             build_message_context({
                 edit_history: [
-                    {prev_topic: "test_topic", timestamp: 1000, user_id: 1},
+                    {
-                    {prev_content: "test_content", timestamp: 1001, user_id: 1},
+                        prev_topic: "test_topic",
+                        topic: "new_topic",
+                        prev_content: "test_content",
+                        timestamp: 1000,
+                        user_id: 1,
+                    },
                 ],
             }),
-            // stream and content changed: Edit
+            // only topic resolved: NO LABEL
             build_message_context({
                 edit_history: [
-                    {prev_content: "test_content", timestamp: 1000, user_id: 1},
+                    {prev_topic: "test_topic", topic: "✔ test_topic", timestamp: 1000, user_id: 1},
-                    {prev_stream: "test_stream", timestamp: 1001, user_id: 1},
                 ],
             }),
-            // topic, stream, and content changed: Edit
+            // only topic unresolved: NO LABEL
             build_message_context({
                 edit_history: [
-                    {prev_topic: "test_topic", timestamp: 1000, user_id: 1},
+                    {prev_topic: "✔ test_topic", topic: "test_topic", timestamp: 1000, user_id: 1},
-                    {prev_stream: "test_stream", timestamp: 1001, user_id: 1},
+                ],
+            }),
+            // multiple edit history logs, with at least one content edit: EDITED
+            build_message_context({
+                edit_history: [
+                    {prev_stream: 1, timestamp: 1000, user_id: 1},
+                    {prev_topic: "old_topic", topic: "test_topic", timestamp: 1001, user_id: 1},
                     {prev_content: "test_content", timestamp: 1002, user_id: 1},
+                    {prev_topic: "test_topic", topic: "✔ test_topic", timestamp: 1003, user_id: 1},
+                ],
+            }),
+            // multiple edit history logs with no content edit: MOVED
+            build_message_context({
+                edit_history: [
+                    {prev_stream: 1, timestamp: 1000, user_id: 1},
+                    {prev_topic: "old_topic", topic: "test_topic", timestamp: 1001, user_id: 1},
+                    {prev_topic: "test_topic", topic: "✔ test_topic", timestamp: 1002, user_id: 1},
+                    {prev_topic: "✔ test_topic", topic: "test_topic", timestamp: 1003, user_id: 1},
                 ],
             }),
         ];
@@ -154,8 +190,8 @@ test("msg_moved_var", () => {
 
         const result = list._message_groups[0].message_containers;
 
-        // no edits: false
+        // no edit history: undefined
-        assert_moved_false(result[0]);
+        assert_moved_undefined(result[0]);
         // stream changed: true
         assert_moved_true(result[1]);
         // topic changed: true
@@ -166,10 +202,14 @@ test("msg_moved_var", () => {
         assert_moved_true(result[4]);
         // topic and content changed: false
         assert_moved_false(result[5]);
-        // stream and content changed: false
+        // only topic resolved: undefined
-        assert_moved_false(result[6]);
+        assert_moved_undefined(result[6]);
-        // topic, stream, and content changed: false
+        // only topic unresolved: undefined
-        assert_moved_false(result[7]);
+        assert_moved_undefined(result[7]);
+        // multiple edits with content edit: false
+        assert_moved_false(result[8]);
+        // multiple edits without content edit: true
+        assert_moved_true(result[9]);
     })();
 });
 
@@ -189,6 +229,7 @@ test("msg_edited_vars", () => {
         message_context.msg = {
             is_me_message: false,
             last_edit_timestamp: (next_timestamp += 1),
+            edit_history: [{prev_content: "test_content", timestamp: 1000, user_id: 1}],
             ...message,
         };
         return message_context;

static/js/message_events.js

@@ -184,7 +184,37 @@ export function update_messages(events) {
 
         const old_stream = sub_store.get(event.stream_id);
 
-        // A topic edit may affect multiple messages, listed in
+        // Save the content edit to the front end msg.edit_history
+        // before topic edits to ensure that combined topic / content
+        // edits have edit_history logged for both before any
+        // potential narrowing as part of the topic edit loop.
+        if (event.orig_content !== undefined) {
+            if (page_params.realm_allow_edit_history) {
+                // Note that we do this for topic edits separately, below.
+                // If an event changed both content and topic, we'll generate
+                // two client-side events, which is probably good for display.
+                const edit_history_entry = {
+                    user_id: event.user_id,
+                    prev_content: event.orig_content,
+                    prev_rendered_content: event.orig_rendered_content,
+                    prev_rendered_content_version: event.prev_rendered_content_version,
+                    timestamp: event.edit_timestamp,
+                };
+                // Add message's edit_history in message dict
+                // For messages that are edited, edit_history needs to
+                // be added to message in frontend.
+                if (msg.edit_history === undefined) {
+                    msg.edit_history = [];
+                }
+                msg.edit_history = [edit_history_entry].concat(msg.edit_history);
+            }
+            message_content_edited = true;
+
+            // Update raw_content, so that editing a few times in a row is fast.
+            msg.raw_content = event.content;
+        }
+
+        // A topic or stream edit may affect multiple messages, listed in
         // event.message_ids. event.message_id is still the first message
         // where the user initiated the edit.
         topic_edited = new_topic !== undefined;
@@ -394,32 +424,6 @@ export function update_messages(events) {
             }
         }
 
-        if (event.orig_content !== undefined) {
-            if (page_params.realm_allow_edit_history) {
-                // Note that we do this for topic edits separately, above.
-                // If an event changed both content and topic, we'll generate
-                // two client-side events, which is probably good for display.
-                const edit_history_entry = {
-                    user_id: event.user_id,
-                    prev_content: event.orig_content,
-                    prev_rendered_content: event.orig_rendered_content,
-                    prev_rendered_content_version: event.prev_rendered_content_version,
-                    timestamp: event.edit_timestamp,
-                };
-                // Add message's edit_history in message dict
-                // For messages that are edited, edit_history needs to
-                // be added to message in frontend.
-                if (msg.edit_history === undefined) {
-                    msg.edit_history = [];
-                }
-                msg.edit_history = [edit_history_entry].concat(msg.edit_history);
-            }
-            message_content_edited = true;
-
-            // Update raw_content, so that editing a few times in a row is fast.
-            msg.raw_content = event.content;
-        }
-
         // Mark the message as edited for the UI. The rendering_only
         // flag is used to indicated update_message events that are
         // triggered by server latency optimizations, not user

static/js/message_list_view.js

@@ -57,22 +57,50 @@ function same_recipient(a, b) {
     return util.same_recipient(a.msg, b.msg);
 }
 
-function message_was_only_moved(message) {
+function analyze_edit_history(message) {
-    // Returns true if the message has had its stream/topic edited
+    // Returns a dict of booleans that describe the message's history:
-    // (i.e. the message was moved), but its content has not been
+    //   * edited: if the message has had its content edited
-    // edited.
+    //   * moved: if the message has had its stream/topic edited
+    //   * resolve_toggled: if the message has had a topic resolve/unresolve edit
+    let edited = false;
     let moved = false;
+    let resolve_toggled = false;
+
     if (message.edit_history !== undefined) {
         for (const edit_history_event of message.edit_history) {
             if (edit_history_event.prev_content) {
-                return false;
+                edited = true;
             }
-            if (edit_history_event.prev_topic || edit_history_event.prev_stream) {
+
+            if (edit_history_event.prev_stream) {
+                moved = true;
+            }
+
+            if (edit_history_event.prev_topic) {
+                // We know it has a topic edit. Now we need to determine if
+                // it was a true move or a resolve/unresolve.
+                if (
+                    resolved_topic.is_resolved(edit_history_event.topic) &&
+                    edit_history_event.topic.slice(2) === edit_history_event.prev_topic
+                ) {
+                    // Resolved.
+                    resolve_toggled = true;
+                    continue;
+                }
+                if (
+                    resolved_topic.is_resolved(edit_history_event.prev_topic) &&
+                    edit_history_event.prev_topic.slice(2) === edit_history_event.topic
+                ) {
+                    // Unresolved.
+                    resolve_toggled = true;
+                    continue;
+                }
+                // Otherwise, it is a real topic rename/move.
                 moved = true;
             }
         }
     }
-    return moved;
+    return {edited, moved, resolve_toggled};
 }
 
 function render_group_display_date(group, message_container) {
@@ -238,8 +266,11 @@ export class MessageListView {
     }
 
     _add_msg_edited_vars(message_container) {
-        // This adds variables to message_container object which calculate bools for
+        // This function computes data on whether the message was edited
-        // checking position of "EDITED" label as well as the edited timestring
+        // and in what ways, as well as where the "EDITED" or "MOVED"
+        // label should be located, and adds it to the message_container
+        // object.
+        //
         // The bools can be defined only when the message is edited
         // (or when the `last_edit_timestr` is defined). The bools are:
         //   * `edited_in_left_col`      -- when label appears in left column.
@@ -249,18 +280,29 @@ export class MessageListView {
         const include_sender = message_container.include_sender;
         const is_hidden = message_container.is_hidden;
         const status_message = Boolean(message_container.status_message);
-        if (last_edit_timestr !== undefined) {
+        const edit_history_details = analyze_edit_history(message_container.msg);
-            message_container.last_edit_timestr = last_edit_timestr;
+
-            message_container.edited_in_left_col = !include_sender && !is_hidden;
+        if (
-            message_container.edited_alongside_sender = include_sender && !status_message;
+            last_edit_timestr === undefined ||
-            message_container.edited_status_msg = include_sender && status_message;
+            !(edit_history_details.moved || edit_history_details.edited)
-            message_container.moved = message_was_only_moved(message_container.msg);
+        ) {
-        } else {
+            // For messages whose edit history at most includes
+            // resolving topics, we don't display an EDITED/MOVED
+            // notice at all. (The message actions popover will still
+            // display an edit history option, so you can see when it
+            // was marked as resolved if you need to).
             delete message_container.last_edit_timestr;
             message_container.edited_in_left_col = false;
             message_container.edited_alongside_sender = false;
             message_container.edited_status_msg = false;
+            return;
         }
+
+        message_container.last_edit_timestr = last_edit_timestr;
+        message_container.edited_in_left_col = !include_sender && !is_hidden;
+        message_container.edited_alongside_sender = include_sender && !status_message;
+        message_container.edited_status_msg = include_sender && status_message;
+        message_container.moved = edit_history_details.moved && !edit_history_details.edited;
     }
 
     set_calculated_message_container_variables(message_container, is_revealed) {