-
Notifications
You must be signed in to change notification settings - Fork 270
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
Don't show notifications for logged-out accounts, on Android #1264
Comments
Hi, I was able to implement the first part (not showing notifications for logged out accounts), but my logic for second (removing existing notifications of an account upon logout) is failing atm.
I suspect the issue is with the groupKey I'm using to perform match. |
This comment has been minimized.
This comment has been minimized.
@tomlin7 Good to hear, thanks! Would you be up for sending a PR that has your work so far? I'd be glad to merge a change that does the first part while we're working on the second. Seeing the code would also make it easier to help debug the second part. (Sorry I missed this earlier — I get a lot of GitHub notifications and don't always keep up. I recommend asking on chat.zulip.org whenever you have a question about how to do something. That way I'll see it sooner, plus more other people will see it who might be able to answer.) |
When we get a notification-message from a Zulip server, but it turns out to be for an account that isn't actually in our list of accounts, we should skip showing any actual notification to the user.
The situation where this can happen is when the user has logged out of an account (by removing it from the list of accounts). When they do, we make a request to the server to stop sending notifications for that account to this device; see implementation. But that request can fail — for example, maybe the device doesn't happen to have an Internet connection at the time the user removes the account. In that case the server will keep sending notifications.
It's annoying to the user if notifications keep showing up after they've logged out. (Also alarming: it suggests the logout didn't fully work.) The user didn't want those notifications, so we should avoid showing them.
Right now this will only benefit Android, because that's where we have our own code involved between the device receiving a notification-message and a notification getting shown in the UI.
Reported today in chat.
Implementation
In NotificationDisplayManager, when a MessageFcmMessage comes in, we should look up the corresponding account; if none is found, don't show anything.
Also when logging out we should remove any existing notifications for that account from the UI.
Related issues
Previous report for the zulip-mobile legacy app, with some discussion:
Notifications when logged out / offline zulip-mobile#5763
(This would have been much harder to fix in the legacy app than in this Flutter app, because notifications were handled there in Kotlin code that didn't have access to the app's stored data.)
We should fix this symptom on iOS too. I think the way to do that will be to start having our own code involved in deciding what notification to show in the UI, just like it is for Android:
ios notif: Control the showing of notifications #1265
(We'll need that for some other nice notification features, too.)
Separately, we should retry the "stop sending notifications" request later if it didn't succeed the first time, so that the server stops sending notification-messages that aren't going to be shown to the user.
This is tricky security-wise, because we don't want to hold onto the user's API key after they've logged out. Doing this may require server-side changes, so that we can hold onto some credential that carries only the ability to do this one request.
The text was updated successfully, but these errors were encountered: