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

Push Notification not received if app is backgrounded #3542

Closed
Jag96 opened this issue Jun 11, 2021 · 20 comments
Closed

Push Notification not received if app is backgrounded #3542

Jag96 opened this issue Jun 11, 2021 · 20 comments
Assignees

Comments

@Jag96
Copy link
Contributor

Jag96 commented Jun 11, 2021

Likely caused by #3508 (QA comment was here). I tried changing the urbanairship-react-native version to 10.2 (was originally at 10.0) but doing so causes #3503 to happen again. Not sure what the issue is here, ideally somebody who can build to a physical Android device to test would look at this one.

Expected Result:

User receives push notification

Actual Result:

No push notification received

Actions Performed:

  1. Launch the app and login
  2. Background the app.
  3. From another account, send a message to this account

Platform:

Android ✔️
iOS ✔️

Build:

1.0.67-0

Notes/Images/Video:

Bug5107452_screen-20210610-194541.mp4
@Jag96 Jag96 added DeployBlockerCash This issue or pull request should block deployment AutoAssignerTriage Auto assign issues for triage to an available triage team member labels Jun 11, 2021
@MelvinBot
Copy link

Triggered auto assignment to @strepanier03 (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 11, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@MelvinBot
Copy link

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

@MariaHCD
Copy link
Contributor

@sketchydroide and @Julesssss would be better equipped to tackle this one quickly 🙌🏼

@MariaHCD MariaHCD removed their assignment Jun 11, 2021
@sketchydroide
Copy link
Contributor

I can take a look, but my availability today is a bit sketchy, so I might need to push it to someone else later if I don't figure it out quickly.

@isagoico
Copy link

This is also reproducible in iOS app. Editing title.

@isagoico isagoico changed the title Android - Push Notification not received if app is backgrounded Push Notification not received if app is backgrounded Jun 11, 2021
@Julesssss
Copy link
Contributor

Julesssss commented Jun 11, 2021

The UA React Native module 11.0.0 release contains breaking changes apparently. Surely that's what we're seeing here?

Major release updating the iOS and Android SDKs to 14.3.0. This release contains small breaking changes to the event handling API

@sketchydroide
Copy link
Contributor

Thanks @isagoico that makes more sense.
No idea what breaking changes they mean, they are incredibly vague on this. I assume it's the fact that the pusher events are different, and will now trigger while the app is in the background, I think it's returning early somewhere here

@sketchydroide
Copy link
Contributor

@roryabraham do you have any ideas about this?

@roryabraham
Copy link
Contributor

There is some extra context about the breaking changes here ... they were made at our request, so that push notifications can wake a closed Android app and execute JS.

@sketchydroide
Copy link
Contributor

@roryabraham so do you think that is responsible for this issue Push Notification not received if app is backgrounded I feel that they might now be handled in a different way (or all are handled) and for that reason not shown, is there a specific type of notification that we want to handle, if so can we filter it out, and show all the others?

@strepanier03
Copy link
Contributor

strepanier03 commented Jun 11, 2021

Not sure if it's still needed but triaging this now.

Unique: Not finding any other reports of this.
Issue hygiene: Proper template used and filled out well from what I can tell, nice job.
Clarity of problem: Problem seems clear,
Value: Echat is high value and prompt push notifications are expected of a high quality chat app.


I cannot test this in the app to recreate, I do not have E-chat on my phone at the moment because I was struggling with working late hours when I would get chats after I'd logged for the day. This is already with Eng so removing my assignment and leaving it with the current labels.

@strepanier03 strepanier03 removed their assignment Jun 11, 2021
@sketchydroide
Copy link
Contributor

between trying to reproduce it myself (and failing) and trying to understand what was broken, I haven't been able to do much about this, if someone else has any ideas please feel to take it, otherwise I'll get back at this on Monday.

@sketchydroide
Copy link
Contributor

I haven't really been able to figure much out about this, I'm not being able to test it on Android (it fails now installing/building) I think I need some help/new eyes

@Julesssss
Copy link
Contributor

I'll be able to take a look tomorrow. I also wonder if @roryabraham might be able to provide some insight too?

@Julesssss Julesssss self-assigned this Jun 15, 2021
@Julesssss
Copy link
Contributor

I built the prod env to test this. Can confirm that notifications aren't displayed when the app is in the background.

The first issue is that the app thinks we are still viewing the last read chat:
Screenshot 2021-06-15 at 15 08 57

If we navigate to an alternate chat, background the app, and send the message again we see a different message, but the notification still fails to display:
Screenshot 2021-06-15 at 15 11 34

I'm going to try reverting the Airship library update to compare results. @Jag96 reported that this issue occurs, but the solution to that might be simpler than the Urban Airship fix.

@Julesssss
Copy link
Contributor

Julesssss commented Jun 15, 2021

Reverting UrbanAirship to 10.0.0 does not fix this issue for me. I think the first issue already existed, and that the second issue is the one that needs to be solved:

Logs show that a notification is going to be created, but this is never displayed

@Julesssss
Copy link
Contributor

Julesssss commented Jun 15, 2021

In fact, I'm now wondering if local notifications ever worked on mobile? Because the native function is noop...

Yep, they were never implemented! This PR seems to be introducing them, but is WIP.

@Julesssss Julesssss added Weekly KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jun 15, 2021
@Julesssss
Copy link
Contributor

Julesssss commented Jun 15, 2021

Yep, they were never implemented!

I am now convinced that this is the case. I even took part in the discussion about how to introduce background notifications discussion (notice the 'When are we displaying notifications now?' table)🤦 .

To confirm that this has never worked, I built an old prod signed build 1.0.19-6 directly from the Play Store. Notice that messages are received while the app is open and killed, but not when backgrounded. Finally, once I open the app, the message is updated to 15 -- none of the 6-15 messages were displayed as the app was backgrounded at the time.

If we were creating the notification from background, it would should displayed instantly (unlike server notifications). This comment proves that we are logging the request to display a background notification, and this link proves that the function which is responsible for creating the notification is empty.

android-background-notifications.mp4

I removed DeployBlocker label and assigned @roryabraham as it seems to be associated with this issue that he is currently working on. Feel free to close this.

@roryabraham
Copy link
Contributor

I can confirm that local notifications never worked on mobile, but it seems that pusher connections are being maintained while the app is backgrounded. I'm not sure if something specific changed here (because this wasn't always the case when I was first implementing push notifications on E.cash), or if it's just something that works inconsistently. In any event, I can hopefully address this by making local notifications work when the app is backgrounded, so that we display a notification regardless of whether we are connected via pusher while the app is backgrounded.

It's also worth noting that if you have a web or desktop client open and logged into the same account, we won't send a push notification because that other client will have an active pusher connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants