From 9636362cbda4458ffbd3eb9e15e0a2285c3b377b Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Tue, 11 Jul 2023 16:43:09 +0530 Subject: [PATCH] events: Fix applying stream creation events in apply_event. There was a bug in apply_event code where only a stream which is not private is added to the "never_subscribed" data after a stream creation event. Instead, it should be added to the "never_subscribed" data irrespective of permission policy of the stream as we already send stream creation events only to those users who can access the stream. Due to the current bug, private streams were not being added to "never_subscribed" data in apply_event for admins as well. This commit fixes it and also makes sure the "never_subscribed" list is sorted which was not done before and was also a bug. The bugs mentioned above were unnoticed as the tests did not cover these cases and this commit also adds tests for those cases. --- zerver/lib/events.py | 22 ++++++++++----------- zerver/lib/test_classes.py | 8 ++++++-- zerver/tests/test_events.py | 38 +++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 13 deletions(-) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index f0571b486c..d2690ca911 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -997,22 +997,22 @@ def apply_event( elif event["type"] == "stream": if event["op"] == "create": for stream in event["streams"]: - if not stream["invite_only"]: - stream_data = copy.deepcopy(stream) - if include_subscribers: - stream_data["subscribers"] = [] + stream_data = copy.deepcopy(stream) + if include_subscribers: + stream_data["subscribers"] = [] - # We know the stream has no traffic, and this - # field is not present in the event. - # - # TODO: Probably this should just be added to the event. - stream_data["stream_weekly_traffic"] = None + # We know the stream has no traffic, and this + # field is not present in the event. + # + # TODO: Probably this should just be added to the event. + stream_data["stream_weekly_traffic"] = None - # Add stream to never_subscribed (if not invite_only) - state["never_subscribed"].append(stream_data) + # Add stream to never_subscribed (if not invite_only) + state["never_subscribed"].append(stream_data) if "streams" in state: state["streams"].append(stream) + state["never_subscribed"].sort(key=lambda elt: elt["name"]) if "streams" in state: state["streams"].sort(key=lambda elt: elt["name"]) diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index b93399698d..f22c3b5263 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -1347,12 +1347,16 @@ Output: return stream.id # Subscribe to a stream directly - def subscribe(self, user_profile: UserProfile, stream_name: str) -> Stream: + def subscribe( + self, user_profile: UserProfile, stream_name: str, invite_only: bool = False + ) -> Stream: realm = user_profile.realm try: stream = get_stream(stream_name, user_profile.realm) except Stream.DoesNotExist: - stream, from_stream_creation = create_stream_if_needed(realm, stream_name) + stream, from_stream_creation = create_stream_if_needed( + realm, stream_name, invite_only=invite_only + ) bulk_add_subscriptions(realm, [stream], [user_profile], acting_user=None) return stream diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index e82deac291..dcaee5761c 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -3044,6 +3044,44 @@ class UserDisplayActionTest(BaseAction): check_realm_user_update("events[0]", events[0], "delivery_email") self.assertEqual(events[0]["person"]["delivery_email"], cordelia.delivery_email) + def test_stream_creation_events(self) -> None: + action = lambda: self.subscribe(self.example_user("hamlet"), "Test stream") + events = self.verify_action(action, num_events=2) + check_stream_create("events[0]", events[0]) + check_subscription_add("events[1]", events[1]) + + # Check that guest user does not receive stream creation itself of public + # stream. + self.user_profile = self.example_user("polonius") + action = lambda: self.subscribe(self.example_user("hamlet"), "Test stream 2") + events = self.verify_action(action, num_events=0, state_change_expected=False) + + self.user_profile = self.example_user("hamlet") + action = lambda: self.subscribe( + self.example_user("hamlet"), "Private test stream", invite_only=True + ) + events = self.verify_action(action, num_events=2) + check_stream_create("events[0]", events[0]) + check_subscription_add("events[1]", events[1]) + + # A non-admin user who is not subscribed to the private stream does not + # receive stream creation event. + self.user_profile = self.example_user("othello") + action = lambda: self.subscribe( + self.example_user("hamlet"), "Private test stream 2", invite_only=True + ) + events = self.verify_action(action, num_events=0, state_change_expected=False) + + # An admin user who is not subscribed to the private stream also + # receives stream creation event. + action = lambda: self.subscribe( + self.example_user("hamlet"), "Private test stream 3", invite_only=True + ) + self.user_profile = self.example_user("iago") + events = self.verify_action(action, num_events=2) + check_stream_create("events[0]", events[0]) + check_subscription_peer_add("events[1]", events[1]) + class SubscribeActionTest(BaseAction): def test_subscribe_events(self) -> None: