From 206014f263df16e894eebf538c2d2f61c34be778 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 2 May 2024 09:22:37 +0530 Subject: [PATCH] groups: Update group objects to pass anonymous groups data for settings. This commit updates code to handle the can_mention_group field correctly if the setting is set to an anonymous user group. --- api_docs/changelog.md | 7 ++++ zerver/lib/user_groups.py | 39 +++++++++++++++++--- zerver/openapi/zulip.yaml | 60 +++++++++++++++++++++++-------- zerver/tests/test_event_system.py | 4 +-- zerver/tests/test_home.py | 6 ++-- zerver/tests/test_user_groups.py | 32 +++++++++++++++++ 6 files changed, 125 insertions(+), 23 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index e8a46ffd31..389d2320ea 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 9.0 +**Feature level 258**: + +* [`GET /user_groups`](/api/get-user-groups), [`POST + /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. + **Feature level 257**: * [`POST /register`](/api/register-queue), diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 5f41491493..24c6b8e29f 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -3,7 +3,7 @@ from dataclasses import dataclass from typing import Collection, Dict, Iterable, Iterator, List, Mapping, Optional, TypedDict, Union from django.db import connection, transaction -from django.db.models import F, QuerySet +from django.db.models import F, Prefetch, QuerySet from django.utils.timezone import now as timezone_now from django.utils.translation import gettext as _ from django_cte import With @@ -38,7 +38,7 @@ class UserGroupDict(TypedDict): members: List[int] direct_subgroup_ids: List[int] is_system_group: bool - can_mention_group: int + can_mention_group: Union[int, AnonymousSettingGroupDict] @dataclass @@ -319,13 +319,44 @@ def check_user_group_name(group_name: str) -> str: return group_name +def get_group_setting_value_for_api( + setting_value_group: UserGroup, +) -> Union[int, AnonymousSettingGroupDict]: + if hasattr(setting_value_group, "named_user_group"): + return setting_value_group.id + + return AnonymousSettingGroupDict( + direct_members=[member.id for member in setting_value_group.direct_members.all()], + direct_subgroups=[subgroup.id for subgroup in setting_value_group.direct_subgroups.all()], + ) + + def user_groups_in_realm_serialized(realm: Realm) -> List[UserGroupDict]: """This function is used in do_events_register code path so this code should be performant. We need to do 2 database queries because Django's ORM doesn't properly support the left join between UserGroup and UserGroupMembership that we need. """ - realm_groups = NamedUserGroup.objects.filter(realm=realm) + realm_groups = ( + NamedUserGroup.objects.select_related( + "can_mention_group", "can_mention_group__named_user_group" + ) + # Using prefetch_related results in one query for each field. This is fine + # for now but would be problematic when more settings would be added. + # + # TODO: We should refactor it such that we only make two queries - one + # to fetch all the realm groups and the second to fetch all the groups + # that they point to and then set the setting fields for realm groups + # accordingly in Python. + .prefetch_related( + Prefetch("can_mention_group__direct_members", queryset=UserProfile.objects.only("id")), + Prefetch( + "can_mention_group__direct_subgroups", queryset=NamedUserGroup.objects.only("id") + ), + ) + .filter(realm=realm) + ) + group_dicts: Dict[int, UserGroupDict] = {} for user_group in realm_groups: group_dicts[user_group.id] = dict( @@ -335,7 +366,7 @@ def user_groups_in_realm_serialized(realm: Realm) -> List[UserGroupDict]: members=[], direct_subgroup_ids=[], is_system_group=user_group.is_system_group, - can_mention_group=user_group.can_mention_group_id, + can_mention_group=get_group_setting_value_for_api(user_group.can_mention_group), ) membership = ( diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 95b26ee141..db3c16f8d9 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -18672,15 +18672,21 @@ paths: **Changes**: New in Zulip 5.0 (feature level 93). can_mention_group: - type: integer - description: | - ID of the 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 to 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. description: | A list of `user_group` objects. example: @@ -19878,15 +19884,41 @@ components: **Changes**: New in Zulip 5.0 (feature level 93). can_mention_group: - type: integer - description: | - ID of the 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. + CanMentionGroup: + oneOf: + - type: object + additionalProperties: false + properties: + direct_members: + description: | + The list of user IDs that have permission to + mention the group. + type: array + items: + type: integer + direct_subgroups: + description: | + The list of user group IDs that have permission + to mention the group. + type: array + items: + type: integer + - type: integer Invite: type: object description: | diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index 0e93f74108..9017b08b4a 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -1155,7 +1155,7 @@ class FetchQueriesTest(ZulipTestCase): self.login_user(user) - with self.assert_database_query_count(41): + with self.assert_database_query_count(43): with mock.patch("zerver.lib.events.always_want") as want_mock: fetch_initial_state_data(user) @@ -1180,7 +1180,7 @@ class FetchQueriesTest(ZulipTestCase): realm_linkifiers=0, realm_playgrounds=1, realm_user=3, - realm_user_groups=3, + realm_user_groups=5, realm_user_settings_defaults=1, recent_private_conversations=1, scheduled_messages=1, diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index f0b40d4b1d..6e0f53a0e5 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -257,7 +257,7 @@ class HomeTest(ZulipTestCase): self.client_post("/json/bots", bot_info) # Verify succeeds once logged-in - with self.assert_database_query_count(51): + with self.assert_database_query_count(53): with patch("zerver.lib.cache.cache_set") as cache_mock: result = self._get_home_page(stream="Denmark") self.check_rendered_logged_in_app(result) @@ -562,7 +562,7 @@ class HomeTest(ZulipTestCase): def test_num_queries_for_realm_admin(self) -> None: # Verify number of queries for Realm admin isn't much higher than for normal users. self.login("iago") - with self.assert_database_query_count(51): + with self.assert_database_query_count(53): with patch("zerver.lib.cache.cache_set") as cache_mock: result = self._get_home_page() self.check_rendered_logged_in_app(result) @@ -593,7 +593,7 @@ class HomeTest(ZulipTestCase): self._get_home_page() # Then for the second page load, measure the number of queries. - with self.assert_database_query_count(46): + with self.assert_database_query_count(48): result = self._get_home_page() # Do a sanity check that our new streams were in the payload. diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index ab8bc47405..eba04f9d52 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -22,6 +22,7 @@ from zerver.lib.streams import ensure_stream from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import most_recent_usermessage from zerver.lib.user_groups import ( + AnonymousSettingGroupDict, get_direct_user_groups, get_recursive_group_members, get_recursive_membership_groups, @@ -75,6 +76,7 @@ class UserGroupTestCase(ZulipTestCase): self.assertEqual(user_groups[0]["description"], "Nobody") self.assertEqual(user_groups[0]["members"], []) self.assertEqual(user_groups[0]["direct_subgroup_ids"], []) + self.assertEqual(user_groups[0]["can_mention_group"], user_group.id) owners_system_group = NamedUserGroup.objects.get(name=SystemGroups.OWNERS, realm=realm) membership = UserGroupMembership.objects.filter(user_group=owners_system_group).values_list( @@ -85,6 +87,7 @@ class UserGroupTestCase(ZulipTestCase): self.assertEqual(user_groups[1]["description"], "Owners of this organization") self.assertEqual(set(user_groups[1]["members"]), set(membership)) self.assertEqual(user_groups[1]["direct_subgroup_ids"], []) + self.assertEqual(user_groups[1]["can_mention_group"], user_group.id) admins_system_group = NamedUserGroup.objects.get( name=SystemGroups.ADMINISTRATORS, realm=realm @@ -93,10 +96,39 @@ class UserGroupTestCase(ZulipTestCase): # Check that owners system group is present in "direct_subgroup_ids" self.assertEqual(user_groups[2]["direct_subgroup_ids"], [owners_system_group.id]) + everyone_group = NamedUserGroup.objects.get( + name=SystemGroups.EVERYONE, realm=realm, is_system_group=True + ) self.assertEqual(user_groups[9]["id"], empty_user_group.id) self.assertEqual(user_groups[9]["name"], "newgroup") self.assertEqual(user_groups[9]["description"], "") self.assertEqual(user_groups[9]["members"], []) + self.assertEqual(user_groups[9]["can_mention_group"], everyone_group.id) + + othello = self.example_user("othello") + setting_group = UserGroup.objects.create(realm=realm) + setting_group.direct_members.set([othello]) + setting_group.direct_subgroups.set([admins_system_group]) + + new_user_group = check_add_user_group( + realm, + "newgroup2", + [othello], + group_settings_map={"can_mention_group": setting_group}, + acting_user=None, + ) + user_groups = user_groups_in_realm_serialized(realm) + self.assertEqual(user_groups[10]["id"], new_user_group.id) + self.assertEqual(user_groups[10]["name"], "newgroup2") + self.assertEqual(user_groups[10]["description"], "") + self.assertEqual(user_groups[10]["members"], [othello.id]) + self.assertEqual( + user_groups[10]["can_mention_group"], + AnonymousSettingGroupDict( + direct_members=[othello.id], + direct_subgroups=[admins_system_group.id], + ), + ) def test_get_direct_user_groups(self) -> None: othello = self.example_user("othello")