narrow/recipient: Fix more bugs, and prep for bigger refactors

jest/jestSetup.js

@@ -101,3 +101,19 @@ jest.mock('react-native-image-picker', () => ({
   launchCamera: jest.fn(),
   launchImageLibrary: jest.fn(),
 }));
+
+// Set up our `logging` module with mocks, which tests can use as desired.
+//
+// This global version just passes the calls right through to the real
+// implementations.  To suppress logging in a specific test, make a call
+// like `logging.warn.mockReturnValue()`.  For more, see:
+//   https://jestjs.io/docs/en/mock-function-api
+// or search our code for `logging.warn.` for examples.
+jest.mock('../src/utils/logging', () => {
+  const logging = jest.requireActual('../src/utils/logging');
+  return {
+    __esModule: true, // eslint-disable-line id-match
+    error: jest.fn().mockImplementation(logging.error),
+    warn: jest.fn().mockImplementation(logging.warn),
+  };
+});

src/boot/loggingContext.js

@@ -0,0 +1,47 @@
+/**
+ * A dead-drop for select data to be included in log events.
+ *
+ * More precisely, for a getter for that data; this way we don't have to be
+ * constantly updating it, as we would if this module contained a copy of
+ * the data itself.
+ *
+ * This arrangement provides a way for the logging code to get this
+ * information without having to import the Redux store.  Otherwise we tend
+ * to get nasty import cycles, because logging is naturally low in the
+ * import graph (imported by lots of other modules) and the app's Redux
+ * store is naturally high in the import graph (importing lots of other
+ * modules).
+ *
+ * @flow strict-local
+ */
+import type { ZulipVersion } from '../utils/zulipVersion';
+
+type LoggingContext = {|
+  serverVersion: ZulipVersion | null,
+|};
+
+let getter: (() => LoggingContext) | null = null;
+
+/**
+ * The logging context, or null if none is available.
+ *
+ * The null case should only happen if we haven't even finished importing
+ * our code yet, in particular the code that provides the Redux store.
+ */
+export const getLoggingContext = (): LoggingContext | null => getter && getter();
+
+/**
+ * Set the getter for the logging context.
+ *
+ * This is called once at startup, after the Redux store is created.
+ */
+// Effectively the getter will contain:
+//  * a reference to the Redux store, to get the state;
+//  * references to some selectors, to pick the relevant data out of the
+//    Redux state.
+// We don't want the logging code to have to know about either of those,
+// lest we create import cycles.  So the getter's implementation lives
+// completely elsewhere and gets injected here.
+export const provideLoggingContext = (newGetter: () => LoggingContext) => {
+  getter = newGetter;
+};

src/boot/store.js

@@ -16,6 +16,8 @@ import { REHYDRATE } from '../actionConstants';
 import rootReducer from './reducers';
 import ZulipAsyncStorage from './ZulipAsyncStorage';
 import createMigration from '../redux-persist-migrate/index';
+import { provideLoggingContext } from './loggingContext';
+import { tryGetActiveAccount } from '../account/accountsSelectors';
 
 // AsyncStorage.clear(); // use to reset storage during development
 
@@ -266,6 +268,10 @@ const store: Store<GlobalState, Action> = createStore(
   ),
 );
 
+provideLoggingContext(() => ({
+  serverVersion: tryGetActiveAccount(store.getState())?.zulipVersion ?? null,
+}));
+
 /**
  * A special identifier used by `remotedev-serialize`.
  *

src/common/PresenceStatusIndicator.js

@@ -3,12 +3,12 @@ import React, { PureComponent } from 'react';
 import { View } from 'react-native';
 import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';
 
-import type { PresenceState, User, UserStatusMapObject, Dispatch } from '../types';
+import type { PresenceState, UserOrBot, UserStatusMapObject, Dispatch } from '../types';
 import { createStyleSheet } from '../styles';
 import { connect } from '../react-redux';
 import { statusFromPresenceAndUserStatus } from '../utils/presence';
 import { getPresence, getUserStatus } from '../selectors';
-import { getUsersByEmail } from '../users/userSelectors';
+import { getAllUsersByEmail } from '../users/userSelectors';
 import { ensureUnreachable } from '../types';
 
 const styles = createStyleSheet({
@@ -72,7 +72,7 @@ const PresenceStatusIndicatorUnavailable = ({ style }: { style: ViewStyleProp })
 type PropsFromConnect = {|
   dispatch: Dispatch,
   presence: PresenceState,
-  usersByEmail: Map<string, User>,
+  allUsersByEmail: Map<string, UserOrBot>,
   userStatus: UserStatusMapObject,
 |};
 
@@ -95,10 +95,10 @@ type Props = $ReadOnly<{|
  */
 class PresenceStatusIndicator extends PureComponent<Props> {
   render() {
-    const { email, presence, style, hideIfOffline, usersByEmail, userStatus } = this.props;
+    const { email, presence, style, hideIfOffline, allUsersByEmail, userStatus } = this.props;
 
     const userPresence = presence[email];
-    const user = usersByEmail.get(email);
+    const user = allUsersByEmail.get(email);
 
     if (!user || !userPresence || !userPresence.aggregated) {
       return null;
@@ -132,6 +132,6 @@ class PresenceStatusIndicator extends PureComponent<Props> {
 
 export default connect(state => ({
   presence: getPresence(state),
-  usersByEmail: getUsersByEmail(state),
+  allUsersByEmail: getAllUsersByEmail(state),
   userStatus: getUserStatus(state),
 }))(PresenceStatusIndicator);

src/message/messagesActions.js

@@ -1,7 +1,7 @@
 /* @flow strict-local */
 import NavigationService from '../nav/NavigationService';
 import type { Narrow, Dispatch, GetState } from '../types';
-import { getAuth, getUsersById } from '../selectors';
+import { getAuth, getAllUsersById } from '../selectors';
 import { getMessageIdFromLink, getNarrowFromLink } from '../utils/internalLinks';
 import openLink from '../utils/openLink';
 import { navigateToChat } from '../nav/navActions';
@@ -28,10 +28,10 @@ export const messageLinkPress = (href: string) => async (
 ) => {
   const state = getState();
   const auth = getAuth(state);
-  const usersById = getUsersById(state);
+  const allUsersById = getAllUsersById(state);
   const streamsById = getStreamsById(state);
   const ownUserId = getOwnUserId(state);
-  const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById, ownUserId);
+  const narrow = getNarrowFromLink(href, auth.realm, allUsersById, streamsById, ownUserId);
   if (narrow) {
     const anchor = getMessageIdFromLink(href, auth.realm);
     dispatch(doNarrow(narrow, anchor));

src/notification/__tests__/notification-test.js

@@ -1,7 +1,7 @@
 // @flow strict-local
 import deepFreeze from 'deep-freeze';
 
-import type { User } from '../../api/modelTypes';
+import type { UserOrBot } from '../../api/modelTypes';
 import type { JSONableDict } from '../../utils/jsonable';
 import { getNarrowFromNotificationData } from '..';
 import { topicNarrow, pmNarrowFromEmail, pmNarrowFromEmails } from '../../utils/narrow';
@@ -11,7 +11,7 @@ import { fromAPNsImpl as extractIosNotificationData } from '../extract';
 import objectEntries from '../../utils/objectEntries';
 
 describe('getNarrowFromNotificationData', () => {
-  const DEFAULT_MAP = new Map<number, User>();
+  const DEFAULT_MAP = new Map<number, UserOrBot>();
   const ownUserId = eg.selfUser.user_id;
 
   test('unknown notification data returns null', () => {
@@ -42,7 +42,7 @@ describe('getNarrowFromNotificationData', () => {
 
   test('on notification for a group message returns a group narrow', () => {
     const users = [eg.selfUser, eg.makeUser(), eg.makeUser(), eg.makeUser()];
-    const usersById: Map<number, User> = new Map(users.map(u => [u.user_id, u]));
+    const allUsersById: Map<number, UserOrBot> = new Map(users.map(u => [u.user_id, u]));
 
     const notification = {
       recipient_type: 'private',
@@ -51,7 +51,7 @@ describe('getNarrowFromNotificationData', () => {
 
     const expectedNarrow = pmNarrowFromEmails(users.slice(1).map(u => u.email));
 
-    const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId);
+    const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId);
 
     expect(narrow).toEqual(expectedNarrow);
   });

src/notification/extract.js

@@ -186,7 +186,11 @@ export const fromAPNsImpl = (rawData: JSONableDict): Notification | void => {
       if (ids.some(id => Number.isNaN(id))) {
         throw err('invalid');
       }
-      return { recipient_type: 'private', pm_users: ids.sort().join(','), ...realm_uri_obj };
+      return {
+        recipient_type: 'private',
+        pm_users: ids.sort((a, b) => a - b).join(','),
+        ...realm_uri_obj,
+      };
     }
 
     if (typeof sender_email !== 'string') {

src/notification/index.js

@@ -3,7 +3,7 @@ import { DeviceEventEmitter, NativeModules, Platform, PushNotificationIOS } from
 import NotificationsIOS from 'react-native-notifications';
 
 import type { Notification } from './types';
-import type { Auth, Dispatch, Identity, Narrow, User } from '../types';
+import type { Auth, Dispatch, Identity, Narrow, UserOrBot } from '../types';
 import { topicNarrow, pmNarrowFromEmail, pmNarrowFromEmails } from '../utils/narrow';
 import type { JSONable, JSONableDict } from '../utils/jsonable';
 import * as api from '../api';
@@ -85,7 +85,7 @@ export const getAccountFromNotificationData = (
 
 export const getNarrowFromNotificationData = (
   data: Notification,
-  usersById: Map<number, User>,
+  allUsersById: Map<number, UserOrBot>,
   ownUserId: number,
 ): Narrow | null => {
   if (!data.recipient_type) {
@@ -106,7 +106,7 @@ export const getNarrowFromNotificationData = (
   }
 
   const ids = data.pm_users.split(',').map(s => parseInt(s, 10));
-  const users = pmKeyRecipientsFromIds(ids, usersById, ownUserId);
+  const users = pmKeyRecipientsFromIds(ids, allUsersById, ownUserId);
   return users && pmNarrowFromEmails(users.map(u => u.email));
 };
 

src/notification/notificationActions.js

@@ -14,7 +14,7 @@ import { getAuth, getActiveAccount } from '../selectors';
 import { getSession, getAccounts } from '../directSelectors';
 import { GOT_PUSH_TOKEN, ACK_PUSH_TOKEN, UNACK_PUSH_TOKEN } from '../actionConstants';
 import { identityOfAccount, authOfAccount } from '../account/accountMisc';
-import { getOwnUserId, getUsersById } from '../users/userSelectors';
+import { getAllUsersById, getOwnUserId } from '../users/userSelectors';
 import { doNarrow } from '../message/messagesActions';
 import { accountSwitch } from '../account/accountActions';
 import { getIdentities } from '../account/accountsSelectors';
@@ -52,7 +52,7 @@ export const narrowToNotification = (data: ?Notification) => (
     return;
   }
 
-  const narrow = getNarrowFromNotificationData(data, getUsersById(state), getOwnUserId(state));
+  const narrow = getNarrowFromNotificationData(data, getAllUsersById(state), getOwnUserId(state));
   if (narrow) {
     dispatch(doNarrow(narrow));
   }

src/typing/__tests__/typingSelectors-test.js

@@ -34,10 +34,7 @@ describe('getCurrentTypingUsers', () => {
     const user1 = eg.makeUser();
     const user2 = eg.makeUser();
 
-    const normalizedRecipients = normalizeRecipientsAsUserIds([
-      { user_id: user1.user_id },
-      { user_id: user2.user_id },
-    ]);
+    const normalizedRecipients = normalizeRecipientsAsUserIds([user1.user_id, user2.user_id]);
 
     const state = eg.reduxState({
       typing: {
@@ -57,7 +54,7 @@ describe('getCurrentTypingUsers', () => {
   test('when in private narrow but different user is typing return NULL_ARRAY', () => {
     const user1 = eg.makeUser();
     const user2 = eg.makeUser();
-    const normalizedRecipients = normalizeRecipientsAsUserIds([{ user_id: user1.user_id }]);
+    const normalizedRecipients = normalizeRecipientsAsUserIds([user1.user_id]);
 
     const state = eg.reduxState({
       typing: {
@@ -76,8 +73,8 @@ describe('getCurrentTypingUsers', () => {
     const anotherUser = eg.makeUser();
 
     const normalizedRecipients = normalizeRecipientsAsUserIds([
-      { user_id: expectedUser.user_id },
-      { user_id: anotherUser.user_id },
+      expectedUser.user_id,
+      anotherUser.user_id,
     ]);
     const state = eg.reduxState({
       typing: {

src/typing/typingReducer.js

@@ -21,7 +21,7 @@ const eventTypingStart = (state, action) => {
   }
 
   const normalizedRecipients: string = normalizeRecipientsAsUserIdsSansMe(
-    action.recipients,
+    action.recipients.map(r => r.user_id),
     action.ownUserId,
   );
   const previousTypingUsers = state[normalizedRecipients] || { userIds: [] };
@@ -48,7 +48,7 @@ const eventTypingStart = (state, action) => {
 
 const eventTypingStop = (state, action) => {
   const normalizedRecipients: string = normalizeRecipientsAsUserIdsSansMe(
-    action.recipients,
+    action.recipients.map(r => r.user_id),
     action.ownUserId,
   );
   const previousTypingUsers = state[normalizedRecipients];

src/typing/typingSelectors.js

@@ -23,7 +23,7 @@ export const getCurrentTypingUsers: Selector<$ReadOnlyArray<UserOrBot>, Narrow>
       if (userId === undefined) {
         throw new Error(`Narrow contains email '${email}' that does not map to any user.`);
       }
-      return { user_id: userId };
+      return userId;
     });
     const normalizedRecipients = normalizeRecipientsAsUserIds(recipients);
     const currentTyping = typing[normalizedRecipients];

src/utils/__tests__/internalLinks-test.js

@@ -1,6 +1,6 @@
 /* @flow strict-local */
 
-import type { User } from '../../api/modelTypes';
+import type { UserOrBot } from '../../api/modelTypes';
 import { streamNarrow, topicNarrow, pmNarrowFromEmails, STARRED_NARROW } from '../narrow';
 import {
   isInternalLink,
@@ -125,7 +125,7 @@ describe('decodeHashComponent', () => {
 
 describe('getNarrowFromLink', () => {
   const [userB, userC] = [eg.makeUser(), eg.makeUser()];
-  const usersById: Map<number, User> = new Map(
+  const allUsersById: Map<number, UserOrBot> = new Map(
     [eg.selfUser, userB, userC].map(u => [u.user_id, u]),
   );
 
@@ -135,7 +135,7 @@ describe('getNarrowFromLink', () => {
     getNarrowFromLink(
       url,
       new URL('https://example.com'),
-      usersById,
+      allUsersById,
       new Map(streams.map(s => [s.stream_id, s])),
       eg.selfUser.user_id,
     );
@@ -270,7 +270,7 @@ describe('getNarrowFromLink', () => {
   });
 
   test('if any of the user ids are not found: return null', () => {
-    const otherId = 1 + Math.max(...usersById.keys());
+    const otherId = 1 + Math.max(...allUsersById.keys());
     const ids = `${userB.user_id},${otherId}`;
     expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(null);
   });