[User Model] Fix notification event types; click, willDisplay, and dismiss #1078
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changed OSNotification field names to match Android and iOS; example id -> notificationId Rename OSNotificationDataPayload -> OSMinifiedNotificationPayload Logic for parsing was moved out of ServiceWorker.ts
Fixed the public types for these and interduced them as types where they were missing. Improved ClickedNotifications name to be PendingClickedNotifications to make it clear these are events that are to be fired once conditions are met.
We changed the OSNotification object format however we need to continue serializing in the same format with indexDb. This is so existing event after migrating to the new major version. Defining serializing mapping like this ensures any future changes also don't break compiablity with indexDb records going forward.
This was done for the same reasons as the last commit
The single executeWebhooks method was handlig 3 different events, each with their own slightly different paload format. We split this out into 3 different classes to map the payloads. And agian, for the some reasons as the other serializers / maps is to ensure the field name changes don't cause breaking changes in the future.
rgomezp
approved these changes
Aug 8, 2023
| */ | ||
| readonly launchURL?: string; | ||
| } | ||
|
|
| @@ -19,7 +24,7 @@ class AppState { | |||
| // default true | |||
| lastKnownOptedIn: boolean = true; | |||
|
|
|||
| clickedNotifications: ClickedNotifications | undefined; | |||
| pendingNotificationClickEvents: PendingNotificationClickEvents | undefined; | |||
| } | |||
|
|
|||
| export { AppState }; | |||
| export class OSWebhookSender { | ||
| async send(payload: IOSWebhookEventPayload): Promise<void> { | ||
| const webhookTargetUrl = await Database.get<string>('Options', `webhooks.${payload.event}`); | ||
| if (!webhookTargetUrl) |
jkasten2
commented
Aug 8, 2023
| readonly url?: string; | ||
| } | ||
|
|
||
| export class NotificationClickForOpenHandlingSerializer { |
There was a problem hiding this comment.
add comment here to check the top of the file for more details.
- This way you see hint in VS code / yoru IDE.
| readonly url?: string; | ||
| } | ||
|
|
||
| export class NotificationClickForOpenHandlingSerializer { |
There was a problem hiding this comment.
Look into adding a test that checks to ensure this is being used to serialize data in and out of indexDb, checking that nothing is doing it directly.
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
1 Line Summary
Fix public SDK API field names and types for the notification event types; click, willDisplay, and dismiss
Details
Scope of changes summary
The changes are related to OSNotification and the places where it is used, in changing the fields it was discovered there is a lot of tight coupling with other things such as indexDb and webhooks. While they may seem unrelated changes to those were needed and refactored to a point to decouple them for the future.
OSNotification
This wasn't using the same field names as Android and iOS so corrected it to match and renamed it to an interface
IOSNotification.Notification Events:
NotificationClickResultinstead ofNotificationClickEventNotificationDismissEventindexDb
Since
IOSNotificationchanged as well as the format for events we introducedOSNotificationDatabaseSerializer.tsto convert the format to and from the database. This was done so no migration is needed, and also to decouple and future proof any changes to these classes.Webhooks
Since we added type where they were missing,
executeWebhooksneeded to be refactored as it was account for 3 different types of payloads; click, display, and dismiss. This was refactored into 3 different payload classes, again to decouple and future proof field changes.Validation
Tests
Test on Chrome on macOS. Ensuring notifications are displayed and click handling directs to the correct URL.
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of arraysyntax. PreferforEachor usemapcontextif possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.contextScreenshots
Info
Checklist
Related Tickets
This change is