-
Notifications
You must be signed in to change notification settings - Fork 440
notif: On Android, use org name instead of URL as notif group summary #2136
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import '../stdlib_checks.dart'; | |
| void main() { | ||
| final baseBaseJson = <String, Object?>{ | ||
| "realm_url": "https://zulip.example.com/", | ||
| "realm_name": "Example Organization", | ||
| "user_id": 234, | ||
| }; | ||
|
|
||
|
|
@@ -19,6 +20,7 @@ void main() { | |
| "realm_id": "4", | ||
| "realm_uri": "https://zulip.example.com/", // TODO(server-9) | ||
| "realm_url": "https://zulip.example.com/", | ||
| "realm_name": "Example Organization", | ||
| "user_id": "234", | ||
| }; | ||
|
|
||
|
|
@@ -115,6 +117,7 @@ void main() { | |
| check(parse(streamJson)) | ||
| ..realmUrl.equals(Uri.parse(baseJson['realm_url'] as String)) | ||
| ..realmUrl.equals(Uri.parse(baseJsonPreE2ee['realm_uri'] as String)) // TODO(server-9) | ||
| ..realmName.equals(baseBaseJson['realm_name'] as String) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a similar check in the RemoveFcmMessage tests below, since those objects also gain this field |
||
| ..userId.equals(234) | ||
| ..senderId.equals(123) | ||
| ..senderAvatarUrl.equals(Uri.parse(streamJson['sender_avatar_url'] as String)) | ||
|
|
@@ -137,6 +140,9 @@ void main() { | |
| }); | ||
|
|
||
| test('optional fields missing cause no error', () { | ||
| check(parse({ ...streamJson }..remove('realm_name'))) | ||
| .realmName.isNull(); | ||
|
|
||
| check(parse({ ...streamJson }..remove('channel_name'))) | ||
| .recipient.isA<FcmMessageChannelRecipient>().which((it) => it | ||
| ..channelId.equals(42) | ||
|
|
@@ -260,6 +266,7 @@ void main() { | |
| check(parse(baseJson)) | ||
| ..realmUrl.equals(Uri.parse(baseJson['realm_url'] as String)) | ||
| ..realmUrl.equals(Uri.parse(preE2eeJson['realm_uri'] as String)) // TODO(server-9) | ||
| ..realmName.equals(baseJson['realm_name'] as String) | ||
| ..userId.equals(234) | ||
| ..messageIds.deepEquals([123, 234]); | ||
| }); | ||
|
|
@@ -318,6 +325,7 @@ extension UnexpectedFcmMessageChecks on Subject<UnexpectedFcmMessage> { | |
|
|
||
| extension FcmMessageWithIdentityChecks on Subject<FcmMessageWithIdentity> { | ||
| Subject<Uri> get realmUrl => has((x) => x.realmUrl, 'realmUrl'); | ||
| Subject<String?> get realmName => has((x) => x.realmName, 'realmName'); | ||
| Subject<int> get userId => has((x) => x.userId, 'userId'); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,14 +24,18 @@ import 'package:zulip/widgets/theme.dart'; | |
| import '../example_data.dart' as eg; | ||
| import '../fake_async.dart'; | ||
| import '../model/binding.dart'; | ||
| import '../model/store_checks.dart'; | ||
| import '../test_images.dart'; | ||
| import '../api/notifications_test.dart'; | ||
|
|
||
| MessageFcmMessage messageFcmMessage( | ||
| Message zulipMessage, { | ||
| String? streamName, | ||
| String? realmName, | ||
| Account? account, | ||
| }) { | ||
| account ??= eg.selfAccount; | ||
| realmName ??= account.realmName; | ||
| final narrow = SendableNarrow.ofMessage(zulipMessage, selfUserId: account.userId); | ||
| return FcmMessage.fromJson({ | ||
| "event": "message", | ||
|
|
@@ -40,6 +44,7 @@ MessageFcmMessage messageFcmMessage( | |
| "realm_id": "4", | ||
| "realm_uri": account.realmUrl.toString(), | ||
| "user_id": account.userId.toString(), | ||
| if (realmName != null) "realm_name": realmName, | ||
|
|
||
| "zulip_message_id": zulipMessage.id.toString(), | ||
| "time": zulipMessage.timestamp.toString(), | ||
|
|
@@ -343,13 +348,19 @@ void main() { | |
| }); | ||
|
|
||
| group('NotificationDisplayManager show', () { | ||
| void checkNotification(MessageFcmMessage data, { | ||
| void checkNotification( | ||
| MessageFcmMessage data, { | ||
| Account? account, | ||
| required List<MessageFcmMessage> messageStyleMessages, | ||
| required String expectedTitle, | ||
| required String expectedTagComponent, | ||
| required bool expectedIsGroupConversation, | ||
| List<int>? expectedIconBitmap = kSolidBlueAvatar, | ||
| String? expectedSummaryText, | ||
| }) { | ||
| account ??= eg.selfAccount; | ||
| assert(account.userId == data.userId | ||
| && account.realmUrl == data.realmUrl); | ||
| assert(messageStyleMessages.every((e) => e.userId == data.userId)); | ||
| assert(messageStyleMessages.every((e) => e.realmUrl == data.realmUrl)); | ||
|
|
||
|
|
@@ -367,6 +378,9 @@ void main() { | |
| FcmMessageDmRecipient(:var allRecipientIds) => | ||
| DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId), | ||
| }).buildAndroidNotificationUrl(); | ||
| expectedSummaryText ??= account.realmName | ||
| ?? data.realmName | ||
| ?? data.realmUrl.toString(); | ||
|
|
||
| final messageStyleMessagesChecks = | ||
| messageStyleMessages.mapIndexed((i, messageData) { | ||
|
|
@@ -429,13 +443,16 @@ void main() { | |
| ..groupKey.equals(expectedGroupKey) | ||
| ..isGroupSummary.equals(true) | ||
| ..inboxStyle.which((it) => it.isNotNull() | ||
| ..summaryText.equals(data.realmUrl.toString())) | ||
| ..summaryText.equals(expectedSummaryText!)) | ||
| ..autoCancel.equals(true) | ||
| ..contentIntent.isNull(), | ||
| ]); | ||
| } | ||
|
|
||
| Future<void> checkNotifications(FakeAsync async, MessageFcmMessage data, { | ||
| Future<void> checkNotifications( | ||
| FakeAsync async, | ||
| MessageFcmMessage data, { | ||
| Account? account, | ||
| required String expectedTitle, | ||
| required String expectedTagComponent, | ||
| required bool expectedIsGroupConversation, | ||
|
|
@@ -448,6 +465,7 @@ void main() { | |
| RemoteMessage(data: data.toJson())); | ||
| async.flushMicrotasks(); | ||
| checkNotification(data, | ||
| account: account, | ||
| messageStyleMessages: [data], | ||
| expectedIsGroupConversation: expectedIsGroupConversation, | ||
| expectedTitle: expectedTitle, | ||
|
|
@@ -458,6 +476,7 @@ void main() { | |
| RemoteMessage(data: data.toJson())); | ||
| async.flushMicrotasks(); | ||
| checkNotification(data, | ||
| account: account, | ||
| messageStyleMessages: [data], | ||
| expectedIsGroupConversation: expectedIsGroupConversation, | ||
| expectedTitle: expectedTitle, | ||
|
|
@@ -634,6 +653,112 @@ void main() { | |
| expectedTagComponent: 'stream:${message.streamId}:${message.topic}'); | ||
| }))); | ||
|
|
||
| test('stream message: different realms', () => runWithHttpClient(() => awaitFakeAsync((async) async { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "channel message", I think? Oh I see; there's a strong pattern of saying "stream" in this test's siblings. So "stream" is fine, I think, and we can sweep through later and change them all together (#631). |
||
| await init(addSelfAccount: false); | ||
|
|
||
| final account1 = eg.account( | ||
| id: 1001, | ||
| user: eg.user(), | ||
| realmUrl: Uri.parse('http://realm1.example'), | ||
| realmName: 'Realm 1'); | ||
| await testBinding.globalStore.add(account1, eg.initialSnapshot()); | ||
| final account2 = eg.account( | ||
| id: 1002, | ||
| user: eg.user(), | ||
| realmUrl: Uri.parse('http://realm2.example'), | ||
| realmName: 'Realm 2'); | ||
| await testBinding.globalStore.add(account2, eg.initialSnapshot()); | ||
|
|
||
| final stream = eg.stream(); | ||
| final topic = 'test topic'; | ||
| final data1 = messageFcmMessage( | ||
| eg.streamMessage(stream: stream, topic: topic), | ||
| account: account1, streamName: stream.name); | ||
| final data2 = messageFcmMessage( | ||
| eg.streamMessage(stream: stream, topic: topic), | ||
| account: account2, streamName: stream.name); | ||
|
|
||
| receiveFcmMessage(async, data1); | ||
| checkNotification(data1, | ||
| account: account1, | ||
| messageStyleMessages: [data1], | ||
| expectedIsGroupConversation: true, | ||
| expectedTitle: '#${stream.name} > $topic', | ||
| expectedTagComponent: 'stream:${stream.streamId}:$topic', | ||
| expectedSummaryText: account1.realmName); | ||
|
|
||
| receiveFcmMessage(async, data2); | ||
| checkNotification(data2, | ||
| account: account2, | ||
| messageStyleMessages: [data2], | ||
| expectedIsGroupConversation: true, | ||
| expectedTitle: '#${stream.name} > $topic', | ||
| expectedTagComponent: 'stream:${stream.streamId}:$topic', | ||
| expectedSummaryText: account2.realmName); | ||
| }))); | ||
|
|
||
| test('stream message: realm name absent in account, falls back to notif data', () => runWithHttpClient(() => awaitFakeAsync((async) async { | ||
| await init(addSelfAccount: false); | ||
|
|
||
| var account = eg.account( | ||
| id: 1001, | ||
| user: eg.user(), | ||
| realmUrl: Uri.parse('http://realm1.example')); | ||
| await testBinding.globalStore.add(account, eg.initialSnapshot()); | ||
| // Override the default realmName from eg.account(). | ||
| account = await testBinding.globalStore.updateAccount(account.id, | ||
| AccountsCompanion(realmName: const Value(null))); | ||
| check(account).realmName.isNull(); | ||
|
|
||
| final stream = eg.stream(); | ||
| final topic = 'test topic'; | ||
| final data = messageFcmMessage( | ||
| eg.streamMessage(stream: stream, topic: topic), | ||
| account: account, | ||
| streamName: stream.name, | ||
| realmName: 'Notif realm name'); | ||
| check(data).realmName.equals('Notif realm name'); | ||
|
|
||
| receiveFcmMessage(async, data); | ||
| checkNotification(data, | ||
| account: account, | ||
| messageStyleMessages: [data], | ||
| expectedIsGroupConversation: true, | ||
| expectedTitle: '#${stream.name} > $topic', | ||
| expectedTagComponent: 'stream:${stream.streamId}:$topic', | ||
| expectedSummaryText: 'Notif realm name'); | ||
| }))); | ||
|
|
||
| test('stream message: realm name absent in account and notif data, falls back to realm URL', () => runWithHttpClient(() => awaitFakeAsync((async) async { | ||
| await init(addSelfAccount: false); | ||
|
|
||
| var account = eg.account( | ||
| id: 1001, | ||
| user: eg.user(), | ||
| realmUrl: Uri.parse('http://realm1.example')); | ||
| await testBinding.globalStore.add(account, eg.initialSnapshot()); | ||
| // Override the default realmName from eg.account(). | ||
| account = await testBinding.globalStore.updateAccount(account.id, | ||
| AccountsCompanion(realmName: const Value(null))); | ||
| check(account).realmName.isNull(); | ||
|
|
||
| final stream = eg.stream(); | ||
| final topic = 'test topic'; | ||
| final data = messageFcmMessage( | ||
| eg.streamMessage(stream: stream, topic: topic), | ||
| account: account, streamName: stream.name); | ||
| check(data).realmName.isNull(); | ||
|
|
||
| receiveFcmMessage(async, data); | ||
| checkNotification(data, | ||
| account: account, | ||
| messageStyleMessages: [data], | ||
| expectedIsGroupConversation: true, | ||
| expectedTitle: '#${stream.name} > $topic', | ||
| expectedTagComponent: 'stream:${stream.streamId}:$topic', | ||
| expectedSummaryText: account.realmUrl.toString()); | ||
| }))); | ||
|
|
||
| test('group DM: 3 users', () => runWithHttpClient(() => awaitFakeAsync((async) async { | ||
| await init(); | ||
| final message = eg.dmMessage(from: eg.thirdUser, to: [eg.otherUser, eg.selfUser]); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's put realm name next to realm URL, since they're more closely related to each other than either is to the user ID