notification: Use user_id to find account if multiple on same realm

android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt

@@ -111,7 +111,8 @@ data class MessageFcmMessage(
      */
     fun dataForOpen(): Bundle = Bundle().apply {
         // NOTE: Keep the JS-side type definition in sync with this code.
-        identity.realmUri.let { putString("realm_uri", it.toString()) }
+        putString("realm_uri", identity.realmUri.toString())
+        identity.userId?.let { putInt("user_id", it) }
         when (recipient) {
             is Recipient.Stream -> {
                 putString("recipient_type", "stream")
@@ -190,6 +191,8 @@ private fun extractIdentity(data: Map<String, String>): Identity =
     Identity(
         serverHost = data.require("server"),
         realmId = data.require("realm_id").parseInt("realm_id"),
+
+        // `realm_uri` was added in server version 1.9.0
         realmUri = data.require("realm_uri").parseUrl("realm_uri"),
 
         // Server versions from 1.6.0 through 2.0.0 (and possibly earlier

src/notification/__tests__/notification-test.js

@@ -12,6 +12,7 @@ import { fromAPNsImpl as extractIosNotificationData } from '../extract';
 import objectEntries from '../../utils/objectEntries';
 
 const realm_uri = eg.realm.toString();
+const user_id = eg.selfUser.user_id;
 
 describe('getNarrowFromNotificationData', () => {
   const ownUserId = eg.selfUser.user_id;
@@ -81,6 +82,14 @@ describe('extract iOS notification data', () => {
         const msg = data;
         expect(verify(msg)).toEqual(msg);
 
+        // new(-ish) optional user_id is accepted and copied
+        // TODO: Rewrite so modern-style payloads are the baseline, e.g.,
+        //   with a `modern` variable instead of `barebones`. Write
+        //   individual tests for supporting older-style payloads, and mark
+        //   those for future deletion, like with `TODO(1.9.0)`.
+        const msg1 = { ...msg, user_id };
+        expect(verify(msg1)).toEqual(msg1);
+
         // unused fields are not copied
         const msg2 = { ...msg, realm_id: 8675309 };
         expect(verify(msg2)).toEqual(msg);
@@ -107,7 +116,7 @@ describe('extract iOS notification data', () => {
     test('very-old-style messages', () => {
       const sender_email = '[email protected]';
       // baseline
-      expect(make({ realm_uri, recipient_type: 'private', sender_email })).toBeTruthy();
+      expect(make({ realm_uri, recipient_type: 'private', sender_email })()).toBeTruthy();
       // missing recipient_type
       expect(make({ realm_uri, sender_email })).toThrow(/archaic/);
       // missing realm_uri
@@ -155,6 +164,13 @@ describe('extract iOS notification data', () => {
           realm_uri: ['array', 'of', 'string'],
         }),
       ).toThrow(/invalid/);
+
+      expect(
+        make({
+          ...barebones.stream,
+          user_id: 'abc',
+        }),
+      ).toThrow(/invalid/);
     });
 
     test('hypothetical future: different event types', () => {

src/notification/extract.js

@@ -2,6 +2,7 @@
 import PushNotificationIOS from '@react-native-community/push-notification-ios';
 
 import type { Notification } from './types';
+import { makeUserId } from '../api/idTypes';
 import type { JSONable, JSONableDict, JSONableInput, JSONableInputDict } from '../utils/jsonable';
 import * as logging from '../utils/logging';
 
@@ -172,24 +173,32 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
     throw err('invalid');
   }
 
-  const { realm_uri } = zulip;
+  const { realm_uri, user_id } = zulip;
   if (realm_uri === undefined) {
     throw err('archaic (pre-1.9.x)');
   }
   if (typeof realm_uri !== 'string') {
     throw err('invalid');
   }
+  if (user_id !== undefined && typeof user_id !== 'number') {
+    throw err('invalid');
+  }
+
+  const identity = {
+    realm_uri,
+    ...(user_id === undefined ? Object.freeze({}) : { user_id: makeUserId(user_id) }),
+  };
 
   if (recipient_type === 'stream') {
     const { stream, topic } = zulip;
     if (typeof stream !== 'string' || typeof topic !== 'string') {
       throw err('invalid');
     }
     return {
+      ...identity,
       recipient_type: 'stream',
       stream,
       topic,
-      realm_uri,
     };
   } else {
     /* recipient_type === 'private' */
@@ -204,16 +213,16 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
         throw err('invalid');
       }
       return {
+        ...identity,
         recipient_type: 'private',
         pm_users: ids.sort((a, b) => a - b).join(','),
-        realm_uri,
       };
     }
 
     if (typeof sender_email !== 'string') {
       throw err('invalid');
     }
-    return { recipient_type: 'private', sender_email, realm_uri };
+    return { ...identity, recipient_type: 'private', sender_email };
   }
 
   /* unreachable */

src/notification/index.js

@@ -4,7 +4,7 @@ import PushNotificationIOS from '@react-native-community/push-notification-ios';
 import type { PushNotificationEventName } from '@react-native-community/push-notification-ios';
 
 import type { Notification } from './types';
-import type { Auth, Dispatch, GlobalDispatch, Identity, Narrow, UserId, UserOrBot } from '../types';
+import type { Auth, Dispatch, GlobalDispatch, Account, Narrow, UserId, UserOrBot } from '../types';
 import { topicNarrow, pm1to1NarrowFromUser, pmNarrowFromRecipients } from '../utils/narrow';
 import type { JSONable, JSONableDict } from '../utils/jsonable';
 import * as api from '../api';
@@ -25,16 +25,16 @@ import { getAccounts } from '../directSelectors';
 /**
  * Identify the account the notification is for, if possible.
  *
- * Returns an index into `identities`, or `null` if we can't tell.
+ * Returns an index into `accounts`, or `null` if we can't tell.
  * In the latter case, logs a warning.
  *
- * @param identities Identities corresponding to the accounts state in Redux.
+ * @param accounts The accounts state in Redux.
  */
 export const getAccountFromNotificationData = (
   data: Notification,
-  identities: $ReadOnlyArray<Identity>,
+  accounts: $ReadOnlyArray<Account>,
 ): number | null => {
-  const { realm_uri } = data;
+  const { realm_uri, user_id } = data;
   if (realm_uri == null) {
     // Old server, no realm info included.  If needed to cater to 1.8.x
     // servers, could try to guess using serverHost; for now, don't.
@@ -50,7 +50,7 @@ export const getAccountFromNotificationData = (
   }
 
   const urlMatches = [];
-  identities.forEach((account, i) => {
+  accounts.forEach((account, i) => {
     if (account.realm.origin === realmUrl.origin) {
       urlMatches.push(i);
     }
@@ -62,7 +62,7 @@ export const getAccountFromNotificationData = (
     // just a race -- this notification was sent before the logout); or
     // there's some confusion where the realm_uri we have is different from
     // the one the server sends in notifications.
-    const knownUrls = identities.map(({ realm }) => realm.href);
+    const knownUrls = accounts.map(({ realm }) => realm.href);
     logging.warn('notification realm_uri not found in accounts', {
       realm_uri,
       parsed_url: realmUrl,
@@ -71,23 +71,50 @@ export const getAccountFromNotificationData = (
     return null;
   }
 
-  if (urlMatches.length > 1) {
-    // The user has several accounts in the notification's realm.  We should
-    // be able to tell the right one using the notification's `user_id`...
-    // except we don't store user IDs in `accounts`, only emails.  Until we
-    // fix that, just ignore the information.
-    logging.warn('notification realm_uri ambiguous; multiple matches found', {
-      realm_uri,
-      parsed_url: realmUrl,
-      match_count: urlMatches.length,
-      unique_identities_count: new Set(urlMatches.map(matchIndex => identities[matchIndex].email))
-        .size,
-    });
-    // TODO get user_id into accounts data, and use that
-    return null;
+  // TODO(server-2.1): Remove this, because user_id will always be present
+  if (user_id === undefined) {
+    if (urlMatches.length > 1) {
+      logging.warn(
+        'notification realm_uri ambiguous; multiple matches found; user_id missing (old server)',
+        {
+          realm_uri,
+          parsed_url: realmUrl,
+          match_count: urlMatches.length,
+          unique_identities_count: new Set(urlMatches.map(matchIndex => accounts[matchIndex].email))
+            .size,
+        },
+      );
+      return null;
+    } else {
+      return urlMatches[0];
+    }
   }
 
-  return urlMatches[0];
+  // There may be multiple accounts in the notification's realm. Pick one
+  // based on the notification's `user_id`.
+  const userMatch = urlMatches.find(urlMatch => accounts[urlMatch].userId === user_id);
+  if (userMatch == null) {
+    // Maybe we didn't get a userId match because the correct account just
+    // hasn't had its userId recorded on it yet. See jsdoc on the Account
+    // type for when that is.
+    const nullUserIdMatches = urlMatches.filter(urlMatch => accounts[urlMatch].userId === null);
+    switch (nullUserIdMatches.length) {
+      case 0:
+        logging.warn(
+          'notifications: No accounts found with matching realm and matching-or-null user ID',
+        );
+        return null;
+      case 1:
+        return nullUserIdMatches[0];
+      default:
+        logging.warn(
+          'notifications: Multiple accounts found with matching realm and null user ID; could not choose',
+          { nullUserIdMatchesCount: nullUserIdMatches.length },
+        );
+        return null;
+    }
+  }
+  return userMatch;
 };
 
 export const getNarrowFromNotificationData = (

src/notification/notificationActions.js

@@ -26,7 +26,7 @@ import { identityOfAccount, authOfAccount } from '../account/accountMisc';
 import { getAllUsersByEmail, getOwnUserId } from '../users/userSelectors';
 import { doNarrow } from '../message/messagesActions';
 import { accountSwitch } from '../account/accountActions';
-import { getIdentities, getAccount, tryGetActiveAccountState } from '../account/accountsSelectors';
+import { getAccount, tryGetActiveAccountState } from '../account/accountsSelectors';
 
 export const gotPushToken = (pushToken: string | null): AccountIndependentAction => ({
   type: GOT_PUSH_TOKEN,
@@ -53,7 +53,7 @@ export const narrowToNotification = (data: ?Notification): GlobalThunkAction<voi
   }
 
   const globalState = getState();
-  const accountIndex = getAccountFromNotificationData(data, getIdentities(globalState));
+  const accountIndex = getAccountFromNotificationData(data, getAccounts(globalState));
   if (accountIndex !== null && accountIndex > 0) {
     // Notification is for a non-active account.  Switch there.
     dispatch(accountSwitch(accountIndex));

src/notification/types.js

@@ -1,4 +1,10 @@
 // @flow strict-local
+import type { UserId } from '../types';
+
+type NotificationBase = {|
+  realm_uri: string,
+  user_id?: UserId,
+|};
 
 /**
  * The data we need in JS/React code for acting on a notification.
@@ -13,8 +19,8 @@
  */
 // NOTE: Keep the Android-side code in sync with this type definition.
 export type Notification =
-  | {| recipient_type: 'stream', stream: string, topic: string, realm_uri: string |}
+  | {| ...NotificationBase, recipient_type: 'stream', stream: string, topic: string |}
   // Group PM messages have `pm_users`, which is sorted, comma-separated IDs.
-  | {| recipient_type: 'private', pm_users: string, realm_uri: string |}
+  | {| ...NotificationBase, recipient_type: 'private', pm_users: string |}
   // 1:1 PM messages lack `pm_users`.
-  | {| recipient_type: 'private', sender_email: string, realm_uri: string |};
+  | {| ...NotificationBase, recipient_type: 'private', sender_email: string |};