From 598601e8fc2b3113dbd43e14b2aa687214ba2c21 Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulip.com>
Date: Tue, 13 Oct 2020 15:56:18 +0000
Subject: [PATCH] stream events: Prevent spurious events.

If a user asks to be subscribed to a stream
that they are already subscribed to, then
that stream won't be in new_stream_user_ids,
and we won't need to send an event for it.

This change makes that happen more automatically.
---
 zerver/lib/actions.py     | 24 +++++++++++++++---------
 zerver/tests/test_subs.py |  9 ++++-----
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py
index 446756d101..238f1b0683 100644
--- a/zerver/lib/actions.py
+++ b/zerver/lib/actions.py
@@ -2893,12 +2893,14 @@ def bulk_add_subscriptions(
     for (sub, stream) in subs_to_add + subs_to_activate:
         new_stream_user_ids[stream.id].add(sub.user_profile_id)
 
+    stream_dict = {stream.id: stream for stream in streams}
+
     # We now send several types of events to notify browsers.  The
     # first batch is notifications to users on invite-only streams
     # that the stream exists.
     send_stream_creation_events_for_private_streams(
         realm=realm,
-        streams=streams,
+        stream_dict=stream_dict,
         new_stream_user_ids=new_stream_user_ids,
     )
 
@@ -2911,7 +2913,7 @@ def bulk_add_subscriptions(
     send_peer_add_events(
         realm=realm,
         new_stream_user_ids=new_stream_user_ids,
-        streams=streams,
+        stream_dict=stream_dict,
         all_subscribers_by_stream=all_subscribers_by_stream,
     )
 
@@ -2966,10 +2968,12 @@ def bulk_add_subs_to_db_with_logging(
 
 def send_stream_creation_events_for_private_streams(
     realm: Realm,
-    streams: Iterable[Stream],
+    stream_dict: Dict[int, Stream],
     new_stream_user_ids: Dict[int, Set[int]],
 ) -> None:
-    for stream in streams:
+    for stream_id, stream_users_ids in new_stream_user_ids.items():
+        stream = stream_dict[stream_id]
+
         if not stream.is_public():
             # Users newly added to invite-only streams
             # need a `create` notification.  The former, because
@@ -2977,25 +2981,27 @@ def send_stream_creation_events_for_private_streams(
             # they get the "subscribe" notification, and the latter so
             # they can manage the new stream.
             # Realm admins already have all created private streams.
-            stream_users_ids = new_stream_user_ids[stream.id]
             realm_admin_ids = {user.id for user in realm.get_admin_users_and_bots()}
             notify_user_ids = list(stream_users_ids - realm_admin_ids)
-            send_stream_creation_event(stream, notify_user_ids)
+
+            if notify_user_ids:
+                send_stream_creation_event(stream, notify_user_ids)
 
 def send_peer_add_events(
     realm: Realm,
-    streams: Iterable[Stream],
+    stream_dict: Dict[int, Stream],
     new_stream_user_ids: Dict[int, Set[int]],
     all_subscribers_by_stream: Dict[int, List[int]],
 ) -> None:
     # The second batch is events for other users who are tracking the
     # subscribers lists of streams in their browser; everyone for
     # public streams and only existing subscribers for private streams.
-    for stream in streams:
+    for stream_id, altered_user_ids in new_stream_user_ids.items():
+        stream = stream_dict[stream_id]
+
         if stream.is_in_zephyr_realm and not stream.invite_only:
             continue
 
-        altered_user_ids = new_stream_user_ids[stream.id]
         subscribed_user_ids = all_subscribers_by_stream[stream.id]
 
         peer_user_ids = get_peer_user_ids_for_stream_change(
diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py
index 4e9d5c5d34..4de4144bd2 100644
--- a/zerver/tests/test_subs.py
+++ b/zerver/tests/test_subs.py
@@ -3016,11 +3016,10 @@ class SubscriptionAPITest(ZulipTestCase):
         with tornado_redirected_to_list(events):
             bulk_add_subscriptions(realm, [new_stream], [self.example_user("iago")])
 
-        self.assert_length(events, 3)
-        create_event, add_event, add_peer_event = events
-        self.assertEqual(create_event['event']['type'], 'stream')
-        self.assertEqual(create_event['event']['op'], 'create')
-        self.assertEqual(create_event['users'], [])
+        # Note that since iago is an admin, he won't get a stream/create
+        # event here.
+        self.assert_length(events, 2)
+        add_event, add_peer_event = events
 
         self.assertEqual(add_event['event']['type'], 'subscription')
         self.assertEqual(add_event['event']['op'], 'add')