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

Defer setting up notifications until user has logged in #324

Open
gnprice opened this issue Oct 16, 2023 · 6 comments
Open

Defer setting up notifications until user has logged in #324

gnprice opened this issue Oct 16, 2023 · 6 comments

Comments

@gnprice
Copy link
Member

gnprice commented Oct 16, 2023

In my initial implementation of notifications (#122), I think for simplicity I'll have it go ahead and initialize the notifications infrastructure as soon as the app is launched: find out the registration token, and listen for incoming notification messages.

This is likely to produce an off-putting UX at onboarding time, though: it's likely to cause a permissions prompt, asking the user to give us permission to show notifications, first thing before they've had any other interaction with the app.

So we should fix it so that we instead do the setup only once the user is actually logged into a Zulip account. That means:

  • Upon launch, if there's an account they're logged into, go ahead and initialize notifications.
  • If we didn't initialize notifications upon launch, then when the user does successfully log into an account, initialize notifications then.
@chrisbobbe
Copy link
Collaborator

In case it's convenient, I think it's probably possible to keep much of the initialization logic—in particular, obtaining the relevant APNs or FCM token—where it is in #122 (at startup), on modern Android and iOS, while still postponing the permissions prompt to be done at first login or wherever is appropriate.

  • On Android 13+, as we learned in android: targetSdkVersion to 33, this time handling new notifs permission zulip-mobile#5761, the permissions prompt won't ordinarily be shown until the app code makes a function call for it. We're free to obtain the FCM token before we make that function call.
  • On iOS 12+ (which covers all iOS versions we'll support), from reading docs, I think we can accomplish it with this sequence:
    1. At startup, call UNUserNotificationCenter.requestAuthorization, but pass the provisional option, to avoid kicking up the permissions prompt. (That option is what constrains this sequence to iOS 12+.) An "Asking permission" doc says, "Unlike explicitly requesting authorization, [using provisional] doesn’t prompt the person for permission to receive notifications."
    2. Then, call UIApplication.registerForRemoteNotifications and receive the token as documented. (Doing step i before this may not be technically necessary, I don't know. Anyway, that requestAuthorization method is documented with this note: "Always call this method before […] registering with the Apple Push Notification service. […]".)
    3. Later, like on a login success, I think we may be able to call UNUserNotificationCenter.requestAuthorization again, this time without the provisional flag. This time, we request the kinds of non-"quiet" notifications we want (alert, sound, etc.), and it'll give the permissions prompt.

@gnprice
Copy link
Member Author

gnprice commented Oct 19, 2023

Cool, thanks for that investigation.

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Oct 19, 2023

Obtaining the token at startup has some advantages I think:

  • Doesn't the request to get the token depend on Internet availability? If so, it seems like it could take a long time and/or benefit from retries, so it seems helpful to start working on it as soon as possible.
    • If you've already been getting notifications for a logged-in account, but the token changed since the last run of the app, it's important to get the token ASAP to avoid missing notifications unnecessarily. In zulip-mobile, we do delay unnecessarily in this case: we never start re-requesting the token before /register succeeds. I think this unnecessary delay, which happens when notifications are indeed authorized, is basically a bad side effect of zulip-mobile's particular approach to showing the permissions prompt in context on iOS.
  • Will we ever want to offer our own notifications that have nothing to do with a client's connection to any Zulip server? E.g., letting people hear about major Zulip Server releases as they come out, etc.

@gnprice
Copy link
Member Author

gnprice commented Oct 20, 2023

  • Will we ever want to offer our own notifications that have nothing to do with a client's connection to any Zulip server? E.g., letting people hear about major Zulip Server releases as they come out, etc.

I think this is unlikely. As a user I'd definitely find that particular example annoying, and I can't think of any other such use cases.

  • If you've already been getting notifications for a logged-in account, but the token changed since the last run of the app, it's important to get the token ASAP to avoid missing notifications unnecessarily.

If you have a logged-in account, then by my original spec above we'd initialize notifications (including getting the token) immediately on launch.

  • Doesn't the request to get the token depend on Internet availability? If so, it seems like it could take a long time and/or benefit from retries, so it seems helpful to start working on it as soon as possible.

This is an interesting point. I'm not sure.

@chrisbobbe
Copy link
Collaborator

Probably more important than whether we always get the token on startup or not is: whatever we choose, let's see if we can avoid a situation like zulip/zulip-mobile#5329. 😄

@gnprice gnprice modified the milestones: Beta 1, Beta 2 Nov 8, 2023
@gnprice gnprice modified the milestones: Beta 2, Beta 3 Nov 22, 2023
@gnprice
Copy link
Member Author

gnprice commented Feb 14, 2024

In my initial implementation of notifications (#122), I think for simplicity I'll have it go ahead and initialize the notifications infrastructure as soon as the app is launched

Seems like this wasn't totally successful in the initial version:

We do get the token right at startup, and then send it to a server upon logging in, but at least on Android we never get permission to actually show notifications, and so when a notification arrives we don't actually succeed in showing it.

When fixing that, I'll see if it's straightforward to defer the notification permission prompt until logging in, as this issue calls for; if not, I might leave this issue for later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

2 participants