Fix forwarding notification opens from non onesignal notifs #1131

emawby · 2022-10-13T17:36:22Z

Description

One Line Summary

Fixing a swizzling issue where for non OneSignal notifications we were not forwarding notification opens to the app delegate remoteNotificationReceive.

Details

We were doing this properly for OneSignal notifs but not properly for non OneSignal notifications. Now the behavior will be the same regardless of notification content/source.

When a notification is opened our swizzled implementation of UserNotificationCenter:didReceiveNotificationResponse:withCompletionHandler: is called. We need to forward this call to other implementations of this method (which we were doing correctly). However if there are no other implementers of the UNUserNotificationCenter method then we also need to try forwarding to the old app delegate selector in case there are listeners to the legacy method but not the new one.

Motivation

swizzling bug

Scope

notification opens

Testing

Unit testing

Added a unit test that tests forwarding opens to the app delegate selector for both OneSignal and non OneSignal payloads.

Manual testing

tested against firebase messaging on a physical iOS device

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
- If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
Any Public API changes are explained in the PR details and conform to existing APIs

Testing

I have included test coverage for these changes, or explained why they are not needed
All automated tests pass, or I explained why that is not possible
I have personally tested this on my device, or explained why that is not possible

Final pass

Code is as readable as possible.
- Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
I have reviewed this PR myself, ensuring it meets each checklist item
- WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is

fhboswell · 2022-10-13T18:45:07Z

iOS_SDK/OneSignalSDK/Source/UNUserNotificationCenter+OneSignal.m

+                       didReceiveNotificationResponse:(UNNotificationResponse *)response
+                                      OneSignalCenter:(id)instance
+                                withCompletionHandler:(void(^)())completionHandler {
    // Call orginal selector if one was set.


Nit: original

fhboswell · 2022-10-13T19:01:37Z

iOS_SDK/OneSignalSDK/Source/UNUserNotificationCenter+OneSignal.m

+
+    [OneSignalUNUserNotificationCenter processiOS10Open:response];
+
+    [OneSignalUNUserNotificationCenter forwardReceivedNotificationResponseWithCenter:center didReceiveNotificationResponse:response OneSignalCenter:self withCompletionHandler:completionHandler];


@emawby Only curious, we want to forward it in any case?

Correct anything that we Swizzle we need to forward regardless of contents/origin. There could be other SDKs that are listening to the method or the app developer could be listening to it as well

Even notifications that test true to
![OneSignalHelper isOneSignalPayload:response.notification.request.content.userInfo]?

So the if statement checks if the following should be run
[OneSignalUNUserNotificationCenter processiOS10Open:response];

Maybe invert the if statement and put [OneSignalUNUserNotificationCenter processiOS10Open:response]; inside it.

good call. I inverted the check

jkasten2

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @emawby, @fhboswell, @jennantilla, and @nan-li)

iOS_SDK/OneSignalSDK/UnitTests/UIApplicationDelegateSwizzlingTests.m line 516 at r2 (raw file):

}

- (void)testNotificationOpenForwardsToLegacySelector {

Thinking we should also create a test for a non-OneSignal notification as well, to confirm the fix worked.

emawby

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fhboswell, @jennantilla, @jkasten2, and @nan-li)

iOS_SDK/OneSignalSDK/UnitTests/UIApplicationDelegateSwizzlingTests.m line 516 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Thinking we should also create a test for a non-OneSignal notification as well, to confirm the fix worked.

The first notification in the test is a Onesignal notification the second one is a non Onesignal notification

We previously were only forwarding to the legacy selector for OneSignal notifications. Now the forwarding behavior will be the same regardless of notif contents/origin.

jkasten2

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @emawby, @fhboswell, @jennantilla, and @nan-li)

iOS_SDK/OneSignalSDK/UnitTests/UIApplicationDelegateSwizzlingTests.m line 516 at r2 (raw file):

Previously, emawby (Elliot Mawby) wrote…

The first notification in the test is a Onesignal notification the second one is a non Onesignal notification

ah my bad, looked it too quick! It would be easier to read if this was two different tests, but that improvement that optional for this PR.

Based on #1131

Cherry picked from #1131

Based on #1131

Cherry picked from #1131

emawby requested review from jkasten2, nan-li, fhboswell and jennantilla October 13, 2022 17:36

emawby force-pushed the fix/forward_opens_from_non_onesignal_notifs branch from 2789b26 to d13759a Compare October 13, 2022 18:42

fhboswell reviewed Oct 13, 2022

View reviewed changes

emawby force-pushed the fix/forward_opens_from_non_onesignal_notifs branch from d13759a to 9555994 Compare October 13, 2022 19:03

emawby requested a review from fhboswell October 13, 2022 19:04

jkasten2 requested changes Oct 13, 2022

View reviewed changes

emawby commented Oct 13, 2022

View reviewed changes

emawby requested a review from jkasten2 October 13, 2022 19:30

emawby added 2 commits October 13, 2022 12:56

Foward notif opens to the legacy app selector for non OneSignal notifs

3f088b0

We previously were only forwarding to the legacy selector for OneSignal notifications. Now the forwarding behavior will be the same regardless of notif contents/origin.

Unit tests for notification open forwarding to legacy selector

5bfb129

emawby force-pushed the fix/forward_opens_from_non_onesignal_notifs branch from 9555994 to 5bfb129 Compare October 13, 2022 19:56

fhboswell approved these changes Oct 13, 2022

View reviewed changes

jkasten2 approved these changes Oct 14, 2022

View reviewed changes

emawby merged commit 80ab08d into main Oct 17, 2022

emawby deleted the fix/forward_opens_from_non_onesignal_notifs branch October 17, 2022 17:52

emawby mentioned this pull request Oct 17, 2022

Release 3.11.3 #1133

Merged

nan-li added a commit that referenced this pull request Oct 25, 2023

Foward notif opens to the legacy app selector for non OneSignal notifs

b5c3642

Based on #1131

nan-li added a commit that referenced this pull request Oct 25, 2023

Unit tests for notification open forwarding to legacy selector

35c4875

Cherry picked from #1131

nan-li mentioned this pull request Oct 25, 2023

[v5] Fix forwarding notification opens from non onesignal notifs #1326

Merged

18 tasks

nan-li added a commit that referenced this pull request Oct 30, 2023

Foward notif opens to the legacy app selector for non OneSignal notifs

8ce71d8

Based on #1131

nan-li added a commit that referenced this pull request Oct 30, 2023

Unit tests for notification open forwarding to legacy selector

5552f69

Cherry picked from #1131

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix forwarding notification opens from non onesignal notifs #1131

Fix forwarding notification opens from non onesignal notifs #1131

Uh oh!

emawby commented Oct 13, 2022 •

edited

Loading

Uh oh!

fhboswell Oct 13, 2022

Uh oh!

emawby Oct 13, 2022

Uh oh!

fhboswell Oct 13, 2022

Uh oh!

emawby Oct 13, 2022

Uh oh!

fhboswell Oct 13, 2022

Uh oh!

emawby Oct 13, 2022

Uh oh!

jkasten2 left a comment

Uh oh!

emawby left a comment

Uh oh!

jkasten2 left a comment

Uh oh!

Uh oh!


		[OneSignalUNUserNotificationCenter processiOS10Open:response];

		[OneSignalUNUserNotificationCenter forwardReceivedNotificationResponseWithCenter:center didReceiveNotificationResponse:response OneSignalCenter:self withCompletionHandler:completionHandler];

Fix forwarding notification opens from non onesignal notifs #1131

Fix forwarding notification opens from non onesignal notifs #1131

Uh oh!

Conversation

emawby commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

One Line Summary

Details

Motivation

Scope

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

Uh oh!

fhboswell Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

emawby Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

fhboswell Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

emawby Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

fhboswell Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

emawby Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

jkasten2 left a comment

Choose a reason for hiding this comment

Uh oh!

emawby left a comment

Choose a reason for hiding this comment

Uh oh!

jkasten2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emawby commented Oct 13, 2022 •

edited

Loading