From 51d382f7171aa09bda3bf5a8d1e771d7481ea219 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 17 Apr 2025 12:43:07 +0000 Subject: [PATCH] users: Return true for admins for `is_moderator`. This change is for consistency with how is_admin works. API design discussion at https://chat.zulip.org/#narrow/channel/378-api-design/topic/Should.20is_moderator.20have.20admins --- api_docs/changelog.md | 7 +++++++ api_docs/roles-and-permissions.md | 3 ++- version.py | 2 +- web/src/message_edit.ts | 4 ++-- web/src/people.ts | 8 +++++++- web/src/user_events.ts | 3 ++- web/tests/message_edit.test.cjs | 8 ++++---- web/tests/people.test.cjs | 6 ++++-- web/tests/user_events.test.cjs | 2 +- zerver/actions/message_edit.py | 3 --- zerver/lib/events.py | 3 ++- zerver/lib/user_groups.py | 4 +--- zerver/lib/users.py | 4 ++++ zerver/models/users.py | 2 +- zerver/openapi/zulip.yaml | 9 ++++++--- zerver/tests/test_subs.py | 13 +++++++++---- zerver/tests/test_users.py | 11 +++-------- zerver/views/development/dev_login.py | 6 ++---- 18 files changed, 58 insertions(+), 40 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 87d219df59..d83ff87d37 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 11.0 +**Feature level 380** + +* [`POST /register`](/api/register-queue), [`GET + /events`](/api/get-events): The `is_moderator` convenience field now + is true for organization administrators, matching how `is_admin` + works for organization owners. + **Feature level 379** * [`PATCH /messages/{message_id}`](/api/update-message): Added diff --git a/api_docs/roles-and-permissions.md b/api_docs/roles-and-permissions.md index ede476dcea..f53ae9c992 100644 --- a/api_docs/roles-and-permissions.md +++ b/api_docs/roles-and-permissions.md @@ -36,7 +36,8 @@ by the [events API](/api/get-events). Note that [`POST /register`](/api/register-queue) also returns an `is_moderator` boolean property specifying whether the current user is -an organization moderator. +at least an organization moderator. The property will be true for admins +and owners too. Additionally, user account data include an `is_billing_admin` property specifying whether the user is a billing administrator for the Zulip diff --git a/version.py b/version.py index 4ffb480132..baa055c6bd 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 = 379 +API_FEATURE_LEVEL = 380 # 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/message_edit.ts b/web/src/message_edit.ts index c1298619b0..0542f46da4 100644 --- a/web/src/message_edit.ts +++ b/web/src/message_edit.ts @@ -117,7 +117,7 @@ export function is_topic_editable(message: Message, edit_limit_seconds_buffer = // Organization admins and moderators can edit message topics indefinitely, // irrespective of the topic editing deadline, if they are in the // can_move_messages_between_topics_group. - if (current_user.is_admin || current_user.is_moderator) { + if (current_user.is_moderator) { return true; } @@ -270,7 +270,7 @@ export function is_stream_editable(message: Message, edit_limit_seconds_buffer = // Organization admins and moderators can edit stream indefinitely, // irrespective of the stream editing deadline, if they are in the // can_move_messages_between_channels_group. - if (current_user.is_admin || current_user.is_moderator) { + if (current_user.is_moderator) { return true; } diff --git a/web/src/people.ts b/web/src/people.ts index 7e39727a7b..9de0a0b94e 100644 --- a/web/src/people.ts +++ b/web/src/people.ts @@ -1432,7 +1432,13 @@ export function _add_user(person: User): void { our realm (like cross-realm bots). */ person.is_moderator = false; - if (person.role === settings_config.user_role_values.moderator.code) { + if ( + [ + settings_config.user_role_values.moderator.code, + settings_config.user_role_values.admin.code, + settings_config.user_role_values.owner.code, + ].includes(person.role) + ) { person.is_moderator = true; } if (person.user_id) { diff --git a/web/src/user_events.ts b/web/src/user_events.ts index 91e7cac1dd..083dcfe099 100644 --- a/web/src/user_events.ts +++ b/web/src/user_events.ts @@ -109,7 +109,8 @@ export const update_person = function update(event: UserUpdate): void { user.is_owner = event.role === settings_config.user_role_values.owner.code; user.is_admin = event.role === settings_config.user_role_values.admin.code || user.is_owner; user.is_guest = event.role === settings_config.user_role_values.guest.code; - user.is_moderator = event.role === settings_config.user_role_values.moderator.code; + user.is_moderator = + user.is_admin || event.role === settings_config.user_role_values.moderator.code; settings_users.update_user_data(event.user_id, event); user_profile.update_profile_modal_ui(user, event); diff --git a/web/tests/message_edit.test.cjs b/web/tests/message_edit.test.cjs index 600ab4946f..7fc61006db 100644 --- a/web/tests/message_edit.test.cjs +++ b/web/tests/message_edit.test.cjs @@ -95,7 +95,7 @@ run_test("is_topic_editable", ({override}) => { }; override(realm, "realm_allow_message_editing", true); override(settings_data, "user_can_move_messages_to_another_topic", () => true); - override(current_user, "is_admin", true); + override(current_user, "is_moderator", true); assert.equal(message_edit.is_topic_editable(message), false); @@ -112,7 +112,7 @@ run_test("is_topic_editable", ({override}) => { override(settings_data, "user_can_move_messages_to_another_topic", () => false); assert.equal(message_edit.is_topic_editable(message), false); - override(current_user, "is_admin", false); + override(current_user, "is_moderator", false); assert.equal(message_edit.is_topic_editable(message), false); message.topic = "translated: (no topic)"; @@ -149,7 +149,7 @@ run_test("is_stream_editable", ({override}) => { }; override(realm, "realm_allow_message_editing", true); override(settings_data, "user_can_move_messages_between_streams", () => true); - override(current_user, "is_admin", true); + override(current_user, "is_moderator", true); assert.equal(message_edit.is_stream_editable(message), false); @@ -166,7 +166,7 @@ run_test("is_stream_editable", ({override}) => { override(settings_data, "user_can_move_messages_between_streams", () => false); assert.equal(message_edit.is_stream_editable(message), false); - override(current_user, "is_admin", false); + override(current_user, "is_moderator", false); assert.equal(message_edit.is_stream_editable(message), false); override(realm, "realm_move_messages_between_streams_limit_seconds", 259200); diff --git a/web/tests/people.test.cjs b/web/tests/people.test.cjs index 9efb5b1742..69a5e2b1f5 100644 --- a/web/tests/people.test.cjs +++ b/web/tests/people.test.cjs @@ -122,7 +122,7 @@ const realm_admin = { is_owner: false, is_admin: true, is_guest: false, - is_moderator: false, + is_moderator: true, is_bot: false, role: 200, }; @@ -146,7 +146,7 @@ const realm_owner = { is_owner: true, is_admin: true, is_guest: false, - is_moderator: false, + is_moderator: true, is_bot: false, role: 100, }; @@ -686,7 +686,9 @@ test_people("user_type", () => { people.add_active_user(realm_admin); people.add_active_user(guest); people.add_active_user(realm_owner); + console.log("AAAaaAAAAAAAAA"); people.add_active_user(moderator); + console.log("BABSDBSDBSDBBDSBSDB"); people.add_active_user(bot_botson); assert.equal(people.get_user_type(me.user_id), $t({defaultMessage: "Member"})); assert.equal(people.get_user_type(realm_admin.user_id), $t({defaultMessage: "Administrator"})); diff --git a/web/tests/user_events.test.cjs b/web/tests/user_events.test.cjs index e3998c776c..c54427d15d 100644 --- a/web/tests/user_events.test.cjs +++ b/web/tests/user_events.test.cjs @@ -133,7 +133,7 @@ run_test("updates", ({override}) => { }); person = people.get_by_email(isaac.email); assert.equal(person.full_name, "Isaac Newton"); - assert.equal(person.is_moderator, false); + assert.equal(person.is_moderator, true); assert.equal(person.is_admin, true); assert.equal(person.role, settings_config.user_role_values.admin.code); diff --git a/zerver/actions/message_edit.py b/zerver/actions/message_edit.py index 1f6f4aae87..be4dd47873 100644 --- a/zerver/actions/message_edit.py +++ b/zerver/actions/message_edit.py @@ -1438,7 +1438,6 @@ def check_update_message( # and the time limit for editing topics is passed, raise an error. if ( 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 ): deadline_seconds = ( @@ -1504,7 +1503,6 @@ def check_update_message( if ( user_profile.realm.move_messages_between_streams_limit_seconds is not None - and not user_profile.is_realm_admin and not user_profile.is_moderator ): deadline_seconds = ( @@ -1518,7 +1516,6 @@ def check_update_message( if ( propagate_mode == "change_all" - and not user_profile.is_realm_admin and not user_profile.is_moderator and message_edit_request.is_message_moved and not message_edit_request.topic_resolved diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 9f3b6ceef5..85f44a0829 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -75,6 +75,7 @@ from zerver.lib.users import ( get_data_for_inaccessible_user, get_users_for_api, is_administrator_role, + is_moderator_role, max_message_id_for_user, ) from zerver.lib.utils import optional_bytes_to_mib @@ -1096,7 +1097,7 @@ def apply_event( if "role" in person: state["is_admin"] = is_administrator_role(person["role"]) state["is_owner"] = person["role"] == UserProfile.ROLE_REALM_OWNER - state["is_moderator"] = person["role"] == UserProfile.ROLE_MODERATOR + state["is_moderator"] = is_moderator_role(person["role"]) state["is_guest"] = person["role"] == UserProfile.ROLE_GUEST # Recompute properties based on is_admin/is_guest state["can_create_private_streams"] = user_profile.can_create_private_streams() diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 4420f2e8e1..5b79666e43 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -1234,10 +1234,8 @@ def check_user_has_permission_by_role( if system_group_name == SystemGroups.ADMINISTRATORS: return user.is_realm_admin - # is_moderator returns False for realm admins and - # owners. if system_group_name == SystemGroups.MODERATORS: - return user.is_realm_admin or user.is_moderator + return user.is_moderator # Handle full members case. return user.role != UserProfile.ROLE_MEMBER or not user.is_provisional_member diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 9eb387c2e1..ab556d1ee3 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -216,6 +216,10 @@ def is_administrator_role(role: int) -> bool: return role in {UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_REALM_OWNER} +def is_moderator_role(role: int) -> bool: + return is_administrator_role(role) or role == UserProfile.ROLE_MODERATOR + + def bulk_get_cross_realm_bots() -> dict[str, UserProfile]: emails = list(settings.CROSS_REALM_BOT_EMAILS) diff --git a/zerver/models/users.py b/zerver/models/users.py index c8bacac88a..e8cadb9ae4 100644 --- a/zerver/models/users.py +++ b/zerver/models/users.py @@ -776,7 +776,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings): @property def is_moderator(self) -> bool: - return self.role == UserProfile.ROLE_MODERATOR + return self.is_realm_admin or self.role == UserProfile.ROLE_MODERATOR @is_moderator.setter def is_moderator(self, value: bool) -> None: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 8ab564688a..fc57c112c5 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -19208,7 +19208,7 @@ paths: description: | Present if `realm_user` is present in `fetch_event_types`. - Whether the current user is an [organization administrator](/api/roles-and-permissions). + Whether the current user is at least an [organization administrator](/api/roles-and-permissions). is_owner: type: boolean description: | @@ -19222,9 +19222,12 @@ paths: description: | Present if `realm_user` is present in `fetch_event_types`. - Whether the current user is an [organization moderator](/api/roles-and-permissions). + Whether the current user is at least an [organization moderator](/api/roles-and-permissions). - **Changes**: New in Zulip 4.0 (feature level 60). + **Changes**: Prior to Zulip 11.0 (feature level 380), this was only true + for users whose role was exactly the moderator role. + + New in Zulip 4.0 (feature level 60). is_guest: type: boolean description: | diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 62b1c0e661..06576e0c0e 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -5628,10 +5628,6 @@ class SubscriptionAPITest(ZulipTestCase): realm, "can_add_subscribers_group", moderators_group, acting_user=None ) - # Moderators, Admins and owners are always full members. - do_change_user_role(self.test_user, UserProfile.ROLE_MODERATOR, acting_user=None) - self.assertFalse(self.test_user.is_provisional_member) - do_change_user_role(self.test_user, UserProfile.ROLE_MEMBER, acting_user=None) # Make sure that we are checking the permission with a full member, # as full member is the user just below moderator in the role hierarchy. @@ -5694,6 +5690,15 @@ class SubscriptionAPITest(ZulipTestCase): ) self.assert_json_error(result, "Insufficient permission") + # Moderators, Admins and owners are always full members. + self.assertTrue(user_profile.is_provisional_member) + do_change_user_role(self.test_user, UserProfile.ROLE_MODERATOR, acting_user=None) + self.assertFalse(self.test_user.is_provisional_member) + do_change_user_role(self.test_user, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None) + self.assertFalse(self.test_user.is_provisional_member) + do_change_user_role(self.test_user, UserProfile.ROLE_REALM_OWNER, acting_user=None) + self.assertFalse(self.test_user.is_provisional_member) + do_set_realm_property(realm, "waiting_period_threshold", 0, acting_user=None) self.subscribe_via_post( self.test_user, ["stream2"], {"principals": orjson.dumps([invitee_user_id]).decode()} diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 2a9d8dd604..6bd1c87e4d 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -116,7 +116,7 @@ class PermissionTest(ZulipTestCase): self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR) user_profile.is_moderator = False - self.assertEqual(user_profile.is_moderator, False) + self.assertEqual(user_profile.is_moderator, True) self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR) user_profile.is_realm_admin = False @@ -593,21 +593,16 @@ class PermissionTest(ZulipTestCase): user_profile.is_realm_admin and not user_profile.is_guest and not user_profile.is_realm_owner - and not user_profile.is_moderator ) elif role == UserProfile.ROLE_REALM_OWNER: return ( user_profile.is_realm_owner and user_profile.is_realm_admin - and not user_profile.is_moderator and not user_profile.is_guest ) elif role == UserProfile.ROLE_MODERATOR: - return ( - user_profile.is_moderator - and not user_profile.is_realm_owner - and not user_profile.is_realm_admin - and not user_profile.is_guest + return user_profile.is_moderator or ( + user_profile.is_realm_admin and not user_profile.is_guest ) if role == UserProfile.ROLE_MEMBER: diff --git a/zerver/views/development/dev_login.py b/zerver/views/development/dev_login.py index 99c13e9492..0eb49edd5b 100644 --- a/zerver/views/development/dev_login.py +++ b/zerver/views/development/dev_login.py @@ -58,10 +58,8 @@ def add_dev_login_context(realm: Realm | None, context: dict[str, Any]) -> None: context["direct_owners"] = sort([u for u in users if u.is_realm_owner]) context["direct_admins"] = sort([u for u in users if u.is_realm_admin and not u.is_realm_owner]) context["guest_users"] = sort([u for u in users if u.is_guest]) - context["direct_moderators"] = sort([u for u in users if u.is_moderator]) - context["direct_users"] = sort( - [u for u in users if not (u.is_realm_admin or u.is_guest or u.is_moderator)] - ) + context["direct_moderators"] = sort([u for u in users if u.role == UserProfile.ROLE_MODERATOR]) + context["direct_users"] = sort([u for u in users if not (u.is_guest or u.is_moderator)]) @csrf_exempt