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

user_groups: Do not allow deleting user groups.

This commit is contained in:
Sahil Batra 2024-09-13 13:14:45 +05:30 committed by Tim Abbott
parent 6a739e263f
commit 5f3a8334be
14 changed files with 14 additions and 324 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
api_docs/changelog.md
View File

@ -38,6 +38,8 @@ format used by the Zulip server that they are interacting with.
is updated.
* [`GET /user_groups`](/api/get-user-groups): Renamed `allow_deactivated`
parameter to `include_deactivated_groups`.
* `DELETE /user_groups/{user_group_id}`: Removed support for user group
deletion as we now support deactivating user groups.
**Feature level 293**

1
api_docs/include/rest-endpoints.md
View File

@ -78,7 +78,6 @@
* [Get user groups](/api/get-user-groups)
* [Create a user group](/api/create-user-group)
* [Update a user group](/api/update-user-group)
* [Delete a user group](/api/remove-user-group)
* [Deactivate a user group](/api/deactivate-user-group)
* [Update user group members](/api/update-user-group-members)
* [Update subgroups of a user group](/api/update-user-group-subgroups)

2
version.py
View File

@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3"
# new level means in api_docs/changelog.md, as well as "**Changes**"
# entries in the endpoint's documentation in `zulip.yaml`.
API_FEATURE_LEVEL = 293 # Last bumped for `allow_private_data_export` setting.
API_FEATURE_LEVEL = 294 # Last bumped for `include_daectivated_groups` client capability.
# 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

4
web/src/server_events_dispatch.js
View File

@ -925,10 +925,6 @@ export function dispatch_normal_event(event) {
}
break;
}
case "remove":
user_groups.remove(user_groups.get_user_group_from_id(event.group_id));
user_group_edit.handle_deleted_group(event.group_id);
break;
case "add_members":
user_groups.add_members(event.group_id, event.user_ids);
user_group_edit.handle_member_edit_event(event.group_id, event.user_ids);

19
web/tests/dispatch.test.js
View File

@ -212,25 +212,6 @@ run_test("user groups", ({override}) => {
assert_same(args.group, event.group);
}
event = event_fixtures.user_group__remove;
{
const stub = make_stub();
override(user_groups, "get_user_group_from_id", stub.f);
override(user_groups, "remove", noop);
const user_group_edit_stub = make_stub();
override(user_group_edit, "handle_deleted_group", user_group_edit_stub.f);
dispatch(event);
assert.equal(stub.num_calls, 1);
assert.equal(user_group_edit_stub.num_calls, 1);
let args = stub.get_args("group_id");
assert_same(args.group_id, event.group_id);
args = user_group_edit_stub.get_args("group_id");
assert_same(args.group_id, event.group_id);
}
event = event_fixtures.user_group__add_members;
{
const stub = make_stub();

6
web/tests/lib/events.js
View File

@ -835,12 +835,6 @@ exports.fixtures = {
direct_subgroup_ids: [3],
},
user_group__remove: {
type: "user_group",
op: "remove",
group_id: 1,
},
user_group__remove_members: {
type: "user_group",
op: "remove_members",

11
zerver/actions/user_groups.py
View File

@ -440,17 +440,6 @@ def remove_subgroups_from_user_group(
do_send_subgroups_update_event("remove_subgroups", user_group, subgroup_ids)
def do_send_delete_user_group_event(realm: Realm, user_group_id: int, realm_id: int) -> None:
event = dict(type="user_group", op="remove", group_id=user_group_id)
send_event_on_commit(realm, event, active_user_ids(realm_id))
def check_delete_user_group(user_group: NamedUserGroup, *, acting_user: UserProfile) -> None:
user_group_id = user_group.id
user_group.delete()
do_send_delete_user_group_event(acting_user.realm, user_group_id, acting_user.realm.id)
@transaction.atomic(savepoint=False)
def do_deactivate_user_group(
user_group: NamedUserGroup, *, acting_user: UserProfile | None

4
zerver/openapi/curl_param_value_generators.py
View File

@ -259,9 +259,7 @@ def create_user_group_data() -> dict[str, object]:
}
@openapi_param_value_generator(
["/user_groups/{user_group_id}:patch", "/user_groups/{user_group_id}:delete"]
)
@openapi_param_value_generator(["/user_groups/{user_group_id}:patch"])
def get_temp_user_group_id() -> dict[str, object]:
user_group, _ = NamedUserGroup.objects.get_or_create(
name="temp",

22
zerver/openapi/python_examples.py
View File

@ -813,8 +813,8 @@ def get_user_groups(client: Client) -> int:
validate_against_openapi_schema(result, "/user_groups", "get", "200")
[hamlet_user_group] = (u for u in result["user_groups"] if u["name"] == "hamletcharacters")
assert hamlet_user_group["description"] == "Characters of Hamlet"
[marketing_user_group] = (u for u in result["user_groups"] if u["name"] == "marketing")
return marketing_user_group["id"]
[leadership_user_group] = (u for u in result["user_groups"] if u["name"] == "leadership")
return leadership_user_group["id"]
def test_user_not_authorized_error(nonadmin_client: Client) -> None:
@ -1628,8 +1628,8 @@ def create_user_group(client: Client) -> None:
ensure_users(user_ids, ["aaron", "zoe", "cordelia", "hamlet"])
# {code_example|start}
request = {
"name": "marketing",
"description": "The marketing team.",
"name": "leadership",
"description": "The leadership team.",
"members": user_ids,
}
result = client.create_user_group(request)
@ -1643,8 +1643,8 @@ def update_user_group(client: Client, user_group_id: int) -> None:
# {code_example|start}
request = {
"group_id": user_group_id,
"name": "marketing",
"description": "The marketing team.",
"name": "leadership",
"description": "The leadership team.",
}
result = client.update_user_group(request)
# {code_example|end}
@ -1652,15 +1652,6 @@ def update_user_group(client: Client, user_group_id: int) -> None:
validate_against_openapi_schema(result, "/user_groups/{user_group_id}", "patch", "200")
@openapi_test_function("/user_groups/{user_group_id}:delete")
def remove_user_group(client: Client, user_group_id: int) -> None:
# {code_example|start}
result = client.remove_user_group(user_group_id)
# {code_example|end}
assert_success_response(result)
validate_against_openapi_schema(result, "/user_groups/{user_group_id}", "delete", "200")
@openapi_test_function("/user_groups/{user_group_id}/members:post")
def update_user_group_members(client: Client, user_group_id: int) -> None:
ensure_users([8, 10, 11], ["cordelia", "hamlet", "iago"])
@ -1761,7 +1752,6 @@ def test_users(client: Client, owner_client: Client) -> None:
user_group_id = get_user_groups(client)
update_user_group(client, user_group_id)
update_user_group_members(client, user_group_id)
remove_user_group(client, user_group_id)
get_alert_words(client)
add_alert_words(client)
remove_alert_words(client)

37
zerver/openapi/zulip.yaml
View File

@ -3417,14 +3417,11 @@ paths:
- type: object
additionalProperties: false
description: |
Event sent to all users when a user group has been deleted.
This event is also sent when a user group is deactivated
but only to clients with `include_deactivated_groups` client
capability set to `false`.
Event sent when a user group is deactivated but only to clients
with `include_deactivated_groups` client capability set to `false`.
**Changes**: Prior to Zulip 10.0 (feature level 294), this
event was only sent when a user group was deleted.
event was sent when a user group was deleted.
properties:
id:
$ref: "#/components/schemas/EventIdSchema"
@ -19995,34 +19992,6 @@ paths:
}
description: |
An example JSON response when the user group ID is invalid:
delete:
operationId: remove-user-group
summary: Delete a user group
tags: ["users"]
description: |
Delete a [user group](/help/user-groups).
parameters:
- $ref: "#/components/parameters/UserGroupId"
responses:
"200":
$ref: "#/components/responses/SimpleSuccess"
"400":
description: Bad request.
content:
application/json:
schema:
allOf:
- $ref: "#/components/schemas/CodedError"
- example:
{
"code": "BAD_REQUEST",
"msg": "Invalid user group",
"result": "error",
}
description: |
An example JSON error response for an invalid user group ID:
/user_groups:
get:
operationId: get-user-groups

10
zerver/tests/test_events.py
View File

@ -109,7 +109,6 @@ from zerver.actions.user_groups import (
bulk_add_members_to_user_groups,
bulk_remove_members_from_user_groups,
check_add_user_group,
check_delete_user_group,
do_change_user_group_permission_setting,
do_deactivate_user_group,
do_update_user_group_description,
@ -1942,15 +1941,6 @@ class NormalActionsTest(BaseAction):
do_update_user_group_name(api_design, "api-deisgn", acting_user=None)
check_user_group_update("events[0]", events[0], "name")
# Reactivate the group to test "remove" event.
backend.deactivated = False
backend.save()
# Test remove event
with self.verify_action() as events:
check_delete_user_group(backend, acting_user=othello)
check_user_group_remove("events[0]", events[0])
def test_default_stream_groups_events(self) -> None:
streams = [
get_stream(stream_name, self.user_profile.realm)

200
zerver/tests/test_user_groups.py
View File

@ -4,7 +4,6 @@ from unittest import mock
import orjson
import time_machine
from django.db import transaction
from django.utils.timezone import now as timezone_now
from zerver.actions.create_realm import do_create_realm
@ -1250,38 +1249,6 @@ class UserGroupAPITestCase(UserGroupTestCase):
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "Invalid user group")
def test_user_group_delete(self) -> None:
hamlet = self.example_user("hamlet")
self.login("hamlet")
params = {
"name": "support",
"members": orjson.dumps([hamlet.id]).decode(),
"description": "Support team",
}
self.client_post("/json/user_groups/create", info=params)
user_group = NamedUserGroup.objects.get(name="support")
# Test success
self.assertEqual(NamedUserGroup.objects.filter(realm=hamlet.realm).count(), 10)
self.assertEqual(UserGroupMembership.objects.count(), 45)
self.assertTrue(NamedUserGroup.objects.filter(id=user_group.id).exists())
result = self.client_delete(f"/json/user_groups/{user_group.id}")
self.assert_json_success(result)
self.assertEqual(NamedUserGroup.objects.filter(realm=hamlet.realm).count(), 9)
self.assertEqual(UserGroupMembership.objects.count(), 44)
self.assertFalse(NamedUserGroup.objects.filter(id=user_group.id).exists())
# Test when invalid user group is supplied; transaction needed for
# error handling
with transaction.atomic():
result = self.client_delete("/json/user_groups/1111")
self.assert_json_error(result, "Invalid user group")
lear_realm = get_realm("lear")
lear_test_group = check_add_user_group(
lear_realm, "test", [self.lear_user("cordelia")], acting_user=None
)
result = self.client_delete(f"/json/user_groups/{lear_test_group.id}")
self.assert_json_error(result, "Invalid user group")
def test_user_group_deactivation(self) -> None:
support_group = self.create_user_group_for_test("support")
leadership_group = self.create_user_group_for_test("leadership")
@ -1839,173 +1806,6 @@ class UserGroupAPITestCase(UserGroupTestCase):
othello.save()
check_create_user_group("othello")
def test_realm_level_policy_for_deleting_user_group(self) -> None:
hamlet = self.example_user("hamlet")
realm = hamlet.realm
def check_delete_user_group(acting_user: str, error_msg: str | None = None) -> None:
user_group = NamedUserGroup.objects.get(name="support")
with transaction.atomic():
result = self.api_delete(
self.example_user(acting_user), f"/api/v1/user_groups/{user_group.id}"
)
if error_msg is None:
self.assert_json_success(result)
self.assert_length(NamedUserGroup.objects.filter(realm=realm), 9)
else:
self.assert_json_error(result, error_msg)
self.create_user_group_for_test("support")
# Check only admins are allowed to delete user group. Admins are allowed even if
# they are not a member of the group.
do_set_realm_property(
realm,
"user_group_edit_policy",
CommonPolicyEnum.ADMINS_ONLY,
acting_user=None,
)
check_delete_user_group("shiva", "Insufficient permission")
check_delete_user_group("iago")
self.create_user_group_for_test("support")
# Check moderators are allowed to delete user group but not members. Moderators are
# allowed even if they are not a member of the group.
do_set_realm_property(
realm,
"user_group_edit_policy",
CommonPolicyEnum.MODERATORS_ONLY,
acting_user=None,
)
check_delete_user_group("hamlet", "Insufficient permission")
check_delete_user_group("shiva")
self.create_user_group_for_test("support")
# Check only members are allowed to delete the user group and they are allowed to create
# a user group only if they are a member of that group.
do_set_realm_property(
realm,
"user_group_edit_policy",
CommonPolicyEnum.MEMBERS_ONLY,
acting_user=None,
)
check_delete_user_group("polonius", "Not allowed for guest users")
check_delete_user_group("cordelia", "Insufficient permission")
check_delete_user_group("othello")
self.create_user_group_for_test("support")
# Check only full members are allowed to delete the user group and they are allowed to delete
# a user group only if they are a member of that group.
do_set_realm_property(
realm,
"user_group_edit_policy",
CommonPolicyEnum.FULL_MEMBERS_ONLY,
acting_user=None,
)
do_set_realm_property(realm, "waiting_period_threshold", 10, acting_user=None)
cordelia = self.example_user("cordelia")
cordelia.date_joined = timezone_now() - timedelta(days=11)
cordelia.save()
othello = self.example_user("othello")
othello.date_joined = timezone_now() - timedelta(days=9)
othello.save()
check_delete_user_group("cordelia", "Insufficient permission")
check_delete_user_group("othello", "Insufficient permission")
othello.date_joined = timezone_now() - timedelta(days=11)
othello.save()
check_delete_user_group("othello")
def test_group_level_setting_for_deleting_user_groups(self) -> None:
othello = self.example_user("othello")
realm = othello.realm
hamlet = self.example_user("hamlet")
leadership_group = check_add_user_group(
get_realm("zulip"), "leadership", [hamlet], acting_user=hamlet
)
def check_delete_user_group(acting_user: str, error_msg: str | None = None) -> None:
user_group = NamedUserGroup.objects.get(name="support")
with transaction.atomic():
result = self.api_delete(
self.example_user(acting_user), f"/api/v1/user_groups/{user_group.id}"
)
if error_msg is None:
self.assert_json_success(result)
else:
self.assert_json_error(result, error_msg)
do_set_realm_property(
realm,
"user_group_edit_policy",
Realm.POLICY_NOBODY,
acting_user=None,
)
support_group = check_add_user_group(
get_realm("zulip"), "support", [othello], acting_user=None
)
# Default value of can_manage_group is "Nobody" system group.
check_delete_user_group("desdemona", "Insufficient permission")
check_delete_user_group("othello", "Insufficient permission")
system_group_dict = get_role_based_system_groups_dict(realm)
owners_group = system_group_dict[SystemGroups.OWNERS]
do_change_user_group_permission_setting(
support_group, "can_manage_group", owners_group, acting_user=None
)
check_delete_user_group("iago", "Insufficient permission")
check_delete_user_group("desdemona")
check_add_user_group(
get_realm("zulip"),
"support",
[othello],
"",
{"can_manage_group": system_group_dict[SystemGroups.MEMBERS]},
acting_user=None,
)
check_delete_user_group("polonius", "Not allowed for guest users")
check_delete_user_group("cordelia")
setting_group = self.create_or_update_anonymous_group_for_setting(
[self.example_user("cordelia")], [leadership_group, owners_group]
)
check_add_user_group(
get_realm("zulip"),
"support",
[othello],
"",
{"can_manage_group": setting_group},
acting_user=None,
)
check_delete_user_group("iago", "Insufficient permission")
check_delete_user_group("hamlet")
check_add_user_group(
get_realm("zulip"),
"support",
[othello],
"",
{"can_manage_group": setting_group},
acting_user=None,
)
check_delete_user_group("cordelia")
check_add_user_group(
get_realm("zulip"),
"support",
[othello],
"",
{"can_manage_group": setting_group},
acting_user=None,
)
check_delete_user_group("desdemona")
def test_realm_level_setting_for_updating_user_groups(self) -> None:
othello = self.example_user("othello")
self.login("othello")

17
zerver/views/user_groups.py
View File

@ -11,7 +11,6 @@ from zerver.actions.user_groups import (
bulk_add_members_to_user_groups,
bulk_remove_members_from_user_groups,
check_add_user_group,
check_delete_user_group,
do_change_user_group_permission_setting,
do_deactivate_user_group,
do_update_user_group_description,
@ -183,22 +182,6 @@ def edit_user_group(
return json_success(request)
@require_member_or_admin
@typed_endpoint
def delete_user_group(
request: HttpRequest,
user_profile: UserProfile,
*,
user_group_id: PathOnly[Json[int]],
) -> HttpResponse:
# For deletion, the user group's recursive subgroups and the user group itself are locked.
with lock_subgroups_with_respect_to_supergroup(
[user_group_id], user_group_id, acting_user=user_profile
) as context:
check_delete_user_group(context.supergroup, acting_user=user_profile)
return json_success(request)
@typed_endpoint
@transaction.atomic
def deactivate_user_group(

3
zproject/urls.py
View File

@ -189,7 +189,6 @@ from zerver.views.upload import (
from zerver.views.user_groups import (
add_user_group,
deactivate_user_group,
delete_user_group,
edit_user_group,
get_is_user_group_member,
get_subgroups_of_user_group,
@ -400,7 +399,7 @@ v1_api_and_json_patterns = [
# user_groups -> zerver.views.user_groups
rest_path("user_groups", GET=get_user_groups),
rest_path("user_groups/create", POST=add_user_group),
rest_path("user_groups/<int:user_group_id>", PATCH=edit_user_group, DELETE=delete_user_group),
rest_path("user_groups/<int:user_group_id>", PATCH=edit_user_group),
rest_path(
"user_groups/<int:user_group_id>/members",
GET=get_user_group_members,