refactor: Simplify event updates for realm_users.

We make a few things cleaner for populating `realm_users`
in `do_event_register` and `apply_events`:

    * We have a `raw_users` intermediate dictionary that
      makes event updates O(1) and cleaner to read.

    * We extract an `is_me` section for all updates that
      apply to the current user.

    * For `update` events, we do a more surgical copying
      of fields from the event into our dict.  This
      prevents us from mutating fields in the event,
      which was sketchy (at least in test mode).  In
      particular, this allowed us to remove some ugly
      `del` code related to avatars.

    * We introduce local vars `was_admin` and `now_admin`.

The cleanup had two test implications:

    * We no longer need to normalize `realm_users`, since
      `apply_events` now sees `raw_users` instead.  Since
      `raw_users` is a dict, there is no need to normalize
      it, unlike lists with possibly random order.

    * We updated the schema for avatar updates to include
      the two fields that we used to hackily delete from
      an event.

zerver/lib/events.py

@@ -45,16 +45,29 @@ from zproject.backends import email_auth_enabled, password_auth_enabled
 from version import ZULIP_VERSION
 
 
-def get_realm_user_dicts(user_profile):
+def get_raw_user_data(realm_id):
-    # type: (UserProfile) -> List[Dict[str, Text]]
+    # type: (int) -> Dict[int, Dict[str, Text]]
-    return [{'email': userdict['email'],
+    user_dicts = get_active_user_dicts_in_realm(realm_id)
-             'user_id': userdict['id'],
+
-             'avatar_url': avatar_url_from_dict(userdict),
+    def user_data(row):
-             'is_admin': userdict['is_realm_admin'],
+        # type: (Dict[str, Any]) -> Dict[str, Any]
-             'is_bot': userdict['is_bot'],
+        avatar_url = avatar_url_from_dict(row)
-             'full_name': userdict['full_name'],
+        is_admin = row['is_realm_admin']
-             'timezone': userdict['timezone']}
+
-            for userdict in get_active_user_dicts_in_realm(user_profile.realm_id)]
+        return dict(
+            email=row['email'],
+            user_id=row['id'],
+            avatar_url=avatar_url,
+            is_admin=is_admin,
+            is_bot=row['is_bot'],
+            full_name=row['full_name'],
+            timezone=row['timezone'],
+        )
+
+    return {
+        row['id']: user_data(row)
+        for row in user_dicts
+    }
 
 def always_want(msg_type):
     # type: (str) -> bool
@@ -156,7 +169,7 @@ def fetch_initial_state_data(user_profile, event_types, queue_id,
         state['realm_filters'] = realm_filters_for_realm(user_profile.realm_id)
 
     if want('realm_user'):
-        state['realm_users'] = get_realm_user_dicts(user_profile)
+        state['raw_users'] = get_raw_user_data(user_profile.realm_id)
         state['avatar_source'] = user_profile.avatar_source
         state['avatar_url_medium'] = avatar_url(user_profile, medium=True)
         state['avatar_url'] = avatar_url(user_profile)
@@ -258,48 +271,47 @@ def apply_event(state, event, user_profile, include_subscribers):
         state['pointer'] = max(state['pointer'], event['pointer'])
     elif event['type'] == "realm_user":
         person = event['person']
-
+        person_user_id = person['user_id']
-        def our_person(p):
-            # type: (Dict[str, Any]) -> bool
-            return p['user_id'] == person['user_id']
 
         if event['op'] == "add":
-            state['realm_users'].append(person)
+            state['raw_users'][person_user_id] = person
         elif event['op'] == "remove":
-            state['realm_users'] = [user for user in state['realm_users'] if not our_person(user)]
+            state['raw_users'].pop(person_user_id, None)
         elif event['op'] == 'update':
-            if (person['user_id'] == user_profile.id and 'avatar_url' in person and 'avatar_url' in state):
+            is_me = (person_user_id == user_profile.id)
-                state['avatar_source'] = person['avatar_source']
-                state['avatar_url'] = person['avatar_url']
-                state['avatar_url_medium'] = person['avatar_url_medium']
-            if 'avatar_source' in person:
-                # Drop these so that they don't modify the
-                # `realm_user` structure in the `p.update()` line
-                # later; they're only used in the above lines
-                del person['avatar_source']
-                del person['avatar_url_medium']
 
-            for field in ['is_admin', 'email', 'full_name']:
+            if is_me:
-                if person['user_id'] == user_profile.id and field in person and field in state:
+                if ('avatar_url' in person and 'avatar_url' in state):
-                    state[field] = person[field]
+                    state['avatar_source'] = person['avatar_source']
+                    state['avatar_url'] = person['avatar_url']
+                    state['avatar_url_medium'] = person['avatar_url_medium']
 
-            for p in state['realm_users']:
+                for field in ['is_admin', 'email', 'full_name']:
-                if our_person(p):
+                    if field in person and field in state:
-                    # In the unlikely event that the current user
+                        state[field] = person[field]
-                    # just changed to/from being an admin, we need
+
-                    # to add/remove the data on all bots in the
+                # In the unlikely event that the current user
-                    # realm.  This is ugly and probably better
+                # just changed to/from being an admin, we need
-                    # solved by removing the all-realm-bots data
+                # to add/remove the data on all bots in the
-                    # given to admin users from this flow.
+                # realm.  This is ugly and probably better
-                    if ('is_admin' in person and 'realm_bots' in state and
+                # solved by removing the all-realm-bots data
-                            user_profile.email == person['email']):
+                # given to admin users from this flow.
-                        if p['is_admin'] and not person['is_admin']:
+                if ('is_admin' in person and 'realm_bots' in state):
-                            state['realm_bots'] = []
+                    prev_state = state['raw_users'][user_profile.id]
-                        if not p['is_admin'] and person['is_admin']:
+                    was_admin = prev_state['is_admin']
-                            state['realm_bots'] = get_owned_bot_dicts(user_profile)
+                    now_admin = person['is_admin']
+
+                    if was_admin and not now_admin:
+                        state['realm_bots'] = []
+                    if not was_admin and now_admin:
+                        state['realm_bots'] = get_owned_bot_dicts(user_profile)
+
+            if person_user_id in state['raw_users']:
+                p = state['raw_users'][person_user_id]
+                for field in p:
+                    if field in person:
+                        p[field] = person[field]
 
-                    # Now update the person
-                    p.update(person)
     elif event['type'] == 'realm_bot':
         if event['op'] == 'add':
             state['realm_bots'].append(event['bot'])
@@ -556,6 +568,13 @@ def do_events_register(user_profile, user_client, apply_markdown=True,
         ret['unread_msgs'] = aggregate_unread_data(ret['raw_unread_msgs'])
         del ret['raw_unread_msgs']
 
+    '''
+    See the note above; the same technique applies below.
+    '''
+    if 'raw_users'in ret:
+        ret['realm_users'] = list(ret['raw_users'].values())
+        del ret['raw_users']
+
     if len(events) > 0:
         ret['last_event_id'] = events[-1]['id']
     else:

zerver/tests/test_events.py

@@ -458,7 +458,6 @@ class EventsRegisterTest(ZulipTestCase):
         # type: (Dict[str, Any], Dict[str, Any], List[Dict[str, Any]]) -> None
         def normalize(state):
             # type: (Dict[str, Any]) -> None
-            state['realm_users'] = {u['email']: u for u in state['realm_users']}
             for u in state['never_subscribed']:
                 if 'subscribers' in u:
                     u['subscribers'].sort()
@@ -946,6 +945,8 @@ class EventsRegisterTest(ZulipTestCase):
                 ('email', check_string),
                 ('user_id', check_int),
                 ('avatar_url', check_string),
+                ('avatar_url_medium', check_string),
+                ('avatar_source', check_string),
             ])),
         ])
         events = self.do_test(