Skip to content

Rename subject/topic in API types, and fix orig/new names in UpdateMessageEvent #751

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 21, 2024
Merged

Rename subject/topic in API types, and fix orig/new names in UpdateMessageEvent #751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
278f9d3
api: Ignore stream_name on UpdateMessageEvent
gnprice Jun 18, 2024
4a4f934
test [nfc]: Add all package:checks getters for Message
gnprice Jun 18, 2024
8778efe
api [nfc]: Rename Message.topic, from "subject"
gnprice Jun 18, 2024
98a2e7c
api [nfc]: Rename Message.matchTopic, from matchSubject
gnprice Jun 18, 2024
dd78af1
api [nfc]: Rename subject/topic on UpdateMessageEvent
gnprice Jun 18, 2024
6c949ea
api [nfc]: Rationalize "orig"/"new" field names on UpdateMessageEvent
gnprice May 5, 2024
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
30 changes: 20 additions & 10 deletions lib/api/model/events.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,8 +584,8 @@ class MessageEvent extends Event {
//
// The other difference in the server API between message objects in these
// events and in the get-messages results is that `matchContent` and
// `matchSubject` are absent here. Already [Message.matchContent] and
// [Message.matchSubject] are optional, so no action is needed on that.
// `matchTopic` are absent here. Already [Message.matchContent] and
// [Message.matchTopic] are optional, so no action is needed on that.
final Message message;

MessageEvent({required super.id, required this.message});
Expand Down Expand Up @@ -617,20 +617,31 @@ class UpdateMessageEvent extends Event {
final bool? renderingOnly; // TODO(server-5)
final int messageId;
final List<int> messageIds;

final List<MessageFlag> flags;
final int? editTimestamp; // TODO(server-5)
final String? streamName;
final int? streamId;

// final String? streamName; // ignore

@JsonKey(name: 'stream_id')
final int? origStreamId;
final int? newStreamId;

final PropagateMode? propagateMode;
final String? origSubject;
final String? subject;

@JsonKey(name: 'orig_subject')
final String? origTopic;
@JsonKey(name: 'subject')
final String? newTopic;

// final List<TopicLink> topicLinks; // TODO handle

final String? origContent;
final String? origRenderedContent;
// final int? prevRenderedContentVersion; // deprecated
final String? content;
final String? renderedContent;

final bool? isMeMessage;

UpdateMessageEvent({
Expand All @@ -641,12 +652,11 @@ class UpdateMessageEvent extends Event {
required this.messageIds,
required this.flags,
required this.editTimestamp,
required this.streamName,
required this.streamId,
required this.origStreamId,
required this.newStreamId,
required this.propagateMode,
required this.origSubject,
required this.subject,
required this.origTopic,
required this.newTopic,
required this.origContent,
required this.origRenderedContent,
required this.content,
Expand Down
14 changes: 6 additions & 8 deletions lib/api/model/events.g.dart
View file
Open in desktop

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

18 changes: 10 additions & 8 deletions lib/api/model/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,8 @@ sealed class Message {
final String senderFullName;
final int senderId;
final String senderRealmStr;
final String subject; // TODO call it "topic" internally; also similar others
@JsonKey(name: 'subject')
final String topic;
Comment on lines -649 to +650
Copy link
Collaborator

Choose a reason for hiding this comment

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

api [nfc]: Rename Message.topic, from "subject"

I wondered what would happen if the payload already contained a 'topic' key, and I wasn't immediately sure. I could imagine servers adding one as part of an API migration.

We just ignore it, it turns out, which is definitely fine for before that API migration happens. But if we rejected it, we'd be putting up an obstacle to the migration. Do we want to test that we don't reject it?

    test('does not reject if servers add a new `topic` field', () {
      check(() => Message.fromJson(baseStreamJson()..['topic'] = 'hello'))
        .returnsNormally();
    });

(Similarly for the other renames in this PR, I guess.)

Copy link
Member Author

Choose a reason for hiding this comment

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

In general our API types and their fromJson constructors always have the property that they don't notice or care if the JSON they receive has additional properties in it that weren't expected. That's a fact about json_serializable / json_annotation (there's a flag to get the opposite behavior, JsonSerializable.disallowUnrecognizedKeys, but it's off by default), and it's one we rely on regularly for routine forward-compatibility with server changes.

So I'm content to leave that as part of the routine boring behavior of the generated deserialization code, which we don't test.

// final List<string> submessages; // TODO handle
final int timestamp;
String get type;
Expand All @@ -656,7 +657,8 @@ sealed class Message {
@JsonKey(fromJson: _flagsFromJson)
List<MessageFlag> flags; // Unrecognized flags won't roundtrip through {to,from}Json.
final String? matchContent;
final String? matchSubject;
@JsonKey(name: 'match_subject')
final String? matchTopic;

static Reactions? _reactionsFromJson(dynamic json) {
final list = (json as List<dynamic>);
Expand Down Expand Up @@ -685,11 +687,11 @@ sealed class Message {
required this.senderFullName,
required this.senderId,
required this.senderRealmStr,
required this.subject,
required this.topic,
required this.timestamp,
required this.flags,
required this.matchContent,
required this.matchSubject,
required this.matchTopic,
});

factory Message.fromJson(Map<String, dynamic> json) {
Expand Down Expand Up @@ -750,11 +752,11 @@ class StreamMessage extends Message {
required super.senderFullName,
required super.senderId,
required super.senderRealmStr,
required super.subject,
required super.topic,
required super.timestamp,
required super.flags,
required super.matchContent,
required super.matchSubject,
required super.matchTopic,
required this.displayRecipient,
required this.streamId,
});
Expand Down Expand Up @@ -852,11 +854,11 @@ class DmMessage extends Message {
required super.senderFullName,
required super.senderId,
required super.senderRealmStr,
required super.subject,
required super.topic,
required super.timestamp,
required super.flags,
required super.matchContent,
required super.matchSubject,
required super.matchTopic,
required this.displayRecipient,
});

Expand Down
16 changes: 8 additions & 8 deletions lib/api/model/model.g.dart
View file
Open in desktop

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

6 changes: 3 additions & 3 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ mixin _MessageSequence {
bool haveSameRecipient(Message prevMessage, Message message) {
if (prevMessage is StreamMessage && message is StreamMessage) {
if (prevMessage.streamId != message.streamId) return false;
if (prevMessage.subject != message.subject) return false;
if (prevMessage.topic != message.topic) return false;
} else if (prevMessage is DmMessage && message is DmMessage) {
if (!_equalIdSequences(prevMessage.allRecipientIds, message.allRecipientIds)) {
return false;
Expand Down Expand Up @@ -335,14 +335,14 @@ class MessageListView with ChangeNotifier, _MessageSequence {
case CombinedFeedNarrow():
return switch (message) {
StreamMessage() =>
store.isTopicVisible(message.streamId, message.subject),
store.isTopicVisible(message.streamId, message.topic),
DmMessage() => true,
};

case StreamNarrow(:final streamId):
assert(message is StreamMessage && message.streamId == streamId);
if (message is! StreamMessage) return false;
return store.isTopicVisibleInStream(streamId, message.subject);
return store.isTopicVisibleInStream(streamId, message.topic);

case TopicNarrow():
case DmNarrow():
Expand Down
4 changes: 2 additions & 2 deletions lib/model/narrow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class TopicNarrow extends Narrow implements SendableNarrow {
const TopicNarrow(this.streamId, this.topic);

factory TopicNarrow.ofMessage(StreamMessage message) {
return TopicNarrow(message.streamId, message.subject);
return TopicNarrow(message.streamId, message.topic);
}

final int streamId;
Expand All @@ -104,7 +104,7 @@ class TopicNarrow extends Narrow implements SendableNarrow {
@override
bool containsMessage(Message message) {
return (message is StreamMessage
&& message.streamId == streamId && message.subject == topic);
&& message.streamId == streamId && message.topic == topic);
}

@override
Expand Down
2 changes: 1 addition & 1 deletion lib/model/unreads.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ class Unreads extends ChangeNotifier {

switch (message) {
case StreamMessage():
_addLastInStreamTopic(message.id, message.streamId, message.subject);
_addLastInStreamTopic(message.id, message.streamId, message.topic);
case DmMessage():
final narrow = DmNarrow.ofMessage(message, selfUserId: selfUserId);
_addLastInDm(message.id, narrow);
Expand Down
2 changes: 1 addition & 1 deletion lib/widgets/action_sheet.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton {
&& topicController.textNormalized == kNoTopicTopic
&& message is StreamMessage
) {
topicController.value = TextEditingValue(text: message.subject);
topicController.value = TextEditingValue(text: message.topic);
}
final tag = composeBoxController.contentController
.registerQuoteAndReplyStart(PerAccountStoreWidget.of(messageListContext),
Expand Down
2 changes: 1 addition & 1 deletion lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ class StreamMessageRecipientHeader extends StatelessWidget {
// https://github.com/zulip/zulip-mobile/issues/5511
final store = PerAccountStoreWidget.of(context);

final topic = message.subject;
final topic = message.topic;

final subscription = store.subscriptions[message.streamId];
final Color backgroundColor;
Expand Down
19 changes: 19 additions & 0 deletions test/api/model/events_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,25 @@ extension MessageEventChecks on Subject<MessageEvent> {
Subject<Message> get message => has((e) => e.message, 'message');
}

extension UpdateMessageEventChecks on Subject<UpdateMessageEvent> {
Subject<int?> get userId => has((e) => e.userId, 'userId');
Subject<bool?> get renderingOnly => has((e) => e.renderingOnly, 'renderingOnly');
Subject<int> get messageId => has((e) => e.messageId, 'messageId');
Subject<List<int>> get messageIds => has((e) => e.messageIds, 'messageIds');
Subject<List<MessageFlag>> get flags => has((e) => e.flags, 'flags');
Subject<int?> get editTimestamp => has((e) => e.editTimestamp, 'editTimestamp');
Subject<int?> get origStreamId => has((e) => e.origStreamId, 'origStreamId');
Subject<int?> get newStreamId => has((e) => e.newStreamId, 'newStreamId');
Subject<PropagateMode?> get propagateMode => has((e) => e.propagateMode, 'propagateMode');
Subject<String?> get origTopic => has((e) => e.origTopic, 'origTopic');
Subject<String?> get newTopic => has((e) => e.newTopic, 'newTopic');
Subject<String?> get origContent => has((e) => e.origContent, 'origContent');
Subject<String?> get origRenderedContent => has((e) => e.origRenderedContent, 'origRenderedContent');
Subject<String?> get content => has((e) => e.content, 'content');
Subject<String?> get renderedContent => has((e) => e.renderedContent, 'renderedContent');
Subject<bool?> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
}

extension HeartbeatEventChecks on Subject<HeartbeatEvent> {
// No properties not covered by Event.
}
Expand Down
33 changes: 33 additions & 0 deletions test/api/model/events_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,39 @@ void main() {
check(mkEvent([MessageFlag.read])).message.flags.deepEquals([MessageFlag.read]);
});

group('update_message', () {
final message = eg.streamMessage();
final baseJson = {
'id': 1,
'type': 'update_message',
'user_id': eg.selfUser.userId,
'rendering_only': false,
'message_id': message.id,
'message_ids': [message.id],
'flags': <String>[],
'edit_timestamp': 1718741351,
'stream_id': eg.stream().streamId,
};

test('stream_id -> origStreamId', () {
check(Event.fromJson({ ...baseJson,
'stream_id': 1,
'new_stream_id': 2,
}) as UpdateMessageEvent)
..origStreamId.equals(1)
..newStreamId.equals(2);
});

test('orig_subject -> origTopic, subject -> newTopic', () {
check(Event.fromJson({ ...baseJson,
'orig_subject': 'foo',
'subject': 'bar',
}) as UpdateMessageEvent)
..origTopic.equals('foo')
..newTopic.equals('bar');
});
});

test('delete_message: require streamId and topic for stream messages', () {
check(() => DeleteMessageEvent.fromJson({
'id': 1,
Expand Down
16 changes: 13 additions & 3 deletions test/api/model/model_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,24 @@ extension StreamColorSwatchChecks on Subject<StreamColorSwatch> {
}

extension MessageChecks on Subject<Message> {
Subject<int> get id => has((e) => e.id, 'id');
Subject<String> get client => has((e) => e.client, 'client');
Subject<String> get content => has((e) => e.content, 'content');
Subject<String> get contentType => has((e) => e.contentType, 'contentType');
Subject<int> get id => has((e) => e.id, 'id');
Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');
Subject<Reactions?> get reactions => has((e) => e.reactions, 'reactions');
Subject<int> get recipientId => has((e) => e.recipientId, 'recipientId');
Subject<String> get senderEmail => has((e) => e.senderEmail, 'senderEmail');
Subject<String> get senderFullName => has((e) => e.senderFullName, 'senderFullName');
Subject<int> get senderId => has((e) => e.senderId, 'senderId');
Subject<String> get senderRealmStr => has((e) => e.senderRealmStr, 'senderRealmStr');
Subject<String> get topic => has((e) => e.topic, 'topic');
Subject<int> get timestamp => has((e) => e.timestamp, 'timestamp');
Subject<String> get type => has((e) => e.type, 'type');
Subject<List<MessageFlag>> get flags => has((e) => e.flags, 'flags');

// TODO accessors for other fields
Subject<String?> get matchContent => has((e) => e.matchContent, 'matchContent');
Subject<String?> get matchTopic => has((e) => e.matchTopic, 'matchTopic');
}

extension ReactionsChecks on Subject<Reactions> {
Expand Down
Loading
Loading