notif: Get token on Android, and send to server #325
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
|
|
||
| import '../core.dart'; | ||
|
|
||
| // This endpoint is undocumented. Compare zulip-mobile: | ||
| // https://github.com/zulip/zulip-mobile/blob/86d94fa89/src/api/notifications/savePushToken.js | ||
| // and see the server implementation: | ||
| // https://github.com/zulip/zulip/blob/34ceafadd/zproject/urls.py#L383 | ||
| // https://github.com/zulip/zulip/blob/34ceafadd/zerver/views/push_notifications.py#L47 | ||
| Future<void> registerFcmToken(ApiConnection connection, { | ||
| required String token, | ||
| }) { | ||
| return connection.post('registerFcmToken', (_) {}, 'users/me/android_gcm_reg_id', { | ||
| 'token': RawParameter(token), | ||
| }); | ||
| } | ||
|
|
||
| // This endpoint is undocumented. Compare zulip-mobile: | ||
| // https://github.com/zulip/zulip-mobile/blob/86d94fa89/src/api/notifications/savePushToken.js | ||
| // and see the server implementation: | ||
| // https://github.com/zulip/zulip/blob/34ceafadd/zproject/urls.py#L378-L381 | ||
| // https://github.com/zulip/zulip/blob/34ceafadd/zerver/views/push_notifications.py#L34 | ||
| Future<void> registerApnsToken(ApiConnection connection, { | ||
| required String token, | ||
| String? appid, | ||
| }) { | ||
| return connection.post('registerApnsToken', (_) {}, 'users/me/apns_device_token', { | ||
| 'token': RawParameter(token), | ||
| if (appid != null) 'appid': RawParameter(appid), | ||
| }); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import 'package:firebase_core/firebase_core.dart'; | ||
|
|
||
| /// Configuration used for receiving notifications on Android. | ||
| /// | ||
| /// This set of options is used for receiving notifications | ||
| /// through the Zulip notification bouncer service: | ||
| /// https://zulip.readthedocs.io/en/latest/production/mobile-push-notifications.html | ||
| /// | ||
| /// These values represent public identifiers for that service | ||
| /// as an application registered with the relevant Google service: | ||
| /// we deliver Android notifications through Firebase Cloud Messaging (FCM). | ||
| /// The values are derived from a `google-services.json` file. | ||
| /// For details, see: | ||
| /// https://developers.google.com/android/guides/google-services-plugin#processing_the_json_file | ||
| const kFirebaseOptionsAndroid = FirebaseOptions( | ||
| // This `appId` and `messagingSenderId` are the same as in zulip-mobile; | ||
| // see zulip-mobile:android/app/src/main/res/values/firebase.xml . | ||
| appId: '1:835904834568:android:6ae61ae43a7c3410', | ||
| messagingSenderId: '835904834568', | ||
|
|
||
| projectId: 'zulip-android', | ||
|
|
||
| // Despite the name, this Google Cloud "API key" is a very different kind | ||
| // of thing from a Zulip "API key". In particular, it's designed to be | ||
| // included in published builds of client applications, and therefore | ||
| // fundamentally public. See docs: | ||
| // https://cloud.google.com/docs/authentication/api-keys | ||
| // | ||
| // This key was created fresh for this use in zulip-flutter. | ||
| // It's easy to create additional keys associated with the same `appId` | ||
| // and other details above, and to enable or disable individual keys. | ||
| // | ||
| // TODO: Perhaps use a different key in published builds; still fundamentally | ||
| // public, but would avoid accidental reuse in dev or modified builds. | ||
| apiKey: 'AIzaSyC6kw5sqCYjxQl2Lbd_8MDmc1lu2EG0pY4', | ||
| ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,9 @@ import '../api/model/initial_snapshot.dart'; | |
| import '../api/model/model.dart'; | ||
| import '../api/route/events.dart'; | ||
| import '../api/route/messages.dart'; | ||
| import '../api/route/notifications.dart'; | ||
| import '../log.dart'; | ||
| import '../notifications.dart'; | ||
| import 'autocomplete.dart'; | ||
| import 'database.dart'; | ||
| import 'message_list.dart'; | ||
|
|
@@ -425,6 +427,8 @@ class LiveGlobalStore extends GlobalStore { | |
| } | ||
|
|
||
| /// A [PerAccountStore] which polls an event queue to stay up to date. | ||
| // TODO decouple "live"ness from polling and registerNotificationToken; | ||
| // the latter are made up of testable internal logic, not external integration | ||
| class LivePerAccountStore extends PerAccountStore { | ||
| LivePerAccountStore.fromInitialSnapshot({ | ||
| required super.account, | ||
|
|
@@ -458,6 +462,9 @@ class LivePerAccountStore extends PerAccountStore { | |
| initialSnapshot: initialSnapshot, | ||
| ); | ||
| store.poll(); | ||
| // TODO do registerNotificationToken before registerQueue: | ||
| // https://github.com/zulip/zulip-flutter/pull/325#discussion_r1365982807 | ||
| store.registerNotificationToken(); | ||
| return store; | ||
| } | ||
|
|
||
|
|
@@ -479,4 +486,25 @@ class LivePerAccountStore extends PerAccountStore { | |
| } | ||
| } | ||
| } | ||
|
|
||
| /// Send this client's notification token to the server, now and if it changes. | ||
| /// | ||
| /// TODO The returned future isn't especially meaningful (it may or may not | ||
| /// mean we actually sent the token). Make it just `void` once we fix the | ||
| /// one test that relies on the future. | ||
| /// | ||
| /// TODO(#321) handle iOS/APNs; currently only Android/FCM | ||
| // TODO(#322) save acked token, to dedupe updating it on the server | ||
| // TODO(#323) track the registerFcmToken/etc request, warn if not succeeding | ||
| Future<void> registerNotificationToken() async { | ||
| // TODO call removeListener on [dispose] | ||
| NotificationService.instance.token.addListener(_registerNotificationToken); | ||
| await _registerNotificationToken(); | ||
| } | ||
|
|
||
| Future<void> _registerNotificationToken() async { | ||
| final token = NotificationService.instance.token.value; | ||
| if (token == null) return; | ||
|
There was a problem hiding this comment. Ah interesting; I guess this early return doesn't make things fall down if the get-device-token request happens to complete after the NotificationService.instance.token.addListener(_registerNotificationToken);With that, it looks like we'll be sure to dispatch the register-with-server request as soon as we have a good result from the get-device-token request, which is good. (And that behavior is covered in the tests; nice!) Still, although that race is handled for users' benefit, I wonder if There was a problem hiding this comment.
I see that the tests use the There was a problem hiding this comment. Yeah, this return type actually was
It's a bit tricky because in the "initially unknown" test case, we first want to wait for I think a fully robust solution probably involves extending TestZulipBinding so that the test can control when |
||
| await registerFcmToken(connection, token: token); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the token-registration call want to go earlier than this, which comes after
/register? For some users,/registerwill regularly take several seconds.In particular, for the case where the user has already been getting and expecting notifications, if the device token has changed since the last run of the app, we have kind of an urgent task to try to register the new token so we don't unnecessarily drop notifications. (I see zulip-mobile doesn't do this earlier than
/registersuccess, but I wonder if that needs to be carried over to zulip-flutter.)I see that we don't have our hands on a
LivePerAccountStore(to call itsregisterNotificationToken) until after/registersucceeds…hmm. But as a comment onLivePerAccountStore.loadsays, we might someday load aLivePerAccountStorefrom local storage, and that would let us call aLivePerAccountStore.registerNotificationTokensooner than the/registerrequest.…But actually, does the job of
registerNotificationTokenreally need to wait until we have aLivePerAccountStore? (Does it need to be a method onLivePerAccountStore?) As implemented, it looks like its only dependencies are theNotificationServiceinstance and anApiConnection. The former is global, and for the latter we already have the value we need before starting the/registerrequest. We may in the future need anackedPushToken, to condition on whether the new token matches the old. But we can get that from theAccountwe already have our hands on before starting the/registerrequest.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it rightly has another dependency: some means of tracking when the listener it adds on
NotificationService.instance.tokenneeds to be removed. That's a convenient role forLivePerAccountStoreto fill…hmm. I see the "TODO call removeListener on [dispose]" inregisterNotificationToken.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good thought. I think I basically just copied this aspect from zulip-mobile.
Yeah. But that's really just because on dispose, we may be about to make a new
LivePerAccountStorefor the same account — so it's a way of preventing duplication. If the listener lived somewhere else that had a different lifetime, then we'd want to cancel it based on that thing's lifetime instead.I guess there's the complication (which we'll eventually implement) that the user's API key could become invalid, or the user or realm get deactivated. In that case we'll want to tear down whatever has that listener, along with the
LivePerAccountStoreand/or whatever's doing the polling for events.Yeah, I don't totally love having it here. It's here because it seems parallel to the event-polling loop
poll; and because this is the most obvious place for the code to live if it's going to be initiated after we get the register response, i.e. fromload.