events: Fix buggy realm_user/update events during user creation.

We've been seeing an exception in server_event_dispatch.js in
production where in large organizations, sometimes when a new user
joined, every other browser in the organization would throw an
exception processing some sort of realm_user/update event.

It turns out the cause was that when a user copies their profile from
an existing user account with a user-uploaded avatar, the code path we
reused to set the avatar properly send a realm_user/update event about
the avatar change -- for a user that hadn't been fully created and
certainly hadn't have the realm_user/add event sent for.

We fix this and add tests and comments to prevent it recurring.

(Removed an incorrect docstring while working on this).

zerver/lib/actions.py

@@ -3312,7 +3312,8 @@ def notify_avatar_url_change(user_profile: UserProfile) -> None:
                     person=payload),
                active_user_ids(user_profile.realm_id))
 
-def do_change_avatar_fields(user_profile: UserProfile, avatar_source: str) -> None:
+def do_change_avatar_fields(user_profile: UserProfile, avatar_source: str,
+                            skip_notify: bool=False) -> None:
     user_profile.avatar_source = avatar_source
     user_profile.avatar_version += 1
     user_profile.save(update_fields=["avatar_source", "avatar_version"])
@@ -3322,6 +3323,7 @@ def do_change_avatar_fields(user_profile: UserProfile, avatar_source: str) -> No
                                  extra_data={'avatar_source': avatar_source},
                                  event_time=event_time)
 
+    if not skip_notify:
         notify_avatar_url_change(user_profile)
 
 def do_delete_avatar_image(user: UserProfile) -> None:

zerver/lib/create_user.py

@@ -19,7 +19,12 @@ from zerver.models import (
 
 
 def copy_user_settings(source_profile: UserProfile, target_profile: UserProfile) -> None:
-    """Warning: Does not save, to avoid extra database queries"""
+    # Important note: Code run from here to configure the user's
+    # settings should not call send_event, as that would cause clients
+    # to throw an exception (we haven't sent the realm_user/add event
+    # yet, so that event will include the updated details of target_profile).
+    #
+    # Note that this function will do at least one save() on target_profile.
     for settings_name in UserProfile.property_types:
         value = getattr(source_profile, settings_name)
         setattr(target_profile, settings_name, value)
@@ -34,7 +39,8 @@ def copy_user_settings(source_profile: UserProfile, target_profile: UserProfile)
 
     if source_profile.avatar_source == UserProfile.AVATAR_FROM_USER:
         from zerver.lib.actions import do_change_avatar_fields
-        do_change_avatar_fields(target_profile, UserProfile.AVATAR_FROM_USER)
+        do_change_avatar_fields(target_profile, UserProfile.AVATAR_FROM_USER,
+                                skip_notify=True)
         copy_avatar(source_profile, target_profile)
 
     copy_hotpots(source_profile, target_profile)

zerver/tests/test_users.py

@@ -27,12 +27,14 @@ from zerver.lib.stream_topic import StreamTopicTarget
 from zerver.lib.test_classes import ZulipTestCase
 from zerver.lib.test_helpers import (
     get_subscription,
+    get_test_image_file,
     queries_captured,
     reset_emails_in_zulip_realm,
     simulated_empty_cache,
     tornado_redirected_to_list,
 )
 from zerver.lib.topic_mutes import add_topic_mute
+from zerver.lib.upload import upload_avatar_image
 from zerver.lib.users import access_user_by_id, get_accounts_for_email, user_ids_to_users
 from zerver.models import (
     CustomProfileField,
@@ -1029,16 +1031,29 @@ class UserProfileTest(ZulipTestCase):
         cordelia.enable_offline_email_notifications = False
         cordelia.enable_stream_push_notifications = True
         cordelia.enter_sends = False
+        cordelia.avatar_source = UserProfile.AVATAR_FROM_USER
         cordelia.save()
 
+        # Upload cordelia's avatar
+        with get_test_image_file('img.png') as image_file:
+            upload_avatar_image(image_file, cordelia, cordelia)
+
         UserHotspot.objects.filter(user=cordelia).delete()
         UserHotspot.objects.filter(user=iago).delete()
         hotspots_completed = ['intro_reply', 'intro_streams', 'intro_topics']
         for hotspot in hotspots_completed:
             UserHotspot.objects.create(user=cordelia, hotspot=hotspot)
 
+        events: List[Mapping[str, Any]] = []
+        with tornado_redirected_to_list(events):
             copy_user_settings(cordelia, iago)
 
+        # Check that we didn't send an realm_user update events to
+        # users; this work is happening before the user account is
+        # created, so any changes will be reflected in the "add" event
+        # introducing the user to clients.
+        self.assertEqual(len(events), 0)
+
         # We verify that cordelia and iago match, but hamlet has the defaults.
         self.assertEqual(iago.full_name, "Cordelia Lear")
         self.assertEqual(cordelia.full_name, "Cordelia Lear")