From 1be0cb1b75cdc3f6fad7a681f17ac795ad79f7b2 Mon Sep 17 00:00:00 2001 From: Vector73 Date: Wed, 16 Oct 2024 21:39:38 +0530 Subject: [PATCH] settings: Add `can_move_messages_between_channels_group` realm setting. Added `can_move_messages_between_channels_group` realm setting to replace `move_messages_between_streams_policy`. --- api_docs/changelog.md | 8 + version.py | 2 +- web/src/message_edit.ts | 4 +- web/src/server_events_dispatch.js | 2 +- web/src/settings_components.ts | 4 +- web/src/settings_config.ts | 2 - web/src/settings_data.ts | 6 +- web/src/settings_org.ts | 96 ++++--- web/src/state_data.ts | 2 +- web/src/stream_popover.js | 2 +- .../organization_permissions_admin.hbs | 11 +- web/tests/settings_data.test.js | 21 +- web/tests/settings_org.test.js | 1 + zerver/actions/create_realm.py | 9 +- zerver/lib/event_schema.py | 1 + ...an_move_messages_between_channels_group.py | 23 ++ ...an_move_messages_between_channels_group.py | 46 ++++ ...an_move_messages_between_channels_group.py | 22 ++ zerver/models/realms.py | 17 ++ zerver/models/users.py | 3 +- zerver/openapi/zulip.yaml | 38 ++- zerver/tests/test_home.py | 1 + zerver/tests/test_message_move_stream.py | 246 +++++++++++------- zerver/tests/test_message_move_topic.py | 8 +- zerver/tests/test_realm.py | 17 +- zerver/views/realm.py | 1 + 26 files changed, 408 insertions(+), 185 deletions(-) create mode 100644 zerver/migrations/0611_realm_can_move_messages_between_channels_group.py create mode 100644 zerver/migrations/0612_set_default_value_for_can_move_messages_between_channels_group.py create mode 100644 zerver/migrations/0613_alter_realm_can_move_messages_between_channels_group.py diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 3382dfb4fb..5ee8ecb8a6 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,14 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 310** + +* `PATCH /realm`, [`GET /events`](/api/get-events), + [`POST /register`](/api/register-queue): + Added `can_move_messages_between_channels_group` realm setting which is a + [group-setting value](/api/group-setting-values) describing the set of users + with permission to move messages from one channel to another in the organization. + **Feature level 309** * [Group-setting values](/api/group-setting-values): Starting with diff --git a/version.py b/version.py index d3b710152d..a64ac69648 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 = 309 # Last bumped for group values in settings. +API_FEATURE_LEVEL = 310 # Last bumped for adding `can_move_messages_between_channels_group`. # 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/message_edit.ts b/web/src/message_edit.ts index 087972a02a..5f2637755a 100644 --- a/web/src/message_edit.ts +++ b/web/src/message_edit.ts @@ -248,8 +248,8 @@ export function is_stream_editable(message: Message, edit_limit_seconds_buffer = } // Organization admins and moderators can edit stream indefinitely, - // irrespective of the stream editing deadline, if - // move_messages_between_streams_policy allows them to do so. + // irrespective of the stream editing deadline, if they are in the + // can_move_messages_between_channels_group. if (current_user.is_admin || current_user.is_moderator) { return true; } diff --git a/web/src/server_events_dispatch.js b/web/src/server_events_dispatch.js index 82abf84d10..5ea201969c 100644 --- a/web/src/server_events_dispatch.js +++ b/web/src/server_events_dispatch.js @@ -212,6 +212,7 @@ export function dispatch_normal_event(event) { bot_creation_policy: settings_bots.update_bot_permissions_ui, can_delete_any_message_group: noop, can_delete_own_message_group: noop, + can_move_messages_between_channels_group: noop, create_multiuse_invite_group: noop, invite_to_stream_policy: noop, default_code_block_language: noop, @@ -233,7 +234,6 @@ export function dispatch_normal_event(event) { move_messages_between_streams_limit_seconds: noop, move_messages_within_stream_limit_seconds: message_edit.update_inline_topic_edit_ui, message_retention_days: noop, - move_messages_between_streams_policy: noop, name: narrow_title.redraw_title, name_changes_disabled: settings_account.update_name_change_display, new_stream_announcements_stream_id: stream_ui_updates.update_announce_stream_option, diff --git a/web/src/settings_components.ts b/web/src/settings_components.ts index 66992e0746..d83f131a5d 100644 --- a/web/src/settings_components.ts +++ b/web/src/settings_components.ts @@ -238,7 +238,6 @@ export const simple_dropdown_realm_settings_schema = realm_schema.pick({ realm_invite_to_stream_policy: true, realm_invite_to_realm_policy: true, realm_wildcard_mention_policy: true, - realm_move_messages_between_streams_policy: true, realm_edit_topic_policy: true, realm_org_type: true, }); @@ -505,6 +504,7 @@ const dropdown_widget_map = new Map([ ["realm_can_delete_any_message_group", null], ["realm_can_delete_own_message_group", null], ["realm_can_manage_all_groups", null], + ["realm_can_move_messages_between_channels_group", null], ["realm_direct_message_initiator_group", null], ["realm_direct_message_permission_group", null], ]); @@ -827,6 +827,7 @@ export function check_realm_settings_property_changed(elem: HTMLElement): boolea case "realm_can_delete_any_message_group": case "realm_can_delete_own_message_group": case "realm_can_manage_all_groups": + case "realm_can_move_messages_between_channels_group": case "realm_direct_message_initiator_group": case "realm_direct_message_permission_group": proposed_val = get_dropdown_list_widget_setting_value($elem); @@ -1067,6 +1068,7 @@ export function populate_data_for_realm_settings_request( "can_manage_all_groups", "can_delete_any_message_group", "can_delete_own_message_group", + "can_move_messages_between_channels_group", "direct_message_initiator_group", "direct_message_permission_group", ]); diff --git a/web/src/settings_config.ts b/web/src/settings_config.ts index cfce2a678f..9ab85e2144 100644 --- a/web/src/settings_config.ts +++ b/web/src/settings_config.ts @@ -348,8 +348,6 @@ export const edit_topic_policy_values = { }, }; -export const move_messages_between_streams_policy_values = email_invite_to_realm_policy_values; - export const time_limit_dropdown_values = [ { text: $t({defaultMessage: "Any time"}), diff --git a/web/src/settings_data.ts b/web/src/settings_data.ts index 258393ffec..11701e4a1d 100644 --- a/web/src/settings_data.ts +++ b/web/src/settings_data.ts @@ -176,7 +176,11 @@ export function user_can_create_web_public_streams(): boolean { } export function user_can_move_messages_between_streams(): boolean { - return user_has_permission(realm.realm_move_messages_between_streams_policy); + return user_has_permission_for_group_setting( + realm.realm_can_move_messages_between_channels_group, + "can_move_messages_between_channels_group", + "realm", + ); } export function user_can_manage_all_groups(): boolean { diff --git a/web/src/settings_org.ts b/web/src/settings_org.ts index d915beef93..12783a69b8 100644 --- a/web/src/settings_org.ts +++ b/web/src/settings_org.ts @@ -111,7 +111,6 @@ type OrganizationSettingsOptions = { common_message_policy_values: SettingOptionValueWithKey[]; invite_to_realm_policy_values: SettingOptionValueWithKey[]; edit_topic_policy_values: SettingOptionValueWithKey[]; - move_messages_between_streams_policy_values: SettingOptionValueWithKey[]; }; export function get_organization_settings_options(): OrganizationSettingsOptions { @@ -131,9 +130,6 @@ export function get_organization_settings_options(): OrganizationSettingsOptions edit_topic_policy_values: settings_components.get_sorted_options_list( settings_config.edit_topic_policy_values, ), - move_messages_between_streams_policy_values: settings_components.get_sorted_options_list( - settings_config.move_messages_between_streams_policy_values, - ), }; } @@ -231,30 +227,38 @@ function set_msg_edit_limit_dropdown(): void { } function message_move_limit_setting_enabled( - related_setting_name: "realm_edit_topic_policy" | "realm_move_messages_between_streams_policy", + related_setting_name: + | "realm_edit_topic_policy" + | "realm_can_move_messages_between_channels_group", ): boolean { - const setting_value_string = $( - `select:not(multiple)#id_${CSS.escape(related_setting_name)}`, - ).val(); - assert(setting_value_string !== undefined); - const setting_value = Number.parseInt(setting_value_string, 10); - - let settings_options; if (related_setting_name === "realm_edit_topic_policy") { - settings_options = settings_config.edit_topic_policy_values; - } else { - settings_options = settings_config.move_messages_between_streams_policy_values; - } + const setting_value_string = $( + `select:not(multiple)#id_${CSS.escape(related_setting_name)}`, + ).val(); + assert(setting_value_string !== undefined); + const setting_value = Number.parseInt(setting_value_string, 10); + const settings_options = settings_config.edit_topic_policy_values; - if (setting_value === settings_options.by_admins_only.code) { - return false; - } + if ( + setting_value === settings_options.by_admins_only.code || + setting_value === settings_options.by_moderators_only.code || + setting_value === settings_options.nobody.code + ) { + return false; + } - if (setting_value === settings_options.by_moderators_only.code) { - return false; + return true; } - - if (setting_value === settings_options.nobody.code) { + const user_group_id = settings_components.get_dropdown_list_widget_setting_value( + $(`#id_${related_setting_name}`), + ); + assert(typeof user_group_id === "number"); + const user_group_name = user_groups.get_user_group_from_id(user_group_id).name; + if ( + user_group_name === "role:administrators" || + user_group_name === "role:moderators" || + user_group_name === "role:nobody" + ) { return false; } @@ -280,7 +284,7 @@ function set_msg_move_limit_setting(property_name: MessageMoveTimeLimitSetting): disable_setting = message_move_limit_setting_enabled("realm_edit_topic_policy"); } else { disable_setting = message_move_limit_setting_enabled( - "realm_move_messages_between_streams_policy", + "realm_can_move_messages_between_channels_group", ); } enable_or_disable_related_message_move_time_limit_setting(property_name, disable_setting); @@ -499,6 +503,9 @@ function update_dependent_subsettings(property_name: string): void { case "realm_can_delete_own_message_group": check_disable_message_delete_limit_setting_dropdown(); break; + case "realm_can_move_messages_between_channels_group": + set_msg_move_limit_setting("realm_move_messages_between_streams_limit_seconds"); + break; case "realm_org_join_restrictions": set_org_join_restrictions_dropdown(); break; @@ -556,6 +563,7 @@ export function discard_realm_property_element_changes(elem: HTMLElement): void case "realm_can_delete_any_message_group": case "realm_can_delete_own_message_group": case "realm_can_manage_all_groups": + case "realm_can_move_messages_between_channels_group": assert(typeof property_value === "string" || typeof property_value === "number"); settings_components.set_dropdown_list_widget_setting_value( property_name, @@ -914,18 +922,32 @@ export function set_up_dropdown_widget_for_realm_group_settings(): void { let dropdown_list_item_click_callback: | ((current_value: string | number | undefined) => void) | undefined; - if (setting_name === "direct_message_permission_group") { - dropdown_list_item_click_callback = ( - current_value: string | number | undefined, - ): void => { - assert(typeof current_value === "number"); - check_disable_direct_message_initiator_group_dropdown(current_value); - }; - } else if ( - setting_name === "can_delete_any_message_group" || - setting_name === "can_delete_own_message_group" - ) { - dropdown_list_item_click_callback = check_disable_message_delete_limit_setting_dropdown; + switch (setting_name) { + case "direct_message_permission_group": { + dropdown_list_item_click_callback = ( + current_value: string | number | undefined, + ): void => { + assert(typeof current_value === "number"); + check_disable_direct_message_initiator_group_dropdown(current_value); + }; + + break; + } + case "can_delete_any_message_group": + case "can_delete_own_message_group": { + dropdown_list_item_click_callback = + check_disable_message_delete_limit_setting_dropdown; + + break; + } + case "can_move_messages_between_channels_group": { + dropdown_list_item_click_callback = () => { + set_msg_move_limit_setting("realm_move_messages_between_streams_limit_seconds"); + }; + + break; + } + // No default } set_up_dropdown_widget( @@ -1224,7 +1246,7 @@ export function build_page(): void { function (this: HTMLElement) { const $policy_dropdown_elem = $(this); const property_name = z - .enum(["realm_edit_topic_policy", "realm_move_messages_between_streams_policy"]) + .enum(["realm_edit_topic_policy", "realm_can_move_messages_between_channels_group"]) .parse(settings_components.extract_property_name($policy_dropdown_elem)); const disable_time_limit_setting = message_move_limit_setting_enabled(property_name); diff --git a/web/src/state_data.ts b/web/src/state_data.ts index c9d9258c1d..50a3557bcf 100644 --- a/web/src/state_data.ts +++ b/web/src/state_data.ts @@ -295,6 +295,7 @@ export const realm_schema = z.object({ realm_can_delete_any_message_group: z.number(), realm_can_delete_own_message_group: z.number(), realm_can_manage_all_groups: z.number(), + realm_can_move_messages_between_channels_group: z.number(), realm_create_multiuse_invite_group: z.number(), realm_date_created: z.number(), realm_default_code_block_language: z.string(), @@ -366,7 +367,6 @@ export const realm_schema = z.object({ realm_message_content_delete_limit_seconds: z.number().nullable(), realm_message_retention_days: z.number(), realm_move_messages_between_streams_limit_seconds: z.number().nullable(), - realm_move_messages_between_streams_policy: z.number(), realm_move_messages_within_stream_limit_seconds: z.number().nullable(), realm_name_changes_disabled: z.boolean(), realm_name: z.string(), diff --git a/web/src/stream_popover.js b/web/src/stream_popover.js index 077d8ee4f0..7623a16976 100644 --- a/web/src/stream_popover.js +++ b/web/src/stream_popover.js @@ -354,7 +354,7 @@ export async function build_move_topic_to_stream_popover( // When the modal is opened for moving the whole topic from left sidebar, // we do not have any message object and so we disable the stream input - // based on the move_messages_between_streams_policy setting and topic + // based on the can_move_messages_between_channels_group setting and topic // input based on edit_topic_policy. In other cases, message object is // available and thus we check the time-based permissions as well in the // below if block to enable or disable the stream and topic input. diff --git a/web/templates/settings/organization_permissions_admin.hbs b/web/templates/settings/organization_permissions_admin.hbs index 8fa3387761..689a9642e8 100644 --- a/web/templates/settings/organization_permissions_admin.hbs +++ b/web/templates/settings/organization_permissions_admin.hbs @@ -201,13 +201,10 @@ -
- - -
+ {{> ../dropdown_widget_with_label + widget_name="realm_can_move_messages_between_channels_group" + label=(t 'Who can move messages to another channel') + value_type="number" }}
diff --git a/web/tests/settings_data.test.js b/web/tests/settings_data.test.js index 91ef79a1d8..668feaa0c7 100644 --- a/web/tests/settings_data.test.js +++ b/web/tests/settings_data.test.js @@ -162,11 +162,6 @@ test_policy( "realm_invite_to_realm_policy", settings_data.user_can_invite_users_by_email, ); -test_policy( - "user_can_move_messages_between_streams", - "realm_move_messages_between_streams_policy", - settings_data.user_can_move_messages_between_streams, -); function test_message_policy(label, policy, validation_func) { run_test(label, ({override}) => { @@ -228,17 +223,6 @@ run_test("user_can_move_messages_to_another_topic_nobody_case", ({override}) => assert.equal(settings_data.user_can_move_messages_to_another_topic(), false); }); -run_test("user_can_move_messages_between_streams_nobody_case", ({override}) => { - override(current_user, "is_admin", true); - override(current_user, "is_guest", false); - override( - realm, - "realm_move_messages_between_streams_policy", - settings_config.move_messages_between_streams_policy_values.nobody.code, - ); - assert.equal(settings_data.user_can_move_messages_between_streams(), false); -}); - test_realm_group_settings( "realm_can_add_custom_emoji_group", settings_data.user_can_add_custom_emoji, @@ -254,6 +238,11 @@ test_realm_group_settings( settings_data.user_can_delete_own_message, ); +test_realm_group_settings( + "realm_can_move_messages_between_channels_group", + settings_data.user_can_move_messages_between_streams, +); + run_test("using_dark_theme", ({override}) => { override(user_settings, "color_scheme", settings_config.color_scheme_values.dark.code); assert.equal(settings_data.using_dark_theme(), true); diff --git a/web/tests/settings_org.test.js b/web/tests/settings_org.test.js index 53e3b20bbe..d38e2e6134 100644 --- a/web/tests/settings_org.test.js +++ b/web/tests/settings_org.test.js @@ -506,6 +506,7 @@ function test_discard_changes_button({override}, discard_changes) { test("set_up", ({override, override_rewire}) => { override_rewire(settings_org, "check_disable_message_delete_limit_setting_dropdown", noop); + override_rewire(settings_org, "message_move_limit_setting_enabled", noop); override(realm, "realm_available_video_chat_providers", { jitsi_meet: { id: 1, diff --git a/zerver/actions/create_realm.py b/zerver/actions/create_realm.py index f53e309064..edd03d6de0 100644 --- a/zerver/actions/create_realm.py +++ b/zerver/actions/create_realm.py @@ -41,7 +41,6 @@ from zerver.models.realm_audit_logs import AuditLogEventType from zerver.models.realms import ( CommonPolicyEnum, InviteToRealmPolicyEnum, - MoveMessagesBetweenStreamsPolicyEnum, get_org_type_display_name, get_realm, ) @@ -125,10 +124,6 @@ def set_realm_permissions_based_on_org_type(realm: Realm) -> None: # Don't allow members (students) to manage user groups or # stream subscriptions. realm.invite_to_stream_policy = CommonPolicyEnum.MODERATORS_ONLY - # Allow moderators (TAs?) to move topics between streams. - realm.move_messages_between_streams_policy = ( - MoveMessagesBetweenStreamsPolicyEnum.MODERATORS_ONLY - ) @transaction.atomic(savepoint=False) @@ -299,6 +294,10 @@ def do_create_realm( Realm.ORG_TYPES["education_nonprofit"]["id"]: SystemGroups.MODERATORS, Realm.ORG_TYPES["education"]["id"]: SystemGroups.MODERATORS, }, + "can_move_messages_between_channels_group": { + Realm.ORG_TYPES["education_nonprofit"]["id"]: SystemGroups.MODERATORS, + Realm.ORG_TYPES["education"]["id"]: SystemGroups.MODERATORS, + }, } set_default_for_realm_permission_group_settings( realm, group_settings_defaults_for_org_types diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 5b94fe66bd..b2628a0995 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1067,6 +1067,7 @@ group_setting_update_data_type = DictType( ("can_delete_any_message_group", group_setting_type), ("can_delete_own_message_group", group_setting_type), ("can_manage_all_groups", group_setting_type), + ("can_move_messages_between_channels_group", group_setting_type), ("direct_message_initiator_group", group_setting_type), ("direct_message_permission_group", group_setting_type), ], diff --git a/zerver/migrations/0611_realm_can_move_messages_between_channels_group.py b/zerver/migrations/0611_realm_can_move_messages_between_channels_group.py new file mode 100644 index 0000000000..6383b25fb7 --- /dev/null +++ b/zerver/migrations/0611_realm_can_move_messages_between_channels_group.py @@ -0,0 +1,23 @@ +# Generated by Django 5.0.9 on 2024-10-12 13:48 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0610_mark_introduce_resolve_topic_modal_as_read"), + ] + + operations = [ + migrations.AddField( + model_name="realm", + name="can_move_messages_between_channels_group", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.RESTRICT, + related_name="+", + to="zerver.usergroup", + ), + ), + ] diff --git a/zerver/migrations/0612_set_default_value_for_can_move_messages_between_channels_group.py b/zerver/migrations/0612_set_default_value_for_can_move_messages_between_channels_group.py new file mode 100644 index 0000000000..8930023146 --- /dev/null +++ b/zerver/migrations/0612_set_default_value_for_can_move_messages_between_channels_group.py @@ -0,0 +1,46 @@ +# Generated by Django 4.2.1 on 2023-06-12 10:47 + +from django.db import migrations +from django.db.backends.base.schema import BaseDatabaseSchemaEditor +from django.db.migrations.state import StateApps +from django.db.models import OuterRef + + +def set_default_value_for_can_move_messages_between_channels_group( + apps: StateApps, schema_editor: BaseDatabaseSchemaEditor +) -> None: + Realm = apps.get_model("zerver", "Realm") + NamedUserGroup = apps.get_model("zerver", "NamedUserGroup") + + move_messages_between_streams_policy_to_group_name = { + 1: "role:members", + 2: "role:administrators", + 3: "role:fullmembers", + 4: "role:moderators", + 6: "role:nobody", + } + + for id, group_name in move_messages_between_streams_policy_to_group_name.items(): + Realm.objects.filter( + can_move_messages_between_channels_group=None, move_messages_between_streams_policy=id + ).update( + can_move_messages_between_channels_group=NamedUserGroup.objects.filter( + name=group_name, realm=OuterRef("id"), is_system_group=True + ).values("pk") + ) + + +class Migration(migrations.Migration): + atomic = False + + dependencies = [ + ("zerver", "0611_realm_can_move_messages_between_channels_group"), + ] + + operations = [ + migrations.RunPython( + set_default_value_for_can_move_messages_between_channels_group, + elidable=True, + reverse_code=migrations.RunPython.noop, + ) + ] diff --git a/zerver/migrations/0613_alter_realm_can_move_messages_between_channels_group.py b/zerver/migrations/0613_alter_realm_can_move_messages_between_channels_group.py new file mode 100644 index 0000000000..bb4ce19f02 --- /dev/null +++ b/zerver/migrations/0613_alter_realm_can_move_messages_between_channels_group.py @@ -0,0 +1,22 @@ +# Generated by Django 5.0.9 on 2024-10-12 13:54 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0612_set_default_value_for_can_move_messages_between_channels_group"), + ] + + operations = [ + migrations.AlterField( + model_name="realm", + name="can_move_messages_between_channels_group", + field=models.ForeignKey( + on_delete=django.db.models.deletion.RESTRICT, + related_name="+", + to="zerver.usergroup", + ), + ), + ] diff --git a/zerver/models/realms.py b/zerver/models/realms.py index d56c49d114..b7f0af6c3a 100644 --- a/zerver/models/realms.py +++ b/zerver/models/realms.py @@ -359,6 +359,11 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub default=POLICY_MEMBERS_ONLY ) + # UserGroup which is allowed to move messages between streams. + can_move_messages_between_channels_group = models.ForeignKey( + "UserGroup", on_delete=models.RESTRICT, related_name="+" + ) + # Global policy for who is allowed to use wildcard mentions in # streams with a large number of subscribers. Anyone can use # wildcard mentions in small streams regardless of this setting. @@ -781,6 +786,15 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub default_group_name=SystemGroups.OWNERS, id_field_name="can_manage_all_groups_id", ), + can_move_messages_between_channels_group=GroupPermissionSetting( + require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, + allow_internet_group=False, + allow_owners_group=False, + allow_nobody_group=True, + allow_everyone_group=False, + default_group_name=SystemGroups.MEMBERS, + id_field_name="can_move_messages_between_channels_group_id", + ), direct_message_initiator_group=GroupPermissionSetting( require_system_group=not settings.ALLOW_GROUP_VALUED_SETTINGS, allow_internet_group=False, @@ -810,6 +824,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub "can_delete_any_message_group", "can_delete_own_message_group", "can_manage_all_groups", + "can_move_messages_between_channels_group", "direct_message_initiator_group", "direct_message_permission_group", ] @@ -1206,6 +1221,8 @@ def get_realm_with_settings(realm_id: int) -> Realm: "can_delete_own_message_group__named_user_group", "can_manage_all_groups", "can_manage_all_groups__named_user_group", + "can_move_messages_between_channels_group", + "can_move_messages_between_channels_group__named_user_group", "direct_message_initiator_group", "direct_message_initiator_group__named_user_group", "direct_message_permission_group", diff --git a/zerver/models/users.py b/zerver/models/users.py index 9c123d06ee..81e1fbc257 100644 --- a/zerver/models/users.py +++ b/zerver/models/users.py @@ -823,6 +823,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings): "can_delete_any_message_group", "can_delete_own_message_group", "can_manage_all_groups", + "can_move_messages_between_channels_group", "create_multiuse_invite_group", "direct_message_initiator_group", "direct_message_permission_group", @@ -898,7 +899,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin, UserBaseSettings): return self.has_permission("create_multiuse_invite_group") def can_move_messages_between_streams(self) -> bool: - return self.has_permission("move_messages_between_streams_policy") + return self.has_permission("can_move_messages_between_channels_group") def can_create_user_groups(self) -> bool: return self.has_permission("can_create_groups") diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index a589ba2083..dd507e16c5 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -4457,6 +4457,20 @@ paths: setting controlled this permission; `true` corresponded to `Everyone`, and `false` to `Admins`. - $ref: "#/components/schemas/GroupSettingValue" + can_move_messages_between_channels_group: + allOf: + - description: | + A [group-setting value](/api/group-setting-values) defining the set of + users who have permission to move messages from one channel to another + in the organization. + + **Changes**: New in Zulip 10.0 (feature level 310). Previously, this + permission was controlled by the enum `move_messages_between_streams_policy`. + Values were 1=Members, 2=Admins, 3=Full members, 4=Moderators, 6=Nobody. + + In Zulip 7.0 (feature level 159), `Nobody` was added as an option to + `move_messages_between_streams_policy` enum. + - $ref: "#/components/schemas/GroupSettingValue" can_manage_all_groups: allOf: - $ref: "#/components/schemas/GroupSettingValue" @@ -8265,8 +8279,8 @@ paths: linked documentation on when users are allowed to update messages are: - `allow_message_editing` + - `can_move_messages_between_channels_group` - `edit_topic_policy` - - `move_messages_between_streams_policy` - `message_content_edit_limit_seconds` - `move_messages_within_stream_limit_seconds` - `move_messages_between_streams_limit_seconds` @@ -8276,7 +8290,11 @@ paths: of the [`realm op: update_dict`](/api/get-events#realm-update_dict) event in [`GET /events`](/api/get-events). - **Changes**: Prior to Zulip 7.0 (feature level 172), anyone could add a + **Changes**: In Zulip 10.0 (feature level 310), `move_messages_between_streams_policy` + was removed and replaced by `can_move_messages_between_channels_group` + realm setting. + + Prior to Zulip 7.0 (feature level 172), anyone could add a topic to channel messages without a topic, regardless of the organization's [topic editing permissions](/help/restrict-moving-messages). As of this feature level, messages without topics have the same restrictions for @@ -16281,6 +16299,22 @@ paths: setting controlled this permission; `true` corresponded to `Everyone`, and `false` to `Admins`. - $ref: "#/components/schemas/GroupSettingValue" + realm_can_move_messages_between_channels_group: + allOf: + - description: | + Present if `realm` is present in `fetch_event_types`. + + A [group-setting value](/api/group-setting-values) defining the set of + users who have permission to move messages from one channel to another + in the organization. + + **Changes**: New in Zulip 10.0 (feature level 310). Previously, this + permission was controlled by the enum `move_messages_between_streams_policy`. + Values were 1=Members, 2=Admins, 3=Full members, 4=Moderators, 6=Nobody. + + In Zulip 7.0 (feature level 159), `Nobody` was added as an option to + `move_messages_between_streams_policy` enum. + - $ref: "#/components/schemas/GroupSettingValue" realm_bot_creation_policy: type: integer description: | diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 6abd363765..0c6e1846f9 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -137,6 +137,7 @@ class HomeTest(ZulipTestCase): "realm_can_delete_any_message_group", "realm_can_delete_own_message_group", "realm_can_manage_all_groups", + "realm_can_move_messages_between_channels_group", "realm_create_multiuse_invite_group", "realm_create_private_stream_policy", "realm_create_public_stream_policy", diff --git a/zerver/tests/test_message_move_stream.py b/zerver/tests/test_message_move_stream.py index 4f82a12a00..3a2fcc425d 100644 --- a/zerver/tests/test_message_move_stream.py +++ b/zerver/tests/test_message_move_stream.py @@ -2,19 +2,21 @@ from datetime import timedelta import orjson -from zerver.actions.realm_settings import do_set_realm_property +from zerver.actions.message_delete import do_delete_messages +from zerver.actions.realm_settings import ( + do_change_realm_permission_group_setting, + do_set_realm_property, +) from zerver.actions.streams import do_change_stream_post_policy +from zerver.actions.user_groups import check_add_user_group from zerver.actions.users import do_change_user_role from zerver.lib.message import has_message_access from zerver.lib.test_classes import ZulipTestCase, get_topic_messages from zerver.lib.test_helpers import queries_captured from zerver.lib.url_encoding import near_stream_message_url -from zerver.models import Message, Stream, UserMessage, UserProfile -from zerver.models.realms import ( - EditTopicPolicyEnum, - MoveMessagesBetweenStreamsPolicyEnum, - get_realm, -) +from zerver.models import Message, NamedUserGroup, Stream, UserMessage, UserProfile +from zerver.models.groups import SystemGroups +from zerver.models.realms import EditTopicPolicyEnum, get_realm from zerver.models.streams import get_stream @@ -49,8 +51,18 @@ class MessageMoveStreamTest(ZulipTestCase): user_profile.save(update_fields=["default_language"]) self.login(user_email) - stream = self.make_stream(old_stream) - stream_to = self.make_stream(new_stream) + try: + stream = get_stream(old_stream, user_profile.realm) + messages = get_topic_messages(user_profile, stream, "test") + do_delete_messages(user_profile.realm, messages, acting_user=None) + except Stream.DoesNotExist: + stream = self.make_stream(old_stream) + try: + stream_to = get_stream(new_stream, user_profile.realm) + messages = get_topic_messages(user_profile, stream_to, "test") + do_delete_messages(user_profile.realm, messages, acting_user=None) + except Stream.DoesNotExist: + stream_to = self.make_stream(new_stream) self.subscribe(user_profile, stream.name) self.subscribe(user_profile, stream_to.name) msg_id = self.send_stream_message( @@ -104,16 +116,21 @@ class MessageMoveStreamTest(ZulipTestCase): def test_change_all_propagate_mode_for_moving_old_messages(self) -> None: user_profile = self.example_user("hamlet") + realm = user_profile.realm id1 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1") id2 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1") id3 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1") id4 = self.send_stream_message(user_profile, "Denmark", topic_name="topic1") self.send_stream_message(user_profile, "Denmark", topic_name="topic1") - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.MEMBERS_ONLY, + members_system_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=realm, is_system_group=True + ) + + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + members_system_group, acting_user=None, ) @@ -784,13 +801,16 @@ class MessageMoveStreamTest(ZulipTestCase): ) def test_move_message_between_streams_policy_setting(self) -> None: - (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( - "othello", "old_stream_1", "new_stream_1", "test" - ) - - def check_move_message_according_to_policy(role: int, expect_fail: bool = False) -> None: - do_change_user_role(user_profile, role, acting_user=None) + othello = self.example_user("othello") + cordelia = self.example_user("cordelia") + realm = othello.realm + def check_move_message_according_to_permission( + username: str, expect_fail: bool = False + ) -> None: + (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( + username, "old_stream", "new_stream", "test" + ) result = self.client_patch( "/json/messages/" + str(msg_id), { @@ -812,77 +832,117 @@ class MessageMoveStreamTest(ZulipTestCase): messages = get_topic_messages(user_profile, new_stream, "test") self.assert_length(messages, 4) - # Check sending messages when policy is MoveMessagesBetweenStreamsPolicyEnum.NOBODY. - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.NOBODY, - acting_user=None, + administrators_system_group = NamedUserGroup.objects.get( + name=SystemGroups.ADMINISTRATORS, realm=realm, is_system_group=True ) - check_move_message_according_to_policy(UserProfile.ROLE_REALM_OWNER, expect_fail=True) - check_move_message_according_to_policy( - UserProfile.ROLE_REALM_ADMINISTRATOR, expect_fail=True + full_members_system_group = NamedUserGroup.objects.get( + name=SystemGroups.FULL_MEMBERS, realm=realm, is_system_group=True + ) + members_system_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=realm, is_system_group=True + ) + moderators_system_group = NamedUserGroup.objects.get( + name=SystemGroups.MODERATORS, realm=realm, is_system_group=True + ) + nobody_system_group = NamedUserGroup.objects.get( + name=SystemGroups.NOBODY, realm=realm, is_system_group=True ) - # Check sending messages when policy is MoveMessagesBetweenStreamsPolicyEnum.ADMINS_ONLY. - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.ADMINS_ONLY, + # Check sending messages when nobody is allowed to move messages. + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + nobody_system_group, acting_user=None, ) - check_move_message_according_to_policy(UserProfile.ROLE_MODERATOR, expect_fail=True) - check_move_message_according_to_policy(UserProfile.ROLE_REALM_ADMINISTRATOR) + check_move_message_according_to_permission("desdemona", expect_fail=True) + check_move_message_according_to_permission("iago", expect_fail=True) - (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( - "othello", "old_stream_2", "new_stream_2", "test" - ) - # Check sending messages when policy is MoveMessagesBetweenStreamsPolicyEnum.MODERATORS_ONLY. - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.MODERATORS_ONLY, + # Check sending messages when only administrators are allowed. + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + administrators_system_group, acting_user=None, ) - check_move_message_according_to_policy(UserProfile.ROLE_MEMBER, expect_fail=True) - check_move_message_according_to_policy(UserProfile.ROLE_MODERATOR) + check_move_message_according_to_permission("shiva", expect_fail=True) + check_move_message_according_to_permission("iago") - (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( - "othello", "old_stream_3", "new_stream_3", "test" - ) - # Check sending messages when policy is MoveMessagesBetweenStreamsPolicyEnum.FULL_MEMBERS_ONLY. - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.FULL_MEMBERS_ONLY, + # Check sending messages when only moderators are allowed. + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + moderators_system_group, acting_user=None, ) - do_set_realm_property( - user_profile.realm, "waiting_period_threshold", 100000, acting_user=None - ) - check_move_message_according_to_policy(UserProfile.ROLE_MEMBER, expect_fail=True) + check_move_message_according_to_permission("cordelia", expect_fail=True) + check_move_message_according_to_permission("shiva") - do_set_realm_property(user_profile.realm, "waiting_period_threshold", 0, acting_user=None) - check_move_message_according_to_policy(UserProfile.ROLE_MEMBER) - - (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( - "othello", "old_stream_4", "new_stream_4", "test" - ) - # Check sending messages when policy is MoveMessagesBetweenStreamsPolicyEnum.MEMBERS_ONLY. - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.MEMBERS_ONLY, + # Check sending messages when full members are allowed. + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + full_members_system_group, acting_user=None, ) - check_move_message_according_to_policy(UserProfile.ROLE_GUEST, expect_fail=True) - check_move_message_according_to_policy(UserProfile.ROLE_MEMBER) + do_set_realm_property(othello.realm, "waiting_period_threshold", 100000, acting_user=None) + check_move_message_according_to_permission("othello", expect_fail=True) + + do_set_realm_property(realm, "waiting_period_threshold", 0, acting_user=None) + check_move_message_according_to_permission("cordelia") + + # Check sending messages when members are allowed. + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + members_system_group, + acting_user=None, + ) + check_move_message_according_to_permission("polonius", expect_fail=True) + check_move_message_according_to_permission("cordelia") + + # Test for checking setting for non-system user group. + user_group = check_add_user_group( + realm, "new_group", [othello, cordelia], acting_user=othello + ) + do_change_realm_permission_group_setting( + realm, "can_move_messages_between_channels_group", user_group, acting_user=None + ) + + # Othello and Cordelia are in the allowed user group, so can move messages. + check_move_message_according_to_permission("othello") + check_move_message_according_to_permission("cordelia") + + # Iago is not in the allowed user group, so cannot move messages. + check_move_message_according_to_permission("iago", expect_fail=True) + + # Test for checking the setting for anonymous user group. + anonymous_user_group = self.create_or_update_anonymous_group_for_setting( + [othello], + [administrators_system_group], + ) + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + anonymous_user_group, + acting_user=None, + ) + + # Othello is the direct member of the anonymous user group, so can move messages. + check_move_message_according_to_permission("othello") + # Iago is in the `administrators_system_group` subgroup, so can move messages. + check_move_message_according_to_permission("iago") + + # Shiva is not in the anonymous user group, so cannot move messages. + check_move_message_according_to_permission("shiva", expect_fail=True) def test_move_message_to_stream_time_limit(self) -> None: shiva = self.example_user("shiva") iago = self.example_user("iago") cordelia = self.example_user("cordelia") + realm = cordelia.realm + test_stream_1 = self.make_stream("test_stream_1") test_stream_2 = self.make_stream("test_stream_2") @@ -900,10 +960,14 @@ class MessageMoveStreamTest(ZulipTestCase): self.send_stream_message(cordelia, test_stream_1.name, topic_name="test", content="third") - do_set_realm_property( - cordelia.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.MEMBERS_ONLY, + members_system_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=realm, is_system_group=True + ) + + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + members_system_group, acting_user=None, ) @@ -968,10 +1032,16 @@ class MessageMoveStreamTest(ZulipTestCase): (user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics( "othello", "old_stream_1", "new_stream_1", "test" ) - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.MEMBERS_ONLY, + realm = user_profile.realm + + members_system_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=realm, is_system_group=True + ) + + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + members_system_group, acting_user=None, ) @@ -1066,10 +1136,14 @@ class MessageMoveStreamTest(ZulipTestCase): realm.save() self.login("cordelia") - do_set_realm_property( - user_profile.realm, - "move_messages_between_streams_policy", - MoveMessagesBetweenStreamsPolicyEnum.MEMBERS_ONLY, + members_system_group = NamedUserGroup.objects.get( + name=SystemGroups.MEMBERS, realm=realm, is_system_group=True + ) + + do_change_realm_permission_group_setting( + realm, + "can_move_messages_between_channels_group", + members_system_group, acting_user=None, ) @@ -1101,7 +1175,7 @@ class MessageMoveStreamTest(ZulipTestCase): "iago", "test move stream", "new stream", "test" ) - with self.assert_database_query_count(51), self.assert_memcached_count(14): + with self.assert_database_query_count(53), self.assert_memcached_count(14): result = self.client_patch( f"/json/messages/{msg_id}", { @@ -1579,12 +1653,6 @@ class MessageMoveStreamTest(ZulipTestCase): to_invite_only=False, ) - def test_can_move_messages_between_streams(self) -> None: - def validation_func(user_profile: UserProfile) -> bool: - return user_profile.can_move_messages_between_streams() - - self.check_has_permission_policies("move_messages_between_streams_policy", validation_func) - def test_move_message_from_private_to_private_with_old_member(self) -> None: admin_user = self.example_user("iago") user_losing_access = self.example_user("cordelia") diff --git a/zerver/tests/test_message_move_topic.py b/zerver/tests/test_message_move_topic.py index 3d0a41a15d..2e1d31e060 100644 --- a/zerver/tests/test_message_move_topic.py +++ b/zerver/tests/test_message_move_topic.py @@ -363,7 +363,7 @@ class MessageMoveTopicTest(ZulipTestCase): set_topic_visibility_policy(desdemona, muted_topics, UserTopic.VisibilityPolicy.MUTED) set_topic_visibility_policy(cordelia, muted_topics, UserTopic.VisibilityPolicy.MUTED) - with self.assert_database_query_count(27): + with self.assert_database_query_count(29): check_update_message( user_profile=desdemona, message_id=message_id, @@ -393,7 +393,7 @@ class MessageMoveTopicTest(ZulipTestCase): ] set_topic_visibility_policy(desdemona, muted_topics, UserTopic.VisibilityPolicy.MUTED) set_topic_visibility_policy(cordelia, muted_topics, UserTopic.VisibilityPolicy.MUTED) - with self.assert_database_query_count(33): + with self.assert_database_query_count(34): check_update_message( user_profile=desdemona, message_id=message_id, @@ -426,7 +426,7 @@ class MessageMoveTopicTest(ZulipTestCase): set_topic_visibility_policy(desdemona, muted_topics, UserTopic.VisibilityPolicy.MUTED) set_topic_visibility_policy(cordelia, muted_topics, UserTopic.VisibilityPolicy.MUTED) - with self.assert_database_query_count(27): + with self.assert_database_query_count(29): check_update_message( user_profile=desdemona, message_id=message_id, @@ -449,7 +449,7 @@ class MessageMoveTopicTest(ZulipTestCase): second_message_id = self.send_stream_message( hamlet, stream_name, topic_name="changed topic name", content="Second message" ) - with self.assert_database_query_count(22): + with self.assert_database_query_count(23): check_update_message( user_profile=desdemona, message_id=second_message_id, diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index 5f431fd4c8..a6a0a47d6c 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -62,12 +62,7 @@ from zerver.models import ( ) from zerver.models.groups import SystemGroups from zerver.models.realm_audit_logs import AuditLogEventType -from zerver.models.realms import ( - CommonPolicyEnum, - InviteToRealmPolicyEnum, - MoveMessagesBetweenStreamsPolicyEnum, - get_realm, -) +from zerver.models.realms import CommonPolicyEnum, InviteToRealmPolicyEnum, get_realm from zerver.models.streams import get_stream from zerver.models.users import get_system_bot, get_user_profile_by_id @@ -119,16 +114,13 @@ class RealmTest(ZulipTestCase): self.assertEqual(realm.can_create_public_channel_group_id, admins_group.id) self.assertEqual(realm.invite_to_realm_policy, InviteToRealmPolicyEnum.ADMINS_ONLY) - self.assertEqual( - realm.move_messages_between_streams_policy, - MoveMessagesBetweenStreamsPolicyEnum.MODERATORS_ONLY, - ) self.assertEqual(realm.invite_to_stream_policy, CommonPolicyEnum.MODERATORS_ONLY) realm = get_realm("test_education_non_profit") moderators_group = NamedUserGroup.objects.get( name=SystemGroups.MODERATORS, realm=realm, is_system_group=True ) self.assertEqual(realm.can_create_groups.id, moderators_group.id) + self.assertEqual(realm.can_move_messages_between_channels_group.id, moderators_group.id) def test_permission_for_education_for_profit_organization(self) -> None: realm = do_create_realm( @@ -143,16 +135,13 @@ class RealmTest(ZulipTestCase): self.assertEqual(realm.can_create_public_channel_group_id, admins_group.id) self.assertEqual(realm.invite_to_realm_policy, InviteToRealmPolicyEnum.ADMINS_ONLY) - self.assertEqual( - realm.move_messages_between_streams_policy, - MoveMessagesBetweenStreamsPolicyEnum.MODERATORS_ONLY, - ) self.assertEqual(realm.invite_to_stream_policy, CommonPolicyEnum.MODERATORS_ONLY) realm = get_realm("test_education_for_profit") moderators_group = NamedUserGroup.objects.get( name=SystemGroups.MODERATORS, realm=realm, is_system_group=True ) self.assertEqual(realm.can_create_groups.id, moderators_group.id) + self.assertEqual(realm.can_move_messages_between_channels_group.id, moderators_group.id) def test_realm_enable_spectator_access(self) -> None: realm = do_create_realm( diff --git a/zerver/views/realm.py b/zerver/views/realm.py index 7872260e7c..769cf34949 100644 --- a/zerver/views/realm.py +++ b/zerver/views/realm.py @@ -145,6 +145,7 @@ def update_realm( can_create_private_channel_group: Json[GroupSettingChangeRequest] | None = None, can_create_web_public_channel_group: Json[GroupSettingChangeRequest] | None = None, can_manage_all_groups: Json[GroupSettingChangeRequest] | None = None, + can_move_messages_between_channels_group: Json[GroupSettingChangeRequest] | None = None, direct_message_initiator_group: Json[GroupSettingChangeRequest] | None = None, direct_message_permission_group: Json[GroupSettingChangeRequest] | None = None, invite_to_stream_policy: Json[CommonPolicyEnum] | None = None,