diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index c61f17c3a1..865ea9db0d 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -80,6 +80,7 @@ import platform import logging import itertools from collections import defaultdict +import copy # This will be used to type annotate parameters in a function if the function # works on both str and unicode in python 2 but in python 3 it only works on str. @@ -1375,9 +1376,19 @@ def bulk_add_subscriptions(streams, users): new_users = [user for user in users if (user.id, stream.id) in new_streams] new_user_ids = [user.id for user in new_users] + non_new_user_ids = set(active_user_ids(user_profile.realm)) - set(new_user_ids) all_subscribed_ids = [user.id for user in all_subs_by_stream[stream.id]] other_user_ids = set(all_subscribed_ids) - set(new_user_ids) - if other_user_ids: + if not stream.invite_only: + # We now do "peer_add" events even for streams users were + # never subscribed to, in order for the neversubscribed + # structure to stay up-to-date. + for user_profile in new_users: + event = dict(type="subscription", op="peer_add", + subscriptions=[stream.name], + user_email=user_profile.email) + send_event(event, non_new_user_ids) + elif other_user_ids: for user_profile in new_users: event = dict(type="subscription", op="peer_add", subscriptions=[stream.name], @@ -1417,11 +1428,10 @@ def do_add_subscription(user_profile, stream, no_log=False): notify_subscriptions_added(user_profile, [(subscription, stream)], lambda stream: emails_by_stream[stream.id], no_log) - user_ids = get_other_subscriber_ids(stream, user_profile.id) event = dict(type="subscription", op="peer_add", subscriptions=[stream.name], user_email=user_profile.email) - send_event(event, user_ids) + send_event(event, active_user_ids(user_profile.realm)) return did_subscribe @@ -2550,32 +2560,42 @@ def decode_email_address(email): # performance impact for loading / for users with large numbers of # subscriptions, so it's worth optimizing. def gather_subscriptions_helper(user_profile): - # type: (UserProfile) -> Tuple[List[Dict[str, Any]], List[Dict[str, Any]], Dict[int, text_type]] + # type: (UserProfile) -> Tuple[List[Dict[str, Any]], List[Dict[str, Any]], List[Dict[str, Any]], Dict[int, text_type]] sub_dicts = Subscription.objects.select_related("recipient").filter( user_profile = user_profile, recipient__type = Recipient.STREAM).values( "recipient__type_id", "in_home_view", "color", "desktop_notifications", "audible_notifications", "active", "pin_to_top") - stream_ids = [sub["recipient__type_id"] for sub in sub_dicts] + stream_ids = set([sub["recipient__type_id"] for sub in sub_dicts]) + all_streams = get_active_streams(user_profile.realm).select_related( + "realm").values("id", "name", "invite_only", "realm_id", \ + "realm__domain", "email_token", "description") - stream_dicts = get_active_streams(user_profile.realm).select_related( - "realm").filter(id__in=stream_ids).values( - "id", "name", "invite_only", "realm_id", "realm__domain", "email_token", "description") + stream_dicts = [stream for stream in all_streams if stream['id'] in stream_ids] stream_hash = {} for stream in stream_dicts: stream_hash[stream["id"]] = stream + all_streams_id = [stream["id"] for stream in all_streams] + subscribed = [] unsubscribed = [] + never_subscribed = [] # Deactivated streams aren't in stream_hash. streams = [stream_hash[sub["recipient__type_id"]] for sub in sub_dicts \ if sub["recipient__type_id"] in stream_hash] streams_subscribed_map = dict((sub["recipient__type_id"], sub["active"]) for sub in sub_dicts) - subscriber_map = bulk_get_subscriber_user_ids(streams, user_profile, streams_subscribed_map) + # Add never subscribed streams to streams_subscribed_map + streams_subscribed_map.update({stream['id']: False for stream in all_streams if stream not in streams}) + + subscriber_map = bulk_get_subscriber_user_ids(all_streams, user_profile, streams_subscribed_map) + + sub_unsub_stream_ids = set() for sub in sub_dicts: + sub_unsub_stream_ids.add(sub["recipient__type_id"]) stream = stream_hash.get(sub["recipient__type_id"]) if not stream: # This stream has been deactivated, don't include it. @@ -2605,6 +2625,22 @@ def gather_subscriptions_helper(user_profile): else: unsubscribed.append(stream_dict) + all_streams_id_set = set(all_streams_id) + never_subscribed_stream_ids = all_streams_id_set - sub_unsub_stream_ids + never_subscribed_streams = [ns_stream_dict for ns_stream_dict in all_streams + if ns_stream_dict['id'] in never_subscribed_stream_ids] + + for stream in never_subscribed_streams: + if not stream['invite_only']: + stream_dict = {'name': stream['name'], + 'invite_only': stream['invite_only'], + 'stream_id': stream['id'], + 'description': stream['description']} + subscribers = subscriber_map[stream["id"]] + if subscribers is not None: + stream_dict['subscribers'] = subscribers + never_subscribed.append(stream_dict) + user_ids = set() for subs in [subscribed, unsubscribed]: for sub in subs: @@ -2614,11 +2650,12 @@ def gather_subscriptions_helper(user_profile): email_dict = get_emails_from_user_ids(list(user_ids)) return (sorted(subscribed, key=lambda x: x['name']), sorted(unsubscribed, key=lambda x: x['name']), + sorted(never_subscribed, key=lambda x: x['name']), email_dict) def gather_subscriptions(user_profile): # type: (UserProfile) -> Tuple[List[Dict[str, Any]], List[Dict[str, Any]]] - subscribed, unsubscribed, email_dict = gather_subscriptions_helper(user_profile) + subscribed, unsubscribed, never_subscribed, email_dict = gather_subscriptions_helper(user_profile) for subs in [subscribed, unsubscribed]: for sub in subs: if 'subscribers' in sub: @@ -2707,9 +2744,10 @@ def fetch_initial_state_data(user_profile, event_types, queue_id): 'used': user_profile.invites_used} if want('subscription'): - subscriptions, unsubscribed, email_dict = gather_subscriptions_helper(user_profile) + subscriptions, unsubscribed, never_subscribed, email_dict = gather_subscriptions_helper(user_profile) state['subscriptions'] = subscriptions state['unsubscribed'] = unsubscribed + state['never_subscribed'] = never_subscribed state['email_dict'] = email_dict if want('update_message_flags'): @@ -2786,6 +2824,20 @@ def apply_events(state, events, user_profile): bot.update(event['bot']) elif event['type'] == 'stream': + if event['op'] == 'create': + for stream in event['streams']: + if not stream['invite_only']: + stream_data = copy.deepcopy(stream) + stream_data['subscribers'] = [] + # Add stream to never_subscribed (if not invite_only) + state['never_subscribed'].append(stream_data) + + if event['op'] == 'delete': + deleted_stream_ids = {stream['stream_id'] for stream in event['streams']} + state['streams'] = [s for s in state['streams'] if s['stream_id'] not in deleted_stream_ids] + state['never_subscribed'] = [stream for stream in state['never_subscribed'] if + stream['stream_id'] not in deleted_stream_ids] + if event['op'] == 'update': # For legacy reasons, we call stream data 'subscriptions' in # the state var here, for the benefit of the JS code. @@ -2833,6 +2885,9 @@ def apply_events(state, events, user_profile): # remove them from unsubscribed if they had been there state['unsubscribed'] = [s for s in state['unsubscribed'] if not was_added(s)] + # remove them from never_subscribed if they had been there + state['never_subscribed'] = [s for s in state['never_subscribed'] if not was_added(s)] + elif event['op'] == "remove": removed_names = set(map(name, event["subscriptions"])) was_removed = lambda s: name(s) in removed_names @@ -2861,6 +2916,10 @@ def apply_events(state, events, user_profile): if (sub['name'] in event['subscriptions'] 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 + user_id not in sub['subscribers']): + sub['subscribers'].append(user_id) elif event['op'] == 'peer_remove': user_id = get_user_profile_by_email(event['user_email']).id for sub in state['subscriptions']: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 46a63003c0..7f74c899a7 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -25,6 +25,7 @@ from zerver.lib.actions import ( do_change_stream_description, do_change_subscription_property, do_create_user, + do_deactivate_stream, do_deactivate_user, do_regenerate_api_key, do_remove_alert_words, @@ -712,6 +713,34 @@ class EventsRegisterTest(AuthedTestCase): error = schema_checker('events[1]', events[1]) self.assert_on_error(error) + def test_deactivate_stream_neversubscribed(self): + # type: () -> None + realm = get_realm('zulip.com') + stream, _ = create_stream_if_needed(realm, 'old_name') + + action = lambda: do_deactivate_stream(stream) + events = self.do_test(action) + + schema_checker = check_dict([ + ('type', equals('stream')), + ('op', equals('delete')), + ('streams', check_list(check_dict([]))), + ]) + error = schema_checker('events[0]', events[0]) + self.assert_on_error(error) + + def test_subscribe_other_user_never_subscribed(self): + action = lambda: self.subscribe_to_stream("othello@zulip.com", u"test_stream") + events = self.do_test(action) + schema_checker = check_dict([ + ('type', equals('subscription')), + ('op', equals('peer_add')), + ('user_email', check_string), + ('subscriptions', check_list(check_string)), + ]) + error = schema_checker('events[2]', events[2]) + self.assert_on_error(error) + def test_subscribe_events(self): # type: ignore # action changes type several times subscription_schema_checker = check_list( diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 821df0037a..361f39d04e 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -25,6 +25,7 @@ from zerver.models import ( from zerver.lib.actions import ( create_stream_if_needed, do_add_default_stream, do_add_subscription, do_change_is_admin, do_create_realm, do_remove_default_stream, do_set_realm_create_stream_by_admins_only, + gather_subscriptions_helper, gather_subscriptions, get_default_streams_for_realm, get_realm, get_stream, get_user_profile_by_email, set_default_streams, get_subscription ) @@ -814,7 +815,7 @@ class SubscriptionAPITest(AuthedTestCase): with tornado_redirected_to_list(events): self.helper_check_subs_before_and_after_add(self.streams + add_streams, {}, add_streams, self.streams, self.test_email, self.streams + add_streams) - self.assert_length(events, 4, True) + self.assert_length(events, 6, True) def test_successful_subscriptions_notifies_pm(self): # type: () -> None @@ -1023,13 +1024,19 @@ class SubscriptionAPITest(AuthedTestCase): ) self.assert_length(queries, 43) - self.assert_length(events, 6, exact=True) + self.assert_length(events, 8, exact=True) for ev in [x for x in events if x['event']['type'] not in ('message', 'stream')]: - self.assertEqual(ev['event']['op'], 'add') - self.assertEqual( - set(ev['event']['subscriptions'][0]['subscribers']), - set([email1, email2]) - ) + if isinstance(ev['event']['subscriptions'][0], dict): + self.assertEqual(ev['event']['op'], 'add') + self.assertEqual( + set(ev['event']['subscriptions'][0]['subscribers']), + set([email1, email2]) + ) + else: + # Check "peer_add" events for streams users were + # never subscribed to, in order for the neversubscribed + # structure to stay up-to-date. + self.assertEqual(ev['event']['op'], 'peer_add') stream = get_stream('multi_user_stream', realm) self.assertEqual(stream.num_subscribers(), 2) @@ -1055,7 +1062,7 @@ class SubscriptionAPITest(AuthedTestCase): set([email1, email2, self.test_email]) ) - self.assertEqual(len(add_peer_event['users']), 2) + self.assertEqual(len(add_peer_event['users']), 13) self.assertEqual(add_peer_event['event']['type'], 'subscription') self.assertEqual(add_peer_event['event']['op'], 'peer_add') self.assertEqual(add_peer_event['event']['user_email'], self.test_email) @@ -1082,7 +1089,7 @@ class SubscriptionAPITest(AuthedTestCase): set([email1, email2, email3, self.test_email]) ) - self.assertEqual(len(add_peer_event['users']), 3) + self.assertEqual(len(add_peer_event['users']), 14) self.assertEqual(add_peer_event['event']['type'], 'subscription') self.assertEqual(add_peer_event['event']['op'], 'peer_add') self.assertEqual(add_peer_event['event']['user_email'], email3) @@ -1586,6 +1593,39 @@ class GetSubscribersTest(AuthedTestCase): self.assertTrue(len(sub["subscribers"]) == len(users_to_subscribe)) self.assert_length(queries, 4, exact=True) + @slow(0.15, "common_subscribe_to_streams is slow") + def test_never_subscribed_streams(self): + # type: () -> None + """ + Check never_subscribed streams are fetched correctly and not include invite_only streams. + """ + realm = get_realm("zulip.com") + streams = ["stream_%s" % i for i in range(10)] + for stream in streams: + create_stream_if_needed(realm, stream) + users_to_subscribe = ["othello@zulip.com", "cordelia@zulip.com"] + ret = self.common_subscribe_to_streams( + self.email, + streams, + dict(principals=ujson.dumps(users_to_subscribe))) + self.assert_json_success(ret) + ret = self.common_subscribe_to_streams( + self.email, + ["stream_invite_only_1"], + dict(principals=ujson.dumps(users_to_subscribe)), + invite_only=True) + self.assert_json_success(ret) + with queries_captured() as queries: + subscribed, unsubscribed, never_subscribed, email_dict = gather_subscriptions_helper(self.user_profile) + self.assertTrue(len(never_subscribed) >= 10) + + # Invite only stream should not be there in never_subscribed streams + for stream_dict in never_subscribed: + if stream_dict["name"].startswith("stream_"): + self.assertFalse(stream_dict['name'] == "stream_invite_only_1") + self.assertTrue(len(stream_dict["subscribers"]) == len(users_to_subscribe)) + self.assert_length(queries, 4, exact=True) + @slow(0.15, "common_subscribe_to_streams is slow") def test_gather_subscriptions_mit(self): # type: () -> None diff --git a/zerver/views/__init__.py b/zerver/views/__init__.py index 44f008a7fb..5752b243a0 100644 --- a/zerver/views/__init__.py +++ b/zerver/views/__init__.py @@ -935,6 +935,7 @@ def home(request): have_initial_messages = user_has_messages, subbed_info = register_ret['subscriptions'], unsubbed_info = register_ret['unsubscribed'], + neversubbed_info = register_ret['never_subscribed'], email_dict = register_ret['email_dict'], people_list = register_ret['realm_users'], bot_list = register_ret['realm_bots'],