fix: differentiate every push event handler installed in app #743

levibostian · 2024-06-17T19:51:34Z

https://linear.app/customerio/issue/MBL-383/[bug]-customer-unable-to-handle-push-notification-event-on-ios-with

At runtime when PushEventHandlerProxy.addPushEventHandler is called, the given push event handler will overrwrite any previously added push event handlers. This results in the SDK only being aware of 1 push event handler even if the iOS app has N number installed.

Testing

In Flutter sample app, install CIO SDK, FlutterFire, and set UNNotificationCenterDelegate.current().delegate = self in AppDelegate for the host app to also handle pushes. This reproduces the bug because we now have at least 2 other push click handlers besides the CIO SDK installed. Set a Xcode breakpoint in PushEventHandlerProxy.addPushEventHandler and check what dictionary key is generated.
I added new automated tests around PushEventHandlerProxy.addPushEventHandler function.

https://linear.app/customerio/issue/MBL-383/[bug]-customer-unable-to-handle-push-notification-event-on-ios-with In a Flutter app, when `PushEventHandlerProxy.addPushEventHandler` is called, `String(describing:)` is generating a non-unique String which results in only 1 entry existing in the `nestedDelegates` entry. This results in the SDK only being aware of 1 push event handler even if the iOS app has N number installed. Testing * In Flutter sample app, install CIO SDK, FlutterFire, and set `UNNotificationCenterDelegate.current().delegate = self` for the host app to also handle pushes. Set a Xcode breakpoint in `PushEventHandlerProxy.addPushEventHandler` and check what key String is generated. After this PR, the key is unique. * The automated test suite already contains a test that should be catching this. Keeping this PR as a draft while I determine why that happened and get it fixed to prevent scenarios like this again.

github-actions · 2024-06-17T19:51:45Z

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.

CocoaPods-FCM: levi/v2-multiple-push-handlers (1718981011)
APN-UIKit: levi/v2-multiple-push-handlers (1718980978)

It passes before and after. But this change will make it better.

levibostian · 2024-06-18T14:40:39Z

Tests/MessagingPush/PushHandling/PushEventHandlerProxyTest.swift

@@ -3,7 +3,7 @@ import Foundation
 import SharedTests
 import XCTest

-class PushEventHandlerProxyTest: UnitTest {
+class PushEventHandlerProxyTest: IntegrationTest {


Only changed so the class had access to the new getNewPushEventHandler() function. Should have no effect on the tests besides that.

levibostian · 2024-06-18T14:44:21Z

Tests/MessagingPush/PushHandling/PushEventHandlerProxyTest.swift

@@ -12,13 +12,47 @@ class PushEventHandlerProxyTest: UnitTest {
        proxy = PushEventHandlerProxyImpl()
    }

+    // MARK: addPushEventHandler


These new tests catch the bug that this PR fixes.

codecov · 2024-06-18T14:46:00Z

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (v2@949b05c). Learn more about missing BASE report.

Files	Patch %	Lines
...gingPush/UserNotificationsFramework/Wrappers.swift	40.00%	3 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##             v2     #743   +/-   ##
=====================================
  Coverage      ?   57.69%           
=====================================
  Files         ?      132           
  Lines         ?     3806           
  Branches      ?        0           
=====================================
  Hits          ?     2196           
  Misses        ?     1610           
  Partials      ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

levibostian · 2024-06-18T14:48:16Z

Sources/MessagingPush/UserNotificationsFramework/Wrappers.swift

@@ -120,6 +120,10 @@ public struct UNNotificationWrapper: PushNotification {
 class UNUserNotificationCenterDelegateWrapper: PushEventHandler {
    private let delegate: UNUserNotificationCenterDelegate

+    var identifier: String {
+        String(describing: delegate)


By returning the instance description of the nested delegate, this is what will uniquely identify UNUserNotificationCenterDelegateWrapper instances from 1 another. Since UNUserNotificationCenterDelegateWrapper is just a wrapper, it's the nested delegate that we really care about.

levibostian · 2024-06-18T14:49:16Z

Sources/MessagingPush/PushHandling/iOSPushEventListener.swift

@@ -21,6 +21,10 @@ class IOSPushEventListener: PushEventHandler {
        self.logger = logger
    }

+    var identifier: String {
+        "Cio.iOSPushEventListener"


The CIO SDK has it's own PushEventHandler instance so we must add the new protocol identifier getter.

This string should uniquely identify the CIO SDK from other push click handlers installed in the app.

levibostian · 2024-06-18T14:49:36Z

Sources/MessagingPush/PushHandling/PushEventHandlerProxy.swift

@@ -27,7 +27,7 @@ class PushEventHandlerProxyImpl: PushEventHandlerProxy {
    public static let shared = PushEventHandlerProxyImpl()

    // Use a map so that we only save 1 instance of a given handler.
-    @Atomic private var nestedDelegates: [String: PushEventHandler] = [:]
+    @Atomic var nestedDelegates: [String: PushEventHandler] = [:]


made internal for tests to access

levibostian · 2024-06-18T14:50:27Z

Sources/MessagingPush/PushHandling/PushEventHandlerProxy.swift

@@ -37,7 +37,7 @@ class PushEventHandlerProxyImpl: PushEventHandlerProxy {
    }

    func addPushEventHandler(_ newHandler: PushEventHandler) {
-        nestedDelegates[String(describing: newHandler)] = newHandler
+        nestedDelegates[newHandler.identifier] = newHandler


The bug fix.

At runtime, multiple UNUserNotificationCenterDelegateWrapper instances will call this function. Using identifier makes sure that each handler is uniquely identified.

Shahroz16

LGTM

levibostian · 2024-06-18T19:28:33Z

added do not merge label until this is resolved

…745)

## [2.13.2](2.13.1...2.13.2) (2024-06-21) ### Bug Fixes * differentiate every push event handler installed in app ([#743](#743)) ([c3573fc](c3573fc))

https://linear.app/customerio/issue/MBL-383/[bug]-customer-unable-to-handle-push-notification-event-on-ios-with (This PR is identical to the one merged into v2: #743) Summary of bug: The iOS SDK had a bug where it was only able to store 1 3rd party push click handing SDK and ignore others that were installed. After this fix, all 3rd party SDKs installed in the app are called for push events in the app.

levibostian self-assigned this Jun 17, 2024

levibostian added 2 commits June 18, 2024 08:30

improve existing integration test

c307ed2

It passes before and after. But this change will make it better.

write tests for addPushEventHandler. fix existing tests

c9f894c

levibostian commented Jun 18, 2024

View reviewed changes

levibostian marked this pull request as ready for review June 18, 2024 14:50

levibostian requested a review from a team June 18, 2024 14:53

levibostian mentioned this pull request Jun 18, 2024

chore: show local push notifications in FCM sample app #744

Closed

Shahroz16 approved these changes Jun 18, 2024

View reviewed changes

levibostian added the Do not merge label Jun 18, 2024

chore: improve logs when forwarding push events to 3rd part handlers (#…

cb4a84d

…745)

levibostian merged commit c3573fc into v2 Jun 21, 2024
9 checks passed

levibostian deleted the levi/v2-multiple-push-handlers branch June 21, 2024 15:22

github-actions bot pushed a commit that referenced this pull request Jun 21, 2024

chore: prepare for 2.13.2

46bda24

## [2.13.2](2.13.1...2.13.2) (2024-06-21) ### Bug Fixes * differentiate every push event handler installed in app ([#743](#743)) ([c3573fc](c3573fc))

levibostian mentioned this pull request Jun 24, 2024

fix: differentiate every push event handler installed in app customerio/customerio-reactnative#283

Merged

levibostian mentioned this pull request Jun 25, 2024

fix: differentiate every push event handler installed in app #751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: differentiate every push event handler installed in app #743

fix: differentiate every push event handler installed in app #743

levibostian commented Jun 17, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024 •

edited

Loading

levibostian Jun 18, 2024

levibostian Jun 18, 2024

codecov bot commented Jun 18, 2024 •

edited

Loading

levibostian Jun 18, 2024

levibostian Jun 18, 2024

levibostian Jun 18, 2024

levibostian Jun 18, 2024

Shahroz16 left a comment

levibostian commented Jun 18, 2024

fix: differentiate every push event handler installed in app #743

fix: differentiate every push event handler installed in app #743

Conversation

levibostian commented Jun 17, 2024 • edited Loading

github-actions bot commented Jun 17, 2024 • edited Loading

Sample app builds 📱

levibostian Jun 18, 2024

Choose a reason for hiding this comment

levibostian Jun 18, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

levibostian Jun 18, 2024

Choose a reason for hiding this comment

levibostian Jun 18, 2024

Choose a reason for hiding this comment

levibostian Jun 18, 2024

Choose a reason for hiding this comment

levibostian Jun 18, 2024

Choose a reason for hiding this comment

Shahroz16 left a comment

Choose a reason for hiding this comment

levibostian commented Jun 18, 2024

levibostian commented Jun 17, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading