From 3e6bfe1b23b7a31587e0b9907ff3674a83f65a2e Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Sat, 7 Oct 2017 07:00:39 -0700 Subject: [PATCH] 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 --- frontend_tests/node_tests/stream_events.js | 14 ++--- static/js/stream_data.js | 11 ---- static/js/stream_events.js | 2 +- zerver/lib/actions.py | 61 ++++++---------------- zerver/lib/events.py | 10 +--- zerver/tests/test_subs.py | 16 +++--- 6 files changed, 33 insertions(+), 81 deletions(-) diff --git a/frontend_tests/node_tests/stream_events.js b/frontend_tests/node_tests/stream_events.js index 9403ba2a56..e343073e0b 100644 --- a/frontend_tests/node_tests/stream_events.js +++ b/frontend_tests/node_tests/stream_events.js @@ -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 diff --git a/static/js/stream_data.js b/static/js/stream_data.js index 0c21122c3c..d277f171f0 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -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') { diff --git a/static/js/stream_events.js b/static/js/stream_events.js index fd8c1987d1..48fc5c171b 100644 --- a/static/js/stream_events.js +++ b/static/js/stream_events.js @@ -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); diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 3034c23d34..06bde86b13 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -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, diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 0f135de38b..6a9cd85b55 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -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]) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index d92304aee3..509da75c6b 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -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