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

Use user_ids, not emails, for bulk stream operations. 

We now return user_ids for subscribers to streams in add-stream
events.  This allows us to eliminate the UserLite class for
both bulk adds and bulk removes.  It also simplifies some JS
code that already wanted to use user_ids, not emails.

Fixes #6898
This commit is contained in:
Steve Howell 2017-10-07 07:00:39 -07:00 committed by Tim Abbott
parent 3c434f0d86
commit 3e6bfe1b23
6 changed files with 33 additions and 81 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
frontend_tests/node_tests/stream_events.js
View File

@ -148,7 +148,7 @@ stream_data.add_sub('Frontend', frontend);
set_global('stream_data', {
subscribe_myself: noop,
set_subscriber_emails: noop,
set_subscribers: noop,
get_colors: noop,
});
set_global('subs', { update_settings_for_subscribed: noop });
@ -218,16 +218,12 @@ stream_data.add_sub('Frontend', frontend);
// Test assigning subscriber emails
with_overrides(function (override) {
global.with_stub(function (stub) {
override('stream_data.set_subscriber_emails', stub.f);
var emails = [
'me@example.com',
'you@example.com',
'zooly@zulip.com',
];
stream_events.mark_subscribed(frontend, emails, '');
override('stream_data.set_subscribers', stub.f);
var user_ids = [15, 20, 25];
stream_events.mark_subscribed(frontend, user_ids, '');
var args = stub.get_args('sub', 'subscribers');
assert.deepEqual(frontend, args.sub);
assert.deepEqual(emails, args.subscribers);
assert.deepEqual(user_ids, args.subscribers);
});
// assign self as well

11
static/js/stream_data.js
View File

@ -292,17 +292,6 @@ exports.set_subscribers = function (sub, user_ids) {
sub.subscribers = Dict.from_array(user_ids || []);
};
exports.set_subscriber_emails = function (sub, emails) {
_.each(emails, function (email) {
var user_id = people.get_user_id(email);
if (!user_id) {
blueslip.error("We tried to set invalid subscriber: " + email);
} else {
sub.subscribers.set(user_id, true);
}
});
};
exports.add_subscriber = function (stream_name, user_id) {
var sub = exports.get_sub(stream_name);
if (typeof sub === 'undefined') {

2
static/js/stream_events.js
View File

@ -103,7 +103,7 @@ exports.mark_subscribed = function (sub, subscribers, color) {
}
stream_data.subscribe_myself(sub);
if (subscribers) {
stream_data.set_subscriber_emails(sub, subscribers);
stream_data.set_subscribers(sub, subscribers);
}
subs.update_settings_for_subscribed(sub);

61
zerver/lib/actions.py
View File

@ -1904,8 +1904,8 @@ def maybe_get_subscriber_emails(stream, user_profile):
subscribers = []
return subscribers
def notify_subscriptions_added(user_profile, sub_pairs, stream_emails, no_log=False):
# type: (UserProfile, Iterable[Tuple[Subscription, Stream]], Callable[[Stream], List[Text]], bool) -> None
def notify_subscriptions_added(user_profile, sub_pairs, stream_user_ids, no_log=False):
# type: (UserProfile, Iterable[Tuple[Subscription, Stream]], Callable[[Stream], List[int]], bool) -> None
if not no_log:
log_event({'type': 'subscription_added',
'user': user_profile.email,
@ -1924,7 +1924,7 @@ def notify_subscriptions_added(user_profile, sub_pairs, stream_emails, no_log=Fa
push_notifications=subscription.push_notifications,
description=stream.description,
pin_to_top=subscription.pin_to_top,
subscribers=stream_emails(stream))
subscribers=stream_user_ids(stream))
for (subscription, stream) in sub_pairs]
event = dict(type="subscription", op="add",
subscriptions=payload)
@ -1951,26 +1951,8 @@ def get_peer_user_ids_for_stream_change(stream, altered_user_ids, subscribed_use
# structure to stay up-to-date.
return set(active_user_ids(stream.realm_id)) - set(altered_user_ids)
class UserLite(object):
'''
This is a lightweight object that we use for highly
optimized codepaths that sometimes process ~30k subscription
rows when bulk-adding streams for newly registered users.
This little wrapper is a lot less expensive than full-blown
UserProfile objects, but it lets the rest of the code work
with the nice object syntax.
Long term, we want to avoid sending around all of these emails,
so we can make this class go away and just deal with user_ids.
'''
def __init__(self, user_id, email):
# type: (int, Text) -> None
self.id = user_id
self.email = email
def query_all_subs_by_stream(streams):
# type: (Iterable[Stream]) -> Dict[int, List[UserLite]]
def get_user_ids_for_streams(streams):
# type: (Iterable[Stream]) -> Dict[int, List[int]]
all_subs = Subscription.objects.filter(
recipient__type=Recipient.STREAM,
recipient__type_id__in=[stream.id for stream in streams],
@ -1979,23 +1961,16 @@ def query_all_subs_by_stream(streams):
).values(
'recipient__type_id',
'user_profile_id',
'user_profile__email',
).order_by(
'recipient__type_id',
)
get_stream_id = itemgetter('recipient__type_id')
all_subs_by_stream = defaultdict(list) # type: Dict[int, List[UserLite]]
all_subs_by_stream = defaultdict(list) # type: Dict[int, List[int]]
for stream_id, rows in itertools.groupby(all_subs, get_stream_id):
users = [
UserLite(
user_id=row['user_profile_id'],
email=row['user_profile__email'],
)
for row in rows
]
all_subs_by_stream[stream_id] = users
user_ids = [row['user_profile_id'] for row in rows]
all_subs_by_stream[stream_id] = user_ids
return all_subs_by_stream
@ -2090,14 +2065,14 @@ def bulk_add_subscriptions(streams, users, from_stream_creation=False, acting_us
# First, get all users subscribed to the streams that we care about
# We fetch all subscription information upfront, as it's used throughout
# the following code and we want to minize DB queries
all_subs_by_stream = query_all_subs_by_stream(streams=streams)
all_subs_by_stream = get_user_ids_for_streams(streams=streams)
def fetch_stream_subscriber_emails(stream):
# type: (Stream) -> List[Text]
def fetch_stream_subscriber_user_ids(stream):
# type: (Stream) -> List[int]
if stream.realm.is_zephyr_mirror_realm and not stream.invite_only:
return []
users = all_subs_by_stream[stream.id]
return [u.email for u in users]
user_ids = all_subs_by_stream[stream.id]
return user_ids
sub_tuples_by_user = defaultdict(list) # type: Dict[int, List[Tuple[Subscription, Stream]]]
new_streams = set() # type: Set[Tuple[int, int]]
@ -2123,7 +2098,7 @@ def bulk_add_subscriptions(streams, users, from_stream_creation=False, acting_us
if len(sub_tuples_by_user[user_profile.id]) == 0:
continue
sub_pairs = sub_tuples_by_user[user_profile.id]
notify_subscriptions_added(user_profile, sub_pairs, fetch_stream_subscriber_emails)
notify_subscriptions_added(user_profile, sub_pairs, fetch_stream_subscriber_user_ids)
# The second batch is events for other users who are tracking the
# subscribers lists of streams in their browser; everyone for
@ -2133,8 +2108,7 @@ def bulk_add_subscriptions(streams, users, from_stream_creation=False, acting_us
continue
new_user_ids = [user.id for user in users if (user.id, stream.id) in new_streams]
subscribed_users = all_subs_by_stream[stream.id]
subscribed_user_ids = [u.id for u in subscribed_users]
subscribed_user_ids = all_subs_by_stream[stream.id]
peer_user_ids = get_peer_user_ids_for_stream_change(
stream=stream,
@ -2240,7 +2214,7 @@ def bulk_remove_subscriptions(users, streams, acting_user=None):
continue
notify_subscriptions_removed(user_profile, streams_by_user[user_profile.id])
all_subs_by_stream = query_all_subs_by_stream(streams=streams)
all_subs_by_stream = get_user_ids_for_streams(streams=streams)
for stream in streams:
if stream.realm.is_zephyr_mirror_realm and not stream.invite_only:
@ -2249,8 +2223,7 @@ def bulk_remove_subscriptions(users, streams, acting_user=None):
altered_users = altered_user_dict[stream.id]
altered_user_ids = [u.id for u in altered_users]
subscribed_users = all_subs_by_stream[stream.id]
subscribed_user_ids = [u.id for u in subscribed_users]
subscribed_user_ids = all_subs_by_stream[stream.id]
peer_user_ids = get_peer_user_ids_for_stream_change(
stream=stream,

10
zerver/lib/events.py
View File

@ -355,15 +355,7 @@ def apply_event(state, event, user_profile, include_subscribers):
return
if event['op'] in ["add"]:
if include_subscribers:
# Convert the emails to user_profile IDs since that's what register() returns
# TODO: Clean up this situation by making the event also have IDs
for item in event["subscriptions"]:
item["subscribers"] = [
get_user(email, user_profile.realm).id
for email in item["subscribers"]
]
else:
if not include_subscribers:
# Avoid letting 'subscribers' entries end up in the list
for i, sub in enumerate(event['subscriptions']):
event['subscriptions'][i] = copy.deepcopy(event['subscriptions'][i])

16
zerver/tests/test_subs.py
View File

@ -1288,6 +1288,7 @@ class SubscriptionAPITest(ZulipTestCase):
"""
self.user_profile = self.example_user('hamlet')
self.test_email = self.user_profile.email
self.test_user = self.user_profile
self.login(self.test_email)
self.test_realm = self.user_profile.realm
self.streams = self.get_streams(self.test_email, self.test_realm)
@ -1615,8 +1616,8 @@ class SubscriptionAPITest(ZulipTestCase):
def test_multi_user_subscription(self):
# type: () -> None
email1 = self.example_email("cordelia")
email2 = self.example_email("iago")
user1 = self.example_user("cordelia")
user2 = self.example_user("iago")
realm = get_realm("zulip")
streams_to_sub = ['multi_user_stream']
events = [] # type: List[Mapping[str, Any]]
@ -1625,7 +1626,7 @@ class SubscriptionAPITest(ZulipTestCase):
self.common_subscribe_to_streams(
self.test_email,
streams_to_sub,
dict(principals=ujson.dumps([email1, email2])),
dict(principals=ujson.dumps([user1.email, user2.email])),
)
self.assert_length(queries, 39)
@ -1635,7 +1636,7 @@ class SubscriptionAPITest(ZulipTestCase):
self.assertEqual(ev['event']['op'], 'add')
self.assertEqual(
set(ev['event']['subscriptions'][0]['subscribers']),
set([email1, email2])
set([user1.id, user2.id])
)
else:
# Check "peer_add" events for streams users were
@ -1664,7 +1665,7 @@ class SubscriptionAPITest(ZulipTestCase):
self.assertEqual(add_event['users'], [get_user(self.test_email, self.test_realm).id])
self.assertEqual(
set(add_event['event']['subscriptions'][0]['subscribers']),
set([email1, email2, self.test_email])
set([user1.id, user2.id, self.test_user.id])
)
self.assertEqual(len(add_peer_event['users']), 17)
@ -1679,6 +1680,7 @@ class SubscriptionAPITest(ZulipTestCase):
events = []
user_profile = self.example_user('othello')
email3 = user_profile.email
user3 = user_profile
realm3 = user_profile.realm
stream = get_stream('multi_user_stream', realm)
with tornado_redirected_to_list(events):
@ -1692,7 +1694,7 @@ class SubscriptionAPITest(ZulipTestCase):
self.assertEqual(add_event['users'], [get_user(email3, realm3).id])
self.assertEqual(
set(add_event['event']['subscriptions'][0]['subscribers']),
set([email1, email2, email3, self.test_email])
set([user1.id, user2.id, user3.id, self.test_user.id])
)
# We don't send a peer_add event to othello
@ -1733,7 +1735,7 @@ class SubscriptionAPITest(ZulipTestCase):
self.assertEqual(add_event['users'], [user_profile.id])
self.assertEqual(
set(add_event['event']['subscriptions'][0]['subscribers']),
set([user_profile.email, existing_user_profile.email])
set([user_profile.id, existing_user_profile.id])
)
# We don't send a peer_add event to othello