From 00e60c0c91343d74144c2c335216685efc52155e Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Fri, 12 Jun 2020 14:54:01 +0000 Subject: [PATCH] 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 | 46 ++++++++++++++++++---- frontend_tests/node_tests/lib/events.js | 4 +- static/js/server_events_dispatch.js | 39 +++++++++++------- templates/zerver/api/changelog.md | 6 +++ zerver/lib/actions.py | 4 +- zerver/lib/events.py | 8 ++-- zerver/tests/test_events.py | 6 +-- zerver/tests/test_subs.py | 7 ++-- 8 files changed, 86 insertions(+), 34 deletions(-) diff --git a/frontend_tests/node_tests/dispatch_subs.js b/frontend_tests/node_tests/dispatch_subs.js index 2364693acf..e5ae725eba 100644 --- a/frontend_tests/node_tests/dispatch_subs.js +++ b/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 process peer_add event'); + blueslip.expect('warn', 'Cannot find stream for peer_add: 99999'); 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); }); diff --git a/frontend_tests/node_tests/lib/events.js b/frontend_tests/node_tests/lib/events.js index 38d45612a0..58cba6964b 100644 --- a/frontend_tests/node_tests/lib/events.js +++ b/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: { diff --git a/static/js/server_events_dispatch.js b/static/js/server_events_dispatch.js index 81ae85707e..6272f5e6d9 100644 --- a/static/js/server_events_dispatch.js +++ b/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) { - if (!stream_data.add_subscriber(stream_name, user_id)) { + function add_peer(stream_id, 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(stream_name, event.user_id); - } - compose_fade.update_faded_users(); + add_peer(event.stream_id, event.user_id); } else if (event.op === 'peer_remove') { - function remove_peer(stream_name, user_id) { - if (!stream_data.remove_subscriber(stream_name, user_id)) { + function remove_peer(stream_id, 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(stream_name, event.user_id); - } - compose_fade.update_faded_users(); + remove_peer(event.stream_id, event.user_id); } else if (event.op === 'remove') { for (const rec of event.subscriptions) { const sub = stream_data.get_sub_by_id(rec.stream_id); diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index a50fda9f38..8b7cdd2a7c 100644 --- a/templates/zerver/api/changelog.md +++ b/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 diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 4da809a51c..0b021c19fa 100644 --- a/zerver/lib/actions.py +++ b/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) diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 7641de5e32..077a7b7b4d 100644 --- a/zerver/lib/events.py +++ b/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": diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 065022d335..1eb76974c8 100644 --- a/zerver/tests/test_events.py +++ b/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')), diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 15256d7d34..9b85512e15 100644 --- a/zerver/tests/test_subs.py +++ b/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']: - removed_user_id = event['event']['user_id'] - notifications.add((user_id, removed_user_id, stream_name)) + stream_id = event['event']['stream_id'] + stream_name = Stream.objects.get(id=stream_id).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)