Allow sending push notifications to multiple identities and with dynamic text #939
Conversation
tnotheis
changed the title
There was a problem hiding this comment.
Hint for reviewer: I simplified the Send method of the connectors a lot. Before my change, you were able to pass multiple registrations, and the Send method sent a push notification to each of them.
Now you can only pass one registration.
Another change to this file was the addition of a notificationText parameter, which serves the purpose of this PR (being able to dynamically pass a text). It's now the responsibility of the PushService to read the notification text from the resource dictionary, or just forward the one it receives from the caller.
There was a problem hiding this comment.
Hint for reviewer: (same as for the ApplePushNotificationServiceConnector)
| Task SendFilteredNotification(IdentityAddress recipient, IPushNotification notification, IEnumerable<string> excludedDevices, CancellationToken cancellationToken); | ||
| Task SendNotification(IPushNotification notification, SendPushNotificationFilter filter, CancellationToken cancellationToken); | ||
|
|
||
| Task SendNotification(IPushNotification notification, SendPushNotificationFilter filter, Dictionary<string, NotificationText> notificationTexts, |
There was a problem hiding this comment.
Hint for reviewer: this is one of the main purposes of this PR. I added this overload, which accepts an additional parameter notificationTexts, which is a dictionary, where the key is the language and the value is the text.
There was a problem hiding this comment.
Hint for reviewer: The other important change in this file is the addition of the filter parameter. This replaces the recipient parameter, as well as the excludedDevices parameter, which allowed me to get rid of the SendFilteredNotification method.
It also enables me to pass multiple recipients by adding multiple addresses to the SendPushNotificationFilter.IncludedIdentities property.
There was a problem hiding this comment.
Hint for reviewer: I deleted most of the tests in this class, because:
- I made a simplification of the
DeleteRegistrationmethod, which makes the test useless, as the new implementation just forwards the call to the repository. - The new
SendNotificationmethod is very hard to test. I couldn't find a better design though. :( Maybe some day we can come up with a better approach. (manual tests were successful)
Readiness checklist