events: Fix races in stream creation event and add tests.

This fixes 2 issues:
* Being added to an invite_only stream did not correctly update the
  "streams" key of the initial state.

* Once that's resolved, subscribe_to_stream when called on a
  nonexistant stream would both send a "create" event (from
  create_stream_if_needed) and an "occupy" event (from
  bulk_add_subscriptions).

  The second event should just be suppressed in that case, and this
  implements that suppression.

tools/lint-all

@@ -412,6 +412,7 @@ def build_custom_checkers(by_lang):
              # This one in check_message is kinda terrible, since it's
              # how most instances are written, but better to exclude something than nothing
              ('zerver/lib/actions.py', 'stream = get_stream(stream_name, realm)'),
+             ('zerver/lib/actions.py', 'get_stream(signups_stream, admin_realm)'),
          ]),
          'description': 'Please use access_stream_by_*() to fetch Stream objects',
          },

zerver/lib/actions.py

@@ -232,6 +232,13 @@ def send_signup_message(sender, signups_stream, user_profile,
 
     # We also send a notification to the Zulip administrative realm
     admin_realm = get_user_profile_by_email(sender).realm
+    try:
+        # Check whether the stream exists
+        get_stream(signups_stream, admin_realm)
+    except Stream.DoesNotExist:
+        # If the signups stream hasn't been created in the admin
+        # realm, don't auto-create it to send to it; just do nothing.
+        return
     internal_send_message(
         admin_realm,
         sender,
@@ -1454,8 +1461,8 @@ def query_all_subs_by_stream(streams):
         all_subs_by_stream[sub.recipient.type_id].append(sub.user_profile)
     return all_subs_by_stream
 
-def bulk_add_subscriptions(streams, users):
-    # type: (Iterable[Stream], Iterable[UserProfile]) -> Tuple[List[Tuple[UserProfile, Stream]], List[Tuple[UserProfile, Stream]]]
+def bulk_add_subscriptions(streams, users, from_creation=False):
+    # type: (Iterable[Stream], Iterable[UserProfile], bool) -> Tuple[List[Tuple[UserProfile, Stream]], List[Tuple[UserProfile, Stream]]]
     recipients_map = bulk_get_recipients(Recipient.STREAM, [stream.id for stream in streams]) # type: Mapping[int, Recipient]
     recipients = [recipient.id for recipient in recipients_map.values()] # type: List[int]
 
@@ -1509,7 +1516,7 @@ def bulk_add_subscriptions(streams, users):
     new_occupied_streams = [stream for stream in
                             set(occupied_streams_after) - set(occupied_streams_before)
                             if not stream.invite_only]
-    if new_occupied_streams:
+    if new_occupied_streams and not from_creation:
         event = dict(type="stream", op="occupy",
                      streams=[stream.to_dict()
                               for stream in new_occupied_streams])

zerver/lib/events.py

@@ -238,6 +238,8 @@ def apply_event(state, event, user_profile, include_subscribers):
                         stream_data['subscribers'] = []
                     # Add stream to never_subscribed (if not invite_only)
                     state['never_subscribed'].append(stream_data)
+                state['streams'].append(stream)
+            state['streams'].sort(key=lambda elt: elt["name"])
 
         if event['op'] == 'delete':
             deleted_stream_ids = {stream['stream_id'] for stream in event['streams']}

zerver/lib/test_classes.py

@@ -417,10 +417,11 @@ class ZulipTestCase(TestCase):
             realm = get_realm_by_email_domain(email)
         try:
             stream = get_stream(stream_name, realm)
+            from_creation = False
         except Stream.DoesNotExist:
-            stream, _ = create_stream_if_needed(realm, stream_name)
+            stream, from_creation = create_stream_if_needed(realm, stream_name)
         user_profile = get_user_profile_by_email(email)
-        bulk_add_subscriptions([stream], [user_profile])
+        bulk_add_subscriptions([stream], [user_profile], from_creation=from_creation)
         return stream
 
     def unsubscribe_from_stream(self, email, stream_name):

zerver/tests/test_events.py

@@ -16,6 +16,7 @@ from zerver.models import (
 )
 
 from zerver.lib.actions import (
+    bulk_add_subscriptions,
     bulk_remove_subscriptions,
     do_add_alert_words,
     check_add_realm_emoji,
@@ -566,22 +567,11 @@ class EventsRegisterTest(ZulipTestCase):
                 ('is_bot', check_bool),
             ])),
         ])
-        stream_create_checker = check_dict([
-            ('type', equals('stream')),
-            ('op', equals('create')),
-            ('streams', check_list(check_dict([
-                ('description', check_string),
-                ('invite_only', check_bool),
-                ('name', check_string),
-                ('stream_id', check_int),
-            ])))
-        ])
 
         events = self.do_test(lambda: self.register("test1@zulip.com", "test1"))
+        self.assert_length(events, 1)
         error = realm_user_add_checker('events[0]', events[0])
         self.assert_on_error(error)
-        error = stream_create_checker('events[1]', events[1])
-        self.assert_on_error(error)
 
     def test_alert_words_events(self):
         # type: () -> None
@@ -1429,13 +1419,14 @@ class EventsRegisterTest(ZulipTestCase):
         # type: () -> None
         action = lambda: self.subscribe_to_stream("othello@zulip.com", u"test_stream")
         events = self.do_test(action)
-        schema_checker = check_dict([
+        self.assert_length(events, 2)
+        peer_add_schema_checker = check_dict([
             ('type', equals('subscription')),
             ('op', equals('peer_add')),
             ('user_id', check_int),
             ('subscriptions', check_list(check_string)),
         ])
-        error = schema_checker('events[2]', events[2])
+        error = peer_add_schema_checker('events[1]', events[1])
         self.assert_on_error(error)
 
     def test_subscribe_events(self):
@@ -1464,6 +1455,16 @@ class EventsRegisterTest(ZulipTestCase):
         subscription_schema_checker = check_list(
             check_dict(subscription_fields),
         )
+        stream_create_schema_checker = check_dict([
+            ('type', equals('stream')),
+            ('op', equals('create')),
+            ('streams', check_list(check_dict([
+                ('name', check_string),
+                ('stream_id', check_int),
+                ('invite_only', check_bool),
+                ('description', check_string),
+            ]))),
+        ])
         add_schema_checker = check_dict([
             ('type', equals('subscription')),
             ('op', equals('add')),
@@ -1551,6 +1552,16 @@ class EventsRegisterTest(ZulipTestCase):
         error = stream_update_schema_checker('events[0]', events[0])
         self.assert_on_error(error)
 
+        # Subscribe to a totally new invite-only stream, so it's just Hamlet on it
+        stream = self.make_stream("private", get_realm("zulip"), invite_only=True)
+        user_profile = get_user_profile_by_email("hamlet@zulip.com")
+        action = lambda: bulk_add_subscriptions([stream], [user_profile])
+        events = self.do_test(action, include_subscribers=include_subscribers)
+        self.assert_length(events, 2)
+        error = stream_create_schema_checker('events[0]', events[0])
+        error = add_schema_checker('events[1]', events[1])
+        self.assert_on_error(error)
+
 class FetchInitialStateDataTest(ZulipTestCase):
     # Non-admin users don't have access to all bots
     def test_realm_bots_non_admin(self):