diff --git a/src/account-info/AccountDetailsScreen.js b/src/account-info/AccountDetailsScreen.js index 0ad805d8968..ca90c04f313 100644 --- a/src/account-info/AccountDetailsScreen.js +++ b/src/account-info/AccountDetailsScreen.js @@ -7,7 +7,7 @@ import { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; import { Screen, ZulipButton, Label } from '../common'; import { IconPrivateChat } from '../common/Icons'; -import { pmNarrowFromEmail } from '../utils/narrow'; +import { pm1to1NarrowFromUser } from '../utils/narrow'; import AccountDetails from './AccountDetails'; import { doNarrow } from '../actions'; import { getUserIsActive, getUserForId } from '../users/userSelectors'; @@ -43,7 +43,7 @@ type Props = $ReadOnly<{| class AccountDetailsScreen extends PureComponent { handleChatPress = () => { const { user, dispatch } = this.props; - dispatch(doNarrow(pmNarrowFromEmail(user.email))); + dispatch(doNarrow(pm1to1NarrowFromUser(user))); }; render() { diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 337af9263f7..f4700739a2a 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -6,9 +6,9 @@ import narrowsReducer from '../narrowsReducer'; import { HOME_NARROW, HOME_NARROW_STR, - pmNarrowFromEmail, + pm1to1NarrowFromUser, ALL_PRIVATE_NARROW_STR, - pmNarrowFromEmails, + pmNarrowFromUsersUnsafe, streamNarrow, topicNarrow, STARRED_NARROW_STR, @@ -23,10 +23,8 @@ import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../../anchor'; import * as eg from '../../__tests__/lib/exampleData'; describe('narrowsReducer', () => { - const privateNarrowStr = JSON.stringify(pmNarrowFromEmail(eg.otherUser.email)); - const groupNarrowStr = JSON.stringify( - pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]), - ); + const privateNarrowStr = JSON.stringify(pm1to1NarrowFromUser(eg.otherUser)); + const groupNarrowStr = JSON.stringify(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); const streamNarrowStr = JSON.stringify(streamNarrow(eg.stream.name)); const egTopic = eg.streamMessage().subject; const topicNarrowStr = JSON.stringify(topicNarrow(eg.stream.name, egTopic)); @@ -147,7 +145,7 @@ describe('narrowsReducer', () => { }); test('message sent to self is stored correctly', () => { - const narrowWithSelfStr = JSON.stringify(pmNarrowFromEmail(eg.selfUser.email)); + const narrowWithSelfStr = JSON.stringify(pm1to1NarrowFromUser(eg.selfUser)); const initialState = Immutable.Map({ [HOME_NARROW_STR]: [], [narrowWithSelfStr]: [], @@ -373,7 +371,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, anchor: 2, - narrow: pmNarrowFromEmail(eg.otherUser.email), + narrow: pm1to1NarrowFromUser(eg.otherUser), messages: [], numBefore: 100, numAfter: 100, @@ -383,7 +381,7 @@ describe('narrowsReducer', () => { const expectedState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2, 3], - [JSON.stringify(pmNarrowFromEmail(eg.otherUser.email))]: [], + [JSON.stringify(pm1to1NarrowFromUser(eg.otherUser))]: [], }); const newState = narrowsReducer(initialState, action); diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 3ffc90b4bea..70d5f0388a5 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -15,7 +15,7 @@ import { streamNarrow, topicNarrow, STARRED_NARROW, - pmNarrowFromEmails, + pmNarrowFromUsersUnsafe, } from '../../utils/narrow'; import { NULL_SUBSCRIPTION } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; @@ -293,7 +293,7 @@ describe('isNarrowValid', () => { streams: [], users: [john, mark], }); - const narrow = pmNarrowFromEmails([john.email, mark.email]); + const narrow = pmNarrowFromUsersUnsafe([john, mark]); const result = isNarrowValid(state, narrow); @@ -312,7 +312,7 @@ describe('isNarrowValid', () => { streams: [], users: [john], }); - const narrow = pmNarrowFromEmails([john.email, mark.email]); + const narrow = pmNarrowFromUsersUnsafe([john, mark]); const result = isNarrowValid(state, narrow); diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index 9132e52dfd2..d93deef2c8d 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -1,3 +1,4 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; import getComposeInputPlaceholder from '../getComposeInputPlaceholder'; @@ -5,67 +6,44 @@ import { pmNarrowFromEmail, streamNarrow, topicNarrow, - pmNarrowFromEmails, + pmNarrowFromUsersUnsafe, } from '../../utils/narrow'; +import * as eg from '../../__tests__/lib/exampleData'; describe('getComposeInputPlaceholder', () => { - test('returns "Message @ThisPerson" object for person narrow', () => { - const narrow = deepFreeze(pmNarrowFromEmail('abc@zulip.com')); - - const ownEmail = 'hamlet@zulip.com'; - - const usersByEmail = new Map([ - [ - 'abc@zulip.com', - { - id: 23, - email: 'abc@zulip.com', - full_name: 'ABC', - }, - ], - [ - 'xyz@zulip.com', - { - id: 22, - email: 'xyz@zulip.com', - full_name: 'XYZ', - }, - ], - ]); + const usersByEmail = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.email, u])); + const ownEmail = eg.selfUser.email; + test('returns "Message @ThisPerson" object for person narrow', () => { + const narrow = deepFreeze(pmNarrowFromEmail(eg.otherUser.email)); const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); - expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: '@ABC' } }); + expect(placeholder).toEqual({ + text: 'Message {recipient}', + values: { recipient: `@${eg.otherUser.full_name}` }, + }); }); test('returns "Jot down something" object for self narrow', () => { - const narrow = deepFreeze(pmNarrowFromEmail('abc@zulip.com')); - - const ownEmail = 'abc@zulip.com'; - - const placeholder = getComposeInputPlaceholder(narrow, ownEmail); + const narrow = deepFreeze(pmNarrowFromEmail(eg.selfUser.email)); + const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Jot down something' }); }); test('returns "Message #streamName" for stream narrow', () => { const narrow = deepFreeze(streamNarrow('Denmark')); - - const placeholder = getComposeInputPlaceholder(narrow); + const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: '#Denmark' } }); }); test('returns properly for topic narrow', () => { const narrow = deepFreeze(topicNarrow('Denmark', 'Copenhagen')); - - const placeholder = getComposeInputPlaceholder(narrow); - expect(placeholder).toEqual({ - text: 'Reply', - }); + const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); + expect(placeholder).toEqual({ text: 'Reply' }); }); test('returns "Message group" object for group narrow', () => { - const narrow = deepFreeze(pmNarrowFromEmails(['abc@zulip.com, xyz@zulip.com'])); - - const placeholder = getComposeInputPlaceholder(narrow); + const narrow = deepFreeze(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); + const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Message group' }); }); }); diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 3254ee0d98b..c48bf1ed312 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -4,7 +4,7 @@ import deepFreeze from 'deep-freeze'; import type { UserOrBot } from '../../api/modelTypes'; import type { JSONableDict } from '../../utils/jsonable'; import { getNarrowFromNotificationData } from '..'; -import { topicNarrow, pmNarrowFromEmail, pmNarrowFromEmails } from '../../utils/narrow'; +import { topicNarrow, pmNarrowFromEmail, pmNarrowFromUsersUnsafe } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; import { fromAPNsImpl as extractIosNotificationData } from '../extract'; @@ -49,7 +49,7 @@ describe('getNarrowFromNotificationData', () => { pm_users: users.map(u => u.user_id).join(','), }; - const expectedNarrow = pmNarrowFromEmails(users.slice(1).map(u => u.email)); + const expectedNarrow = pmNarrowFromUsersUnsafe(users.slice(1)); const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId); @@ -61,9 +61,9 @@ describe('getNarrowFromNotificationData', () => { recipient_type: 'private', pm_users: '1,2,4', }; - const usersById = new Map(); + const allUsersById = new Map(); - const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId); + const narrow = getNarrowFromNotificationData(notification, allUsersById, ownUserId); expect(narrow).toBe(null); }); diff --git a/src/notification/index.js b/src/notification/index.js index db5feed6488..03e0b6a7ad0 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -4,7 +4,7 @@ import NotificationsIOS from 'react-native-notifications'; import type { Notification } from './types'; import type { Auth, Dispatch, Identity, Narrow, UserOrBot } from '../types'; -import { topicNarrow, pmNarrowFromEmail, pmNarrowFromEmails } from '../utils/narrow'; +import { topicNarrow, pmNarrowFromEmail, pmNarrowFromUsers } from '../utils/narrow'; import type { JSONable, JSONableDict } from '../utils/jsonable'; import * as api from '../api'; import * as logging from '../utils/logging'; @@ -107,7 +107,7 @@ export const getNarrowFromNotificationData = ( const ids = data.pm_users.split(',').map(s => parseInt(s, 10)); const users = pmKeyRecipientsFromIds(ids, allUsersById, ownUserId); - return users && pmNarrowFromEmails(users.map(u => u.email)); + return users === null ? null : pmNarrowFromUsers(users); }; const getInitialNotification = async (): Promise => { diff --git a/src/pm-conversations/GroupPmConversationItem.js b/src/pm-conversations/GroupPmConversationItem.js index 65e844c2c9f..13d7863ce4b 100644 --- a/src/pm-conversations/GroupPmConversationItem.js +++ b/src/pm-conversations/GroupPmConversationItem.js @@ -14,44 +14,36 @@ const componentStyles = createStyleSheet({ }, }); -type Props = $ReadOnly<{| - email: string, - usersByEmail: Map, +type Props = $ReadOnly<{| + users: U, unreadCount: number, - onPress: (emails: string) => void, + onPress: (users: U) => void, |}>; /** * A list item describing one group PM conversation. * */ -export default class GroupPmConversationItem extends PureComponent { +export default class GroupPmConversationItem> extends PureComponent< + Props, +> { handlePress = () => { - const { email, onPress } = this.props; - onPress(email); + const { users, onPress } = this.props; + onPress(users); }; render() { - const { email, usersByEmail, unreadCount } = this.props; - const allUsers = email.split(',').map(e => usersByEmail.get(e)); - - const allUsersFound = allUsers.every(user => user); - - if (!allUsersFound) { - return null; - } - - // $FlowFixMe Flow doesn't see the `every` check above. - const allNames = allUsers.map(user => user.full_name); + const { users, unreadCount } = this.props; + const names = users.map(user => user.full_name); return ( - + diff --git a/src/pm-conversations/PmConversationList.js b/src/pm-conversations/PmConversationList.js index f6d335bc9be..5866ed8fef3 100644 --- a/src/pm-conversations/PmConversationList.js +++ b/src/pm-conversations/PmConversationList.js @@ -4,7 +4,8 @@ import { FlatList } from 'react-native'; import type { Dispatch, PmConversationData, UserOrBot } from '../types'; import { createStyleSheet } from '../styles'; -import { pmNarrowFromEmail, pmNarrowFromEmails } from '../utils/narrow'; +import { type PmKeyUsers } from '../utils/recipient'; +import { pm1to1NarrowFromUser, pmNarrowFromUsers } from '../utils/narrow'; import UserItem from '../users/UserItem'; import GroupPmConversationItem from './GroupPmConversationItem'; import { doNarrow } from '../actions'; @@ -19,7 +20,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| dispatch: Dispatch, conversations: PmConversationData[], - usersByEmail: Map, + allUsersByEmail: Map, |}>; /** @@ -27,43 +28,37 @@ type Props = $ReadOnly<{| * */ export default class PmConversationList extends PureComponent { handleUserNarrow = (user: UserOrBot) => { - this.props.dispatch(doNarrow(pmNarrowFromEmail(user.email))); + this.props.dispatch(doNarrow(pm1to1NarrowFromUser(user))); }; - handleGroupNarrow = (email: string) => { - this.props.dispatch(doNarrow(pmNarrowFromEmails(email.split(',')))); + handleGroupNarrow = (users: PmKeyUsers) => { + this.props.dispatch(doNarrow(pmNarrowFromUsers(users))); }; render() { - const { conversations, usersByEmail } = this.props; + const { conversations } = this.props; return ( item.recipients} + keyExtractor={item => item.key} renderItem={({ item }) => { - if (item.recipients.indexOf(',') === -1) { - const user = usersByEmail.get(item.recipients); - - if (!user) { - return null; - } - + const users = item.keyRecipients; + if (users.length === 1) { return ( - + + ); + } else { + return ( + ); } - - return ( - - ); }} /> ); diff --git a/src/pm-conversations/PmConversationsCard.js b/src/pm-conversations/PmConversationsCard.js index 7ad72cd7ced..bc1872db94f 100644 --- a/src/pm-conversations/PmConversationsCard.js +++ b/src/pm-conversations/PmConversationsCard.js @@ -43,7 +43,7 @@ type Props = $ReadOnly<{| dispatch: Dispatch, conversations: PmConversationData[], - usersByEmail: Map, + allUsersByEmail: Map, |}>; /** @@ -54,7 +54,7 @@ class PmConversationsCard extends PureComponent { context: ThemeData; render() { - const { dispatch, conversations, usersByEmail } = this.props; + const { dispatch, conversations, allUsersByEmail } = this.props; return ( @@ -85,7 +85,7 @@ class PmConversationsCard extends PureComponent { )} @@ -95,5 +95,5 @@ class PmConversationsCard extends PureComponent { export default connect(state => ({ conversations: getRecentConversations(state), - usersByEmail: getAllUsersByEmail(state), + allUsersByEmail: getAllUsersByEmail(state), }))(PmConversationsCard); diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index 7759bfa8e52..cd1dfe96c47 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -6,8 +6,8 @@ import { ALL_PRIVATE_NARROW_STR } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; describe('getRecentConversations', () => { - const userJohn = { ...eg.makeUser({ name: 'John' }), user_id: 1 }; - const userMark = { ...eg.makeUser({ name: 'Mark' }), user_id: 2 }; + const userJohn = eg.makeUser(); + const userMark = eg.makeUser(); test('when no messages, return no conversations', () => { const state = eg.reduxState({ @@ -33,7 +33,7 @@ describe('getRecentConversations', () => { const meAndMarkPm = eg.pmMessage({ id: 2, recipients: [eg.selfUser, userMark] }); const meAndJohnPm2 = eg.pmMessage({ id: 3, recipients: [eg.selfUser, userJohn] }); const meOnlyPm = eg.pmMessage({ id: 4, recipients: [eg.selfUser] }); - const meJohnAndMarkPm = eg.pmMessage({ id: 0, recipients: [eg.selfUser, userMark, userJohn] }); + const meJohnAndMarkPm = eg.pmMessage({ id: 0, recipients: [eg.selfUser, userJohn, userMark] }); const state = eg.reduxState({ realm: eg.realmState({ email: eg.selfUser.email }), @@ -84,29 +84,29 @@ describe('getRecentConversations', () => { const expectedResult = [ { - ids: eg.selfUser.user_id.toString(), - recipients: eg.selfUser.email, + key: eg.selfUser.user_id.toString(), + keyRecipients: [eg.selfUser], msgId: meOnlyPm.id, unread: 1, }, { - ids: userJohn.user_id.toString(), - recipients: userJohn.email, + key: userJohn.user_id.toString(), + keyRecipients: [userJohn], msgId: meAndJohnPm2.id, unread: 2, }, { - ids: userMark.user_id.toString(), - recipients: userMark.email, + key: userMark.user_id.toString(), + keyRecipients: [userMark], msgId: meAndMarkPm.id, unread: 1, }, { - ids: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] + key: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] .sort((a, b) => a - b) .map(String) .join(','), - recipients: [userJohn.email, userMark.email].sort().join(','), + keyRecipients: [userJohn, userMark].sort((a, b) => a.user_id - b.user_id), msgId: meJohnAndMarkPm.id, unread: 1, }, @@ -178,29 +178,29 @@ describe('getRecentConversations', () => { const expectedResult = [ { - ids: eg.selfUser.user_id.toString(), - recipients: eg.selfUser.email, + key: eg.selfUser.user_id.toString(), + keyRecipients: [eg.selfUser], msgId: meOnlyPm.id, unread: 1, }, { - ids: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] + key: [eg.selfUser.user_id, userJohn.user_id, userMark.user_id] .sort((a, b) => a - b) .map(String) .join(','), - recipients: [userJohn.email, userMark.email].sort().join(','), + keyRecipients: [userJohn, userMark].sort((a, b) => a.user_id - b.user_id), msgId: meJohnAndMarkPm.id, unread: 1, }, { - ids: userJohn.user_id.toString(), - recipients: userJohn.email, + key: userJohn.user_id.toString(), + keyRecipients: [userJohn], msgId: meAndJohnPm2.id, unread: 2, }, { - ids: userMark.user_id.toString(), - recipients: userMark.email, + key: userMark.user_id.toString(), + keyRecipients: [userMark], msgId: meAndMarkPm2.id, unread: 1, }, diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index cfc6e2f4f33..72b771e283c 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -3,44 +3,37 @@ import { createSelector } from 'reselect'; import type { Message, PmConversationData, Selector, User } from '../types'; import { getPrivateMessages } from '../message/messageSelectors'; -import { getOwnUser } from '../users/userSelectors'; +import { getAllUsersById, getOwnUser } from '../users/userSelectors'; import { getUnreadByPms, getUnreadByHuddles } from '../unread/unreadSelectors'; -import { - normalizeRecipientsSansMe, - pmUnreadsKeyFromMessage, - recipientsOfPrivateMessage, -} from '../utils/recipient'; +import { pmUnreadsKeyFromMessage, pmKeyRecipientUsersFromMessage } from '../utils/recipient'; export const getRecentConversations: Selector = createSelector( getOwnUser, getPrivateMessages, getUnreadByPms, getUnreadByHuddles, + getAllUsersById, ( ownUser: User, messages: Message[], unreadPms: { [number]: number }, unreadHuddles: { [string]: number }, + allUsersById, ): PmConversationData[] => { - const recipients = messages.map(msg => ({ - // Note this can be a different set of users from those in `emails` below. - ids: pmUnreadsKeyFromMessage(msg, ownUser.user_id), - - // The users represented in this `emails` string are sorted by email address. - emails: normalizeRecipientsSansMe(recipientsOfPrivateMessage(msg), ownUser.email), - - msgId: msg.id, - })); + const recipients = messages + .map(msg => { + // Note this can be a different set of users from those in `keyRecipients`. + const unreadsKey = pmUnreadsKeyFromMessage(msg, ownUser.user_id); + const keyRecipients = pmKeyRecipientUsersFromMessage(msg, allUsersById, ownUser.user_id); + return keyRecipients === null ? null : { unreadsKey, keyRecipients, msgId: msg.id }; + }) + .filter(Boolean); const latestByRecipient = new Map(); recipients.forEach(recipient => { - const prev = latestByRecipient.get(recipient.emails); + const prev = latestByRecipient.get(recipient.unreadsKey); if (!prev || recipient.msgId > prev.msgId) { - latestByRecipient.set(recipient.emails, { - ids: recipient.ids, - recipients: recipient.emails, - msgId: recipient.msgId, - }); + latestByRecipient.set(recipient.unreadsKey, recipient); } }); @@ -49,7 +42,9 @@ export const getRecentConversations: Selector = createSele ); return sortedByMostRecent.map(recipient => ({ - ...recipient, + key: recipient.unreadsKey, + keyRecipients: recipient.keyRecipients, + msgId: recipient.msgId, unread: // This business of looking in one place and then the other is kind // of messy. Fortunately it always works, because the key spaces @@ -58,7 +53,7 @@ export const getRecentConversations: Selector = createSele /* $FlowFixMe: The keys of unreadPms are logically numbers, but because it's an object they end up converted to strings, so this access with string keys works. We should probably use a Map for this and similar maps. */ - unreadPms[recipient.ids] || unreadHuddles[recipient.ids], + unreadPms[recipient.unreadsKey] || unreadHuddles[recipient.unreadsKey], })); }, ); diff --git a/src/title/__tests__/titleSelectors-test.js b/src/title/__tests__/titleSelectors-test.js index 2ace21ff47b..c944bdaafce 100644 --- a/src/title/__tests__/titleSelectors-test.js +++ b/src/title/__tests__/titleSelectors-test.js @@ -1,45 +1,34 @@ -import deepFreeze from 'deep-freeze'; +/* @flow strict-local */ import { DEFAULT_TITLE_BACKGROUND_COLOR, getTitleBackgroundColor } from '../titleSelectors'; -import { pmNarrowFromEmails, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow'; - -const subscriptions = [{ name: 'all', color: '#fff' }, { name: 'announce', color: '#000' }]; +import { pmNarrowFromUsersUnsafe, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow'; +import * as eg from '../../__tests__/lib/exampleData'; describe('getTitleBackgroundColor', () => { - test('return default for screens other than chat, i.e narrow is undefined', () => { - const state = deepFreeze({ - subscriptions, - }); + const exampleColor = '#fff'; + const state = eg.reduxState({ + subscriptions: [{ ...eg.makeSubscription({ stream: eg.stream }), color: exampleColor }], + }); + test('return default for screens other than chat, i.e narrow is undefined', () => { expect(getTitleBackgroundColor(state, undefined)).toEqual(DEFAULT_TITLE_BACKGROUND_COLOR); }); test('return stream color for stream and topic narrow', () => { - const state = deepFreeze({ - subscriptions, - }); - - expect(getTitleBackgroundColor(state, streamNarrow('all'))).toEqual('#fff'); + expect(getTitleBackgroundColor(state, streamNarrow(eg.stream.name))).toEqual(exampleColor); }); test('return null stream color for invalid stream or unknown subscriptions', () => { - const state = deepFreeze({ - subscriptions, - }); - - expect(getTitleBackgroundColor(state, streamNarrow('feedback'))).toEqual('gray'); + const unknownStream = eg.makeStream(); + expect(getTitleBackgroundColor(state, streamNarrow(unknownStream.name))).toEqual('gray'); }); test('return default for non topic/stream narrow', () => { - const state = deepFreeze({ - subscriptions, - }); - - expect(getTitleBackgroundColor(state, pmNarrowFromEmail('abc@zulip.com'))).toEqual( + expect(getTitleBackgroundColor(state, pmNarrowFromEmail(eg.otherUser.email))).toEqual( DEFAULT_TITLE_BACKGROUND_COLOR, ); expect( - getTitleBackgroundColor(state, pmNarrowFromEmails(['abc@zulip.com', 'def@zulip.com'])), + getTitleBackgroundColor(state, pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])), ).toEqual(DEFAULT_TITLE_BACKGROUND_COLOR); }); }); diff --git a/src/types.js b/src/types.js index 93f7f63e740..9300b42fd1b 100644 --- a/src/types.js +++ b/src/types.js @@ -5,6 +5,7 @@ import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/Style import type { SubsetProperties } from './generics'; import type { Auth, Topic, Message, ReactionType } from './api/apiTypes'; import type { ZulipVersion } from './utils/zulipVersion'; +import type { PmKeyUsers } from './utils/recipient'; export type * from './generics'; export type * from './reduxTypes'; @@ -318,9 +319,9 @@ export type TabNavigationOptionsPropsType = {| * Summary of a PM conversation (either 1:1 or group PMs). */ export type PmConversationData = {| - ids: string, + key: string, + keyRecipients: PmKeyUsers, msgId: number, - recipients: string, unread: number, |}; diff --git a/src/typing/__tests__/typingSelectors-test.js b/src/typing/__tests__/typingSelectors-test.js index 5cabaee5d68..fd060bf9374 100644 --- a/src/typing/__tests__/typingSelectors-test.js +++ b/src/typing/__tests__/typingSelectors-test.js @@ -2,7 +2,7 @@ import type { GlobalState } from '../../types'; import { getCurrentTypingUsers } from '../typingSelectors'; -import { HOME_NARROW, pmNarrowFromEmail, pmNarrowFromEmails } from '../../utils/narrow'; +import { HOME_NARROW, pm1to1NarrowFromUser, pmNarrowFromUsersUnsafe } from '../../utils/narrow'; import { NULL_ARRAY } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; import { normalizeRecipientsAsUserIds } from '../../utils/recipient'; @@ -25,7 +25,7 @@ describe('getCurrentTypingUsers', () => { users: [expectedUser], }); - const typingUsers = getCurrentTypingUsers(state, pmNarrowFromEmail(expectedUser.email)); + const typingUsers = getCurrentTypingUsers(state, pm1to1NarrowFromUser(expectedUser)); expect(typingUsers).toEqual([expectedUser]); }); @@ -43,10 +43,7 @@ describe('getCurrentTypingUsers', () => { users: [user1, user2], }); - const typingUsers = getCurrentTypingUsers( - state, - pmNarrowFromEmails([user1.email, user2.email]), - ); + const typingUsers = getCurrentTypingUsers(state, pmNarrowFromUsersUnsafe([user1, user2])); expect(typingUsers).toEqual([user1, user2]); }); @@ -63,7 +60,7 @@ describe('getCurrentTypingUsers', () => { users: [user1, user2], }); - const typingUsers = getCurrentTypingUsers(state, pmNarrowFromEmail(user2.email)); + const typingUsers = getCurrentTypingUsers(state, pm1to1NarrowFromUser(user2)); expect(typingUsers).toEqual(NULL_ARRAY); }); @@ -85,7 +82,7 @@ describe('getCurrentTypingUsers', () => { const typingUsers = getCurrentTypingUsers( state, - pmNarrowFromEmails([expectedUser.email, anotherUser.email]), + pmNarrowFromUsersUnsafe([expectedUser, anotherUser]), ); expect(typingUsers).toEqual([expectedUser]); @@ -99,7 +96,7 @@ describe('getCurrentTypingUsers', () => { }); const getTypingUsers = () => - getCurrentTypingUsers(state, pmNarrowFromEmail(deactivatedUser.email)); + getCurrentTypingUsers(state, pm1to1NarrowFromUser(deactivatedUser)); expect(getTypingUsers).not.toThrow(); expect(getTypingUsers()).toEqual([]); @@ -112,8 +109,7 @@ describe('getCurrentTypingUsers', () => { realm: eg.realmState({ crossRealmBots: [crossRealmBot] }), }); - const getTypingUsers = () => - getCurrentTypingUsers(state, pmNarrowFromEmail(crossRealmBot.email)); + const getTypingUsers = () => getCurrentTypingUsers(state, pm1to1NarrowFromUser(crossRealmBot)); expect(getTypingUsers).not.toThrow(); expect(getTypingUsers()).toEqual([]); diff --git a/src/unread/UnreadCards.js b/src/unread/UnreadCards.js index 352fd474ad1..98a64894bec 100644 --- a/src/unread/UnreadCards.js +++ b/src/unread/UnreadCards.js @@ -20,7 +20,7 @@ import { doNarrow } from '../actions'; type Props = $ReadOnly<{| conversations: PmConversationData[], dispatch: Dispatch, - usersByEmail: Map, + allUsersByEmail: Map, unreadStreamsAndTopics: UnreadStreamItem[], |}>; @@ -91,6 +91,6 @@ class UnreadCards extends PureComponent { export default connect(state => ({ conversations: getUnreadConversations(state), - usersByEmail: getAllUsersByEmail(state), + allUsersByEmail: getAllUsersByEmail(state), unreadStreamsAndTopics: getUnreadStreamsAndTopicsSansMuted(state), }))(UnreadCards); diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 7cb9ff5558c..1b623973117 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -236,7 +236,7 @@ export const getUnreadCountForNarrow: Selector = createSelector( ( narrow, streams, - usersByEmail, + allUsersByEmail, ownEmail, unreadTotal, unreadStreams, @@ -278,7 +278,7 @@ export const getUnreadCountForNarrow: Selector = createSelector( if (isGroupPmNarrow(narrow)) { const userIds = [...narrow[0].operand.split(','), ownEmail] - .map(email => (usersByEmail.get(email) || NULL_USER).user_id) + .map(email => (allUsersByEmail.get(email) || NULL_USER).user_id) .sort((a, b) => a - b) .join(','); const unread = unreadHuddles.find(x => x.user_ids_string === userIds); @@ -286,7 +286,7 @@ export const getUnreadCountForNarrow: Selector = createSelector( } if (is1to1PmNarrow(narrow)) { - const sender = usersByEmail.get(narrow[0].operand); + const sender = allUsersByEmail.get(narrow[0].operand); if (!sender) { return 0; } diff --git a/src/user-groups/CreateGroupScreen.js b/src/user-groups/CreateGroupScreen.js index e5a181894ed..5f5a7543bf9 100644 --- a/src/user-groups/CreateGroupScreen.js +++ b/src/user-groups/CreateGroupScreen.js @@ -7,8 +7,14 @@ import type { Dispatch, User } from '../types'; import { connect } from '../react-redux'; import { Screen } from '../common'; import { doNarrow, navigateBack } from '../actions'; -import { pmNarrowFromEmails } from '../utils/narrow'; +import { pmNarrowFromUsers } from '../utils/narrow'; +import { pmKeyRecipientsFromUsers } from '../utils/recipient'; import UserPickerCard from '../user-picker/UserPickerCard'; +import { getOwnUserId } from '../users/userSelectors'; + +type SelectorProps = {| + +ownUserId: number, +|}; type Props = $ReadOnly<{| // Since we've put this screen in a stack-nav route config, and we @@ -18,6 +24,7 @@ type Props = $ReadOnly<{| navigation: NavigationStackProp, dispatch: Dispatch, + ...SelectorProps, |}>; type State = {| @@ -32,11 +39,9 @@ class CreateGroupScreen extends PureComponent { handleFilterChange = (filter: string) => this.setState({ filter }); handleCreateGroup = (selected: User[]) => { - const { dispatch } = this.props; - - const recipients = selected.map(user => user.email); + const { dispatch, ownUserId } = this.props; NavigationService.dispatch(navigateBack()); - dispatch(doNarrow(pmNarrowFromEmails(recipients))); + dispatch(doNarrow(pmNarrowFromUsers(pmKeyRecipientsFromUsers(selected, ownUserId)))); }; render() { @@ -49,4 +54,6 @@ class CreateGroupScreen extends PureComponent { } } -export default connect<{||}, _, _>()(CreateGroupScreen); +export default connect(state => ({ + ownUserId: getOwnUserId(state), +}))(CreateGroupScreen); diff --git a/src/users/UsersCard.js b/src/users/UsersCard.js index bc3582d0467..e5032e28f1e 100644 --- a/src/users/UsersCard.js +++ b/src/users/UsersCard.js @@ -5,7 +5,7 @@ import React, { PureComponent } from 'react'; import NavigationService from '../nav/NavigationService'; import type { Dispatch, PresenceState, User, UserOrBot } from '../types'; import { connect } from '../react-redux'; -import { pmNarrowFromEmail } from '../utils/narrow'; +import { pm1to1NarrowFromUser } from '../utils/narrow'; import UserList from './UserList'; import { getUsers, getPresence } from '../selectors'; import { navigateBack, doNarrow } from '../actions'; @@ -21,7 +21,7 @@ class UsersCard extends PureComponent { handleUserNarrow = (user: UserOrBot) => { const { dispatch } = this.props; NavigationService.dispatch(navigateBack()); - dispatch(doNarrow(pmNarrowFromEmail(user.email))); + dispatch(doNarrow(pm1to1NarrowFromUser(user))); }; render() { diff --git a/src/users/usersActions.js b/src/users/usersActions.js index f4e37a815ac..74e5cfbe39e 100644 --- a/src/users/usersActions.js +++ b/src/users/usersActions.js @@ -64,11 +64,11 @@ export const sendTypingStart = (narrow: Narrow) => async ( return; } - const usersByEmail = getAllUsersByEmail(getState()); + const allUsersByEmail = getAllUsersByEmail(getState()); const recipientIds = caseNarrowPartial(narrow, { pm: emails => emails, }).map(email => { - const user = usersByEmail.get(email); + const user = allUsersByEmail.get(email); if (!user) { throw new Error('unknown user'); } diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 47bb9ab59e2..b961d7ef124 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import type { UserOrBot } from '../../api/modelTypes'; -import { streamNarrow, topicNarrow, pmNarrowFromEmails, STARRED_NARROW } from '../narrow'; +import { streamNarrow, topicNarrow, pmNarrowFromUsersUnsafe, STARRED_NARROW } from '../narrow'; import { isInternalLink, isMessageLink, @@ -257,7 +257,7 @@ describe('getNarrowFromLink', () => { test('on group PM link', () => { const ids = `${userB.user_id},${userC.user_id}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual( - pmNarrowFromEmails([userB.email, userC.email]), + pmNarrowFromUsersUnsafe([userB, userC]), ); }); @@ -265,7 +265,7 @@ describe('getNarrowFromLink', () => { // The webapp doesn't generate these, but best to handle them anyway. const ids = `${eg.selfUser.user_id},${userB.user_id},${userC.user_id}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual( - pmNarrowFromEmails([userB.email, userC.email]), + pmNarrowFromUsersUnsafe([userB, userC]), ); }); @@ -282,7 +282,7 @@ describe('getNarrowFromLink', () => { test('on a message link', () => { const ids = `${userB.user_id},${userC.user_id}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group/near/2`)).toEqual( - pmNarrowFromEmails([userB.email, userC.email]), + pmNarrowFromUsersUnsafe([userB, userC]), ); expect(get('https://example.com/#narrow/stream/jest/topic/test/near/1')).toEqual( diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 81496b9689f..70be9f78ae9 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -5,8 +5,7 @@ import { isHomeNarrow, pmNarrowFromEmail, is1to1PmNarrow, - pmNarrowFromEmails, - isGroupPmNarrow, + pmNarrowFromUsersUnsafe, specialNarrow, isSpecialNarrow, ALL_PRIVATE_NARROW, @@ -41,55 +40,22 @@ describe('HOME_NARROW', () => { describe('pmNarrowFromEmail', () => { test('produces an one item list, pm-with operator and single email', () => { - expect(pmNarrowFromEmail('bob@example.com')).toEqual([ + expect(pmNarrowFromEmail(eg.otherUser.email)).toEqual([ { operator: 'pm-with', - operand: 'bob@example.com', + operand: eg.otherUser.email, }, ]); }); test('if operator is "pm-with" and only one email, then it is a private narrow', () => { expect(is1to1PmNarrow(HOME_NARROW)).toBe(false); - expect(is1to1PmNarrow(pmNarrowFromEmail('bob@example.com'))).toBe(true); + expect(is1to1PmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); expect( is1to1PmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com', - }, - ]), - ).toBe(true); - }); -}); - -describe('pmNarrowFromEmails', () => { - test('returns a narrow with specified recipients', () => { - expect(pmNarrowFromEmails(['bob@example.com', 'john@example.com'])).toEqual([ - { - operator: 'pm-with', - operand: 'bob@example.com,john@example.com', - }, - ]); - }); - - test('a group narrow is only private chat with more than one recipients', () => { - expect(isGroupPmNarrow(HOME_NARROW)).toBe(false); - expect(isGroupPmNarrow(pmNarrowFromEmail('bob@example.com'))).toBe(false); - expect(isGroupPmNarrow(pmNarrowFromEmails(['bob@example.com', 'john@example.com']))).toBe(true); - expect( - isGroupPmNarrow([ - { - operator: 'pm-with', - operand: 'bob@example.com', - }, - ]), - ).toBe(false); - expect( - isGroupPmNarrow([ - { - operator: 'pm-with', - operand: 'bob@example.com,john@example.com', + operand: eg.otherUser.email, }, ]), ).toBe(true); @@ -100,13 +66,13 @@ describe('isPmNarrow', () => { test('a private or group narrow is any "pm-with" narrow', () => { expect(isPmNarrow(undefined)).toBe(false); expect(isPmNarrow(HOME_NARROW)).toBe(false); - expect(isPmNarrow(pmNarrowFromEmail('bob@example.com'))).toBe(true); - expect(isPmNarrow(pmNarrowFromEmails(['bob@example.com', 'john@example.com']))).toBe(true); + expect(isPmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); + expect(isPmNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe(true); expect( isPmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com', + operand: eg.otherUser.email, }, ]), ).toBe(true); @@ -114,7 +80,7 @@ describe('isPmNarrow', () => { isPmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com,john@example.com', + operand: [eg.otherUser.email, eg.thirdUser.email].join(','), }, ]), ).toBe(true); @@ -127,10 +93,10 @@ describe('isStreamOrTopicNarrow', () => { expect(isStreamOrTopicNarrow(streamNarrow('some stream'))).toBe(true); expect(isStreamOrTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true); expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false); - expect(isStreamOrTopicNarrow(pmNarrowFromEmail('a@a.com'))).toBe(false); - expect( - isStreamOrTopicNarrow(pmNarrowFromEmails(['john@example.com', 'mark@example.com'])), - ).toBe(false); + expect(isStreamOrTopicNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(false); + expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe( + false, + ); expect(isStreamOrTopicNarrow(STARRED_NARROW)).toBe(false); }); }); @@ -255,7 +221,7 @@ describe('isMessageInNarrow', () => { ['group-PM, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], ['stream message', false, eg.streamMessage()], ]], - ['group-PM conversation', pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]), [ + ['group-PM conversation', pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]), [ ['matching group-PM, inbound', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], ['matching group-PM, outbound', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], ['1:1 within group, inbound', false, eg.pmMessage()], @@ -263,7 +229,7 @@ describe('isMessageInNarrow', () => { ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], ['stream message', false, eg.streamMessage()], ]], - ['group-PM conversation, including self', pmNarrowFromEmails([eg.selfUser.email, eg.otherUser.email, eg.thirdUser.email]), [ + ['group-PM conversation, including self', pmNarrowFromUsersUnsafe([eg.selfUser, eg.otherUser, eg.thirdUser]), [ ['matching group-PM, inbound', true, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], ['matching group-PM, outbound', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], ['1:1 within group, inbound', false, eg.pmMessage()], @@ -388,7 +354,7 @@ describe('getNarrowsForMessage', () => { expectedNarrows: [ HOME_NARROW, ALL_PRIVATE_NARROW, - pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]), + pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]), ], }, ]; @@ -421,7 +387,7 @@ describe('getNarrowForReply', () => { const message = eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser], }); - const expectedNarrow = pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]); + const expectedNarrow = pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]); const actualNarrow = getNarrowForReply(message, eg.selfUser); diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 055ceb0c5a8..3abbb7aaffe 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { addBreadcrumb } from '@sentry/react-native'; import type { Narrow, Stream, UserOrBot } from '../types'; -import { topicNarrow, streamNarrow, pmNarrowFromEmails, specialNarrow } from './narrow'; +import { topicNarrow, streamNarrow, specialNarrow, pmNarrowFromUsers } from './narrow'; import { pmKeyRecipientsFromIds } from './recipient'; import { isUrlOnRealm } from './url'; @@ -128,9 +128,14 @@ export const getNarrowFromLink = ( switch (type) { case 'pm': { + // TODO: This case is pretty useless in practice, due to basically a + // bug in the webapp: the URL that appears in the location bar for a + // group PM conversation excludes self, so it's unusable for anyone + // else. In particular this will foil you if, say, you try to give + // someone else in the conversation a link to a particular message. const ids = parsePmOperand(paths[1]); const users = pmKeyRecipientsFromIds(ids, allUsersById, ownUserId); - return users && pmNarrowFromEmails(users.map(u => u.email)); + return users === null ? null : pmNarrowFromUsers(users); } case 'topic': return topicNarrow(parseStreamOperand(paths[1], streamsById), parseTopicOperand(paths[3])); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 209c36aa509..bba9bf09192 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -2,12 +2,14 @@ import isEqual from 'lodash.isequal'; import unescape from 'lodash.unescape'; -import type { Narrow, Message, Outbox, User } from '../types'; +import type { Narrow, Message, Outbox, User, UserOrBot } from '../types'; import { normalizeRecipientsSansMe, pmKeyRecipientsFromMessage, recipientsOfPrivateMessage, streamNameOfStreamMessage, + type PmKeyRecipients, + type PmKeyUsers, } from './recipient'; export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => @@ -39,48 +41,86 @@ const pmNarrowByString = (emails: string): Narrow => [ /** * A PM narrow, either 1:1 or group. * - * The users represented in `emails` should agree, as a (multi)set, with - * `pmKeyRecipientsFromMessage`. But this isn't checked, and we've had bugs - * where they don't; some consumers of this data re-normalize to be sure. + * The list of users represented in `emails` must agree with what + * `pmKeyRecipientsFromMessage` would return. Effectively this means: + * * it actually comes from `pmKeyRecipientsFromMessage`, or one of its + * relatives in `src/utils/recipient.js`; + * * or it's a singleton list, which those would always be a no-op on. * - * They might not have a consistent sorting. (This would be good to fix.) - * Consumers of this data should sort for themselves when making comparisons. + * We enforce this by keeping this constructor private to this module, and + * only calling it from higher-level constructors whose input types enforce + * one of these conditions or the other. (Well, and one more which is only + * to be used in test code.) + * + * In the past, before this was checked and before it was done consistently, + * we had quite a lot of bugs due to different parts of our code + * accidentally disagreeing on whether to include the self-user, or on how + * to sort the list (by user ID vs. email), or neglecting to sort it at all. + */ +const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); + +/** + * DEPRECATED. Use `pm1to1NarrowFromUser` instead. + * + * This function is being replaced by `pm1to1NarrowFromUser`, as part of + * migrating from emails to user IDs to identify users. Don't add new uses + * of this function; use that one instead. */ -// Ideally, all callers should agree on how they're sorted, too. Because -// they don't, we have latent bugs (possibly a live one somewhere) where we -// can wind up with several distinct narrows that are actually the same -// group PM conversation. -// -// For example this happens if you have a group PM conversation where email -// and ID sorting don't happen to coincide; visit a group PM conversation -// from the main nav (either the unreads or PMs screen) -- which sorts by -// email; and then visit the same conversation from a recipient bar on the -// "all messages" narrow -- which sorts by ID. The Redux logs in the -// debugger will show two different entries in `state.narrows`. This bug is -// merely latent only because it doesn't (as far as we know) have any -// user-visible effect. -// -// Known call stacks: -// * OK, perilously, unsorted: CreateGroupScreen: the self user isn't -// offered in the UI, so effectively the list is filtered; can call -// with just one email, but happily this works out the same as pmNarrow -// * OK, email: PmConversationList < PmConversationCard: the data comes -// from `getRecentConversations`, which filters and sorts by email -// * OK, email: PmConversationList < UnreadCards: ditto -// * OK, unsorted: getNarrowFromLink. Though there's basically a bug in -// the webapp, where the URL that appears in the location bar for a -// group PM conversation excludes self -- so it's unusable if you try -// to give someone else in it a link to a particular message, say. -// * OK, unsorted: getNarrowForReply -// * Good: getNarrowFromNotificationData: filters, and starts from -// notification's pm_users, which is sorted. -// * Good: messageHeaderAsHtml: comes from pmKeyRecipientsFromMessage, -// which filters and sorts by ID -export const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); - -/** Convenience wrapper for `pmNarrowFromEmails`. */ export const pmNarrowFromEmail = (email: string): Narrow => pmNarrowFromEmails([email]); +/** + * A PM narrow, either 1:1 or group. + * + * The argument's type guarantees that it comes from + * `pmKeyRecipientsFromMessage` or one of its related functions. This + * ensures that we've properly either removed the self user, or not. + * + * See also `pmNarrowFromUsers`, which does the same thing with a slightly + * different form of input. + */ +export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow => + pmNarrowFromEmails(recipients.map(r => r.email)); + +/** + * A PM narrow, either 1:1 or group. + * + * This is just like `pmNarrowFromRecipients`, but taking a slightly + * different form of input. Use whichever one is more convenient. + * + * See also `pm1to1NarrowFromUser`, which is more convenient when you have a + * single specific user. + */ +export const pmNarrowFromUsers = (recipients: PmKeyUsers): Narrow => + pmNarrowFromEmails(recipients.map(r => r.email)); + +/** + * FOR TESTS ONLY. Like pmNarrowFromUsers, but without validation. + * + * This exists purely for convenience in tests. Unlike the other Narrow + * constructors, its type does not require the argument to have come from a + * function that applies our "maybe filter out self" convention. The caller + * is still required to do so, but nothing checks this. + * + * Outside of tests, always use pmNarrowFromUsers instead. Use + * pmKeyRecipientsFromUsers, along with an ownUserId value, to produce the + * needed input. + * + * This does take care of sorting the input as needed. + */ +// It'd be fine for test data to go through the usual filtering logic; the +// annoying thing is just that that requires an ownUserId value. +export const pmNarrowFromUsersUnsafe = (recipients: UserOrBot[]): Narrow => + pmNarrowFromEmails(recipients.sort((a, b) => a.user_id - b.user_id).map(r => r.email)); + +/** + * A 1:1 PM narrow, possibly with self. + * + * This has the same effect as calling pmNarrowFromUsers, but for code that + * statically has just one other user it's a bit more convenient because it + * doesn't require going through our `recipient` helpers. + */ +export const pm1to1NarrowFromUser = (user: UserOrBot): Narrow => pmNarrowFromEmails([user.email]); + export const specialNarrow = (operand: string): Narrow => [ { operator: 'is', @@ -345,7 +385,7 @@ export const getNarrowsForMessage = ( if (message.type === 'private') { result.push(ALL_PRIVATE_NARROW); - result.push(pmNarrowFromEmails(pmKeyRecipientsFromMessage(message, ownUser).map(x => x.email))); + result.push(pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUser))); } else { const streamName = streamNameOfStreamMessage(message); result.push(topicNarrow(streamName, message.subject)); @@ -374,7 +414,7 @@ export const getNarrowsForMessage = ( // now that it's free of fiddly details from the Narrow data structure export const getNarrowForReply = (message: Message | Outbox, ownUser: User) => { if (message.type === 'private') { - return pmNarrowFromEmails(pmKeyRecipientsFromMessage(message, ownUser).map(x => x.email)); + return pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUser)); } else { const streamName = streamNameOfStreamMessage(message); return topicNarrow(streamName, message.subject); diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 35f4ae6aee8..bc8483ebb2b 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -26,10 +26,55 @@ export const recipientsOfPrivateMessage = ( return recipients; }; -// Filter a list of PM recipients in the quirky way that we do. +/** + * A list of users identifying a PM conversation, as per pmKeyRecipientsFromMessage. + * + * This is an "opaque type alias" for an array of plain old data. + * See Flow docs: https://flow.org/en/docs/types/opaque-types/ + * + * That means: + * * For code outside this module, it's some unknown subtype of the given + * array type. + * * Secretly, it actually is just that array type, and code inside this + * module can see that. + * * (In general, the public type bound and the secret underlying type can + * be different, but in this case we've made them the same.) + * + * As a result: + * * The only way to produce a value of this type is with code inside this + * module. (For code outside the module, the secret underlying type + * could have any number of requirements it can't see; it could even be + * `empty`, which has no values.) + * * But code outside this module can still freely *consume* the data in a + * value of this type, just like any other value of the given array type. + * + * Or to say the same things from a different angle: + * * For code inside this module, this is just like a normal type alias, + * to the secret/private underlying type. + * * For code outside this module trying to produce a value of this type, + * it's a brick wall -- it's effectively like the `empty` type. + * * For code outside this module trying to consume a value of this type, + * it's just like a normal type alias to the public type bound; which in + * this case we've chosen to make the same as the private underlying type. + * + * See also `pmNarrowFromRecipients`, which requires a value of this type. + */ +export opaque type PmKeyRecipients: $ReadOnlyArray = $ReadOnlyArray; + +/** + * A list of users identifying a PM conversation, as per pmKeyRecipientsFromMessage. + * + * This is just like `PmKeyRecipients` but with a different selection of + * details about the users. See there for discussion. + * + * See also `pmNarrowFromUsers`, which requires a value of this type. + */ +export opaque type PmKeyUsers: $ReadOnlyArray = $ReadOnlyArray; + +// Filter a list of PM recipients in the quirky way that we do, and sort. // // Specifically: all users, except the self-user, except if it's the -// self-1:1 thread then include the self-user after all. +// self-1:1 thread then include the self-user after all. Then sort by ID. // // This is a module-private helper. See callers for what this set of // conditions *means* -- two different things, in fact, that have the same @@ -38,16 +83,31 @@ const filterRecipients = ( recipients: $ReadOnlyArray, ownUserId: number, ): $ReadOnlyArray => - recipients.length === 1 ? recipients : recipients.filter(r => r.id !== ownUserId); + recipients.length === 1 + ? recipients + : recipients.filter(r => r.id !== ownUserId).sort((a, b) => a.id - b.id); + +// Like filterRecipients, but on User objects. +const filterRecipientUsers = ( + recipients: $ReadOnlyArray, + ownUserId: number, +): $ReadOnlyArray => + recipients.length === 1 + ? recipients + : recipients.filter(r => r.user_id !== ownUserId).sort((a, b) => a.user_id - b.user_id); // Like filterRecipients, but on user IDs directly. const filterRecipientsAsUserIds = >( recipients: T, ownUserId: number, -): T => (recipients.length === 1 ? recipients : recipients.filter(r => r !== ownUserId)); +): T => + recipients.length === 1 + ? recipients + : recipients.filter(r => r !== ownUserId).sort((a, b) => a - b); // Like filterRecipients, but identifying users by email address. // Prefer filterRecipients instead. +// No sort; caller must sort if needed. const filterRecipientsByEmail = ( recipients: $ReadOnlyArray, ownEmail: string, @@ -82,7 +142,7 @@ export const normalizeRecipients = (recipients: $ReadOnlyArray<{ +email: string, }; /** - * The same set of users as pmKeyRecipientsFromMessage, in quirkier form. + * The same list of users as pmKeyRecipientsFromMessage, in quirkier form. * * Prefer normalizeRecipientsAsUserIdsSansMe over this; see #3764. * See that function for further discussion. @@ -98,9 +158,7 @@ export const normalizeRecipientsAsUserIds = (recipients: number[]) => recipients.sort((a, b) => a - b).join(','); /** - * The same set of users as pmKeyRecipientsFromMessage, in quirkier form. - * - * Sorted by user ID. + * The same list of users as pmKeyRecipientsFromMessage, in quirkier form. */ // Note that sorting by user ID is the same as the server does for group PMs // (see comment on Message#display_recipient). Then for 1:1 PMs the @@ -128,7 +186,9 @@ export const pmUiRecipientsFromMessage = ( }; /** - * The set of users to identify a PM conversation by in our data structures. + * The list of users to identify a PM conversation by in our data structures. + * + * This list is sorted by user ID. * * Typically we go on to take either the emails or user IDs in the result, * stringify them, and join with `,` to produce a string key. IDs are @@ -150,14 +210,16 @@ export const pmUiRecipientsFromMessage = ( * It would be great to unify on a single version, as the variation is a * possible source of bugs. */ -// The resulting users are sorted by user ID. That's because: +// The list would actually be sorted even without explicit sorting, because: // * For group PMs, the server provides them in that order; see comment // on Message#display_recipient. // * For 1:1 PMs, we only keep one user in the list. +// But we also sort them ourselves, so as not to rely on that fact about +// the server; it's easy enough to do. export const pmKeyRecipientsFromMessage = ( message: Message | Outbox, ownUser: User, -): $ReadOnlyArray => { +): PmKeyRecipients => { if (message.type !== 'private') { throw new Error('pmKeyRecipientsFromMessage: expected PM, got stream message'); } @@ -165,9 +227,9 @@ export const pmKeyRecipientsFromMessage = ( }; /** - * The set of users to identify a PM conversation by in our data structures. + * The list of users to identify a PM conversation by in our data structures. * - * This produces the same set of users as `pmKeyRecipientsFromMessage`, just + * This produces the same list of users as `pmKeyRecipientsFromMessage`, just * from a different form of input. * * The input may either include or exclude self, without affecting the @@ -177,7 +239,7 @@ export const pmKeyRecipientsFromIds = ( userIds: number[], allUsersById: Map, ownUserId: number, -): UserOrBot[] | null => { +): PmKeyUsers | null => { const users = []; for (const id of userIds) { if (id === ownUserId && userIds.length > 1) { @@ -189,9 +251,29 @@ export const pmKeyRecipientsFromIds = ( } users.push(user); } - return users; + return users.sort((a, b) => a.user_id - b.user_id); +}; + +/** + * Just like pmKeyRecipientsFromMessage, but in a slightly different format. + */ +export const pmKeyRecipientUsersFromMessage = ( + message: Message | Outbox, + allUsersById: Map, + ownUserId: number, +): PmKeyUsers | null => { + const userIds = recipientsOfPrivateMessage(message).map(r => r.id); + return pmKeyRecipientsFromIds(userIds, allUsersById, ownUserId); }; +/** + * Just like pmKeyRecipientsFromMessage, but with slightly different formats of data. + */ +export const pmKeyRecipientsFromUsers = ( + users: $ReadOnlyArray, + ownUserId: number, +): PmKeyUsers => filterRecipientUsers(users, ownUserId); + /** * The key this PM is filed under in the "unread messages" data structure. * diff --git a/src/webview/html/messageHeaderAsHtml.js b/src/webview/html/messageHeaderAsHtml.js index 2672cef114b..95b96acb6d7 100644 --- a/src/webview/html/messageHeaderAsHtml.js +++ b/src/webview/html/messageHeaderAsHtml.js @@ -2,7 +2,7 @@ import template from './template'; import type { Message, Narrow, Outbox } from '../../types'; import type { BackgroundData } from '../MessageList'; -import { streamNarrow, topicNarrow, caseNarrow, pmNarrowFromEmails } from '../../utils/narrow'; +import { streamNarrow, topicNarrow, caseNarrow, pmNarrowFromRecipients } from '../../utils/narrow'; import { foregroundColorFromBackground } from '../../utils/color'; import { humanDate } from '../../utils/date'; import { @@ -85,7 +85,7 @@ export default ( if (item.type === 'private' && headerStyle === 'full') { const keyRecipients = pmKeyRecipientsFromMessage(item, ownUser); - const narrowObj = pmNarrowFromEmails(keyRecipients.map(r => r.email)); + const narrowObj = pmNarrowFromRecipients(keyRecipients); const narrowStr = JSON.stringify(narrowObj); const uiRecipients = pmUiRecipientsFromMessage(item, ownUser);