Notification conversion #382

KennyHuRadar · 2024-08-16T15:24:28Z

We are creating 3 new events:

opened_radar_notification: when user opens the app via tapping on notification generated by the Radar SDK
opened_notification: when user opens the app by tapping on other notifications from the app
delivered_on_premise_notification: when our SDK detects on premise notification getting delivered

delivered_on_premise_notification may be noticed by our SDK and called when:

didEnterRegion is fired via the OS (real time receipt but requires background)
when background task fires (technically may be as frequent as once an hour, but in practice may be once or twice a day)
when the user next opens the app

QA that background processing works:

lmeier · 2024-08-22T19:26:08Z

RadarSDK/RadarNotificationHelper.m

+}
+
+ (void)checkForSentOnPremiseNotifications {
+    // check if there any pending notifications that have been sent, we will only be doing this if the app is not been opened by a notification


lmeier · 2024-08-22T19:28:47Z

RadarSDK/RadarNotificationHelper.m

+ (void)scheduleBackgroundNotificationChecks {
+    if (@available(iOS 13.0, *)) {
+        BGAppRefreshTaskRequest *request = [[BGAppRefreshTaskRequest alloc] initWithIdentifier:@"io.radar.notificationCheck"];
+        request.earliestBeginDate = [NSDate dateWithTimeIntervalSinceNow:60];


let's schedule this for an hour?

lmeier · 2024-08-27T18:36:44Z

RadarSDK/RadarNotificationHelper.m

+    [RadarState clearPendingNotificationRequests];
+
+    // Call the original method (which is now swizzled)
+    [self swizzled_userNotificationCenter:center didReceiveNotificationResponse:response withCompletionHandler:completionHandler];


I don't have firsthand experience with swizzling, but this looks like a recursive call, which if it were, wouldn't be right. What's going on here?

since we swizzled the methods, [[self swizzled_userNotificationCenter:center didReceiveNotificationResponse:response withCompletionHandler:completionHandler];] now links to the UNUserNotificationCenter

lmeier · 2024-08-27T18:38:08Z

RadarSDK/RadarNotificationHelper.m

+    // do we still want to log the normal app open event?
+    [RadarSettings updateLastAppOpenTime];


Note to self to check this against the opened_app logic.

yeah this is right, since we are now having the open notification be a form of the opened_app event, we do not want to call the code inside logOpenedAppConversion

lmeier · 2024-08-27T18:39:40Z

RadarSDK/RadarNotificationHelper.m

+
+ (void)addOnPremiseNotificationRequests:(NSArray<UNNotificationRequest *> *)requests {
+    UNUserNotificationCenter *notificationCenter = [UNUserNotificationCenter currentNotificationCenter];
+    [notificationCenter getNotificationSettingsWithCompletionHandler:^(UNNotificationSettings *settings) {


Let's use this to grab the authorization status and stash it in state as well

I agree that this its probably something we want to do and send up like we do send up location authorization.
However this is slightly off tangent to this PR and requires some server work as well, should we do this in another PR?

lmeier · 2024-08-27T18:44:49Z

RadarSDK/RadarNotificationHelper.m

+        if (error) {
+            [[RadarLogger sharedInstance] logWithLevel:RadarLogLevelError message:[NSString stringWithFormat:@"Error scheduling app refresh task: %@", error]];
+        }


Seeing the following in Waypoint:

Error scheduling app refresh task: Error Domain=BGTaskSchedulerErrorDomain Code=3 "(null)"

We get this when we try and schedule on the simulator.

KennyHuRadar · 2024-08-28T21:01:31Z

per offline discussion, we want to get notification permissions and just store it in RadarState for now.

KennyHuRadar · 2024-08-30T15:20:06Z

https://expo.dev/accounts/radarlabs/projects/waypoint/builds/3710b2c5-2bd5-4672-bf4a-129ac06c87c0

…n they fire

lmeier · 2024-09-04T15:48:23Z

RadarSDK/RadarNotificationHelper.m

+ (void)checkForSentOnPremiseNotifications {
+    NSArray<UNNotificationRequest *> *registeredNotifications = [RadarState pendingNotificationRequests];
+    if (NSClassFromString(@"XCTestCase") == nil) {
+        [[UNUserNotificationCenter currentNotificationCenter] getPendingNotificationRequestsWithCompletionHandler:^(NSArray<UNNotificationRequest *> *_Nonnull requests) {
+            NSMutableSet *pendingIdentifiers = [NSMutableSet new];
+
+            for (UNNotificationRequest *request in requests) {
+                [pendingIdentifiers addObject:request.identifier];
+            }
+
+            for (UNNotificationRequest *request in registeredNotifications) {
+                if (![pendingIdentifiers containsObject:request.identifier]) {
+                    [[RadarLogger sharedInstance] logWithLevel:RadarLogLevelDebug message:[NSString stringWithFormat:@"Found pending notification | identifier = %@", request]];
+                    NSDate *lastCheckedTime = [RadarState lastCheckedOnPremiseNotification];
+                    [Radar logConversionWithNotification:request eventName:@"delivered_on_premise_notification" conversionSource:nil deliveredAfter:lastCheckedTime];
+                    // prevent double counting of the same notification
+                    [RadarState removePendingNotificationRequest:request];
+                }
+            }            
+        }];
+    }
+    [RadarState lastCheckedOnPremiseNotification];
+}


Let's add some RadarLogger logs to this

lmeier · 2024-09-04T19:41:49Z

RadarSDK/RadarNotificationHelper.m

+        NSMutableArray *identifiers = [NSMutableArray new];
+        for (UNNotificationRequest *request in requests) {
+            if ([request.identifier hasPrefix:kSyncGeofenceIdentifierPrefix]) {
+                [[RadarLogger sharedInstance] logWithLevel:RadarLogLevelInfo message:[NSString stringWithFormat:@"Found pending notification | identifier = %@", request.identifier]];


let's say "Found pending notification to remove |..."

lmeier · 2024-09-04T19:45:22Z

RadarSDK/RadarLocationManager.m

+    [RadarNotificationHelper checkForSentOnPremiseNotifications];
+
+    [RadarState clearPendingNotificationRequests];
+
+    [RadarNotificationHelper removePendingNotificationsWithCompletionHandler: ^{
+        [RadarNotificationHelper addOnPremiseNotificationRequests:requests];


Possible race? Do we need to call removePendingNotificationsWithCompletionHandler in a completion handler of clearPendingNotificationRequests (which itself maybe is called in a completion handler of checkForSentOnPremiseNotifications)

we do indeed need a completion handler for checkForSentOnPremiseNotifications but not for clearPendingNotificationRequests since it does not step on the other calls.

KennyHuRadar and others added 9 commits August 15, 2024 09:28

save for branch switch

1dd8a1f

Merge branch 'master' into swizzling-unnotification-center

6930308

skip swizzling for tests

cbce1a7

use different type for each kind of opening notification

564138b

save for eod

8c0d25c

eod-ish save

a5ac78f

run only notification center checks on device and not tests

4f48ed7

make the callback nullable

a7d3039

modularize notification delivery detetection logic and also refactor

d1ada93

KennyHuRadar marked this pull request as ready for review August 21, 2024 18:06

KennyHuRadar added 5 commits August 21, 2024 14:38

implement background processing and cleanup

07287a3

clean up

688fddb

version restrict background processing

6052111

clean up

143f105

clean up

ebbcfd4

lmeier reviewed Aug 22, 2024

View reviewed changes

KennyHuRadar added 13 commits August 23, 2024 11:01

clean up

225f917

resolve merge conflict

6fbefc0

move swizzle out of init

17606a9

add swift name

9163718

change init of background jobs

41b9c0e

move swizzling back into init

6256006

do not add notifications if we do not have permissions

ee95354

change name to native setup

6b89e9c

log all app opens for now

5985f0e

add info to metadata

a9154aa

change to metadata fields

dd2d1a8

change name to conversionSource

e3dd770

change casing

7edff55

KennyHuRadar added 3 commits August 27, 2024 14:05

only log either notification open or normal app open

14e879b

do nothing for the method notification

f287a75

formatting

65a31ad

KennyHuRadar changed the title ~~[WIP] notification conversion~~ Notification conversion Aug 27, 2024

lmeier reviewed Aug 27, 2024

View reviewed changes

KennyHuRadar and others added 2 commits August 27, 2024 15:57

remove dead comment

07b120b

Merge branch 'master' into swizzling-unnotification-center

28ea860

KennyHuRadar added 3 commits August 29, 2024 13:25

remember notification permission into radar state

51e80cb

change logging level

fab81f6

move the call so that it does not perform in test env

1b20ad1

temp adding webhook pings to bg tasks so we have a better idea on whe…

d6dfc1a

…n they fire

lmeier reviewed Sep 4, 2024

View reviewed changes

KennyHuRadar added 2 commits September 4, 2024 15:16

changes and fixes, server side working

5c9ff70

add more logging

ad9bbba

lmeier reviewed Sep 4, 2024

View reviewed changes

KennyHuRadar added 2 commits September 4, 2024 15:52

fix potential race condition

fab123a

remove clearing of pending notifications when the app foregrounds

eb46559

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification conversion #382

Notification conversion #382

KennyHuRadar commented Aug 16, 2024 •

edited

Loading

lmeier Aug 22, 2024

lmeier Aug 22, 2024

lmeier Aug 27, 2024

KennyHuRadar Aug 27, 2024

lmeier Aug 27, 2024

KennyHuRadar Aug 27, 2024

lmeier Aug 27, 2024

KennyHuRadar Aug 27, 2024

lmeier Aug 27, 2024

KennyHuRadar Aug 27, 2024

KennyHuRadar commented Aug 28, 2024

KennyHuRadar commented Aug 30, 2024

lmeier Sep 4, 2024

lmeier Sep 4, 2024

lmeier Sep 4, 2024

KennyHuRadar Sep 4, 2024

		// do we still want to log the normal app open event?
		[RadarSettings updateLastAppOpenTime];

Notification conversion #382

Are you sure you want to change the base?

Notification conversion #382

Conversation

KennyHuRadar commented Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KennyHuRadar commented Aug 28, 2024

KennyHuRadar commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KennyHuRadar commented Aug 16, 2024 •

edited

Loading