api types: Update InitialDataRealm and RealmDataForUpdate to FL 237 #5806

chrisbobbe · 2023-12-20T19:39:20Z

We can use this for #5804 and #5805.

Four of the nine added /register-response fields seem to have a small documentation issue: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.60.2Fregister.60.3A.20fields.20missing.20.22Present.20if.20.5B.E2.80.A6.5D.22.3F/near/1707782

For three of them, I didn't find them mentioned in #api design:

realm_can_access_all_users_group
realm_create_multiuse_invite_group
realm_move_messages_between_streams_limit_seconds

gnprice

Thanks for taking care of this! Generally all looks good. A few comments below, mainly from making my own pass through the relevant swath of https://zulip.com/api/changelog .

gnprice · 2023-12-20T23:53:18Z

src/users/userSelectors.js

-    // On future servers, we expect this case will never happen: we'll always include
-    // a delivery_email when you have access, including when the visibility is Everyone
-    // https://github.com/zulip/zulip-mobile/pull/5515#discussion_r997731727
+    // Not reached on FL 163+, where delivery_email is always present.


Suggested change

// Not reached on FL 163+, where delivery_email is always present.

// TODO(server-7.0): Not reached on FL 163+, where delivery_email is always present.

gnprice · 2023-12-21T00:00:14Z

src/api/initialDataTypes.js

  server_needs_upgrade?: boolean,

+  // TODO(server-8.0): Added in feat. 221
+  server_supported_permission_settings?: JSONableDict, // unstable


Reading through the changelog, there's also these at FL 164:

POST /register: Added the server_presence_ping_interval_seconds and server_presence_offline_threshold_seconds fields for clients to use when implementing the presence system.

And similarly, at FL 204:

POST /register: Added server_typing_started_wait_period_milliseconds, server_typing_stopped_wait_period_milliseconds, and server_typing_started_expiry_period_milliseconds fields for clients to use when implementing typing notifications protocol.

Hmm. The commit is supposed to be about specifically updating InitialDataRealm, and those fields aren't designated

Present if realm is present in fetch_event_types.

. Do you think that's an accident, like the four accidents I reported on CZO?

Also, these are addressed in 47a8b3f and 45951db, respectively.

those fields aren't designated

Present if realm is present in fetch_event_types.

. Do you think that's an accident, like the four accidents I reported on CZO?

Looks like it must be — in zerver/lib/events.py, if you search for those names you find they get set in the if want("realm"): block.

Which has this comment:

if want("realm"): # The realm bundle includes both realm properties and server # properties, since it's rare that one would want one and not # the other. We expect most clients to want it.

(It goes on from there, but that's the part relevant to this question.)

Also, these are addressed in 47a8b3f

Hmm, yeah, so I put those in InitialDataBase. But given the above, they actually belong in InitialDataRealm.

and 45951db, respectively.

I'd describe that more as skipping addressing it 🙂 More precisely, we skipped there actually looking at these fields (and propagating them through the reducer into RealmState), so as a result we didn't bother adding them to the API type either.

Here, since we're bumping the "This is current to …" note, it'd be good to add these types along the way in order to keep that note accurate. I think the comments in 45951db don't even need any adjustment for that.

Cool; this all makes sense. I've noted the fields missing the "if realm is present" note in the relevant CZO discussion: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.60.2Fregister.60.3A.20fields.20missing.20.22Present.20if.20.5B.E2.80.A6.5D.22.3F/near/1712821

gnprice · 2023-12-21T00:04:26Z

src/api/initialDataTypes.js

+  realm_can_access_all_users_group?: boolean,
+
+  // TODO(server-8.0): Added in feat. 209
+  realm_create_multiuse_invite_group?: number,



Reading through the changelog, here's a small other item at FL 195:

GET /events, POST /register: The default_code_block_language realm setting is now consistently an empty string when no default pygments language code is set. Previously, the server had a bug that meant it might represent no default for this realm setting as either null or an empty string. Clients supporting older server versions should treat either value (null or "") as no default being set.

So the line realm_default_code_block_language: string | null, should get a TODO(server-8.0) comment saying it's just string in current versions.

gnprice · 2023-12-21T00:08:57Z

src/api/initialDataTypes.js

+  // TODO(server-8.0): Added in feat. 225
+  realm_can_access_all_users_group?: boolean,


Suggested change

// TODO(server-8.0): Added in feat. 225

realm_can_access_all_users_group?: boolean,

// TODO(server-8.0): Added in feat. 225

realm_can_access_all_users_group_id?: boolean,

Right?

… Oh huh, the changelog and the /api/register-queue page disagree. The latter says …_group, so I guess that's what it actually is.

Huh, interesting, yeah.

gnprice · 2023-12-21T00:10:20Z

src/api/initialDataTypes.js

+  // TODO(server-8.0): Added in feat. 231
+  realm_push_notifications_enabled_end_timestamp?: boolean,


Suggested change

// TODO(server-8.0): Added in feat. 231

realm_push_notifications_enabled_end_timestamp?: boolean,

// TODO(server-8.0): Added in feat. 231

realm_push_notifications_enabled_end_timestamp?: number | null,

since it's "integer | null" in the docs

Indeed, thanks for catching!

chrisbobbe · 2023-12-23T01:41:28Z

Thanks for the review! Revision pushed, and please see my comment above: #5806 (comment)

gnprice · 2023-12-28T01:12:15Z

Thanks! Reply above on that one thread. Otherwise all LGTM.

I can't find a message in `#api design` on CZO that's explicitly about removing realm_email_address_visibility from the register response. The `#api design` discussion about transitioning that from an org- to a user-level setting is here: https://chat.zulip.org/#narrow/stream/378-api-design/topic/email.20address.20visibility/near/1296131 That discussion includes mentions of the user-object fields that are about email addresses, and it includes mentions of the per-user visibility setting, but I don't see anything about removing the realm setting from the /register response. In retrospect, its removal there would have been an obvious thing to anticipate and handle in our types. Oh well, better late than never. Luckily I don't think there was a user-facing bug here. At FL 163+, the value that we've been typing as the enum EmailAddressVisibility will actually be `undefined`, but that's OK because on 163+ we don't end up doing anything with it.

…ealm Discussion: zulip#5806 (comment)

chrisbobbe · 2024-01-03T22:39:22Z

Thanks for the review! Merged, after addressing that reply.

chrisbobbe requested a review from gnprice December 20, 2023 19:39

chrisbobbe changed the title ~~api: Update InitialDataRealm and RealmDataForUpdate to FL 237~~ api types: Update InitialDataRealm and RealmDataForUpdate to FL 237 Dec 20, 2023

chrisbobbe mentioned this pull request Dec 20, 2023

notifs: Live-update state of whether realm has enabled notifications #5807

Merged

chrisbobbe added the P1 high-priority label Dec 20, 2023

gnprice reviewed Dec 21, 2023

View reviewed changes

chrisbobbe force-pushed the pr-api-realm-types-update branch from a2ba056 to 6b46817 Compare December 22, 2023 20:09

chrisbobbe mentioned this pull request Dec 22, 2023

Show "(guest)" by guest users' names, when setting enabled #5809

Merged

chrisbobbe added 3 commits January 3, 2024 13:42

api types [nfc]: Move two fields from InitialDataBase to InitialDataR…

24f5c29

…ealm Discussion: zulip#5806 (comment)

api: Update InitialDataRealm and RealmDataForUpdate to FL 237

5762333

chrisbobbe force-pushed the pr-api-realm-types-update branch from 6b46817 to 5762333 Compare January 3, 2024 22:36

chrisbobbe merged commit 5762333 into zulip:main Jan 3, 2024
1 check passed

chrisbobbe deleted the pr-api-realm-types-update branch January 3, 2024 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api types: Update InitialDataRealm and RealmDataForUpdate to FL 237 #5806

api types: Update InitialDataRealm and RealmDataForUpdate to FL 237 #5806

chrisbobbe commented Dec 20, 2023 •

edited

Loading

gnprice left a comment

gnprice Dec 20, 2023

gnprice Dec 21, 2023

gnprice Dec 21, 2023

chrisbobbe Dec 22, 2023

gnprice Dec 28, 2023

chrisbobbe Jan 3, 2024

gnprice Dec 21, 2023

gnprice Dec 21, 2023

chrisbobbe Dec 22, 2023

gnprice Dec 21, 2023

chrisbobbe Dec 22, 2023

chrisbobbe commented Dec 23, 2023 •

edited

Loading

gnprice commented Dec 28, 2023

chrisbobbe commented Jan 3, 2024

	// Not reached on FL 163+, where delivery_email is always present.
	// TODO(server-7.0): Not reached on FL 163+, where delivery_email is always present.

		// TODO(server-8.0): Added in feat. 225
		realm_can_access_all_users_group?: boolean,

		// TODO(server-8.0): Added in feat. 231
		realm_push_notifications_enabled_end_timestamp?: boolean,

api types: Update InitialDataRealm and RealmDataForUpdate to FL 237 #5806

api types: Update InitialDataRealm and RealmDataForUpdate to FL 237 #5806

Conversation

chrisbobbe commented Dec 20, 2023 • 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

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

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

Choose a reason for hiding this comment

chrisbobbe commented Dec 23, 2023 • edited Loading

gnprice commented Dec 28, 2023

chrisbobbe commented Jan 3, 2024

chrisbobbe commented Dec 20, 2023 •

edited

Loading

chrisbobbe commented Dec 23, 2023 •

edited

Loading