From 440f9e397a235c4d4db344897f98d8f17638322b Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 16 Mar 2023 18:02:30 +0530 Subject: [PATCH] message_edit: Apply topic edit restrictions to "(no topic)" messages. Previously, editing topic of "(no topic)" messages was allowed irrespective of time limit or the "edit_topic_policy" setting. Since we are working in the direction of having "no topic" messages feel reasonable, this commit changes the code to not consider them as a special case and topic editing restrictions apply to them as well now like all other messages. We still highlight the topic edit icon in recipient bar without hovering for "no topic" messages, but it is only shown when user has permission to edit topics. --- api_docs/changelog.md | 5 +++++ web/src/message_edit.js | 4 ---- web/src/message_list_view.js | 11 +++++++++-- web/tests/message_edit.test.js | 2 +- zerver/actions/message_edit.py | 21 +-------------------- zerver/openapi/zulip.yaml | 5 +++++ zerver/tests/test_message_edit.py | 12 +++++++----- 7 files changed, 28 insertions(+), 32 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index d099b5c3b3..c2a8d8ef69 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,11 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 7.0 +**Feature level 172** + +* [`PATCH /messages/{message_id}`](/api/update-message): Topic editing + restrictions now apply to messages without a topic as well. + **Feature level 171**: * [`POST /fetch_api_key`](/api/fetch-api-key), diff --git a/web/src/message_edit.js b/web/src/message_edit.js index a5a2f49c6c..626a75515a 100644 --- a/web/src/message_edit.js +++ b/web/src/message_edit.js @@ -54,10 +54,6 @@ export function is_topic_editable(message, edit_limit_seconds_buffer = 0) { return false; } - if (message.topic === compose.empty_topic_placeholder()) { - return true; - } - if (!settings_data.user_can_move_messages_to_another_topic()) { return false; } diff --git a/web/src/message_list_view.js b/web/src/message_list_view.js index a5b592c752..976835323b 100644 --- a/web/src/message_list_view.js +++ b/web/src/message_list_view.js @@ -156,14 +156,21 @@ function set_timestr(message_container) { function set_topic_edit_properties(group, message) { group.always_visible_topic_edit = false; group.on_hover_topic_edit = false; + + const is_topic_editable = message_edit.is_topic_editable(message); + // if a user who can edit a topic, can resolve it as well - group.user_can_resolve_topic = message_edit.is_topic_editable(message); + group.user_can_resolve_topic = is_topic_editable; + + if (!is_topic_editable) { + return; + } // Messages with no topics should always have an edit icon visible // to encourage updating them. Admins can also edit any topic. if (message.topic === compose.empty_topic_placeholder()) { group.always_visible_topic_edit = true; - } else if (message_edit.is_topic_editable(message)) { + } else { group.on_hover_topic_edit = true; } } diff --git a/web/tests/message_edit.test.js b/web/tests/message_edit.test.js index 1fcd599cf9..ea94a7aa4f 100644 --- a/web/tests/message_edit.test.js +++ b/web/tests/message_edit.test.js @@ -112,7 +112,7 @@ run_test("is_topic_editable", ({override}) => { assert.equal(message_edit.is_topic_editable(message), false); message.topic = "translated: (no topic)"; - assert.equal(message_edit.is_topic_editable(message), true); + assert.equal(message_edit.is_topic_editable(message), false); message.topic = "test topic"; override(settings_data, "user_can_move_messages_to_another_topic", () => false); diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index a91012979b..1f6935a603 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -106,22 +106,6 @@ def validate_message_edit_payload( raise JsonableError(_("Widgets cannot be edited.")) -def can_edit_topic( - user_profile: UserProfile, - is_no_topic_msg: bool, -) -> bool: - # We allow anyone to edit (no topic) messages to help tend them. - if is_no_topic_msg: - return True - - # The can_move_messages_to_another_topic helper returns whether the user can edit - # the topic or not based on edit_topic_policy setting and the user's role. - if user_profile.can_move_messages_to_another_topic(): - return True - - return False - - def maybe_send_resolve_topic_notifications( *, user_profile: UserProfile, @@ -995,9 +979,7 @@ def check_update_message( ): raise JsonableError(_("You don't have permission to edit this message")) - is_no_topic_msg = message.topic_name() == "(no topic)" - - if topic_name is not None and not can_edit_topic(user_profile, is_no_topic_msg): + if topic_name is not None and not user_profile.can_move_messages_to_another_topic(): raise JsonableError(_("You don't have permission to edit this message")) # If there is a change to the content, check that it hasn't been too long @@ -1019,7 +1001,6 @@ def check_update_message( and user_profile.realm.move_messages_within_stream_limit_seconds is not None and not user_profile.is_realm_admin and not user_profile.is_moderator - and not is_no_topic_msg ): deadline_seconds = ( user_profile.realm.move_messages_within_stream_limit_seconds + edit_limit_buffer diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index fc2c17a7f0..b2f407b4ae 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -6520,6 +6520,11 @@ paths: Before Zulip 7.0 (feature level 159), message senders were allowed to edit the topic indefinitely. + **Changes**: Before Zulip 7.0 (feature level 172), anyone could add + a topic to messages without topic regardless of topic edit + permissions. Now topic editing restrictions apply to messages without a + topic as well. + [config-message-editing]: /help/restrict-message-editing-and-deletion x-curl-examples-parameters: oneOf: diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index b968c45c80..4b97fd21df 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -1195,6 +1195,13 @@ class EditMessageTest(EditMessageTestCase): id_, "G", "The time limit for editing this message's topic has passed", "hamlet" ) + # topic edit permissions apply on "no topic" messages as well + message.set_topic_name("(no topic)") + message.save() + do_edit_message_assert_error( + id_, "G", "The time limit for editing this message's topic has passed", "cordelia" + ) + # set the topic edit limit to two weeks do_set_realm_property( hamlet.realm, @@ -1205,11 +1212,6 @@ class EditMessageTest(EditMessageTestCase): do_edit_message_assert_success(id_, "G", "cordelia") do_edit_message_assert_success(id_, "H", "hamlet") - # anyone should be able to edit "no topic" indefinitely - message.set_topic_name("(no topic)") - message.save() - do_edit_message_assert_success(id_, "D", "cordelia") - @mock.patch("zerver.actions.message_edit.send_event") def test_edit_topic_public_history_stream(self, mock_send_event: mock.MagicMock) -> None: stream_name = "Macbeth"