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

Improve report comment push notification background updates #4008

Closed
roryabraham opened this issue Jul 13, 2021 · 11 comments
Closed

Improve report comment push notification background updates #4008

roryabraham opened this issue Jul 13, 2021 · 11 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

Both iOS and Android impose limitations on how frequently push notifications can wake the app from the background, and on how long those apps can remain awake in the background. Some variables that help determine whether this happens are:

  • How much battery life the device has
  • Whether the device is on low-power mode
  • The availability of cellular data
  • How frequently the user uses the app

iOS is far more strict, warning that developers "shouldn't count on more than 2-3 background updates per hour". While we likely can't do anything to circumvent these OS limitations, we can make the most of the background time we're given.

Action Performed:

With the app backgrounded or closed, receive a report comment push notification that wakes the app for a background update.

Expected Result:

  1. Look at the sequence number of the ReportAction included in the payload of the push notification.
  2. If it is equal to the highest sequence number we had previously in Onyx plus one, just add the single report action to Onyx.
  3. Otherwise, fetch all the report actions between the highest sequence number we had previously in Onyx and the one included in the payload of the push notification. Thus ensuring that any messages we missed on that report will be added to Onyx.

Alternate Solution

Always fetch the report actions for the given report. Use pagination and fetch the newest ones first, until we receive a response that contains a report action with the highest sequence number we previously had in Onyx. This might be simpler, but also might be "riskier", because I'm not sure that the OS will want to let you make network requests when woken from the background. So we might only want to fall back on that behavior if necessary.

Actual Result:

The one ReportAction included in the payload of the push notification is added to the report in Onyx.

Workaround:

n/a

Platform:

iOS
Android

Version Number: 1.0.77-2

View all open jobs on Upwork

@roryabraham roryabraham added Engineering Weekly KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff labels Jul 13, 2021
@Julesssss Julesssss self-assigned this Jul 15, 2021
@Julesssss
Copy link
Contributor

Hey @roryabraham, happy to pick this one up. Do you think it depends on your current E.cash PR being merged?

@roryabraham
Copy link
Contributor Author

Not necessarily! I think this can be worked on independently. The callback in question is right here. The current E.cash PR moves that stuff to it's own function, subscribeToReportCommentPushNotifications, but otherwise doesn't change the content of the callbacks.

@Julesssss
Copy link
Contributor

Started on this today. I've been following the config steps listed in this issue to build a release build that can receive notifications.

The release build is taking 10 minutes 😩 but at least the setup steps are completed for more progress tomorrow. I'm also seeing an infinite spinner on login, which seems to be occurring on the latest main commit: Slack Thread

@Julesssss
Copy link
Contributor

Okay, after some further issues with Ngrok and finding a workaround to the infinite spinner, I was able to make progress.

Setup Process

  • Follow 'Get a prod build running' steps
  • Disable these lines in Web-E
  • Add lintOptions {checkReleaseBuilds false } to the app/build.gradle file
  • Add android:debuggable="true"to ...src/main/AndroidManifest.xml

Logs for reference

App in foreground
Screenshot 2021-07-22 at 17 36 36

App in background
Screenshot 2021-07-22 at 17 37 19

App killed (Headless process)
Screenshot 2021-07-22 at 17 38 36

App killed (blocked from background start)
Screenshot 2021-07-22 at 17 40 15

@Julesssss
Copy link
Contributor

I'm starting to wonder if we should change our plan here. It seems to me that the goal of always having Onyx up to date is unrealistic in practice, as the OS's do not want us to do this and actively work against us if we don't follow the recommended practices:

1) We don't seem to drop the missing notifications. Android OS blocks notifications after a while, but once the cooldown period ends and a new notification is received, all of the missing notifications appear together. I missed 4 notifications, then 10 minutes later sent a 5th and received all 5 at once. If the headless process is working for all of these, then a solution may not be required.

2) If it is possible to prevent the notification cooldown, by not updating Onyx, would that not be preferred? The arguments for this are A) We could receive message notifications sooner, as the cooldown period won't be applied, B) We already have logic for retrieving messages at app open -- could efficiencies not be made with the API requests instead, & C) while updating Onyx in the background can provide a better user experience, it is unlikely to be reliable.

3) iOS seems more restrictive than Android. The trouble we go through to implement this might be in vain if similar workarounds aren't applicable for iOS.

4) This is going against the OS platform best practices. For Android specifically, we are supposed to implement JobScheduler in order to do work in the background (query for new messages, persist to Onyx) (even more restrictions apply to killed or idle apps!). This wouldn't be so bad. Every X minutes we would get all the latest messages and if the user opens the app prior to this then they simply need to retrieve the data before displaying it. Another way is to have a persistent foreground notification, which allows us to continue background services (I believe this is how WhatsApp is operating). OS restrictions are only going to get more severe in future updates.

@Julesssss
Copy link
Contributor

Feel free to ignore the above ^

I was collecting some thoughts, which I'll revise and share in the relevant places.

@Julesssss
Copy link
Contributor

Okay, here's today's progress/thoughts:

I'm still working on the assumption that we need to retrieve missing data, but it seems to me that each 'missing' notification will eventually be unblocked by the OS, which will let through all the notifications that were queued at once. Perhaps this is why Onyx isn't being updated correctly. If 5 notifications come through and start headless background tasks, could the error lie here instead?

Also, I haven't yet been able to prove this but I believe the reason for the dropped notifications is the fact that we run network tasks for a killed app against the OS guidelines. I think we might be able to avoid being dropped in the first place by following platform guidelines and scheduling background services for fetching new messages and reports. It is possible to run any code in service unrestricted as long as it is scheduled correctly and we display a foreground notification to the user ('Checking for messages').

Screenshot 2021-07-26 at 16 12 58

WhatsApp, Messenger, Signal, Viber, and most likely many other messaging apps do this. This might require further development from Urban Airship, but this could solve also the problem of dropped notifications and missing Onyx data.

@roryabraham
Copy link
Contributor Author

roryabraham commented Jul 26, 2021

Okay, so it sounds like you're suggesting we implement something like react-native-background-fetch to reliably update Onyx in the OS's preferred method. I think it's a good idea, but do you think that we should do this instead of or in addition to background updates via push notifications? I think it would be great to do this in addition to the background updates via push notification that we've already built out.

This might require further development from Urban Airship

Since scheduled background updates are largely unrelated to push notifications, I'm not sure this will require any further development from UA. I think we can and probably should use another library for this.

this could solve also the problem of dropped notifications and missing Onyx data

Hmmm are you saying that some notifications have been dropped altogether? I'm not sure I observed that – only that Onyx isn't consistently updated when a notification is received.

It is possible to run any code in service unrestricted as long as it is scheduled correctly and we display a foreground notification to the user

Can you provide a link to the documentation that states that a foreground notification is displayed to the user? NDB because we can probably generate that locally using react-native-push-notification, but would be good to know for sure that this is necessary.

@quinthar
Copy link
Contributor

quinthar commented Jul 26, 2021 via email

@Julesssss
Copy link
Contributor

Yeah, I think this should be in addition to the notification background updates (but we probably wouldn't need to implement the expected result listed in this issue description).

Hmmm are you saying that some notifications have been dropped altogether? I'm not sure I observed that

Nope, what I meant to say is that if we use periodic checks instead of making additional API calls in a background service, then I think the OS would throttle our notifications less often. This remains to be seen though.

Can you provide a link to the documentation that states that a foreground notification is displayed to the user

Sure, here. Here is the change introduced in Android 12, which mentions that if less than 10 seconds, the notification won't be displayed.

Background Service Limitations: While an app is idle, there are limits to its use of background services. This does not apply to foreground services, which are more noticeable to the user.

It's worth mentioning that this is not just any notification, but a special notification that is tied to the background service. According to this blog post, not all of the React Native libraries support this, so we'll need to check carefully if we believe that following these rules will lead to less throttling of notifications.

Finally, here is the main point. This limitation will apply once we target Android 12 (we will be forced to within 12 months):

Apps that target Android 12 (API level 31) can no longer start foreground services while running in the background, except for a few special cases.

The special cases that we will depend on are:

  • Your app receives a high-priority message using Firebase Cloud Messaging
  • Your app transitions from a user-visible state, such as an activity (or the foreground notification)

@Julesssss
Copy link
Contributor

Alright, looks like we should close this issue and move further discussion to 'Implement periodic background updates on mobile platforms'. Thanks for opening the issue, Rory!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

3 participants