From 44fdbe5f04e43357c3bfba86c3564762b9802022 Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Mon, 24 Mar 2025 19:03:11 +0100 Subject: [PATCH] compose-closed-ui: Refactor get_recipient_label. Refactors get_recipient_label so that it's a bit clearer what the recipient_information parameter is for and what we do when that parameter is undefined. In doing so, we no longer treat the constructed objects, that are passed as the recipient_information parameter, and actual Message objects, that we get from the current message list view, as the same thing. (cherry picked from commit 7d3b77e490d16e7fbafa8077585e2c25e303455d) --- web/src/compose_closed_ui.ts | 71 ++++++++++++++++------------ web/tests/compose_closed_ui.test.cjs | 22 ++++++++- 2 files changed, 62 insertions(+), 31 deletions(-) diff --git a/web/src/compose_closed_ui.ts b/web/src/compose_closed_ui.ts index 029b5add4d..2b385c0b92 100644 --- a/web/src/compose_closed_ui.ts +++ b/web/src/compose_closed_ui.ts @@ -42,35 +42,11 @@ export type ReplyRecipientInformation = { export function get_recipient_label( recipient_information?: ReplyRecipientInformation, ): RecipientLabel | undefined { - // TODO: This code path is bit of a type-checking disaster; we mix - // actual message objects with fake objects containing just a - // couple fields, both those constructed here and potentially - // passed in. - - if (recipient_information === undefined) { - // We check the current message list for information about the - // reply recipient for the closed compose box button label. - if (message_lists.current === undefined) { - return undefined; - } - if (message_lists.current.visibly_empty()) { - // For empty narrows where there's a clear reply target, - // i.e. stream+topic or a single direct message conversation, - // we label the button as replying to the thread. - const stream_id = narrow_state.stream_id(narrow_state.filter(), true); - const topic = narrow_state.topic(); - if (stream_id !== undefined && topic !== undefined) { - return get_stream_recipient_label(stream_id, topic); - } else if (narrow_state.pm_ids_string()) { - const user_ids = people.user_ids_string_to_ids_array(narrow_state.pm_ids_string()!); - return {label_text: message_store.get_pm_full_names(user_ids)}; - } - } else { - recipient_information = message_lists.current.selected_message(); - } - } - - if (recipient_information) { + if (recipient_information !== undefined) { + // If we're in either the Inbox or Recent Conversations view, + // we try to update the closed compose box button label with + // information about the reply target from the focused row in + // the view. if ( recipient_information.stream_id !== undefined && recipient_information.topic !== undefined @@ -79,10 +55,45 @@ export function get_recipient_label( recipient_information.stream_id, recipient_information.topic, ); - } else if (recipient_information.display_reply_to) { + } + if (recipient_information.display_reply_to !== undefined) { return {label_text: recipient_information.display_reply_to}; } } + + // Otherwise, we check the current message list for information + // about the reply target for the closed compose box button label. + if (message_lists.current === undefined) { + return undefined; + } + + if (message_lists.current.visibly_empty()) { + // For empty narrows where there's a clear reply target, + // i.e. channel and topic or a direct message conversation, + // we label the button as replying to the thread. + const stream_id = narrow_state.stream_id(narrow_state.filter(), true); + const topic = narrow_state.topic(); + const user_ids_string = narrow_state.pm_ids_string(); + if (stream_id !== undefined && topic !== undefined) { + return get_stream_recipient_label(stream_id, topic); + } + if (user_ids_string !== undefined) { + const user_ids = people.user_ids_string_to_ids_array(user_ids_string); + return {label_text: message_store.get_pm_full_names(user_ids)}; + } + // Show the standard button text for empty narrows without + // a clear reply target, e.g., an empty search view. + return undefined; + } + + const selected_message = message_lists.current.selected_message(); + if (selected_message !== undefined) { + if (selected_message?.is_stream) { + return get_stream_recipient_label(selected_message.stream_id, selected_message.topic); + } + return {label_text: selected_message.display_reply_to}; + } + // Fall through to show the standard button text. return undefined; } diff --git a/web/tests/compose_closed_ui.test.cjs b/web/tests/compose_closed_ui.test.cjs index 92aa436c0b..fb930686bf 100644 --- a/web/tests/compose_closed_ui.test.cjs +++ b/web/tests/compose_closed_ui.test.cjs @@ -24,6 +24,9 @@ mock_esm("../src/message_list_view", { mock_esm("../src/people.ts", { maybe_get_user_by_id: noop, }); +mock_esm("../src/settings_data", { + user_has_permission_for_group_setting: () => true, +}); const stream_data = zrequire("stream_data"); // Code we're actually using/testing @@ -31,7 +34,10 @@ const compose_closed_ui = zrequire("compose_closed_ui"); const {Filter} = zrequire("filter"); const {MessageList} = zrequire("message_list"); const {MessageListData} = zrequire("message_list_data"); -const {set_realm} = zrequire("state_data"); +const {set_current_user, set_realm} = zrequire("state_data"); + +const current_user = {}; +set_current_user(current_user); const REALM_EMPTY_TOPIC_DISPLAY_NAME = "general chat"; set_realm({realm_empty_topic_display_name: REALM_EMPTY_TOPIC_DISPLAY_NAME}); @@ -74,34 +80,48 @@ run_test("reply_label", () => { [ { id: 0, + is_stream: true, + is_private: false, stream_id: stream_one.stream_id, topic: "first_topic", }, { id: 1, + is_stream: true, + is_private: false, stream_id: stream_one.stream_id, topic: "second_topic", }, { id: 2, + is_stream: true, + is_private: false, stream_id: stream_two.stream_id, topic: "third_topic", }, { id: 3, + is_stream: true, + is_private: false, stream_id: stream_two.stream_id, topic: "second_topic", }, { id: 4, + is_stream: false, + is_private: true, display_reply_to: "some user", }, { id: 5, + is_stream: false, + is_private: true, display_reply_to: "some user, other user", }, { id: 6, + is_stream: true, + is_private: false, stream_id: stream_two.stream_id, topic: "", },