From 5f3a8334be588d3efbc8c1f43daf31f11bf44664 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Fri, 13 Sep 2024 13:14:45 +0530 Subject: [PATCH] user_groups: Do not allow deleting user groups. --- api_docs/changelog.md | 2 + api_docs/include/rest-endpoints.md | 1 - version.py | 2 +- web/src/server_events_dispatch.js | 4 - web/tests/dispatch.test.js | 19 -- web/tests/lib/events.js | 6 - zerver/actions/user_groups.py | 11 - zerver/openapi/curl_param_value_generators.py | 4 +- zerver/openapi/python_examples.py | 22 +- zerver/openapi/zulip.yaml | 37 +--- zerver/tests/test_events.py | 10 - zerver/tests/test_user_groups.py | 200 ------------------ zerver/views/user_groups.py | 17 -- zproject/urls.py | 3 +- 14 files changed, 14 insertions(+), 324 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index cd95f8d569..4261dc6115 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -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** diff --git a/api_docs/include/rest-endpoints.md b/api_docs/include/rest-endpoints.md index 8a2826269a..de65162f76 100644 --- a/api_docs/include/rest-endpoints.md +++ b/api_docs/include/rest-endpoints.md @@ -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) diff --git a/version.py b/version.py index e03874bcab..0edac40ef6 100644 --- a/version.py +++ b/version.py @@ -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 diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index c7f65838ee..a1a0f56547 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -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); diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index 76c3e83e22..431d4bbb49 100644 --- a/web/tests/dispatch.test.js +++ b/web/tests/dispatch.test.js @@ -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(); diff --git a/web/tests/lib/events.js b/web/tests/lib/events.js index c4fbd2dbe9..1b209ef98b 100644 --- a/web/tests/lib/events.js +++ b/web/tests/lib/events.js @@ -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", diff --git a/zerver/actions/user_groups.py b/zerver/actions/user_groups.py index a855f70217..997bf10a34 100644 --- a/zerver/actions/user_groups.py +++ b/zerver/actions/user_groups.py @@ -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 diff --git a/zerver/openapi/curl_param_value_generators.py b/zerver/openapi/curl_param_value_generators.py index a246986432..1e4136db0d 100644 --- a/zerver/openapi/curl_param_value_generators.py +++ b/zerver/openapi/curl_param_value_generators.py @@ -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", diff --git a/zerver/openapi/python_examples.py b/zerver/openapi/python_examples.py index 479c0218f1..da74c9e540 100644 --- a/zerver/openapi/python_examples.py +++ b/zerver/openapi/python_examples.py @@ -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) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index b728d8829e..bb2df7bda3 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -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 diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index bfec4cff48..15fba21367 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -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) diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 57217b1c54..5a7fbcf5bd 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -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") diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index 1f15b41d59..4bda173e18 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -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( diff --git a/zproject/urls.py b/zproject/urls.py index cd82588bd5..b7ca301fb7 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -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/", PATCH=edit_user_group, DELETE=delete_user_group), + rest_path("user_groups/", PATCH=edit_user_group), rest_path( "user_groups//members", GET=get_user_group_members,