api types: Sync User and CrossRealmBot types with the doc, as of FL 121 #5351

chrisbobbe · 2022-04-20T20:49:59Z

After #5349, on the path to fixing #5250.

gnprice

Thanks! Comments below.

gnprice · 2022-04-21T01:09:52Z

src/api/modelTypes.js

  // For background on the "*bot*" fields, see user docs on bots:
  //   https://zulip.com/help/add-a-bot-or-integration
  // Note that certain bots are represented by a different type entirely,
  // namely `CrossRealmBot`.
-  is_bot: boolean,


nit: Let's keep this comment next to one of the fields it's about, to increase the chance that it's noticed when it's relevant. Perhaps is_bot -- that seems like the one where you're most likely to be looking and be in need of the context that CrossRealmBot is over there.

gnprice · 2022-04-21T01:15:04Z

src/api/modelTypes.js

-  is_bot: boolean,
-  bot_type?: number,
-  bot_owner?: string,


It sure is awkward that these three get scattered from each other. :-/ Also that user_id is in a random place in the middle instead of at the top, and that is_guest is nowhere near is_admin and is_owner.

I think the reordering is worth the trade-off in order to make it more tractable to bring the type up to date, but it's a shame that it actually gets harder to read.

Perhaps first reorder the API docs to match the more logical order that's in models.py and in this type? Then we don't have to regress on that.

Replied with a proposed ordering in chat.

Sent zulip/zulip#21883; I guess I'll await that and apply the result to my next revision.

For my next revision, I'd appreciate the time saved by applying the new order on top of the existing commits in the branch, rather than having an early commit that takes the new ordering then resolve all the merge conflicts that'd create.

(I've actually gone ahead and done it the proper way for my next revision. 🙂)

gnprice · 2022-04-21T01:16:50Z

src/api/modelTypes.js

-  // (This one doesn't appear in `/users` responses.)
-  profile_data?: empty, // When we need this, describe its actual type.
+  // If we use this, avoid `avatar_url` falling out of sync with it.
+  -avatar_version: number,


api types: Add write-only `avatar_version` to User type

This doesn't end up having that effect, because the object type is wrapped in $ReadOnly<…>.

In order to make it possible to start marking properties as write-only, you can have a prep commit replace the $ReadOnly with a + on each individual property. For an example: f551158

gnprice · 2022-04-21T01:21:55Z

src/api/modelTypes.js

+  // TODO(server-3.0): Replaced in FL 1 by bot_owner_id
+  bot_owner?: string,
+
+  is_active: boolean,


api types: Add `is_active` to User type The doc says it is present in `realm_users` and `realm_non_active_users`; trust that.

Huh. It seems to actually contradict itself on that:

realm_non_active_users: (object)[]

Present if realm_user is present in fetch_event_types.

A array of dictionaries where each entry describes a user whose account has been deactivated. Note that unlike the usual User dictionary this does not contain the is_active key as all the users present in this array have deactivated accounts.

even though it then goes on to list is_active in the actual list of properties for those objects.

Same for realm_users.

Hmm. I'll bring it up in #api documentation.

https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.60is_active.60.20in.20.60.2Fregister.60.20response/near/1371430

gnprice · 2022-04-21T01:24:27Z

src/api/modelTypes.js

+  // `null` if the user isn't a bot.
+  // `1` for a `Generic` bot.
+  // `2` for an `Incoming webhook` bot.
+  // `3` for an `Outgoing webhook` bot.
+  // `4` for an `Embedded` bot.
+  bot_type: number | null,


Better to leave this comment out, I think, in favor of deferring to the API docs.

gnprice · 2022-04-21T01:30:32Z

src/api/modelTypes.js

+  // Possible values are:
+  //   Organization owner => 100
+  //   Organization administrator => 200
+  //   Organization moderator => 300
+  //   Member => 400
+  //   Guest => 600
+  role?: number,


In principle I'd say the same thing about this as about bot_type: leave these details to just be in the API docs. But maybe the role is an important enough property to make an exception.

Probably fine to leave these details out. The comment doesn't reveal anything surprising or counterintuitive, and it seems like the kind of thing where the API doc might change. (For example, to add new roles, or to stop using => where : would do fine and not get confused with ≥.) And then if it changes, we don't have to go and make a change in our code comment.

chrisbobbe · 2022-04-27T01:10:01Z

Thanks for the review! Revision pushed.

…n doc Following the recent reordering applied to the doc in zulip/zulip#21918 .

…fier

This makes it easy to mark deprecated properties write-only.

The other two properties in the comment don't appear in `realm_user` items in the /register response, according to the doc. So, stop mentioning them. (They do appear at toplevel in the /register response, if `realm_user` is present in `fetch_event_types`; there, they presumably apply to the logged-in user.)

As the comment says, some ancient servers don't send this. But we don't support those ancient servers; so, no need to make the property optional.

…e doc

This makes it easy to mark deprecated properties write-only.

These tighter types aren't made explicit in the doc (perhaps they should be), but they're inferrable from how cross-realm bots work.

gnprice · 2022-04-27T21:44:34Z

Thanks for the revision! Looks good -- merging.

As Greg pointed out in zulip#5351, at zulip#5351 (comment) : > [The write-only marker] doesn't end up having that effect, because > the object type is wrapped in $ReadOnly<…>.

chrisbobbe requested a review from gnprice April 20, 2022 20:49

chrisbobbe mentioned this pull request Apr 20, 2022

api types: Sync more types with the API docs #5349

Merged

gnprice reviewed Apr 21, 2022

View reviewed changes

chrisbobbe force-pushed the pr-user-cross-realm-bot-sync branch from 8209de1 to 7b56564 Compare April 27, 2022 01:09

chrisbobbe force-pushed the pr-user-cross-realm-bot-sync branch from 7b56564 to a1cc583 Compare April 27, 2022 01:16

chrisbobbe added 14 commits April 27, 2022 14:43

api types [nfc]: Establish that User type's properties follow order i…

1920f2e

…n doc Following the recent reordering applied to the doc in zulip/zulip#21918 .

api types [nfc]: Remove unnecessary reference to a server-code identi…

50092ec

…fier

api types [nfc]: Individually mark read-only the properties of User

078eafa

This makes it easy to mark deprecated properties write-only.

flow: Replace two uses of the unsound $Shape

4d73565

api types: Update comment on an is_active in User type

7fc2bb5

api types: Describe actual type of User.profile_data based on the doc

54d674d

As the comment says, some ancient servers don't send this. But we don't support those ancient servers; so, no need to make the property optional.

api types: Fix User.bot_type type to follow the doc

e96979c

api types [nfc]: Standardize comment on User.is_guest

5d2fa11

api types: Sync User with the doc, as of FL 121

0c506ae

api types [nfc]: Reorder CrossRealmBot type's properties to follow th…

9325bd2

…e doc

api types [nfc]: Individually mark CrossRealmBot's properties read-only

18c608b

This makes it easy to mark deprecated properties write-only.

api types: Sync CrossRealmBot with the doc, as of FL 121

ab4d746

api types: Narrow our CrossRealmBot type with some assumptions

900a007

These tighter types aren't made explicit in the doc (perhaps they should be), but they're inferrable from how cross-realm bots work.

gnprice force-pushed the pr-user-cross-realm-bot-sync branch from a1cc583 to 900a007 Compare April 27, 2022 21:44

gnprice merged commit 900a007 into zulip:main Apr 27, 2022

chrisbobbe deleted the pr-user-cross-realm-bot-sync branch April 27, 2022 21:45

chrisbobbe mentioned this pull request May 6, 2022

api types: Finish syncing InitialDataRealmUser with the doc as of FL 121 #5376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api types: Sync User and CrossRealmBot types with the doc, as of FL 121 #5351

api types: Sync User and CrossRealmBot types with the doc, as of FL 121 #5351

chrisbobbe commented Apr 20, 2022 •

edited

Loading

gnprice left a comment

gnprice Apr 21, 2022

gnprice Apr 21, 2022

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022

chrisbobbe Apr 27, 2022 •

edited

Loading

gnprice Apr 21, 2022

gnprice Apr 21, 2022

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022

gnprice Apr 21, 2022

gnprice Apr 21, 2022

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe commented Apr 27, 2022

gnprice commented Apr 27, 2022

api types: Sync User and CrossRealmBot types with the doc, as of FL 121 #5351

api types: Sync User and CrossRealmBot types with the doc, as of FL 121 #5351

Conversation

chrisbobbe commented Apr 20, 2022 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Apr 27, 2022

gnprice commented Apr 27, 2022

chrisbobbe commented Apr 20, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe Apr 27, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022 •

edited

Loading

chrisbobbe Apr 21, 2022 •

edited

Loading