Start Requiring Zulip Server 5 #904

PIG208 · 2024-08-22T21:00:31Z

Blocked by #267, but these are migrations that can happen in parallel. I imagine that there won't be much overhead rebasing this kind of changes.

Fixes: #268

PIG208 · 2024-08-23T21:13:30Z

The PR itself is ready, but we won't be ready to merge it until we refuse to connect to older servers. Going through some reviews will still be helpful, though.

chrisbobbe

Thanks for going ahead with this! This will be good to have ready to merge once #267 is done (that's on my plate right now). Small comments below.

chrisbobbe · 2024-08-27T03:18:35Z

lib/api/model/model.dart

@@ -205,7 +205,7 @@ class User {
  // bool isOwner; // obsoleted by [role]; ignore
  // bool isAdmin; // obsoleted by [role]; ignore
  // bool isGuest; // obsoleted by [role]; ignore
-  bool? isBillingAdmin; // TODO(server-5)
+  bool isBillingAdmin;


api: Mark isBillingAdmin as requried, relying on server 5+, FL 73+ While `realm_user` update events also have this field added at the same feature level, it remains optional. See "Feature level 73" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

I would just leave out the sentence about how isBillingAdmin remains optional in RealmUserUpdateEvent. That's the normal, expected thing to happen; it matches all the other person fields, in that they might be null if they didn't change in the event.

Also nit in summary line: 'required"

chrisbobbe · 2024-08-27T03:23:05Z

lib/api/model/model.dart

-    // This field is absent in `realm_users` and `realm_non_active_users`,
-    // which contain no system bots; it's present in `cross_realm_bots`.


api: Drop _readIsSystemBot, relying on server 5+, FL 83+

This comment still applies, right? The change made in FL 83 is just that is_cross_realm_bot stops being a name we have to care about. The comment can go above the defaultValue: false, and it seems like we should keep the test that confirms that false is chosen if is_system_bot is absent.

chrisbobbe · 2024-08-27T03:40:16Z

lib/api/model/model.dart

-        }
+      if (topic != null || prevTopic != null) {
+        // Both are present if the topic was edited
+        assert(topic != null && prevTopic != null);


The condition in this assert might happen because of unexpected data from the server. To validate data from the server, we'll want code that runs in production, and reserve asserts to check our own invariants that we maintain within the bounds of the app. Could replace the assert and comment with:

// Crunch-shell validation: both are present if the topic was edited topic as String; prevTopic as String;

PIG208 · 2024-08-27T04:33:54Z

Thanks! Updated the PR.

chrisbobbe · 2024-08-28T22:17:11Z

Thanks! LGTM except bump on this part of #904 (comment) :

it seems like we should keep the test that confirms that false is chosen if is_system_bot is absent.

PIG208 · 2024-08-29T06:56:07Z

Got that test back. Thanks for the review!

chrisbobbe · 2024-08-29T21:46:28Z

Great! Marking for Greg's review once he's back from vacation.

gnprice · 2024-09-07T04:56:35Z

BTW there's generally no need to push a revision when it's a pure rebase, with no merge conflicts or other changes. The update causes notifications, and doesn't really save effort later — there'll typically be another rebase anyway just before merge, and git rebase is a very easy step to do at merge time when there's no conflict to resolve.

(As it happens I'm just about to take a look at this PR, but it wasn't because of the notification from the revision just now; it was from scanning the list of PRs marked for my review.)

gnprice

Thanks @PIG208 for taking care of these, and thanks @chrisbobbe for the previous reviews!

Generally this looks great. Small comments below.

gnprice · 2024-09-07T05:03:20Z

lib/api/model/model.dart

-  bool? isBillingAdmin; // TODO(server-5)
+  bool isBillingAdmin;


nit in commit message:

api: Mark isBillingAdmin as required, relying on server 5+, FL 73+ See "Feature level 73" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

In the summary line, let's say User.isBillingAdmin rather than just isBillingAdmin. Similarly for fields in later commits.

That helps the reader place the field in the relevant context within the overall model.

In this case it also disambiguates things: there's an isBillingAdmin field over on RealmUserUpdateEvent too, and as a previous revision noted (as discussed in a previous review above), that one remains optional.

Yeah, I think the context info is pretty helpful. While the commit summaries get longer, it is justified for this kind of migration changes.

gnprice · 2024-09-07T05:13:59Z

lib/api/model/model.dart

-    // This field is absent in `realm_users` and `realm_non_active_users`,
-    // which contain no system bots; it's present in `cross_realm_bots`.
-    return (json[key] as bool?)
-        ?? (json['is_cross_realm_bot'] as bool?) // TODO(server-5): renamed to `is_system_bot`


nit in commit message:

api: Drop _readIsSystemBot, relying on server 5+, FL 83+

Can focus the summary on what changed in the API, rather than on the method name which is an implementation detail of our (now-deleted) code:

api: Drop is_cross_realm_bot fallback, relying on server 5+, FL 83+

gnprice · 2024-09-07T05:19:50Z

lib/api/route/realm.dart

-  final String? zulipMergeBase; // TODO(server-5)
+  final String zulipMergeBase;


These changes in the GetServerSettingsResult type are a bit sticky because this is where we learn the version of a server in the first place. For #267 we'll want to give a reasonable error message when the server is too old, and that'll probably include saying what version the server actually is (as well as the minimum version we support).

Ultimately we do want to keep modernizing this type, like the others. But there might be some wrinkles around how we handle it. I guess the bottom line is that the details of these particular changes might shift once we have an implementation of #267 in hand. (/cc @chrisbobbe)

gnprice · 2024-09-07T05:23:56Z

lib/model/store.dart

-  final UserSettings? userSettings; // TODO(server-5)
+  final UserSettings userSettings;


This should get a corresponding update in PerAccountStoreChecks.

(Similarly in other commits.)

gnprice · 2024-09-07T05:25:12Z

lib/api/model/web_auth.dart

-  final int? userId; // TODO(server-5) new in FL 108
+  final int userId;


This is one that especially benefits from identifying the class:

login: Mark userId as required, relying on server 5+, FL 108+.

so e.g.:

api: Mark WebAuthPayload.userId as required, relying on server 5+, FL 108+

Otherwise "userId" could be all sorts of things.

bump / nit: use api: for prefix, like for other API changes

(in this revision it's:

login: Mark WebAuthPayload.userId as required, relying on server 5+, FL 108+.

)

gnprice · 2024-09-07T05:27:17Z

lib/api/model/web_auth.dart

-        userId = int.tryParse(userIdStr, radix: 10);
-        if (userId == null) throw const FormatException();
-      }
+      int? userId = int.tryParse(userIdStr, radix: 10);


nit: since this is now initialized just once, make it final:

Suggested change

int? userId = int.tryParse(userIdStr, radix: 10);

final userId = int.tryParse(userIdStr, radix: 10);

gnprice · 2024-09-07T05:30:12Z

lib/api/model/events.dart

-  final int? userId; // TODO(server-5)
-  final bool? renderingOnly; // TODO(server-5)
+  final int userId;
+  final bool renderingOnly;


nit: this summary line seems to cover only one of the three changes here:

api: Mark renderingOnly as required, relying on server 5+, FL 114+

Can say "fields on UpdateMessageEvent" to skip listing all three of them in the crowded space of the summary line.

gnprice · 2024-09-07T05:34:18Z

test/example_data.dart

@@ -489,8 +489,8 @@ DeleteMessageEvent deleteMessageEvent(List<StreamMessage> messages) {

 UpdateMessageEvent updateMessageEditEvent(
  Message origMessage, {
-  int? userId = -1, // null means null; default is [selfUser.userId]
-  bool? renderingOnly = false,
+  int userId = -1, // default is [selfUser.userId]


How about:

Suggested change

int userId = -1, // default is [selfUser.userId]

int? userId, // default is [selfUser.userId]

That seems more conventional. The old code was odder because it had to deal with null being a valid value for the underlying field, and so not available to signify "use the default".

gnprice · 2024-09-07T06:10:36Z

lib/api/route/messages.dart

-///
-/// Gives null if the server reports that the message doesn't exist.
-// TODO(server-5) Simplify this away; just use getMessage.
-Future<Message?> getMessageCompat(ApiConnection connection, {


nit in commit message:

See also: 631f4d68c58a84dff66a6a9a80322584f0d60152

Instead say:

See also commit 631f4d68c58a84dff66a6a9a80322584f0d60152.

That way it's clear this hex blob refers to a commit (implicitly one in this same repo).

Marked as TODO as this might change in the future. See zulip#904 (comment). See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

… required, relying on server 5+, FL 116+ See zulip#904 (comment). See "Feature level 116" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-09-10T18:26:12Z

Thanks for the review! Added some TODOs commits for GetServerSettingsResult changes, making them easy to drop, and addressed the issues from the review. This revision should be fairly readable with range-diff.

gnprice

Thanks for the revisions! Just a few comments below.

gnprice · 2024-09-16T23:35:31Z

lib/api/route/realm.g.dart

-      zulipMergeBase: json['zulip_merge_base'] as String?,
+      zulipMergeBase: json['zulip_merge_base'] as String,


This doesn't seem to have a corresponding change in realm.dart.

Presumably this came about as you made some edits and missed rerunning build_runner for one of them, or something like that. I am puzzled that our CI didn't complain, though — tools/check build_runner should have failed because this doesn't match what a freshly-generated file should say. So that would be worth trying to debug.

For future reference, this was fixed in #947.

gnprice · 2024-09-16T23:37:09Z

lib/api/route/realm.dart

+  // TODO: Modernize this once we get to #267
+  final String? zulipMergeBase;


This should keep a TODO(server-5) comment until we do actually modernize it. That way it naturally shows up in any future round of grepping for opportunities to modernize our API usage.

gnprice · 2024-09-16T23:38:01Z

lib/api/model/web_auth.dart

-  final int? userId; // TODO(server-5) new in FL 108
+  final int userId;


bump / nit: use api: for prefix, like for other API changes

(in this revision it's:

login: Mark WebAuthPayload.userId as required, relying on server 5+, FL 108+.

)

gnprice · 2024-09-16T23:41:28Z

lib/api/model/events.dart

-  final int? editTimestamp; // TODO(server-5)
+  final int editTimestamp;


nit: this doesn't seem accurate:

Fields including `userId` and `editTimestamp` are optional and act as alternatives to `renderingOnly` for older server versions.

since those fields are no longer optional.

I think the commit message can skip explaining the fallback that previously existed for renderingOnly. It's directly relevant only when looking at the past; and the details are in this commit's diff.

Yeah, "were" would be more accurate here. Leaving this out makes sense.

gnprice · 2024-09-16T23:43:17Z

lib/api/model/initial_snapshot.dart

 class UnreadDmSnapshot {
-  @JsonKey(readValue: _readOtherUserId)
  final int otherUserId;


nit:

api: Remove sender_id fallback for UnreadMessagesSnapshot.otherUserId, relying on server 5+, FL 119+

that's not the class this field is on 🙂

Good find! I should learn to not trust the diffs heading and be more careful from now on 😓

diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 565d89fe..6be29e78 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -269,15 +269,9 @@ class UnreadMessagesSnapshot { /// An item in [UnreadMessagesSnapshot.dms]. @JsonSerializable(fieldRename: FieldRename.snake) class UnreadDmSnapshot { - @JsonKey(readValue: _readOtherUserId) final int otherUserId; final List<int> unreadMessageIds; - // TODO(server-5): Simplify away. - static dynamic _readOtherUserId(Map<dynamic, dynamic> json, String key) { - return json[key] ?? json['sender_id']; - } -

Ah, yeah, the diff-hunk headers can be misleading like that sometimes 🙂

gnprice · 2024-09-16T23:50:48Z

Also marked as draft to reflect that we won't be ready to merge this until we have #267.

Marking as draft helps avoid me forgetting that and going ahead and merging after a next round of review, if the thread otherwise looks ready 🙂

The diffs generated from running `tools/check build_runner` were not detected. See comment: zulip#904 (comment) We normally exclude diffs from generated files through our `.gitattributes` configurations. This happens to `git --quiet -- [<path>...]` too. -a/--text allows us to include those files regardless. Signed-off-by: Zixuan James Li <[email protected]>

Marked as TODO as this might change in the future. See zulip#904 (comment). See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

… required, relying on server 5+, FL 116+ See zulip#904 (comment). See "Feature level 116" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

See "Feature level 73" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

…n server 5+, FL 83+ See "Feature level 83" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

Marked as TODO as this might change in the future. See zulip#904 (comment). See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

…er 5+, FL 88+ See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

… relying on server 5+, FL 89+ See "Feature level 89" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

… 108+. See "Feature level 108" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

…, FL 114+ See "Feature level 114" from Zulip API changelog: https://zulip.com/api/changelog See also: https://zulip.com/api/get-events#update_message Signed-off-by: Zixuan James Li <[email protected]>

….streamId, relying on server 5+, FL 115+. See "Feature level 115" from Zulip API changelog: https://zulip.com/api/changelog See also: https://github.com/zulip/zulip/issues/ 18067 Signed-off-by: Zixuan James Li <[email protected]>

… required, relying on server 5+, FL 116+ See zulip#904 (comment). See "Feature level 116" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

…, relying on server 5+, FL 118+ "topic" is no longer optional when the topic was edited, we also don't need to expect "prev_subject" as opposed to "prev_topic". See "Feature level 118" from Zulip API changelog: https://zulip.com/api/changelog See also: https://zulip.com/api/get-messages#response Signed-off-by: Zixuan James Li <[email protected]>

…ing on server 5+, FL 119+ See "Feature level 119" from Zulip API changelog: https://zulip.com/api/changelog See also: https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/register.3A.20When.20was.20.60unread_msgs.2Epms.5B.5D.2Eother_user_id.60.20added.3F Signed-off-by: Zixuan James Li <[email protected]>

See "Feature level 120" from Zulip API changelog: https://zulip.com/api/changelog See also commit 631f4d6. Fixes: zulip#268 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the server-5 branch 4 times, most recently from de9d000 to 6984723 Compare August 23, 2024 20:40

PIG208 marked this pull request as ready for review August 23, 2024 21:11

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Aug 23, 2024

chrisbobbe reviewed Aug 27, 2024

View reviewed changes

PIG208 force-pushed the server-5 branch from 6984723 to ac3b56a Compare August 27, 2024 04:33

PIG208 force-pushed the server-5 branch from ac3b56a to 594b962 Compare August 29, 2024 06:55

PIG208 force-pushed the server-5 branch from 594b962 to 08bab16 Compare August 29, 2024 17:45

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 29, 2024

PIG208 force-pushed the server-5 branch from 08bab16 to 3a4496e Compare September 7, 2024 04:24

gnprice reviewed Sep 7, 2024

View reviewed changes

PIG208 force-pushed the server-5 branch from 3a4496e to 665bd08 Compare September 10, 2024 17:57

PIG208 force-pushed the server-5 branch from 665bd08 to 95a8714 Compare September 10, 2024 18:22

PIG208 requested review from gnprice and removed request for gnprice September 10, 2024 18:26

gnprice reviewed Sep 16, 2024

View reviewed changes

gnprice marked this pull request as draft September 16, 2024 23:48

PIG208 mentioned this pull request Sep 17, 2024

check: Use --text when potentially diffing generated files #947

Merged

PIG208 force-pushed the server-5 branch from 95a8714 to 1ba230d Compare October 8, 2024 19:22

PIG208 added 12 commits October 23, 2024 16:58

api: Mark User.isBillingAdmin as required, relying on server 5+, FL 73+

79d408d

See "Feature level 73" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

api: Drop is_cross_realm_bot fallback for User.isSystemBot, relying o…

b42ed60

…n server 5+, FL 83+ See "Feature level 83" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

TODO api: Add TODO to modernize GetServerSettingsResult.zulipMergeBase

b19e29e

Marked as TODO as this might change in the future. See zulip#904 (comment). See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

api: Mark InitialSnapshot.zulipMergeBase as required, relying on serv…

666a6f7

…er 5+, FL 88+ See "Feature level 88" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

api: Mark {InitialSnapshot,PerAccountStore}.userSettings as required,…

c170a3c

… relying on server 5+, FL 89+ See "Feature level 89" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

api: Mark WebAuthPayload.userId as required, relying on server 5+, FL…

f5bb081

… 108+. See "Feature level 108" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

api: Mark UpdateMessageEvent fields as required, relying on server 5+…

16d5899

…, FL 114+ See "Feature level 114" from Zulip API changelog: https://zulip.com/api/changelog See also: https://zulip.com/api/get-events#update_message Signed-off-by: Zixuan James Li <[email protected]>

TODO api: Mark GetServerSettingsResult.realmWebPublicAccessEnabled as…

2d475c6

… required, relying on server 5+, FL 116+ See zulip#904 (comment). See "Feature level 116" from Zulip API changelog: https://zulip.com/api/changelog Signed-off-by: Zixuan James Li <[email protected]>

api: Remove legacy getMessageCompat, relying on server 5+, FL 120+

51be4fc

See "Feature level 120" from Zulip API changelog: https://zulip.com/api/changelog See also commit 631f4d6. Fixes: zulip#268 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the server-5 branch 3 times, most recently from da846f2 to 51be4fc Compare October 23, 2024 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start Requiring Zulip Server 5 #904

Start Requiring Zulip Server 5 #904

PIG208 commented Aug 22, 2024 •

edited

Loading

PIG208 commented Aug 23, 2024

chrisbobbe left a comment

chrisbobbe Aug 27, 2024

chrisbobbe Aug 27, 2024

chrisbobbe Aug 27, 2024

PIG208 commented Aug 27, 2024

chrisbobbe commented Aug 28, 2024

PIG208 commented Aug 29, 2024

chrisbobbe commented Aug 29, 2024

gnprice commented Sep 7, 2024

gnprice left a comment

gnprice Sep 7, 2024

PIG208 Sep 10, 2024

gnprice Sep 7, 2024

gnprice Sep 7, 2024

gnprice Sep 7, 2024

gnprice Sep 7, 2024

gnprice Sep 16, 2024

gnprice Sep 7, 2024

gnprice Sep 7, 2024

gnprice Sep 7, 2024

gnprice Sep 7, 2024

PIG208 commented Sep 10, 2024

gnprice left a comment

gnprice Sep 16, 2024

PIG208 Oct 8, 2024

gnprice Sep 16, 2024

gnprice Sep 16, 2024

gnprice Sep 16, 2024

PIG208 Oct 8, 2024

gnprice Sep 16, 2024

PIG208 Oct 8, 2024

gnprice Oct 8, 2024

gnprice commented Sep 16, 2024

		// This field is absent in `realm_users` and `realm_non_active_users`,
		// which contain no system bots; it's present in `cross_realm_bots`.

		final String? zulipMergeBase; // TODO(server-5)
		final String zulipMergeBase;

		final UserSettings? userSettings; // TODO(server-5)
		final UserSettings userSettings;

		final int? userId; // TODO(server-5) new in FL 108
		final int userId;

	int? userId = int.tryParse(userIdStr, radix: 10);
	final userId = int.tryParse(userIdStr, radix: 10);

	int userId = -1, // default is [selfUser.userId]
	int? userId, // default is [selfUser.userId]

		zulipMergeBase: json['zulip_merge_base'] as String?,
		zulipMergeBase: json['zulip_merge_base'] as String,

		// TODO: Modernize this once we get to #267
		final String? zulipMergeBase;

		final int? editTimestamp; // TODO(server-5)
		final int editTimestamp;

Start Requiring Zulip Server 5 #904

Are you sure you want to change the base?

Start Requiring Zulip Server 5 #904

Conversation

PIG208 commented Aug 22, 2024 • edited Loading

PIG208 commented Aug 23, 2024

chrisbobbe 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

PIG208 commented Aug 27, 2024

chrisbobbe commented Aug 28, 2024

PIG208 commented Aug 29, 2024

chrisbobbe commented Aug 29, 2024

gnprice commented Sep 7, 2024

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

PIG208 commented Sep 10, 2024

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

gnprice commented Sep 16, 2024

PIG208 commented Aug 22, 2024 •

edited

Loading