From 73f11853ec4eec28989370e811040a58098d3eba Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Sat, 17 Sep 2022 12:05:29 +0530 Subject: [PATCH] streams: Allow setting can_remove_subscribers_group_id while creating streams. This commit adds API support to set can_remove_subscribers_group setting when creating streams. --- api_docs/changelog.md | 4 ++ version.py | 2 +- zerver/lib/streams.py | 14 +++++-- zerver/openapi/zulip.yaml | 1 + zerver/tests/test_subs.py | 85 +++++++++++++++++++++++++++++++++++++-- zerver/views/streams.py | 16 ++++++++ 6 files changed, 113 insertions(+), 9 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index f7ee3acf37..9126b993a5 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -25,6 +25,10 @@ format used by the Zulip server that they are interacting with. * [`PATCH /streams/{stream_id}`](/api/update-stream): Added `can_remove_subscribers_group_id` parameter to support changing `can_remove_subscribers_group` setting. +* [`POST /users/me/subscriptions`](/api/subscribe): Added + `can_remove_subscribers_group_id` parameter to set + `can_remove_subscribers_group` setting while creating + streams. **Feature level 160** diff --git a/version.py b/version.py index 71f5f85f4c..6b1d93b42c 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 159 +API_FEATURE_LEVEL = 161 # 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/streams.py b/zerver/lib/streams.py index 6043463771..c2788b19ae 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -62,6 +62,7 @@ class StreamDict(TypedDict, total=False): stream_post_policy: int history_public_to_subscribers: Optional[bool] message_retention_days: Optional[int] + can_remove_subscribers_group: Optional[UserGroup] def get_stream_permission_policy_name( @@ -125,15 +126,19 @@ def create_stream_if_needed( history_public_to_subscribers: Optional[bool] = None, stream_description: str = "", message_retention_days: Optional[int] = None, + can_remove_subscribers_group: Optional[UserGroup] = None, acting_user: Optional[UserProfile] = None, ) -> Tuple[Stream, bool]: history_public_to_subscribers = get_default_value_for_history_public_to_subscribers( realm, invite_only, history_public_to_subscribers ) - administrators_user_group = UserGroup.objects.get( - name=UserGroup.ADMINISTRATORS_GROUP_NAME, is_system_group=True, realm=realm - ) + if can_remove_subscribers_group is None: + can_remove_subscribers_group = UserGroup.objects.get( + name=UserGroup.ADMINISTRATORS_GROUP_NAME, is_system_group=True, realm=realm + ) + + assert can_remove_subscribers_group is not None with transaction.atomic(): (stream, created) = Stream.objects.get_or_create( realm=realm, @@ -147,7 +152,7 @@ def create_stream_if_needed( history_public_to_subscribers=history_public_to_subscribers, is_in_zephyr_realm=realm.is_zephyr_mirror_realm, message_retention_days=message_retention_days, - can_remove_subscribers_group=administrators_user_group, + can_remove_subscribers_group=can_remove_subscribers_group, ), ) @@ -196,6 +201,7 @@ def create_streams_if_needed( history_public_to_subscribers=stream_dict.get("history_public_to_subscribers"), stream_description=stream_dict.get("description", ""), message_retention_days=stream_dict.get("message_retention_days", None), + can_remove_subscribers_group=stream_dict.get("can_remove_subscribers_group", None), acting_user=acting_user, ) diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 1d21a211e9..0520d84ae6 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -7855,6 +7855,7 @@ paths: - $ref: "#/components/parameters/HistoryPublicToSubscribers" - $ref: "#/components/parameters/StreamPostPolicy" - $ref: "#/components/parameters/MessageRetentionDays" + - $ref: "#/components/parameters/CanRemoveSubscribersGroupId" responses: "200": description: Success. diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 5171543e33..856d93dff4 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -269,6 +269,9 @@ class TestCreateStreams(ZulipTestCase): self.assertTrue(self.example_user("desdemona").id in events[0]["users"]) self.assertEqual(events[0]["event"]["streams"][0]["name"], "Private stream") + moderators_system_group = UserGroup.objects.get( + name="@role:moderators", realm=realm, is_system_group=True + ) new_streams, existing_streams = create_streams_if_needed( realm, [ @@ -278,6 +281,7 @@ class TestCreateStreams(ZulipTestCase): "invite_only": True, "stream_post_policy": Stream.STREAM_POST_POLICY_ADMINS, "message_retention_days": -1, + "can_remove_subscribers_group": moderators_system_group, } for (stream_name, stream_description) in zip(stream_names, stream_descriptions) ], @@ -294,6 +298,7 @@ class TestCreateStreams(ZulipTestCase): self.assertTrue(stream.invite_only) self.assertTrue(stream.stream_post_policy == Stream.STREAM_POST_POLICY_ADMINS) self.assertTrue(stream.message_retention_days == -1) + self.assertEqual(stream.can_remove_subscribers_group.id, moderators_system_group.id) new_streams, existing_streams = create_streams_if_needed( realm, @@ -466,6 +471,78 @@ class TestCreateStreams(ZulipTestCase): # But it should be marked as read for Iago, the stream creator. self.assert_length(iago_unread_messages, 0) + def test_can_remove_subscribers_group_on_stream_creation(self) -> None: + user = self.example_user("hamlet") + realm = user.realm + self.login_user(user) + moderators_system_group = UserGroup.objects.get( + name="@role:moderators", realm=realm, is_system_group=True + ) + admins_system_group = UserGroup.objects.get( + name="@role:administrators", realm=realm, is_system_group=True + ) + + post_data = { + "subscriptions": orjson.dumps( + [{"name": "new_stream1", "description": "First new stream"}] + ).decode(), + "can_remove_subscribers_group_id": orjson.dumps(moderators_system_group.id).decode(), + } + result = self.api_post(user, "/api/v1/users/me/subscriptions", post_data, subdomain="zulip") + self.assert_json_success(result) + stream = get_stream("new_stream1", realm) + self.assertEqual(stream.can_remove_subscribers_group.id, moderators_system_group.id) + + post_data = { + "subscriptions": orjson.dumps( + [{"name": "new_stream2", "description": "Second new stream"}] + ).decode(), + } + result = self.api_post(user, "/api/v1/users/me/subscriptions", post_data, subdomain="zulip") + self.assert_json_success(result) + stream = get_stream("new_stream2", realm) + self.assertEqual(stream.can_remove_subscribers_group.id, admins_system_group.id) + + hamletcharacters_group = UserGroup.objects.get(name="hamletcharacters", realm=realm) + post_data = { + "subscriptions": orjson.dumps( + [{"name": "new_stream3", "description": "Third new stream"}] + ).decode(), + "can_remove_subscribers_group_id": orjson.dumps(hamletcharacters_group.id).decode(), + } + result = self.api_post(user, "/api/v1/users/me/subscriptions", post_data, subdomain="zulip") + self.assert_json_error( + result, "'can_remove_subscribers_group' must be a system user group." + ) + + internet_group = UserGroup.objects.get( + name="@role:internet", is_system_group=True, realm=realm + ) + post_data = { + "subscriptions": orjson.dumps( + [{"name": "new_stream3", "description": "Third new stream"}] + ).decode(), + "can_remove_subscribers_group_id": orjson.dumps(internet_group.id).decode(), + } + result = self.api_post(user, "/api/v1/users/me/subscriptions", post_data, subdomain="zulip") + self.assert_json_error( + result, + "'can_remove_subscribers_group' setting cannot be set to '@role:internet' group.", + ) + + owners_group = UserGroup.objects.get(name="@role:owners", is_system_group=True, realm=realm) + post_data = { + "subscriptions": orjson.dumps( + [{"name": "new_stream3", "description": "Third new stream"}] + ).decode(), + "can_remove_subscribers_group_id": orjson.dumps(owners_group.id).decode(), + } + result = self.api_post(user, "/api/v1/users/me/subscriptions", post_data, subdomain="zulip") + self.assert_json_error( + result, + "'can_remove_subscribers_group' setting cannot be set to '@role:owners' group.", + ) + class RecipientTest(ZulipTestCase): def test_recipient(self) -> None: @@ -4325,7 +4402,7 @@ class SubscriptionAPITest(ZulipTestCase): # Now add ourselves with self.tornado_redirected_to_list(events, expected_num_events=2): - with self.assert_database_query_count(12): + with self.assert_database_query_count(13): self.common_subscribe_to_streams( self.test_user, streams_to_sub, @@ -4650,7 +4727,7 @@ class SubscriptionAPITest(ZulipTestCase): # any tornado subscription events events: List[Mapping[str, Any]] = [] with self.tornado_redirected_to_list(events, expected_num_events=0): - with self.assert_database_query_count(4): + with self.assert_database_query_count(5): self.common_subscribe_to_streams( mit_user, stream_names, @@ -4698,7 +4775,7 @@ class SubscriptionAPITest(ZulipTestCase): test_user_ids = [user.id for user in test_users] - with self.assert_database_query_count(19): + with self.assert_database_query_count(20): with cache_tries_captured() as cache_tries: with mock.patch("zerver.views.streams.send_messages_for_new_subscribers"): self.common_subscribe_to_streams( @@ -5611,7 +5688,7 @@ class GetSubscribersTest(ZulipTestCase): polonius.id, ] - with self.assert_database_query_count(46): + with self.assert_database_query_count(47): self.common_subscribe_to_streams( self.user_profile, streams, diff --git a/zerver/views/streams.py b/zerver/views/streams.py index f8164154c9..ca1ba58eed 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -99,6 +99,7 @@ from zerver.lib.validator import ( from zerver.models import ( Realm, Stream, + UserGroup, UserMessage, UserProfile, get_active_user, @@ -555,6 +556,7 @@ def add_subscriptions_backend( message_retention_days: Union[str, int] = REQ( json_validator=check_string_or_int, default=RETENTION_DEFAULT ), + can_remove_subscribers_group_id: Optional[int] = REQ(json_validator=check_int, default=None), announce: bool = REQ(json_validator=check_bool, default=False), principals: Union[Sequence[str], Sequence[int]] = REQ( json_validator=check_principals, @@ -565,6 +567,19 @@ def add_subscriptions_backend( realm = user_profile.realm stream_dicts = [] color_map = {} + + if can_remove_subscribers_group_id is not None: + can_remove_subscribers_group = access_user_group_for_setting( + can_remove_subscribers_group_id, + user_profile, + setting_name="can_remove_subscribers_group", + require_system_group=True, + ) + else: + can_remove_subscribers_group = UserGroup.objects.get( + name="@role:administrators", realm=user_profile.realm, is_system_group=True + ) + for stream_dict in streams_raw: # 'color' field is optional # check for its presence in the streams_raw first @@ -585,6 +600,7 @@ def add_subscriptions_backend( stream_dict_copy["message_retention_days"] = parse_message_retention_days( message_retention_days, Stream.MESSAGE_RETENTION_SPECIAL_VALUES_MAP ) + stream_dict_copy["can_remove_subscribers_group"] = can_remove_subscribers_group stream_dicts.append(stream_dict_copy)