From a77fc6aa7921d4b549ea0ff42c0a865784be60d3 Mon Sep 17 00:00:00 2001 From: Vector73 Date: Tue, 8 Jul 2025 04:46:33 +0000 Subject: [PATCH] stream_settings: Add new disable_topics option to topics_policy. Adds new configuration option `disable_topics` in `topics_policy` channel setting to support disabling topics in the channel. Fixes #34553. --- api_docs/changelog.md | 11 +++++ tools/linter_lib/custom_check.py | 1 + version.py | 2 +- web/src/compose_recipient.ts | 19 +++++++- web/src/hash_util.ts | 3 +- web/src/message_edit.ts | 5 ++ web/src/popover_menus_data.ts | 5 +- web/src/settings_components.ts | 29 +++++++++++ web/src/settings_config.ts | 9 ++++ web/src/stream_data.ts | 20 +++++++- web/src/stream_list.ts | 8 ++++ web/src/stream_popover.ts | 48 ++++++++++++++++--- web/src/stream_topic_history.ts | 17 +++++++ web/src/stream_types.ts | 1 + web/src/tippyjs.ts | 25 ++++++++++ web/styles/compose.css | 17 ++++++- web/styles/popovers.css | 4 ++ web/styles/subscriptions.css | 3 +- .../stream_settings/stream_permissions.hbs | 1 + .../topics_already_exist_error.hbs | 7 +++ web/templates/topics_not_allowed_error.hbs | 4 ++ web/tests/message_edit.test.cjs | 1 + web/tests/popover_menus_data.test.cjs | 1 + web/tests/settings_org.test.cjs | 1 + web/tests/stream_data.test.cjs | 27 +++++++++++ web/tests/stream_topic_history.test.cjs | 11 +++++ zerver/actions/message_edit.py | 19 +++++--- zerver/actions/message_send.py | 16 +++---- zerver/actions/streams.py | 3 ++ zerver/lib/exceptions.py | 26 ++++++++++ zerver/lib/streams.py | 10 ++++ zerver/models/streams.py | 1 + zerver/openapi/zulip.yaml | 13 ++++- zerver/tests/test_message_move_stream.py | 22 ++++++++- zerver/tests/test_message_move_topic.py | 22 ++++++++- zerver/tests/test_message_send.py | 34 +++++++++++++ zerver/tests/test_subs.py | 40 ++++++++++++++++ zerver/views/streams.py | 24 +++++++--- 38 files changed, 472 insertions(+), 38 deletions(-) create mode 100644 web/templates/stream_settings/topics_already_exist_error.hbs create mode 100644 web/templates/topics_not_allowed_error.hbs diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 994d08cd38..79df10d4b7 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,17 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 11.0 +**Feature level 404** + +* [`GET /users/me/subscriptions`](/api/get-subscriptions), + [`GET /streams`](/api/get-streams), [`GET /events`](/api/get-events), + [`POST /register`](/api/register-queue): Added new `empty_topic_only` + option to `topics_policy` field in Stream and Subscription objects. +* [`POST /users/me/subscriptions`](/api/subscribe), + [`PATCH /streams/{stream_id}`](/api/update-stream): Added new + `empty_topic_only` option to `topics_policy` to support channels + with topics disabled. + **Feature level 403** * [`POST /register`](/api/register-queue): Added a `url_options` object diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index 9fe2063b14..bdca6a2ff7 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -250,6 +250,7 @@ python_rules = RuleList( "exclude": FILES_WITH_LEGACY_SUBJECT, "exclude_line": { ("zerver/lib/message.py", "message__subject__iexact=message.topic_name(),"), + ("zerver/lib/streams.py", '.exclude(subject="")'), ("zerver/views/streams.py", "message__subject__iexact=topic_name,"), ("zerver/lib/message_cache.py", 'and obj["subject"] == ""'), ( diff --git a/version.py b/version.py index 4b4b1216f7..401f68f308 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 403 +API_FEATURE_LEVEL = 404 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/web/src/compose_recipient.ts b/web/src/compose_recipient.ts index fdfd326380..f2f9dab048 100644 --- a/web/src/compose_recipient.ts +++ b/web/src/compose_recipient.ts @@ -325,6 +325,8 @@ function on_hidden_callback(): void { return; } if (compose_state.get_message_type() === "stream") { + // Enable or disable topic input based on `topics_policy`. + update_topic_displayed_text(compose_state.topic()); // Always move focus to the topic input even if it's not empty, // since it's likely the user will want to update the topic // after updating the stream. @@ -399,16 +401,17 @@ export function update_topic_displayed_text(topic_name = "", has_topic_focus = f compose_state.topic(topic_name); const $input = $("input#stream_message_recipient_topic"); - const is_empty_string_topic = topic_name === ""; const recipient_widget_hidden = $(".compose_select_recipient-dropdown-list-container").length === 0; const $topic_not_mandatory_placeholder = $("#topic-not-mandatory-placeholder"); // reset + $input.prop("disabled", false); $input.attr("placeholder", ""); - $input.removeClass("empty-topic-display"); + $input.removeClass("empty-topic-display empty-topic-only"); $topic_not_mandatory_placeholder.removeClass("visible"); $topic_not_mandatory_placeholder.hide(); + $("#compose_recipient_box").removeClass("disabled"); if (!stream_data.can_use_empty_topic(compose_state.stream_id())) { $input.attr("placeholder", $t({defaultMessage: "Topic"})); @@ -417,6 +420,17 @@ export function update_topic_displayed_text(topic_name = "", has_topic_focus = f // placeholder will always be "Topic" and never "general chat". return; } + + // If `topics_policy` is set to `empty_topic_only`, disable the topic input + // and empty the input box. + if (stream_data.is_empty_topic_only_channel(compose_state.stream_id())) { + compose_state.topic(""); + $input.prop("disabled", true); + $input.addClass("empty-topic-only"); + $("#compose_recipient_box").addClass("disabled"); + $("textarea#compose-textarea").trigger("focus"); + has_topic_focus = false; + } // Otherwise, we have some adjustments to make to display: // * a placeholder with the default topic name stylized // * the empty string topic stylized @@ -424,6 +438,7 @@ export function update_topic_displayed_text(topic_name = "", has_topic_focus = f $topic_not_mandatory_placeholder.toggleClass("visible", $input.val() === ""); } + const is_empty_string_topic = compose_state.topic() === ""; if (is_empty_string_topic && !has_topic_focus && recipient_widget_hidden) { $input.attr("placeholder", util.get_final_topic_display_name("")); $input.addClass("empty-topic-display"); diff --git a/web/src/hash_util.ts b/web/src/hash_util.ts index 6eeb2982c1..966a4dc319 100644 --- a/web/src/hash_util.ts +++ b/web/src/hash_util.ts @@ -93,7 +93,8 @@ export function by_stream_url(stream_id: number): string { export function channel_url_by_user_setting(channel_id: number): string { if ( user_settings.web_channel_default_view === - web_channel_default_view_values.list_of_topics.code + web_channel_default_view_values.list_of_topics.code && + !stream_data.is_empty_topic_only_channel(channel_id) ) { return by_channel_topic_list_url(channel_id); } diff --git a/web/src/message_edit.ts b/web/src/message_edit.ts index 6c2d4292f8..5e8a398e9c 100644 --- a/web/src/message_edit.ts +++ b/web/src/message_edit.ts @@ -112,6 +112,11 @@ export function is_topic_editable(message: Message, edit_limit_seconds_buffer = return false; } + // Cannot edit topics only in the channel with topics disabled. + if (stream_data.is_empty_topic_only_channel(message.stream_id)) { + return false; + } + const stream = stream_data.get_sub_by_id(message.stream_id); assert(stream !== undefined); if (!stream_data.user_can_move_messages_within_channel(stream)) { diff --git a/web/src/popover_menus_data.ts b/web/src/popover_menus_data.ts index dd56f1220e..11fbf5d66a 100644 --- a/web/src/popover_menus_data.ts +++ b/web/src/popover_menus_data.ts @@ -267,8 +267,11 @@ export function get_topic_popover_content_context({ const has_starred_messages = starred_messages.get_count_in_topic(sub.stream_id, topic_name) > 0; const has_unread_messages = num_unread_for_topic(sub.stream_id, topic_name) > 0; const can_move_topic = stream_data.user_can_move_messages_out_of_channel(sub); - const can_rename_topic = stream_data.user_can_move_messages_within_channel(sub); + const can_rename_topic = + stream_data.user_can_move_messages_within_channel(sub) && + !stream_data.is_empty_topic_only_channel(sub.stream_id); const can_resolve_topic = !sub.is_archived && stream_data.can_resolve_topics(sub); + const visibility_policy = user_topics.get_topic_visibility_policy(sub.stream_id, topic_name); const all_visibility_policies = user_topics.all_visibility_policies; const is_spectator = page_params.is_spectator; diff --git a/web/src/settings_components.ts b/web/src/settings_components.ts index 7d30933230..b50fe7deeb 100644 --- a/web/src/settings_components.ts +++ b/web/src/settings_components.ts @@ -29,6 +29,7 @@ import type {CustomProfileField, GroupSettingValue} from "./state_data.ts"; import {current_user, realm, realm_schema} from "./state_data.ts"; import * as stream_data from "./stream_data.ts"; import * as stream_settings_containers from "./stream_settings_containers.ts"; +import * as stream_topic_history from "./stream_topic_history.ts"; import { type StreamPermissionGroupSetting, stream_permission_group_settings_schema, @@ -1471,9 +1472,37 @@ function should_disable_save_button_for_group_settings(settings: string[]): bool return false; } +function should_disable_save_button_for_stream_settings(stream_id: number): boolean { + // Disable save button if "Only general chat topic allowed" option is selected + // for `topics_policy` and there are topics other than the general chat in the + // current channel. + const topics_policy_value = $("#id_topics_policy").val(); + if ( + topics_policy_value === + settings_config.get_stream_topics_policy_values().empty_topic_only.code && + stream_topic_history.stream_has_locally_available_named_topics(stream_id) + ) { + $("#settings-topics-already-exist-error").show(); + return true; + } + $("#settings-topics-already-exist-error").hide(); + return false; +} + function enable_or_disable_save_button($subsection_elem: JQuery): void { const $save_button = $subsection_elem.find(".save-button"); + if ($subsection_elem.closest(".advanced-configurations-container").length > 0) { + const $settings_container = $subsection_elem.closest(".subscription_settings"); + const stream_id_string = $settings_container.attr("data-stream-id"); + assert(stream_id_string !== undefined); + const stream_id = Number.parseInt(stream_id_string, 10); + if (should_disable_save_button_for_stream_settings(stream_id)) { + $save_button.prop("disabled", true); + return; + } + } + const time_limit_settings = [...$subsection_elem.find(".time-limit-setting")]; if ( time_limit_settings.length > 0 && diff --git a/web/src/settings_config.ts b/web/src/settings_config.ts index 0bf1f9a01e..91458da578 100644 --- a/web/src/settings_config.ts +++ b/web/src/settings_config.ts @@ -309,6 +309,7 @@ type RealmTopicsPolicyValues = { type StreamTopicsPolicyValues = { inherit: PolicyValue; + empty_topic_only: PolicyValue; } & RealmTopicsPolicyValues; export const get_realm_topics_policy_values = (): RealmTopicsPolicyValues => { @@ -331,6 +332,7 @@ export const get_realm_topics_policy_values = (): RealmTopicsPolicyValues => { export const get_stream_topics_policy_values = (): StreamTopicsPolicyValues => { const realm_topics_policy_values = get_realm_topics_policy_values(); + const empty_topic_name = util.get_final_topic_display_name(""); return { inherit: { @@ -338,6 +340,13 @@ export const get_stream_topics_policy_values = (): StreamTopicsPolicyValues => { description: $t({defaultMessage: "Automatic"}), }, ...realm_topics_policy_values, + empty_topic_only: { + code: "empty_topic_only", + description: $t( + {defaultMessage: 'Only "{empty_topic_name}" topic allowed'}, + {empty_topic_name}, + ), + }, }; }; diff --git a/web/src/stream_data.ts b/web/src/stream_data.ts index a0d488a7ef..ead30fd5cd 100644 --- a/web/src/stream_data.ts +++ b/web/src/stream_data.ts @@ -1111,7 +1111,25 @@ export function can_use_empty_topic(stream_id: number | undefined): boolean { topics_policy = realm.realm_topics_policy; } return ( - topics_policy === settings_config.get_realm_topics_policy_values().allow_empty_topic.code + topics_policy === + settings_config.get_stream_topics_policy_values().allow_empty_topic.code || + topics_policy === settings_config.get_stream_topics_policy_values().empty_topic_only.code + ); +} + +export function is_empty_topic_only_channel(stream_id: number | undefined): boolean { + if (stream_id === undefined) { + return false; + } + const sub = sub_store.get(stream_id); + assert(sub !== undefined); + + let topics_policy = sub.topics_policy; + if (sub.topics_policy === settings_config.get_stream_topics_policy_values().inherit.code) { + topics_policy = realm.realm_topics_policy; + } + return ( + topics_policy === settings_config.get_stream_topics_policy_values().empty_topic_only.code ); } diff --git a/web/src/stream_list.ts b/web/src/stream_list.ts index ac09cca04a..ba33d6bd7e 100644 --- a/web/src/stream_list.ts +++ b/web/src/stream_list.ts @@ -919,6 +919,14 @@ export function set_event_handlers({ const current_narrow_stream_id = narrow_state.stream_id(); const current_topic = narrow_state.topic(); + if (stream_data.is_empty_topic_only_channel(stream_id)) { + // If the channel doesn't support topics, take you + // directly to general chat regardless of settings. + const empty_topic_url = hash_util.by_channel_topic_permalink(stream_id, ""); + browser_history.go_to_location(empty_topic_url); + return; + } + if ( user_settings.web_channel_default_view === web_channel_default_view_values.list_of_topics.code diff --git a/web/src/stream_popover.ts b/web/src/stream_popover.ts index 9dbef6d169..b84c4628b4 100644 --- a/web/src/stream_popover.ts +++ b/web/src/stream_popover.ts @@ -672,6 +672,11 @@ export async function build_move_topic_to_stream_popover( const send_notification_to_old_thread = params.send_notification_to_old_thread === "on"; const current_stream_id = Number.parseInt(params.current_stream_id, 10); + // Can only move to empty topic if topics are disabled in the destination channel. + if (stream_data.is_empty_topic_only_channel(select_stream_id ?? current_stream_id)) { + new_topic_name = ""; + } + if (new_topic_name !== undefined) { // new_topic_name can be undefined when the new topic input is disabled when // user does not have permission to edit topic. @@ -794,6 +799,11 @@ export async function build_move_topic_to_stream_popover( // user does not have permission to edit topic. new_topic_name = args.topic_name; } + + if (stream_data.is_empty_topic_only_channel(selected_stream.stream_id)) { + new_topic_name = ""; + } + assert(new_topic_name !== undefined); // Don't show warning for empty topic as the user is probably // about to type a new topic name. Note that if topics are @@ -849,6 +859,19 @@ export async function build_move_topic_to_stream_popover( } } + function disable_topic_input_if_topics_are_disabled_in_channel(stream_id: number): void { + const $topic_input = $("#move_topic_form input.move_messages_edit_topic"); + if (stream_data.is_empty_topic_only_channel(stream_id)) { + $topic_input.val(""); + $topic_input.prop("disabled", true); + $topic_input.addClass("empty-topic-only"); + update_topic_input_placeholder_on_blur(); + } else { + // Removes tooltip if topics are allowed. + $topic_input.removeClass("empty-topic-only"); + } + } + function move_topic_on_update(event: JQuery.ClickEvent, dropdown: {hide: () => void}): void { stream_widget_value = Number.parseInt($(event.currentTarget).attr("data-unique-id")!, 10); const $topic_input = $("#move_topic_form input.move_messages_edit_topic"); @@ -877,6 +900,8 @@ export async function build_move_topic_to_stream_popover( $topic_input.prop("disabled", true); } + disable_topic_input_if_topics_are_disabled_in_channel(selected_stream.stream_id); + const topic_input_value = $topic_input.val(); assert(topic_input_value !== undefined); @@ -893,8 +918,11 @@ export async function build_move_topic_to_stream_popover( event.preventDefault(); event.stopPropagation(); - // Move focus to the topic input after a new stream is selected. - $topic_input.trigger("focus"); + // Move focus to the topic input after a new stream is selected + // if it is not disabled. + if (!$topic_input.prop("disabled")) { + $topic_input.trigger("focus"); + } } // The following logic is correct only when @@ -1085,14 +1113,21 @@ export async function build_move_topic_to_stream_popover( // If the user can't edit the topic, it is not possible for them to make // following kind of moves: // 1) messages from empty topics to channels where empty topics are disabled. + // 2) messages from named topics to channels where topics are disabled. // So we filter them out here. if ( !stream_data.user_can_move_messages_within_channel(current_stream) && - !stream_data.user_can_move_messages_within_channel(stream) && - topic_name === "" && - !stream_data.can_use_empty_topic(stream.stream_id) + !stream_data.user_can_move_messages_within_channel(stream) ) { - return false; + if ( + topic_name !== "" && + stream_data.is_empty_topic_only_channel(stream.stream_id) + ) { + return false; + } + if (topic_name === "" && !stream_data.can_use_empty_topic(stream.stream_id)) { + return false; + } } return stream_data.can_post_messages_in_stream(stream); }); @@ -1152,6 +1187,7 @@ export async function build_move_topic_to_stream_popover( update_move_messages_count_text(selected_option, message?.id); }); } + disable_topic_input_if_topics_are_disabled_in_channel(current_stream_id); } function focus_on_move_modal_render(): void { diff --git a/web/src/stream_topic_history.ts b/web/src/stream_topic_history.ts index 3c2c9a4ad6..efcd8562dd 100644 --- a/web/src/stream_topic_history.ts +++ b/web/src/stream_topic_history.ts @@ -33,6 +33,23 @@ export function stream_has_topics(stream_id: number): boolean { return history.has_topics(); } +export function stream_has_locally_available_named_topics(stream_id: number): boolean { + if (!stream_dict.has(stream_id)) { + return false; + } + + const history = stream_dict.get(stream_id); + assert(history !== undefined); + + const topic_names = history.topics.keys(); + for (const topic_name of topic_names) { + if (topic_name !== "") { + return true; + } + } + return false; +} + export function stream_has_locally_available_resolved_topics(stream_id: number): boolean { if (!stream_dict.has(stream_id)) { return false; diff --git a/web/src/stream_types.ts b/web/src/stream_types.ts index 6ef555cb5d..c4f87b105b 100644 --- a/web/src/stream_types.ts +++ b/web/src/stream_types.ts @@ -25,6 +25,7 @@ export type StreamPermissionGroupSetting = z.infer; diff --git a/web/src/tippyjs.ts b/web/src/tippyjs.ts index 55abfce818..f0c00b7fbf 100644 --- a/web/src/tippyjs.ts +++ b/web/src/tippyjs.ts @@ -7,6 +7,7 @@ import render_change_visibility_policy_button_tooltip from "../templates/change_ import render_information_density_update_button_tooltip from "../templates/information_density_update_button_tooltip.hbs"; import render_org_logo_tooltip from "../templates/org_logo_tooltip.hbs"; import render_tooltip_templates from "../templates/tooltip_templates.hbs"; +import render_topics_not_allowed_error from "../templates/topics_not_allowed_error.hbs"; import * as compose_validate from "./compose_validate.ts"; import {$t} from "./i18n.ts"; @@ -877,4 +878,28 @@ export function initialize(): void { instance.destroy(); }, }); + + tippy.delegate("body", { + target: "#compose_recipient_box, #move-topic-new-topic-input-wrapper", + delay: LONG_HOVER_DELAY, + onShow(instance) { + const $elem = $(instance.reference); + if ($($elem).find(".empty-topic-only").prop("disabled")) { + const error_message = render_topics_not_allowed_error({ + empty_string_topic_display_name: util.get_final_topic_display_name(""), + }); + instance.setContent(ui_util.parse_html(error_message)); + // `display: flex` doesn't show the tooltip content inline when general chat + // is in the error message. + $(instance.popper).find(".tippy-content").css("display", "block"); + return undefined; + } + instance.destroy(); + return false; + }, + appendTo: () => document.body, + onHidden(instance) { + instance.destroy(); + }, + }); } diff --git a/web/styles/compose.css b/web/styles/compose.css index 7a862bbe05..7a96512029 100644 --- a/web/styles/compose.css +++ b/web/styles/compose.css @@ -1107,6 +1107,11 @@ textarea.new_message_textarea { #recipient_box_clear_topic_button { background: none; border: none; + + &:disabled { + cursor: default; + opacity: 1; + } } /* Styles for input in the recipient_box */ @@ -1180,10 +1185,20 @@ textarea.new_message_textarea { ~ #recipient_box_clear_topic_button { visibility: hidden; } + + #stream_message_recipient_topic:disabled + ~ #recipient_box_clear_topic_button { + visibility: hidden; + } /* This will reset the bootstrap margin-bottom: 10px value for the inputs */ & input { margin-bottom: 0; } + + &.disabled { + background-color: transparent; + border-color: transparent; + } } #compose_select_recipient_widget { @@ -1293,7 +1308,7 @@ textarea.new_message_textarea { outline: 1px solid var(--color-outline-low-attention-input-pill); } - &:hover { + &:hover:not(.disabled) { background-color: var( --color-compose-recipient-box-background-color ); diff --git a/web/styles/popovers.css b/web/styles/popovers.css index 07567322c9..b8b33e7325 100644 --- a/web/styles/popovers.css +++ b/web/styles/popovers.css @@ -907,6 +907,10 @@ ul.popover-group-menu-member-list { &.empty-topic-display::placeholder { color: inherit; } + + &.empty-topic-only { + cursor: default; + } } } diff --git a/web/styles/subscriptions.css b/web/styles/subscriptions.css index 5e8381d1a6..df44f8d863 100644 --- a/web/styles/subscriptions.css +++ b/web/styles/subscriptions.css @@ -109,7 +109,8 @@ } .user_group_creation_error, -.stream_creation_error { +.stream_creation_error, +#settings-topics-already-exist-error { display: none; margin-left: 2px; color: hsl(0deg 100% 50%); diff --git a/web/templates/stream_settings/stream_permissions.hbs b/web/templates/stream_settings/stream_permissions.hbs index 803b6abe68..4da5f1eaa1 100644 --- a/web/templates/stream_settings/stream_permissions.hbs +++ b/web/templates/stream_settings/stream_permissions.hbs @@ -129,6 +129,7 @@ + {{> topics_already_exist_error .}} diff --git a/web/templates/stream_settings/topics_already_exist_error.hbs b/web/templates/stream_settings/topics_already_exist_error.hbs new file mode 100644 index 0000000000..722bc5c711 --- /dev/null +++ b/web/templates/stream_settings/topics_already_exist_error.hbs @@ -0,0 +1,7 @@ +
+ {{#tr}} + To enable this configuration, all messages in this channel must be in the topic. Consider renaming other topics to . + {{#*inline "z-link-rename"}}{{> @partial-block}}{{/inline}} + {{#*inline "z-empty-string-topic-display-name"}}{{empty_string_topic_display_name}}{{/inline}} + {{/tr}} +
diff --git a/web/templates/topics_not_allowed_error.hbs b/web/templates/topics_not_allowed_error.hbs new file mode 100644 index 0000000000..0ee215222c --- /dev/null +++ b/web/templates/topics_not_allowed_error.hbs @@ -0,0 +1,4 @@ +{{#tr}} + Only the topic is allowed in this channel. + {{#*inline "z-empty-string-topic-display-name"}}{{empty_string_topic_display_name}}{{/inline}} +{{/tr}} diff --git a/web/tests/message_edit.test.cjs b/web/tests/message_edit.test.cjs index 64a77b1b5e..83510d60ee 100644 --- a/web/tests/message_edit.test.cjs +++ b/web/tests/message_edit.test.cjs @@ -98,6 +98,7 @@ run_test("is_topic_editable", ({override}) => { override(stream_data, "is_stream_archived", () => false); override(stream_data, "user_can_move_messages_within_channel", () => true); override(stream_data, "get_sub_by_id", () => ({})); + override(stream_data, "is_empty_topic_only_channel", () => false); override(current_user, "is_moderator", true); assert.equal(message_edit.is_topic_editable(message), false); diff --git a/web/tests/popover_menus_data.test.cjs b/web/tests/popover_menus_data.test.cjs index 71ce81ad40..9317101a37 100644 --- a/web/tests/popover_menus_data.test.cjs +++ b/web/tests/popover_menus_data.test.cjs @@ -56,6 +56,7 @@ mock_esm("../src/stream_data", { is_stream_archived: () => false, get_sub_by_id: () => noop, user_can_move_messages_within_channel: () => true, + is_empty_topic_only_channel: () => false, }); mock_esm("../src/group_permission_settings", { get_group_permission_setting_config() { diff --git a/web/tests/settings_org.test.cjs b/web/tests/settings_org.test.cjs index 4d9225f383..cc708bc83b 100644 --- a/web/tests/settings_org.test.cjs +++ b/web/tests/settings_org.test.cjs @@ -181,6 +181,7 @@ function test_change_save_button_state() { props, } = createSaveButtons("msg-editing"); $save_button_header.attr("id", "org-msg-editing"); + $("#org-msg-editing").closest = () => ({}); { settings_components.change_save_button_state($save_button_controls, "unsaved"); diff --git a/web/tests/stream_data.test.cjs b/web/tests/stream_data.test.cjs index 8377b3a277..41b7b78faf 100644 --- a/web/tests/stream_data.test.cjs +++ b/web/tests/stream_data.test.cjs @@ -2209,3 +2209,30 @@ run_test("can_archive_stream", ({override}) => { social.can_administer_channel_group = me_group.id; assert.equal(stream_data.can_archive_stream(social), true); }); + +run_test("is_empty_topic_only_channel", ({override}) => { + const social = { + subscribed: true, + color: "red", + name: "social", + stream_id: 2, + topics_policy: "empty_topic_only", + }; + stream_data.add_sub(social); + const scotland = { + subscribed: true, + color: "red", + name: "scotland", + stream_id: 3, + topics_policy: "inherit", + }; + override(realm, "realm_topics_policy", "allow_empty_topic"); + assert.equal(stream_data.is_empty_topic_only_channel(undefined), false); + + stream_data.add_sub(scotland); + override(current_user, "user_id", me.user_id); + + override(current_user, "is_admin", true); + assert.equal(stream_data.is_empty_topic_only_channel(social.stream_id), true); + assert.equal(stream_data.is_empty_topic_only_channel(scotland.stream_id), false); +}); diff --git a/web/tests/stream_topic_history.test.cjs b/web/tests/stream_topic_history.test.cjs index e4e2ab24d2..4a05ec0245 100644 --- a/web/tests/stream_topic_history.test.cjs +++ b/web/tests/stream_topic_history.test.cjs @@ -263,6 +263,7 @@ test("test_stream_has_topics", () => { const stream_id = 88; assert.equal(stream_topic_history.stream_has_topics(stream_id), false); + assert.equal(stream_topic_history.stream_has_locally_available_named_topics(stream_id), false); stream_topic_history.find_or_create(stream_id); @@ -270,6 +271,15 @@ test("test_stream_has_topics", () => { // mean we have actual topics. assert.equal(stream_topic_history.stream_has_topics(stream_id), false); + stream_topic_history.add_message({ + stream_id, + message_id: 888, + topic_name: "", + }); + + assert.equal(stream_topic_history.stream_has_topics(stream_id), true); + assert.equal(stream_topic_history.stream_has_locally_available_named_topics(stream_id), false); + stream_topic_history.add_message({ stream_id, message_id: 888, @@ -277,6 +287,7 @@ test("test_stream_has_topics", () => { }); assert.equal(stream_topic_history.stream_has_topics(stream_id), true); + assert.equal(stream_topic_history.stream_has_locally_available_named_topics(stream_id), true); }); test("test_stream_has_resolved_topics", () => { diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index abe384a69d..18dd9903b7 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -32,6 +32,7 @@ from zerver.lib.exceptions import ( MessagesNotAllowedInEmptyTopicError, PreviousMessageContentMismatchedError, StreamWildcardMentionNotAllowedError, + TopicsNotAllowedError, TopicWildcardMentionNotAllowedError, ) from zerver.lib.markdown import MessageRenderingResult, topic_links @@ -1482,15 +1483,21 @@ def build_message_edit_request( target_stream = access_stream_by_id_for_message(user_profile, stream_id)[0] is_stream_edited = True + topics_policy = get_stream_topics_policy(message.realm, target_stream) + empty_topic_display_name = get_topic_display_name("", user_profile.default_language) + target_topic_empty = ("(no topic)", "") if ( - target_topic_name in ("(no topic)", "") + target_topic_name in target_topic_empty and (is_topic_edited or is_stream_edited) - and get_stream_topics_policy(message.realm, target_stream) - == StreamTopicsPolicyEnum.disable_empty_topic.value + and topics_policy == StreamTopicsPolicyEnum.disable_empty_topic.value ): - raise MessagesNotAllowedInEmptyTopicError( - get_topic_display_name("", user_profile.default_language) - ) + raise MessagesNotAllowedInEmptyTopicError(empty_topic_display_name) + + if ( + topics_policy == StreamTopicsPolicyEnum.empty_topic_only.value + and target_topic_name not in target_topic_empty + ): + raise TopicsNotAllowedError(empty_topic_display_name) return StreamMessageEditRequest( is_content_edited=is_content_edited, diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index 2a5ebf2b40..c49b94ae7b 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -35,6 +35,7 @@ from zerver.lib.exceptions import ( StreamDoesNotExistError, StreamWildcardMentionNotAllowedError, StreamWithIDDoesNotExistError, + TopicsNotAllowedError, TopicWildcardMentionNotAllowedError, ZephyrMessageAlreadySentError, ) @@ -1785,14 +1786,13 @@ def check_message( # else can sneak past the access check. assert sender.bot_type == sender.OUTGOING_WEBHOOK_BOT - if ( - get_stream_topics_policy(realm, stream) - == StreamTopicsPolicyEnum.disable_empty_topic.value - and topic_name == "" - ): - raise MessagesNotAllowedInEmptyTopicError( - get_topic_display_name("", sender.default_language) - ) + topics_policy = get_stream_topics_policy(realm, stream) + empty_topic_display_name = get_topic_display_name("", sender.default_language) + if topics_policy == StreamTopicsPolicyEnum.disable_empty_topic.value and topic_name == "": + raise MessagesNotAllowedInEmptyTopicError(empty_topic_display_name) + + if topics_policy == StreamTopicsPolicyEnum.empty_topic_only.value and topic_name != "": + raise TopicsNotAllowedError(empty_topic_display_name) elif addressee.is_private(): user_profiles = addressee.user_profiles() diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 73a9876236..5b0a4b04cb 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -1757,6 +1757,9 @@ def do_set_stream_property(stream: Stream, name: str, value: Any, acting_user: U StreamTopicsPolicyEnum.disable_empty_topic.value: _( "No *{empty_topic_display_name}* topic" ).format(empty_topic_display_name=empty_topic_display_name), + StreamTopicsPolicyEnum.empty_topic_only.value: _( + "Only *{empty_topic_display_name}* topic allowed" + ).format(empty_topic_display_name=empty_topic_display_name), } NOTIFICATION_MESSAGES = { diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 8fb28f85f4..41bfb3662b 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -549,6 +549,32 @@ class MessagesNotAllowedInEmptyTopicError(JsonableError): ) +class TopicsNotAllowedError(JsonableError): + data_fields = ["empty_topic_display_name"] + + def __init__(self, empty_topic_display_name: str) -> None: + self.empty_topic_display_name = empty_topic_display_name + + @staticmethod + @override + def msg_format() -> str: + return _("Only the {empty_topic_display_name} topic is allowed in this channel.") + + +class CannotSetTopicsPolicyError(JsonableError): + data_fields = ["empty_topic_display_name"] + + def __init__(self, empty_topic_display_name: str) -> None: + self.empty_topic_display_name = empty_topic_display_name + + @staticmethod + @override + def msg_format() -> str: + return _( + "To enable this configuration, all messages in this channel must be in the {empty_topic_display_name} topic. Consider renaming or deleting other topics." + ) + + class DirectMessagePermissionError(JsonableError): def __init__(self, is_nobody_group: bool) -> None: if is_nobody_group: diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 82df3cf4a6..c92046b95d 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -264,9 +264,19 @@ def get_user_ids_with_metadata_access_via_permission_groups(stream: Stream) -> s def channel_events_topic_name(stream: Stream) -> str: + if stream.topics_policy == StreamTopicsPolicyEnum.empty_topic_only.value: + return "" return str(Realm.STREAM_EVENTS_NOTIFICATION_TOPIC_NAME) +def channel_has_named_topics(stream: Stream) -> bool: + return ( + Message.objects.filter(realm_id=stream.realm_id, recipient=stream.recipient) + .exclude(subject="") + .exists() + ) + + @transaction.atomic(savepoint=False) def create_stream_if_needed( realm: Realm, diff --git a/zerver/models/streams.py b/zerver/models/streams.py index 843cadd5fc..5e28744ac7 100644 --- a/zerver/models/streams.py +++ b/zerver/models/streams.py @@ -27,6 +27,7 @@ class StreamTopicsPolicyEnum(Enum): inherit = 1 allow_empty_topic = 2 disable_empty_topic = 3 + empty_topic_only = 4 class Stream(models.Model): diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index e9ac4c49a1..528bb7e0e9 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -27514,14 +27514,23 @@ components: - inherit - allow_empty_topic - disable_empty_topic + - empty_topic_only description: | - Configuration defining the default policy for sending messages in the empty topic. + Setting defining the policy for sending messages to the empty topic in + this channel. - inherit - Channel will inherit the organization-level setting, `realm_topics_policy`. - allow_empty_topic - Topics are not required to send messages in the channel. - disable_empty_topic - Topics are required to send messages in the channel. + - empty_topic_only - Only the empty topic is available in this channel. - **Changes**: New in Zulip 11.0 (feature level 392). + The `empty_topic_only` policy can only be enabled in a channel if all + messages in the channel are already in the empty topic. + + **Changes**: In Zulip 11.0 (feature level 404), `empty_topic_only` option + was added. + + New in Zulip 11.0 (feature level 392). ChannelCanAddSubscribersGroup: allOf: - $ref: "#/components/schemas/GroupSettingValue" diff --git a/zerver/tests/test_message_move_stream.py b/zerver/tests/test_message_move_stream.py index 656e1e017f..d1ab580d21 100644 --- a/zerver/tests/test_message_move_stream.py +++ b/zerver/tests/test_message_move_stream.py @@ -54,13 +54,16 @@ class MessageMoveStreamTest(ZulipTestCase): self, user: str, orig_stream: Stream, + orig_topic_name: str = "test", stream_id: int | None = None, topic_name: str | None = None, expected_error: str | None = None, ) -> None: user_profile = self.example_user(user) self.subscribe(user_profile, orig_stream.name) - message_id = self.send_stream_message(user_profile, orig_stream.name) + message_id = self.send_stream_message( + user_profile, orig_stream.name, topic_name=orig_topic_name + ) params_dict: dict[str, str | int] = {} if stream_id is not None: @@ -1331,6 +1334,23 @@ class MessageMoveStreamTest(ZulipTestCase): # is set to `allow_empty_topic`. self.assert_move_message("desdemona", stream_2, stream_id=stream_1.id, topic_name="") + do_set_stream_property( + stream_2, + "topics_policy", + StreamTopicsPolicyEnum.empty_topic_only.value, + acting_user=desdemona, + ) + + # Cannot move messages to topics other than empty topic in the channels with + # `topics_policy` set to `empty_topic_only`. + self.assert_move_message( + "desdemona", + stream_1, + stream_id=stream_2.id, + expected_error="Only the general chat topic is allowed in this channel.", + ) + self.assert_move_message("desdemona", stream_1, stream_id=stream_2.id, topic_name="") + def test_move_message_to_stream_with_topic_editing_not_allowed(self) -> None: (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( "othello", "old_stream_1", "new_stream_1", "test" diff --git a/zerver/tests/test_message_move_topic.py b/zerver/tests/test_message_move_topic.py index ab325db821..74eecc16d7 100644 --- a/zerver/tests/test_message_move_topic.py +++ b/zerver/tests/test_message_move_topic.py @@ -51,13 +51,16 @@ class MessageMoveTopicTest(ZulipTestCase): self, user: str, orig_stream: Stream, + orig_topic_name: str = "test", stream_id: int | None = None, topic_name: str | None = None, expected_error: str | None = None, ) -> None: user_profile = self.example_user(user) self.subscribe(user_profile, orig_stream.name) - message_id = self.send_stream_message(user_profile, orig_stream.name) + message_id = self.send_stream_message( + user_profile, orig_stream.name, topic_name=orig_topic_name + ) params_dict: dict[str, str | int] = {} if stream_id is not None: @@ -2654,3 +2657,20 @@ class MessageMoveTopicTest(ZulipTestCase): expected_error="Sending messages to the general chat is not allowed in this channel.", ) self.assert_move_message("desdemona", stream_1, topic_name="new topic") + + do_set_stream_property( + stream_1, + "topics_policy", + StreamTopicsPolicyEnum.empty_topic_only.value, + acting_user=desdemona, + ) + + # Cannot move messages to topics other than empty topic in the channels with + # `topics_policy` set to `empty_topic_only`. + self.assert_move_message( + "desdemona", + stream_1, + orig_topic_name="", + topic_name="new topic", + expected_error="Only the general chat topic is allowed in this channel.", + ) diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index 9790f50a5f..82dfea5488 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -45,6 +45,7 @@ from zerver.lib.exceptions import ( DirectMessagePermissionError, JsonableError, MessagesNotAllowedInEmptyTopicError, + TopicsNotAllowedError, ) from zerver.lib.message import get_raw_unread_data, get_recent_private_conversations from zerver.lib.message_cache import MessageDict @@ -3684,3 +3685,36 @@ class CheckMessageTest(ZulipTestCase): "Sending messages to the general chat is not allowed in this channel.", ): check_message(sender, client, addressee, message_content, realm) + + def test_message_send_in_channel_with_topics_disabled(self) -> None: + realm = get_realm("zulip") + sender = self.example_user("iago") + client = make_client(name="test suite") + stream = get_stream("Denmark", realm) + empty_topic = "" + named_topic = "test topic" + message_content = "whatever" + addressee_named_topic = Addressee.for_stream(stream, named_topic) + addressee_empty_topic = Addressee.for_stream(stream, empty_topic) + self.login_user(sender) + + realm.refresh_from_db() + ret = check_message(sender, client, addressee_named_topic, message_content, realm) + self.assertEqual(ret.message.topic_name(), named_topic) + + ret = check_message(sender, client, addressee_empty_topic, message_content, realm) + self.assertEqual(ret.message.topic_name(), empty_topic) + + do_set_stream_property( + stream, "topics_policy", StreamTopicsPolicyEnum.empty_topic_only.value, sender + ) + + # Can only send messages to empty topics when `topics_policy` is set to `empty_topic_only`. + ret = check_message(sender, client, addressee_empty_topic, message_content, realm) + self.assertEqual(ret.message.topic_name(), empty_topic) + + with self.assertRaisesRegex( + TopicsNotAllowedError, + "Only the general chat topic is allowed in this channel.", + ): + check_message(sender, client, addressee_named_topic, message_content, realm) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index cc632fca66..86c5db5de6 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -18,6 +18,7 @@ from zerver.actions.bots import do_change_bot_owner from zerver.actions.channel_folders import check_add_channel_folder from zerver.actions.create_realm import do_create_realm from zerver.actions.default_streams import do_add_default_stream, do_create_default_stream_group +from zerver.actions.message_delete import do_delete_messages from zerver.actions.realm_settings import ( do_change_realm_permission_group_setting, do_change_realm_plan_type, @@ -73,6 +74,7 @@ from zerver.models import ( Attachment, DefaultStream, DefaultStreamGroup, + Message, NamedUserGroup, Realm, RealmAuditLog, @@ -2296,6 +2298,44 @@ class StreamAdminTest(ZulipTestCase): ) self.assert_json_error(result, "Insufficient permission") + desdemona = self.example_user("desdemona") + self.login("desdemona") + + new_stream_name = "TestStream" + new_stream = self.make_stream(new_stream_name, hamlet.realm) + self.subscribe(desdemona, new_stream_name) + self.send_stream_message(desdemona, new_stream_name, "test content", "") + + result = self.client_patch( + f"/json/streams/{new_stream.id}", + {"topics_policy": StreamTopicsPolicyEnum.allow_empty_topic.name}, + ) + self.assert_json_success(result) + + # Cannot set `topics_policy` to `empty_topic_only` when there are messages + # in non-empty topics in the current channel. + result = self.client_patch( + f"/json/streams/{new_stream.id}", + {"topics_policy": StreamTopicsPolicyEnum.empty_topic_only.name}, + ) + self.assert_json_error( + result, + "To enable this configuration, all messages in this channel must be in the general chat topic. Consider renaming or deleting other topics.", + ) + + topic_messages = Message.objects.filter( + realm=hamlet.realm, + recipient=new_stream.recipient, + ) + do_delete_messages(hamlet.realm, list(topic_messages), acting_user=desdemona) + self.send_stream_message(desdemona, new_stream_name, "test content", "") + + result = self.client_patch( + f"/json/streams/{new_stream.id}", + {"topics_policy": StreamTopicsPolicyEnum.empty_topic_only.name}, + ) + self.assert_json_success(result) + def test_can_set_topics_policy_group(self) -> None: user = self.example_user("hamlet") realm = user.realm diff --git a/zerver/views/streams.py b/zerver/views/streams.py index c934daf6d6..864577de47 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -57,6 +57,7 @@ from zerver.lib.default_streams import get_default_stream_ids_for_realm from zerver.lib.email_mirror_helpers import encode_email_address, get_channel_email_token from zerver.lib.exceptions import ( CannotManageDefaultChannelError, + CannotSetTopicsPolicyError, JsonableError, OrganizationOwnerRequiredError, ) @@ -74,6 +75,7 @@ from zerver.lib.streams import ( access_stream_for_delete_or_update_requiring_metadata_access, access_web_public_stream, channel_events_topic_name, + channel_has_named_topics, check_stream_name_available, do_get_streams, filter_stream_authorization_for_adding_subscribers, @@ -86,6 +88,7 @@ from zerver.lib.streams import ( ) from zerver.lib.subscription_info import gather_subscriptions from zerver.lib.topic import ( + get_topic_display_name, get_topic_history_for_public_stream, get_topic_history_for_stream, maybe_rename_general_chat_to_empty_topic, @@ -387,6 +390,21 @@ def update_stream_backend( if not user_profile.can_create_web_public_streams(): raise JsonableError(_("Insufficient permission")) + if topics_policy is not None and isinstance(topics_policy, StreamTopicsPolicyEnum): + if not user_profile.can_set_topics_policy(): + raise JsonableError(_("Insufficient permission")) + + # Cannot set `topics_policy` to `empty_topic_only` when there are messages + # in non-empty topics in the current channel. + if topics_policy == StreamTopicsPolicyEnum.empty_topic_only and channel_has_named_topics( + stream + ): + raise CannotSetTopicsPolicyError( + get_topic_display_name("", user_profile.default_language) + ) + + do_set_stream_property(stream, "topics_policy", topics_policy.value, user_profile) + if ( is_private is not None or is_web_public is not None @@ -422,12 +440,6 @@ def update_stream_backend( if is_archived is not None and not is_archived: do_unarchive_stream(stream, stream.name, acting_user=None) - if topics_policy is not None and isinstance(topics_policy, StreamTopicsPolicyEnum): - if not user_profile.can_set_topics_policy(): - raise JsonableError(_("Insufficient permission")) - - do_set_stream_property(stream, "topics_policy", topics_policy.value, user_profile) - if description is not None: if "\n" in description: # We don't allow newline characters in stream descriptions.