From 3f80bc1b417525039a184d4f7d7c8e11ec1167d7 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Tue, 30 Apr 2024 18:46:58 +0530 Subject: [PATCH] groups: Allow setting anonymous group to settings while editing. This commit adds support to set can_mention_group setting to anonymous group while editing groups. --- api_docs/changelog.md | 3 +- version.py | 2 +- zerver/actions/user_groups.py | 39 +++++++++++++-- zerver/lib/event_schema.py | 2 +- zerver/lib/user_groups.py | 4 +- zerver/openapi/zulip.yaml | 48 +++++++++++------- zerver/tests/test_audit_log.py | 86 ++++++++++++++++++++++++++++++++ zerver/tests/test_events.py | 16 ++++++ zerver/tests/test_user_groups.py | 78 +++++++++++++++++++++++++++++ zerver/views/user_groups.py | 54 +++++++++++++++----- 10 files changed, 291 insertions(+), 41 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 9883b3703e..17a8c29ff8 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -26,7 +26,8 @@ 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 +* [`POST /user_groups/create`](/api/create-user-group), [`PATCH + /user_groups/{user_group_id}`](/api/update-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. diff --git a/version.py b/version.py index 5ce318009f..d88d801941 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 257 +API_FEATURE_LEVEL = 258 # 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/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index 0c4b13a40f..bff5678927 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -1,3 +1,4 @@ +from dataclasses import asdict from datetime import datetime from typing import Dict, List, Mapping, Optional, Sequence, TypedDict, Union @@ -8,6 +9,7 @@ from django.utils.translation import gettext as _ from zerver.lib.exceptions import JsonableError from zerver.lib.user_groups import ( + AnonymousSettingGroupDict, get_group_setting_value_for_api, get_role_based_system_groups_dict, set_defaults_for_group_settings, @@ -207,7 +209,7 @@ def check_add_user_group( def do_send_user_group_update_event( - user_group: NamedUserGroup, data: Dict[str, Union[str, int]] + user_group: NamedUserGroup, data: Dict[str, Union[str, int, AnonymousSettingGroupDict]] ) -> None: event = dict(type="user_group", op="update", group_id=user_group.id, data=data) send_event(user_group.realm, event, active_user_ids(user_group.realm_id)) @@ -431,6 +433,15 @@ def check_delete_user_group(user_group: NamedUserGroup, *, acting_user: UserProf do_send_delete_user_group_event(acting_user.realm, user_group_id, acting_user.realm.id) +def get_group_setting_value_for_audit_log_data( + setting_value: Union[int, AnonymousSettingGroupDict], +) -> Union[int, Dict[str, List[int]]]: + if isinstance(setting_value, int): + return setting_value + + return asdict(setting_value) + + @transaction.atomic(savepoint=False) def do_change_user_group_permission_setting( user_group: NamedUserGroup, @@ -442,6 +453,20 @@ def do_change_user_group_permission_setting( old_value = getattr(user_group, setting_name) setattr(user_group, setting_name, setting_value_group) user_group.save() + + old_setting_api_value = get_group_setting_value_for_api(old_value) + new_setting_api_value = get_group_setting_value_for_api(setting_value_group) + + if not hasattr(old_value, "named_user_group") and hasattr( + setting_value_group, "named_user_group" + ): + # We delete the UserGroup which the setting was set to + # previously if it does not have any linked NamedUserGroup + # object, as it is not used anywhere else. A new UserGroup + # object would be created if the setting is later set to + # a combination of users and groups. + old_value.delete() + RealmAuditLog.objects.create( realm=user_group.realm, acting_user=acting_user, @@ -449,11 +474,17 @@ def do_change_user_group_permission_setting( event_time=timezone_now(), modified_user_group=user_group, extra_data={ - RealmAuditLog.OLD_VALUE: old_value.id, - RealmAuditLog.NEW_VALUE: setting_value_group.id, + RealmAuditLog.OLD_VALUE: get_group_setting_value_for_audit_log_data( + old_setting_api_value + ), + RealmAuditLog.NEW_VALUE: get_group_setting_value_for_audit_log_data( + new_setting_api_value + ), "property": setting_name, }, ) - event_data_dict: Dict[str, Union[str, int]] = {setting_name: setting_value_group.id} + event_data_dict: Dict[str, Union[str, int, AnonymousSettingGroupDict]] = { + setting_name: new_setting_api_value + } do_send_user_group_update_event(user_group, event_data_dict) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 183cd261d3..7ce886d658 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1857,7 +1857,7 @@ user_group_data_type = DictType( optional_keys=[ ("name", str), ("description", str), - ("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 73823e36de..43751be292 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -242,9 +242,7 @@ def update_or_create_user_group_for_setting( direct_subgroups: List[int], current_setting_value: Optional[UserGroup], ) -> UserGroup: - if current_setting_value is not None and not hasattr( - current_setting_value, "named_user_group" - ): # nocoverage + if current_setting_value is not None and not hasattr(current_setting_value, "named_user_group"): # 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. diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 760fef2aa2..7c13fd8133 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -3174,17 +3174,22 @@ paths: The new description of the group. Only present if the description changed. can_mention_group: - type: integer - description: | - ID of the new user group whose members are allowed to mention the - group. + allOf: + - $ref: "#/components/schemas/CanMentionGroup" + - 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 group. - **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` field was always an integer. - New in Zulip 8.0 (feature level 191). Previously, groups - could be mentioned if and only if they were not system groups. + **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. example: { "type": "user_group", @@ -18544,19 +18549,24 @@ paths: type: string example: The marketing team. can_mention_group: - description: | - ID of the new user group whose members are allowed to mention the - 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 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` this field was always 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: 12 encoding: can_mention_group: diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 4444c25d5a..cc34a43022 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -81,6 +81,7 @@ from zerver.models import ( RealmPlayground, Recipient, Subscription, + UserGroup, UserProfile, ) from zerver.models.groups import SystemGroups @@ -1333,3 +1334,88 @@ class TestRealmAuditLog(ZulipTestCase): "property": "can_mention_group", }, ) + + moderators_group = NamedUserGroup.objects.get( + name=SystemGroups.MODERATORS, realm=user_group.realm, is_system_group=True + ) + old_group = user_group.can_mention_group + new_group = UserGroup.objects.create(realm=user_group.realm) + new_group.direct_members.set([hamlet.id]) + new_group.direct_subgroups.set([moderators_group.id]) + + now = timezone_now() + do_change_user_group_permission_setting( + user_group, "can_mention_group", new_group, acting_user=None + ) + audit_log_entries = RealmAuditLog.objects.filter( + event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, + event_time__gte=now, + ) + self.assert_length(audit_log_entries, 1) + self.assertIsNone(audit_log_entries[0].acting_user) + self.assertDictEqual( + audit_log_entries[0].extra_data, + { + RealmAuditLog.OLD_VALUE: old_group.id, + RealmAuditLog.NEW_VALUE: { + "direct_members": [hamlet.id], + "direct_subgroups": [moderators_group.id], + }, + "property": "can_mention_group", + }, + ) + + othello = self.example_user("othello") + new_group = UserGroup.objects.create(realm=user_group.realm) + new_group.direct_members.set([othello.id]) + new_group.direct_subgroups.set([moderators_group.id]) + + now = timezone_now() + do_change_user_group_permission_setting( + user_group, "can_mention_group", new_group, acting_user=None + ) + audit_log_entries = RealmAuditLog.objects.filter( + event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, + event_time__gte=now, + ) + self.assert_length(audit_log_entries, 1) + self.assertIsNone(audit_log_entries[0].acting_user) + self.assertDictEqual( + audit_log_entries[0].extra_data, + { + RealmAuditLog.OLD_VALUE: { + "direct_members": [hamlet.id], + "direct_subgroups": [moderators_group.id], + }, + RealmAuditLog.NEW_VALUE: { + "direct_members": [othello.id], + "direct_subgroups": [moderators_group.id], + }, + "property": "can_mention_group", + }, + ) + + new_group = NamedUserGroup.objects.get( + name=SystemGroups.EVERYONE, realm=user_group.realm, is_system_group=True + ) + now = timezone_now() + do_change_user_group_permission_setting( + user_group, "can_mention_group", new_group, acting_user=None + ) + audit_log_entries = RealmAuditLog.objects.filter( + event_type=RealmAuditLog.USER_GROUP_GROUP_BASED_SETTING_CHANGED, + event_time__gte=now, + ) + self.assert_length(audit_log_entries, 1) + self.assertIsNone(audit_log_entries[0].acting_user) + self.assertDictEqual( + audit_log_entries[0].extra_data, + { + RealmAuditLog.OLD_VALUE: { + "direct_members": [othello.id], + "direct_subgroups": [moderators_group.id], + }, + RealmAuditLog.NEW_VALUE: new_group.id, + "property": "can_mention_group", + }, + ) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index ec6236e78d..0ac4379704 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1745,6 +1745,22 @@ class NormalActionsTest(BaseAction): backend, "can_mention_group", moderators_group, acting_user=None ) check_user_group_update("events[0]", events[0], "can_mention_group") + self.assertEqual(events[0]["data"]["can_mention_group"], moderators_group.id) + + setting_group = UserGroup.objects.create(realm=self.user_profile.realm) + setting_group.direct_members.set([othello.id]) + setting_group.direct_subgroups.set([moderators_group.id]) + with self.verify_action() as events: + do_change_user_group_permission_setting( + backend, "can_mention_group", setting_group, acting_user=None + ) + check_user_group_update("events[0]", events[0], "can_mention_group") + self.assertEqual( + events[0]["data"]["can_mention_group"], + AnonymousSettingGroupDict( + direct_members=[othello.id], direct_subgroups=[moderators_group.id] + ), + ) # Test add members hamlet = self.example_user("hamlet") diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index b6c2e814ab..f1783b77b4 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -671,6 +671,62 @@ class UserGroupAPITestCase(UserGroupTestCase): support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) self.assertEqual(support_group.can_mention_group, nobody_group.usergroup_ptr) + othello = self.example_user("othello") + params = { + "can_mention_group": orjson.dumps( + { + "direct_members": [othello.id], + "direct_subgroups": [moderators_group.id, marketing_group.id], + } + ).decode() + } + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_success(result) + support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) + self.assertCountEqual( + list(support_group.can_mention_group.direct_members.all()), + [othello], + ) + self.assertCountEqual( + list(support_group.can_mention_group.direct_subgroups.all()), + [marketing_group, moderators_group], + ) + + prospero = self.example_user("prospero") + params = { + "can_mention_group": orjson.dumps( + { + "direct_members": [othello.id, prospero.id], + "direct_subgroups": [moderators_group.id, marketing_group.id], + } + ).decode() + } + previous_can_mention_group_id = support_group.can_mention_group_id + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_success(result) + support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) + + # Test that the existing UserGroup object is updated. + self.assertEqual(support_group.can_mention_group_id, previous_can_mention_group_id) + self.assertCountEqual( + list(support_group.can_mention_group.direct_members.all()), + [othello, prospero], + ) + self.assertCountEqual( + list(support_group.can_mention_group.direct_subgroups.all()), + [marketing_group, moderators_group], + ) + + params = {"can_mention_group": orjson.dumps(marketing_group.id).decode()} + previous_can_mention_group_id = support_group.can_mention_group_id + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_success(result) + support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) + + # Test that the previous UserGroup object is deleted. + self.assertFalse(UserGroup.objects.filter(id=previous_can_mention_group_id).exists()) + self.assertEqual(support_group.can_mention_group_id, marketing_group.id) + owners_group = NamedUserGroup.objects.get( name="role:owners", realm=hamlet.realm, is_system_group=True ) @@ -699,6 +755,28 @@ class UserGroupAPITestCase(UserGroupTestCase): result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) self.assert_json_error(result, "Invalid user group") + params = { + "can_mention_group": orjson.dumps( + { + "direct_members": [1111, othello.id], + "direct_subgroups": [moderators_group.id, marketing_group.id], + } + ).decode() + } + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_error(result, "Invalid user ID: 1111") + + params = { + "can_mention_group": orjson.dumps( + { + "direct_members": [prospero.id, othello.id], + "direct_subgroups": [1111, marketing_group.id], + } + ).decode() + } + result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) + self.assert_json_error(result, "Invalid user group ID: 1111") + def test_user_group_update_to_already_existing_name(self) -> None: hamlet = self.example_user("hamlet") self.login_user(hamlet) diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 23f1225919..9db36b0a2f 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -30,6 +30,7 @@ from zerver.lib.user_groups import ( access_user_group_for_setting, check_user_group_name, get_direct_memberships_of_users, + get_group_setting_value_for_api, get_subgroup_ids, get_user_group_direct_member_ids, get_user_group_member_ids, @@ -39,7 +40,7 @@ from zerver.lib.user_groups import ( ) from zerver.lib.users import access_user_by_id, user_ids_to_users 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 import NamedUserGroup, UserGroup, UserProfile from zerver.models.users import get_system_bot from zerver.views.streams import compose_views @@ -121,6 +122,34 @@ def get_user_group(request: HttpRequest, user_profile: UserProfile) -> HttpRespo return json_success(request, data={"user_groups": user_groups}) +def are_both_setting_values_equal( + first_setting_value: Union[int, AnonymousSettingGroupDict], + second_setting_value: Union[int, AnonymousSettingGroupDict], +) -> bool: + if isinstance(first_setting_value, int) and isinstance(second_setting_value, int): + return first_setting_value == second_setting_value + + if isinstance(first_setting_value, AnonymousSettingGroupDict) and isinstance( + second_setting_value, AnonymousSettingGroupDict + ): + return set(first_setting_value.direct_members) == set( + second_setting_value.direct_members + ) and set(first_setting_value.direct_subgroups) == set( + second_setting_value.direct_subgroups + ) + + return False + + +def check_setting_value_changed( + current_value: UserGroup, + new_setting_value: Union[int, AnonymousSettingGroupDict], +) -> bool: + current_setting_api_value = get_group_setting_value_for_api(current_value) + + return not are_both_setting_values_equal(current_setting_api_value, new_setting_value) + + @transaction.atomic @require_user_group_edit_permission @has_request_variables @@ -130,11 +159,11 @@ def edit_user_group( user_group_id: int = REQ(json_validator=check_int, path_only=True), name: Optional[str] = REQ(default=None), description: Optional[str] = REQ(default=None), - 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: - if name is None and description is None and can_mention_group_id is None: + if name is None and description is None and can_mention_group is None: raise JsonableError(_("No new data supplied")) user_group = access_user_group_by_id(user_group_id, user_profile, for_read=False) @@ -148,20 +177,21 @@ def edit_user_group( 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 and request_settings_dict[ - setting_group_id_name - ] != getattr(user_group, setting_group_id_name): - setting_value_group_id = request_settings_dict[setting_group_id_name] + if request_settings_dict[setting_name] is None: + continue + + current_value = getattr(user_group, setting_name) + new_setting_value = parse_group_setting_value(request_settings_dict[setting_name]) + if check_setting_value_changed(current_value, new_setting_value): setting_value_group = access_user_group_for_setting( - setting_value_group_id, + new_setting_value, user_profile, setting_name=setting_name, permission_configuration=permission_config, + current_setting_value=current_value, ) do_change_user_group_permission_setting( user_group, setting_name, setting_value_group, acting_user=user_profile