diff --git a/README.md b/README.md index 4c8cf7824f..98490cd6a7 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,15 @@ to keep the generated files up to date: $ dart run build_runner watch --delete-conflicting-outputs ``` +In our API types, constructors should generally avoid default values for +their parameters, even `null`. This means writing e.g. `required this.foo` +rather than just `this.foo`, even when `foo` is nullable. +This is because it's common in the Zulip API for a null or missing value +to be quite salient in meaning, and not a boring value appropriate for a +default, so that it's best to ensure callers make an explicit choice. +If passing explicit values in tests is cumbersome, a factory function +in `test/example_data.dart` is an appropriate way to share defaults. + ### Upgrading Flutter diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index 241aa70a41..4e2723eb59 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -249,6 +249,9 @@ class RealmUserUpdateEvent extends RealmUserEvent { RealmUserUpdateEvent({ required super.id, required this.userId, + // Unlike in most of the API bindings, we leave these constructor arguments + // optional. That's because in these events only one or a handful of these + // will appear in a given event value. this.fullName, this.avatarUrl, this.avatarVersion, @@ -384,23 +387,23 @@ class UpdateMessageEvent extends Event { UpdateMessageEvent({ required super.id, - this.userId, - this.renderingOnly, + required this.userId, + required this.renderingOnly, required this.messageId, required this.messageIds, required this.flags, - this.editTimestamp, - this.streamName, - this.streamId, - this.newStreamId, - this.propagateMode, - this.origSubject, - this.subject, - this.origContent, - this.origRenderedContent, - this.content, - this.renderedContent, - this.isMeMessage, + required this.editTimestamp, + required this.streamName, + required this.streamId, + required this.newStreamId, + required this.propagateMode, + required this.origSubject, + required this.subject, + required this.origContent, + required this.origRenderedContent, + required this.content, + required this.renderedContent, + required this.isMeMessage, }); factory UpdateMessageEvent.fromJson(Map json) => @@ -434,8 +437,8 @@ class DeleteMessageEvent extends Event { required super.id, required this.messageIds, required this.messageType, - this.streamId, - this.topic, + required this.streamId, + required this.topic, }); factory DeleteMessageEvent.fromJson(Map json) { diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 8d7f983453..6f38f4fed6 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -73,11 +73,11 @@ class InitialSnapshot { } InitialSnapshot({ - this.queueId, + required this.queueId, required this.lastEventId, required this.zulipFeatureLevel, required this.zulipVersion, - this.zulipMergeBase, + required this.zulipMergeBase, required this.alertWords, required this.customProfileFields, required this.recentPrivateConversations, diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 6cf4a06f01..86e70ef965 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -142,7 +142,7 @@ class User { Map? profileData; @JsonKey(readValue: _readIsSystemBot) - bool? isSystemBot; // TODO(server-5) + bool isSystemBot; static Map? _readProfileData(Map json, String key) { final value = (json[key] as Map?); @@ -153,8 +153,12 @@ class User { return (value != null && value.isNotEmpty) ? value : null; } - static bool? _readIsSystemBot(Map json, String key) { - return json[key] ?? json['is_cross_realm_bot']; + static bool _readIsSystemBot(Map 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`. + return json[key] + ?? json['is_cross_realm_bot'] // TODO(server-5): renamed to `is_system_bot` + ?? false; } User({ @@ -169,14 +173,14 @@ class User { required this.isGuest, required this.isBillingAdmin, required this.isBot, - this.botType, - this.botOwnerId, + required this.botType, + required this.botOwnerId, required this.role, required this.timezone, required this.avatarUrl, required this.avatarVersion, - this.profileData, - this.isSystemBot, + required this.profileData, + required this.isSystemBot, }); factory User.fromJson(Map json) => _$UserFromJson(json); @@ -190,7 +194,14 @@ class ProfileFieldUserData { final String value; final String? renderedValue; - ProfileFieldUserData({required this.value, this.renderedValue}); + ProfileFieldUserData({ + required this.value, + // Unlike in most of the API bindings, we leave this constructor argument + // optional. That's because for most types of custom profile fields, + // this property is always indeed absent, and because this constructor is + // otherwise convenient to write many calls to in our test code. + this.renderedValue, + }); factory ProfileFieldUserData.fromJson(Map json) => _$ProfileFieldUserDataFromJson(json); @@ -317,22 +328,22 @@ class Subscription { required this.renderedDescription, required this.dateCreated, required this.inviteOnly, - this.desktopNotifications, - this.emailNotifications, - this.wildcardMentionsNotify, - this.pushNotifications, - this.audibleNotifications, + required this.desktopNotifications, + required this.emailNotifications, + required this.wildcardMentionsNotify, + required this.pushNotifications, + required this.audibleNotifications, required this.pinToTop, required this.emailAddress, required this.isMuted, - this.isWebPublic, + required this.isWebPublic, required this.color, required this.streamPostPolicy, - this.messageRetentionDays, + required this.messageRetentionDays, required this.historyPublicToSubscribers, - this.firstMessageId, - this.streamWeeklyTraffic, - this.canRemoveSubscribersGroupId, + required this.firstMessageId, + required this.streamWeeklyTraffic, + required this.canRemoveSubscribersGroupId, }); factory Subscription.fromJson(Map json) => @@ -395,7 +406,7 @@ sealed class Message { required this.contentType, required this.id, required this.isMeMessage, - this.lastEditTimestamp, + required this.lastEditTimestamp, required this.reactions, required this.recipientId, required this.senderEmail, @@ -405,8 +416,8 @@ sealed class Message { required this.subject, required this.timestamp, required this.flags, - this.matchContent, - this.matchSubject, + required this.matchContent, + required this.matchSubject, }); factory Message.fromJson(Map json) { @@ -460,7 +471,7 @@ class StreamMessage extends Message { required super.contentType, required super.id, required super.isMeMessage, - super.lastEditTimestamp, + required super.lastEditTimestamp, required super.reactions, required super.recipientId, required super.senderEmail, @@ -470,8 +481,8 @@ class StreamMessage extends Message { required super.subject, required super.timestamp, required super.flags, - super.matchContent, - super.matchSubject, + required super.matchContent, + required super.matchSubject, required this.displayRecipient, required this.streamId, }); @@ -562,7 +573,7 @@ class DmMessage extends Message { required super.contentType, required super.id, required super.isMeMessage, - super.lastEditTimestamp, + required super.lastEditTimestamp, required super.reactions, required super.recipientId, required super.senderEmail, @@ -572,8 +583,8 @@ class DmMessage extends Message { required super.subject, required super.timestamp, required super.flags, - super.matchContent, - super.matchSubject, + required super.matchContent, + required super.matchSubject, required this.displayRecipient, }); diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index 4a9fcb7640..74e7432f2e 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -95,7 +95,7 @@ User _$UserFromJson(Map json) => User( (k, e) => MapEntry(int.parse(k), ProfileFieldUserData.fromJson(e as Map)), ), - isSystemBot: User._readIsSystemBot(json, 'is_system_bot') as bool?, + isSystemBot: User._readIsSystemBot(json, 'is_system_bot') as bool, ); Map _$UserToJson(User instance) => { diff --git a/lib/api/route/account.dart b/lib/api/route/account.dart index e678932485..fdf57d1ce6 100644 --- a/lib/api/route/account.dart +++ b/lib/api/route/account.dart @@ -32,7 +32,7 @@ class FetchApiKeyResult { FetchApiKeyResult({ required this.apiKey, required this.email, - this.userId, + required this.userId, }); factory FetchApiKeyResult.fromJson(Map json) => diff --git a/lib/api/route/events.dart b/lib/api/route/events.dart index 68ea815086..1565fe422c 100644 --- a/lib/api/route/events.dart +++ b/lib/api/route/events.dart @@ -41,7 +41,7 @@ class GetEventsResult { GetEventsResult({ required this.events, - this.queueId, + required this.queueId, }); factory GetEventsResult.fromJson(Map json) => diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index a82b38159f..21638ee44c 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -226,11 +226,9 @@ class DmDestination extends MessageDestination { @JsonSerializable(fieldRename: FieldRename.snake) class SendMessageResult { final int id; - final String? deliverAt; SendMessageResult({ required this.id, - this.deliverAt, }); factory SendMessageResult.fromJson(Map json) => diff --git a/lib/api/route/messages.g.dart b/lib/api/route/messages.g.dart index 6c34be86d7..3eea6dc1e1 100644 --- a/lib/api/route/messages.g.dart +++ b/lib/api/route/messages.g.dart @@ -43,13 +43,11 @@ Map _$GetMessagesResultToJson(GetMessagesResult instance) => SendMessageResult _$SendMessageResultFromJson(Map json) => SendMessageResult( id: json['id'] as int, - deliverAt: json['deliver_at'] as String?, ); Map _$SendMessageResultToJson(SendMessageResult instance) => { 'id': instance.id, - 'deliver_at': instance.deliverAt, }; UploadFileResult _$UploadFileResultFromJson(Map json) => diff --git a/lib/api/route/realm.dart b/lib/api/route/realm.dart index 890709316e..7a8678de5f 100644 --- a/lib/api/route/realm.dart +++ b/lib/api/route/realm.dart @@ -52,7 +52,7 @@ class GetServerSettingsResult { required this.authenticationMethods, required this.zulipFeatureLevel, required this.zulipVersion, - this.zulipMergeBase, + required this.zulipMergeBase, required this.pushNotificationsEnabled, required this.isIncompatible, required this.emailAuthEnabled, @@ -61,7 +61,7 @@ class GetServerSettingsResult { required this.realmName, required this.realmIcon, required this.realmDescription, - this.realmWebPublicAccessEnabled, + required this.realmWebPublicAccessEnabled, }); factory GetServerSettingsResult.fromJson(Map json) => diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 743eb50c21..90d6177e5a 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -59,9 +59,9 @@ void main() { }); test('is_system_bot', () { - check(mkUser({}).isSystemBot).isNull(); - check(mkUser({'is_cross_realm_bot': true}).isSystemBot).equals(true); - check(mkUser({'is_system_bot': true}).isSystemBot).equals(true); + check(mkUser({}).isSystemBot).isFalse(); + check(mkUser({'is_cross_realm_bot': true}).isSystemBot).isTrue(); + check(mkUser({'is_system_bot': true}).isSystemBot).isTrue(); }); }); diff --git a/test/api/route/route_checks.dart b/test/api/route/route_checks.dart index 8fe2b7fa6f..6d310ab200 100644 --- a/test/api/route/route_checks.dart +++ b/test/api/route/route_checks.dart @@ -3,7 +3,6 @@ import 'package:zulip/api/route/messages.dart'; extension SendMessageResultChecks on Subject { Subject get id => has((e) => e.id, 'id'); - Subject get deliverAt => has((e) => e.deliverAt, 'deliverAt'); } // TODO add similar extensions for other classes in api/route/*.dart diff --git a/test/example_data.dart b/test/example_data.dart index b18479879b..137ab923bf 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -42,11 +42,14 @@ User user({ isGuest: false, isBillingAdmin: false, isBot: false, + botType: null, + botOwnerId: null, role: UserRole.member, timezone: 'UTC', avatarUrl: avatarUrl, avatarVersion: 0, profileData: profileData, + isSystemBot: false, ); } @@ -279,6 +282,40 @@ const _unreadMsgs = unreadMsgs; // Events. // +UpdateMessageEvent updateMessageEditEvent( + Message origMessage, { + int? userId = -1, // null means null; default is [selfUser.userId] + bool? renderingOnly = false, + int? messageId, + List? flags, + int? editTimestamp, + String? streamName, + String? renderedContent, + bool isMeMessage = false, +}) { + messageId ??= origMessage.id; + return UpdateMessageEvent( + id: 0, + userId: userId == -1 ? selfUser.userId : userId, + renderingOnly: renderingOnly, + messageId: messageId, + messageIds: [messageId], + flags: flags ?? origMessage.flags, + editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp + streamName: streamName, + streamId: origMessage is StreamMessage ? origMessage.streamId : null, + newStreamId: null, + propagateMode: null, + origSubject: null, + subject: null, + origContent: 'some probably-mismatched old Markdown', + origRenderedContent: origMessage.content, + content: 'some probably-mismatched new Markdown', + renderedContent: renderedContent ?? origMessage.content, + isMeMessage: isMeMessage, + ); +} + UpdateMessageFlagsRemoveEvent updateMessageFlagsRemoveEvent( MessageFlag flag, Iterable messages, { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index c49978ef58..68706f0be1 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -265,16 +265,11 @@ void main() async { test('update a message', () async { final originalMessage = eg.streamMessage( content: "

Hello, world

"); - final updateEvent = UpdateMessageEvent( - id: 1, - messageId: originalMessage.id, - messageIds: [originalMessage.id], + final updateEvent = eg.updateMessageEditEvent(originalMessage, flags: [MessageFlag.starred], renderedContent: "

Hello, edited

", editTimestamp: 99999, isMeMessage: true, - userId: 1, - renderingOnly: false, ); prepare(); await prepareMessages(foundOldest: true, messages: [originalMessage]); @@ -299,15 +294,9 @@ void main() async { test('ignore when message not present', () async { final originalMessage = eg.streamMessage( content: "

Hello, world

"); - final updateEvent = UpdateMessageEvent( - id: 1, + final updateEvent = eg.updateMessageEditEvent(originalMessage, messageId: originalMessage.id + 1, - messageIds: [originalMessage.id + 1], - flags: originalMessage.flags, renderedContent: "

Hello, edited

", - editTimestamp: 99999, - userId: 1, - renderingOnly: false, ); prepare(); await prepareMessages(foundOldest: true, messages: [originalMessage]); @@ -324,11 +313,7 @@ void main() async { final originalMessage = eg.streamMessage( lastEditTimestamp: 78492, content: "

Hello, world

"); - final updateEvent = UpdateMessageEvent( - id: 1, - messageId: originalMessage.id, - messageIds: [originalMessage.id], - flags: originalMessage.flags, + final updateEvent = eg.updateMessageEditEvent(originalMessage, renderedContent: "

Hello, world

Some link preview
", editTimestamp: 99999, renderingOnly: legacy ? null : true, @@ -618,10 +603,8 @@ void main() async { checkNotifiedOnce(); // Then test maybeUpdateMessage, where a header is and remains needed… - UpdateMessageEvent updateEvent(Message message) => UpdateMessageEvent( - id: 1, messageId: message.id, messageIds: [message.id], - flags: message.flags, - renderedContent: '${message.content}

edited

', + UpdateMessageEvent updateEvent(Message message) => eg.updateMessageEditEvent( + message, renderedContent: '${message.content}

edited

', ); model.maybeUpdateMessage(updateEvent(model.messages.first)); checkNotifiedOnce(); diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 0d2206f10a..dc1b7cce4e 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -286,7 +286,7 @@ void main() { if (!isKnownToModel) { check(because: "no crash if message is in model's blindspots", () => model.handleUpdateMessageEvent( - UpdateMessageEvent(id: 0, messageId: message.id, messageIds: [], flags: newFlags), + eg.updateMessageEditEvent(message, flags: newFlags), )).returnsNormally(); // Rarely, this event will be about an unread that's unknown // to the model, or at least one of the model's components; @@ -307,7 +307,7 @@ void main() { continue; } model.handleUpdateMessageEvent( - UpdateMessageEvent(id: 0, messageId: message.id, messageIds: [], flags: newFlags), + eg.updateMessageEditEvent(message, flags: newFlags), ); if ( @@ -376,7 +376,13 @@ void main() { streamId: message.streamId, topic: message.subject, ), - DmMessage() => DeleteMessageEvent(id: 0, messageType: MessageType.private, messageIds: [message.id]), + DmMessage() => DeleteMessageEvent( + id: 0, + messageType: MessageType.private, + messageIds: [message.id], + streamId: null, + topic: null, + ), }; model.handleDeleteMessageEvent(event); checkNotifiedOnce(); @@ -412,6 +418,8 @@ void main() { id: 0, messageIds: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], messageType: MessageType.private, + streamId: null, + topic: null, )); checkNotifiedOnce(); checkMatchesMessages([]); @@ -445,6 +453,8 @@ void main() { id: 0, messageIds: [message.id], messageType: MessageType.private, + streamId: null, + topic: null, )); // TODO improve implementation; then: // checkNotNotified();