diff --git a/api_docs/changelog.md b/api_docs/changelog.md index f633f747d2..34aff18150 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 10.0 +**Feature level 298** + +* [`POST /user_groups/{user_group_id}/deactivate`](/api/deactivate-user-group): + Server now returns a specific error response (`"code": CANNOT_DEACTIVATE_GROUP_IN_USE`) + when a user group cannot be deactivated because it is in use. The + error response contains details about where the user group is being used. + **Feature level 297** * [`GET /events`](/api/get-events), [`POST /register`](/api/register-queue): diff --git a/version.py b/version.py index 8478de6bdc..32a6075a26 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 = 297 # Last bumped for saved_snippets +API_FEATURE_LEVEL = 298 # Last bumped for CANNOT_DEACTIVATE_GROUP_IN_USE error. # 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/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 82c1689152..d17d1c1033 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -54,6 +54,7 @@ class ErrorCode(Enum): PUSH_NOTIFICATIONS_DISALLOWED = auto() EXPECTATION_MISMATCH = auto() SYSTEM_GROUP_REQUIRED = auto() + CANNOT_DEACTIVATE_GROUP_IN_USE = auto() class JsonableError(Exception): @@ -715,3 +716,19 @@ class IncompatibleParameterValuesError(JsonableError): @override def msg_format() -> str: return _("Incompatible values for '{first_parameter}' and '{second_parameter}'.") + + +class CannotDeactivateGroupInUseError(JsonableError): + code = ErrorCode.CANNOT_DEACTIVATE_GROUP_IN_USE + data_fields = ["objections"] + + def __init__( + self, + objections: list[dict[str, Any]], + ) -> None: + self.objections = objections + + @staticmethod + @override + def msg_format() -> str: + return _("Cannot deactivate user group in use.") diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index e0a263ba2c..5622367cf9 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -2,7 +2,7 @@ from collections import defaultdict from collections.abc import Collection, Iterable, Iterator, Mapping from contextlib import contextmanager from dataclasses import asdict, dataclass -from typing import TypedDict +from typing import Any, TypedDict from django.conf import settings from django.db import connection, transaction @@ -13,6 +13,7 @@ from django_cte import With from psycopg2.sql import SQL, Literal from zerver.lib.exceptions import ( + CannotDeactivateGroupInUseError, JsonableError, PreviousSettingValueMismatchedError, SystemGroupRequiredError, @@ -198,14 +199,14 @@ def access_user_group_for_deactivation( user_group_id, user_profile, permission_setting="can_manage_group" ) - if ( + objections: list[dict[str, Any]] = [] + supergroup_ids = ( user_group.direct_supergroups.exclude(named_user_group=None) .filter(named_user_group__deactivated=False) - .exists() - ): - raise JsonableError( - _("You cannot deactivate a user group that is subgroup of any user group.") - ) + .values_list("id", flat=True) + ) + if supergroup_ids: + objections.append(dict(type="subgroup", supergroup_ids=list(supergroup_ids))) anonymous_supergroup_ids = user_group.direct_supergroups.filter( named_user_group=None @@ -214,10 +215,10 @@ def access_user_group_for_deactivation( # We check both the cases - whether the group is being directly used # as the value of a setting or as a subgroup of an anonymous group # used for a setting. - setting_group_ids_using_deactivating_user_group = [ - *list(anonymous_supergroup_ids), + setting_group_ids_using_deactivating_user_group = { + *set(anonymous_supergroup_ids), user_group.id, - ] + } stream_setting_query = Q() for setting_name in Stream.stream_permission_group_settings: @@ -225,12 +226,19 @@ def access_user_group_for_deactivation( **{f"{setting_name}__in": setting_group_ids_using_deactivating_user_group} ) - if ( - Stream.objects.filter(realm_id=user_group.realm_id, deactivated=False) - .filter(stream_setting_query) - .exists() + for stream in Stream.objects.filter(realm_id=user_group.realm_id, deactivated=False).filter( + stream_setting_query ): - raise JsonableError(_("You cannot deactivate a user group which is used for setting.")) + objection_settings = [ + setting_name + for setting_name in Stream.stream_permission_group_settings + if getattr(stream, setting_name + "_id") + in setting_group_ids_using_deactivating_user_group + ] + if len(objection_settings) > 0: + objections.append( + dict(type="channel", channel_id=stream.id, settings=objection_settings) + ) group_setting_query = Q() for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: @@ -238,22 +246,33 @@ def access_user_group_for_deactivation( **{f"{setting_name}__in": setting_group_ids_using_deactivating_user_group} ) - if ( - NamedUserGroup.objects.filter(realm_id=user_group.realm_id, deactivated=False) - .filter(group_setting_query) - .exists() - ): - raise JsonableError(_("You cannot deactivate a user group which is used for setting.")) + for group in NamedUserGroup.objects.filter( + realm_id=user_group.realm_id, deactivated=False + ).filter(group_setting_query): + objection_settings = [] + for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: + if ( + getattr(group, setting_name + "_id") + in setting_group_ids_using_deactivating_user_group + ): + objection_settings.append(setting_name) - realm_setting_query = Q() + if len(objection_settings) > 0: + objections.append( + dict(type="user_group", group_id=group.id, settings=objection_settings) + ) + + objection_settings = [] + realm = user_group.realm for setting_name in Realm.REALM_PERMISSION_GROUP_SETTINGS: - realm_setting_query |= Q( - **{f"{setting_name}__in": setting_group_ids_using_deactivating_user_group} - ) + if getattr(realm, setting_name + "_id") in setting_group_ids_using_deactivating_user_group: + objection_settings.append(setting_name) - if Realm.objects.filter(id=user_group.realm_id).filter(realm_setting_query).exists(): - raise JsonableError(_("You cannot deactivate a user group which is used for setting.")) + if objection_settings: + objections.append(dict(type="realm", settings=objection_settings)) + if len(objections) > 0: + raise CannotDeactivateGroupInUseError(objections) return user_group diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 9627989c65..f50a0223e4 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -20651,6 +20651,44 @@ paths: responses: "200": $ref: "#/components/responses/SimpleSuccess" + "400": + description: Bad request. + content: + application/json: + schema: + oneOf: + - allOf: + - $ref: "#/components/schemas/CodedError" + - example: + { + "code": "BAD_REQUEST", + "msg": "Invalid user group", + "result": "error", + } + description: | + An example JSON response when the user group ID is invalid. + - allOf: + - $ref: "#/components/schemas/CodedError" + - example: + { + "code": "CANNOT_DEACTIVATE_GROUP_IN_USE", + "msg": "Cannot deactivate user group in use.", + "objections": + [ + { + "type": "realm", + "settings": + ["can_create_public_channel_group"], + }, + ], + "result": "error", + } + description: | + An example JSON response when the user group being deactivated + is used for a setting or as a subgroup. + + **Changes**: New in Zulip 10.0 (feature level 298). Previously, + this error returned the `"BAD_REQUEST"` code. /real-time: # This entry is a hack; it exists to give us a place to put the text # documenting the parameters for call_on_each_event and friends. diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 54f149525c..6900b6edf7 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -1355,8 +1355,10 @@ class UserGroupAPITestCase(UserGroupTestCase): ) # Check that group that is subgroup of another group cannot be deactivated. result = self.client_post(f"/json/user_groups/{leadership_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group that is subgroup of any user group." + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual( + data["objections"], [{"type": "subgroup", "supergroup_ids": [support_group.id]}] ) # If the supergroup is itself deactivated, then subgroup can be deactivated. @@ -1393,18 +1395,18 @@ class UserGroupAPITestCase(UserGroupTestCase): ) result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group which is used for setting." - ) + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual(data["objections"], [{"type": "realm", "settings": [setting_name]}]) do_change_realm_permission_group_setting( realm, setting_name, support_group, acting_user=None ) result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group which is used for setting." - ) + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual(data["objections"], [{"type": "realm", "settings": [setting_name]}]) # Reset the realm setting to one of the system group so this setting # does not interfere when testing for another setting. @@ -1419,8 +1421,11 @@ class UserGroupAPITestCase(UserGroupTestCase): ) result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group which is used for setting." + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual( + data["objections"], + [{"type": "channel", "channel_id": stream.id, "settings": [setting_name]}], ) # Test the group can be deactivated, if the stream which uses @@ -1444,8 +1449,11 @@ class UserGroupAPITestCase(UserGroupTestCase): ) result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group which is used for setting." + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual( + data["objections"], + [{"type": "channel", "channel_id": stream.id, "settings": [setting_name]}], ) # Test the group can be deactivated, if the stream which uses @@ -1473,8 +1481,17 @@ class UserGroupAPITestCase(UserGroupTestCase): ) result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group which is used for setting." + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual( + data["objections"], + [ + { + "type": "user_group", + "group_id": leadership_group.id, + "settings": [setting_name], + } + ], ) # Test the group can be deactivated, if the user group which uses @@ -1499,8 +1516,17 @@ class UserGroupAPITestCase(UserGroupTestCase): ) result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") - self.assert_json_error( - result, "You cannot deactivate a user group which is used for setting." + self.assert_json_error(result, "Cannot deactivate user group in use.") + data = orjson.loads(result.content) + self.assertEqual( + data["objections"], + [ + { + "type": "user_group", + "group_id": leadership_group.id, + "settings": [setting_name], + } + ], ) # Test the group can be deactivated, if the user group which uses