Skip to content

Commit be550c4

Browse files
PIG208PIG208
committed
unreads: Handle updates when there are moved messages
Signed-off-by: Zixuan James Li <[email protected]>
1 parent e14008e commit be550c4

File tree

2 files changed

+246
-5
lines changed

2 files changed

+246
-5
lines changed

‎lib/model/unreads.dart

+40-5
Original file line numberDiff line numberDiff line change
@@ -259,10 +259,8 @@ class Unreads extends ChangeNotifier {
259259
(f) => f == MessageFlag.mentioned || f == MessageFlag.wildcardMentioned,
260260
);
261261

262-
// We assume this event can't signal a change in a message's 'read' flag.
263-
// TODO can it actually though, when it's about messages being moved into an
264-
// unsubscribed stream?
265-
// https://chat.zulip.org/#narrow/stream/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/1639957
262+
// We expect the event's 'read' flag to be boring,
263+
// matching the message's local unread state.
266264
final bool isRead = event.flags.contains(MessageFlag.read);
267265
assert(() {
268266
final isUnreadLocally = isUnread(messageId);
@@ -272,6 +270,17 @@ class Unreads extends ChangeNotifier {
272270
// We were going to check something but can't; shrug.
273271
if (isUnreadLocally == null) return true;
274272

273+
final newChannelId = event.moveData?.newStreamId;
274+
if (newChannelId != null && !channelStore.subscriptions.containsKey(newChannelId)) {
275+
// When unread messages are moved to an unsubscribed channel, the server
276+
// marks them as read without sending a mark-as-read event. Clients are
277+
// asked to special-case this by marking them as read, which we do in
278+
// _handleMessageMove. That contract is clear enough and doesn't involve
279+
// this event's 'read' flag, so don't bother logging about the flag;
280+
// its behavior seems like an implementation detail that could change.
281+
return true;
282+
}
283+
275284
if (isUnreadLocally != isUnreadInEvent) {
276285
// If this happens, then either:
277286
// - the server and client have been out of sync about the message's
@@ -296,13 +305,39 @@ class Unreads extends ChangeNotifier {
296305
madeAnyUpdate |= mentions.add(messageId);
297306
}
298307

299-
// TODO(#901) handle moved messages
308+
madeAnyUpdate |= _handleMessageMove(event);
300309

301310
if (madeAnyUpdate) {
302311
notifyListeners();
303312
}
304313
}
305314

315+
bool _handleMessageMove(UpdateMessageEvent event) {
316+
if (event.moveData == null) {
317+
// No moved messages.
318+
return false;
319+
}
320+
final UpdateMessageMoveData(
321+
:origStreamId, :newStreamId, :origTopic, :newTopic) = event.moveData!;
322+
323+
final messageToMoveIds = _removeAllInStreamTopic(
324+
event.messageIds.toSet(), origStreamId, origTopic);
325+
if (messageToMoveIds == null || messageToMoveIds.isEmpty) {
326+
// No known unreads affected by move; nothing to do.
327+
return false;
328+
}
329+
330+
if (!channelStore.subscriptions.containsKey(newStreamId)) {
331+
// Unreads moved to an unsubscribed channel; just drop them.
332+
// See also:
333+
// https://chat.zulip.org/#narrow/channel/378-api-design/topic/mark-as-read.20events.20with.20message.20moves.3F/near/2101926
334+
return true;
335+
}
336+
337+
_addAllInStreamTopic(messageToMoveIds..sort(), newStreamId, newTopic);
338+
return true;
339+
}
340+
306341
void handleDeleteMessageEvent(DeleteMessageEvent event) {
307342
mentions.removeAll(event.messageIds);
308343
final messageIdsSet = Set.of(event.messageIds);

‎test/model/unreads_test.dart

+206
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,212 @@ void main() {
469469
}
470470
}
471471
});
472+
473+
group('moves', () {
474+
final origChannel = eg.stream();
475+
const origTopic = 'origTopic';
476+
const newTopic = 'newTopic';
477+
478+
Future<void> prepareStore() async {
479+
prepare();
480+
await channelStore.addStream(origChannel);
481+
await channelStore.addSubscription(eg.subscription(origChannel));
482+
}
483+
484+
group('move read messages', () {
485+
final readMessages = List<StreamMessage>.generate(10,
486+
(_) => eg.streamMessage(
487+
stream: origChannel, topic: origTopic, flags: [MessageFlag.read]));
488+
489+
test('to new channel', () async {
490+
await prepareStore();
491+
final newChannel = eg.stream();
492+
await channelStore.addStream(newChannel);
493+
await channelStore.addSubscription(eg.subscription(newChannel));
494+
fillWithMessages(readMessages);
495+
496+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
497+
origMessages: readMessages,
498+
newStreamId: newChannel.streamId));
499+
checkNotNotified();
500+
checkMatchesMessages([]);
501+
});
502+
503+
test('to new topic', () async {
504+
await prepareStore();
505+
fillWithMessages(readMessages);
506+
507+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
508+
origMessages: readMessages,
509+
newTopicStr: newTopic));
510+
checkNotNotified();
511+
checkMatchesMessages([]);
512+
});
513+
514+
test('from topic with unreads', () async {
515+
await prepareStore();
516+
final unreadMessage = eg.streamMessage(
517+
stream: origChannel, topic: origTopic);
518+
fillWithMessages([...readMessages, unreadMessage]);
519+
520+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
521+
origMessages: readMessages,
522+
newTopicStr: newTopic));
523+
checkNotNotified();
524+
checkMatchesMessages([unreadMessage]);
525+
});
526+
527+
test('to topic with unreads', () async {
528+
await prepareStore();
529+
final unreadMessage = eg.streamMessage(
530+
stream: origChannel, topic: newTopic);
531+
fillWithMessages([...readMessages, unreadMessage]);
532+
533+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
534+
origMessages: readMessages,
535+
newTopicStr: newTopic,
536+
));
537+
checkNotNotified();
538+
checkMatchesMessages([unreadMessage]);
539+
});
540+
});
541+
542+
group('move unread messages', () {
543+
final unreadMessages = List<StreamMessage>.generate(10,
544+
(_) => eg.streamMessage(stream: origChannel, topic: origTopic));
545+
546+
test('to another subscribed channel; same topic name', () async {
547+
await prepareStore();
548+
final newChannel = eg.stream();
549+
await channelStore.addStream(newChannel);
550+
await channelStore.addSubscription(eg.subscription(newChannel));
551+
fillWithMessages(unreadMessages);
552+
553+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
554+
origMessages: unreadMessages,
555+
newStreamId: newChannel.streamId));
556+
checkNotifiedOnce();
557+
checkMatchesMessages([
558+
for (final message in unreadMessages)
559+
Message.fromJson(
560+
message.toJson()..['stream_id'] = newChannel.streamId),
561+
]);
562+
});
563+
564+
test('to another subscribed channel; different topic name', () async {
565+
await prepareStore();
566+
final newChannel = eg.stream();
567+
await channelStore.addStream(newChannel);
568+
await channelStore.addSubscription(eg.subscription(newChannel));
569+
fillWithMessages(unreadMessages);
570+
571+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
572+
origMessages: unreadMessages,
573+
newStreamId: newChannel.streamId,
574+
newTopicStr: newTopic));
575+
checkNotifiedOnce();
576+
checkMatchesMessages([
577+
for (final message in unreadMessages)
578+
Message.fromJson(
579+
message.toJson()
580+
..['stream_id'] = newChannel.streamId
581+
..['subject'] = newTopic
582+
),
583+
]);
584+
});
585+
586+
test('to unsubscribed channel', () async {
587+
await prepareStore();
588+
final newChannel = eg.stream();
589+
await channelStore.addStream(newChannel);
590+
assert(!channelStore.subscriptions.containsKey(newChannel.streamId));
591+
fillWithMessages(unreadMessages);
592+
593+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
594+
origMessages: unreadMessages,
595+
newStreamId: newChannel.streamId));
596+
checkNotifiedOnce();
597+
checkMatchesMessages([]);
598+
});
599+
600+
test('to new topic', () async {
601+
await prepareStore();
602+
fillWithMessages(unreadMessages);
603+
604+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
605+
origMessages: unreadMessages,
606+
newTopicStr: newTopic));
607+
checkNotifiedOnce();
608+
checkMatchesMessages([
609+
for (final message in unreadMessages)
610+
Message.fromJson(message.toJson()..['subject'] = newTopic),
611+
]);
612+
});
613+
614+
test('from topic containing other unreads', () async {
615+
await prepareStore();
616+
final unreadMessage = eg.streamMessage(
617+
stream: origChannel, topic: origTopic);
618+
fillWithMessages([...unreadMessages, unreadMessage]);
619+
620+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
621+
origMessages: unreadMessages,
622+
newTopicStr: newTopic));
623+
checkNotifiedOnce();
624+
checkMatchesMessages([
625+
for (final message in unreadMessages)
626+
Message.fromJson(message.toJson()..['subject'] = newTopic),
627+
unreadMessage,
628+
]);
629+
});
630+
631+
test('to topic containing other unreads', () async {
632+
await prepareStore();
633+
final unreadMessage = eg.streamMessage(
634+
stream: origChannel, topic: newTopic);
635+
fillWithMessages([...unreadMessages, unreadMessage]);
636+
637+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
638+
origMessages: unreadMessages,
639+
newTopicStr: newTopic));
640+
checkNotifiedOnce();
641+
checkMatchesMessages([
642+
for (final message in unreadMessages)
643+
Message.fromJson(message.toJson()..['subject'] = newTopic),
644+
unreadMessage,
645+
]);
646+
});
647+
648+
test('tolerates unsorted messages', () async {
649+
await prepareStore();
650+
final unreadMessages = List.generate(10,
651+
(i) => eg.streamMessage(id: 1000-i, stream: origChannel, topic: origTopic));
652+
fillWithMessages(unreadMessages);
653+
654+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
655+
origMessages: unreadMessages,
656+
newTopicStr: newTopic));
657+
checkNotifiedOnce();
658+
checkMatchesMessages([
659+
for (final message in unreadMessages)
660+
Message.fromJson(message.toJson()..['subject'] = newTopic)
661+
]);
662+
});
663+
664+
test('tolerates unreads unknown to the model', () async {
665+
await prepareStore();
666+
final unknownUnreadMessage = eg.streamMessage(
667+
stream: eg.stream(), topic: origTopic);
668+
fillWithMessages(unreadMessages);
669+
670+
model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom(
671+
origMessages: [unknownUnreadMessage],
672+
newTopicStr: newTopic));
673+
checkNotNotified();
674+
checkMatchesMessages(unreadMessages);
675+
});
676+
});
677+
});
472678
});
473679

474680

0 commit comments

Comments
 (0)