events: Use stream_id for peer_add/peer_remove.

Two things were broken here:
    * we were using name(s) instead of id(s)
    * we were always sending lists that only
      had one element

Now we just send "stream_id" instead of "subscriptions".

If anything, we should start sending a list of users
instead of a list of streams.  For example, see
the code below:

    if peer_user_ids:
        for new_user_id in new_user_ids:
            event = dict(type="subscription", op="peer_add",
                         stream_id=stream.id,
                         user_id=new_user_id)
            send_event(realm, event, peer_user_ids)

Note that this only affects the webapp, as mobile/ZT
don't use this.

frontend_tests/node_tests/dispatch_subs.js

@@ -42,8 +42,11 @@ test('add', (override) => {
 });
 
 test('peer add/remove', (override) => {
+    let event = event_fixtures.subscription__peer_add;
+
     stream_data.add_sub({
         name: 'devel',
+        stream_id: event.stream_id,
     });
 
     const stream_edit_stub = global.make_stub();
@@ -52,7 +55,6 @@ test('peer add/remove', (override) => {
     const compose_fade_stub = global.make_stub();
     override('compose_fade.update_faded_users', compose_fade_stub.f);
 
-    let event = event_fixtures.subscription__peer_add;
     dispatch(event);
     assert.equal(compose_fade_stub.num_calls, 1);
     assert.equal(stream_edit_stub.num_calls, 1);
@@ -106,27 +108,57 @@ test('add error handling', (override) => {
 
 });
 
-test('peer event error handling', (override) => {
+test('peer event error handling (bad stream_ids)', (override) => {
     override('compose_fade.update_faded_users', noop);
 
     const add_event = {
         type: 'subscription',
         op: 'peer_add',
-        subscriptions: ['bogus'],
+        stream_id: 99999,
     };
 
-    blueslip.expect('warn', 'We got an add_subscriber call for a non-existent stream.');
+    blueslip.expect('warn', 'Cannot find stream for peer_add: 99999');
-    blueslip.expect('warn', 'Cannot process peer_add event');
     dispatch(add_event);
     blueslip.reset();
 
     const remove_event = {
         type: 'subscription',
         op: 'peer_remove',
-        subscriptions: ['bogus'],
+        stream_id: 99999,
+    };
+
+    blueslip.expect('warn', 'Cannot find stream for peer_remove: 99999');
+    dispatch(remove_event);
+});
+
+test('peer event error handling (add_subscriber)', (override) => {
+    stream_data.add_sub({
+        name: 'devel',
+        stream_id: 1,
+    });
+
+    override('stream_data.add_subscriber', () => false);
+
+    const add_event = {
+        type: 'subscription',
+        op: 'peer_add',
+        stream_id: 1,
+        user_id: 99999, // id is irrelevant
+    };
+
+    blueslip.expect('warn', 'Cannot process peer_add event');
+    dispatch(add_event);
+    blueslip.reset();
+
+    override('stream_data.remove_subscriber', () => false);
+
+    const remove_event = {
+        type: 'subscription',
+        op: 'peer_remove',
+        stream_id: 1,
+        user_id: 99999, // id is irrelevant
     };
 
-    blueslip.expect('warn', 'We got a remove_subscriber call for a non-existent stream bogus');
     blueslip.expect('warn', 'Cannot process peer_remove event.');
     dispatch(remove_event);
 });

frontend_tests/node_tests/lib/events.js

@@ -405,14 +405,14 @@ exports.fixtures = {
         type: 'subscription',
         op: 'peer_add',
         user_id: exports.test_user.user_id,
-        subscriptions: ['devel'],
+        stream_id: 42,
     },
 
     subscription__peer_remove: {
         type: 'subscription',
         op: 'peer_remove',
         user_id: exports.test_user.user_id,
-        subscriptions: ['devel'],
+        stream_id: 42,
     },
 
     subscription__update: {

static/js/server_events_dispatch.js

@@ -330,30 +330,41 @@ exports.dispatch_normal_event = function dispatch_normal_event(event) {
                 }
             }
         } else if (event.op === 'peer_add') {
-            function add_peer(stream_name, user_id) {
+            function add_peer(stream_id, user_id) {
-                if (!stream_data.add_subscriber(stream_name, user_id)) {
+                const sub = stream_data.get_sub_by_id(stream_id);
+
+                if (!sub) {
+                    blueslip.warn('Cannot find stream for peer_add: ' + stream_id);
+                    return;
+                }
+
+                if (!stream_data.add_subscriber(sub.name, user_id)) {
                     blueslip.warn('Cannot process peer_add event');
                     return;
                 }
 
-                stream_edit.rerender(stream_name);
+                stream_edit.rerender(sub.name);
+                compose_fade.update_faded_users();
             }
-            for (const stream_name of event.subscriptions) {
+            add_peer(event.stream_id, event.user_id);
-                add_peer(stream_name, event.user_id);
-            }
-            compose_fade.update_faded_users();
         } else if (event.op === 'peer_remove') {
-            function remove_peer(stream_name, user_id) {
+            function remove_peer(stream_id, user_id) {
-                if (!stream_data.remove_subscriber(stream_name, user_id)) {
+                const sub = stream_data.get_sub_by_id(stream_id);
+
+                if (!sub) {
+                    blueslip.warn('Cannot find stream for peer_remove: ' + stream_id);
+                    return;
+                }
+
+                if (!stream_data.remove_subscriber(sub.name, user_id)) {
                     blueslip.warn('Cannot process peer_remove event.');
                     return;
                 }
-                stream_edit.rerender(stream_name);
+
+                stream_edit.rerender(sub.name);
+                compose_fade.update_faded_users();
             }
-            for (const stream_name of event.subscriptions) {
+            remove_peer(event.stream_id, event.user_id);
-                remove_peer(stream_name, event.user_id);
-            }
-            compose_fade.update_faded_users();
         } else if (event.op === 'remove') {
             for (const rec of event.subscriptions) {
                 const sub = stream_data.get_sub_by_id(rec.stream_id);

templates/zerver/api/changelog.md

@@ -10,6 +10,12 @@ below features are supported.
 
 ## Changes in Zulip 2.2
 
+**Feature level 19**
+
+* [`GET /events`](/api/get-events): `subscriptions` event with
+  `op="peer_add"` and `op="peer_remove"` now identify the modified
+  stream by a `stream_id` field, replacing the old `name` field.
+
 **Feature level 18**
 
 * [`POST /register`](/api/register-queue): Added

zerver/lib/actions.py

@@ -2919,7 +2919,7 @@ def bulk_add_subscriptions(streams: Iterable[Stream],
         if peer_user_ids:
             for new_user_id in new_user_ids:
                 event = dict(type="subscription", op="peer_add",
-                             subscriptions=[stream.name],
+                             stream_id=stream.id,
                              user_id=new_user_id)
                 send_event(realm, event, peer_user_ids)
 
@@ -3063,7 +3063,7 @@ def bulk_remove_subscriptions(users: Iterable[UserProfile],
             for removed_user in altered_users:
                 event = dict(type="subscription",
                              op="peer_remove",
-                             subscriptions=[stream.name],
+                             stream_id=stream.id,
                              user_id=removed_user.id)
                 send_event(our_realm, event, peer_user_ids)
 

zerver/lib/events.py

@@ -652,19 +652,21 @@ def apply_event(state: Dict[str, Any],
                 if sub['name'].lower() == event['name'].lower():
                     sub[event['property']] = event['value']
         elif event['op'] == 'peer_add':
+            stream_id = event['stream_id']
             user_id = event['user_id']
             for sub in state['subscriptions']:
-                if (sub['name'] in event['subscriptions'] and
+                if (sub['stream_id'] == stream_id and
                         user_id not in sub['subscribers']):
                     sub['subscribers'].append(user_id)
             for sub in state['never_subscribed']:
-                if (sub['name'] in event['subscriptions'] and
+                if (sub['stream_id'] == stream_id and
                         user_id not in sub['subscribers']):
                     sub['subscribers'].append(user_id)
         elif event['op'] == 'peer_remove':
+            stream_id = event['stream_id']
             user_id = event['user_id']
             for sub in state['subscriptions']:
-                if (sub['name'] in event['subscriptions'] and
+                if (sub['stream_id'] == stream_id and
                         user_id in sub['subscribers']):
                     sub['subscribers'].remove(user_id)
     elif event['type'] == "presence":

zerver/tests/test_events.py

@@ -2545,7 +2545,7 @@ class EventsRegisterTest(ZulipTestCase):
             ('type', equals('subscription')),
             ('op', equals('peer_add')),
             ('user_id', check_int),
-            ('subscriptions', check_list(check_string)),
+            ('stream_id', check_int),
         ])
         error = peer_add_schema_checker('events[1]', events[1])
         self.assert_on_error(error)
@@ -2618,13 +2618,13 @@ class EventsRegisterTest(ZulipTestCase):
             ('type', equals('subscription')),
             ('op', equals('peer_add')),
             ('user_id', check_int),
-            ('subscriptions', check_list(check_string)),
+            ('stream_id', check_int),
         ])
         peer_remove_schema_checker = self.check_events_dict([
             ('type', equals('subscription')),
             ('op', equals('peer_remove')),
             ('user_id', check_int),
-            ('subscriptions', check_list(check_string)),
+            ('stream_id', check_int),
         ])
         stream_update_schema_checker = self.check_events_dict([
             ('type', equals('stream')),

zerver/tests/test_subs.py

@@ -2936,9 +2936,10 @@ class SubscriptionAPITest(ZulipTestCase):
         notifications = set()
         for event in peer_events:
             for user_id in event['users']:
-                for stream_name in event['event']['subscriptions']:
+                stream_id = event['event']['stream_id']
-                    removed_user_id = event['event']['user_id']
+                stream_name = Stream.objects.get(id=stream_id).name
-                    notifications.add((user_id, removed_user_id, stream_name))
+                removed_user_id = event['event']['user_id']
+                notifications.add((user_id, removed_user_id, stream_name))
 
         # POSITIVE CASES FIRST
         self.assertIn((user3.id, user1.id, 'stream1'), notifications)