Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Allow fetching subscribers for streams the user has never subscribed to. 

This allows the frontend to fetch data on the subscribers list (etc.)
for streams where the user has never been subscribed, making it
possible to implement UI showing details like subscribe counts on the
subscriptions page.

This is likely a performance regression for very large teams with
large numbers of streams; we'll want to do some testing to determine
the impact (and thus whether we should make this feature only fully
enabled for larger realms).
This commit is contained in:
Kartik Maji 2016-07-12 14:57:16 -07:00 committed by Tim Abbott
parent 5462341cb4
commit 599b15cb84
4 changed files with 149 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

81
zerver/lib/actions.py
View File

@ -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']:

29
zerver/tests/test_events.py
View File

@ -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(

48
zerver/tests/test_subs.py
View File

@ -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')]:
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

1
zerver/views/__init__.py
View File

@ -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'],