streams: Fix sending stream-related events to guests.

Previous behavior-
- Guest did not receive stream creation events for new
web-public streams.
- Guest did not receive peer_add and peer_remove events
for web-public and subscribed public streams.

This commit fixes the behavior to be -
- Guests now receive stream creation events for new
web-public streams.
- Guest now receive peer_add and peer_remove events for
web-public and subscribed public streams.

api_docs/changelog.md

@@ -20,6 +20,17 @@ format used by the Zulip server that they are interacting with.
 
 ## Changes in Zulip 8.0
 
+**Feature level 220**
+
+* [`GET /events`](/api/get-events): Stream creation events for web-public
+  streams are now sent to all guest users in the organization as well.
+
+* [`GET /events`](/api/get-events): The `subscription` events for `op:
+  "peer_add"` and `op: "peer_remove"` are now sent to subscribed guest
+  users for public streams and to all the guest users for web-public
+  streams; previously, they incorrectly only received these for
+  private streams.
+
 **Feature level 219**
 
 * [`PATCH /realm/user_settings_defaults`](/api/update-realm-user-settings-defaults)

version.py

@@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.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 = 219
+API_FEATURE_LEVEL = 220
 
 # 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

zerver/actions/create_user.py

@@ -674,5 +674,5 @@ def do_reactivate_user(user_profile: UserProfile, *, acting_user: Optional[UserP
         realm=user_profile.realm,
         altered_user_dict=altered_user_dict,
         stream_dict=stream_dict,
-        private_peer_dict=subscriber_peer_info.private_peer_dict,
+        subscriber_peer_info=subscriber_peer_info,
     )

zerver/actions/streams.py

@@ -31,7 +31,7 @@ from zerver.lib.queue import queue_event_on_commit, queue_json_publish
 from zerver.lib.stream_color import pick_colors
 from zerver.lib.stream_subscription import (
     SubInfo,
-    bulk_get_private_peers,
+    SubscriberPeerInfo,
     bulk_get_subscriber_peer_info,
     get_active_subscriptions_for_stream_id,
     get_bulk_stream_subscriber_info,
@@ -62,6 +62,7 @@ from zerver.models import (
     UserGroup,
     UserProfile,
     active_non_guest_user_ids,
+    active_user_ids,
     get_system_bot,
 )
 from zerver.tornado.django_api import send_event, send_event_on_commit
@@ -466,7 +467,7 @@ def send_stream_creation_events_for_previously_inaccessible_streams(
             # Realm admins already have all created private streams.
             realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()}
             notify_user_ids = list(stream_users_ids - realm_admin_ids)
-        else:
+        elif not stream.is_web_public:
             # Guese users need a `create` notification for
             # public streams as well because they need the stream
             # to exist before they get the "subscribe" notification.
@@ -481,7 +482,7 @@ def send_peer_subscriber_events(
     realm: Realm,
     stream_dict: Dict[int, Stream],
     altered_user_dict: Dict[int, Set[int]],
-    private_peer_dict: Dict[int, Set[int]],
+    subscriber_peer_info: SubscriberPeerInfo,
 ) -> None:
     # Send peer_add/peer_remove events to other users who are tracking the
     # subscribers lists of streams in their browser; everyone for
@@ -492,6 +493,7 @@ def send_peer_subscriber_events(
     private_stream_ids = [
         stream_id for stream_id in altered_user_dict if stream_dict[stream_id].invite_only
     ]
+    private_peer_dict = subscriber_peer_info.private_peer_dict
 
     for stream_id in private_stream_ids:
         altered_user_ids = altered_user_dict[stream_id]
@@ -512,14 +514,27 @@ def send_peer_subscriber_events(
         if not stream_dict[stream_id].invite_only and not stream_dict[stream_id].is_in_zephyr_realm
     ]
 
+    web_public_stream_ids = [
+        stream_id for stream_id in public_stream_ids if stream_dict[stream_id].is_web_public
+    ]
+
+    subscriber_dict = subscriber_peer_info.subscribed_ids
+
     if public_stream_ids:
         user_streams: Dict[int, Set[int]] = defaultdict(set)
 
-        public_peer_ids = set(active_non_guest_user_ids(realm.id))
+        non_guest_user_ids = set(active_non_guest_user_ids(realm.id))
+
+        if web_public_stream_ids:
+            web_public_peer_ids = set(active_user_ids(realm.id))
 
         for stream_id in public_stream_ids:
             altered_user_ids = altered_user_dict[stream_id]
-            peer_user_ids = public_peer_ids - altered_user_ids
+
+            if stream_id in web_public_stream_ids:
+                peer_user_ids = web_public_peer_ids - altered_user_ids
+            else:
+                peer_user_ids = (non_guest_user_ids | subscriber_dict[stream_id]) - altered_user_ids
 
             if peer_user_ids and altered_user_ids:
                 if len(altered_user_ids) == 1:
@@ -545,7 +560,11 @@ def send_peer_subscriber_events(
                     send_event_on_commit(realm, event, peer_user_ids)
 
         for user_id, stream_ids in user_streams.items():
-            peer_user_ids = public_peer_ids - {user_id}
+            if stream_id in web_public_stream_ids:
+                peer_user_ids = web_public_peer_ids - altered_user_ids
+            else:
+                peer_user_ids = (non_guest_user_ids | subscriber_dict[stream_id]) - altered_user_ids
+
             event = dict(
                 type="subscription",
                 op=op,
@@ -689,7 +708,7 @@ def bulk_add_subscriptions(
         realm=realm,
         altered_user_dict=altered_user_dict,
         stream_dict=stream_dict,
-        private_peer_dict=subscriber_peer_info.private_peer_dict,
+        subscriber_peer_info=subscriber_peer_info,
     )
 
     return (
@@ -703,11 +722,9 @@ def send_peer_remove_events(
     streams: List[Stream],
     altered_user_dict: Dict[int, Set[int]],
 ) -> None:
-    private_streams = [stream for stream in streams if stream.invite_only]
+    subscriber_peer_info = bulk_get_subscriber_peer_info(
-
-    private_peer_dict = bulk_get_private_peers(
         realm=realm,
-        private_streams=private_streams,
+        streams=streams,
     )
     stream_dict = {stream.id: stream for stream in streams}
 
@@ -716,7 +733,7 @@ def send_peer_remove_events(
         realm=realm,
         stream_dict=stream_dict,
         altered_user_dict=altered_user_dict,
-        private_peer_dict=private_peer_dict,
+        subscriber_peer_info=subscriber_peer_info,
     )
 
 

zerver/lib/stream_subscription.py

@@ -198,8 +198,7 @@ def bulk_get_subscriber_peer_info(
         realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()}
 
         for stream_id in private_stream_ids:
-            # This is the same business rule as we use in
+            # Realm admins can see all private stream
-            # bulk_get_private_peers. Realm admins can see all private stream
             # subscribers.
             subscribed_user_ids = stream_user_ids.get(stream_id, set())
             subscribed_ids[stream_id] = subscribed_user_ids
@@ -215,34 +214,6 @@ def bulk_get_subscriber_peer_info(
     )
 
 
-def bulk_get_private_peers(
-    realm: Realm,
-    private_streams: List[Stream],
-) -> Dict[int, Set[int]]:
-    if not private_streams:
-        return {}
-
-    for stream in private_streams:
-        # Our caller should only pass us private streams.
-        assert stream.invite_only
-
-    peer_ids: Dict[int, Set[int]] = {}
-
-    realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()}
-
-    stream_ids = {stream.id for stream in private_streams}
-    stream_user_ids = get_user_ids_for_streams(stream_ids)
-
-    for stream in private_streams:
-        # This is the same business rule as we use in
-        # bulk_get_subscriber_peer_info.  Realm admins can see all private
-        # stream subscribers.
-        subscribed_user_ids = stream_user_ids.get(stream.id, set())
-        peer_ids[stream.id] = subscribed_user_ids | realm_admin_ids
-
-    return peer_ids
-
-
 def handle_stream_notifications_compatibility(
     user_profile: Optional[UserProfile],
     stream_dict: Dict[str, Any],

zerver/lib/streams.py

@@ -31,6 +31,7 @@ from zerver.models import (
     UserGroup,
     UserProfile,
     active_non_guest_user_ids,
+    active_user_ids,
     bulk_get_streams,
     get_realm_stream,
     get_stream,
@@ -179,7 +180,11 @@ def create_stream_if_needed(
             )
     if created:
         if stream.is_public():
-            send_stream_creation_event(realm, stream, active_non_guest_user_ids(stream.realm_id))
+            if stream.is_web_public:
+                notify_user_ids = active_user_ids(stream.realm_id)
+            else:
+                notify_user_ids = active_non_guest_user_ids(stream.realm_id)
+            send_stream_creation_event(realm, stream, notify_user_ids)
         else:
             realm_admin_ids = [user.id for user in stream.realm.get_admin_users_and_bots()]
             send_stream_creation_event(realm, stream, realm_admin_ids)

zerver/lib/test_classes.py

@@ -1389,14 +1389,18 @@ Output:
 
     # Subscribe to a stream directly
     def subscribe(
-        self, user_profile: UserProfile, stream_name: str, invite_only: bool = False
+        self,
+        user_profile: UserProfile,
+        stream_name: str,
+        invite_only: bool = False,
+        is_web_public: 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, invite_only=invite_only
+                realm, stream_name, invite_only=invite_only, is_web_public=is_web_public
             )
         bulk_add_subscriptions(realm, [stream], [user_profile], acting_user=None)
         return stream

zerver/openapi/zulip.yaml

@@ -782,8 +782,12 @@ paths:
                                 to see a given stream's subscriber list, they will receive this event
                                 for the existing subscriptions to the stream.
 
-                                **Changes**: Prior to Zulip 8.0 (feature level 205), this event was not
+                                **Changes**: Prior to Zulip 8.0 (feature level 220), this event was
-                                sent when a user gained access to a stream due to their [role
+                                incorrectly not sent to guest users when subscribers to web-public
+                                streams and subscribed public streams changed.
+
+                                Prior to Zulip 8.0 (feature level 205), this event was not sent when
+                                a user gained access to a stream due to their [role
                                 changing](/help/roles-and-permissions).
 
                                 Prior to Zulip 6.0 (feature level 134), this event was not sent when a
@@ -841,10 +845,13 @@ paths:
                                 from streams. Sent to all users if the stream is public or to only
                                 the existing subscribers if the stream is private.
 
-                                **Changes**: In Zulip 4.0 (feature level 35), the singular
+                                **Changes**: Prior to Zulip 8.0 (feature level 220), this event was
-                                `user_id` and `stream_id` integers included in this event
+                                incorrectly not sent to guest users when subscribers to web-public
-                                were replaced with plural `user_ids` and `stream_ids` integer
+                                streams and subscribed public streams changed.
-                                arrays.
+
+                                In Zulip 4.0 (feature level 35), the singular `user_id` and
+                                `stream_id` integers included in this event were replaced
+                                with plural `user_ids` and `stream_ids` integer arrays.
 
                                 In Zulip 3.0 (feature level 19), the `stream_id` field was
                                 added to identify the stream the user unsubscribed from,
@@ -1197,9 +1204,11 @@ paths:
                                 Note that organization administrators who are not subscribed will
                                 not be able to see content on the stream; just that it exists.
 
-                                **Changes**: Prior to Zulip 8.0 (feature level 205), this event was
+                                **Changes**: Prior to Zulip 8.0 (feature level 220), this event was
-                                not sent when a user gained access to a stream due to their role
+                                incorrectly not sent to guest users a web-public stream was created.
-                                changing.
+
+                                Prior to Zulip 8.0 (feature level 205), this event was not sent
+                                when a user gained access to a stream due to their role changing.
 
                                 Prior to Zulip 8.0 (feature level 192), this event was not sent
                                 when guest users gained access to a public stream by being

zerver/tests/test_events.py

@@ -3557,6 +3557,14 @@ class UserDisplayActionTest(BaseAction):
         action = lambda: self.subscribe(self.example_user("hamlet"), "Test stream 2")
         events = self.verify_action(action, num_events=0, state_change_expected=False)
 
+        # Check that guest user receives stream creation event for web-public stream.
+        action = lambda: self.subscribe(
+            self.example_user("hamlet"), "Web public test stream", is_web_public=True
+        )
+        events = self.verify_action(action, num_events=2, state_change_expected=True)
+        check_stream_create("events[0]", events[0])
+        check_subscription_peer_add("events[1]", events[1])
+
         self.user_profile = self.example_user("hamlet")
         action = lambda: self.subscribe(
             self.example_user("hamlet"), "Private test stream", invite_only=True
@@ -3779,8 +3787,32 @@ class SubscribeActionTest(BaseAction):
         stream.invite_only = False
         stream.save()
 
-        # Subscribe as a guest to a public stream.
+        # Test events for guest user.
         self.user_profile = self.example_user("polonius")
+
+        # Guest user does not receive peer_add/peer_remove events for unsubscribed
+        # public streams.
+        action = lambda: bulk_add_subscriptions(
+            user_profile.realm, [stream], [self.example_user("othello")], acting_user=None
+        )
+        events = self.verify_action(
+            action,
+            include_subscribers=include_subscribers,
+            num_events=0,
+            state_change_expected=False,
+        )
+
+        action = lambda: bulk_remove_subscriptions(
+            user_profile.realm, [self.example_user("othello")], [stream], acting_user=None
+        )
+        events = self.verify_action(
+            action,
+            include_subscribers=include_subscribers,
+            num_events=0,
+            state_change_expected=False,
+        )
+
+        # Subscribe as a guest to a public stream.
         action = lambda: bulk_add_subscriptions(
             user_profile.realm, [stream], [self.user_profile], acting_user=None
         )
@@ -3788,6 +3820,55 @@ class SubscribeActionTest(BaseAction):
         check_stream_create("events[0]", events[0])
         check_subscription_add("events[1]", events[1])
 
+        action = lambda: bulk_add_subscriptions(
+            user_profile.realm, [stream], [self.example_user("othello")], acting_user=None
+        )
+        events = self.verify_action(
+            action,
+            include_subscribers=include_subscribers,
+            state_change_expected=include_subscribers,
+        )
+        check_subscription_peer_add("events[0]", events[0])
+
+        action = lambda: bulk_remove_subscriptions(
+            user_profile.realm, [self.example_user("othello")], [stream], acting_user=None
+        )
+        events = self.verify_action(
+            action,
+            include_subscribers=include_subscribers,
+            state_change_expected=include_subscribers,
+        )
+        check_subscription_peer_remove("events[0]", events[0])
+
+        stream = self.make_stream("web-public-stream", self.user_profile.realm, is_web_public=True)
+        # Guest user receives peer_add/peer_remove events for unsubscribed
+        # web-public streams.
+        action = lambda: bulk_add_subscriptions(
+            user_profile.realm, [stream], [self.example_user("othello")], acting_user=None
+        )
+        events = self.verify_action(
+            action,
+            include_subscribers=include_subscribers,
+            state_change_expected=include_subscribers,
+        )
+
+        action = lambda: bulk_remove_subscriptions(
+            user_profile.realm, [self.example_user("othello")], [stream], acting_user=None
+        )
+        events = self.verify_action(
+            action,
+            include_subscribers=include_subscribers,
+            state_change_expected=include_subscribers,
+        )
+
+        # Subscribe as a guest to web-public stream. Guest does not receive stream creation
+        # event for web-public stream.
+        action = lambda: bulk_add_subscriptions(
+            user_profile.realm, [stream], [self.user_profile], acting_user=None
+        )
+        events = self.verify_action(action, include_subscribers=include_subscribers, num_events=1)
+        check_subscription_add("events[0]", events[0])
+
 
 class DraftActionTest(BaseAction):
     def do_enable_drafts_synchronization(self, user_profile: UserProfile) -> None:

zerver/tests/test_invite.py

@@ -142,7 +142,7 @@ class StreamSetupTest(ZulipTestCase):
 
         new_user = self.create_simple_new_user(realm, new_user_email)
 
-        with self.assert_database_query_count(12):
+        with self.assert_database_query_count(13):
             set_up_streams_for_new_human_user(
                 user_profile=new_user,
                 prereg_user=prereg_user,

zerver/tests/test_subs.py

@@ -2598,7 +2598,7 @@ class StreamAdminTest(ZulipTestCase):
         those you aren't on.
         """
         result = self.attempt_unsubscribe_of_principal(
-            query_count=16,
+            query_count=17,
             target_users=[self.example_user("cordelia")],
             is_realm_admin=True,
             is_subbed=True,
@@ -2625,7 +2625,7 @@ class StreamAdminTest(ZulipTestCase):
             for name in ["cordelia", "prospero", "iago", "hamlet", "outgoing_webhook_bot"]
         ]
         result = self.attempt_unsubscribe_of_principal(
-            query_count=27,
+            query_count=28,
             cache_count=8,
             target_users=target_users,
             is_realm_admin=True,
@@ -2686,7 +2686,7 @@ class StreamAdminTest(ZulipTestCase):
 
     def test_admin_remove_others_from_stream_legacy_emails(self) -> None:
         result = self.attempt_unsubscribe_of_principal(
-            query_count=16,
+            query_count=17,
             target_users=[self.example_user("cordelia")],
             is_realm_admin=True,
             is_subbed=True,
@@ -2700,7 +2700,7 @@ class StreamAdminTest(ZulipTestCase):
 
     def test_admin_remove_multiple_users_from_stream_legacy_emails(self) -> None:
         result = self.attempt_unsubscribe_of_principal(
-            query_count=19,
+            query_count=20,
             target_users=[self.example_user("cordelia"), self.example_user("prospero")],
             is_realm_admin=True,
             is_subbed=True,
@@ -2747,7 +2747,7 @@ class StreamAdminTest(ZulipTestCase):
         webhook_bot = self.example_user("webhook_bot")
         do_change_bot_owner(webhook_bot, bot_owner=user_profile, acting_user=user_profile)
         result = self.attempt_unsubscribe_of_principal(
-            query_count=13,
+            query_count=14,
             target_users=[webhook_bot],
             is_realm_admin=False,
             is_subbed=True,