Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning banner if moving topic or multiple messages to a pre-existing conversation. #31151 #31279

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions web/src/stream_popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import $ from "jquery";
import assert from "minimalistic-assert";

import render_inline_decorated_stream_name from "../templates/inline_decorated_stream_name.hbs";
import render_message_moving_warning_banner from "../templates/modal_banner/message_moving_warning_banner.hbs";
import render_move_topic_to_stream from "../templates/move_topic_to_stream.hbs";
import render_left_sidebar_stream_actions_popover from "../templates/popovers/left_sidebar/left_sidebar_stream_actions_popover.hbs";

import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as compose_actions from "./compose_actions";
import * as compose_banner from "./compose_banner";
import * as composebox_typeahead from "./composebox_typeahead";
import * as dialog_widget from "./dialog_widget";
import * as dropdown_widget from "./dropdown_widget";
Expand All @@ -26,6 +28,7 @@ import * as stream_data from "./stream_data";
import * as stream_settings_api from "./stream_settings_api";
import * as stream_settings_components from "./stream_settings_components";
import * as stream_settings_ui from "./stream_settings_ui";
import * as stream_topic_history from "./stream_topic_history";
import * as sub_store from "./sub_store";
import * as ui_report from "./ui_report";
import * as ui_util from "./ui_util";
Expand Down Expand Up @@ -557,6 +560,7 @@ export async function build_move_topic_to_stream_popover(
stream_widget_value = $(event.currentTarget).attr("data-unique-id");

update_submit_button_disabled_state(stream_widget_value);
update_warning_banner();
set_stream_topic_typeahead();
render_selected_stream();

Expand All @@ -565,9 +569,55 @@ export async function build_move_topic_to_stream_popover(
event.stopPropagation();
}

function update_warning_banner() {
// The warning banner only shows when
// 1) submit button is NOT disabled, and
// 2) target topic is an existing topic, and
// 3) propagate_mode is NOT change_one

// submit button
if ($("#move_topic_modal .dialog_submit_button")[0].disabled) {
$("#move_topic_modal .move_topic_form_warning_container").html("");
return;
}

// target topic
if (stream_widget_value !== undefined) {
const {new_topic_name} = get_params_from_form();
if (
!stream_topic_history.stream_has_topic(
Number.parseInt(stream_widget_value, 10),
new_topic_name,
)
) {
$("#move_topic_modal .move_topic_form_warning_container").html("");
return;
}
}
Comment on lines +584 to +596
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can simplify this nested if statement into two statements on the same level.


// propagate_mode
if ($("#move_topic_modal select.message_edit_topic_propagate").val() === "change_one") {
$("#move_topic_modal .move_topic_form_warning_container").html("");
return;
}

$("#move_topic_modal .move_topic_form_warning_container").html(
render_message_moving_warning_banner({
banner_type: compose_banner.WARNING,
hide_close_button: true,
classname: "message_moving_warning_banner",
}),
);
}

function move_topic_post_render() {
$("#move_topic_modal .dialog_submit_button").prop("disabled", true);

$("#move_topic_modal select.message_edit_topic_propagate")[0]?.addEventListener(
"change",
() => update_warning_banner(),
);

const $topic_input = $("#move_topic_form .move_messages_edit_topic");
move_topic_to_stream_topic_typeahead = composebox_typeahead.initialize_topic_edit_typeahead(
$topic_input,
Expand All @@ -581,6 +631,7 @@ export async function build_move_topic_to_stream_popover(
const select_stream_id = current_stream_id;
$topic_input.on("input", () => {
update_submit_button_disabled_state(select_stream_id);
update_warning_banner();
});
return;
}
Expand Down Expand Up @@ -609,6 +660,7 @@ export async function build_move_topic_to_stream_popover(
$("#move_topic_to_stream_widget").prop("disabled", disable_stream_input);
$("#move_topic_modal .move_messages_edit_topic").on("input", () => {
update_submit_button_disabled_state(stream_widget_value);
update_warning_banner();
});
}

Expand Down
15 changes: 15 additions & 0 deletions web/src/stream_topic_history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,17 @@ export function stream_has_topics(stream_id: number): boolean {
return history.has_topics();
}

export function stream_has_topic(stream_id: number, topic_name: string): boolean {
if (!stream_dict.has(stream_id)) {
return false;
}

const history = stream_dict.get(stream_id);
assert(history !== undefined);

return history.has_topic(topic_name);
}

export type TopicHistoryEntry = {
count: number;
message_id: number;
Expand Down Expand Up @@ -126,6 +137,10 @@ export class PerStreamHistory {
return this.topics.size !== 0;
}

has_topic(topic_name: string): boolean {
return this.topics.has(topic_name);
}

update_stream_with_message_id(message_id: number): void {
if (message_id > this.max_message_id) {
this.max_message_id = message_id;
Expand Down
7 changes: 7 additions & 0 deletions web/templates/modal_banner/message_moving_warning_banner.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{{#> modal_banner }}
<p class="banner_message">
{{#tr}}
You are moving messages to a topic that already exists. Messages from these topics will be combined.
heyiming marked this conversation as resolved.
Show resolved Hide resolved
{{/tr}}
</p>
{{/modal_banner}}
1 change: 1 addition & 0 deletions web/templates/move_topic_to_stream.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<p class="white-space-preserve-wrap">{{#tr}}Move all messages in <strong>{topic_name}</strong>{{/tr}} to:</p>
{{/unless}}
<form id="move_topic_form">
<div class="move_topic_form_warning_container"></div>
{{#if only_topic_edit }}
<p>{{t "Rename topic to:" }}</p>
{{else if from_message_actions_popover}}
Expand Down
19 changes: 19 additions & 0 deletions web/tests/stream_topic_history.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,25 @@ test("test_stream_has_topics", () => {
assert.equal(stream_topic_history.stream_has_topics(stream_id), true);
});

test("test_stream_has_topic", () => {
const stream_id = 88;
const topic_name = "fake topic name";

assert.equal(stream_topic_history.stream_has_topic(stream_id, topic_name), false);

stream_topic_history.find_or_create(stream_id);

assert.equal(stream_topic_history.stream_has_topic(stream_id, topic_name), false);

stream_topic_history.add_message({
stream_id,
message_id: 888,
topic_name,
});

assert.equal(stream_topic_history.stream_has_topic(stream_id, topic_name), true);
});

test("server_history_end_to_end", () => {
stream_topic_history.reset();

Expand Down