Skip to content

model: Prevent duplicate flags when processing update_message_flags. #2033

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

Open
sarafarajnasardi wants to merge 1 commit into
base: main
Choose a base branch
from
+38 −4
Open

model: Prevent duplicate flags when processing update_message_flags. #2033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions lib/model/message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,9 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage

if (isAdd && (event as UpdateMessageFlagsAddEvent).all) {
for (final message in messages.values) {
message.flags.add(event.flag);
if (!message.flags.contains(event.flag)) {
message.flags.add(event.flag);
}
Comment on lines 737 to +741
Copy link
Collaborator

@chrisbobbe chrisbobbe Dec 12, 2025

Choose a reason for hiding this comment

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

Please also write a test exercising this case.

}

for (final view in _messageListViews) {
Expand All @@ -750,9 +752,13 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage
if (message == null) continue; // a message we don't know about yet
anyMessageFound = true;

isAdd
? message.flags.add(event.flag)
: message.flags.remove(event.flag);
if (isAdd) {
if (!message.flags.contains(event.flag)) {
message.flags.add(event.flag);
}
} else {
message.flags.remove(event.flag);
}
}
if (anyMessageFound) {
// TODO(#818): Support MentionsNarrow live-updates when handling
Expand Down
28 changes: 28 additions & 0 deletions test/model/message_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1702,6 +1702,34 @@ void main() {
check(store).messages.values
.single.flags.deepEquals([MessageFlag.starred, MessageFlag.read]);
});

test('avoid duplicate flags', () async {
// Regression test for https://github.com/zulip/zulip-flutter/issues/1986
await prepare();
final message = eg.streamMessage(flags: [MessageFlag.read]);
await prepareMessages([message]);

await store.handleEvent(mkAddEvent(MessageFlag.read, [message.id]));
check(store).messages.values.single.flags.deepEquals([MessageFlag.read]);
});

test('handle update_message_flags: add flag "all" is idempotent', () async {
Copy link
Collaborator

@chrisbobbe chrisbobbe Dec 15, 2025

Choose a reason for hiding this comment

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

I'd like to give the two new tests more similar descriptions, so it's more obvious that they're testing very similar logic. How about:

  • 'prevent duplicate flags, when all: false'
  • 'prevent duplicate flags, when all: true'

or similar.

// Regression test for https://github.com/zulip/zulip-flutter/issues/1986
await prepare();
final m1 = eg.streamMessage(flags: [MessageFlag.read]);
final m2 = eg.streamMessage(flags: []);
Comment on lines +1719 to +1720
Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

nit:

Suggested change
final m1 = eg.streamMessage(flags: [MessageFlag.read]);
final m2 = eg.streamMessage(flags: []);
final message1 = eg.streamMessage(flags: [MessageFlag.read]);
final message2 = eg.streamMessage(flags: []);

This point about avoiding abbreviations, in the Flutter upstream style guide, applies equally in Zulip:
https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-abbreviations

(And not only in the mobile repo — in fact I think Tim applies it more strongly than I do, and we've been doing so since long before we used Flutter.)

await prepareMessages([m1, m2]);

await store.handleEvent(UpdateMessageFlagsAddEvent(
id: 1,
Comment on lines +1723 to +1724
Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

nit: like the other cases in this group, use mkAddEvent for brevity and clarity

all: true,
flag: MessageFlag.read,
messages: [],
));

check(store.messages[m1.id]!.flags).deepEquals([MessageFlag.read]);
check(store.messages[m2.id]!.flags).deepEquals([MessageFlag.read]);
Comment on lines +1730 to +1731
Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

nit: Conversely, for making this a bit more compact again, see the test cases with "message1" and "message2" earlier in this group. Those give examples of how the package:checks API enables some handy shorthands.

});
});

group('remove flag', () {
Expand Down