From 2f34e6d24ca06f5109daf38fcd229f0f42813351 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Wed, 14 May 2025 20:08:47 +0530 Subject: [PATCH] streams: Add API support to update folder of a stream. Fixes part of #31972. --- api_docs/unmerged.d/ZF-57cda6.md | 5 ++ zerver/actions/streams.py | 31 ++++++++++++ zerver/lib/event_schema.py | 4 ++ zerver/models/realm_audit_logs.py | 1 + zerver/openapi/zulip.yaml | 19 +++++++- zerver/tests/test_audit_log.py | 58 ++++++++++++++++++++++ zerver/tests/test_events.py | 14 ++++++ zerver/tests/test_subs.py | 81 +++++++++++++++++++++++++++++++ zerver/views/streams.py | 10 ++++ 9 files changed, 222 insertions(+), 1 deletion(-) create mode 100644 api_docs/unmerged.d/ZF-57cda6.md diff --git a/api_docs/unmerged.d/ZF-57cda6.md b/api_docs/unmerged.d/ZF-57cda6.md new file mode 100644 index 0000000000..12128d4871 --- /dev/null +++ b/api_docs/unmerged.d/ZF-57cda6.md @@ -0,0 +1,5 @@ +* [`PATCH /streams/{stream_id}`](/api/update-stream): Added support for updating +folder to which the channel belongs. + +* [`GET /events`](/api/get-events): `value` field in `stream/update` events can +have `null` when channel is removed from a folder. diff --git a/zerver/actions/streams.py b/zerver/actions/streams.py index 0491db4f18..f94942fb2e 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -66,6 +66,7 @@ from zerver.models import ( ArchivedAttachment, Attachment, ChannelEmailAddress, + ChannelFolder, DefaultStream, DefaultStreamGroup, Message, @@ -1827,3 +1828,33 @@ def do_change_stream_group_based_setting( # object would be created if the setting is later set to # a combination of users and groups. old_user_group.delete() + + +@transaction.atomic(durable=True) +def do_change_stream_folder( + stream: Stream, folder: ChannelFolder | None, *, acting_user: UserProfile +) -> None: + old_folder_id = stream.folder_id + stream.folder = folder + stream.save(update_fields=["folder"]) + RealmAuditLog.objects.create( + realm=stream.realm, + acting_user=acting_user, + modified_stream=stream, + event_type=AuditLogEventType.CHANNEL_FOLDER_CHANGED, + event_time=timezone_now(), + extra_data={ + RealmAuditLog.OLD_VALUE: old_folder_id, + RealmAuditLog.NEW_VALUE: stream.folder_id, + }, + ) + + event = dict( + op="update", + type="stream", + property="folder_id", + value=stream.folder_id, + stream_id=stream.id, + name=stream.name, + ) + send_event_on_commit(stream.realm, event, can_access_stream_metadata_user_ids(stream)) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 12369510fc..0beee72ad6 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -545,6 +545,7 @@ def check_stream_update( "stream_id", "first_message_id", "is_archived", + "folder_id", } if prop == "description": @@ -584,6 +585,9 @@ def check_stream_update( elif prop == "is_archived": assert extra_keys == set() assert isinstance(value, bool) + elif prop == "folder_id": + assert extra_keys == set() + assert value is None or isinstance(value, int) else: raise AssertionError(f"Unknown property: {prop}") diff --git a/zerver/models/realm_audit_logs.py b/zerver/models/realm_audit_logs.py index 4a5e65a1e0..d00589b311 100644 --- a/zerver/models/realm_audit_logs.py +++ b/zerver/models/realm_audit_logs.py @@ -96,6 +96,7 @@ class AuditLogEventType(IntEnum): CHANNEL_MESSAGE_RETENTION_DAYS_CHANGED = 605 CHANNEL_PROPERTY_CHANGED = 607 CHANNEL_GROUP_BASED_SETTING_CHANGED = 608 + CHANNEL_FOLDER_CHANGED = 609 USER_GROUP_CREATED = 701 USER_GROUP_DELETED = 702 diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 52a7429262..d5bd4ea129 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1495,7 +1495,10 @@ paths: description: | The new value of the changed property. - **Changes**: Starting with Zulip 10.0 (feature level 320), this + **Changes**: Starting with Zulip 11.0 (feature level ZF-57cda6), + this value can be `null` when a channel is removed from the folder. + + Starting with Zulip 10.0 (feature level 320), this field can be an object for `can_remove_subscribers_group` property, which is a [group-setting value][setting-values], when the setting is set to a combination of users and groups. @@ -1524,6 +1527,7 @@ paths: - type: integer - type: boolean - type: string + nullable: true rendered_description: type: string description: | @@ -21372,6 +21376,17 @@ paths: **Changes**: New in Zulip 11.0 (feature level 388). type: boolean example: true + folder_id: + description: | + ID of the new folder to which the channel should belong. + + It can be `None` if the user wants to just remove the channel + from its existing folder. + + **Changes**: New in Zulip 11.0 (feature level ZF-57cda6). + type: integer + nullable: true + example: 1 can_add_subscribers_group: allOf: - description: | @@ -21522,6 +21537,8 @@ paths: contentType: application/json is_default_stream: contentType: application/json + folder_id: + contentType: application/json can_add_subscribers_group: contentType: application/json can_remove_subscribers_group: diff --git a/zerver/tests/test_audit_log.py b/zerver/tests/test_audit_log.py index 039ff12a9d..6bb771f4d6 100644 --- a/zerver/tests/test_audit_log.py +++ b/zerver/tests/test_audit_log.py @@ -52,6 +52,7 @@ from zerver.actions.realm_settings import ( from zerver.actions.streams import ( bulk_add_subscriptions, bulk_remove_subscriptions, + do_change_stream_folder, do_change_subscription_property, do_deactivate_stream, do_rename_stream, @@ -824,6 +825,63 @@ class TestRealmAuditLog(ZulipTestCase): ) self.assertEqual(stream.name, "updated name") + def test_change_stream_folder(self) -> None: + user = self.example_user("iago") + stream = self.make_stream("test", user.realm) + frontend_folder = check_add_channel_folder("Frontend", "", acting_user=user) + backend_folder = check_add_channel_folder("Backend", "", acting_user=user) + + now = timezone_now() + do_change_stream_folder(stream, frontend_folder, acting_user=user) + self.assertEqual( + RealmAuditLog.objects.filter( + realm=user.realm, + event_type=AuditLogEventType.CHANNEL_FOLDER_CHANGED, + event_time__gte=now, + acting_user=user, + modified_stream=stream, + extra_data={ + RealmAuditLog.OLD_VALUE: None, + RealmAuditLog.NEW_VALUE: frontend_folder.id, + }, + ).count(), + 1, + ) + + now = timezone_now() + do_change_stream_folder(stream, backend_folder, acting_user=user) + self.assertEqual( + RealmAuditLog.objects.filter( + realm=user.realm, + event_type=AuditLogEventType.CHANNEL_FOLDER_CHANGED, + event_time__gte=now, + acting_user=user, + modified_stream=stream, + extra_data={ + RealmAuditLog.OLD_VALUE: frontend_folder.id, + RealmAuditLog.NEW_VALUE: backend_folder.id, + }, + ).count(), + 1, + ) + + now = timezone_now() + do_change_stream_folder(stream, None, acting_user=user) + self.assertEqual( + RealmAuditLog.objects.filter( + realm=user.realm, + event_type=AuditLogEventType.CHANNEL_FOLDER_CHANGED, + event_time__gte=now, + acting_user=user, + modified_stream=stream, + extra_data={ + RealmAuditLog.OLD_VALUE: backend_folder.id, + RealmAuditLog.NEW_VALUE: None, + }, + ).count(), + 1, + ) + def test_change_user_settings(self) -> None: user = self.example_user("hamlet") value: bool | int | str | Enum diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index a9313aa220..6f7cc4d7f7 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -113,6 +113,7 @@ from zerver.actions.streams import ( bulk_add_subscriptions, bulk_remove_subscriptions, do_change_stream_description, + do_change_stream_folder, do_change_stream_group_based_setting, do_change_stream_message_retention_days, do_change_stream_permission, @@ -4946,6 +4947,19 @@ class SubscribeActionTest(BaseAction): check_stream_update("events[0]", events[0]) check_message("events[1]", events[1]) + channel_folder = check_add_channel_folder("Frontend", "", acting_user=iago) + with self.verify_action(include_subscribers=include_subscribers) as events: + do_change_stream_folder(stream, channel_folder, acting_user=iago) + check_stream_update("events[0]", events[0]) + self.assertEqual(events[0]["property"], "folder_id") + self.assertEqual(events[0]["value"], channel_folder.id) + + with self.verify_action(include_subscribers=include_subscribers) as events: + do_change_stream_folder(stream, None, acting_user=iago) + check_stream_update("events[0]", events[0]) + self.assertEqual(events[0]["property"], "folder_id") + self.assertIsNone(events[0]["value"]) + # Update stream privacy - make stream web-public with self.verify_action(include_subscribers=include_subscribers, num_events=2) as events: do_change_stream_permission( diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 20c74c8f95..f7709b8a52 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -3718,6 +3718,87 @@ class StreamAdminTest(ZulipTestCase): ) self.archive_stream(priv_stream, expect_can_subscribe=False) + def test_updating_stream_folder(self) -> None: + iago = self.example_user("iago") + channel_folder = check_add_channel_folder("Frontend", "", acting_user=iago) + stream = self.make_stream("test_stream") + + self.assertIsNone(stream.folder_id) + + self.login("desdemona") + result = self.client_patch( + f"/json/streams/{stream.id}", + {"folder_id": orjson.dumps(channel_folder.id).decode()}, + ) + self.assert_json_success(result) + stream = get_stream("test_stream", iago.realm) + self.assertEqual(stream.folder_id, channel_folder.id) + + result = self.client_patch( + f"/json/streams/{stream.id}", + {"folder_id": orjson.dumps(None).decode()}, + ) + self.assert_json_success(result) + stream = get_stream("test_stream", iago.realm) + self.assertIsNone(stream.folder_id) + + # Test invalid value. + result = self.client_patch( + f"/json/streams/{stream.id}", + {"folder_id": orjson.dumps(99).decode()}, + ) + self.assert_json_error(result, "Invalid channel folder ID") + + def test_permission_to_change_stream_folder(self) -> None: + iago = self.example_user("iago") + hamlet = self.example_user("hamlet") + realm = iago.realm + channel_folder = check_add_channel_folder("Frontend", "", acting_user=iago) + stream = self.make_stream("test_stream") + + self.assertIsNone(stream.folder_id) + + nobody_group = NamedUserGroup.objects.get( + name=SystemGroups.NOBODY, realm=realm, is_system_group=True + ) + do_change_stream_group_based_setting( + stream, + "can_administer_channel_group", + nobody_group, + acting_user=iago, + ) + + result = self.api_patch( + iago, + f"/api/v1/streams/{stream.id}", + {"folder_id": orjson.dumps(channel_folder.id).decode()}, + ) + self.assert_json_success(result) + stream = get_stream("test_stream", realm) + self.assertEqual(stream.folder_id, channel_folder.id) + + result = self.api_patch( + hamlet, + f"/api/v1/streams/{stream.id}", + {"folder_id": orjson.dumps(None).decode()}, + ) + self.assert_json_error(result, "You do not have permission to administer this channel.") + + do_change_stream_group_based_setting( + stream, + "can_administer_channel_group", + UserGroupMembersData(direct_members=[hamlet.id], direct_subgroups=[]), + acting_user=iago, + ) + result = self.api_patch( + hamlet, + f"/api/v1/streams/{stream.id}", + {"folder_id": orjson.dumps(None).decode()}, + ) + self.assert_json_success(result) + stream = get_stream("test_stream", realm) + self.assertIsNone(stream.folder_id) + def attempt_unsubscribe_of_principal( self, target_users: list[UserProfile], diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 67cd9082ca..f84b7abfd4 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -11,6 +11,7 @@ from django.http import HttpRequest, HttpResponse from django.utils.translation import gettext as _ from django.utils.translation import override as override_language from pydantic import BaseModel, Field, Json, NonNegativeInt, StringConstraints, model_validator +from pydantic_partials.sentinels import Missing, MissingType from zerver.actions.default_streams import ( do_add_default_stream, @@ -32,6 +33,7 @@ from zerver.actions.streams import ( bulk_add_subscriptions, bulk_remove_subscriptions, do_change_stream_description, + do_change_stream_folder, do_change_stream_group_based_setting, do_change_stream_message_retention_days, do_change_stream_permission, @@ -277,6 +279,7 @@ def update_stream_backend( can_send_message_group: Json[GroupSettingChangeRequest] | None = None, can_remove_subscribers_group: Json[GroupSettingChangeRequest] | None = None, can_subscribe_group: Json[GroupSettingChangeRequest] | None = None, + folder_id: Json[int | None] | MissingType = Missing, ) -> HttpResponse: # Most settings updates only require metadata access, not content # access. We will check for content access further when and where @@ -417,6 +420,13 @@ def update_stream_backend( check_stream_name_available(user_profile.realm, new_name) do_rename_stream(stream, new_name, user_profile) + if folder_id is not Missing: + assert not isinstance(folder_id, MissingType) + folder: ChannelFolder | None = None + if folder_id is not None: + folder = get_channel_folder_by_id(folder_id, user_profile.realm) + do_change_stream_folder(stream, folder, acting_user=user_profile) + nobody_group = get_system_user_group_by_name(SystemGroups.NOBODY, user_profile.realm_id) request_settings_dict = locals() for setting_name, permission_configuration in Stream.stream_permission_group_settings.items():