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

@@ -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.
 

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

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.OLD_VALUE: get_group_setting_value_for_audit_log_data(
-            RealmAuditLog.NEW_VALUE: setting_value_group.id,
+                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)

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),
     ],
 )
 

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(
+    if current_setting_value is not None and not hasattr(current_setting_value, "named_user_group"):
-        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.

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
+                                      allOf:
-                                      description: |
+                                        - $ref: "#/components/schemas/CanMentionGroup"
-                                        ID of the new user group whose members are allowed to mention the
+                                        - description: |
-                                        group.
+                                            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),
+                                            **Changes**: Before Zulip 9.0 (feature level 258), the
-                                        the `can_mention_group` setting
+                                            `can_mention_group` field was always an integer.
-                                        was named `can_mention_group_id`.
 
-                                        New in Zulip 8.0 (feature level 191). Previously, groups
+                                            **Changes**: Before Zulip 8.0 (feature level 198),
-                                        could be mentioned if and only if they were not system groups.
+                                            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: |
+                  allOf:
-                    ID of the new user group whose members are allowed to mention the
+                    - description: |
-                    group.
+                        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"`
+                        This setting cannot be set to `"role:internet"` and `"role:owners"`
-                    system groups.
+                        system groups.
 
-                    **Changes**: Before Zulip 8.0 (feature level 198),
+                        **Changes**: Before Zulip 9.0 (feature level 258), the
-                    the `can_mention_group` setting was named `can_mention_group_id`.
+                        `can_mention_group` this field was always an integer.
 
-                    New in Zulip 8.0 (feature level 191). Previously, groups
+                        **Changes**: Before Zulip 8.0 (feature level 198),
-                    could be mentioned if and only if they were not system groups.
+                        the `can_mention_group` setting was named `can_mention_group_id`.
-                  type: integer
+
+                        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:

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",
+            },
+        )

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")

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)

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: Optional[Union[Dict[str, List[int]], int]] = REQ(
-        "can_mention_group", json_validator=check_int, default=None
+        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_name not in request_settings_dict:  # nocoverage
-
-        if setting_group_id_name not in request_settings_dict:  # nocoverage
             continue
 
-        if request_settings_dict[setting_group_id_name] is not None and request_settings_dict[
+        if request_settings_dict[setting_name] is None:
-            setting_group_id_name
+            continue
-        ] != getattr(user_group, setting_group_id_name):
+
-            setting_value_group_id = request_settings_dict[setting_group_id_name]
+        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