Skip to content

Start Requiring Zulip Server 5 #904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -708,13 +708,13 @@ class UpdateMessageEvent extends Event {
@JsonKey(includeToJson: true)
String get type => 'update_message';

final int? userId; // TODO(server-5)
final bool? renderingOnly; // TODO(server-5)
final int userId;
final bool renderingOnly;
Comment on lines -711 to +712
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

final int messageId;
final List<int> messageIds;

final List<MessageFlag> flags;
final int? editTimestamp; // TODO(server-5)
final int editTimestamp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// final String? streamName; // ignore

Expand Down
6 changes: 3 additions & 3 deletions lib/api/model/events.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 2 additions & 14 deletions lib/api/model/initial_snapshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class InitialSnapshot {
final int lastEventId;
final int zulipFeatureLevel;
final String zulipVersion;
final String? zulipMergeBase; // TODO(server-5)
final String zulipMergeBase;

final List<String> alertWords;

Expand Down Expand Up @@ -54,13 +54,7 @@ class InitialSnapshot {

final List<ZulipStream> streams;

// Servers pre-5.0 don't have `user_settings`, and instead provide whatever
// user settings they support at toplevel in the initial snapshot. Since we're
// likely to desupport pre-5.0 servers before wide release, we prefer to
// ignore the toplevel fields and use `user_settings` where present instead,
// even at the expense of functionality with pre-5.0 servers.
// TODO(server-5) remove pre-5.0 comment
final UserSettings? userSettings; // TODO(server-5)
final UserSettings userSettings;

final List<UserTopicItem>? userTopics; // TODO(server-6)

Expand Down Expand Up @@ -312,15 +306,9 @@ class UnreadMessagesSnapshot {
/// An item in [UnreadMessagesSnapshot.dms].
@JsonSerializable(fieldRename: FieldRename.snake)
class UnreadDmSnapshot {
@JsonKey(readValue: _readOtherUserId)
final int otherUserId;
Comment on lines 308 to 309
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

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

that's not the class this field is on 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'];
-  }
-

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

final List<int> unreadMessageIds;

// TODO(server-5): Simplify away.
static dynamic _readOtherUserId(Map<dynamic, dynamic> json, String key) {
return json[key] ?? json['sender_id'];
}

UnreadDmSnapshot({
required this.otherUserId,
required this.unreadMessageIds,
Expand Down
12 changes: 4 additions & 8 deletions lib/api/model/initial_snapshot.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 10 additions & 19 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

final bool isBot;
final int? botType; // TODO enum
int? botOwnerId;
Expand All @@ -221,7 +221,9 @@ class User {
@JsonKey(readValue: _readProfileData)
Map<int, ProfileFieldUserData>? profileData;

@JsonKey(readValue: _readIsSystemBot)
// This field is absent in `realm_users` and `realm_non_active_users`,
// which contain no system bots; it's present in `cross_realm_bots`.
@JsonKey(defaultValue: false)
final bool isSystemBot;

static Map<String, dynamic>? _readProfileData(Map<dynamic, dynamic> json, String key) {
Expand All @@ -233,14 +235,6 @@ class User {
return (value != null && value.isNotEmpty) ? value : null;
}

static bool _readIsSystemBot(Map<dynamic, dynamic> json, String key) {
// This field is absent in `realm_users` and `realm_non_active_users`,
// which contain no system bots; it's present in `cross_realm_bots`.
Comment on lines -237 to -238
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (json[key] as bool?)
?? (json['is_cross_realm_bot'] as bool?) // TODO(server-5): renamed to `is_system_bot`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+

?? false;
}

User({
required this.userId,
required this.deliveryEmail,
Expand Down Expand Up @@ -924,18 +918,15 @@ enum MessageEditState {
continue;
}

// TODO(server-5) prev_subject was the old name of prev_topic on pre-5.0 servers
final prevTopicStr = (entry['prev_topic'] ?? entry['prev_subject']) as String?;
final prevTopicStr = entry['prev_topic'] as String?;
final prevTopic = prevTopicStr == null ? null : TopicName.fromJson(prevTopicStr);
final topicStr = entry['topic'] as String?;
final topic = topicStr == null ? null : TopicName.fromJson(topicStr);
if (prevTopic != null) {
// TODO(server-5) pre-5.0 servers do not have the 'topic' field
if (topic == null) {
hasMoved = true;
} else {
hasMoved |= !topicMoveWasResolveOrUnresolve(topic, prevTopic);
}
if (topic != null || prevTopic != null) {
// Crunchy-shell validation: Both are present if the topic was edited
topic as TopicName;
prevTopic as TopicName;
hasMoved |= !topicMoveWasResolveOrUnresolve(topic, prevTopic);
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/api/model/model.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 4 additions & 9 deletions lib/api/model/web_auth.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import 'package:flutter/foundation.dart';
class WebAuthPayload {
final Uri realm;
final String email;
final int? userId; // TODO(server-5) new in FL 108
final int userId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+.

)

final String otpEncryptedApiKey;

WebAuthPayload._({
Expand All @@ -25,21 +25,16 @@ class WebAuthPayload {
queryParameters: {
'realm': String realmStr,
'email': String email,
// 'user_id' handled below
'user_id': String userIdStr,
'otp_encrypted_api_key': String otpEncryptedApiKey,
},
)
) {
final Uri? realm = Uri.tryParse(realmStr);
if (realm == null) throw const FormatException();

// TODO(server-5) require in queryParameters (new in FL 108)
final userIdStr = url.queryParameters['user_id'];
int? userId;
if (userIdStr != null) {
userId = int.tryParse(userIdStr, radix: 10);
if (userId == null) throw const FormatException();
}
final userId = int.tryParse(userIdStr, radix: 10);
if (userId == null) throw const FormatException();

if (!RegExp(r'^[0-9a-fA-F]{64}$').hasMatch(otpEncryptedApiKey)) {
throw const FormatException();
Expand Down
6 changes: 1 addition & 5 deletions lib/api/notifications.dart
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,9 @@ sealed class FcmMessageRecipient {
@JsonSerializable(fieldRename: FieldRename.snake, createToJson: false)
@_IntConverter()
class FcmMessageChannelRecipient extends FcmMessageRecipient {
// Sending the stream ID in notifications is new in Zulip Server 5.
// But handling the lack of it would add complication, and we don't strictly
// need to -- we intend (#268) to cut pre-server-5 support before beta release.
// TODO(server-5): cut comment
final int streamId;

// Current servers (as of 2023) always send the stream name. But
// Current servers (as of 2025) always send the stream name. But
// future servers might not, once clients get the name from local data.
// So might as well be ready.
@JsonKey(name: 'stream')
Expand Down
48 changes: 0 additions & 48 deletions lib/api/route/messages.dart
Original file line number Diff line number Diff line change
@@ -1,64 +1,16 @@
import 'package:json_annotation/json_annotation.dart';

import '../core.dart';
import '../exception.dart';
import '../model/model.dart';
import '../model/narrow.dart';

part 'messages.g.dart';

/// Convenience function to get a single message from any server.
///
/// This encapsulates a server-feature check.
///
/// 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, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

required int messageId,
bool? applyMarkdown,
}) async {
final useLegacyApi = connection.zulipFeatureLevel! < 120;
if (useLegacyApi) {
final response = await getMessages(connection,
narrow: [ApiNarrowMessageId(messageId)],
anchor: NumericAnchor(messageId),
numBefore: 0,
numAfter: 0,
applyMarkdown: applyMarkdown,

// Hard-code this param to `true`, as the new single-message API
// effectively does:
// https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60client_gravatar.60.20in.20.60messages.2F.7Bmessage_id.7D.60/near/1418337
clientGravatar: true,
);
return response.messages.firstOrNull;
} else {
try {
final response = await getMessage(connection,
messageId: messageId,
applyMarkdown: applyMarkdown,
);
return response.message;
} on ZulipApiException catch (e) {
if (e.code == 'BAD_REQUEST') {
// Servers use this code when the message doesn't exist, according to
// the example in the doc.
return null;
}
rethrow;
}
}
}

/// https://zulip.com/api/get-message
///
/// This binding only supports feature levels 120+.
// TODO(server-5) remove FL 120+ mention in doc, and the related `assert`
Future<GetMessageResult> getMessage(ApiConnection connection, {
required int messageId,
bool? applyMarkdown,
}) {
assert(connection.zulipFeatureLevel! >= 120);
return connection.get('getMessage', GetMessageResult.fromJson, 'messages/$messageId', {
if (applyMarkdown != null) 'apply_markdown': applyMarkdown,
});
Expand Down
5 changes: 3 additions & 2 deletions lib/api/route/realm.dart
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class GetServerSettingsResult {

final int zulipFeatureLevel;
final String zulipVersion;
final String? zulipMergeBase; // TODO(server-5)
// TODO(server-5): Modernize this once we get to #267.
final String? zulipMergeBase;

final bool pushNotificationsEnabled;
final bool isIncompatible;
Expand All @@ -50,7 +51,7 @@ class GetServerSettingsResult {
final String realmName;
final String realmIcon;
final String realmDescription;
final bool? realmWebPublicAccessEnabled; // TODO(server-5)
final bool realmWebPublicAccessEnabled;

GetServerSettingsResult({
required this.authenticationMethods,
Expand Down
2 changes: 1 addition & 1 deletion lib/api/route/realm.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions lib/model/emoji.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ sealed class EmojiDisplay {

EmojiDisplay({required this.emojiName});

EmojiDisplay resolve(UserSettings? userSettings) { // TODO(server-5)
EmojiDisplay resolve(UserSettings userSettings) {
if (this is TextEmojiDisplay) return this;
if (userSettings?.emojiset == Emojiset.text) {
if (userSettings.emojiset == Emojiset.text) {
return TextEmojiDisplay(emojiName: emojiName);
}
return this;
Expand Down
4 changes: 1 addition & 3 deletions lib/model/message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,7 @@ class MessageStoreImpl with MessageStore {
}

void _handleUpdateMessageEventTimestamp(UpdateMessageEvent event) {
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
if (event.editTimestamp == null || isRenderingOnly) {
if (event.renderingOnly) {
// A rendering-only update gets omitted from the message edit history,
// and [Message.lastEditTimestamp] is the last timestamp of that history.
// So on a rendering-only update, the timestamp doesn't get updated.
Expand Down
8 changes: 4 additions & 4 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
/// Will throw if called after [dispose] has been called.
Account get account => _globalStore.getAccount(accountId)!;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should get a corresponding update in PerAccountStoreChecks.

(Similarly in other commits.)


final TypingNotifier typingNotifier;

Expand Down Expand Up @@ -626,11 +626,11 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
}
switch (event.property!) {
case UserSettingName.twentyFourHourTime:
userSettings?.twentyFourHourTime = event.value as bool;
userSettings.twentyFourHourTime = event.value as bool;
case UserSettingName.displayEmojiReactionUsers:
userSettings?.displayEmojiReactionUsers = event.value as bool;
userSettings.displayEmojiReactionUsers = event.value as bool;
case UserSettingName.emojiset:
userSettings?.emojiset = event.value as Emojiset;
userSettings.emojiset = event.value as Emojiset;
}
notifyListeners();

Expand Down
Loading