diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 389d2320ea..9883b3703e 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -26,6 +26,9 @@ format used by the Zulip server that they are interacting with. /register`](/api/register-queue): `can_mention_group` field can now either be an ID of a named user group with the permission, or an object describing the set of users and groups with the permission. +* [`POST /user_groups/create`](/api/create-user-group): The + `can_mention_group` parameter can now either be an ID of a named + user group or an object describing a set of users and groups. **Feature level 257**: diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index 43e51dfd60..0c4b13a40f 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -8,6 +8,7 @@ from django.utils.translation import gettext as _ from zerver.lib.exceptions import JsonableError from zerver.lib.user_groups import ( + get_group_setting_value_for_api, get_role_based_system_groups_dict, set_defaults_for_group_settings, ) @@ -175,7 +176,7 @@ def do_send_create_user_group_event( id=user_group.id, is_system_group=user_group.is_system_group, direct_subgroup_ids=[direct_subgroup.id for direct_subgroup in direct_subgroups], - can_mention_group=user_group.can_mention_group_id, + can_mention_group=get_group_setting_value_for_api(user_group.can_mention_group), ), ) send_event(user_group.realm, event, active_user_ids(user_group.realm_id)) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 0aa5c8b4b5..183cd261d3 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1790,6 +1790,18 @@ update_message_flags_remove_event = event_dict_type( check_update_message_flags_remove = make_checker(update_message_flags_remove_event) +group_setting_type = UnionType( + [ + int, + DictType( + required_keys=[ + ("direct_members", ListType(int)), + ("direct_subgroups", ListType(int)), + ] + ), + ] +) + group_type = DictType( required_keys=[ ("id", int), @@ -1798,7 +1810,7 @@ group_type = DictType( ("direct_subgroup_ids", ListType(int)), ("description", str), ("is_system_group", bool), - ("can_mention_group", int), + ("can_mention_group", group_setting_type), ] ) diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 24c6b8e29f..73823e36de 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -241,8 +241,10 @@ def update_or_create_user_group_for_setting( direct_members: List[int], direct_subgroups: List[int], current_setting_value: Optional[UserGroup], -) -> UserGroup: # nocoverage - if current_setting_value is not None and not hasattr(current_setting_value, "named_user_group"): +) -> UserGroup: + if current_setting_value is not None and not hasattr( + current_setting_value, "named_user_group" + ): # nocoverage # We do not create a new group if the setting was already set # to an anonymous group. The memberships of existing group # itself are updated. @@ -287,16 +289,16 @@ def access_user_group_for_setting( # The API would not allow passing the setting parameter as a Dict # if require_system_group is true for a setting. - assert permission_configuration.require_system_group is False # nocoverage + assert permission_configuration.require_system_group is False user_group = update_or_create_user_group_for_setting( user_profile.realm, setting_user_group.direct_members, setting_user_group.direct_subgroups, current_setting_value, - ) # nocoverage + ) - return user_group # nocoverage + return user_group def check_user_group_name(group_name: str) -> str: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index db3c16f8d9..760fef2aa2 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -18389,19 +18389,24 @@ paths: type: integer example: [1, 2, 3, 4] can_mention_group: - description: | - ID of the user group whose members are allowed to mention the new - user group. + allOf: + - description: | + Either the ID of a named user group that has permission to + mention the group, or an object describing the set of users + and groups who have permission mention the new group. - This setting cannot be set to `"role:internet"` and `"role:owners"` - system groups. + This setting cannot be set to `"role:internet"` and + `"role:owners"` system groups. - **Changes**: Before Zulip 8.0 (feature level 198), - the `can_mention_group` setting was named `can_mention_group_id`. + **Changes**: Before Zulip 9.0 (feature level 258), the + `can_mention_group` setting could only be set to an integer. - New in Zulip 8.0 (feature level 191). Previously, groups - could be mentioned if and only if they were not system groups. - type: integer + **Changes**: Before Zulip 8.0 (feature level 198), + the `can_mention_group` setting was named `can_mention_group_id`. + + New in Zulip 8.0 (feature level 191). Previously, groups + could be mentioned if and only if they were not system groups. + - $ref: "#/components/schemas/CanMentionGroup" example: 11 required: - name diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 2a3e058a46..ec6236e78d 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -216,6 +216,7 @@ from zerver.lib.test_helpers import ( from zerver.lib.timestamp import convert_to_UTC, datetime_to_timestamp from zerver.lib.topic import TOPIC_NAME from zerver.lib.types import ProfileDataElementUpdateDict +from zerver.lib.user_groups import AnonymousSettingGroupDict from zerver.models import ( Attachment, CustomProfileField, @@ -231,6 +232,7 @@ from zerver.models import ( RealmUserDefault, Service, Stream, + UserGroup, UserMessage, UserPresence, UserProfile, @@ -1696,6 +1698,34 @@ class NormalActionsTest(BaseAction): self.user_profile.realm, "backend", [othello], "Backend team", acting_user=None ) check_user_group_add("events[0]", events[0]) + everyone_group = NamedUserGroup.objects.get( + name=SystemGroups.EVERYONE, realm=self.user_profile.realm, is_system_group=True + ) + self.assertEqual(events[0]["group"]["can_mention_group"], everyone_group.id) + + moderators_group = NamedUserGroup.objects.get( + name=SystemGroups.MODERATORS, realm=self.user_profile.realm, is_system_group=True + ) + user_group = UserGroup.objects.create(realm=self.user_profile.realm) + user_group.direct_members.set([othello]) + user_group.direct_subgroups.set([moderators_group]) + + with self.verify_action() as events: + check_add_user_group( + self.user_profile.realm, + "frontend", + [othello], + "", + {"can_mention_group": user_group}, + acting_user=None, + ) + check_user_group_add("events[0]", events[0]) + self.assertEqual( + events[0]["group"]["can_mention_group"], + AnonymousSettingGroupDict( + direct_members=[othello.id], direct_subgroups=[moderators_group.id] + ), + ) # Test name update backend = NamedUserGroup.objects.get(name="backend") @@ -1710,9 +1740,6 @@ class NormalActionsTest(BaseAction): check_user_group_update("events[0]", events[0], "description") # Test can_mention_group setting update - moderators_group = NamedUserGroup.objects.get( - name="role:moderators", realm=self.user_profile.realm, is_system_group=True - ) with self.verify_action() as events: do_change_user_group_permission_setting( backend, "can_mention_group", moderators_group, acting_user=None diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index eba04f9d52..b6c2e814ab 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -434,6 +434,47 @@ class UserGroupAPITestCase(UserGroupTestCase): marketing_group = NamedUserGroup.objects.get(name="marketing", realm=hamlet.realm) self.assertEqual(marketing_group.can_mention_group, nobody_group.usergroup_ptr) + othello = self.example_user("othello") + params = { + "name": "backend", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Backend team", + "can_mention_group": orjson.dumps( + { + "direct_members": [othello.id], + "direct_subgroups": [leadership_group.id, moderators_group.id], + } + ).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_success(result) + backend_group = NamedUserGroup.objects.get(name="backend", realm=hamlet.realm) + self.assertCountEqual( + list(backend_group.can_mention_group.direct_members.all()), + [othello], + ) + self.assertCountEqual( + list(backend_group.can_mention_group.direct_subgroups.all()), + [leadership_group, moderators_group], + ) + + params = { + "name": "help", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Troubleshooting team", + "can_mention_group": orjson.dumps( + { + "direct_members": [], + "direct_subgroups": [moderators_group.id], + } + ).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_success(result) + help_group = NamedUserGroup.objects.get(name="help", realm=hamlet.realm) + # We do not create a new UserGroup object in such case. + self.assertEqual(help_group.can_mention_group_id, moderators_group.id) + internet_group = NamedUserGroup.objects.get( name="role:internet", realm=hamlet.realm, is_system_group=True ) @@ -471,6 +512,34 @@ class UserGroupAPITestCase(UserGroupTestCase): result = self.client_post("/json/user_groups/create", info=params) self.assert_json_error(result, "Invalid user group") + params = { + "name": "frontend", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Frontend team", + "can_mention_group": orjson.dumps( + { + "direct_members": [1111], + "direct_subgroups": [leadership_group.id, moderators_group.id], + } + ).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_error(result, "Invalid user ID: 1111") + + params = { + "name": "frontend", + "members": orjson.dumps([hamlet.id]).decode(), + "description": "Frontend team", + "can_mention_group": orjson.dumps( + { + "direct_members": [othello.id], + "direct_subgroups": [1111, moderators_group.id], + } + ).decode(), + } + result = self.client_post("/json/user_groups/create", info=params) + self.assert_json_error(result, "Invalid user group ID: 1111") + def test_user_group_get(self) -> None: # Test success user_profile = self.example_user("hamlet") diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index af1605ea15..23f1225919 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -1,4 +1,4 @@ -from typing import List, Optional, Sequence +from typing import Dict, List, Optional, Sequence, Union from django.conf import settings from django.db import transaction @@ -23,7 +23,9 @@ from zerver.lib.exceptions import JsonableError from zerver.lib.mention import MentionBackend, silent_mention_syntax_for_user from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success +from zerver.lib.types import Validator from zerver.lib.user_groups import ( + AnonymousSettingGroupDict, access_user_group_by_id, access_user_group_for_setting, check_user_group_name, @@ -36,12 +38,40 @@ from zerver.lib.user_groups import ( user_groups_in_realm_serialized, ) from zerver.lib.users import access_user_by_id, user_ids_to_users -from zerver.lib.validator import check_bool, check_int, check_list +from zerver.lib.validator import check_bool, check_dict_only, check_int, check_list, check_union from zerver.models import NamedUserGroup, UserProfile from zerver.models.users import get_system_bot from zerver.views.streams import compose_views +def parse_group_setting_value( + setting_value: Union[int, Dict[str, List[int]]], +) -> Union[int, AnonymousSettingGroupDict]: + if isinstance(setting_value, int): + return setting_value + + if len(setting_value["direct_members"]) == 0 and len(setting_value["direct_subgroups"]) == 1: + return setting_value["direct_subgroups"][0] + + return AnonymousSettingGroupDict( + direct_members=setting_value["direct_members"], + direct_subgroups=setting_value["direct_subgroups"], + ) + + +check_group_setting: Validator[Union[int, Dict[str, List[int]]]] = check_union( + [ + check_int, + check_dict_only( + [ + ("direct_members", check_list(check_int)), + ("direct_subgroups", check_list(check_int)), + ] + ), + ] +) + + @require_user_group_edit_permission @has_request_variables def add_user_group( @@ -50,8 +80,8 @@ def add_user_group( name: str = REQ(), members: Sequence[int] = REQ(json_validator=check_list(check_int), default=[]), description: str = REQ(), - can_mention_group_id: Optional[int] = REQ( - "can_mention_group", json_validator=check_int, default=None + can_mention_group: Optional[Union[Dict[str, List[int]], int]] = REQ( + json_validator=check_group_setting, default=None ), ) -> HttpResponse: user_profiles = user_ids_to_users(members, user_profile.realm) @@ -60,15 +90,13 @@ def add_user_group( group_settings_map = {} request_settings_dict = locals() for setting_name, permission_config in NamedUserGroup.GROUP_PERMISSION_SETTINGS.items(): - setting_group_id_name = permission_config.id_field_name - - if setting_group_id_name not in request_settings_dict: # nocoverage + if setting_name not in request_settings_dict: # nocoverage continue - if request_settings_dict[setting_group_id_name] is not None: - setting_value_group_id = request_settings_dict[setting_group_id_name] + if request_settings_dict[setting_name] is not None: + setting_value = parse_group_setting_value(request_settings_dict[setting_name]) setting_value_group = access_user_group_for_setting( - setting_value_group_id, + setting_value, user_profile, setting_name=setting_name, permission_configuration=permission_config,