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

Add Airship Notifications for Mobile Clients #288

Closed
marcaaron opened this issue Aug 24, 2020 · 14 comments
Closed

Add Airship Notifications for Mobile Clients #288

marcaaron opened this issue Aug 24, 2020 · 14 comments
Assignees

Comments

@marcaaron
Copy link
Contributor

Similar to #285

But for getting Airship device notifications to work on native devices.

@quinthar
Copy link
Contributor

To clarify, this is both to deliver notifications that users should see and tap on, but also to just push out background updates whenever there are new messages (so they are all stored locally and ready to go), right?

@quinthar
Copy link
Contributor

Or are we just counting on re-synchronizing all the reports when you open up the app and reconnect?

@marcaaron
Copy link
Contributor Author

but also to just push out background updates whenever there are new messages (so they are all stored locally and ready to go), right?

I'm less certain if Airship lets us add data to a React Native app like that. But we can look into it.

@marcaaron
Copy link
Contributor Author

There seem to be ways to background fetch data so maybe it ends up being a combination of two libs if we can't do it with Airship alone. Uncharted waters for me! Maybe @roryabraham has some ideas?

https://github.com/transistorsoft/react-native-background-fetch

@roryabraham
Copy link
Contributor

roryabraham commented Aug 25, 2020

To clarify, this is both to deliver notifications that users should see and tap on, but also to just push out background updates whenever there are new messages (so they are all stored locally and ready to go), right?

I think Airship alone would probably be sufficient. From the Airship docs:

The Airship module makes a distinction between “user notifications,” which can be seen by the user, and invisible notifications that carry only data for the app to process.

There is an event that is triggered when a push-notification is received, so if we wanted to silently update the app's internal data without notifying the user, we could send an "invisible notification", and setup a callback for the PushReceived event which updates the Ion data while the app is backgrounded. We could do the same thing when visible push-notifications are received, if we wanted to. The only thing I'm unsure about is what type of data can be passed with a push-notification (i.e: if could send a report JSON with an invisible push-notification)

@roryabraham roryabraham self-assigned this Aug 25, 2020
@marcaaron
Copy link
Contributor Author

The only thing I'm unsure about is what type of data can be passed with a push-notification (i.e: if could send a report JSON with an invisible push-notification)

Yes! There are limits to the size of the payload. We currently unset the report action when its too large. However, we still send the reportID so we know that the app needs to fetch new info from the server.

https://github.com/Expensify/Web-Expensify/blob/2303d2adc2ccc4f16efbc14ae7557f4e49668458/lib/MobilePushNotifications.php#L30-L35

So one idea would be to send our notification with a simple instruction like reportID rather than the entire data payload we need. Then the app could then handle the various notification types and background fetch to get the updates from the server.

@AndrewGable
Copy link
Contributor

@roryabraham - How are things going with this one?

@roryabraham
Copy link
Contributor

@AndrewGable Working on it now

@AndrewGable
Copy link
Contributor

Ok awesome, if you need any API keys and/or configuration help in UrbanAirship I am a ping away!

@roryabraham
Copy link
Contributor

So far, I've added the airship library, and followed the setup instructions for iOS and Android. I just took the API keys from Mobile, so we should be good there.

So now, I'm at the point where I'll actually start writing code, and this is my plan:

For background info, check out this PR on Browser (web/desktop) notifications, but TL;DR Currently, there is some desktop-specific notification code in src/lib/Notification/BrowserNotifications.js. This is used by src/lib/Notification/index.js. Currently, we just have a noop for src/lib/Notification/index.native.js.

Instead, I'll port the code from BrowserNotifications.js to src/lib/Notification/index.js, and implement the Airship notifications in src/lib/Notification/index.native.js, (which is currently just a noop). Both files will export the same set of functions - currently just showCommentNotification. If/when necessary, we will just noop the functions that are relevant to one platform and not the other.

@quinthar
Copy link
Contributor

quinthar commented Sep 11, 2020

To clarify, is the goal to do as @marcaaron suggests here:

So one idea would be to send our notification with a simple instruction like reportID rather than the entire data payload we need. Then the app could then handle the various notification types and background fetch to get the updates from the server.

If so, can you elaborate more precisely on what happens? A couple questions:

  1. Do we push the silent notification to all mobile apps via Airship, whether or not they are currently connected via Pusher? If not, how do we target the notification to ignore mobile apps already connected via Pusher?

  2. When the app is woken up in the background, does it use a different startup sequence than when it's brought to the foreground? I imagine when opening the app for real, we create a bunch of UI elements, then subscribe those elements to Ion, etc. But if we are opening it up in the background, do we detect that somehow and instead use a different sequent that just silently updates Ion and then goes back to sleep?

  3. What happens if the app is running when it gets the silent notification? If (1) does its job right I assume we try to avoid silent notifying actively running applications, but I imagine it'll happen sometimes no matter what. I assume the app will just ignore the silent notification entirely and wait for the Pusher to update it with the new action?

  4. If there are multiple back to back push notifications, are they guaranteed to be delivered in order, and only one at a time? For example, they wouldn't simultaneously open up and update the app, causing some kind of race condition, right? I'm guessing they will NOT necessarily be in order, but WILL be atomically applied (ie, only one notification will be delivered to the app at a time)

  5. If we assume the app is totally killed, and we get a silent update notification, and Airship starts the app in some kind of silent background update mode -- does it kill it right after, or does that app linger in this "zombie" form (where it is not connected to Pusher, and has no UI) for a while, perhaps to receive the next push notification

  6. If (5) does leave the background app in this zombie mode, when the user goes to open the app, does it "upgrade" this existing running zombie app into a full user app (ie, connect to Pusher, and add all the UI), or does it actually just kill the zombie app and then start a new app fresh from scratch?

Given those assumptions, am I correct in assuming the basic flow is:

  1. Every time a reportAction is added to a report, we broadcast using Airship a silent notification to everyone that report is shared, filtering out apps that are currently connected. The notification contains:

    • the reportID of the report that has changed
    • the sequenceNumber of the new action
  2. If the app is already running and connected to Pusher when Airship sends the silent notification, the app simply ignores it.

  3. If the app is not running or not connected to Pusher, it invokes a "silent updater", which simply:

    1. Looks up the last action stored for this report
    2. If we have anything, and it's indicating there is a newer sequence number available, we ask for everything after the last sequence number we received:
      • For example, if we last downloaded seqNum 10, and Airship delivered a push notification for seqNum 20, then we request "everything over seqNum 10". This way if there was a seqNum 21 added right after, we'll get it all in one request, and won't need to do multiple requests
    3. Whatever actions it gets are inserted straight into Ion
    4. Additionally, it will Ion.merge('report_XXXX', '$.maxSeqNum: YY, $.unread:true') to update the report object to indicate there is a new unread action on that report
  4. If this is a new report, Ion has a handler that will detect the new report with an unknown name, and will call the API to fetch the missing information

Is all that about right?

@roryabraham
Copy link
Contributor

roryabraham commented Sep 11, 2020

Answers to the first 6 questions in slack here.


Every time a reportAction is added to a report, we broadcast using Airship a silent notification to everyone that report is shared, filtering out apps that are currently connected. The notification contains the reportID of the report that has changed and the sequenceNumber of the new action.

I was thinking that, more generally, each silent notification can just trigger any action, providing the name of the action and any parameters. So, if a new report action was created, we could send a push notification with the data:

{
    "action": "REPORT_FETCH_ACTIONS",
    "reportID": 746,
}

Which would trigger Report::fetchActions.

If a new report was created, we could just send:

{
    "action": "REPORT_FETCH_ALL",
}

If the app is already running and connected to Pusher when Airship sends the silent notification, the app simply ignores it.

This isn't set up, but sounds like a good idea 👍

@mallenexpensify
Copy link
Contributor

@roryabraham do you have an update on this? It's one of the only CRITICAL issues on the milestone and @chiragsalian is waiting for it so he can work on #331 (comment)

@roryabraham
Copy link
Contributor

Yeah, so this is prettymuch ready in that the Airship library has been added to the codebase. I've been holding off on writing more code, pending some consensus from people on the points outlined in the TODO/RequestForComments on the PR. Also see this issue for discussion of the main decision that needs to be made.

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

6 participants