From c4e03ff605e228c155ded9e831c3684516445d7e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 16 Jun 2025 16:09:17 -0700 Subject: [PATCH 1/8] unreads test: Add/use a checkInvariants function like in message-list tests And fix a test that was breaking an invariant in its setup. --- test/model/unreads_test.dart | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index de554b220c..8b03f5f231 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -4,6 +4,7 @@ import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/algorithms.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/unreads.dart'; @@ -12,6 +13,18 @@ import '../example_data.dart' as eg; import 'test_store.dart'; import 'unreads_checks.dart'; +void checkInvariants(Unreads model) { + for (final MapEntry(value: topics) in model.streams.entries) { + for (final MapEntry(value: messageIds) in topics.entries) { + check(isSortedWithoutDuplicates(messageIds)).isTrue(); + } + } + + for (final MapEntry(value: messageIds) in model.dms.entries) { + check(isSortedWithoutDuplicates(messageIds)).isTrue(); + } +} + void main() { // These variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. @@ -23,7 +36,10 @@ void main() { check(notifiedCount).equals(count); notifiedCount = 0; } - void checkNotNotified() => checkNotified(count: 0); + void checkNotNotified() { + checkInvariants(model); + checkNotified(count: 0); + } void checkNotifiedOnce() => checkNotified(count: 1); /// Initialize [model] and the rest of the test state. @@ -38,9 +54,11 @@ void main() { ), }) { store = eg.store(initialSnapshot: eg.initialSnapshot(unreadMsgs: initial)); + checkInvariants(store.unreads); notifiedCount = 0; model = store.unreads ..addListener(() { + checkInvariants(model); notifiedCount++; }); checkNotNotified(); @@ -600,12 +618,11 @@ void main() { test('tolerates unsorted messages', () async { await prepareStore(); final unreadMessages = List.generate(10, (i) => - eg.streamMessage( - id: 1000 - i, stream: origChannel, topic: origTopic)); + eg.streamMessage(stream: origChannel, topic: origTopic)); fillWithMessages(unreadMessages); model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( - origMessages: unreadMessages, + origMessages: unreadMessages.reversed.toList(), newTopicStr: newTopic)); checkNotifiedOnce(); checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic)); From 3bca4767711e0e467c256b2997caa1d21a9da306 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 16 Jun 2025 16:30:48 -0700 Subject: [PATCH 2/8] unreads test [nfc]: Require isSortedWithoutDuplicates in fillWithMessages And change some callers slightly, to adapt. Since fillWithMessages is implemented as handling a sequence of new-message events, require the messages to be sorted without duplicates, for realism. This doesn't change what the model's state looks like for any of the tests, because messages in the input with the "read" flag are ignored by model.handleMessageEvent, and we're not changing the order of the unread messages. Also, the previous commit started checking the invariant that the model's message-ID lists are sorted. --- test/model/unreads_test.dart | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 8b03f5f231..ae7ffe1063 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -64,7 +64,8 @@ void main() { checkNotNotified(); } - void fillWithMessages(Iterable messages) { + void fillWithMessages(List messages) { + check(isSortedWithoutDuplicates(messages.map((m) => m.id).toList())).isTrue(); for (final message in messages) { model.handleMessageEvent(eg.messageEvent(message)); } @@ -260,9 +261,9 @@ void main() { group('isUnread', () { final unreadDmMessage = eg.dmMessage( from: eg.otherUser, to: [eg.selfUser], flags: []); + final unreadChannelMessage = eg.streamMessage(flags: []); final readDmMessage = eg.dmMessage( from: eg.otherUser, to: [eg.selfUser], flags: [MessageFlag.read]); - final unreadChannelMessage = eg.streamMessage(flags: []); final readChannelMessage = eg.streamMessage(flags: [MessageFlag.read]); final allMessages = [ @@ -499,11 +500,11 @@ void main() { prepare(); await store.addStream(origChannel); await store.addSubscription(eg.subscription(origChannel)); + unreadMessages = List.generate(10, + (_) => eg.streamMessage(stream: origChannel, topic: origTopic)); readMessages = List.generate(10, (_) => eg.streamMessage(stream: origChannel, topic: origTopic, flags: [MessageFlag.read])); - unreadMessages = List.generate(10, - (_) => eg.streamMessage(stream: origChannel, topic: origTopic)); } List copyMessagesWith(Iterable messages, { From a518e4fe9600a60608ac008ec852b47287f674b8 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 16 Jun 2025 16:46:57 -0700 Subject: [PATCH 3/8] unreads test: Make room for two more message IDs (9, 10) in a test's setup --- test/model/unreads_test.dart | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index ae7ffe1063..1c8a907c1f 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -138,14 +138,14 @@ void main() { eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'c', unreadMessageIds: [7, 8]), ], dms: [ - UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [9, 10]), - UnreadDmSnapshot(otherUserId: 2, unreadMessageIds: [11, 12]), + UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [11, 12]), + UnreadDmSnapshot(otherUserId: 2, unreadMessageIds: [13, 14]), ], huddles: [ - UnreadHuddleSnapshot(userIdsString: '1,2,${eg.selfUser.userId}', unreadMessageIds: [13, 14]), - UnreadHuddleSnapshot(userIdsString: '2,3,${eg.selfUser.userId}', unreadMessageIds: [15, 16]), + UnreadHuddleSnapshot(userIdsString: '1,2,${eg.selfUser.userId}', unreadMessageIds: [15, 16]), + UnreadHuddleSnapshot(userIdsString: '2,3,${eg.selfUser.userId}', unreadMessageIds: [17, 18]), ], - mentions: [6, 12, 16], + mentions: [6, 14, 18], oldUnreadsMissing: false, )); checkMatchesMessages([ @@ -157,14 +157,14 @@ void main() { eg.streamMessage(id: 6, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]), eg.streamMessage(id: 7, stream: stream2, topic: 'c', flags: []), eg.streamMessage(id: 8, stream: stream2, topic: 'c', flags: []), - eg.dmMessage(id: 9, from: user1, to: [eg.selfUser], flags: []), - eg.dmMessage(id: 10, from: user1, to: [eg.selfUser], flags: []), - eg.dmMessage(id: 11, from: user2, to: [eg.selfUser], flags: []), - eg.dmMessage(id: 12, from: user2, to: [eg.selfUser], flags: [MessageFlag.mentioned]), - eg.dmMessage(id: 13, from: user1, to: [user2, eg.selfUser], flags: []), - eg.dmMessage(id: 14, from: user1, to: [user2, eg.selfUser], flags: []), - eg.dmMessage(id: 15, from: user2, to: [user3, eg.selfUser], flags: []), - eg.dmMessage(id: 16, from: user2, to: [user3, eg.selfUser], flags: [MessageFlag.wildcardMentioned]), + eg.dmMessage(id: 11, from: user1, to: [eg.selfUser], flags: []), + eg.dmMessage(id: 12, from: user1, to: [eg.selfUser], flags: []), + eg.dmMessage(id: 13, from: user2, to: [eg.selfUser], flags: []), + eg.dmMessage(id: 14, from: user2, to: [eg.selfUser], flags: [MessageFlag.mentioned]), + eg.dmMessage(id: 15, from: user1, to: [user2, eg.selfUser], flags: []), + eg.dmMessage(id: 16, from: user1, to: [user2, eg.selfUser], flags: []), + eg.dmMessage(id: 17, from: user2, to: [user3, eg.selfUser], flags: []), + eg.dmMessage(id: 18, from: user2, to: [user3, eg.selfUser], flags: [MessageFlag.wildcardMentioned]), ]); }); }); From c1b3734a99487971c15fb5859ddb7ab6c064612b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 17 Jun 2025 16:26:00 -0700 Subject: [PATCH 4/8] model: Add makeTopicKeyedMap for case-insensitive-but-preserving topic maps --- lib/model/channel.dart | 20 ++++++++++++++++++++ test/model/channel_test.dart | 28 ++++++++++++++++++++++++++++ test/stdlib_checks.dart | 5 +++++ 3 files changed, 53 insertions(+) diff --git a/lib/model/channel.dart b/lib/model/channel.dart index a13e19cc18..368f691eec 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -1,3 +1,5 @@ +import 'dart:collection'; + import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; @@ -369,3 +371,21 @@ class ChannelStoreImpl with ChannelStore { } } } + +/// A [Map] with [TopicName] keys and [V] values. +/// +/// When one of these is created by [makeTopicKeyedMap], +/// key equality is done case-insensitively; see there. +/// +/// This type should only be used for maps created by [makeTopicKeyedMap]. +/// It would be nice to enforce that. +typedef TopicKeyedMap = Map; + +/// Make a case-insensitive, case-preserving [TopicName]-keyed [LinkedHashMap]. +/// +/// The equality function is [TopicName.isSameAs], +/// and the hash code is [String.hashCode] of [TopicName.canonicalize]. +TopicKeyedMap makeTopicKeyedMap() => LinkedHashMap( + equals: (a, b) => a.isSameAs(b), + hashCode: (k) => k.canonicalize().hashCode, +); diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 5c00aca814..3e462f8e79 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -7,7 +7,9 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/channel.dart'; +import '../api/model/model_checks.dart'; import '../example_data.dart' as eg; +import '../stdlib_checks.dart'; import 'test_store.dart'; void main() { @@ -412,4 +414,30 @@ void main() { .equals(UserTopicVisibilityPolicy.none); }); }); + + group('makeTopicKeyedMap', () { + test('"a" equals "A"', () { + final map = makeTopicKeyedMap() + ..[eg.t('a')] = 1 + ..[eg.t('A')] = 2; + check(map) + ..[eg.t('a')].equals(2) + ..[eg.t('A')].equals(2) + ..entries.which((it) => it.single + ..key.apiName.equals('a') + ..value.equals(2)); + }); + + test('"A" equals "a"', () { + final map = makeTopicKeyedMap() + ..[eg.t('A')] = 1 + ..[eg.t('a')] = 2; + check(map) + ..[eg.t('A')].equals(2) + ..[eg.t('a')].equals(2) + ..entries.which((it) => it.single + ..key.apiName.equals('A') + ..value.equals(2)); + }); + }); } diff --git a/test/stdlib_checks.dart b/test/stdlib_checks.dart index 8bfaea54fd..28b5f5e5f2 100644 --- a/test/stdlib_checks.dart +++ b/test/stdlib_checks.dart @@ -16,6 +16,11 @@ extension ListChecks on Subject> { Subject operator [](int index) => has((l) => l[index], '[$index]'); } +extension MapEntryChecks on Subject> { + Subject get key => has((e) => e.key, 'key'); + Subject get value => has((e) => e.value, 'value'); +} + extension NullableMapChecks on Subject?> { void deepEquals(Map? expected) { if (expected == null) { From ca17ee3efa275f8b077cc49c461c3e64f7b75830 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 18 Jun 2025 14:50:22 -0700 Subject: [PATCH 5/8] recent_senders: Treat topics case-insensitively --- lib/model/recent_senders.dart | 9 +++++--- test/model/recent_senders_test.dart | 34 +++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lib/model/recent_senders.dart b/lib/model/recent_senders.dart index a5c4bca778..f8b046641d 100644 --- a/lib/model/recent_senders.dart +++ b/lib/model/recent_senders.dart @@ -4,6 +4,7 @@ import 'package:flutter/foundation.dart'; import '../api/model/events.dart'; import '../api/model/model.dart'; import 'algorithms.dart'; +import 'channel.dart'; /// Tracks the latest messages sent by each user, in each stream and topic. /// @@ -16,7 +17,7 @@ class RecentSenders { // topicSenders[streamId][topic][senderId] = MessageIdTracker @visibleForTesting - final Map>> topicSenders = {}; + final Map>> topicSenders = {}; /// The latest message the given user sent to the given stream, /// or null if no such message is known. @@ -27,6 +28,8 @@ class RecentSenders { /// The latest message the given user sent to the given topic, /// or null if no such message is known. + /// + /// Topics are treated case-insensitively; see [TopicName.isSameAs]. int? latestMessageIdOfSenderInTopic({ required int streamId, required TopicName topic, @@ -53,7 +56,7 @@ class RecentSenders { } for (final entry in messagesByUserInTopic.entries) { final (streamId, topic, senderId) = entry.key; - (((topicSenders[streamId] ??= {})[topic] ??= {}) + (((topicSenders[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) [senderId] ??= MessageIdTracker()).addAll(entry.value); } } @@ -64,7 +67,7 @@ class RecentSenders { final StreamMessage(:streamId, :topic, :senderId, id: int messageId) = message; ((streamSenders[streamId] ??= {}) [senderId] ??= MessageIdTracker()).add(messageId); - (((topicSenders[streamId] ??= {})[topic] ??= {}) + (((topicSenders[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) [senderId] ??= MessageIdTracker()).add(messageId); } diff --git a/test/model/recent_senders_test.dart b/test/model/recent_senders_test.dart index 602362cbcc..990811d216 100644 --- a/test/model/recent_senders_test.dart +++ b/test/model/recent_senders_test.dart @@ -1,13 +1,16 @@ import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/model/channel.dart'; import 'package:zulip/model/recent_senders.dart'; import '../example_data.dart' as eg; /// [messages] should be sorted by [id] ascending. void checkMatchesMessages(RecentSenders model, List messages) { final Map>> messagesByUserInStream = {}; - final Map>>> messagesByUserInTopic = {}; + final Map>>> messagesByUserInTopic = {}; for (final message in messages) { if (message is! StreamMessage) { throw UnsupportedError('Message of type ${message.runtimeType} is not expected.'); @@ -17,7 +20,7 @@ void checkMatchesMessages(RecentSenders model, List messages) { ((messagesByUserInStream[streamId] ??= {}) [senderId] ??= {}).add(messageId); - (((messagesByUserInTopic[streamId] ??= {})[topic] ??= {}) + (((messagesByUserInTopic[streamId] ??= makeTopicKeyedMap())[topic] ??= {}) [senderId] ??= {}).add(messageId); } @@ -125,6 +128,16 @@ void main() { [eg.streamMessage(stream: streamA, topic: 'other', sender: userX)]); }); + test('case-insensitive topics', () { + checkHandleMessages( + [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], + [eg.streamMessage(stream: streamA, topic: 'ThInG', sender: userX)]); + check(model.topicSenders).values.single.deepEquals( + {eg.t('thing'): + {userX.userId: (Subject it) => + it.isA().ids.length.equals(2)}}); + }); + test('add new stream', () { checkHandleMessages( [eg.streamMessage(stream: streamA, topic: 'thing', sender: userX)], @@ -161,6 +174,16 @@ void main() { Map.fromEntries(messages.map((msg) => MapEntry(msg.id, msg)))); checkMatchesMessages(model, [messages[1]]); + + // check case-insensitivity + model.handleDeleteMessageEvent(DeleteMessageEvent( + id: 0, + messageIds: [messages[1].id], + messageType: MessageType.stream, + streamId: stream.streamId, + topic: eg.t('oThEr'), + ), {messages[1].id: messages[1]}); + checkMatchesMessages(model, []); }); test('RecentSenders.latestMessageIdOfSenderInStream', () { @@ -200,6 +223,9 @@ void main() { check(model.latestMessageIdOfSenderInTopic(streamId: 1, topic: eg.t('a'), senderId: 10)).equals(300); + // case-insensitivity + check(model.latestMessageIdOfSenderInTopic(streamId: 1, + topic: eg.t('A'), senderId: 10)).equals(300); // No message of user 20 in topic "a". check(model.latestMessageIdOfSenderInTopic(streamId: 1, topic: eg.t('a'), senderId: 20)).equals(null); @@ -211,3 +237,7 @@ void main() { topic: eg.t('a'), senderId: 10)).equals(null); }); } + +extension MessageIdTrackerChecks on Subject { + Subject> get ids => has((x) => x.ids, 'ids'); +} From 7382bdf168017f21a0bf5ab252022c65bbdec85e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 16 Jun 2025 16:09:55 -0700 Subject: [PATCH 6/8] unreads: Treat topics case-insensitively Fixes: #980 --- lib/model/unreads.dart | 26 ++++++++--- test/model/unreads_test.dart | 84 ++++++++++++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 15 deletions(-) diff --git a/lib/model/unreads.dart b/lib/model/unreads.dart index ad6b7b4b8d..e65197a980 100644 --- a/lib/model/unreads.dart +++ b/lib/model/unreads.dart @@ -41,14 +41,22 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { required CorePerAccountStore core, required ChannelStore channelStore, }) { - final streams = >>{}; + final streams = >>{}; final dms = >{}; final mentions = Set.of(initial.mentions); for (final unreadChannelSnapshot in initial.channels) { final streamId = unreadChannelSnapshot.streamId; final topic = unreadChannelSnapshot.topic; - (streams[streamId] ??= {})[topic] = QueueList.from(unreadChannelSnapshot.unreadMessageIds); + final topics = (streams[streamId] ??= makeTopicKeyedMap()); + topics.update(topic, + // Older servers differentiate topics case-sensitively, but shouldn't: + // https://github.com/zulip/zulip/pull/31869 + // Our topic-keyed map is case-insensitive. When we've seen this + // topic before, modulo case, aggregate instead of clobbering. + // TODO(server-10) simplify away + (value) => setUnion(value, unreadChannelSnapshot.unreadMessageIds), + ifAbsent: () => QueueList.from(unreadChannelSnapshot.unreadMessageIds)); } for (final unreadDmSnapshot in initial.dms) { @@ -88,7 +96,10 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { // int count; /// Unread stream messages, as: stream ID → topic → message IDs (sorted). - final Map>> streams; + /// + /// The topic-keyed map is case-insensitive and case-preserving; + /// it comes from [makeTopicKeyedMap]. + final Map>> streams; /// Unread DM messages, as: DM narrow → message IDs (sorted). final Map> dms; @@ -405,7 +416,7 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { _slowRemoveAllInDms(messageIdsSet); } case UpdateMessageFlagsRemoveEvent(): - final newlyUnreadInStreams = >>{}; + final newlyUnreadInStreams = >>{}; final newlyUnreadInDms = >{}; for (final messageId in event.messages) { final detail = event.messageDetails![messageId]; @@ -420,7 +431,7 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { } switch (detail.type) { case MessageType.stream: - final topics = (newlyUnreadInStreams[detail.streamId!] ??= {}); + final topics = (newlyUnreadInStreams[detail.streamId!] ??= makeTopicKeyedMap()); final messageIds = (topics[detail.topic!] ??= QueueList()); messageIds.add(messageId); case MessageType.direct: @@ -488,14 +499,15 @@ class Unreads extends PerAccountStoreBase with ChangeNotifier { } void _addLastInStreamTopic(int messageId, int streamId, TopicName topic) { - ((streams[streamId] ??= {})[topic] ??= QueueList()).addLast(messageId); + ((streams[streamId] ??= makeTopicKeyedMap())[topic] ??= QueueList()) + .addLast(messageId); } // [messageIds] must be sorted ascending and without duplicates. void _addAllInStreamTopic(QueueList messageIds, int streamId, TopicName topic) { assert(messageIds.isNotEmpty); assert(isSortedWithoutDuplicates(messageIds)); - final topics = streams[streamId] ??= {}; + final topics = streams[streamId] ??= makeTopicKeyedMap(); topics.update(topic, ifAbsent: () => messageIds, // setUnion dedupes existing and incoming unread IDs, diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 1c8a907c1f..4810e3e269 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -5,11 +5,13 @@ import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/algorithms.dart'; +import 'package:zulip/model/channel.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/model/unreads.dart'; import '../example_data.dart' as eg; +import '../stdlib_checks.dart'; import 'test_store.dart'; import 'unreads_checks.dart'; @@ -76,7 +78,7 @@ void main() { assert(Set.of(messages.map((m) => m.id)).length == messages.length, 'checkMatchesMessages: duplicate messages in test input'); - final Map>> expectedStreams = {}; + final Map>> expectedStreams = {}; final Map> expectedDms = {}; final Set expectedMentions = {}; for (final message in messages) { @@ -85,7 +87,7 @@ void main() { } switch (message) { case StreamMessage(): - final perTopic = expectedStreams[message.streamId] ??= {}; + final perTopic = expectedStreams[message.streamId] ??= makeTopicKeyedMap(); final messageIds = perTopic[message.topic] ??= QueueList(); messageIds.add(message.id); case DmMessage(): @@ -136,6 +138,9 @@ void main() { eg.unreadChannelMsgs(streamId: stream1.streamId, topic: 'b', unreadMessageIds: [3, 4]), eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'b', unreadMessageIds: [5, 6]), eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'c', unreadMessageIds: [7, 8]), + + // TODO(server-10) drop this (see implementation) + eg.unreadChannelMsgs(streamId: stream2.streamId, topic: 'C', unreadMessageIds: [9, 10]), ], dms: [ UnreadDmSnapshot(otherUserId: 1, unreadMessageIds: [11, 12]), @@ -157,6 +162,8 @@ void main() { eg.streamMessage(id: 6, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]), eg.streamMessage(id: 7, stream: stream2, topic: 'c', flags: []), eg.streamMessage(id: 8, stream: stream2, topic: 'c', flags: []), + eg.streamMessage(id: 9, stream: stream2, topic: 'C', flags: []), + eg.streamMessage(id: 10, stream: stream2, topic: 'C', flags: []), eg.dmMessage(id: 11, from: user1, to: [eg.selfUser], flags: []), eg.dmMessage(id: 12, from: user1, to: [eg.selfUser], flags: []), eg.dmMessage(id: 13, from: user2, to: [eg.selfUser], flags: []), @@ -201,10 +208,10 @@ void main() { await store.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream, topic: 'a', flags: []), - eg.streamMessage(stream: stream, topic: 'a', flags: []), - eg.streamMessage(stream: stream, topic: 'b', flags: []), + eg.streamMessage(stream: stream, topic: 'A', flags: []), eg.streamMessage(stream: stream, topic: 'b', flags: []), eg.streamMessage(stream: stream, topic: 'b', flags: []), + eg.streamMessage(stream: stream, topic: 'B', flags: []), eg.streamMessage(stream: stream, topic: 'c', flags: []), ]); check(model.countInChannel (stream.streamId)).equals(5); @@ -220,9 +227,13 @@ void main() { test('countInTopicNarrow', () { final stream = eg.stream(); prepare(); - fillWithMessages(List.generate(7, (i) => eg.streamMessage( - stream: stream, topic: 'a', flags: []))); - check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(7); + final messages = [ + ...List.generate(7, (i) => eg.streamMessage(stream: stream, topic: 'a', flags: [])), + ...List.generate(2, (i) => eg.streamMessage(stream: stream, topic: 'A', flags: [])), + ]; + fillWithMessages(messages); + check(model.countInTopicNarrow(stream.streamId, eg.t('a'))).equals(9); + check(model.countInTopicNarrow(stream.streamId, eg.t('A'))).equals(9); }); test('countInDmNarrow', () { @@ -370,6 +381,24 @@ void main() { }); } }); + + test('topics case-insensitive but case-preserving', () { + final stream = eg.stream(); + final message1 = eg.streamMessage(stream: stream, topic: 'aaa'); + final message2 = eg.streamMessage(stream: stream, topic: 'AaA'); + final message3 = eg.streamMessage(stream: stream, topic: 'aAa'); + prepare(); + fillWithMessages([message1]); + model.handleMessageEvent(eg.messageEvent(message2)); + model.handleMessageEvent(eg.messageEvent(message3)); + checkNotified(count: 2); + checkMatchesMessages([message1, message2, message3]); + // Redundant with checkMatchesMessages, but for explicitness here: + check(model).streams.values.single + .entries.single + ..key.equals(eg.t('aaa')) + ..value.length.equals(3); + }); }); group('DM messages', () { @@ -629,6 +658,39 @@ void main() { checkMatchesMessages(copyMessagesWith(unreadMessages, newTopic: newTopic)); }); + test('topics case-insensitive but case-preserving', () async { + final message1 = eg.streamMessage(stream: origChannel, topic: 'aaa', flags: []); + final message2 = eg.streamMessage(stream: origChannel, topic: 'aaa', flags: []); + final messages = [message1, message2]; + await prepareStore(); + fillWithMessages(messages); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + // 'AAA' finds the key 'aaa' + origMessages: copyMessagesWith([message1], newTopic: 'AAA'), + newTopicStr: 'bbb')); + checkNotifiedOnce(); + checkMatchesMessages([ + ...copyMessagesWith([message1], newTopic: 'bbb'), + message2, + ]); + + model.handleUpdateMessageEvent(eg.updateMessageEventMoveFrom( + origMessages: [message2], + // 'BBB' finds the key 'bbb' + newTopicStr: 'BBB')); + checkNotifiedOnce(); + checkMatchesMessages([ + ...copyMessagesWith([message1], newTopic: 'bbb'), + ...copyMessagesWith([message2], newTopic: 'BBB'), + ]); + // Redundant with checkMatchesMessages, but for explicitness here: + check(model).streams.values.single + .entries.single + ..key.equals(eg.t('bbb')) + ..value.length.equals(2); + }); + test('tolerates unreads unknown to the model', () async { await prepareStore(); fillWithMessages(unreadMessages); @@ -690,6 +752,7 @@ void main() { fillWithMessages(messages); final expectedRemainingMessages = Set.of(messages); + assert(messages.any((m) => m.id == 14)); for (final message in messages) { final event = switch (message) { StreamMessage() => DeleteMessageEvent( @@ -697,7 +760,12 @@ void main() { messageType: MessageType.stream, messageIds: [message.id], streamId: message.streamId, - topic: message.topic, + topic: () { + if (message.id != 14) return message.topic; + final uppercase = message.topic.apiName.toUpperCase(); + assert(message.topic.apiName != uppercase); + return eg.t(uppercase); // exercise case-insensitivity of topics + }(), ), DmMessage() => DeleteMessageEvent( id: 0, From cf572810f78ed7ec4408017a33f089dc21d694aa Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 18 Jun 2025 13:17:50 -0700 Subject: [PATCH 7/8] test [nfc]: s/addUserTopic/setUserTopic/ in PerAccountStoreTestExtension The event's doc says: https://zulip.com/api/get-events#user_topic > Event sent to a user's clients when the user mutes/unmutes a > topic, or otherwise modifies their personal per-topic > configuration. The new name is still accurate for "adding" a configuration (i.e. setting it to something not-"none"), but now also covers updating a configuration (moving between not-"none" configurations) and removing one (resetting to "none). --- test/model/channel_test.dart | 34 +++++++++++++++---------------- test/model/message_list_test.dart | 20 +++++++++--------- test/model/test_store.dart | 2 +- test/model/unreads_test.dart | 6 +++--- test/widgets/inbox_test.dart | 10 ++++----- test/widgets/topic_list_test.dart | 2 +- 6 files changed, 37 insertions(+), 37 deletions(-) diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 3e462f8e79..7aa020acf4 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -148,7 +148,7 @@ void main() { test('with nothing for topic', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.muted); check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) .equals(UserTopicVisibilityPolicy.none); }); @@ -160,7 +160,7 @@ void main() { UserTopicVisibilityPolicy.unmuted, UserTopicVisibilityPolicy.followed, ]) { - await store.addUserTopic(stream1, 'topic', policy); + await store.setUserTopic(stream1, 'topic', policy); check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) .equals(policy); } @@ -195,7 +195,7 @@ void main() { final store = eg.store(); await store.addStream(stream1); await store.addSubscription(eg.subscription(stream1)); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isFalse(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isFalse(); }); @@ -204,7 +204,7 @@ void main() { final store = eg.store(); await store.addStream(stream1); await store.addSubscription(eg.subscription(stream1, isMuted: true)); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); }); @@ -213,7 +213,7 @@ void main() { final store = eg.store(); await store.addStream(stream1); await store.addSubscription(eg.subscription(stream1, isMuted: true)); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); }); @@ -343,7 +343,7 @@ void main() { group('events', () { test('add with new stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.muted), ]); @@ -351,8 +351,8 @@ void main() { test('add in existing stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.muted), eg.userTopicItem(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted), @@ -361,8 +361,8 @@ void main() { test('update existing policy', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.unmuted), ]); @@ -370,9 +370,9 @@ void main() { test('remove, with others in stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'other topic', UserTopicVisibilityPolicy.unmuted), ]); @@ -380,16 +380,16 @@ void main() { test('remove, as last in stream', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); compareTopicVisibility(store, [ ]); }); test('treat unknown enum value as removing', () async { final store = eg.store(); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.addUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unknown); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unknown); compareTopicVisibility(store, [ ]); }); diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 0779af52a1..dbe2e6d8eb 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -359,7 +359,7 @@ void main() { final stream = eg.stream(); final otherStream = eg.stream(); await prepare(narrow: ChannelNarrow(stream.streamId)); - await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); await prepareOutboxMessagesTo([ StreamDestination(stream.streamId, eg.t('topic')), StreamDestination(stream.streamId, eg.t('muted')), @@ -1124,7 +1124,7 @@ void main() { eg.streamMessage(id: 2, stream: stream, topic: topic), ]; await prepareMessages(foundOldest: true, messages: messages); - await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); await prepareOutboxMessagesTo([ StreamDestination(stream.streamId, eg.t(topic)), StreamDestination(stream.streamId, eg.t('muted')), @@ -2218,9 +2218,9 @@ void main() { await prepare(narrow: const CombinedFeedNarrow()); await store.addStreams([stream1, stream2]); await store.addSubscription(eg.subscription(stream1)); - await store.addUserTopic(stream1, 'B', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'B', UserTopicVisibilityPolicy.muted); await store.addSubscription(eg.subscription(stream2, isMuted: true)); - await store.addUserTopic(stream2, 'C', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream2, 'C', UserTopicVisibilityPolicy.unmuted); // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: [ @@ -2278,8 +2278,8 @@ void main() { await prepare(narrow: ChannelNarrow(stream.streamId)); await store.addStream(stream); await store.addSubscription(eg.subscription(stream, isMuted: true)); - await store.addUserTopic(stream, 'A', UserTopicVisibilityPolicy.unmuted); - await store.addUserTopic(stream, 'C', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'A', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream, 'C', UserTopicVisibilityPolicy.muted); // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: [ @@ -2323,7 +2323,7 @@ void main() { await prepare(narrow: ChannelNarrow(stream.streamId)); await store.addStream(stream); await store.addSubscription(eg.subscription(stream)); - await store.addUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'muted', UserTopicVisibilityPolicy.muted); await prepareMessages(foundOldest: true, messages: []); // Check filtering on sent messages… @@ -2356,7 +2356,7 @@ void main() { await prepare(narrow: eg.topicNarrow(stream.streamId, 'A')); await store.addStream(stream); await store.addSubscription(eg.subscription(stream, isMuted: true)); - await store.addUserTopic(stream, 'A', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'A', UserTopicVisibilityPolicy.muted); // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: [ @@ -2386,7 +2386,7 @@ void main() { const mutedTopic = 'muted'; await prepare(narrow: const MentionsNarrow()); await store.addStream(stream); - await store.addUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); await store.addSubscription(eg.subscription(stream, isMuted: true)); List getMessages(int startingId) => [ @@ -2424,7 +2424,7 @@ void main() { const mutedTopic = 'muted'; await prepare(narrow: const StarredMessagesNarrow()); await store.addStream(stream); - await store.addUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, mutedTopic, UserTopicVisibilityPolicy.muted); await store.addSubscription(eg.subscription(stream, isMuted: true)); List getMessages(int startingId) => [ diff --git a/test/model/test_store.dart b/test/model/test_store.dart index d979e737f9..1d086d4290 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -287,7 +287,7 @@ extension PerAccountStoreTestExtension on PerAccountStore { await handleEvent(SubscriptionAddEvent(id: 1, subscriptions: subscriptions)); } - Future addUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async { + Future setUserTopic(ZulipStream stream, String topic, UserTopicVisibilityPolicy visibilityPolicy) async { await handleEvent(eg.userTopicEvent(stream.streamId, topic, visibilityPolicy)); } diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 4810e3e269..e1c5ca6986 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -186,7 +186,7 @@ void main() { await store.addSubscription(eg.subscription(stream1)); await store.addSubscription(eg.subscription(stream2)); await store.addSubscription(eg.subscription(stream3, isMuted: true)); - await store.addUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream1, 'a', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream1, topic: 'a', flags: []), eg.streamMessage(stream: stream1, topic: 'b', flags: []), @@ -204,8 +204,8 @@ void main() { prepare(); await store.addStream(stream); await store.addSubscription(eg.subscription(stream)); - await store.addUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); - await store.addUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'a', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream, 'c', UserTopicVisibilityPolicy.muted); fillWithMessages([ eg.streamMessage(stream: stream, topic: 'a', flags: []), eg.streamMessage(stream: stream, topic: 'A', flags: []), diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index 4d24e5d831..87bb2f2f0a 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -227,7 +227,7 @@ void main() { streams: [stream], subscriptions: [subscription], unreadMessages: [eg.streamMessage(stream: stream, topic: 'lunch')]); - await store.addUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.muted); + await store.setUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.muted); await tester.pump(); check(tester.widgetList(find.text('lunch'))).length.equals(0); }); @@ -249,7 +249,7 @@ void main() { streams: [stream], subscriptions: [subscription], unreadMessages: [eg.streamMessage(stream: stream, topic: 'lunch')]); - await store.addUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(stream, 'lunch', UserTopicVisibilityPolicy.unmuted); await tester.pump(); check(tester.widgetList(find.text('lunch'))).length.equals(1); }); @@ -327,7 +327,7 @@ void main() { streams: [channel], subscriptions: [eg.subscription(channel)], unreadMessages: [message]); - await store.addUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); + await store.setUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); await tester.pump(); check(hasIcon(tester, parent: findRowByLabel(tester, topic), @@ -340,7 +340,7 @@ void main() { subscriptions: [eg.subscription(channel)], unreadMessages: [eg.streamMessage(stream: channel, topic: topic, flags: [MessageFlag.mentioned])]); - await store.addUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); + await store.setUserTopic(channel, topic, UserTopicVisibilityPolicy.followed); await tester.pump(); check(hasIcon(tester, parent: findRowByLabel(tester, topic), @@ -356,7 +356,7 @@ void main() { streams: [channel], subscriptions: [eg.subscription(channel, isMuted: true)], unreadMessages: [message]); - await store.addUserTopic(channel, topic, UserTopicVisibilityPolicy.unmuted); + await store.setUserTopic(channel, topic, UserTopicVisibilityPolicy.unmuted); await tester.pump(); check(hasIcon(tester, parent: findRowByLabel(tester, topic), diff --git a/test/widgets/topic_list_test.dart b/test/widgets/topic_list_test.dart index cf76ff3917..f87ddaf1ee 100644 --- a/test/widgets/topic_list_test.dart +++ b/test/widgets/topic_list_test.dart @@ -42,7 +42,7 @@ void main() { await store.addStream(channel); await store.addSubscription(eg.subscription(channel)); for (final userTopic in userTopics) { - await store.addUserTopic( + await store.setUserTopic( channel, userTopic.topicName.apiName, userTopic.visibilityPolicy); } topics ??= [eg.getStreamTopicsEntry()]; From a81e43adff207e0060315af9e2886cec0c9df063 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 18 Jun 2025 13:14:32 -0700 Subject: [PATCH 8/8] model: Treat topics case-insensitively in topic-visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worth noting some helpful feedback from Greg in review, about the additions to test/model/channel_test.dart: https://github.com/zulip/zulip-flutter/pull/1608#discussion_r2155967343 > I think my ideal version of these tests would have most of the new > checks in this file be separate test cases, rather than added to > existing tests. This also doesn't need a full matrix with the > other variables: for example, this check can be done with any one > of the non-`none` policies, rather than all three. > > These are fine, though. No need to rewrite them — we have > higher-priority things to do. --- lib/model/channel.dart | 12 +++++---- test/model/channel_test.dart | 48 +++++++++++++++++++++++++++++++++--- test/widgets/inbox_test.dart | 35 ++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 9 deletions(-) diff --git a/lib/model/channel.dart b/lib/model/channel.dart index 368f691eec..4dbc61756e 100644 --- a/lib/model/channel.dart +++ b/lib/model/channel.dart @@ -43,6 +43,8 @@ mixin ChannelStore { /// /// For policies directly applicable in the UI, see /// [isTopicVisibleInStream] and [isTopicVisible]. + /// + /// Topics are treated case-insensitively; see [TopicName.isSameAs]. UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic); /// The raw data structure underlying [topicVisibilityPolicy]. @@ -173,13 +175,13 @@ class ChannelStoreImpl with ChannelStore { streams.putIfAbsent(stream.streamId, () => stream); } - final topicVisibility = >{}; + final topicVisibility = >{}; for (final item in initialSnapshot.userTopics ?? const []) { if (_warnInvalidVisibilityPolicy(item.visibilityPolicy)) { // Not a value we expect. Keep it out of our data structures. // TODO(log) continue; } - final forStream = topicVisibility.putIfAbsent(item.streamId, () => {}); + final forStream = topicVisibility.putIfAbsent(item.streamId, () => makeTopicKeyedMap()); forStream[item.topicName] = item.visibilityPolicy; } @@ -206,9 +208,9 @@ class ChannelStoreImpl with ChannelStore { final Map subscriptions; @override - Map> get debugTopicVisibility => topicVisibility; + Map> get debugTopicVisibility => topicVisibility; - final Map> topicVisibility; + final Map> topicVisibility; @override UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) { @@ -366,7 +368,7 @@ class ChannelStoreImpl with ChannelStore { topicVisibility.remove(event.streamId); } } else { - final forStream = topicVisibility.putIfAbsent(event.streamId, () => {}); + final forStream = topicVisibility.putIfAbsent(event.streamId, () => makeTopicKeyedMap()); forStream[event.topicName] = visibilityPolicy; } } diff --git a/test/model/channel_test.dart b/test/model/channel_test.dart index 7aa020acf4..0fd83f64f9 100644 --- a/test/model/channel_test.dart +++ b/test/model/channel_test.dart @@ -1,4 +1,3 @@ - import 'package:checks/checks.dart'; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; @@ -163,6 +162,10 @@ void main() { await store.setUserTopic(stream1, 'topic', policy); check(store.topicVisibilityPolicy(stream1.streamId, eg.t('topic'))) .equals(policy); + + // Case-insensitive + check(store.topicVisibilityPolicy(stream1.streamId, eg.t('ToPiC'))) + .equals(policy); } }); }); @@ -198,6 +201,10 @@ void main() { await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isFalse(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isFalse(); + + // Case-insensitive + check(store.isTopicVisibleInStream(stream1.streamId, eg.t('ToPiC'))).isFalse(); + check(store.isTopicVisible (stream1.streamId, eg.t('ToPiC'))).isFalse(); }); test('with policy unmuted', () async { @@ -207,6 +214,10 @@ void main() { await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unmuted); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); + + // Case-insensitive + check(store.isTopicVisibleInStream(stream1.streamId, eg.t('tOpIc'))).isTrue(); + check(store.isTopicVisible (stream1.streamId, eg.t('tOpIc'))).isTrue(); }); test('with policy followed', () async { @@ -216,6 +227,10 @@ void main() { await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.followed); check(store.isTopicVisibleInStream(stream1.streamId, eg.t('topic'))).isTrue(); check(store.isTopicVisible (stream1.streamId, eg.t('topic'))).isTrue(); + + // Case-insensitive + check(store.isTopicVisibleInStream(stream1.streamId, eg.t('TOPIC'))).isTrue(); + check(store.isTopicVisible (stream1.streamId, eg.t('TOPIC'))).isTrue(); }); }); @@ -223,6 +238,18 @@ void main() { UserTopicEvent mkEvent(UserTopicVisibilityPolicy policy) => eg.userTopicEvent(stream1.streamId, 'topic', policy); + // For testing case-insensitivity + UserTopicEvent mkEventDifferentlyCased(UserTopicVisibilityPolicy policy) => + eg.userTopicEvent(stream1.streamId, 'ToPiC', policy); + + assert(() { + // (sanity check on mkEvent and mkEventDifferentlyCased) + final event1 = mkEvent(UserTopicVisibilityPolicy.followed); + final event2 = mkEventDifferentlyCased(UserTopicVisibilityPolicy.followed); + return event1.topicName.isSameAs(event2.topicName) + && event1.topicName.apiName != event2.topicName.apiName; + }()); + void checkChanges(PerAccountStore store, UserTopicVisibilityPolicy newPolicy, UserTopicVisibilityEffect expectedInStream, @@ -230,6 +257,10 @@ void main() { final event = mkEvent(newPolicy); check(store.willChangeIfTopicVisibleInStream(event)).equals(expectedInStream); check(store.willChangeIfTopicVisible (event)).equals(expectedOverall); + + final event2 = mkEventDifferentlyCased(newPolicy); + check(store.willChangeIfTopicVisibleInStream(event2)).equals(expectedInStream); + check(store.willChangeIfTopicVisible (event2)).equals(expectedOverall); } test('stream not muted, policy none -> followed, no change', () async { @@ -366,6 +397,12 @@ void main() { compareTopicVisibility(store, [ eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.unmuted), ]); + + // case-insensitivity + await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.followed); + compareTopicVisibility(store, [ + eg.userTopicItem(stream1, 'topic', UserTopicVisibilityPolicy.followed), + ]); }); test('remove, with others in stream', () async { @@ -381,7 +418,8 @@ void main() { test('remove, as last in stream', () async { final store = eg.store(); await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.none); + // case-insensitivity + await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.none); compareTopicVisibility(store, [ ]); }); @@ -389,7 +427,8 @@ void main() { test('treat unknown enum value as removing', () async { final store = eg.store(); await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.muted); - await store.setUserTopic(stream1, 'topic', UserTopicVisibilityPolicy.unknown); + // case-insensitivity + await store.setUserTopic(stream1, 'ToPiC', UserTopicVisibilityPolicy.unknown); compareTopicVisibility(store, [ ]); }); @@ -406,7 +445,8 @@ void main() { ])); check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 1'))) .equals(UserTopicVisibilityPolicy.muted); - check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 2'))) + // case-insensitivity + check(store.topicVisibilityPolicy(stream.streamId, eg.t('ToPiC 2'))) .equals(UserTopicVisibilityPolicy.unmuted); check(store.topicVisibilityPolicy(stream.streamId, eg.t('topic 3'))) .equals(UserTopicVisibilityPolicy.followed); diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index 87bb2f2f0a..fe96b427ab 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -9,6 +9,7 @@ import 'package:zulip/widgets/color.dart'; import 'package:zulip/widgets/home.dart'; import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/channel_colors.dart'; +import 'package:zulip/widgets/unread_count_badge.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; @@ -58,6 +59,7 @@ void main() { List? subscriptions, List? users, required List unreadMessages, + List? otherMessages, NavigatorObserver? navigatorObserver, }) async { addTearDown(testBinding.reset); @@ -362,6 +364,39 @@ void main() { parent: findRowByLabel(tester, topic), icon: ZulipIcons.unmute)).isTrue(); }); + + testWidgets('unmuted (topics treated case-insensitively)', (tester) async { + // Case-insensitivity of both topic-visibility and unreads data + // TODO(#1065) this belongs in test/model/ once the inbox page has + // its own view-model + + final message1 = eg.streamMessage(stream: channel, topic: 'aaa'); + final message2 = eg.streamMessage(stream: channel, topic: 'AaA', flags: [MessageFlag.read]); + final message3 = eg.streamMessage(stream: channel, topic: 'aAa', flags: [MessageFlag.read]); + await setupPage(tester, + users: [eg.selfUser, eg.otherUser], + streams: [channel], + subscriptions: [eg.subscription(channel, isMuted: true)], + unreadMessages: [message1]); + await store.setUserTopic(channel, 'aaa', UserTopicVisibilityPolicy.unmuted); + await tester.pump(); + + check(find.descendant( + of: find.byWidget(findRowByLabel(tester, 'aaa')!), + matching: find.widgetWithText(UnreadCountBadge, '1'))).findsOne(); + + await store.handleEvent(eg.updateMessageFlagsRemoveEvent(MessageFlag.read, [message2])); + await tester.pump(); + check(find.descendant( + of: find.byWidget(findRowByLabel(tester, 'aaa')!), + matching: find.widgetWithText(UnreadCountBadge, '2'))).findsOne(); + + await store.handleEvent(eg.updateMessageFlagsRemoveEvent(MessageFlag.read, [message3])); + await tester.pump(); + check(find.descendant( + of: find.byWidget(findRowByLabel(tester, 'aaa')!), + matching: find.widgetWithText(UnreadCountBadge, '3'))).findsOne(); + }); }); group('collapsing', () {