notif: Get token on Android, and send to server #325

gnprice · 2023-10-17T01:06:34Z

~~This PR is stacked atop #319.~~ → Rebased now that #319 is merged.

This implements part of #320.

To make an end-to-end demo, we also listen for notification messages, and just print them to the debug log.

chrisbobbe

Thanks; exciting to see this on its way! A few comments below from reading the code.

chrisbobbe · 2023-10-19T18:29:08Z

lib/notifications.dart

+    // On a typical launch of the app (other than the first one after install),
+    // this is the only way we learn the token value; onTokenRefresh never fires.


At first I misunderstood "onTokenRefresh never fires" to mean that there isn't a possible case where the FCM system decides to replace the token during an app session (resulting in an onTokenRefresh call).

I think what you mean (since you do mention that case in an implementation comment in _onTokenRefresh) is that an onTokenRefresh call isn't part of the startup sequence except on the first startup after install.

I wonder if my misreading started with interpreting "on a typical launch of the app" instead as "in a session that started with a typical launch". Anyway, I wonder if there's an alternative wording that helps prevent a misreading like mine?

Hmm, not quite that either — here's another way of putting what I mean here:

In a typical session of the app, onTokenRefresh never fires and our _onTokenRefresh callback is never called. In that case this code is the only way we learn the token value in that session.

That's "typical" in that the exceptions are the first time the app runs after install, and any time the token changes while the app is running. (And the latter is expected to be rare.)

The real point, though, is just that there are some cases where onTokenRefresh never fires and so we have to rely on this return value from getToken. As it happens, they're not even an edge case; they're the usual case.

chrisbobbe · 2023-10-19T18:56:42Z

lib/model/store.dart

@@ -451,6 +453,7 @@ class LivePerAccountStore extends PerAccountStore {
      initialSnapshot: initialSnapshot,
    );
    store.poll();
+    store.registerNotificationToken();


Does the token-registration call want to go earlier than this, which comes after /register? For some users, /register will 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 /register success, 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 its registerNotificationToken) until after /register succeeds…hmm. But as a comment on LivePerAccountStore.load says, we might someday load a LivePerAccountStore from local storage, and that would let us call a LivePerAccountStore.registerNotificationToken sooner than the /register request.

…But actually, does the job of registerNotificationToken really need to wait until we have a LivePerAccountStore? (Does it need to be a method on LivePerAccountStore?) As implemented, it looks like its only dependencies are the NotificationService instance and an ApiConnection. The former is global, and for the latter we already have the value we need before starting the /register request. We may in the future need an ackedPushToken, to condition on whether the new token matches the old. But we can get that from the Account we already have our hands on before starting the /register request.

As implemented, it looks like its only dependencies are […]

I guess it rightly has another dependency: some means of tracking when the listener it adds on NotificationService.instance.token needs to be removed. That's a convenient role for LivePerAccountStore to fill…hmm. I see the "TODO call removeListener on [dispose]" in registerNotificationToken.

Does the token-registration call want to go earlier than this, which comes after /register?

Hmm, good thought. I think I basically just copied this aspect from zulip-mobile.

I guess it rightly has another dependency: some means of tracking when the listener it adds on NotificationService.instance.token needs to be removed. That's a convenient role for LivePerAccountStore to fill…hmm. I see the "TODO call removeListener on [dispose]" in registerNotificationToken.

Yeah. But that's really just because on dispose, we may be about to make a new LivePerAccountStore for 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 LivePerAccountStore and/or whatever's doing the polling for events.

…But actually, does the job of registerNotificationToken really need to wait until we have a LivePerAccountStore? (Does it need to be a method on LivePerAccountStore?)

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. from load.

chrisbobbe · 2023-10-19T19:25:37Z

lib/model/store.dart

+
+  Future<void> _registerNotificationToken() async {
+    final token = NotificationService.instance.token.value;
+    if (token == null) return;


Ah interesting; I guess this early return doesn't make things fall down if the get-device-token request happens to complete after the await _registerNotificationToken() (above) is called. That's because of this listener we add:

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 _registerNotificationToken and its public wrapper registerNotificationToken have an unnecessarily misleading return type. I think their callers can't tell by awaiting the Future whether the token registration succeeded, failed (as with an API error), or is still pending. Is that right? Maybe void as a return type is a better match for that behavior.

Maybe void is the right return type given that.

I see that the tests use the Future<void> return type by awaiting the returned Future. Instead of doing that, I wonder if e.g. FakeAsync.elapse could be used instead?

Yeah, this return type actually was void in a draft before I wrote the tests.

Instead of doing that, I wonder if e.g. FakeAsync.elapse could be used instead?

It's a bit tricky because in the "initially unknown" test case, we first want to wait for store.registerNotificationToken to complete whatever it's going to do, then check no request was made, and only after that wait for NotificationService.start to finish, including its getToken call.

I think a fully robust solution probably involves extending TestZulipBinding so that the test can control when getToken finishes.

chrisbobbe · 2023-10-19T19:29:13Z

lib/notifications.dart

+  }
+
+  Future<void> _getToken() async {
+    final value = await ZulipBinding.instance.firebaseMessaging.getToken(); // TODO(log) if null


Logging might be right for now, but I wonder about:

retrying

preparing to warn the user that setup failed or is not complete

I think this request will naturally fail in ordinary circumstances, like no Internet connection, no Google Play Services, etc., right?

I assume it might also hang indefinitely in some failure modes.

I think those could be TODOs for now though.

Yeah, when it fails we'll want to note that for showing a warning (as in #323). Which, it's true, TODO(log) doesn't cover.

I assume it might also hang indefinitely in some failure modes.

Yeah, could be. We'll want #323 or related warnings to appear for the user if we haven't gotten a result here.

chrisbobbe · 2023-10-19T19:45:03Z

test/example_data.dart

+LivePerAccountStore liveStore({Account? account, InitialSnapshot? initialSnapshot}) {
+  return LivePerAccountStore.fromInitialSnapshot(
+    account: account ?? selfAccount,
+    connection: FakeApiConnection.fromAccount(account ?? selfAccount),
+    initialSnapshot: initialSnapshot ?? _initialSnapshot(),
+  );
+}


One thing I meant to put in my review (just posted now) but forgot: why should test code want a LivePerAccountStore? 🙂

Well, to test registerNotificationToken. 🙂 Or poll — we don't have tests for that, but probably should, and there's ways it should get more complex than it is and then those should definitely get tested.

This does mean that those two methods make LivePerAccountStore not really analogous to LiveZulipBinding or ApiConnection.live — unlike those, it contains nontrivial logic of its own that isn't just integrating with external side effects outside of Dart, logic that is amenable to unit tests. That definitely suggests there's some kind of refactoring to be done here that would draw the lines more cleanly.

Ah yeah I guess that makes sense.

gnprice · 2023-10-24T00:25:28Z

Thanks for the review! Revision pushed.

For several threads I stuck to adding TODO comments — there's refactors that would be good to make here, but I think this will work OK as it is, and I'd like to save those to circle back to after getting notifications working as a whole.

This will help keep things organized as we add more and more features to the binding class, by letting the data and logic for each feature live all in one contiguous section of the code.

This code is so trivial that I'm torn on whether the tests are actually useful; they feel a lot like just repeating the implementation. But they were easy to write.

This implements part of zulip#320. To make an end-to-end demo, we also listen for notification messages, and just print them to the debug log.

chrisbobbe · 2023-10-25T03:39:08Z

Thanks, LGTM! Merged.

…Token The called method returns a not so useful future that is used for testing. See also: zulip#325 (comment) Signed-off-by: Zixuan James Li <[email protected]>

The called method returns a not so useful future that is used for testing. See also: zulip#325 (comment) Signed-off-by: Zixuan James Li <[email protected]>

gnprice added the a-notifications label Oct 17, 2023

gnprice force-pushed the pr-notif branch 2 times, most recently from b7d8055 to b92ed6a Compare October 19, 2023 01:12

chrisbobbe reviewed Oct 19, 2023

View reviewed changes

gnprice force-pushed the pr-notif branch from b92ed6a to 1d4bfa0 Compare October 24, 2023 00:22

gnprice mentioned this pull request Oct 24, 2023

notif: Parse notification messages, on Android #333

Merged

gnprice added 7 commits October 24, 2023 23:24

deps: Add firebase_core, firebase_messaging

90ad2b9

notif: Add Firebase options

33f6a52

test [nfc]: Split up TestZulipBinding.reset

639d616

This will help keep things organized as we add more and more features to the binding class, by letting the data and logic for each feature live all in one contiguous section of the code.

binding: Add firebaseInitializeApp, firebaseMessaging

2a7d145

api: Add routes registerFcmToken, registerApnsToken

b8290cd

This code is so trivial that I'm torn on whether the tests are actually useful; they feel a lot like just repeating the implementation. But they were easy to write.

test: Add eg.liveStore, a LivePerAccountStore

ff4ad6a

notif: Get token on Android, and send to server

11d456d

This implements part of zulip#320. To make an end-to-end demo, we also listen for notification messages, and just print them to the debug log.

chrisbobbe force-pushed the pr-notif branch from 1d4bfa0 to 11d456d Compare October 25, 2023 03:38

chrisbobbe merged commit 11d456d into zulip:main Oct 25, 2023

gnprice deleted the pr-notif branch October 25, 2023 19:22

chrisbobbe mentioned this pull request Oct 25, 2023

deps: Flutter, packages to latest #335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notif: Get token on Android, and send to server #325

notif: Get token on Android, and send to server #325

gnprice commented Oct 17, 2023 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Oct 19, 2023

gnprice Oct 20, 2023

chrisbobbe Oct 19, 2023

chrisbobbe Oct 19, 2023

gnprice Oct 20, 2023

chrisbobbe Oct 19, 2023

chrisbobbe Oct 19, 2023

gnprice Oct 20, 2023

chrisbobbe Oct 19, 2023

gnprice Oct 20, 2023

chrisbobbe Oct 19, 2023

gnprice Oct 20, 2023

chrisbobbe Oct 23, 2023

gnprice commented Oct 24, 2023

chrisbobbe commented Oct 25, 2023

		// On a typical launch of the app (other than the first one after install),
		// this is the only way we learn the token value; onTokenRefresh never fires.

notif: Get token on Android, and send to server #325

notif: Get token on Android, and send to server #325

Conversation

gnprice commented Oct 17, 2023 • edited Loading

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Oct 24, 2023

chrisbobbe commented Oct 25, 2023

gnprice commented Oct 17, 2023 •

edited

Loading

chrisbobbe left a comment •

edited

Loading