bug fix: Send stream notifications to all users for public streams.

If a stream is public, we now send notifications to all realm users
if the name or description of the stream changes.  For private
streams, the behavior remains the same.

We do this by introducing a method called
can_access_stream_user_ids().

(showell helped with this fix)

Fixes #2195

zerver/lib/actions.py

@@ -132,14 +132,26 @@ def active_user_ids(realm):
     # type: (Realm) -> List[int]
     return [userdict['id'] for userdict in get_active_user_dicts_in_realm(realm)]
 
-def stream_user_ids(stream):
+def can_access_stream_user_ids(stream):
-    # type: (Stream) -> List[int]
+    # type: (Stream) -> Set[int]
-    subscriptions = Subscription.objects.filter(recipient__type=Recipient.STREAM,
-                                                recipient__type_id=stream.id)
-    if stream.invite_only:
-        subscriptions = subscriptions.filter(active=True)
 
-    return [sub['user_profile_id'] for sub in subscriptions.values('user_profile_id')]
+    # return user ids of users who can access the attributes of
+    # a stream, such as its name/description
+    if stream.is_public():
+        return set(active_user_ids(stream.realm))
+    else:
+        return private_stream_user_ids(stream)
+
+def private_stream_user_ids(stream):
+    # type: (Stream) -> Set[int]
+
+    # TODO: Find similar queries elsewhere and de-duplicate this code.
+    subscriptions = Subscription.objects.filter(
+        recipient__type=Recipient.STREAM,
+        recipient__type_id=stream.id,
+        active=True)
+
+    return {sub['user_profile_id'] for sub in subscriptions.values('user_profile_id')}
 
 def bot_owner_userids(user_profile):
     # type: (UserProfile) -> Sequence[int]
@@ -1927,7 +1939,7 @@ def do_rename_stream(realm, old_name, new_name, log=True):
             value=value,
             name=old_name
         )
-        send_event(event, stream_user_ids(stream))
+        send_event(event, can_access_stream_user_ids(stream))
 
     # Even though the token doesn't change, the web client needs to update the
     # email forwarding address to display the correctly-escaped new name.
@@ -1942,7 +1954,7 @@ def do_change_stream_description(realm, stream_name, new_description):
     event = dict(type='stream', op='update',
                  property='description', name=stream_name,
                  value=new_description)
-    send_event(event, stream_user_ids(stream))
+    send_event(event, can_access_stream_user_ids(stream))
 
 def do_create_realm(string_id, name, restricted_to_domain=None,
                     invite_required=None, org_type=None):

zerver/tests/test_subs.py

@@ -36,7 +36,7 @@ from zerver.lib.actions import (
     gather_subscriptions_helper, bulk_add_subscriptions, bulk_remove_subscriptions,
     gather_subscriptions, get_default_streams_for_realm, get_realm, get_stream,
     get_user_profile_by_email, set_default_streams, get_subscription,
-    create_streams_if_needed
+    create_streams_if_needed, active_user_ids
 )
 
 from zerver.views.streams import (
@@ -173,6 +173,43 @@ class StreamAdminTest(ZulipTestCase):
         result = self.client_delete('/json/streams/new_stream')
         self.assert_json_error(result, 'Must be a realm administrator')
 
+    def test_private_stream_live_updates(self):
+        # type: () -> None
+        email = 'hamlet@zulip.com'
+        self.login(email)
+
+        user_profile = get_user_profile_by_email(email)
+        do_change_is_admin(user_profile, True)
+
+        self.make_stream('private_stream', invite_only=True)
+        self.subscribe_to_stream(email, 'private_stream')
+        self.subscribe_to_stream('cordelia@zulip.com', 'private_stream')
+
+        events = [] # type: List[Dict[str, Any]]
+        with tornado_redirected_to_list(events):
+            result = self.client_patch('/json/streams/private_stream',
+                                       {'description': ujson.dumps('Test description')})
+        self.assert_json_success(result)
+
+        cordelia = get_user_profile_by_email('cordelia@zulip.com')
+        prospero = get_user_profile_by_email('prospero@zulip.com')
+
+        notified_user_ids = set(events[-1]['users'])
+        self.assertIn(user_profile.id, notified_user_ids)
+        self.assertIn(cordelia.id, notified_user_ids)
+        self.assertNotIn(prospero.id, notified_user_ids)
+
+        events = []
+        with tornado_redirected_to_list(events):
+            result = self.client_patch('/json/streams/private_stream',
+                                       {'new_name': ujson.dumps('whatever')})
+        self.assert_json_success(result)
+
+        notified_user_ids = set(events[-1]['users'])
+        self.assertIn(user_profile.id, notified_user_ids)
+        self.assertIn(cordelia.id, notified_user_ids)
+        self.assertNotIn(prospero.id, notified_user_ids)
+
     def test_rename_stream(self):
         # type: () -> None
         email = 'hamlet@zulip.com'
@@ -196,14 +233,19 @@ class StreamAdminTest(ZulipTestCase):
             value='stream_name2',
             name='stream_name1'
         ))
-        users = events[1]['users']
+        notified_user_ids = set(events[1]['users'])
-        self.assertEqual(users, [user_profile.id])
 
         stream_name1_exists = get_stream('stream_name1', realm)
         self.assertFalse(stream_name1_exists)
         stream_name2_exists = get_stream('stream_name2', realm)
         self.assertTrue(stream_name2_exists)
 
+        self.assertEqual(notified_user_ids, set(active_user_ids(realm)))
+        self.assertIn(user_profile.id,
+                      notified_user_ids)
+        self.assertIn(get_user_profile_by_email('prospero@zulip.com').id,
+                      notified_user_ids)
+
         # Test case to handle unicode stream name change
         # *NOTE: Here Encoding is needed when Unicode string is passed as an argument*
         with tornado_redirected_to_list(events):
@@ -276,13 +318,18 @@ class StreamAdminTest(ZulipTestCase):
             value='Test description',
             name='stream_name1'
         ))
-        users = events[0]['users']
+        notified_user_ids = set(events[0]['users'])
-        self.assertEqual(users, [user_profile.id])
 
         stream = Stream.objects.get(
             name='stream_name1',
             realm=realm,
         )
+        self.assertEqual(notified_user_ids, set(active_user_ids(realm)))
+        self.assertIn(user_profile.id,
+                      notified_user_ids)
+        self.assertIn(get_user_profile_by_email('prospero@zulip.com').id,
+                      notified_user_ids)
+
         self.assertEqual('Test description', stream.description)
 
     def test_change_stream_description_requires_realm_admin(self):