From 7e7113ad843868c552d85ce352a1063c2b5307a5 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 15 Oct 2024 11:31:55 -0700 Subject: [PATCH] groups: Enable group-settings value on groups in production. The main change is redefining ALLOW_GROUP_VALUED_SETTINGS to not control code, but instead to instead control the configuration for whether settings that have not been converted to use our modern UI patterns should require system groups. Fundamentally, it's the same for the realm/stream group-valued settings, which don't have the new UI patterns yet. We remove the visual hiding of the "can manage group" setting, which was hidden only due to transitions being incomplete. --- api_docs/changelog.md | 7 +++ api_docs/group-setting-values.md | 12 +++-- version.py | 2 +- web/src/user_group_create.ts | 5 -- web/src/user_group_edit.js | 5 -- zerver/lib/user_groups.py | 9 ---- zerver/models/realms.py | 18 +++---- zerver/tests/test_realm.py | 46 ----------------- zerver/tests/test_user_groups.py | 85 -------------------------------- 9 files changed, 25 insertions(+), 164 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 828d11f93c..3382dfb4fb 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 309** + +* [Group-setting values](/api/group-setting-values): Starting with + this feature level, it's now possible to use group-setting values in + production for those settings whose value is not required to be a + system group + **Feature level 308** * [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events), diff --git a/api_docs/group-setting-values.md b/api_docs/group-setting-values.md index 24653c80b4..0fdece00ef 100644 --- a/api_docs/group-setting-values.md +++ b/api_docs/group-setting-values.md @@ -5,11 +5,15 @@ using [user groups](/help/user-groups), which offer much more flexible configuration than the older [roles](/api/roles-and-permissions) system. !!! warn "" + Note that many group-valued settings are configured to require + a single system group for their value via + `server_supported_permission_settings`, pending web app UI + changes to fully support group-setting values. - This API feature is under development, and currently only values that - correspond to a single named user group are permitted in - production environments, pending the web application UI supporting - displaying more complex values correctly. + **Changes**: Before Zulip 10.0 (feature level 309), only system + groups were permitted values for group-setting values in + production environments, regardless of the values in + `server_supported_permission_settings`. In the API, these settings are represented using a **group-setting value**, which can take two forms: diff --git a/version.py b/version.py index a01ab0bb90..d3b710152d 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 = 308 # Last bumped for can_leave_group. +API_FEATURE_LEVEL = 309 # Last bumped for group values in settings. # 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/user_group_create.ts b/web/src/user_group_create.ts index aac65ad878..8426b02f80 100644 --- a/web/src/user_group_create.ts +++ b/web/src/user_group_create.ts @@ -5,7 +5,6 @@ import * as channel from "./channel"; import {$t, $t_html} from "./i18n"; import * as keydown_util from "./keydown_util"; import * as loading from "./loading"; -import {page_params} from "./page_params"; import * as settings_components from "./settings_components"; import type {GroupSettingPillContainer} from "./typeahead_helper"; import * as ui_report from "./ui_report"; @@ -133,10 +132,6 @@ export function show_new_user_group_modal(): void { user_group_create_members.build_widgets(); clear_error_display(); - - if (!page_params.development_environment) { - $("#new_group_can_manage_group_widget_container").hide(); - } } function create_user_group(): void { diff --git a/web/src/user_group_edit.js b/web/src/user_group_edit.js index 40381cecfe..664ab074ea 100644 --- a/web/src/user_group_edit.js +++ b/web/src/user_group_edit.js @@ -18,7 +18,6 @@ import {$t, $t_html} from "./i18n"; import * as ListWidget from "./list_widget"; import * as loading from "./loading"; import * as overlays from "./overlays"; -import {page_params} from "./page_params"; import * as people from "./people"; import * as scroll_util from "./scroll_util"; import * as settings_components from "./settings_components"; @@ -198,10 +197,6 @@ function show_general_settings(group) { group, }); update_general_panel_ui(group); - - if (!page_params.development_environment) { - $(".can-manage-group-container").hide(); - } } function update_general_panel_ui(group) { diff --git a/zerver/lib/user_groups.py b/zerver/lib/user_groups.py index 74f3dbde45..eb351f04ee 100644 --- a/zerver/lib/user_groups.py +++ b/zerver/lib/user_groups.py @@ -4,7 +4,6 @@ from contextlib import contextmanager from dataclasses import asdict, dataclass from typing import Any, TypedDict -from django.conf import settings from django.db import connection, transaction from django.db.models import F, Q, QuerySet from django.utils.timezone import now as timezone_now @@ -355,11 +354,6 @@ def check_setting_configuration_for_system_groups( setting_name: str, permission_configuration: GroupPermissionSetting, ) -> None: - if setting_name != "can_mention_group" and ( - not settings.ALLOW_GROUP_VALUED_SETTINGS and not setting_group.is_system_group - ): - raise SystemGroupRequiredError(setting_name) - if permission_configuration.require_system_group and not setting_group.is_system_group: raise SystemGroupRequiredError(setting_name) @@ -1018,9 +1012,6 @@ def parse_group_setting_value( if len(setting_value.direct_members) == 0 and len(setting_value.direct_subgroups) == 1: return setting_value.direct_subgroups[0] - if not settings.ALLOW_GROUP_VALUED_SETTINGS: - raise SystemGroupRequiredError(setting_name) - return setting_value diff --git a/zerver/models/realms.py b/zerver/models/realms.py index fb37a7b4cb..d56c49d114 100644 --- a/zerver/models/realms.py +++ b/zerver/models/realms.py @@ -704,7 +704,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub allowed_system_groups=[SystemGroups.EVERYONE, SystemGroups.MEMBERS], ), can_add_custom_emoji_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=False, allow_nobody_group=False, @@ -713,7 +713,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="can_add_custom_emoji_group_id", ), can_create_groups=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=True, allow_nobody_group=False, @@ -722,7 +722,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="can_create_groups_id", ), can_create_public_channel_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=False, allow_nobody_group=False, @@ -731,7 +731,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="can_create_public_channel_group_id", ), can_create_private_channel_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=False, allow_nobody_group=False, @@ -755,7 +755,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub ], ), can_delete_any_message_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=False, allow_nobody_group=False, @@ -764,7 +764,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="can_delete_any_message_group_id", ), can_delete_own_message_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=False, allow_nobody_group=False, @@ -773,7 +773,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="can_delete_own_message_group_id", ), can_manage_all_groups=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=True, allow_nobody_group=False, @@ -782,7 +782,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="can_manage_all_groups_id", ), direct_message_initiator_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=True, allow_nobody_group=True, @@ -791,7 +791,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub id_field_name="direct_message_initiator_group_id", ), direct_message_permission_group=GroupPermissionSetting( - require_system_group=False, + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, allow_owners_group=True, allow_nobody_group=True, diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 5c3f1eb1b9..5f431fd4c8 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -2032,52 +2032,6 @@ class RealmAPITest(ZulipTestCase): realm = get_realm("zulip") self.assertEqual(getattr(realm, setting_name), admins_group.usergroup_ptr) - # Test case when ALLOW_GROUP_VALUED_SETTINGS is False. - with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False): - result = self.client_patch( - "/json/realm", - { - setting_name: orjson.dumps( - { - "new": { - "direct_members": [othello.id], - "direct_subgroups": [moderators_group.id, leadership_group.id], - } - } - ).decode() - }, - ) - self.assert_json_error(result, f"'{setting_name}' must be a system user group.") - - result = self.client_patch( - "/json/realm", - { - setting_name: orjson.dumps( - { - "new": leadership_group.id, - } - ).decode() - }, - ) - self.assert_json_error(result, f"'{setting_name}' must be a system user group.") - - result = self.client_patch( - "/json/realm", - { - setting_name: orjson.dumps( - { - "new": { - "direct_members": [], - "direct_subgroups": [moderators_group.id], - } - } - ).decode() - }, - ) - self.assert_json_success(result) - realm = get_realm("zulip") - self.assertEqual(getattr(realm, setting_name), moderators_group.usergroup_ptr) - def test_update_realm_properties(self) -> None: for prop in Realm.property_types: # push_notifications_enabled is maintained by the server, not via the API. diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index 8bd7e5654e..f6a90ae832 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -722,51 +722,6 @@ class UserGroupAPITestCase(UserGroupTestCase): leadership_group.deactivated = False leadership_group.save() - with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False): - params = { - "name": "frontend", - "members": orjson.dumps([hamlet.id]).decode(), - "description": "Frontend team", - } - params[setting_name] = orjson.dumps( - { - "direct_members": [othello.id], - "direct_subgroups": [moderators_group.id], - } - ).decode() - result = self.client_post("/json/user_groups/create", info=params) - self.assert_json_error(result, f"'{setting_name}' must be a system user group.") - - params = { - "name": "frontend", - "members": orjson.dumps([hamlet.id]).decode(), - "description": "Frontend team", - } - params[setting_name] = orjson.dumps( - { - "direct_members": [], - "direct_subgroups": [moderators_group.id], - } - ).decode() - result = self.client_post("/json/user_groups/create", info=params) - self.assert_json_success(result) - frontend_group = NamedUserGroup.objects.get(name="frontend", realm=hamlet.realm) - self.assertEqual(getattr(frontend_group, setting_name).id, moderators_group.id) - - params = { - "name": "devops", - "members": orjson.dumps([hamlet.id]).decode(), - "description": "Devops team", - } - params[setting_name] = orjson.dumps(leadership_group.id).decode() - result = self.client_post("/json/user_groups/create", info=params) - if setting_name == "can_mention_group": - self.assert_json_success(result) - devops_group = NamedUserGroup.objects.get(name="devops", realm=hamlet.realm) - self.assertEqual(getattr(devops_group, setting_name).id, leadership_group.id) - else: - self.assert_json_error(result, f"'{setting_name}' must be a system user group.") - def test_set_group_settings_during_user_group_creation(self) -> None: for setting_name in NamedUserGroup.GROUP_PERMISSION_SETTINGS: self.do_test_set_group_setting_during_user_group_creation(setting_name) @@ -1041,46 +996,6 @@ class UserGroupAPITestCase(UserGroupTestCase): leadership_group.deactivated = False leadership_group.save() - # Test case when ALLOW_GROUP_VALUED_SETTINGS is False. - with self.settings(ALLOW_GROUP_VALUED_SETTINGS=False): - params[setting_name] = orjson.dumps( - { - "new": { - "direct_members": [othello.id], - "direct_subgroups": [moderators_group.id, marketing_group.id], - } - } - ).decode() - result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) - self.assert_json_error(result, f"'{setting_name}' must be a system user group.") - - params[setting_name] = orjson.dumps( - { - "new": { - "direct_members": [], - "direct_subgroups": [moderators_group.id], - } - } - ).decode() - result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) - self.assert_json_success(result) - support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) - self.assertEqual(getattr(support_group, setting_name).id, moderators_group.id) - - params[setting_name] = orjson.dumps( - { - "new": marketing_group.id, - } - ).decode() - result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params) - - if setting_name == "can_mention_group": - self.assert_json_success(result) - support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm) - self.assertEqual(getattr(support_group, setting_name).id, marketing_group.id) - else: - self.assert_json_error(result, f"'{setting_name}' must be a system user group.") - def test_update_user_group_permission_settings(self) -> None: hamlet = self.example_user("hamlet") check_add_user_group(hamlet.realm, "support", [hamlet], acting_user=hamlet)