push_device: Move registering push token off of UpdateMachine; fix ad-hoc Future used for tests

lib/model/push_device.dart

@@ -0,0 +1,93 @@
+import 'dart:async';
+
+import '../notifications/receive.dart';
+import 'store.dart';
+
+/// Manages telling the server this device's push token,
+/// and tracking the server's responses on the status of push devices.
+// TODO(#1764) do that tracking of responses
+class PushDeviceManager extends PerAccountStoreBase {
+  PushDeviceManager({required super.core}) {
+    _registerTokenAndSubscribe();
+  }
+
+  bool _disposed = false;
+
+  /// Cleans up resources and tells the instance not to make new API requests.
+  ///
+  /// After this is called, the instance is not in a usable state
+  /// and should be abandoned.
+  void dispose() {
+    assert(!_disposed);
+    NotificationService.instance.token.removeListener(_registerToken);
+    _disposed = true;
+  }
+
+  /// Send this client's notification token to the server, now and if it changes.
+  // TODO(#322) save acked token, to dedupe updating it on the server
+  // TODO(#323) track the addFcmToken/etc request, warn if not succeeding
+  void _registerTokenAndSubscribe() async {
+    _debugMaybePause();
+    if (_debugRegisterTokenProceed != null) {
+      await _debugRegisterTokenProceed!.future;
+    }
+
+    NotificationService.instance.token.addListener(_registerToken);
+    await _registerToken();
+
+    _debugRegisterTokenCompleted?.complete();
+  }
+
+  Future<void> _registerToken() async {
+    // TODO it would be nice to register the token before even registerQueue:
+    //   https://github.com/zulip/zulip-flutter/pull/325#discussion_r1365982807
+    await NotificationService.instance.registerToken(connection);
+  }
+
+  Completer<void>? _debugRegisterTokenProceed;
+  Completer<void>? _debugRegisterTokenCompleted;
+
+  void _debugMaybePause() {
+    assert(() {
+      if (debugAutoPause) {
+        _debugRegisterTokenProceed = Completer();
+        _debugRegisterTokenCompleted = Completer();
+      }
+      return true;
+    }());
+  }
+
+  /// Unpause registering the token (after [debugAutoPause]),
+  /// returning a future that completes when any immediate request is completed.
+  ///
+  /// This has no effect if [debugAutoPause] was false
+  /// when this instance was constructed,
+  /// and therefore no effect outside of debug mode.
+  Future<void> debugUnpauseRegisterToken() async {
+    _debugRegisterTokenProceed!.complete();
+    await _debugRegisterTokenCompleted!.future;
+  }
+
+  /// In debug mode, controls whether new instances should pause
+  /// before registering the token with the server.
+  ///
+  /// When paused, token registration can be unpaused
+  /// with [debugUnpauseRegisterToken].
+  ///
+  /// Outside of debug mode, this is always false and the setter has no effect.
+  static bool get debugAutoPause {
+    bool result = false;
+    assert(() {
+      result = _debugAutoPause;
+      return true;
+    }());
+    return result;
+  }
+  static bool _debugAutoPause = false;
+  static set debugAutoPause(bool value) {
+    assert(() {
+      _debugAutoPause = value;
+      return true;
+    }());
+  }
+}

lib/model/store.dart

@@ -17,14 +17,14 @@ import '../api/route/events.dart';
 import '../api/backoff.dart';
 import '../api/route/realm.dart';
 import '../log.dart';
-import '../notifications/receive.dart';
 import 'actions.dart';
 import 'autocomplete.dart';
 import 'database.dart';
 import 'emoji.dart';
 import 'localizations.dart';
 import 'message.dart';
 import 'presence.dart';
+import 'push_device.dart';
 import 'realm.dart';
 import 'recent_dm_conversations.dart';
 import 'recent_senders.dart';
@@ -529,6 +529,7 @@ class PerAccountStore extends PerAccountStoreBase with
       emoji: EmojiStoreImpl(core: core,
         allRealmEmoji: initialSnapshot.realmEmoji),
       userSettings: initialSnapshot.userSettings,
+      pushDevices: PushDeviceManager(core: core),
       savedSnippets: SavedSnippetStoreImpl(core: core,
         savedSnippets: initialSnapshot.savedSnippets ?? []),
       typingNotifier: TypingNotifier(realm: realm),
@@ -552,6 +553,7 @@ class PerAccountStore extends PerAccountStoreBase with
     required RealmStoreImpl realm,
     required EmojiStoreImpl emoji,
     required this.userSettings,
+    required this.pushDevices,
     required SavedSnippetStoreImpl savedSnippets,
     required this.typingNotifier,
     required UserStoreImpl users,
@@ -623,6 +625,8 @@ class PerAccountStore extends PerAccountStoreBase with
 
   final UserSettings userSettings;
 
+  final PushDeviceManager pushDevices;
+
   @override
   Map<int, SavedSnippet> get savedSnippets => _savedSnippets.savedSnippets;
   final SavedSnippetStoreImpl _savedSnippets;
@@ -714,6 +718,7 @@ class PerAccountStore extends PerAccountStoreBase with
     presence.dispose();
     typingStatus.dispose();
     typingNotifier.dispose();
+    pushDevices.dispose();
     updateMachine?.dispose();
     connection.close();
     _disposed = true;
@@ -1118,9 +1123,6 @@ class UpdateMachine {
       //   serverEmojiDataUrl are already unsupported at time of writing.)
       unawaited(updateMachine.fetchEmojiData(initialSnapshot.serverEmojiDataUrl!));
     }
-    // TODO do registerNotificationToken before registerQueue:
-    //   https://github.com/zulip/zulip-flutter/pull/325#discussion_r1365982807
-    unawaited(updateMachine.registerNotificationToken());
     store.presence.start();
     return updateMachine;
   }
@@ -1505,28 +1507,6 @@ class UpdateMachine {
         store.realmUrl.toString(), error.toString()));
   }
 
-  /// 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(#322) save acked token, to dedupe updating it on the server
-  // TODO(#323) track the addFcmToken/etc request, warn if not succeeding
-  Future<void> registerNotificationToken() async {
-    assert(!_disposed);
-    if (!debugEnableRegisterNotificationToken) {
-      return;
-    }
-    NotificationService.instance.token.addListener(_registerNotificationToken);
-    await _registerNotificationToken();
-  }
-
-  Future<void> _registerNotificationToken() async {
-    final token = NotificationService.instance.token.value;
-    if (token == null) return;
-    await NotificationService.registerToken(store.connection, token: token);
-  }
-
   /// Cleans up resources and tells the instance not to make new API requests.
   ///
   /// After this is called, the instance is not in a usable state
@@ -1537,7 +1517,6 @@ class UpdateMachine {
   /// requests to error. [PerAccountStore.dispose] does that.
   void dispose() {
     assert(!_disposed);
-    NotificationService.instance.token.removeListener(_registerNotificationToken);
     _disposed = true;
   }
 
@@ -1561,26 +1540,6 @@ class UpdateMachine {
     }());
   }
 
-  /// In debug mode, controls whether [registerNotificationToken] should
-  /// have its normal effect.
-  ///
-  /// Outside of debug mode, this is always true and the setter has no effect.
-  static bool get debugEnableRegisterNotificationToken {
-    bool result = true;
-    assert(() {
-      result = _debugEnableRegisterNotificationToken;
-      return true;
-    }());
-    return result;
-  }
-  static bool _debugEnableRegisterNotificationToken = true;
-  static set debugEnableRegisterNotificationToken(bool value) {
-    assert(() {
-      _debugEnableRegisterNotificationToken = value;
-      return true;
-    }());
-  }
-
   @override
   String toString() => '${objectRuntimeType(this, 'UpdateMachine')}#${shortHash(this)}';
 }

lib/notifications/receive.dart

@@ -147,7 +147,10 @@ class NotificationService {
     token.value = value;
   }
 
-  static Future<void> registerToken(ApiConnection connection, {required String token}) async {
+  Future<void> registerToken(ApiConnection connection) async {
+    final token = this.token.value;
+    if (token == null) return;
+
     switch (defaultTargetPlatform) {
       case TargetPlatform.android:
         await addFcmToken(connection, token: token);

test/model/actions_test.dart

@@ -7,6 +7,7 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:http/http.dart' as http;
 import 'package:http/testing.dart' as http_testing;
 import 'package:zulip/model/actions.dart';
+import 'package:zulip/model/push_device.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/notifications/receive.dart';
 
@@ -147,6 +148,8 @@ void main() {
     }));
 
     test('notifications are removed after logout', () => awaitFakeAsync((async) async {
+      PushDeviceManager.debugAutoPause = true;
+      addTearDown(() => PushDeviceManager.debugAutoPause = false);
       await prepare();
       testBinding.firebaseMessagingInitialToken = '123';
       addTearDown(NotificationService.debugReset);
@@ -180,6 +183,7 @@ void main() {
 
     test('fallback to current token if acked is missing', () => awaitFakeAsync((async) async {
       await prepare(ackedPushToken: null);
+      addTearDown(NotificationService.debugReset);
       NotificationService.instance.token = ValueNotifier('asdf');
 
       final newConnection = separateConnection()
@@ -193,6 +197,7 @@ void main() {
 
     test('no error if acked token and current token both missing', () => awaitFakeAsync((async) async {
       await prepare(ackedPushToken: null);
+      addTearDown(NotificationService.debugReset);
       NotificationService.instance.token = ValueNotifier(null);
 
       final newConnection = separateConnection();

test/model/push_device_test.dart

@@ -0,0 +1,130 @@
+import 'package:checks/checks.dart';
+import 'package:flutter/foundation.dart';
+import 'package:http/http.dart' as http;
+import 'package:test/scaffolding.dart';
+import 'package:zulip/model/push_device.dart';
+import 'package:zulip/notifications/receive.dart';
+
+import '../api/fake_api.dart';
+import '../example_data.dart' as eg;
+import '../fake_async.dart';
+import 'binding.dart';
+import 'store_test.dart';
+import '../stdlib_checks.dart';
+
+void main() {
+  TestZulipBinding.ensureInitialized();
+
+  group('register token', () {
+    late PushDeviceManager model;
+    late FakeApiConnection connection;
+
+    void prepareStore() {
+      PushDeviceManager.debugAutoPause = true;
+      addTearDown(() => PushDeviceManager.debugAutoPause = false);
+      final store = eg.store();
+      model = store.pushDevices;
+      connection = store.connection as FakeApiConnection;
+    }
+
+    void checkLastRequestApns({required String token, required String appid}) {
+      check(connection.takeRequests()).single.isA<http.Request>()
+        ..method.equals('POST')
+        ..url.path.equals('/api/v1/users/me/apns_device_token')
+        ..bodyFields.deepEquals({'token': token, 'appid': appid});
+    }
+
+    void checkLastRequestFcm({required String token}) {
+      check(connection.takeRequests()).single.isA<http.Request>()
+        ..method.equals('POST')
+        ..url.path.equals('/api/v1/users/me/android_gcm_reg_id')
+        ..bodyFields.deepEquals({'token': token});
+    }
+
+    testAndroidIos('token already known', () => awaitFakeAsync((async) async {
+      // This tests the case where [NotificationService.start] has already
+      // learned the token before the store is created.
+      // (This is probably the common case.)
+      addTearDown(testBinding.reset);
+      testBinding.firebaseMessagingInitialToken = '012abc';
+      testBinding.packageInfoResult = eg.packageInfo(packageName: 'com.zulip.flutter');
+      addTearDown(NotificationService.debugReset);
+      await NotificationService.instance.start();
+
+      // On store startup, send the token.
+      prepareStore();
+      connection.prepare(json: {});
+      await model.debugUnpauseRegisterToken();
+      if (defaultTargetPlatform == TargetPlatform.android) {
+        checkLastRequestFcm(token: '012abc');
+      } else {
+        checkLastRequestApns(token: '012abc', appid: 'com.zulip.flutter');
+      }
+
+      if (defaultTargetPlatform == TargetPlatform.android) {
+        // If the token changes, send it again.
+        testBinding.firebaseMessaging.setToken('456def');
+        connection.prepare(json: {});
+        async.flushMicrotasks();
+        checkLastRequestFcm(token: '456def');
+      }
+    }));
+
+    testAndroidIos('token initially unknown', () => awaitFakeAsync((async) async {
+      // This tests the case where the store is created while our
+      // request for the token is still pending.
+      addTearDown(testBinding.reset);
+      testBinding.firebaseMessagingInitialToken = '012abc';
+      testBinding.packageInfoResult = eg.packageInfo(packageName: 'com.zulip.flutter');
+      addTearDown(NotificationService.debugReset);
+      final startFuture = NotificationService.instance.start();
+
+      // TODO this test is a bit brittle in its interaction with asynchrony;
+      //   to fix, probably extend TestZulipBinding to control when getToken finishes.
+      //
+      // The aim here is to first wait for `model.debugUnpauseRegisterToken`
+      // 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.
+
+      // On store startup, send nothing (because we have nothing to send).
+      prepareStore();
+      await model.debugUnpauseRegisterToken();
+      check(connection.lastRequest).isNull();
+
+      // When the token later appears, send it.
+      connection.prepare(json: {});
+      await startFuture;
+      async.flushMicrotasks();
+      if (defaultTargetPlatform == TargetPlatform.android) {
+        checkLastRequestFcm(token: '012abc');
+      } else {
+        checkLastRequestApns(token: '012abc', appid: 'com.zulip.flutter');
+      }
+
+      if (defaultTargetPlatform == TargetPlatform.android) {
+        // If the token subsequently changes, send it again.
+        testBinding.firebaseMessaging.setToken('456def');
+        connection.prepare(json: {});
+        async.flushMicrotasks();
+        checkLastRequestFcm(token: '456def');
+      }
+    }));
+
+    test('on iOS, use provided app ID from packageInfo', () => awaitFakeAsync((async) async {
+      final origTargetPlatform = debugDefaultTargetPlatformOverride;
+      addTearDown(() => debugDefaultTargetPlatformOverride = origTargetPlatform);
+      debugDefaultTargetPlatformOverride = TargetPlatform.iOS;
+      addTearDown(testBinding.reset);
+      testBinding.firebaseMessagingInitialToken = '012abc';
+      testBinding.packageInfoResult = eg.packageInfo(packageName: 'com.example.test');
+      addTearDown(NotificationService.debugReset);
+      await NotificationService.instance.start();
+
+      prepareStore();
+      connection.prepare(json: {});
+      await model.debugUnpauseRegisterToken();
+      checkLastRequestApns(token: '012abc', appid: 'com.example.test');
+    }));
+  });
+}