diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 9169ce3fa5..cda5e36b15 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -5007,65 +5007,52 @@ def do_change_can_create_users(user_profile: UserProfile, value: bool) -> None: user_profile.save(update_fields=["can_create_users"]) -def do_change_stream_invite_only( - stream: Stream, invite_only: bool, history_public_to_subscribers: Optional[bool] = None -) -> None: - history_public_to_subscribers = get_default_value_for_history_public_to_subscribers( - stream.realm, - invite_only, - history_public_to_subscribers, - ) - stream.invite_only = invite_only - stream.history_public_to_subscribers = history_public_to_subscribers - stream.is_web_public = False - stream.save(update_fields=["invite_only", "history_public_to_subscribers", "is_web_public"]) - event = dict( - op="update", - type="stream", - property="invite_only", - value=invite_only, - history_public_to_subscribers=history_public_to_subscribers, - is_web_public=False, - stream_id=stream.id, - name=stream.name, - ) - send_event(stream.realm, event, can_access_stream_user_ids(stream)) - - -def do_make_stream_web_public(stream: Stream) -> None: - stream.is_web_public = True - stream.invite_only = False - stream.history_public_to_subscribers = True - stream.save(update_fields=["invite_only", "history_public_to_subscribers", "is_web_public"]) - - # We reuse "invite_only" stream update API route here because - # both are similar events and similar UI updates will be required - # by the client to update this property for the user. - event = dict( - op="update", - type="stream", - property="invite_only", - value=False, - history_public_to_subscribers=True, - is_web_public=True, - stream_id=stream.id, - name=stream.name, - ) - send_event(stream.realm, event, can_access_stream_user_ids(stream)) - - def do_change_stream_permission( stream: Stream, + *, invite_only: Optional[bool] = None, history_public_to_subscribers: Optional[bool] = None, is_web_public: Optional[bool] = None, ) -> None: - # TODO: Ideally this would be just merged with do_change_stream_invite_only. + # A note on these assertions: It's possible we'd be better off + # making all callers of this function pass the full set of + # parameters, rather than having default values. Doing so would + # allow us to remove the messy logic below, where we sometimes + # ignore the passed parameters. + # + # But absent such a refactoring, it's important to assert that + # we're not requesting an unsupported configurations. if is_web_public: - do_make_stream_web_public(stream) + assert history_public_to_subscribers is not False + assert invite_only is not True + stream.is_web_public = True + stream.invite_only = False + stream.history_public_to_subscribers = True else: assert invite_only is not None - do_change_stream_invite_only(stream, invite_only, history_public_to_subscribers) + # is_web_public is Falsey + history_public_to_subscribers = get_default_value_for_history_public_to_subscribers( + stream.realm, + invite_only, + history_public_to_subscribers, + ) + stream.invite_only = invite_only + stream.history_public_to_subscribers = history_public_to_subscribers + stream.is_web_public = False + + stream.save(update_fields=["invite_only", "history_public_to_subscribers", "is_web_public"]) + + event = dict( + op="update", + type="stream", + property="invite_only", + value=stream.invite_only, + history_public_to_subscribers=stream.history_public_to_subscribers, + is_web_public=stream.is_web_public, + stream_id=stream.id, + name=stream.name, + ) + send_event(stream.realm, event, can_access_stream_user_ids(stream)) def send_change_stream_post_policy_notification( diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index 837a33bb13..d94a8a2bb7 100644 --- a/zerver/tests/test_bots.py +++ b/zerver/tests/test_bots.py @@ -9,7 +9,7 @@ from django.test import override_settings from zulip_bots.custom_exceptions import ConfigValidationError from zerver.lib.actions import ( - do_change_stream_invite_only, + do_change_stream_permission, do_deactivate_user, do_set_realm_property, ) @@ -420,7 +420,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): user_profile = self.example_user("hamlet") stream = get_stream("Denmark", user_profile.realm) self.subscribe(user_profile, stream.name) - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] @@ -464,7 +464,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) bot_info = { "full_name": "The Bot of Hamlet", @@ -492,7 +492,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") user_profile = self.example_user("hamlet") stream = self.subscribe(user_profile, "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] @@ -536,7 +536,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) self.assert_num_bots_equal(0) bot_info = { @@ -1132,7 +1132,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") user_profile = self.example_user("hamlet") stream = self.subscribe(user_profile, "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) bot_info = { "full_name": "The Bot of Hamlet", @@ -1158,7 +1158,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) bot_info = { "full_name": "The Bot of Hamlet", @@ -1239,7 +1239,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") user_profile = self.example_user("hamlet") stream = self.subscribe(user_profile, "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) bot_info = { "full_name": "The Bot of Hamlet", @@ -1264,7 +1264,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) bot_info = { "full_name": "The Bot of Hamlet", diff --git a/zerver/tests/test_message_flags.py b/zerver/tests/test_message_flags.py index 934fe6337a..a846fd18bb 100644 --- a/zerver/tests/test_message_flags.py +++ b/zerver/tests/test_message_flags.py @@ -5,7 +5,7 @@ import orjson from django.db import connection from django.http import HttpResponse -from zerver.lib.actions import do_change_stream_invite_only +from zerver.lib.actions import do_change_stream_permission from zerver.lib.fix_unreads import fix, fix_unsubscribed from zerver.lib.message import ( MessageDict, @@ -1264,12 +1264,12 @@ class MessageAccessTests(ZulipTestCase): # Guest user can access messages of subscribed private streams if # history is public to subscribers - do_change_stream_invite_only(stream, True, history_public_to_subscribers=True) + do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=True) result = self.change_star(message_id) self.assert_json_success(result) # With history not public to subscribers, they can still see new messages - do_change_stream_invite_only(stream, True, history_public_to_subscribers=False) + do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=False) self.login_user(normal_user) message_id = [ self.send_stream_message(normal_user, stream_name, "test 2"), @@ -1312,7 +1312,7 @@ class MessageAccessTests(ZulipTestCase): self.assert_length(filtered_messages, 1) self.assertEqual(filtered_messages[0].id, message_two_id) - do_change_stream_invite_only(stream, True, history_public_to_subscribers=True) + do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=True) with queries_captured() as queries: filtered_messages = bulk_access_messages(later_subscribed_user, messages, stream=stream) diff --git a/zerver/tests/test_message_topics.py b/zerver/tests/test_message_topics.py index 07f63bdd36..e11273ac79 100644 --- a/zerver/tests/test_message_topics.py +++ b/zerver/tests/test_message_topics.py @@ -1,6 +1,6 @@ from django.utils.timezone import now as timezone_now -from zerver.lib.actions import do_change_stream_invite_only +from zerver.lib.actions import do_change_stream_permission from zerver.lib.test_classes import ZulipTestCase from zerver.models import Message, UserMessage, get_client, get_realm, get_stream @@ -132,7 +132,7 @@ class TopicHistoryTest(ZulipTestCase): ) # Now make stream private, but subscribe cordelia - do_change_stream_invite_only(stream, True) + do_change_stream_permission(stream, invite_only=True) self.subscribe(self.example_user("cordelia"), stream.name) result = self.client_get(endpoint, {}) @@ -234,7 +234,7 @@ class TopicDeleteTest(ZulipTestCase): self.assertEqual(self.get_last_message().id, last_msg_id) # Make stream private with limited history - do_change_stream_invite_only(stream, invite_only=True, history_public_to_subscribers=False) + do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=False) # ADMIN USER subscribed now user_profile = self.example_user("iago") @@ -266,7 +266,7 @@ class TopicDeleteTest(ZulipTestCase): self.assertEqual(self.get_last_message().id, last_msg_id) # Make the stream's history public to subscribers - do_change_stream_invite_only(stream, invite_only=True, history_public_to_subscribers=True) + do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=True) # Delete the topic should now remove all messages result = self.client_post( endpoint, diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index a230209731..66a3fb1360 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -4,11 +4,7 @@ from unittest import mock import orjson from django.http import HttpResponse -from zerver.lib.actions import ( - do_change_stream_invite_only, - do_make_stream_web_public, - notify_reaction_update, -) +from zerver.lib.actions import do_change_stream_permission, notify_reaction_update from zerver.lib.cache import cache_get, to_dict_cache_key_id from zerver.lib.emoji import emoji_name_to_emoji_code from zerver.lib.exceptions import JsonableError @@ -547,7 +543,7 @@ class ReactionEventTest(ZulipTestCase): self.assert_json_success(remove) # Make stream history public to subscribers - do_change_stream_invite_only(stream, False, history_public_to_subscribers=True) + do_change_stream_permission(stream, invite_only=False, history_public_to_subscribers=True) # Since stream history is public to subscribers, reacting to # message_before_id should notify all subscribers: # Iago and Hamlet. @@ -566,7 +562,7 @@ class ReactionEventTest(ZulipTestCase): self.assert_json_success(remove) # Make stream web_public as well. - do_make_stream_web_public(stream) + do_change_stream_permission(stream, is_web_public=True) # For is_web_public streams, events even on old messages # should go to all subscribers, including guests like polonius. with self.tornado_redirected_to_list(events, expected_num_events=1): diff --git a/zerver/views/streams.py b/zerver/views/streams.py index ee4e3cc80e..3c835f8231 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -325,7 +325,10 @@ def update_stream_backend( if is_private is not None or is_web_public is not None: do_change_stream_permission( - stream, is_private, history_public_to_subscribers, is_web_public + stream, + invite_only=is_private, + history_public_to_subscribers=history_public_to_subscribers, + is_web_public=is_web_public, ) return json_success()