diff --git a/web/src/message_edit_history.ts b/web/src/message_edit_history.ts index 6960c365d2..31ee9187af 100644 --- a/web/src/message_edit_history.ts +++ b/web/src/message_edit_history.ts @@ -332,8 +332,11 @@ export function initialize(): void { } if ( - realm.realm_message_edit_history_visibility_policy !== - message_edit_history_visibility_policy_values.never.code + realm.realm_message_edit_history_visibility_policy === + message_edit_history_visibility_policy_values.always.code || + (realm.realm_message_edit_history_visibility_policy === + message_edit_history_visibility_policy_values.moves_only.code && + message.last_moved_timestamp !== undefined) ) { fetch_and_render_message_history(message); $("#message-history-overlay .exit-sign").trigger("focus"); diff --git a/web/src/message_events.ts b/web/src/message_events.ts index 6ab459d436..91a72fa294 100644 --- a/web/src/message_events.ts +++ b/web/src/message_events.ts @@ -3,6 +3,8 @@ import _ from "lodash"; import assert from "minimalistic-assert"; import {z} from "zod"; +import * as resolved_topic from "../shared/src/resolved_topic.ts"; + import * as activity from "./activity.ts"; import * as alert_words from "./alert_words.ts"; import * as channel from "./channel.ts"; @@ -375,6 +377,16 @@ export function insert_new_messages( return messages; } +function topic_resolve_toggled(new_topic: string, original_topic: string): boolean { + if (resolved_topic.is_resolved(new_topic) && new_topic.slice(2) === original_topic) { + return true; + } + if (resolved_topic.is_resolved(original_topic) && original_topic.slice(2) === new_topic) { + return true; + } + return false; +} + export function update_messages(events: UpdateMessageEvent[]): void { const messages_to_rerender: Message[] = []; let changed_narrow = false; @@ -582,7 +594,15 @@ export function update_messages(events: UpdateMessageEvent[]): void { ...moved_message.edit_history, ]; } - moved_message.last_edit_timestamp = event.edit_timestamp; + + if (stream_changed) { + moved_message.last_moved_timestamp = event.edit_timestamp; + } else if (topic_edited) { + assert(new_topic !== undefined); + if (!topic_resolve_toggled(new_topic, orig_topic)) { + moved_message.last_moved_timestamp = event.edit_timestamp; + } + } // Update the unread counts; again, this must be called // before we modify the topic field on the message. @@ -753,7 +773,7 @@ export function update_messages(events: UpdateMessageEvent[]): void { // flag is used to indicated update_message events that are // triggered by server latency optimizations, not user // interactions; these should not generate edit history updates. - if (!event.rendering_only) { + if (!event.rendering_only && any_message_content_edited) { anchor_message.last_edit_timestamp = event.edit_timestamp; } diff --git a/web/src/message_list_tooltips.ts b/web/src/message_list_tooltips.ts index c7e53db1b9..b57b932f5f 100644 --- a/web/src/message_list_tooltips.ts +++ b/web/src/message_list_tooltips.ts @@ -384,20 +384,36 @@ export function initialize(): void { if (!message_container.modified) { return false; } - // We know the message has been modified, so we either have a timestamp - // from the server or from a local edit. - assert(message_container.last_edit_timestamp !== undefined); - const last_modified_time_string = get_time_string( - message_container.last_edit_timestamp, - ); + let edited_time_string = ""; + let moved_time_string = ""; + if (message_container.edited) { + // We know the message has been edited, so we either have a timestamp + // from the server or from a local edit. + assert(message_container.last_edit_timestamp !== undefined); + edited_time_string = get_time_string(message_container.last_edit_timestamp); + } + if (message_container.moved) { + // We know the message has been moved, so we have a timestamp from + // the server. + assert(message_container.last_moved_timestamp !== undefined); + moved_time_string = get_time_string(message_container.last_moved_timestamp); + } + const edit_history_access = + realm.realm_message_edit_history_visibility_policy === + message_edit_history_visibility_policy_values.always.code; + const message_moved_and_move_history_access = + realm.realm_message_edit_history_visibility_policy === + message_edit_history_visibility_policy_values.moves_only.code && + message_container.moved; instance.setContent( parse_html( render_message_edit_notice_tooltip({ moved: message_container.moved, - last_edit_timestr: last_modified_time_string, - realm_message_edit_history_is_visible: - realm.realm_message_edit_history_visibility_policy !== - message_edit_history_visibility_policy_values.never.code, + edited: message_container.edited, + edited_time_string, + moved_time_string, + edit_history_access, + message_moved_and_move_history_access, }), ), ); diff --git a/web/src/message_list_view.ts b/web/src/message_list_view.ts index 0a402085f8..3ea4adef7d 100644 --- a/web/src/message_list_view.ts +++ b/web/src/message_list_view.ts @@ -55,11 +55,13 @@ export type MessageContainer = { include_sender: boolean; is_hidden: boolean; last_edit_timestamp: number | undefined; + last_moved_timestamp: number | undefined; mention_classname: string | undefined; message_edit_notices_in_left_col: boolean; message_edit_notices_alongside_sender: boolean; message_edit_notices_for_status_message: boolean; modified: boolean; + edited: boolean; moved: boolean; msg: Message; sender_is_bot: boolean; @@ -164,69 +166,6 @@ function same_recipient(a: MessageContainer | undefined, b: MessageContainer | u return util.same_recipient(a.msg, b.msg); } -function analyze_edit_history( - message: Message, - last_edit_timestamp: number | undefined, -): { - edited: boolean; - moved: boolean; - resolve_toggled: boolean; -} { - // 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) { - edited = true; - } - - if (edit_history_event.prev_stream) { - moved = true; - } - - if (edit_history_event.prev_topic !== undefined) { - // TODO: Possibly this assert could be removed if we tightened the type - // on edit history elements such that a `prev_topic` being present means a - // `topic` element is. - assert(edit_history_event.topic !== undefined); - // 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; - } - } - } else if (last_edit_timestamp !== undefined) { - // When the edit_history is disabled for the organization, we do not receive the edit_history - // variable in the message object. In this case, we will check if the last_edit_timestamp is - // available or not. Since we don't have the edit_history, we can't determine if the message - // was moved or edited. Therefore, we simply mark the messages as edited. - edited = true; - } - return {edited, moved, resolve_toggled}; -} - function get_group_display_date(message: Message, display_year: boolean): string { const time = new Date(message.timestamp * 1000); const date_element = timerender.render_date(time, display_year); @@ -592,8 +531,10 @@ export class MessageListView { ); } - _get_message_edited_vars(message: Message): { + _get_message_edited_and_moved_vars(message: Message): { last_edit_timestamp: number | undefined; + last_moved_timestamp: number | undefined; + edited: boolean; moved: boolean; modified: boolean; } { @@ -603,25 +544,14 @@ export class MessageListView { } else { last_edit_timestamp = message.last_edit_timestamp; } - const edit_history_details = analyze_edit_history(message, last_edit_timestamp); - - if (!last_edit_timestamp || !(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). - return { - last_edit_timestamp: undefined, - moved: false, - modified: false, - }; - } + const last_moved_timestamp = message.last_moved_timestamp; return { last_edit_timestamp, - moved: edit_history_details.moved && !edit_history_details.edited, - modified: true, + last_moved_timestamp, + edited: last_edit_timestamp !== undefined, + moved: last_moved_timestamp !== undefined, + modified: last_edit_timestamp !== undefined || last_moved_timestamp !== undefined, }; } @@ -646,6 +576,8 @@ export class MessageListView { include_sender: boolean; status_message: string | false; last_edit_timestamp: number | undefined; + last_moved_timestamp: number | undefined; + edited: boolean; moved: boolean; modified: boolean; } { @@ -745,7 +677,7 @@ export class MessageListView { mention_classname, include_sender, ...this._maybe_get_me_message(is_hidden, message), - ...this._get_message_edited_vars(message), + ...this._get_message_edited_and_moved_vars(message), }; } diff --git a/web/src/message_store.ts b/web/src/message_store.ts index a6a548176a..6a9509624a 100644 --- a/web/src/message_store.ts +++ b/web/src/message_store.ts @@ -74,6 +74,7 @@ export const raw_message_schema = z.intersection( id: z.number(), is_me_message: z.boolean(), last_edit_timestamp: z.optional(z.number()), + last_moved_timestamp: z.optional(z.number()), reactions: z.array(message_reaction_schema), recipient_id: z.number(), sender_email: z.string(), diff --git a/web/src/server_message.ts b/web/src/server_message.ts index b18e306a3d..7b7a27bb1c 100644 --- a/web/src/server_message.ts +++ b/web/src/server_message.ts @@ -49,6 +49,7 @@ export const server_message_schema = z id: z.number(), is_me_message: z.boolean(), last_edit_timestamp: z.number().optional(), + last_moved_timestamp: z.number().optional(), reactions: message_reaction_schema, recipient_id: z.number(), sender_email: z.string(), diff --git a/web/templates/edited_notice.hbs b/web/templates/edited_notice.hbs index 7a7412e4d9..2537c7585d 100644 --- a/web/templates/edited_notice.hbs +++ b/web/templates/edited_notice.hbs @@ -4,13 +4,13 @@
{{t "SAVING"}}
- {{else if moved}} -
- {{t "MOVED"}} -
- {{else}} + {{else if edited}}
{{t "EDITED"}}
+ {{else}} +
+ {{t "MOVED"}} +
{{/if}} {{/if}} diff --git a/web/templates/message_edit_notice_tooltip.hbs b/web/templates/message_edit_notice_tooltip.hbs index 3f27632399..0940140c6e 100644 --- a/web/templates/message_edit_notice_tooltip.hbs +++ b/web/templates/message_edit_notice_tooltip.hbs @@ -1,15 +1,30 @@
- {{#if realm_message_edit_history_is_visible}} -
{{t "View edit history"}}
- {{/if}} -
- {{#if moved}} - {{t 'Last moved {last_edit_timestr}.'}} - {{else}} - {{t 'Last edited {last_edit_timestr}.'}} + {{#if edit_history_access}} + {{#if edited}} + {{#if moved}} +
{{t "View edit and move history"}}
+ {{else}} +
{{t "View edit history"}}
+ {{/if}} + {{else if moved}} +
{{t "View move history"}}
{{/if}} + {{else if message_moved_and_move_history_access}} +
{{t "View move history"}}
+ {{/if}} + {{#if edited}} +
+ {{t 'Last edited {edited_time_string}.'}} +
+ {{/if}} + {{#if moved}} +
+ {{t 'Last moved {moved_time_string}.'}}
+ {{/if}}
-{{#if realm_message_edit_history_is_visible}} +{{#if edit_history_access}} +{{tooltip_hotkey_hints "Shift" "H"}} +{{else if message_moved_and_move_history_access}} {{tooltip_hotkey_hints "Shift" "H"}} {{/if}} diff --git a/web/tests/message_list_view.test.cjs b/web/tests/message_list_view.test.cjs index a152833916..0a22292d13 100644 --- a/web/tests/message_list_view.test.cjs +++ b/web/tests/message_list_view.test.cjs @@ -45,27 +45,9 @@ function test(label, f) { }); } -test("msg_moved_var", () => { +test("msg_edited_and_moved_vars", () => { // This is a test to verify that when the stream or topic is changed // (and the content is not), the message says "MOVED" rather than "EDITED." - // See the end of the test for the list of cases verified. - - function build_message_context(message = {}, message_context = {}) { - message_context = { - ...message_context, - }; - if ("edit_history" in message) { - message_context.msg = { - last_edit_timestamp: (next_timestamp += 1), - ...message, - }; - } else { - message_context.msg = { - ...message, - }; - } - return message_context; - } function build_message_group(messages) { return {message_containers: messages}; @@ -83,85 +65,29 @@ test("msg_moved_var", () => { return list; } - function assert_moved_true(message_container) { - assert.equal(message_container.moved, true); - } - function assert_moved_false(message_container) { - assert.equal(message_container.moved, false); - } - (function test_msg_moved_var() { const messages = [ - // no edit history: NO LABEL - build_message_context({}), - // stream changed: MOVED - build_message_context({ - edit_history: [{prev_stream: 1, timestamp: 1000, user_id: 1}], - }), - // topic changed (not resolved/unresolved): MOVED - build_message_context({ - edit_history: [ - {prev_topic: "test_topic", topic: "new_topic", timestamp: 1000, user_id: 1}, - ], - }), - // content edited: EDITED - build_message_context({ - edit_history: [{prev_content: "test_content", timestamp: 1000, user_id: 1}], - }), - // stream and topic edited: MOVED - build_message_context({ - edit_history: [ - { - prev_stream: 1, - prev_topic: "test_topic", - topic: "new_topic", - timestamp: 1000, - user_id: 1, - }, - ], - }), - // topic and content changed: EDITED - build_message_context({ - edit_history: [ - { - prev_topic: "test_topic", - topic: "new_topic", - prev_content: "test_content", - timestamp: 1000, - user_id: 1, - }, - ], - }), - // only topic resolved: NO LABEL - build_message_context({ - edit_history: [ - {prev_topic: "test_topic", topic: "✔ test_topic", timestamp: 1000, user_id: 1}, - ], - }), - // only topic unresolved: NO LABEL - build_message_context({ - edit_history: [ - {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}, - ], - }), + // no edit or moved timestamps + {msg: {}}, + // edit timestamp: EDITED + { + msg: { + last_edit_timestamp: (next_timestamp += 1), + }, + }, + // moved timestamp: MOVED + { + msg: { + last_moved_timestamp: (next_timestamp += 1), + }, + }, + // both edit and moved timestamp: EDITED + { + msg: { + last_edit_timestamp: (next_timestamp += 1), + last_moved_timestamp: (next_timestamp += 1), + }, + }, ]; const message_group = build_message_group(messages); @@ -171,32 +97,28 @@ test("msg_moved_var", () => { Object.assign( message_container, list._maybe_get_me_message(message_container.is_hidden, message_container.msg), - list._get_message_edited_vars(message_container.msg), + list._get_message_edited_and_moved_vars(message_container.msg), ); } const result = list._message_groups[0].message_containers; - // no edit history: false - assert_moved_false(result[0]); - // stream changed: true - assert_moved_true(result[1]); - // topic changed: true - assert_moved_true(result[2]); - // content edited: false - assert_moved_false(result[3]); - // stream and topic edited: true - assert_moved_true(result[4]); - // topic and content changed: false - assert_moved_false(result[5]); - // only topic resolved: false - assert_moved_false(result[6]); - // only topic unresolved: false - assert_moved_false(result[7]); - // multiple edits with content edit: false - assert_moved_false(result[8]); - // multiple edits without content edit: true - assert_moved_true(result[9]); + // no edit or moved timestamps + assert.equal(result[0].edited, false); + assert.equal(result[0].moved, false); + assert.equal(result[0].modified, false); + // edit timestamp: EDITED + assert.equal(result[1].edited, true); + assert.equal(result[1].moved, false); + assert.equal(result[1].modified, true); + // moved timestamp: MOVED + assert.equal(result[2].edited, false); + assert.equal(result[2].moved, true); + assert.equal(result[2].modified, true); + // both edit and moved timestamp: EDITED + assert.equal(result[3].edited, true); + assert.equal(result[3].moved, true); + assert.equal(result[3].modified, true); })(); }); @@ -283,7 +205,7 @@ test("message_edited_vars", () => { Object.assign( message_container, list._maybe_get_me_message(message_container.is_hidden, message_container.msg), - list._get_message_edited_vars(message_container.msg), + list._get_message_edited_and_moved_vars(message_container.msg), ); } @@ -368,7 +290,7 @@ test("muted_message_vars", () => { ]; const message_group = build_message_group(messages); const list = build_list([message_group]); - list._get_message_edited_vars = noop; + list._get_message_edited_and_moved_vars = noop; // Sender is not muted. let result = calculate_variables(list, messages);