Handle user update events

[borisyankov]

Fixes #3397 Part of #3408

Handles the three types of user data updates possible:

• full name
• avatar
• custom profile field

Extra care is taken not to mutate the state if not necessary.

This is done for two reasons:

• currently editing values throught he web app causes two events,
  one of which is not necessary (maybe should be fixed)
• this is likely the largest state, doing an extra isEqual can
  save copying all this data

[gnprice]

Thanks @borisyankov !

ba41931 flow: Add UserAvatarSource type

• Ah cool, good fix!

+/** Gravatar or Upload */
+export type UserAvatarSource = 'G' | 'U';

• How did you determine that this was the complete list? A pointer to API docs if they exist, or to something in a file like zerver/models.py if not, would be a good reference to add. (See examples of each of those elsewhere in this file.)

d32b0e3 flow: Describe the EventUserUpdateAction action precisely

+  ...UserUpdatePayloadName | UserUpdatePayloadAvatar | UserUpdatePayloadCustomProfileField,

• Huh interesting! I don't think we've previously done a spread of a union like this. If Flow handles this well, there are some other places we can benefit from using it too.

• Nit: let's pick a single order to both mention these three types in here and to define them in. That makes it a bit easier to read this line and the definitions together.

• Huh, it's interesting that, according to these types:

  • The server always sends user_id.
  • It sometimes sends email -- depending on what other data is being updated.

  Is that really the behavior? If so, it's worth a brief comment, to make explicit that you noticed the oddity (and reassure the reader that it wasn't an oversight.)

• Relatedly: what happens when the user changes email?

• Also: can a single event have several of these?

a277dab Add 'updateUser' helper function

    This function takes a 'user' and an update information object
    given by us by the back-end and produces a new, updated user object.

• This would make a great bit of jsdoc for the function itself. 😄

  (Though in that context it can be a bit terser: "Produce an updated User from the old one and an" etc.)

• I'd be inclined to stick this inside the reducer file, and squash it with the commit that adds to the reducer -- it feels like it's pretty specific to helping the reducer with its job. In particular, the description is basically the description of a small reducer.

+    return {
+      ...oldValue,
+      avatar_url: updateData.avatar_url,
+    };

• Huh, it's puzzling that this ignores the other properties, avatar_source and avatar_url_medium.

  ... I see, we don't even have those on User.

  This is a bug somewhere waiting to happen. :-/ Some change in the future has us start using those properties, and we forget to come back to this path (which isn't a common case, so it's easy to miss) and add them here too. But if the server really isn't sending those properties in normal User objects, and yet is including them in these updates, it's a server issue. Hmm.

  I think the best we can do immediately is a couple of brief (like one-line) comments warning our future selves about this discrepancy. I think one on User is probably the most important, because that's a place we'd be updating if we started using these properties.

f049142 usersReducers: Handle 'EVENT_USER_UPDATE' server events

    Extra care is taken not to mutate the state if not necessary.
    
    This is done for two reasons:
     * currently editing values throught he web app causes two events,
       one of which is not necessary (maybe should be fixed)
     * this is likely the largest state, doing an extra `isEqual` can
       save copying all this data

• Hmm. Definitely it'd be nice to avoid updates, but... is it actually realistic that this optimization will often have any effect? I feel like if we get an update event, it'd be odd and unusual for it to turn out that nothing changed.

  If that's right, then I'd rather save the complexity of the optimization -- it isn't huge, but the optimization's value will be smaller still.

      const userIndex = state.findIndex(x => x.user_id === action.person.user_id);
      const oldUser: User = state[userIndex];

      if (userIndex === -1) {
        return state;
      }

• This is kind of backward -- you're assuring the type-checker that userIndex is valid, just a line before you check for the case when it is in fact invalid.

  Fortunately, it's easy to avoid this lie: just swap the order.

[anh2111htd]

Hey @borisyankov, I'm editing #3421 based on this PR. It seems that Zulip server API allows an update request to have multiple payloads, email and full name for example, so is there any reason to keep UserUpdatePayload to have only one payload?

[chrisbobbe]

Thanks, all, for looking into this! Closing as replaced by #5386, as mentioned there.