From 84bdf86246669dbc1fdd0e6dd54ed64bd78223d3 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 8 Feb 2022 17:52:39 -0800 Subject: [PATCH] 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 --- .../node_tests/message_list_view.js | 99 +++++++++++++------ static/js/message_events.js | 58 ++++++----- static/js/message_list_view.js | 74 +++++++++++--- 3 files changed, 159 insertions(+), 72 deletions(-) diff --git a/frontend_tests/node_tests/message_list_view.js b/frontend_tests/node_tests/message_list_view.js index 9ad13262b9..1dad1e3194 100644 --- a/frontend_tests/node_tests/message_list_view.js +++ b/frontend_tests/node_tests/message_list_view.js @@ -73,10 +73,16 @@ test("msg_moved_var", () => { message_context = { ...message_context, }; - message_context.msg = { - last_edit_timestamp: (next_timestamp += 1), - ...message, - }; + if ("edit_history" in message) { + message_context.msg = { + 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. - build_message_context(), - // stream changed: Move + // no edit history: NO LABEL + build_message_context({}), + // 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_stream: "test_stream", timestamp: 1001, user_id: 1}, + {prev_topic: "test_topic", topic: "✔ test_topic", timestamp: 1000, 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_stream: "test_stream", timestamp: 1001, user_id: 1}, + {prev_topic: "✔ test_topic", topic: "test_topic", timestamp: 1000, 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 - assert_moved_false(result[0]); + // no edit history: undefined + 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 - assert_moved_false(result[6]); - // topic, stream, and content changed: false - assert_moved_false(result[7]); + // only topic resolved: undefined + assert_moved_undefined(result[6]); + // only topic unresolved: undefined + 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; diff --git a/static/js/message_events.js b/static/js/message_events.js index 27534ea0ff..1c89536857 100644 --- a/static/js/message_events.js +++ b/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 diff --git a/static/js/message_list_view.js b/static/js/message_list_view.js index 6ea6a13f61..06bda51f4e 100644 --- a/static/js/message_list_view.js +++ b/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) { - // Returns true if the message has had its stream/topic edited - // (i.e. the message was moved), but its content has not been - // edited. +function analyze_edit_history(message) { + // Returns a dict of booleans that describe the message's history: + // * edited: if the message has had its content 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 - // checking position of "EDITED" label as well as the edited timestring + // This function computes data on whether the message was edited + // 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) { - 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 = message_was_only_moved(message_container.msg); - } else { + const edit_history_details = analyze_edit_history(message_container.msg); + + if ( + last_edit_timestr === undefined || + !(edit_history_details.moved || edit_history_details.edited) + ) { + // 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) {