iOS - Push notifications are not opening correct chat

isagoico commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Have some notifications from New Expensify conversations in iOS
Tap any of the notification
Expected Result:

User should be taken to correct chat.

Actual Result:

Some of the time, the user is not taken to correct chat.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Get taken to the wrong chat and then go click the correct one.

Platform:

Where is this issue occurring?

iOS

Version Number: 1.1.10-0

Reproducible in staging?: Yes Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @roryabraham Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1635210815095800

View all open jobs on GitHub

MelvinBot commented 3 years ago

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

MelvinBot commented 3 years ago

Triggered auto assignment to @adelekennedy (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

adelekennedy commented 3 years ago

Created the job in Upwork: Internal External

adelekennedy commented 3 years ago

@Luke9389 whoops, I'm a couple days late on this but no bites so far. Going to double the job

roryabraham commented 3 years ago

Whoops, this has to be internal. External contributors have no way of testing push notifications locally.

roryabraham commented 3 years ago

@adelekennedy please take down the Upwork job

adelekennedy commented 3 years ago

@roryabraham thank you! Is this something I could have spotted and missed? (I want to make sure I'm following procedure)

roryabraham commented 3 years ago

@adelekennedy I think you followed procedure just fine. Push notifications just happen to be one of those things that we've found external contributors can't work on.

@Luke9389 the reason is because iOS push notifications can't be tested on a simulator, and you can't build the mobile app to a physical device unless you add the device to our mobile provisioning profile in the App Store.

Luke9389 commented 3 years ago

Dude, nice catch. Sorry about that! Will make note of this for the future. Actually... do you know if we have a list of topics that need to be internal? Maybe it's not necessary, idk.

roryabraham commented 3 years ago

do you know if we have a list of topics that need to be internal?

I am not aware of such a list

Luke9389 commented 3 years ago

Yea I searched SO and didn't see one. Do you think it's necessary? What else would go on there aside from notifications? "Anything requiring Web-E changes". Seems like this may not be needed...

adelekennedy commented 3 years ago

The jobs been closed 💀

Luke9389 commented 3 years ago

Now that this is internal, I'm gonna unassign from this.

MelvinBot commented 2 years ago

This issue has not been updated in over 15 days. eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

neil-marcellini commented 2 years ago

Yesterday I tested opening notifications when the app is in the background on a physical IOS device. For some reason the call to Linking.openURL does not work. Opening the deep link to report 185 on the simulator with xcrun simctl openurl booted new-expensify://r/185 also did not work. I think there may be an issue with the structure of the linking config.

However, I did find that replacing Linking.openURL with Navigation.navigate(ROUTES.getReportRoute(reportID)); ended up working, at least when the app is only in the background. I'm going to test that solution on a deployment build on my IOS device so that I can see if opening the notification works when the app has not been started.

roryabraham commented 2 years ago

@neil-marcellini Thanks for working through this, I know firsthand how challenging it can be to test.

Opening the deep link to report 185 on the simulator with xcrun simctl openurl booted new-expensify://r/185 also did not work.

This is weird ... this is probably the key to solving this issue. Have you tried running xcrun simctl openurl ... when running the app in a simulator instead of on a physical device? Is it possible that your xcrun command is not properly targeting your physical device?

However, I did find that replacing Linking.openURL with Navigation.navigate(ROUTES.getReportRoute(reportID)); ended up working, at least when the app is only in the background. I'm going to test that solution on a deployment build on my IOS device so that I can see if opening the notification works when the app has not been started.

Honestly, my recommendation would be to not go down this route until you figure out why deeplinking isn't working, because I know from experience that Navigation.navigate almost certainly won't work if you're waking the app from a push notification when it's completely closed.

Of course, you can choose to approach it differently and it's probably valuable for you do get practice running a deployment build on your device. It's up to you and I'm here if you need any support.

neil-marcellini commented 2 years ago

Have you tried running xcrun simctl openurl ... when running the app in a simulator instead of on a physical device?

Yes that's what I did. As with tapping on a notification, it opens the list of reports but not the report itself. Maybe the problem is that the drawer is open and is covering the page beneath? I'll see if I can verify that.

neil-marcellini commented 2 years ago

@roryabraham I tried editing Navigation.getDefaultDrawerState to always return 'closed' to see if the drawer was covering the report when it is deep linked to. Once I made that change deep linking to a report worked. Why is the default drawer state set to 'open' on small screens? Could we change the default? Or could we change the default drawer state when handling the deep link?

roryabraham commented 2 years ago

@neil-marcellini Great investigation. We already have a check in place here to see if we tapped a notification before the navigation container was ready, and if we did we set the default drawer state to closed. If seems that's just not working in the case when the app is already open because Navigation.setDidTapNotification does nothing if the navigation container is already ready.

I suggest we change this callback like so:

// Open correct report when push notification is clicked
PushNotification.onSelected(PushNotification.TYPE.REPORT_COMMENT, ({reportID}) => {
+   if (Navigation.isReady()) {
+       Navigation.navigate(ROUTES.getReportRoute(reportID))
+   } else {
+       // Navigation container is not yet ready, use deeplinking to open to correct report instead
         Navigation.setDidTapNotification();
         Linking.openURL(`${CONST.DEEPLINK_BASE_URL}${ROUTES.getReportRoute(reportID)}`);
+   }
});

That way, we use Navigation.navigate as the default, which will work if the app was just backgrounded. But we use deeplinking if we need to (i.e: app was just killed)

neil-marcellini commented 2 years ago

Ok, that sounds like a good plan. I'll implement this and test it out. My only concern is what if we want to handle deep links from sources other than notifications while the app is open? For example maybe we have a link to a specific report in an email? In that case it feels like we should have the logic you specified above in a callback that handles all incoming deep links. What do you think? Maybe that's a project for the future?

roryabraham commented 2 years ago

In that case it feels like we should have the logic you specified above in a callback that handles all incoming deep links.

That's a good point – it might be nice to have a more inherent solution. It could also be a pretty tricky as I don't think there's a straightforward way to handle this. I've done some research and here's a rough sketch of what I've come up with. It needs to be worked through and tested and I'm not sure it will work:

Override getStateFromPath in the linkingConfig as described here, and set up the following logic:

import {getStateFromPath} from '@react-navigation/native';

const OVERRIDE_DRAWER_STATE_ROUTES = [
    {
        regex: /report\/\d\/?/,
        drawerState: 'closed',
    },
];

let currentDeeplinkURL = null;
Linking.addEventListener('url', ({url}) => currentDeeplinkURL = url);

...
export default {
    prefixes: [...],
    config: {...},
    getStateFromPath: (path, config) => {
        if (Linking.getInitialURL() === path || currentDeeplinkURL === path) {
            if (currentDeeplinkURL) {
                // Reset deeplink URL
                currentDeeplinkURL = null;
            }

            for (let override of OVERRIDE_DRAWER_STATE_ROUTES) {
                  if (override.regex.test(path)) {
                      Navigation.setDrawerState(override.drawerState);
                      break;
                  }
            }
        }

        return getStateFromPath(path, config);
    }
}

In Navigation.js, implement the following:

let initialDrawerState = null;
...
function setDrawerState(state) {
    if (!_.contains(['open', 'closed'], state)) {
        Log.hmmm(`Attempting to set invalid drawer state ${state}`);
        return;
    }

    if (navigationRef.isReady()) {
        if (state === 'open') {
            openDrawer();
        } else {
            closeDrawer();
        }
    } else {
        initialDrawerState = state;
    }
}
...

function getDefaultDrawerState() {
    if (!_.isNull(initialDrawerState)) {
        return initialDrawerState;
    }

    return isSmallScreenWidth ? 'open' : 'closed';
}

And that might work – if you deeplink to a specific report, we'll:

Notice that you used a deeplink
Match the path against the a set of explicit overrides for deeplinks (i.e: since you navigated to a specific report, we'll set the drawer state to closed).
If the navigation container is ready, we'll just navigate to the report and close the drawer. If it's not yet ready, we'll save a default drawer state to be used when the container mounts.

It's definitely a weird implementation, and I'm curious for input from others.

roryabraham commented 2 years ago

FWIW a better implementation would be to add a feature to react-navigation such that the linking config has two new fields:

willHandleDeeplink – pass this function and it will be called with the url of the deeplink before any navigation occurs.
didHandleDeeplink – pass this function and it will be called with the url of the deeplink after the navigation occurs.

That could help resolve some potential race conditions with the above implementation. For example, this might be possible with the proposal I wrote above (not sure):

With the app in the background, click on a deep link.
We notice that you set a deep link, and set the drawer state to closed, and dispatch the closeDrawer action.
Then react-navigation processes the deeplink and navigates to the report, but the defaultDrawerState has been reset.

roryabraham commented 2 years ago

Anyways, it would be great to get this nailed down to make deep-link handling easier in the future, but if things aren't working out we could always just fix the problem at hand with this simple solution for now. cc @parasharrajat if you have any opinions on this discussion.

roryabraham commented 2 years ago

Oh, actually @parasharrajat wrestled with this problem a while back and we decided that his proposal was too complicated to be worth it: https://github.com/Expensify/App/issues/5027#issuecomment-951417341. If we really want to get this deep-link drawer state settled we might need a react-navigation feature request of some kind, maybe the willHandleDeeplink event handler I suggested above. 🤔

parasharrajat commented 2 years ago

Hmm, https://github.com/Expensify/App/issues/6079#issuecomment-1026367170 This might not work for all platforms. The problem I was trying to solve was a little different but covers this issue as well. URL for home page (LHN) and report page is the same which creates a challenge for us. How to decide what should be shown to the user? LHN or report?

Drawer usage history state to manage open and closed state and when you open the app directly from URL, there is no History API state. We need a way to create a proper/complete navigation state via URL.

What I tried was to add a param and change its value whenever the Drawer status changes. I tried LinkingConfig but it is limited and does give us full control. It is mostly there for you to create the initial state from the URL but when you interact with the app it does not have any effect on the state (I don't remember exactly if this was the case but there was a limitation). So to get full control over the navigation state and url generation, I customized DrawerRouter.

This could be a good feature in DrawerRouter if we can manage the Drawer state from url. Even if there is a way to manage it with configuration, I haven't found one.

roryabraham commented 2 years ago

Maybe we should gather our thoughts a bit and create a feature request in react-navigation?

roryabraham commented 2 years ago

In the meantime @neil-marcellini I would recommend taking an easier route to fix this issue and going with this simpler suggestion.

neil-marcellini commented 2 years ago

@roryabraham I'm still having trouble installing a release build on my IOS device. I also tried following these instructions but I wasn't sure if that actually installs a production build.

neil-marcellini commented 2 years ago

This issue is still occurring, and now it occurs on all platforms, so I'm not sure if #7633 fixed the problem for IOS. See the slack conversation here

roryabraham commented 2 years ago

Reverted https://github.com/Expensify/App/pull/7633

neil-marcellini commented 2 years ago

I'm going to drop this one so that someone with more react navigation experience can take it. Also, for whoever takes it next, it would be great to have a way to locally test notifications opened when the app is in the background.