diff --git a/frontend_tests/node_tests/message_edit.js b/frontend_tests/node_tests/message_edit.js index 95b1941f67..cc0e37d9a8 100644 --- a/frontend_tests/node_tests/message_edit.js +++ b/frontend_tests/node_tests/message_edit.js @@ -2,7 +2,7 @@ const {strict: assert} = require("assert"); -const {zrequire} = require("../zjsunit/namespace"); +const {mock_esm, zrequire} = require("../zjsunit/namespace"); const {run_test} = require("../zjsunit/test"); const {page_params} = require("../zjsunit/zpage_params"); @@ -14,7 +14,10 @@ const settings_config = zrequire("settings_config"); const get_editability = message_edit.get_editability; const editability_types = message_edit.editability_types; -run_test("get_editability", () => { +const settings_data = mock_esm("../../static/js/settings_data"); + +run_test("get_editability", (override) => { + override(settings_data, "user_can_edit_topic_of_any_message", () => true); // You can't edit a null message assert.equal(get_editability(null), editability_types.NO); // You can't edit a message you didn't send @@ -95,21 +98,26 @@ run_test("get_editability", () => { assert.equal(message_edit.is_topic_editable(message), true); message.sent_by_me = true; - page_params.realm_edit_topic_policy = - settings_config.common_message_policy_values.by_admins_only.code; + override(settings_data, "user_can_edit_topic_of_any_message", () => false); assert.equal(message_edit.is_topic_editable(message), true); message.sent_by_me = false; - page_params.realm_edit_topic_policy = - settings_config.common_message_policy_values.by_admins_only.code; assert.equal(message_edit.is_topic_editable(message), false); message.sent_by_me = false; - page_params.realm_edit_topic_policy = - settings_config.common_message_policy_values.by_admins_only.code; page_params.is_admin = true; assert.equal(message_edit.is_topic_editable(message), true); + page_params.is_admin = false; + override(settings_data, "user_can_edit_topic_of_any_message", () => true); + assert.equal(message_edit.is_topic_editable(message), true); + + message.timestamp = current_timestamp - 600000; + assert.equal(message_edit.is_topic_editable(message), false); + + page_params.is_moderator = true; + assert.equal(message_edit.is_topic_editable(message), true); + page_params.realm_allow_message_editing = false; assert.equal(message_edit.is_topic_editable(message), false); }); diff --git a/frontend_tests/node_tests/settings_data.js b/frontend_tests/node_tests/settings_data.js index 3c8253ba8d..de8f5c86a7 100644 --- a/frontend_tests/node_tests/settings_data.js +++ b/frontend_tests/node_tests/settings_data.js @@ -173,3 +173,46 @@ test_policy( "realm_move_messages_between_streams_policy", settings_data.user_can_move_messages_between_streams, ); + +run_test("user_can_edit_topic_of_any_message", () => { + const can_edit_topic_of_any_message = settings_data.user_can_edit_topic_of_any_message; + + page_params.is_admin = true; + page_params.realm_edit_topic_policy = + settings_config.common_message_policy_values.by_admins_only.code; + assert.equal(can_edit_topic_of_any_message(), true); + + page_params.is_admin = false; + page_params.is_moderator = true; + assert.equal(can_edit_topic_of_any_message(), false); + + page_params.realm_edit_topic_policy = + settings_config.common_message_policy_values.by_moderators_only.code; + assert.equal(can_edit_topic_of_any_message(), true); + + page_params.is_moderator = false; + assert.equal(can_edit_topic_of_any_message(), false); + + page_params.is_guest = true; + page_params.realm_edit_topic_policy = + settings_config.common_message_policy_values.by_everyone.code; + assert.equal(can_edit_topic_of_any_message(), true); + + page_params.realm_edit_topic_policy = + settings_config.common_message_policy_values.by_members.code; + assert.equal(can_edit_topic_of_any_message(), false); + + page_params.is_guest = false; + assert.equal(can_edit_topic_of_any_message(), true); + + page_params.realm_edit_topic_policy = + settings_config.common_message_policy_values.by_full_members.code; + page_params.user_id = 30; + people.add_active_user(isaac); + isaac.date_joined = new Date(Date.now()); + page_params.realm_waiting_period_threshold = 10; + assert.equal(can_edit_topic_of_any_message(), false); + + isaac.date_joined = new Date(Date.now() - 20 * 86400000); + assert.equal(can_edit_topic_of_any_message(), true); +}); diff --git a/static/js/message_edit.js b/static/js/message_edit.js index 4e9e094405..3bc019df6f 100644 --- a/static/js/message_edit.js +++ b/static/js/message_edit.js @@ -24,7 +24,6 @@ import * as overlays from "./overlays"; import {page_params} from "./page_params"; import * as resize from "./resize"; import * as rows from "./rows"; -import * as settings_config from "./settings_config"; import * as settings_data from "./settings_data"; import * as stream_bar from "./stream_bar"; import * as stream_data from "./stream_data"; @@ -69,14 +68,16 @@ export function is_topic_editable(message, edit_limit_seconds_buffer = 0) { return true; } - if ( - page_params.realm_edit_topic_policy === - settings_config.common_message_policy_values.by_admins_only.code - ) { - // If you're another non-admin user, you need community topic editing enabled. + if (!settings_data.user_can_edit_topic_of_any_message()) { return false; } + // moderators can edit the topic if edit_topic_policy allows them to do so, + // irrespective of the topic editing deadline. + if (page_params.is_moderator) { + return true; + } + // If you're using community topic editing, there's a deadline. return ( page_params.realm_community_topic_editing_limit_seconds + diff --git a/static/js/settings_config.js b/static/js/settings_config.js index 6cd4f62b26..c7cb591c7d 100644 --- a/static/js/settings_config.js +++ b/static/js/settings_config.js @@ -204,8 +204,23 @@ export const common_message_policy_values = { code: 5, description: $t({defaultMessage: "Admins, members and guests"}), }, - by_admins_only: { + by_members: { order: 2, + code: 1, + description: $t({defaultMessage: "Admins and members"}), + }, + by_full_members: { + order: 3, + code: 3, + description: $t({defaultMessage: "Admins and full members"}), + }, + by_moderators_only: { + order: 4, + code: 4, + description: $t({defaultMessage: "Admins and moderators"}), + }, + by_admins_only: { + order: 5, code: 2, description: $t({defaultMessage: "Admins only"}), }, diff --git a/static/js/settings_data.js b/static/js/settings_data.js index c5aafd56da..440c44709e 100644 --- a/static/js/settings_data.js +++ b/static/js/settings_data.js @@ -144,3 +144,13 @@ export function user_can_create_streams() { export function user_can_move_messages_between_streams() { return user_has_permission(page_params.realm_move_messages_between_streams_policy); } + +export function user_can_edit_topic_of_any_message() { + if ( + page_params.realm_edit_topic_policy === + settings_config.common_message_policy_values.by_everyone.code + ) { + return true; + } + return user_has_permission(page_params.realm_edit_topic_policy); +} diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index f327a850e1..a0023a02da 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -2819,12 +2819,9 @@ def can_edit_content_or_topic( if is_no_topic_msg: return True - # Organization administrators can always edit topics - if user_profile.is_realm_admin: - return True - - # The edit_topic_policy setting controls which users can edit topics. - if user_profile.realm.edit_topic_policy == Realm.POLICY_EVERYONE: + # The can_edit_topic_of_any_message 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_edit_topic_of_any_message(): return True return False @@ -2885,6 +2882,7 @@ def check_update_message( topic_name is not None and message.sender != user_profile and not user_profile.is_realm_admin + and not user_profile.is_moderator and not is_no_topic_msg ): deadline_seconds = Realm.DEFAULT_COMMUNITY_TOPIC_EDITING_LIMIT_SECONDS + edit_limit_buffer diff --git a/zerver/models.py b/zerver/models.py index 7c7591251d..c4c12ead2e 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -264,7 +264,10 @@ class Realm(models.Model): ] COMMON_MESSAGE_POLICY_TYPES = [ + POLICY_MEMBERS_ONLY, POLICY_ADMINS_ONLY, + POLICY_FULL_MEMBERS_ONLY, + POLICY_MODERATORS_ONLY, POLICY_EVERYONE, ] @@ -1571,6 +1574,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): def has_permission(self, policy_name: str) -> bool: if policy_name not in [ "create_stream_policy", + "edit_topic_policy", "invite_to_stream_policy", "invite_to_realm_policy", "move_messages_between_streams_policy", @@ -1611,6 +1615,11 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): def can_move_messages_between_streams(self) -> bool: return self.has_permission("move_messages_between_streams_policy") + def can_edit_topic_of_any_message(self) -> bool: + if self.realm.edit_topic_policy == Realm.POLICY_EVERYONE: + return True + return self.has_permission("edit_topic_policy") + def can_access_public_streams(self) -> bool: return not (self.is_guest or self.realm.is_zephyr_mirror_realm) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 51ba8a3ab2..beba75f4d4 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3488,7 +3488,10 @@ paths: description: | The policy for which users can edit topics of any message. + * 1 = members only * 2 = admins only + * 3 = full members only + * 4 = moderators only * 5 = everyone **Changes**: New in Zulip 5.0 (feature level 75), replacing the @@ -8745,7 +8748,10 @@ paths: The policy for which users can edit topics of any message. + * 1 = members only * 2 = admins only + * 3 = full members only + * 4 = moderators only * 5 = everyone **Changes**: New in Zulip 5.0 (feature level 75), replacing the diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 6c0124b37c..1da7eddfbe 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -6,6 +6,7 @@ from unittest import mock import orjson from django.db import IntegrityError from django.http import HttpResponse +from django.utils.timezone import now as timezone_now from zerver.lib.actions import ( do_change_stream_post_policy, @@ -854,17 +855,51 @@ class EditMessageTest(EditMessageTestCase): message.date_sent = message.date_sent - datetime.timedelta(seconds=180) message.save() + # Guest user must be subscribed to the stream to access the message. + polonius = self.example_user("polonius") + self.subscribe(polonius, "Scotland") + # any user can edit the topic of a message set_message_editing_params(True, 0, Realm.POLICY_EVERYONE) - do_edit_message_assert_success(id_, "A", "cordelia") + do_edit_message_assert_success(id_, "A", "polonius") - # only admins can edit the topics of messages - set_message_editing_params(True, 0, Realm.POLICY_ADMINS_ONLY) - do_edit_message_assert_success(id_, "B", "iago") + # only members can edit topic of a message + set_message_editing_params(True, 0, Realm.POLICY_MEMBERS_ONLY) + do_edit_message_assert_error( + id_, "B", "You don't have permission to edit this message", "polonius" + ) + do_edit_message_assert_success(id_, "B", "cordelia") + + # only full members can edit topic of a message + set_message_editing_params(True, 0, Realm.POLICY_FULL_MEMBERS_ONLY) + + cordelia = self.example_user("cordelia") + do_set_realm_property(cordelia.realm, "waiting_period_threshold", 10, acting_user=None) + + cordelia.date_joined = timezone_now() - datetime.timedelta(days=9) + cordelia.save() do_edit_message_assert_error( id_, "C", "You don't have permission to edit this message", "cordelia" ) + cordelia.date_joined = timezone_now() - datetime.timedelta(days=11) + cordelia.save() + do_edit_message_assert_success(id_, "C", "cordelia") + + # only moderators can edit topic of a message + set_message_editing_params(True, 0, Realm.POLICY_MODERATORS_ONLY) + do_edit_message_assert_error( + id_, "D", "You don't have permission to edit this message", "cordelia" + ) + do_edit_message_assert_success(id_, "D", "shiva") + + # only admins can edit the topics of messages + set_message_editing_params(True, 0, Realm.POLICY_ADMINS_ONLY) + do_edit_message_assert_error( + id_, "E", "You don't have permission to edit this message", "shiva" + ) + do_edit_message_assert_success(id_, "E", "iago") + # users cannot edit topics if allow_message_editing is False set_message_editing_params(False, 0, Realm.POLICY_EVERYONE) do_edit_message_assert_error( @@ -876,8 +911,9 @@ class EditMessageTest(EditMessageTestCase): message.save() set_message_editing_params(True, 0, Realm.POLICY_EVERYONE) do_edit_message_assert_success(id_, "E", "iago") + do_edit_message_assert_success(id_, "F", "shiva") do_edit_message_assert_error( - id_, "F", "The time limit for editing this message's topic has passed", "cordelia" + id_, "G", "The time limit for editing this message's topic has passed", "cordelia" ) # anyone should be able to edit "no topic" indefinitely diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 6c04a24c4b..1dbf68769e 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -930,6 +930,21 @@ class RealmAPITest(ZulipTestCase): self.assertEqual(realm.message_content_edit_limit_seconds, 200) self.assertEqual(realm.edit_topic_policy, Realm.POLICY_ADMINS_ONLY) + realm = self.update_with_api("edit_topic_policy", Realm.POLICY_MODERATORS_ONLY) + self.assertEqual(realm.allow_message_editing, False) + self.assertEqual(realm.message_content_edit_limit_seconds, 200) + self.assertEqual(realm.edit_topic_policy, Realm.POLICY_MODERATORS_ONLY) + + realm = self.update_with_api("edit_topic_policy", Realm.POLICY_FULL_MEMBERS_ONLY) + self.assertEqual(realm.allow_message_editing, False) + self.assertEqual(realm.message_content_edit_limit_seconds, 200) + self.assertEqual(realm.edit_topic_policy, Realm.POLICY_FULL_MEMBERS_ONLY) + + realm = self.update_with_api("edit_topic_policy", Realm.POLICY_MEMBERS_ONLY) + self.assertEqual(realm.allow_message_editing, False) + self.assertEqual(realm.message_content_edit_limit_seconds, 200) + self.assertEqual(realm.edit_topic_policy, Realm.POLICY_MEMBERS_ONLY) + # Test an invalid value for edit_topic_policy invalid_edit_topic_policy_value = 10 req = {"edit_topic_policy": orjson.dumps(invalid_edit_topic_policy_value).decode()}