Skip to content

Late setNotificationOpenedHandler call processing #826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Dec 7, 2020

Changes

  • If OneSignal.setNotificationOpenedHandler is called after
    didFinishLaunchingWithOptions then we still want their handler to fire.
    • So basically the OneSignal SDK will hold onto the event for them
      and fire it as soon as handler is setup.
  • This means more flexibility on where / when the developer wants to call
    setNotificationOpenedHandler.
  • This is the exact behavior provided in the Android SDK in this commit.

Manual Testing

Tested on iOS 14.2 with the following steps.

  1. Send notification to device
  2. Swipe away app.
    • To get app into a killed state.
  3. Tap on notification.
    • This is a "cold start" thanks to step 2
  4. Code setup to call OneSignal.setNotificationOpenedHandler with a delay of 3 seconds
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 3 * NSEC_PER_SEC), dispatch_get_main_queue(), ^{
    id openNotificationHandler = ^(OSNotificationOpenedResult *result) {
        UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Notification Opened Event!" message:@"Notification Opened Event!" delegate:self cancelButtonTitle:@"Close" otherButtonTitles:@"Ok", nil];
        [alert show];
    };
    [OneSignal setNotificationOpenedHandler:openNotificationHandler];
});
  1. Wait 3 seconds
  2. Confirmed alert dialog showed.

When to merge

  • This can be done either before or after the final 3.0.0 release.
  • However should be done before any wrapper final releases that depend on 3.0.0.
    • Otherwise wrapper will continue to need to handle this edge case, which is not ideal. Ideal too as we will be retesting this flow as well.

This change is Reviewable

@jkasten2
Copy link
Member Author

jkasten2 commented Dec 7, 2020

Test failure is due to log size in TravisCI. I confirmed all tests passed locally.

if (!notificationOpenedHandler)
return;

for(OSNotificationOpenedResult* notification in [self getUnprocessedOpenedNotifis]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after the for 🙏

Copy link
Member Author

@jkasten2 jkasten2 Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I always do that 🤣

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created fixup commit 4e0fc0c to address this.

@@ -358,8 +379,10 @@ + (void)handleNotificationAction:(OSNotificationActionType)actionType actionID:(

[OneSignalTrackFirebaseAnalytics trackOpenEvent:result];

if (!notificationOpenedHandler)
if (!notificationOpenedHandler) {
[[self getUnprocessedOpenedNotifis] addObject:result];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe have this addObject in a method, to have the UnprocessedOpenedNotifis mutations centralized?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created fixup commit 4e0fc0c to address this.

@jkasten2 jkasten2 requested a review from Jeasmine December 7, 2020 20:02
* If OneSignal.setNotificationOpenedHandler is called after
didFinishLaunchingWithOptions then we still want their handler to fire.
   - So basicly the OneSignal SDK will hold onto the event for them
   and fire it as soon as handler is setup.
* This means more flexability on where / when the developer wants to call
setNotificationOpenedHandler.
* This is the exact behavior provided in the Android SDK in this commit.
   - OneSignal/OneSignal-Android-SDK@ce5ad7d#diff-2708f86f77e539cab647ecede9d5fcbead3bf0f954d7f9e536eafbc78ba760d6R340-R344
@jkasten2 jkasten2 force-pushed the feat/add_late_setNotificationOpenedHandler_processing branch from 4e0fc0c to 1a59df2 Compare December 7, 2020 20:36
@jkasten2
Copy link
Member Author

jkasten2 commented Dec 7, 2020

Ran git rebase -i --autosquash major_release_3.0.0 then git push -f to clean up fixup commit.

@jkasten2 jkasten2 merged commit 1c68072 into major_release_3.0.0 Dec 7, 2020
@jkasten2 jkasten2 deleted the feat/add_late_setNotificationOpenedHandler_processing branch December 7, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants