user_groups: Include settings and supergroups in error response.

The error response when a user group cannot be deactivated due
to it being used as a subgroup or for a setting includes details
about the supergroups, streams, user groups as well the settings
for which it is used.
This commit is contained in:
Sahil Batra 2024-09-26 15:46:07 +05:30 committed by Tim Abbott
parent 19ba94b946
commit b8a039ee99
6 changed files with 151 additions and 44 deletions

View File

@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with.
## Changes in Zulip 10.0 ## 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** **Feature level 297**
* [`GET /events`](/api/get-events), [`POST /register`](/api/register-queue): * [`GET /events`](/api/get-events), [`POST /register`](/api/register-queue):

View File

@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3"
# new level means in api_docs/changelog.md, as well as "**Changes**" # new level means in api_docs/changelog.md, as well as "**Changes**"
# entries in the endpoint's documentation in `zulip.yaml`. # 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 # 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 # only when going from an old version of the code to a newer version. Bump

View File

@ -54,6 +54,7 @@ class ErrorCode(Enum):
PUSH_NOTIFICATIONS_DISALLOWED = auto() PUSH_NOTIFICATIONS_DISALLOWED = auto()
EXPECTATION_MISMATCH = auto() EXPECTATION_MISMATCH = auto()
SYSTEM_GROUP_REQUIRED = auto() SYSTEM_GROUP_REQUIRED = auto()
CANNOT_DEACTIVATE_GROUP_IN_USE = auto()
class JsonableError(Exception): class JsonableError(Exception):
@ -715,3 +716,19 @@ class IncompatibleParameterValuesError(JsonableError):
@override @override
def msg_format() -> str: def msg_format() -> str:
return _("Incompatible values for '{first_parameter}' and '{second_parameter}'.") 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.")

View File

@ -2,7 +2,7 @@ from collections import defaultdict
from collections.abc import Collection, Iterable, Iterator, Mapping from collections.abc import Collection, Iterable, Iterator, Mapping
from contextlib import contextmanager from contextlib import contextmanager
from dataclasses import asdict, dataclass from dataclasses import asdict, dataclass
from typing import TypedDict from typing import Any, TypedDict
from django.conf import settings from django.conf import settings
from django.db import connection, transaction from django.db import connection, transaction
@ -13,6 +13,7 @@ from django_cte import With
from psycopg2.sql import SQL, Literal from psycopg2.sql import SQL, Literal
from zerver.lib.exceptions import ( from zerver.lib.exceptions import (
CannotDeactivateGroupInUseError,
JsonableError, JsonableError,
PreviousSettingValueMismatchedError, PreviousSettingValueMismatchedError,
SystemGroupRequiredError, SystemGroupRequiredError,
@ -198,14 +199,14 @@ def access_user_group_for_deactivation(
user_group_id, user_profile, permission_setting="can_manage_group" 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) user_group.direct_supergroups.exclude(named_user_group=None)
.filter(named_user_group__deactivated=False) .filter(named_user_group__deactivated=False)
.exists() .values_list("id", flat=True)
):
raise JsonableError(
_("You cannot deactivate a user group that is subgroup of any user group.")
) )
if supergroup_ids:
objections.append(dict(type="subgroup", supergroup_ids=list(supergroup_ids)))
anonymous_supergroup_ids = user_group.direct_supergroups.filter( anonymous_supergroup_ids = user_group.direct_supergroups.filter(
named_user_group=None 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 # 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 # as the value of a setting or as a subgroup of an anonymous group
# used for a setting. # used for a setting.
setting_group_ids_using_deactivating_user_group = [ setting_group_ids_using_deactivating_user_group = {
*list(anonymous_supergroup_ids), *set(anonymous_supergroup_ids),
user_group.id, user_group.id,
] }
stream_setting_query = Q() stream_setting_query = Q()
for setting_name in Stream.stream_permission_group_settings: 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} **{f"{setting_name}__in": setting_group_ids_using_deactivating_user_group}
) )
if ( for stream in Stream.objects.filter(realm_id=user_group.realm_id, deactivated=False).filter(
Stream.objects.filter(realm_id=user_group.realm_id, deactivated=False) stream_setting_query
.filter(stream_setting_query)
.exists()
): ):
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() group_setting_query = Q()
for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: 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} **{f"{setting_name}__in": setting_group_ids_using_deactivating_user_group}
) )
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 ( if (
NamedUserGroup.objects.filter(realm_id=user_group.realm_id, deactivated=False) getattr(group, setting_name + "_id")
.filter(group_setting_query) in setting_group_ids_using_deactivating_user_group
.exists()
): ):
raise JsonableError(_("You cannot deactivate a user group which is used for setting.")) objection_settings.append(setting_name)
realm_setting_query = Q() if len(objection_settings) > 0:
for setting_name in Realm.REALM_PERMISSION_GROUP_SETTINGS: objections.append(
realm_setting_query |= Q( dict(type="user_group", group_id=group.id, settings=objection_settings)
**{f"{setting_name}__in": setting_group_ids_using_deactivating_user_group}
) )
if Realm.objects.filter(id=user_group.realm_id).filter(realm_setting_query).exists(): objection_settings = []
raise JsonableError(_("You cannot deactivate a user group which is used for setting.")) realm = user_group.realm
for setting_name in Realm.REALM_PERMISSION_GROUP_SETTINGS:
if getattr(realm, setting_name + "_id") in setting_group_ids_using_deactivating_user_group:
objection_settings.append(setting_name)
if objection_settings:
objections.append(dict(type="realm", settings=objection_settings))
if len(objections) > 0:
raise CannotDeactivateGroupInUseError(objections)
return user_group return user_group

View File

@ -20651,6 +20651,44 @@ paths:
responses: responses:
"200": "200":
$ref: "#/components/responses/SimpleSuccess" $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: /real-time:
# This entry is a hack; it exists to give us a place to put the text # 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. # documenting the parameters for call_on_each_event and friends.

View File

@ -1355,8 +1355,10 @@ class UserGroupAPITestCase(UserGroupTestCase):
) )
# Check that group that is subgroup of another group cannot be deactivated. # Check that group that is subgroup of another group cannot be deactivated.
result = self.client_post(f"/json/user_groups/{leadership_group.id}/deactivate") result = self.client_post(f"/json/user_groups/{leadership_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group that is subgroup of any user group." 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. # 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") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group which is used for setting." data = orjson.loads(result.content)
) self.assertEqual(data["objections"], [{"type": "realm", "settings": [setting_name]}])
do_change_realm_permission_group_setting( do_change_realm_permission_group_setting(
realm, setting_name, support_group, acting_user=None realm, setting_name, support_group, acting_user=None
) )
result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group which is used for setting." 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 # Reset the realm setting to one of the system group so this setting
# does not interfere when testing for another 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") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group which is used for setting." 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 # 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") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group which is used for setting." 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 # 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") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group which is used for setting." 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 # 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") result = self.client_post(f"/json/user_groups/{support_group.id}/deactivate")
self.assert_json_error( self.assert_json_error(result, "Cannot deactivate user group in use.")
result, "You cannot deactivate a user group which is used for setting." 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 # Test the group can be deactivated, if the user group which uses