Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

user_groups: Add add_can_members_group to user group. 

The default value for this field that we wanted to have was that group
itlself. But we are deferring that to later in order to reach the point
of switching over to the groups system sooner. Till then, we will use
`group_creator` as the default. See
https://chat.zulip.org/#narrow/stream/101-design/topic/Group.20add.20members.20dropdown/near/1952904
for more details.

For migration plan details, see
https://chat.zulip.org/#narrow/stream/101-design/topic/Group.20add.20members.20dropdown/near/1952902

The increase in query count from 7 to 9 in the query count test for
creating a user group is because of group_creator being the default for
the new field.
This commit is contained in:
Shubham Padia 2024-10-07 17:00:15 +00:00 committed by Tim Abbott
parent 553409c1ca
commit b305ca14dd
15 changed files with 334 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
api_docs/changelog.md
View File

@ -20,6 +20,18 @@ format used by the Zulip server that they are interacting with.
## Changes in Zulip 10.0
**Feature level 305**
* [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events),
[`GET /user_groups`](/api/get-user-groups): Add `can_add_members_group` to
user group objects.
* [`POST /user_groups/create`](/api/create-user-group): Added `can_add_members_group`
parameter to support setting the user group which can add members to the user
group.
* [`PATCH /user_groups/{user_group_id}`](/api/update-user-group): Added
`can_add_members_group` parameter to support changing the user group which
can add members to the specified user group.
**Feature level 304**
* [`GET /export/realm`](/api/get-realm-exports),

1
zerver/actions/user_groups.py
View File

@ -188,6 +188,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_add_members_group=get_group_setting_value_for_api(user_group.can_add_members_group),
can_join_group=get_group_setting_value_for_api(user_group.can_join_group),
can_manage_group=get_group_setting_value_for_api(user_group.can_manage_group),
can_mention_group=get_group_setting_value_for_api(user_group.can_mention_group),

2
zerver/lib/event_schema.py
View File

@ -1848,6 +1848,7 @@ group_type = DictType(
("direct_subgroup_ids", ListType(int)),
("description", str),
("is_system_group", bool),
("can_add_members_group", group_setting_type),
("can_join_group", group_setting_type),
("can_manage_group", group_setting_type),
("can_mention_group", group_setting_type),
@ -1898,6 +1899,7 @@ user_group_data_type = DictType(
optional_keys=[
("name", str),
("description", str),
("can_add_members_group", group_setting_type),
("can_join_group", group_setting_type),
("can_manage_group", group_setting_type),
("can_mention_group", group_setting_type),

3
zerver/lib/import_realm.py
View File

@ -757,6 +757,7 @@ def bulk_import_named_user_groups(data: TableData) -> None:
group["name"],
group["description"],
group["is_system_group"],
group["can_add_members_group_id"],
group["can_join_group_id"],
group["can_manage_group_id"],
group["can_mention_group_id"],
@ -768,7 +769,7 @@ def bulk_import_named_user_groups(data: TableData) -> None:
query = SQL(
"""
INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated, date_created)
INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_add_members_group_id, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated, date_created)
VALUES %s
"""
)

14
zerver/lib/user_groups.py
View File

@ -55,6 +55,7 @@ class UserGroupDict(TypedDict):
creator_id: int | None
date_created: int | None
is_system_group: bool
can_add_members_group: int | AnonymousSettingGroupDict
can_join_group: int | AnonymousSettingGroupDict
can_manage_group: int | AnonymousSettingGroupDict
can_mention_group: int | AnonymousSettingGroupDict
@ -554,6 +555,8 @@ def user_groups_in_realm_serialized(
UserGroup and UserGroupMembership that we need.
"""
realm_groups = NamedUserGroup.objects.select_related(
"can_add_members_group",
"can_add_members_group__named_user_group",
"can_join_group",
"can_join_group__named_user_group",
"can_manage_group",
@ -610,6 +613,9 @@ def user_groups_in_realm_serialized(
members=direct_member_ids,
direct_subgroup_ids=direct_subgroup_ids,
is_system_group=user_group.is_system_group,
can_add_members_group=get_setting_value_for_user_group_object(
user_group.can_add_members_group, group_members, group_subgroups
),
can_join_group=get_setting_value_for_user_group_object(
user_group.can_join_group, group_members, group_subgroups
),
@ -834,7 +840,7 @@ def bulk_create_system_user_groups(groups: list[dict[str, str]], realm: Realm) -
user_group_ids = [id for (id,) in cursor.fetchall()]
rows = [
SQL("({},{},{},{},{},{},{},{},{})").format(
SQL("({},{},{},{},{},{},{},{},{},{})").format(
Literal(user_group_ids[idx]),
Literal(realm.id),
Literal(group["name"]),
@ -843,13 +849,14 @@ def bulk_create_system_user_groups(groups: list[dict[str, str]], realm: Realm) -
Literal(initial_group_setting_value),
Literal(initial_group_setting_value),
Literal(initial_group_setting_value),
Literal(initial_group_setting_value),
Literal(False),
)
for idx, group in enumerate(groups)
]
query = SQL(
"""
INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated)
INSERT INTO zerver_namedusergroup (usergroup_ptr_id, realm_id, name, description, is_system_group, can_add_members_group_id, can_join_group_id, can_manage_group_id, can_mention_group_id, deactivated)
VALUES {rows}
"""
).format(rows=SQL(", ").join(rows))
@ -931,7 +938,8 @@ def create_system_user_groups_for_realm(realm: Realm) -> dict[int, NamedUserGrou
user_group = set_defaults_for_group_settings(group, {}, system_groups_name_dict)
groups_with_updated_settings.append(user_group)
NamedUserGroup.objects.bulk_update(
groups_with_updated_settings, ["can_join_group", "can_manage_group", "can_mention_group"]
groups_with_updated_settings,
["can_add_members_group", "can_join_group", "can_manage_group", "can_mention_group"],
)
subgroup_objects: list[GroupGroupMembership] = []

23
zerver/migrations/0599_namedusergroup_add_can_add_members_group.py Normal file
View File

@ -0,0 +1,23 @@
# Generated by Django 5.0.9 on 2024-10-07 07:23
import django.db.models.deletion
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
("zerver", "0598_alter_namedusergroup_can_join_group"),
]
operations = [
migrations.AddField(
model_name="namedusergroup",
name="can_add_members_group",
field=models.ForeignKey(
null=True,
on_delete=django.db.models.deletion.RESTRICT,
related_name="+",
to="zerver.usergroup",
),
),
]

144
zerver/migrations/0600_set_default_for_can_add_members_group.py Normal file
View File

@ -0,0 +1,144 @@
# Generated by Django 5.0.9 on 2024-10-07 17:05
from django.db import migrations, transaction
from django.db.backends.base.schema import BaseDatabaseSchemaEditor
from django.db.migrations.state import StateApps
from django.db.models import F, Max, Min, OuterRef
def set_default_value_for_can_add_members_group(
apps: StateApps, schema_editor: BaseDatabaseSchemaEditor
) -> None:
NamedUserGroup = apps.get_model("zerver", "NamedUserGroup")
Realm = apps.get_model("zerver", "Realm")
BATCH_SIZE = 1000
max_id = NamedUserGroup.objects.filter(can_add_members_group=None).aggregate(Max("id"))[
"id__max"
]
if max_id is None:
# Do nothing if there are no user groups on the server.
return
lower_bound = NamedUserGroup.objects.filter(can_add_members_group=None).aggregate(Min("id"))[
"id__min"
]
owners_system_group_ids = NamedUserGroup.objects.filter(name="role:owners").values_list(
"id", flat=True
)
realm_ids_allowing_owners_to_manage_all_groups = Realm.objects.filter(
can_manage_all_groups__in=owners_system_group_ids
).values_list("id", flat=True)
admins_system_group_ids = NamedUserGroup.objects.filter(name="role:administrators").values_list(
"id", flat=True
)
realm_ids_allowing_admins_to_manage_all_groups = Realm.objects.filter(
can_manage_all_groups__in=admins_system_group_ids
).values_list("id", flat=True)
moderators_system_group_ids = NamedUserGroup.objects.filter(name="role:moderators").values_list(
"id", flat=True
)
realm_ids_allowing_moderators_to_manage_all_groups = Realm.objects.filter(
can_manage_all_groups__in=moderators_system_group_ids
).values_list("id", flat=True)
members_system_group_ids = NamedUserGroup.objects.filter(name="role:members").values_list(
"id", flat=True
)
realm_ids_allowing_members_to_manage_all_groups = Realm.objects.filter(
can_manage_all_groups__in=members_system_group_ids
).values_list("id", flat=True)
while lower_bound <= max_id:
upper_bound = lower_bound + BATCH_SIZE - 1
print(f"Processing batch {lower_bound} to {upper_bound} for NamedUserGroup")
with transaction.atomic():
# Initialize to the corresponding system group, i.e. just
# copy from can_manage_all_groups. Previously, either
# owners/administrators/moderators or group members were
# allowed to manage the group if they were part of
# can_manage_all_groups. Non members were not allowed to
# manage the group unless for the roles mentioned above.
# That is why copying from can_manage_all_groups should
# give the identical set of users permission.
NamedUserGroup.objects.filter(
id__range=(lower_bound, upper_bound),
can_add_members_group=None,
realm_id__in=realm_ids_allowing_owners_to_manage_all_groups,
).update(
can_add_members_group=NamedUserGroup.objects.filter(
name="role:owners",
realm_for_sharding=OuterRef("realm_for_sharding"),
is_system_group=True,
).values("pk")
)
NamedUserGroup.objects.filter(
id__range=(lower_bound, upper_bound),
can_add_members_group=None,
realm_id__in=realm_ids_allowing_admins_to_manage_all_groups,
).update(
can_add_members_group=NamedUserGroup.objects.filter(
name="role:administrators",
realm_for_sharding=OuterRef("realm_for_sharding"),
is_system_group=True,
).values("pk")
)
NamedUserGroup.objects.filter(
id__range=(lower_bound, upper_bound),
can_add_members_group=None,
realm_id__in=realm_ids_allowing_moderators_to_manage_all_groups,
).update(
can_add_members_group=NamedUserGroup.objects.filter(
name="role:moderators",
realm_for_sharding=OuterRef("realm_for_sharding"),
is_system_group=True,
).values("pk")
)
# Initialize can_add_members_group to the group itself.
# This should give the identical set of users permission,
# since users could only exercise the permission previously
# if they were a member of the group.
NamedUserGroup.objects.filter(
id__range=(lower_bound, upper_bound),
can_add_members_group=None,
realm_id__in=realm_ids_allowing_members_to_manage_all_groups,
).update(can_add_members_group=F("id"))
# For the remaining group options, there is no direct
# translation to can_add_members_group. We'll pick the
# closest safe choice i.e. moderators for this.
NamedUserGroup.objects.filter(
id__range=(lower_bound, upper_bound),
can_add_members_group=None,
).update(
can_add_members_group=NamedUserGroup.objects.filter(
name="role:moderators",
realm_for_sharding=OuterRef("realm_for_sharding"),
is_system_group=True,
).values("pk")
)
lower_bound += BATCH_SIZE
class Migration(migrations.Migration):
atomic = False
dependencies = [
("zerver", "0599_namedusergroup_add_can_add_members_group"),
]
operations = [
migrations.RunPython(
set_default_value_for_can_add_members_group,
elidable=True,
reverse_code=migrations.RunPython.noop,
)
]

22
zerver/migrations/0601_alter_namedusergroup_can_add_members_group.py Normal file
View File

@ -0,0 +1,22 @@
# Generated by Django 5.0.9 on 2024-10-09 08:25
import django.db.models.deletion
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
("zerver", "0600_set_default_for_can_add_members_group"),
]
operations = [
migrations.AlterField(
model_name="namedusergroup",
name="can_add_members_group",
field=models.ForeignKey(
on_delete=django.db.models.deletion.RESTRICT,
related_name="+",
to="zerver.usergroup",
),
),
]

13
zerver/models/groups.py
View File

@ -59,6 +59,9 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang
)
is_system_group = models.BooleanField(default=False, db_column="is_system_group")
can_add_members_group = models.ForeignKey(
UserGroup, on_delete=models.RESTRICT, related_name="+"
)
can_join_group = models.ForeignKey(UserGroup, on_delete=models.RESTRICT, related_name="+")
can_manage_group = models.ForeignKey(UserGroup, on_delete=models.RESTRICT, related_name="+")
can_mention_group = models.ForeignKey(
@ -95,6 +98,16 @@ class NamedUserGroup(UserGroup): # type: ignore[django-manager-missing] # djang
}
GROUP_PERMISSION_SETTINGS = {
"can_add_members_group": GroupPermissionSetting(
require_system_group=False,
allow_internet_group=False,
allow_owners_group=True,
allow_nobody_group=True,
allow_everyone_group=False,
default_group_name="group_creator",
default_for_system_groups=SystemGroups.NOBODY,
id_field_name="can_add_members_group_id",
),
"can_join_group": GroupPermissionSetting(
require_system_group=False,
allow_internet_group=False,

2
zerver/openapi/curl_param_value_generators.py
View File

@ -264,6 +264,7 @@ def get_temp_user_group_id() -> dict[str, object]:
user_group, _ = NamedUserGroup.objects.get_or_create(
name="temp",
realm=get_realm("zulip"),
can_add_members_group_id=11,
can_join_group_id=11,
can_manage_group_id=11,
can_mention_group_id=11,
@ -279,6 +280,7 @@ def get_temp_user_group_id_for_deactivation() -> dict[str, object]:
user_group, _ = NamedUserGroup.objects.get_or_create(
name="temp-deactivation",
realm=get_realm("zulip"),
can_add_members_group_id=11,
can_join_group_id=11,
can_manage_group_id=11,
can_mention_group_id=11,

95
zerver/openapi/zulip.yaml
View File

@ -3175,6 +3175,7 @@ paths:
"description": "Backend team",
"id": 2,
"is_system_group": false,
"can_add_members_group": 16,
"can_join_group": 16,
"can_manage_group": 16,
"can_mention_group": 11,
@ -3232,6 +3233,20 @@ paths:
description: |
The new description of the group. Only present if the description
changed.
can_add_members_group:
allOf:
- $ref: "#/components/schemas/GroupSettingValue"
- description: |
A [group-setting value][setting-values] defining the set of users who
have permission to add members to this group. Only present if this user
group permission setting changed.
**Changes**: New in Zulip 10.0 (feature level 305). Previously, this
permission was controlled by the `can_manage_group` setting.
Will be one of the following:
[setting-values]: /api/group-setting-values
can_join_group:
allOf:
- $ref: "#/components/schemas/GroupSettingValue"
@ -20248,6 +20263,19 @@ paths:
items:
type: integer
example: [1, 2, 3, 4]
can_add_members_group:
allOf:
- description: |
A [group-setting value][setting-values] defining the set of users who
have permission to add members to this user group.
**Changes**: New in Zulip 10.0 (feature level 305). Previously, this
permission was controlled by the `can_manage_group` setting.
[setting-values]: /api/group-setting-values
[system-groups]: /api/group-setting-values#system-groups
- $ref: "#/components/schemas/GroupSettingValue"
example: 11
can_join_group:
allOf:
- description: |
@ -20306,6 +20334,8 @@ paths:
encoding:
members:
contentType: application/json
can_add_members_group:
contentType: application/json
can_join_group:
contentType: application/json
can_manage_group:
@ -20442,6 +20472,38 @@ paths:
a required field.
type: string
example: The marketing team.
can_add_members_group:
description: |
The set of users who have permission to add members to this user group
expressed as an [update to a group-setting value][update-group-setting].
**Changes**: New in Zulip 10.0 (feature level 305). Previously, this
permission was controlled by the `can_manage_group` setting.
[update-group-setting]: /api/group-setting-values#updating-group-setting-values
[system-groups]: /api/group-setting-values#system-groups
type: object
additionalProperties: false
properties:
new:
allOf:
- description: |
The new [group-setting value](/api/group-setting-values) for the set of users
who would have the permission to add members to the group.
- $ref: "#/components/schemas/GroupSettingValue"
old:
allOf:
- description: |
The expected current [group-setting value](/api/group-setting-values)
for the set of users who have the permission to add members to the group.
- $ref: "#/components/schemas/GroupSettingValue"
required:
- new
example:
{
"new": {"direct_members": [10], "direct_subgroups": [11]},
"old": 11,
}
can_join_group:
description: |
The set of users who have permission to join this user group
@ -20557,6 +20619,8 @@ paths:
"old": 11,
}
encoding:
can_add_members_group:
contentType: application/json
can_join_group:
contentType: application/json
can_manage_group:
@ -20706,6 +20770,19 @@ paths:
modified by users.
**Changes**: New in Zulip 5.0 (feature level 93).
can_add_members_group:
allOf:
- $ref: "#/components/schemas/GroupSettingValue"
- description: |
A [group-setting value][setting-values] defining the set of users who
have permission to add members to this user group.
**Changes**: New in Zulip 10.0 (feature level 305). Previously, this
permission was controlled by the `can_manage_group` setting.
Will be one of the following:
[setting-values]: /api/group-setting-values
can_join_group:
allOf:
- $ref: "#/components/schemas/GroupSettingValue"
@ -20777,6 +20854,7 @@ paths:
"members": [1],
"direct_subgroup_ids": [],
"is_system_group": true,
"can_add_members_group": 16,
"can_join_group": 16,
"can_manage_group": 16,
"can_mention_group": 11,
@ -20790,6 +20868,7 @@ paths:
"members": [2],
"direct_subgroup_ids": [1],
"is_system_group": true,
"can_add_members_group": 17,
"can_join_group": 17,
"can_manage_group": 17,
"can_mention_group": 12,
@ -20803,6 +20882,7 @@ paths:
"members": [3, 4],
"direct_subgroup_ids": [],
"is_system_group": false,
"can_add_members_group": 20,
"can_join_group": 20,
"can_manage_group": 20,
"can_mention_group": 13,
@ -22069,6 +22149,19 @@ components:
directly modified by users.
**Changes**: New in Zulip 5.0 (feature level 93).
can_add_members_group:
allOf:
- $ref: "#/components/schemas/GroupSettingValue"
- description: |
A [group-setting value][setting-values] defining the set of users who
have permission to add members to this user group.
**Changes**: New in Zulip 10.0 (feature level 305). Previously, this
permission was controlled by the `can_manage_group` setting.
Will be one of the following:
[setting-values]: /api/group-setting-values
can_join_group:
allOf:
- $ref: "#/components/schemas/GroupSettingValue"
@ -24222,7 +24315,7 @@ components:
The ID of the target user group.
schema:
type: integer
example: 34
example: 35
required: true
QueueId:
name: queue_id

2
zerver/tests/test_import_export.py
View File

@ -499,7 +499,7 @@ class RealmImportExportTest(ExportFile):
self.assertEqual(exported_realm_user_default[0]["default_language"], "de")
exported_usergroups = data["zerver_usergroup"]
self.assert_length(exported_usergroups, 10)
self.assert_length(exported_usergroups, 11)
self.assertFalse("direct_members" in exported_usergroups[2])
self.assertFalse("direct_subgroups" in exported_usergroups[2])

2
zerver/tests/test_user_groups.py
View File

@ -1615,7 +1615,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
for i in range(50)
]
with self.assert_database_query_count(7):
with self.assert_database_query_count(9):
user_group = create_user_group_in_database(
name="support",
members=[hamlet, cordelia, *original_users],

2
zerver/transaction_tests/test_user_groups.py
View File

@ -78,8 +78,10 @@ class UserGroupRaceConditionTestCase(ZulipTransactionTestCase):
# can_manage_group in this test, deleting that group
# should be reconsidered.
can_manage_group = group.can_manage_group
can_add_members_group = group.can_add_members_group
group.delete()
can_manage_group.delete()
can_add_members_group.delete()
transaction.on_commit(self.created_user_groups.clear)
super().tearDown()

4
zerver/views/user_groups.py
View File

@ -57,6 +57,7 @@ def add_user_group(
name: str,
members: Json[list[int]],
description: str,
can_add_members_group: Json[int | AnonymousSettingGroupDict] | None = None,
can_join_group: Json[int | AnonymousSettingGroupDict] | None = None,
can_manage_group: Json[int | AnonymousSettingGroupDict] | None = None,
can_mention_group: Json[int | AnonymousSettingGroupDict] | None = None,
@ -117,6 +118,7 @@ def edit_user_group(
user_group_id: PathOnly[int],
name: str | None = None,
description: str | None = None,
can_add_members_group: Json[GroupSettingChangeRequest] | None = None,
can_join_group: Json[GroupSettingChangeRequest] | None = None,
can_manage_group: Json[GroupSettingChangeRequest] | None = None,
can_mention_group: Json[GroupSettingChangeRequest] | None = None,
@ -124,6 +126,7 @@ def edit_user_group(
if (
name is None
and description is None
and can_add_members_group is None
and can_join_group is None
and can_manage_group is None
and can_mention_group is None
@ -136,6 +139,7 @@ def edit_user_group(
if user_group.deactivated and (
description is not None
or can_add_members_group is not None
or can_join_group is not None
or can_mention_group is not None
or can_manage_group is not None