From 40f2560fd975a5bccd87daef88a2db3a49c2676d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 17 Oct 2023 14:21:00 -0700 Subject: [PATCH 1/5] api/notif: Parse notification messages the server sends over FCM Much of this is based on the Kotlin code that does this same job in the zulip-mobile RN app: android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt Those Kotlin tests benefit from a feature of their test framework where when an expectation like `expect.that(foo).isEqualTo(bar)` (analogous to our `check(foo).equals(bar)`) is unsatisfied, it records the failure without immediately ending the test function. As a result, when something goes wrong we get very fine-grained results, much like we would with package:checks if all the test functions were translated as `group` and the individual expectations as `test`, rather than as `check` -- but without the overhead that the latter approach requires in the source code, particularly in making up names for the individual test cases. In this version, we go ahead and translate to `group` and `test` for a couple of the test functions where their lack would be most keenly felt, resorting to a hack to avoid cluttering the code with lots of individual names. We let the rest be just `test` and `check`. --- lib/api/notifications.dart | 274 ++++++++++++++++++++++++++++++ lib/api/notifications.g.dart | 73 ++++++++ test/api/notifications_test.dart | 277 +++++++++++++++++++++++++++++++ 3 files changed, 624 insertions(+) create mode 100644 lib/api/notifications.dart create mode 100644 lib/api/notifications.g.dart create mode 100644 test/api/notifications_test.dart diff --git a/lib/api/notifications.dart b/lib/api/notifications.dart new file mode 100644 index 0000000000..02b89892f2 --- /dev/null +++ b/lib/api/notifications.dart @@ -0,0 +1,274 @@ + +import 'package:json_annotation/json_annotation.dart'; + +part 'notifications.g.dart'; + +/// Parsed version of an FCM message, of any type. +/// +/// Firebase Cloud Messaging (FCM) is the service run by Google that we use +/// for delivering notifications to Android devices. An FCM message may +/// be to tell us we should show a notification, or something else like +/// to remove one (because the user read the underlying Zulip message). +/// +/// The word "message" can be confusing in this context, +/// and in our notification code we usually stick to more specific phrases: +/// +/// * An "FCM message" is one of the blobs we receive over FCM; what FCM docs +/// call a "message", and is also known as a "data notification". +/// +/// One of these might correspond to zero, one, or more actual notifications +/// we show in the UI. +/// +/// * A "Zulip message" is the thing that in other Zulip contexts we call +/// simply a "message": a [Message], the central item in the Zulip app model. +sealed class FcmMessage { + FcmMessage(); + + factory FcmMessage.fromJson(Map json) { + switch (json['event']) { + case 'message': return MessageFcmMessage.fromJson(json); + case 'remove': return RemoveFcmMessage.fromJson(json); + default: return UnexpectedFcmMessage.fromJson(json); + } + } + + Map toJson(); +} + +/// An [FcmMessage] of a type (a value of `event`) we didn't know about. +class UnexpectedFcmMessage extends FcmMessage { + final Map json; + + UnexpectedFcmMessage.fromJson(this.json); + + @override + Map toJson() => json; +} + +/// Base class for [FcmMessage]s that identify what Zulip account they're for. +/// +/// This includes all known types of FCM messages from Zulip +/// (all [FcmMessage] subclasses other than [UnexpectedFcmMessage]), +/// and it seems likely that it always will. +sealed class FcmMessageWithIdentity extends FcmMessage { + /// The server's `EXTERNAL_HOST` setting. This is a hostname, + /// or a colon-separated hostname-plus-port. + /// + /// For documentation, see zulip-server:zproject/prod_settings_template.py . + final String server; + + /// The realm's ID within the server. + final int realmId; + + /// The realm's own URL. + /// + /// This is a real, absolute URL which is the base for all URLs a client uses + /// with this realm. It corresponds to [GetServerSettingsResult.realmUri]. + final Uri realmUri; + + /// This user's ID within the server. + /// + /// Useful mainly in the case where the user has multiple accounts in the + /// same realm. + final int userId; + + FcmMessageWithIdentity({ + required this.server, + required this.realmId, + required this.realmUri, + required this.userId, + }); +} + +/// Parsed version of an FCM message of type `message`. +/// +/// This corresponds to a Zulip message for which the user wants to +/// see a notification. +/// +/// The word "message" can be confusing in this context. +/// See [FcmMessage] for discussion. +@JsonSerializable(fieldRename: FieldRename.snake) +@_IntConverter() +@_IntListConverter() +class MessageFcmMessage extends FcmMessageWithIdentity { + @JsonKey(includeToJson: true, name: 'event') + String get type => 'message'; + + final int senderId; + final String senderEmail; + final Uri senderAvatarUrl; + final String senderFullName; + + @JsonKey(includeToJson: false, readValue: _readWhole) + final FcmMessageRecipient recipient; + + final int zulipMessageId; + final int time; // in Unix seconds UTC, like [Message.timestamp] + + /// The content of the Zulip message, rendered as plain text. + /// + /// This is based on the HTML content, but reduced to plain text specifically + /// for use in notifications. For details, see `get_mobile_push_content` in + /// zulip/zulip:zerver/lib/push_notifications.py . + final String content; + + static Object? _readWhole(Map json, String key) => json; + + MessageFcmMessage({ + required super.server, + required super.realmId, + required super.realmUri, + required super.userId, + required this.senderId, + required this.senderEmail, + required this.senderAvatarUrl, + required this.senderFullName, + required this.recipient, + required this.zulipMessageId, + required this.content, + required this.time, + }); + + factory MessageFcmMessage.fromJson(Map json) { + assert(json['event'] == 'message'); + return _$MessageFcmMessageFromJson(json); + } + + @override + Map toJson() { + final result = _$MessageFcmMessageToJson(this); + final recipient = this.recipient; + switch (recipient) { + case FcmMessageDmRecipient(allRecipientIds: [_] || [_, _]): + break; + case FcmMessageDmRecipient(:var allRecipientIds): + result['pm_users'] = const _IntListConverter().toJson(allRecipientIds); + case FcmMessageStreamRecipient(): + result['stream_id'] = const _IntConverter().toJson(recipient.streamId); + if (recipient.streamName != null) result['stream'] = recipient.streamName; + result['topic'] = recipient.topic; + } + return result; + } +} + +/// Data identifying where a Zulip message was sent, as part of an [FcmMessage]. +sealed class FcmMessageRecipient { + FcmMessageRecipient(); + + factory FcmMessageRecipient.fromJson(Map json) { + // There's also a `recipient_type` field, but we don't really need it. + // The presence or absence of `stream_id` is just as informative. + return json.containsKey('stream_id') + ? FcmMessageStreamRecipient.fromJson(json) + : FcmMessageDmRecipient.fromJson(json); + } +} + +/// An [FcmMessageRecipient] for a Zulip message to a stream. +@JsonSerializable(fieldRename: FieldRename.snake, createToJson: false) +@_IntConverter() +class FcmMessageStreamRecipient extends FcmMessageRecipient { + // Sending the stream ID in notifications is new in Zulip Server 5. + // But handling the lack of it would add complication, and we don't strictly + // need to -- we intend (#268) to cut pre-server-5 support before beta release. + // TODO(server-5): cut comment + final int streamId; + + // Current servers (as of 2023) always send the stream name. But + // future servers might not, once clients get the name from local data. + // So might as well be ready. + @JsonKey(name: 'stream') + final String? streamName; + + final String topic; + + FcmMessageStreamRecipient({required this.streamId, required this.streamName, required this.topic}); + + factory FcmMessageStreamRecipient.fromJson(Map json) => + _$FcmMessageStreamRecipientFromJson(json); +} + +/// An [FcmMessageRecipient] for a Zulip message that was a DM. +class FcmMessageDmRecipient extends FcmMessageRecipient { + final List allRecipientIds; + + FcmMessageDmRecipient({required this.allRecipientIds}); + + factory FcmMessageDmRecipient.fromJson(Map json) { + return FcmMessageDmRecipient(allRecipientIds: switch (json) { + // Group DM conversations ("huddles") are represented with `pm_users`, + // which lists all the user IDs in the conversation. + // TODO check they're sorted. + {'pm_users': var pmUsers} => const _IntListConverter().fromJson(pmUsers), + + // 1:1 DM conversations have no `pm_users`. Knowing that it's a + // 1:1 DM, `sender_id` is enough to identify the conversation. + {'sender_id': var senderId, 'user_id': var userId} => + _pairSet(_parseInt(senderId), _parseInt(userId)), + + _ => throw Exception("bad recipient"), + }); + } + + /// The set {id1, id2}, represented as a sorted list. + // (In set theory this is called the "pair" of id1 and id2: https://en.wikipedia.org/wiki/Axiom_of_pairing .) + static List _pairSet(int id1, int id2) { + if (id1 == id2) return [id1]; + if (id1 < id2) return [id1, id2]; + return [id2, id1]; + } +} + +@JsonSerializable(fieldRename: FieldRename.snake) +@_IntConverter() +@_IntListConverter() +class RemoveFcmMessage extends FcmMessageWithIdentity { + @JsonKey(includeToJson: true, name: 'event') + String get type => 'remove'; + + // Servers have sent zulipMessageIds, obsoleting the singular zulipMessageId + // and just sending the first ID there redundantly, since 2019. + // See zulip-mobile@4acd07376. + + final List zulipMessageIds; + // final String? zulipMessageId; // obsolete; ignore + + RemoveFcmMessage({ + required super.server, + required super.realmId, + required super.realmUri, + required super.userId, + required this.zulipMessageIds, + }); + + factory RemoveFcmMessage.fromJson(Map json) { + assert(json['event'] == 'remove'); + return _$RemoveFcmMessageFromJson(json); + } + + @override + Map toJson() => _$RemoveFcmMessageToJson(this); +} + +class _IntListConverter extends JsonConverter, String> { + const _IntListConverter(); + + @override + List fromJson(String json) => json.split(',').map(_parseInt).toList(); + + @override + String toJson(List value) => value.join(','); +} + +class _IntConverter extends JsonConverter { + const _IntConverter(); + + @override + int fromJson(String json) => _parseInt(json); + + @override + String toJson(int value) => value.toString(); +} + +int _parseInt(String string) => int.parse(string, radix: 10); diff --git a/lib/api/notifications.g.dart b/lib/api/notifications.g.dart new file mode 100644 index 0000000000..aa587fb7d1 --- /dev/null +++ b/lib/api/notifications.g.dart @@ -0,0 +1,73 @@ +// GENERATED CODE - DO NOT MODIFY BY HAND + +// ignore_for_file: constant_identifier_names, unnecessary_cast + +part of 'notifications.dart'; + +// ************************************************************************** +// JsonSerializableGenerator +// ************************************************************************** + +MessageFcmMessage _$MessageFcmMessageFromJson(Map json) => + MessageFcmMessage( + server: json['server'] as String, + realmId: const _IntConverter().fromJson(json['realm_id'] as String), + realmUri: Uri.parse(json['realm_uri'] as String), + userId: const _IntConverter().fromJson(json['user_id'] as String), + senderId: const _IntConverter().fromJson(json['sender_id'] as String), + senderEmail: json['sender_email'] as String, + senderAvatarUrl: Uri.parse(json['sender_avatar_url'] as String), + senderFullName: json['sender_full_name'] as String, + recipient: FcmMessageRecipient.fromJson( + MessageFcmMessage._readWhole(json, 'recipient') + as Map), + zulipMessageId: + const _IntConverter().fromJson(json['zulip_message_id'] as String), + content: json['content'] as String, + time: const _IntConverter().fromJson(json['time'] as String), + ); + +Map _$MessageFcmMessageToJson(MessageFcmMessage instance) => + { + 'server': instance.server, + 'realm_id': const _IntConverter().toJson(instance.realmId), + 'realm_uri': instance.realmUri.toString(), + 'user_id': const _IntConverter().toJson(instance.userId), + 'event': instance.type, + 'sender_id': const _IntConverter().toJson(instance.senderId), + 'sender_email': instance.senderEmail, + 'sender_avatar_url': instance.senderAvatarUrl.toString(), + 'sender_full_name': instance.senderFullName, + 'zulip_message_id': const _IntConverter().toJson(instance.zulipMessageId), + 'time': const _IntConverter().toJson(instance.time), + 'content': instance.content, + }; + +FcmMessageStreamRecipient _$FcmMessageStreamRecipientFromJson( + Map json) => + FcmMessageStreamRecipient( + streamId: const _IntConverter().fromJson(json['stream_id'] as String), + streamName: json['stream'] as String?, + topic: json['topic'] as String, + ); + +RemoveFcmMessage _$RemoveFcmMessageFromJson(Map json) => + RemoveFcmMessage( + server: json['server'] as String, + realmId: const _IntConverter().fromJson(json['realm_id'] as String), + realmUri: Uri.parse(json['realm_uri'] as String), + userId: const _IntConverter().fromJson(json['user_id'] as String), + zulipMessageIds: const _IntListConverter() + .fromJson(json['zulip_message_ids'] as String), + ); + +Map _$RemoveFcmMessageToJson(RemoveFcmMessage instance) => + { + 'server': instance.server, + 'realm_id': const _IntConverter().toJson(instance.realmId), + 'realm_uri': instance.realmUri.toString(), + 'user_id': const _IntConverter().toJson(instance.userId), + 'event': instance.type, + 'zulip_message_ids': + const _IntListConverter().toJson(instance.zulipMessageIds), + }; diff --git a/test/api/notifications_test.dart b/test/api/notifications_test.dart new file mode 100644 index 0000000000..7047d67490 --- /dev/null +++ b/test/api/notifications_test.dart @@ -0,0 +1,277 @@ +import 'package:checks/checks.dart'; +import 'package:test/scaffolding.dart'; +import 'package:zulip/api/notifications.dart'; + +import '../stdlib_checks.dart'; + +void main() { + final baseBaseJson = { + "server": "zulip.example.cloud", + "realm_id": "4", + "realm_uri": "https://zulip.example.com/", + "user_id": "234", + }; + + void checkParseFails(Map data) { + check(() => FcmMessage.fromJson(data)).throws(); + } + + group('FcmMessage', () { + test('parse fails on missing or bad event type', () { + check(FcmMessage.fromJson({})).isA(); + check(FcmMessage.fromJson({'event': 'nonsense'})).isA(); + }); + }); + + group('MessageFcmMessage', () { + final baseJson = { + ...baseBaseJson, + "event": "message", + + "sender_id": "123", + "sender_email": "sender@example.com", + "sender_avatar_url": "https://zulip.example.com/avatar/123.jpeg", + "sender_full_name": "A Sender", + + "time": "1546300800", + "zulip_message_id": "12345", + + "content": "This is a message", + "content_truncated": "This is a m…", + }; + + final streamJson = { + ...baseJson, + "recipient_type": "stream", + "stream_id": "42", + "stream": "denmark", + "topic": "play", + + "alert": "New stream message from A Sender in denmark", + }; + + final groupDmJson = { + ...baseJson, + "recipient_type": "private", + "pm_users": "123,234,345", + + "alert": "New private group message from A Sender", + }; + + final dmJson = { + ...baseJson, + "recipient_type": "private", + + "alert": "New private message from A Sender", + }; + + test("'message' messages parse as MessageFcmMessage", () { + check(FcmMessage.fromJson(streamJson)).isA(); + }); + + MessageFcmMessage parse(Map json) { + return FcmMessage.fromJson(json) as MessageFcmMessage; + } + + test("fields get parsed right in 'message' happy path", () { + check(parse(streamJson)) + ..server.equals(baseJson['server']!) + ..realmId.equals(4) + ..realmUri.equals(Uri.parse(baseJson['realm_uri']!)) + ..userId.equals(234) + ..senderId.equals(123) + ..senderEmail.equals(streamJson['sender_email']!) + ..senderAvatarUrl.equals(Uri.parse(streamJson['sender_avatar_url']!)) + ..senderFullName.equals(streamJson['sender_full_name']!) + ..zulipMessageId.equals(12345) + ..recipient.isA().which(it() + ..streamId.equals(42) + ..streamName.equals(streamJson['stream']!) + ..topic.equals(streamJson['topic']!)) + ..content.equals(streamJson['content']!) + ..time.equals(1546300800); + + check(parse(groupDmJson)) + .recipient.isA() + .allRecipientIds.deepEquals([123, 234, 345]); + + check(parse(dmJson)) + .recipient.isA() + .allRecipientIds.deepEquals([123, 234]); + }); + + test('optional fields missing cause no error', () { + check(parse({ ...streamJson }..remove('stream'))) + .recipient.isA().which(it() + ..streamId.equals(42) + ..streamName.isNull()); + }); + + test('toJson round-trips', () { + void checkRoundTrip(Map json) { + check(parse(json).toJson()) + .deepEquals({ ...json } + ..remove('recipient_type') // Redundant with stream_id. + ..remove('content_truncated') // Redundant with content. + ..remove('alert') // Redundant with the other data; we make our own UI. + ); + } + + checkRoundTrip(streamJson); + checkRoundTrip(groupDmJson); + checkRoundTrip(dmJson); + checkRoundTrip({ ...streamJson }..remove('stream')); + }); + + test('ignored fields missing have no effect', () { + final baseline = parse(streamJson); + check(parse({ ...streamJson }..remove('recipient_type'))).jsonEquals(baseline); + check(parse({ ...streamJson }..remove('content_truncated'))).jsonEquals(baseline); + check(parse({ ...streamJson }..remove('alert'))).jsonEquals(baseline); + }); + + test('obsolete or novel fields have no effect', () { + final baseline = parse(dmJson); + void checkInert(Map extraJson) => + check(parse({ ...dmJson, ...extraJson })).jsonEquals(baseline); + + // Cut in 2017, in zulip/zulip@c007b9ea4. + checkInert({ 'user': 'client@example.com' }); + + // Hypothetical future field. + checkInert({ 'awesome_feature': 'enabled' }); + }); + + group("parse failures on malformed 'message'", () { + int n = 1; + test("${n++}", () => checkParseFails({ ...dmJson }..remove('server'))); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('realm_id'))); + test("${n++}", () => checkParseFails({ ...dmJson, 'realm_id': '12,34' })); + test("${n++}", () => checkParseFails({ ...dmJson, 'realm_id': 'abc' })); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('realm_uri'))); + test(skip: true, // Dart's Uri.parse is lax in what it accepts. + "${n++}", () => checkParseFails({ ...dmJson, 'realm_uri': 'zulip.example.com' })); + test(skip: true, // Dart's Uri.parse is lax in what it accepts. + "${n++}", () => checkParseFails({ ...dmJson, 'realm_uri': '/examplecorp' })); + + test("${n++}", () => checkParseFails({ ...streamJson, 'stream_id': '12,34' })); + test("${n++}", () => checkParseFails({ ...streamJson, 'stream_id': 'abc' })); + test("${n++}", () => checkParseFails({ ...streamJson }..remove('topic'))); + test("${n++}", () => checkParseFails({ ...groupDmJson, 'pm_users': 'abc,34' })); + test("${n++}", () => checkParseFails({ ...groupDmJson, 'pm_users': '12,abc' })); + test("${n++}", () => checkParseFails({ ...groupDmJson, 'pm_users': '12,' })); + + test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_avatar_url'))); + test(skip: true, // Dart's Uri.parse is lax in what it accepts. + "${n++}", () => checkParseFails({ ...dmJson, 'sender_avatar_url': '/avatar/123.jpeg' })); + test(skip: true, // Dart's Uri.parse is lax in what it accepts. + "${n++}", () => checkParseFails({ ...dmJson, 'sender_avatar_url': '' })); + + test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_id'))); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_email'))); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_full_name'))); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('zulip_message_id'))); + test("${n++}", () => checkParseFails({ ...dmJson, 'zulip_message_id': '12,34' })); + test("${n++}", () => checkParseFails({ ...dmJson, 'zulip_message_id': 'abc' })); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('content'))); + test("${n++}", () => checkParseFails({ ...dmJson }..remove('time'))); + test("${n++}", () => checkParseFails({ ...dmJson, 'time': '12:34' })); + }); + }); + + group('RemoveFcmMessage', () { + final baseJson = { + ...baseBaseJson, + 'event': 'remove', + + 'zulip_message_ids': '123,234', + 'zulip_message_id': '123', + }; + + test("'remove' messages parse as RemoveFcmMessage", () { + check(FcmMessage.fromJson(baseJson)).isA(); + }); + + RemoveFcmMessage parse(Map json) { + return FcmMessage.fromJson(json) as RemoveFcmMessage; + } + + test('fields get parsed right in happy path', () { + check(parse(baseJson)) + ..server.equals(baseJson['server']!) + ..realmId.equals(4) + ..realmUri.equals(Uri.parse(baseJson['realm_uri']!)) + ..userId.equals(234) + ..zulipMessageIds.deepEquals([123, 234]); + }); + + test('toJson round-trips', () { + check(parse(baseJson).toJson()) + .deepEquals({ ...baseJson }..remove('zulip_message_id')); + }); + + test('ignored fields missing have no effect', () { + final baseline = parse(baseJson); + check(parse({ ...baseJson }..remove('zulip_message_id'))).jsonEquals(baseline); + }); + + test('obsolete or novel fields have no effect', () { + final baseline = parse(baseJson); + check(parse({ ...baseJson, 'awesome_feature': 'enabled' })).jsonEquals(baseline); + }); + + group('parse failures on malformed data', () { + int n = 1; + + test("${n++}", () => checkParseFails({ ...baseJson }..remove('server'))); + test("${n++}", () => checkParseFails({ ...baseJson }..remove('realm_id'))); + test("${n++}", () => checkParseFails({ ...baseJson, 'realm_id': 'abc' })); + test("${n++}", () => checkParseFails({ ...baseJson, 'realm_id': '12,34' })); + test("${n++}", () => checkParseFails({ ...baseJson }..remove('realm_uri'))); + test(skip: true, // Dart's Uri.parse is lax in what it accepts. + "${n++}", () => checkParseFails({ ...baseJson, 'realm_uri': 'zulip.example.com' })); + test(skip: true, // Dart's Uri.parse is lax in what it accepts. + "${n++}", () => checkParseFails({ ...baseJson, 'realm_uri': '/examplecorp' })); + + for (final badIntList in ["abc,34", "12,abc", "12,", ""]) { + test("${n++}", () => checkParseFails({ ...baseJson, 'zulip_message_ids': badIntList })); + } + }); + }); +} + +extension UnexpectedFcmMessageChecks on Subject { + Subject> get json => has((x) => x.json, 'json'); +} + +extension FcmMessageWithIdentityChecks on Subject { + Subject get server => has((x) => x.server, 'server'); + Subject get realmId => has((x) => x.realmId, 'realmId'); + Subject get realmUri => has((x) => x.realmUri, 'realmUri'); + Subject get userId => has((x) => x.userId, 'userId'); +} + +extension MessageFcmMessageChecks on Subject { + Subject get senderId => has((x) => x.senderId, 'senderId'); + Subject get senderEmail => has((x) => x.senderEmail, 'senderEmail'); + Subject get senderAvatarUrl => has((x) => x.senderAvatarUrl, 'senderAvatarUrl'); + Subject get senderFullName => has((x) => x.senderFullName, 'senderFullName'); + Subject get recipient => has((x) => x.recipient, 'recipient'); + Subject get zulipMessageId => has((x) => x.zulipMessageId, 'zulipMessageId'); + Subject get time => has((x) => x.time, 'time'); + Subject get content => has((x) => x.content, 'content'); +} + +extension FcmMessageStreamRecipientChecks on Subject { + Subject get streamId => has((x) => x.streamId, 'streamId'); + Subject get streamName => has((x) => x.streamName, 'streamName'); + Subject get topic => has((x) => x.topic, 'topic'); +} + +extension FcmMessageDmRecipientChecks on Subject { + Subject> get allRecipientIds => has((x) => x.allRecipientIds, 'allRecipientIds'); +} + +extension RemoveFcmMessageChecks on Subject { + Subject> get zulipMessageIds => has((x) => x.zulipMessageIds, 'zulipMessageIds'); +} From 679c2965b5baed054a3f09dc2e4390e62d1e8303 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 24 Oct 2023 11:54:51 -0700 Subject: [PATCH 2/5] api/notif test: Cut some redundant test cases The first version of this test file included these for ease of comparison with zulip-mobile's FcmMessageTest.kt; but they don't really add anything over the more detailed test cases that follow. --- test/api/notifications_test.dart | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/test/api/notifications_test.dart b/test/api/notifications_test.dart index 7047d67490..3545c55480 100644 --- a/test/api/notifications_test.dart +++ b/test/api/notifications_test.dart @@ -65,15 +65,11 @@ void main() { "alert": "New private message from A Sender", }; - test("'message' messages parse as MessageFcmMessage", () { - check(FcmMessage.fromJson(streamJson)).isA(); - }); - MessageFcmMessage parse(Map json) { return FcmMessage.fromJson(json) as MessageFcmMessage; } - test("fields get parsed right in 'message' happy path", () { + test("fields get parsed right in happy path", () { check(parse(streamJson)) ..server.equals(baseJson['server']!) ..realmId.equals(4) @@ -188,10 +184,6 @@ void main() { 'zulip_message_id': '123', }; - test("'remove' messages parse as RemoveFcmMessage", () { - check(FcmMessage.fromJson(baseJson)).isA(); - }); - RemoveFcmMessage parse(Map json) { return FcmMessage.fromJson(json) as RemoveFcmMessage; } From 7f61c93608cb5f0448ecea4cbbcd14fdb70353d2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 24 Oct 2023 11:53:00 -0700 Subject: [PATCH 3/5] api/notif test: Cut "alert" from test data, reflecting server update --- test/api/notifications_test.dart | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/test/api/notifications_test.dart b/test/api/notifications_test.dart index 3545c55480..a5d3b660ce 100644 --- a/test/api/notifications_test.dart +++ b/test/api/notifications_test.dart @@ -24,6 +24,9 @@ void main() { }); group('MessageFcmMessage', () { + // These JSON test data aim to reflect what current servers send. + // We ignore some of the fields; see tests. + final baseJson = { ...baseBaseJson, "event": "message", @@ -46,23 +49,17 @@ void main() { "stream_id": "42", "stream": "denmark", "topic": "play", - - "alert": "New stream message from A Sender in denmark", }; final groupDmJson = { ...baseJson, "recipient_type": "private", "pm_users": "123,234,345", - - "alert": "New private group message from A Sender", }; final dmJson = { ...baseJson, "recipient_type": "private", - - "alert": "New private message from A Sender", }; MessageFcmMessage parse(Map json) { @@ -109,7 +106,6 @@ void main() { .deepEquals({ ...json } ..remove('recipient_type') // Redundant with stream_id. ..remove('content_truncated') // Redundant with content. - ..remove('alert') // Redundant with the other data; we make our own UI. ); } @@ -123,7 +119,6 @@ void main() { final baseline = parse(streamJson); check(parse({ ...streamJson }..remove('recipient_type'))).jsonEquals(baseline); check(parse({ ...streamJson }..remove('content_truncated'))).jsonEquals(baseline); - check(parse({ ...streamJson }..remove('alert'))).jsonEquals(baseline); }); test('obsolete or novel fields have no effect', () { @@ -134,6 +129,9 @@ void main() { // Cut in 2017, in zulip/zulip@c007b9ea4. checkInert({ 'user': 'client@example.com' }); + // Cut in 2023, in zulip/zulip@5d8897b90. + checkInert({ 'alert': 'New private message from A Sender' }); + // Hypothetical future field. checkInert({ 'awesome_feature': 'enabled' }); }); From 30e77e076546d6d0dbed42cfc4d18f0f29969f5c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 18 Oct 2023 16:56:46 -0700 Subject: [PATCH 4/5] api/notif: Cut obsolete senderEmail in notification messages In zulip-mobile we use this to identify the conversation in the case of a 1:1 DM thread, as a legacy of senderId having not always been present. Here we'll use the numeric ID from the start. --- lib/api/notifications.dart | 3 +-- lib/api/notifications.g.dart | 2 -- test/api/notifications_test.dart | 5 ++--- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/api/notifications.dart b/lib/api/notifications.dart index 02b89892f2..dbca5aeaa8 100644 --- a/lib/api/notifications.dart +++ b/lib/api/notifications.dart @@ -95,7 +95,7 @@ class MessageFcmMessage extends FcmMessageWithIdentity { String get type => 'message'; final int senderId; - final String senderEmail; + // final String senderEmail; // obsolete; ignore final Uri senderAvatarUrl; final String senderFullName; @@ -120,7 +120,6 @@ class MessageFcmMessage extends FcmMessageWithIdentity { required super.realmUri, required super.userId, required this.senderId, - required this.senderEmail, required this.senderAvatarUrl, required this.senderFullName, required this.recipient, diff --git a/lib/api/notifications.g.dart b/lib/api/notifications.g.dart index aa587fb7d1..07282ff076 100644 --- a/lib/api/notifications.g.dart +++ b/lib/api/notifications.g.dart @@ -15,7 +15,6 @@ MessageFcmMessage _$MessageFcmMessageFromJson(Map json) => realmUri: Uri.parse(json['realm_uri'] as String), userId: const _IntConverter().fromJson(json['user_id'] as String), senderId: const _IntConverter().fromJson(json['sender_id'] as String), - senderEmail: json['sender_email'] as String, senderAvatarUrl: Uri.parse(json['sender_avatar_url'] as String), senderFullName: json['sender_full_name'] as String, recipient: FcmMessageRecipient.fromJson( @@ -35,7 +34,6 @@ Map _$MessageFcmMessageToJson(MessageFcmMessage instance) => 'user_id': const _IntConverter().toJson(instance.userId), 'event': instance.type, 'sender_id': const _IntConverter().toJson(instance.senderId), - 'sender_email': instance.senderEmail, 'sender_avatar_url': instance.senderAvatarUrl.toString(), 'sender_full_name': instance.senderFullName, 'zulip_message_id': const _IntConverter().toJson(instance.zulipMessageId), diff --git a/test/api/notifications_test.dart b/test/api/notifications_test.dart index a5d3b660ce..d9c9d9c55c 100644 --- a/test/api/notifications_test.dart +++ b/test/api/notifications_test.dart @@ -73,7 +73,6 @@ void main() { ..realmUri.equals(Uri.parse(baseJson['realm_uri']!)) ..userId.equals(234) ..senderId.equals(123) - ..senderEmail.equals(streamJson['sender_email']!) ..senderAvatarUrl.equals(Uri.parse(streamJson['sender_avatar_url']!)) ..senderFullName.equals(streamJson['sender_full_name']!) ..zulipMessageId.equals(12345) @@ -106,6 +105,7 @@ void main() { .deepEquals({ ...json } ..remove('recipient_type') // Redundant with stream_id. ..remove('content_truncated') // Redundant with content. + ..remove('sender_email') // Redundant with sender_id. ); } @@ -119,6 +119,7 @@ void main() { final baseline = parse(streamJson); check(parse({ ...streamJson }..remove('recipient_type'))).jsonEquals(baseline); check(parse({ ...streamJson }..remove('content_truncated'))).jsonEquals(baseline); + check(parse({ ...streamJson }..remove('sender_email'))).jsonEquals(baseline); }); test('obsolete or novel fields have no effect', () { @@ -162,7 +163,6 @@ void main() { "${n++}", () => checkParseFails({ ...dmJson, 'sender_avatar_url': '' })); test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_id'))); - test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_email'))); test("${n++}", () => checkParseFails({ ...dmJson }..remove('sender_full_name'))); test("${n++}", () => checkParseFails({ ...dmJson }..remove('zulip_message_id'))); test("${n++}", () => checkParseFails({ ...dmJson, 'zulip_message_id': '12,34' })); @@ -243,7 +243,6 @@ extension FcmMessageWithIdentityChecks on Subject { extension MessageFcmMessageChecks on Subject { Subject get senderId => has((x) => x.senderId, 'senderId'); - Subject get senderEmail => has((x) => x.senderEmail, 'senderEmail'); Subject get senderAvatarUrl => has((x) => x.senderAvatarUrl, 'senderAvatarUrl'); Subject get senderFullName => has((x) => x.senderFullName, 'senderFullName'); Subject get recipient => has((x) => x.recipient, 'recipient'); From 7a3f13db34cd2418d82623eb334f8102c5fa80b9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Jun 2023 22:46:53 -0700 Subject: [PATCH 5/5] notif: Parse messages received, still just debug-printing --- lib/notifications.dart | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/notifications.dart b/lib/notifications.dart index 2e891e2f5e..ab1fadbe1c 100644 --- a/lib/notifications.dart +++ b/lib/notifications.dart @@ -1,5 +1,6 @@ import 'package:flutter/foundation.dart'; +import 'api/notifications.dart'; import 'log.dart'; import 'model/binding.dart'; @@ -69,6 +70,10 @@ class NotificationService { static void _onRemoteMessage(FirebaseRemoteMessage message) { assert(debugLog("notif message: ${message.data}")); - // TODO(#122): parse data; show notification UI + final data = FcmMessage.fromJson(message.data); + if (data is MessageFcmMessage) { + assert(debugLog('notif message content: ${data.content}')); + // TODO(#122): show notification UI + } } }