Skip to content
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

iOS notifications showing in foreground #298

Closed
roryabraham opened this issue Oct 20, 2020 · 26 comments
Closed

iOS notifications showing in foreground #298

roryabraham opened this issue Oct 20, 2020 · 26 comments

Comments

@roryabraham
Copy link

roryabraham commented Oct 20, 2020

Preliminary Info

What Airship dependencies are you using?

Using urbanairship-react-native ^8.1.0

What are the versions of any relevant development tools you are using?

Using XCode 12.0 on physical iPhone device running 13.6.1

Report

What unexpected behavior are you seeing?

I have been working with Urban Airship in my React Native project for a few weeks, and it's working pretty well. At one point, I wanted to try to get notifications to appear in the foreground in iOS, so I added the following code:

UrbanAirship.setForegroundPresentationOptions(_.mapObject(_.invert(NotificationOptionsIOS), () => true));

And it worked! Notifications began appearing in the foreground. However, I am now unable to revert this action, and no matter what, notifications always appear in the foreground. I tried removing the above code, and I also tried replacing it with the following:

UrbanAirship.setForegroundPresentationOptions();
UrbanAirship.setForegroundPresentationOptions(_.mapObject(_.invert(NotificationOptionsIOS), () => false));

Each time, I ran this from the root of my React Native project:

rm -rf node_modules && npm install
cd ios && pod deintegrate && pod install

Then did a clean build in XCode (CMD+SHIFT+K), and re-ran the app. Each time, notifications continued to appear in the foreground.

I also have no custom notifications, and nothing in my AirshipConfig.plist other than the necessary app keys

What is the expected behavior?

Notifications should not appear in the foreground by default on iOS.

What are the steps to reproduce the unexpected behavior?

Not 100% sure how to reproduce, but I would try setting all foreground options to true, then removing that same code. I assume that this is either easily reproducible, or some weird issue with some cached linked binary or something. Looking at the code for UrbanAirship.setForegroundPresentationOptions, it looks like setting these values to null or false might not overwrite existing values.

@roryabraham
Copy link
Author

roryabraham commented Oct 20, 2020

I also tried clearing the linked binaries from my project's build phases in XCode -> MyProject -> Build Phases -> Link binary with libraries

image

@marc-scig
Copy link
Contributor

@roryabraham hmm that is indeed strange. The code for setForegroundPresentationOptions looks ok to me at first glance, but let me see if I can reproduce this first before speculating on what might be going on.

@marc-scig
Copy link
Contributor

@roryabraham ok I think I see what's going on here. It's not the setForegroundPresentationOptions function per se, but rather the way the options are stored. We store the options in NSUserDefaults so that they only have to be set once, and there's a bit of logic in the the UARCTAutopilot class that retrieves them from disk at startup, but it's reading the value as the wrong type. This looks like an easy fix, so I'll get the ball rolling on a patch.

@roryabraham
Copy link
Author

Great! Thanks!

@marc-scig
Copy link
Contributor

Hey @roryabraham,

9.0.1 is out, which has a fix for this issue. Let us know if you encounter any additional problems.

@roryabraham
Copy link
Author

Great! I'll adopt the patch and let you know if there are any issues. Thanks for jumping on this quickly

@roryabraham
Copy link
Author

Hmmm - unfortunately it looks like this didn't actually resolve my issue. After updating to 9.0.1 I repeated all the same steps with no behavior change.

@rlepinski
Copy link
Contributor

Sorry to hear you still have the issue, we will take another look to see what else we missed.

@rlepinski rlepinski reopened this Oct 29, 2020
@roryabraham
Copy link
Author

Thanks. Let me know if there's anything you want me to test on my side!

@rlepinski
Copy link
Contributor

rlepinski commented Oct 30, 2020

@roryabraham I am unable to reproduce. The new patch seems to work great for me using your exact code:

    if (enabled) {
      UrbanAirship.setForegroundPresentationOptions(_.mapObject(_.invert(NotificationOptionsIOS), () => true));
    } else {
      UrbanAirship.setForegroundPresentationOptions(_.mapObject(_.invert(NotificationOptionsIOS), () => false));
    }

I tested switching the value at runtime and did a cold start of the app to verify it persisted properly.

You probably did, but could you verify you ran pod install after your update? We currently do not log the version of the plugin on start and the log for presentation options is logging the wrong value (dictionary instead of the integer), so the only way you can verify its using the right code is if you enable verbose logging in the airship config and look for this line:

        "X-UA-Frameworks" = "react-native:9.0.1";

Which gets logged when we upload analytics events (should happen within 10 seconds of a launch).

@roryabraham
Copy link
Author

Hi @rlepinski sorry for the long delay between replies, but even doing a pod deintegrate && pod install, we're seeing this issue in our production application even on 9.0.1

@rlepinski
Copy link
Contributor

@roryabraham Could you try to reproduce in a sample app for us? I am unable to so its hard to figure out whats going wrong. We will be doing a minor release next week, which I will make it easier to determine what version of the SDK and plugin by doing proper logging.

@rlepinski
Copy link
Contributor

@roryabraham I just released 10.0.0 with better logging, but it does require Xcode 12. If you could update, you can verify a log message saying both the SDK and react Airship version as well as better logging when the foreground presentation options change.

https://github.com/urbanairship/react-native-module/blob/main/urbanairship-react-native/ios/UARCTModule/UARCTAutopilot.m#L32
https://github.com/urbanairship/react-native-module/blob/main/urbanairship-react-native/ios/UARCTModule/UARCTAutopilot.m#L52
https://github.com/urbanairship/react-native-module/blob/main/urbanairship-react-native/ios/UARCTModule/UrbanAirshipReactModule.m#L301

It does require log level to be debug, you can set it in the AirshipConfigOptions by setting developmentLogLevel/productionLogLevel to DEBUG

@roryabraham
Copy link
Author

Hi @rlepinski, thanks for following up again. After upgrading to 10.0.0, these are all the UA logs I'm seeing:

2020-12-21 16:26:03.291091-0800 Expensify.cash[715:123724] [I] +[UAirship executeUnsafeTakeOff:] [Line 301] UAirship Take Off! Lib Version: 14.2.0 App Key: uulSSfTDQJ2r0PMpjRrhmQ Production: NO.
2020-12-21 16:26:03.294240-0800 Expensify.cash[715:123724] Channel ID: 3f2241f9-eccf-4261-afef-30a57376c841
2020-12-21 16:26:03.298967-0800 Expensify.cash[715:123724] [D] -[UANamedUser updateNamedUserAssociation] [Line 139] Named user already updated. Skipping.
2020-12-21 16:26:03.315939-0800 Expensify.cash[715:123724] [I] +[UAirship executeUnsafeTakeOff:] [Line 358] Automatic setup enabled.
2020-12-21 16:26:03.324115-0800 Expensify.cash[715:123724] [D] -[UAAutomationEngine updateTriggersWithScheduleID:type:argument:incrementAmount:] [Line 611] Updating triggers with type: 7
2020-12-21 16:26:03.324250-0800 Expensify.cash[715:123724] [I] +[UARCTAutopilot performTakeOff:] [Line 32] Airship ReactNative version: 10.0.0, SDK version: 14.2.0
2020-12-21 16:26:03.333664-0800 Expensify.cash[715:123724] [D] -[UAInboxMessageList retrieveMessageListWithSuccessBlock:withFailureBlock:] [Line 142] Retrieving message list.
2020-12-21 16:26:03.333728-0800 Expensify.cash[715:123724] [D] -[UAAutomationEngine updateTriggersWithScheduleID:type:argument:incrementAmount:] [Line 611] Updating triggers with type: 0
2020-12-21 16:26:03.333795-0800 Expensify.cash[715:123724] [D] -[UAAutomationEngine updateTriggersWithScheduleID:type:argument:incrementAmount:] [Line 611] Updating triggers with type: 8
2020-12-21 16:26:03.352001-0800 Expensify.cash[715:123724] [D] -[UAInboxMessageList refreshInboxWithCompletionHandler:]_block_invoke_2 [Line 354] Inbox messages updated.
2020-12-21 16:26:03.352077-0800 Expensify.cash[715:123724] [D] -[UAAnalytics addEvent:]_block_invoke [Line 227] Adding app_init event 36D420DE-3273-4B30-84B9-91AC3386BD4D.
2020-12-21 16:26:03.352135-0800 Expensify.cash[715:123724] [D] -[UAAnalytics addEvent:]_block_invoke [Line 227] Adding app_foreground event F13D0533-4A96-480C-B086-F09ED571991D.
2020-12-21 16:26:03.355071-0800 Expensify.cash[715:123724] [I] +[UAAppIntegration application:didRegisterForRemoteNotificationsWithDeviceToken:] [Line 35] Application registered device token: daab81662a7b8b9004742804453701144dbc29cac4c87cb04aca36bc614936b9
2020-12-21 16:26:03.355195-0800 Expensify.cash[715:123724] Device token: daab81662a7b8b9004742804453701144dbc29cac4c87cb04aca36bc614936b9
2020-12-21 16:26:03.365512-0800 Expensify.cash[715:123724] [D] -[UAPush updateRegistration] [Line 617] APNS registration is out of date, updating.
2020-12-21 16:26:03.368948-0800 Expensify.cash[715:123724] [D] -[UAAnalytics addEvent:]_block_invoke [Line 227] Adding device_registration event 92BCC1FB-9E32-42F4-8A77-F718B76D25A3.
2020-12-21 16:26:03.369100-0800 Expensify.cash[715:123724] [D] -[UAChannelRegistrar registerForcefully:]_block_invoke_2 [Line 143] Ignoring registration request, registration is up to date.
2020-12-21 16:26:03.419848-0800 Expensify.cash[715:123724] [D] -[UAChannelRegistrar registerForcefully:]_block_invoke_2 [Line 143] Ignoring registration request, registration is up to date.
2020-12-21 16:26:03.528976-0800 Expensify.cash[715:123965] [D] -[UAInboxMessageList retrieveMessageListWithSuccessBlock:withFailureBlock:]_block_invoke [Line 187] Retrieve message list succeeded with status: 200
2020-12-21 16:26:03.538244-0800 Expensify.cash[715:123724] [D] -[UAInboxMessageList refreshInboxWithCompletionHandler:]_block_invoke_2 [Line 354] Inbox messages updated.

Then after adding this code:

UrbanAirship.setForegroundPresentationOptions(_.mapObject(_.invert(NotificationOptionsIOS), () => false));

This log shows up:

2020-12-21 16:29:17.873754-0800 Expensify.cash[715:123724] [D] -[UrbanAirshipReactModule setForegroundPresentationOptions:] [Line 301] Foreground presentation options set: 0 from dictionary: {
    alert = 0;
    badge = 0;
    sound = 0;
}

Then after receiving a notification with the app in the foreground:

2020-12-21 16:30:24.097740-0800 Expensify.cash[715:123724] [D] +[UAAppIntegration userNotificationCenter:willPresentNotification:withCompletionHandler:] [Line 80] Notification center will present notification: <UNNotification: 0x280b6fc90; source: com.chat.expensify.chat date: 2020-12-22 00:30:24 +0000, request: <UNNotificationRequest: 0x280b6f2a0; identifier: BD71F9EE-46DE-43AA-9990-1B81B4CF4C8C, content: <UNNotificationContent: 0x283e2c270; title: (null), subtitle: (null), body: <redacted>, summaryArgument: , summaryArgumentCount: 0, categoryIdentifier: , launchImageName: , threadIdentifier: , attachments: (
), badge: (null), sound: <UNNotificationSound: 0x282e2d180>, realert: 0, trigger: <UNPushNotificationTrigger: 0x2807107e0; contentAvailable: NO, mutableContent: NO>>, intents: (
)>
2020-12-21 16:30:24.098466-0800 Expensify.cash[715:123724] [I] +[UAAppIntegration handleIncomingNotification:foregroundPresentation:completionHandler:] [Line 191] Received notification: {
    "_" = "490f284f-a0e3-4398-aa4c-37bd6419cf7c";
    aps =     {
        alert = "New message from [email protected]: \"yoyoyoyoyo\"";
        sound = default;
    };
    "com.urbanairship.metadata" = "eyJ2ZXJzaW9uX2lkIjoxLCJ0aW1lIjoxNjA4NTk3MDIzODUwLCJwdXNoX2lkIjoiNmRjZWM5NDctNTU3NS00ODhiLWFkMmMtNWNlMzcyNDdjMDg3In0=";
    fromExpensify = 1;
    payload =     {
        reportAction =         {
            actionName = ADDCOMMENT;
            actorAccountID = 7;
            actorEmail = "[email protected]";
            automatic = 0;
            avatar = "https://d2g02b6ed2w9fz.cloudfront.net/4e94aeba210608da1eb3eadcc1ea7a038e27b640_128.jpeg";
            clientGUID = "";
            created = "Dec 21 2020 4:30pm PST";
            message =             (
                                {
                    html = yoyoyoyoyo;
                    text = yoyoyoyoyo;
                    type = COMMENT;
                }
            );
            person =             (
                                {
                    style = strong;
                    text = "[email protected]";
                    type = TEXT;
                }
            );
            sequenceNumber = 11;
            shouldShow = 1;
            timestamp = 1608597023;
        };
        reportID = 282;
        shouldScrollToLastUnread = 1;
        type = reportComment;
    };
}

And the notification is still presented in the foreground. As for a sample application, we've actually just open-sourced the application that manifests this bug, and the repo for that is here. Then the relevant code all lives in here.

@roryabraham
Copy link
Author

We're working on streamlining things, but for now, in order to reproduce in our application, you'll have to:

  1. Clone the repo and run the web app per the instructions in the README
  2. Sign up with two test accounts
  3. Email us at [email protected] with the subject line Slack Channel Invite. We'll then add you to our slack channel for external contributors. Once you're in that channel, it will be the best place to ask questions about the local development environment.
  4. Ping us in the #expensify-contributors slack channel to add your test accounts to a policy that enables chatting.
  5. Then, run the application on a physical iPhone device. Usually it works for me if I just:
    1. npm i && cd ios && pod install
    2. Open the ios folder in xcode 12
    3. Click on the ExpensifyCash target, then Signing and Capabilities, then Automatically Manage Signing
    4. Click the play button to run the app.
  6. Run the web application
  7. On iOS, sign into one of your test accounts and keep the app open in the foreground
  8. On web, sign into the other test account
  9. Send a message from web -> iOS, and the notification will appear in the foreground.

As I said, we're actively working to make this process a bit easier for external contributors, but in the meantime if you have questions then the #expensify-contributors slack channel would be the quickest place to get a response.

@rlepinski
Copy link
Contributor

Looks like you have another push provider plugin that instructed you to implement the userNotificationCenter:willPresentNotification:withCompletionHandler: method. Our SDK right now just merges all the options together defaultOptions | push delegate options | app delegate options. You can safely remove that method or I can give you code to check if its an airship notification or not if you want that behavior for other notifications.

@roryabraham
Copy link
Author

Looks like you have another push provider plugin that instructed you to implement the userNotificationCenter:willPresentNotification:withCompletionHandler:

Could you point me to the log line that that indicated that to you? We do also use pusher-js

@rlepinski
Copy link
Contributor

rlepinski commented Dec 22, 2020

The log lines all looked fine to me, so I looked at your app. Ill check out pusher-js but https://github.com/Expensify/Expensify.cash/blob/master/ios/ExpensifyCash/AppDelegate.m#L64 is going to be the reason why you get alert, badge, and sound in foreground.

@rlepinski
Copy link
Contributor

pusher-js by itself does not appear to do either remote or local notifications on its own so I think you are good.

@roryabraham
Copy link
Author

roryabraham commented Dec 22, 2020

Okay, you figured that out impressively quickly! You were right - we use @react-native-community/push-notification-ios to handle the home-screen unread indicator for chats on iOS. I think that this means we can't safely remove this code without breaking the "unread chat indicator". So can we take you up on your offer to "check if it's an airship notification or not"?

@rlepinski
Copy link
Contributor

Could you point me to the code that uses @react-native-community/push-notification-ios while I put together code for airship message checking?

@roryabraham
Copy link
Author

Could you point me to the code that uses @react-native-community/push-notification-ios

Yep, super minimal use right here

@rlepinski
Copy link
Contributor

rlepinski commented Dec 22, 2020

That just sets the badge number, it does not rely on any notifications. We actually have that as well for our module if you want to drop a dependency. You can delete that method in the app delegate. I forgot that detecting an Airship notification on iOS is kinda painful, ideally we dont have to do that...

@roryabraham
Copy link
Author

We actually have that as well for our module

I was just wondering that! Hold on, I'll try this and see if it resolves the issue. That would be ideal for both of us 😄

@rlepinski
Copy link
Contributor

If you need to check, I believe this works:

if (notification.request.content.userInfo[@"com.urbanairship.metadata"]) {
        completionHandler(0);
    } else {
        completionHandler(UNAuthorizationOptionSound | UNAuthorizationOptionAlert | UNAuthorizationOptionBadge);
    }

but I havent tested it. I can in the AM if you need it.

@roryabraham
Copy link
Author

Thanks for the support! This issue has been resolved!

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

No branches or pull requests

3 participants