users reducer: Handle all user updates from realm_user op: update #5386

chrisbobbe · 2022-05-20T04:01:12Z

And, to get there (and also just because it's good practice), write down with a Flow type what exactly we expect to get from servers in this event.

In particular, this is on the path to correctly resolving #5250, as well as #2900 / #5363, which are server release goals.

Replaces: #3403
Related: #3408

This will let us do an `else if`.

gnprice

Thanks @chrisbobbe! This looks great; tiny comment below, and a workaround for that Flow issue.

gnprice · 2022-05-20T22:18:22Z

src/users/usersReducer.js

+                  return { ...user, ...event.person };
+                } else {
+                  return user;


We still keep the behavior of only allowing avatar_url and role changes to apply to the state [1]. […] [1] Because we've been wary of accepting server data that we haven't vetted; see 729655187.

And indeed we can now see that was well justified -- with the nice new type definitions, there are two kinds of these events where the properties don't actually line up with properties on the user!

gnprice · 2022-05-20T22:20:08Z

src/users/__tests__/usersReducer-test.js

@@ -51,22 +52,15 @@ describe('usersReducer', () => {

    /**
     * Check that an update event with supplied `person` works.


nit: describe('EVENT_USER_UPDATE' should update, e.g. to "EVENT > realm_user > update"

gnprice · 2022-05-20T23:17:46Z

src/users/__tests__/usersReducer-test.js

+    // TODO: Try to make `user_id` optional again.
+    const check = (person: $PropertyType<RealmUserUpdateEvent, 'person'>) => {


I have a fix for this, which I showed you in the office. Here's a try-flow link for the exploration we did of this Flow issue.

Following the generic "server event" pattern Greg started in 979d283. We still convert avatar_url to our special AvatarURL form at the edge, in eventToAction, for crunchy shell: https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md Also, give a Flow type to the event itself as we receive it from the server: RealmUserUpdateEventRaw. We should keep this up-to-date with the server API documentation. (We also have RealmUserUpdateEvent, for the slightly processed form that has AvatarURL.) We still keep the behavior of only allowing avatar_url and role changes to apply to the state [1]. But now, that code is in usersReducer instead of eventToAction. For the reducer tests that simulate actions changing *other* user data, that means they'd now fail. So, skip them, with a plan to re-enable. [1] Because we've been wary of accepting server data that we haven't vetted; see 7296551. The API doc seems to have been improved since then, and now we've written down a type for it. So now we can proceed more confidently, which is nice!

chrisbobbe · 2022-05-20T23:36:44Z

Revision pushed! Please hold off merging until I've done some manual testing, but you can look at the revision in the meantime if you want.

gnprice · 2022-05-20T23:42:36Z

src/users/__tests__/usersReducer-test.js

-    // Excluded: "When the owner of a bot changes." The `users` state
-    // doesn't include cross-realm bots.
+    test.skip('When the owner of a bot changes.', () => {
+      check({ user_id: theUser.user_id, bot_owner_id: (theUser.bot_owner_id ?? 1) + 1 });


nit: this and the rest of this commit still introduce setting user_id, then that's cut in a later commit

gnprice · 2022-05-20T23:49:28Z

src/api/transportTypes.js

+  // TODO: Follow changes in the user's email address, whether while we're
+  //   event polling or not.
  email: string,


I also went and added a mention of this case on #3408.

The reducer still doesn't handle all these cases, so we can't un-skip them yet. But now at least we're prepared to cover all the cases once the reducer *does* handle them, soon. See the list of cases in the doc: https://zulip.com/api/get-events#realm_user-update The removed comment was wrong to say that we should exclude a test for bot-owner changes. The users state doesn't include cross-realm bots, it's true. But it does include regular bots, and those are the bots that have owners.

Replaces: zulip#3403

chrisbobbe · 2022-05-21T01:27:08Z

Thanks! Revision pushed, with the results of my manual testing: zulip/zulip#22103, and a commit at the tip that follows from that.

gnprice

Thanks! The revision looks good, with one nit below. Please merge at will.

gnprice · 2022-05-21T01:54:45Z

src/users/usersReducer.js

+                            // This assertion is cumbersome. But it fills a
+                            // gap in Flow's coverage…that apparently Flow
+                            // doesn't announce by marking anything with
+                            // `any`. Remove when doing so doesn't stop Flow
+                            // from catching something wrong on
+                            // `value` or `rendered_value`.
+                          }: $Values<$NonMaybeType<$PropertyType<User, 'profile_data'>>>),


nit: I like to mark situations like this with something like FlowIssue (no $) in the comment. That helps in finding it, either for a broad sweep for issues we've run into in Flow (perhaps to consider dropping the workarounds after a Flow upgrade occasionally?)… or for finding a specific issue one is thinking of but doesn't remember precisely where we encountered it.

See zulip/zulip#22103, which I sent after observing that this was sometimes null.

chrisbobbe · 2022-05-23T23:41:42Z

Thanks! The revision looks good, with one nit below. Please merge at will.

Thanks for the review! Merged, with that tweak.

timabbott · 2022-05-24T00:22:07Z

It's really exciting to see this!

chrisbobbe · 2022-05-24T00:27:27Z

Glad to hear it, Tim! You might also be excited to see that we've finally got round to #4933.

chrisbobbe requested a review from gnprice May 20, 2022 04:01

chrisbobbe added P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. labels May 20, 2022

chrisbobbe added 2 commits May 20, 2022 16:15

eventToAction [nfc]: Express a conditional differently

b0bf0a4

This will let us do an `else if`.

eventToAction: Let User.role live-update with user-update events

8471961

gnprice reviewed May 20, 2022

View reviewed changes

chrisbobbe force-pushed the pr-all-user-updates branch from 06f6200 to d287d32 Compare May 20, 2022 23:33

gnprice reviewed May 20, 2022

View reviewed changes

chrisbobbe added 3 commits May 20, 2022 17:14

users reducer: Handle all user updates from realm_user op: update

d7c57df

Replaces: zulip#3403

Auth type [nfc]: Add TODO for updating Auth.email

32da02a

chrisbobbe force-pushed the pr-all-user-updates branch from 6487d01 to 50c55a8 Compare May 21, 2022 01:26

gnprice reviewed May 21, 2022

View reviewed changes

realm_user event: Fix custom_profile_field.value to be string | null

5dc4770

See zulip/zulip#22103, which I sent after observing that this was sometimes null.

chrisbobbe force-pushed the pr-all-user-updates branch from 50c55a8 to 5dc4770 Compare May 23, 2022 23:40

chrisbobbe merged commit 5dc4770 into zulip:main May 23, 2022

chrisbobbe deleted the pr-all-user-updates branch May 23, 2022 23:41

chrisbobbe mentioned this pull request May 25, 2022

Handle user update events #3403

Closed

chrisbobbe mentioned this pull request Jun 3, 2022

permission selectors: Add, with getCanCreateWebPublicStreams #5384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

users reducer: Handle all user updates from realm_user op: update #5386

users reducer: Handle all user updates from realm_user op: update #5386

chrisbobbe commented May 20, 2022 •

edited

Loading

gnprice left a comment

gnprice May 20, 2022

gnprice May 20, 2022

gnprice May 20, 2022

chrisbobbe commented May 20, 2022 •

edited

Loading

gnprice May 20, 2022

gnprice May 20, 2022

chrisbobbe May 21, 2022

chrisbobbe commented May 21, 2022

gnprice left a comment

gnprice May 21, 2022

chrisbobbe commented May 23, 2022

timabbott commented May 24, 2022

chrisbobbe commented May 24, 2022

		@@ -51,22 +52,15 @@ describe('usersReducer', () => {

		/**
		* Check that an update event with supplied `person` works.

		// TODO: Try to make `user_id` optional again.
		const check = (person: $PropertyType<RealmUserUpdateEvent, 'person'>) => {

users reducer: Handle all user updates from realm_user op: update #5386

users reducer: Handle all user updates from realm_user op: update #5386

Conversation

chrisbobbe commented May 20, 2022 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice May 20, 2022

Choose a reason for hiding this comment

gnprice May 20, 2022

Choose a reason for hiding this comment

gnprice May 20, 2022

Choose a reason for hiding this comment

chrisbobbe commented May 20, 2022 • edited Loading

gnprice May 20, 2022

Choose a reason for hiding this comment

gnprice May 20, 2022

Choose a reason for hiding this comment

chrisbobbe May 21, 2022

Choose a reason for hiding this comment

chrisbobbe commented May 21, 2022

gnprice left a comment

Choose a reason for hiding this comment

gnprice May 21, 2022

Choose a reason for hiding this comment

chrisbobbe commented May 23, 2022

timabbott commented May 24, 2022

chrisbobbe commented May 24, 2022

chrisbobbe commented May 20, 2022 •

edited

Loading

chrisbobbe commented May 20, 2022 •

edited

Loading