diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 5f2885600fb..00679e75174 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -179,6 +179,7 @@ export const selfAccount: Account = makeAccount({ export const selfAuth: Auth = deepFreeze(authOfAccount(selfAccount)); export const otherUser: User = makeUser({ name: 'other' }); +export const thirdUser: User = makeUser({ name: 'third' }); export const crossRealmBot: CrossRealmBot = makeCrossRealmBot({ name: 'bot' }); @@ -293,14 +294,23 @@ const randMessageId: () => number = makeUniqueRandInt('message ID', 10000000); * * Beware! These values may not be representative. */ -export const pmMessage = (extra?: $Rest): Message => { +export const pmMessage = (args?: {| + ...$Rest, + sender?: User, + recipients?: User[], +|}): Message => { + // The `Object.freeze` is to work around a Flow issue: + // https://github.com/facebook/flow/issues/2386#issuecomment-695064325 + const { sender = otherUser, recipients = [otherUser, selfUser], ...extra } = + args ?? Object.freeze({}); + const baseMessage: Message = { ...messagePropertiesBase, - ...messagePropertiesFromSender(otherUser), + ...messagePropertiesFromSender(sender), content: 'This is an example PM message.', content_type: 'text/markdown', - display_recipient: [displayRecipientFromUser(selfUser)], + display_recipient: recipients.map(displayRecipientFromUser), id: randMessageId(), recipient_id: 2342, stream_id: -1, @@ -327,9 +337,9 @@ const messagePropertiesFromStream = (stream1: Stream) => { * Beware! These values may not be representative. */ export const streamMessage = (args?: {| ...$Rest, stream?: Stream |}): Message => { - // The redundant `stream` in the ?? case avoids a Flow issue: - // https://github.com/facebook/flow/issues/2386 - const { stream: streamInner = stream, ...extra } = args ?? { stream }; + // The `Object.freeze` is to work around a Flow issue: + // https://github.com/facebook/flow/issues/2386#issuecomment-695064325 + const { stream: streamInner = stream, ...extra } = args ?? Object.freeze({}); const baseMessage: Message = { ...messagePropertiesBase, diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index ec5768729c0..29717049f5e 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -286,16 +286,19 @@ describe('narrowsReducer', () => { [groupNarrowStr]: [2, 4], }); - const message = eg.pmMessage({ id: 5, flags: [] }); + const message = eg.pmMessage({ + id: 5, + flags: [], + sender: eg.selfUser, + display_recipient: [eg.displayRecipientFromUser(eg.selfUser), { email: 'mark@example.com' }], + }); const action = deepFreeze({ ...eg.eventNewMessageActionBase, message, - caughtUp: { - [HOME_NARROW_STR]: { older: false, newer: true }, - [ALL_PRIVATE_NARROW_STR]: { older: false, newer: true }, - [privateNarrowStr]: { older: false, newer: true }, - }, + caughtUp: Object.fromEntries( + Object.keys(initialState).map(key => [key, { older: false, newer: true }]), + ), }); const expectedState = { diff --git a/src/message/messagesActions.js b/src/message/messagesActions.js index 1febbe3f225..3a62c927d23 100644 --- a/src/message/messagesActions.js +++ b/src/message/messagesActions.js @@ -8,6 +8,7 @@ import { FIRST_UNREAD_ANCHOR } from '../anchor'; import { getStreamsById } from '../subscriptions/subscriptionSelectors'; import * as api from '../api'; import { isUrlOnRealm } from '../utils/url'; +import { getOwnUserId } from '../users/userSelectors'; /** * Navigate to the given narrow. @@ -28,7 +29,8 @@ export const messageLinkPress = (href: string) => async ( const auth = getAuth(state); const usersById = getUsersById(state); const streamsById = getStreamsById(state); - const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById); + const ownUserId = getOwnUserId(state); + const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById, ownUserId); if (narrow) { const anchor = getMessageIdFromLink(href, auth.realm); dispatch(doNarrow(narrow, anchor)); diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 6c197106628..849bafb1200 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -12,11 +12,12 @@ import objectEntries from '../../utils/objectEntries'; describe('getNarrowFromNotificationData', () => { const DEFAULT_MAP = new Map(); + const ownUserId = eg.selfUser.user_id; test('unknown notification data returns null', () => { // $FlowFixMe: actually validate APNs messages const notification: Notification = {}; - const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP); + const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); expect(narrow).toBe(null); }); @@ -26,7 +27,7 @@ describe('getNarrowFromNotificationData', () => { stream: 'some stream', topic: 'some topic', }; - const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP); + const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); expect(narrow).toEqual(topicNarrow('some stream', 'some topic')); }); @@ -35,12 +36,12 @@ describe('getNarrowFromNotificationData', () => { recipient_type: 'private', sender_email: 'mark@example.com', }; - const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP); + const narrow = getNarrowFromNotificationData(notification, DEFAULT_MAP, ownUserId); expect(narrow).toEqual(privateNarrow('mark@example.com')); }); test('on notification for a group message returns a group narrow', () => { - const users = [eg.makeUser(), eg.makeUser(), eg.makeUser(), eg.makeUser()]; + const users = [eg.selfUser, eg.makeUser(), eg.makeUser(), eg.makeUser()]; const usersById: Map = new Map(users.map(u => [u.user_id, u])); const notification = { @@ -48,9 +49,9 @@ describe('getNarrowFromNotificationData', () => { pm_users: users.map(u => u.user_id).join(','), }; - const expectedNarrow = groupNarrow(users.map(u => u.email)); + const expectedNarrow = groupNarrow(users.slice(1).map(u => u.email)); - const narrow = getNarrowFromNotificationData(notification, usersById); + const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId); expect(narrow).toEqual(expectedNarrow); }); @@ -62,7 +63,7 @@ describe('getNarrowFromNotificationData', () => { }; const usersById = new Map(); - const narrow = getNarrowFromNotificationData(notification, usersById); + const narrow = getNarrowFromNotificationData(notification, usersById, ownUserId); expect(narrow).toBe(null); }); diff --git a/src/notification/extract.js b/src/notification/extract.js index 3f14165748f..1e598d123a0 100644 --- a/src/notification/extract.js +++ b/src/notification/extract.js @@ -182,10 +182,11 @@ export const fromAPNsImpl = (rawData: JSONableDict): Notification | void => { if (typeof pm_users !== 'string') { throw err('invalid'); } - if (pm_users.split(',').some(s => Number.isNaN(parseInt(s, 10)))) { + const ids = pm_users.split(',').map(s => parseInt(s, 10)); + if (ids.some(id => Number.isNaN(id))) { throw err('invalid'); } - return { recipient_type: 'private', pm_users, ...realm_uri_obj }; + return { recipient_type: 'private', pm_users: ids.sort().join(','), ...realm_uri_obj }; } if (typeof sender_email !== 'string') { diff --git a/src/notification/index.js b/src/notification/index.js index 59826da1ad2..5bc92470073 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -17,6 +17,7 @@ import { import { identityOfAuth } from '../account/accountMisc'; import { fromAPNs } from './extract'; import { tryParseUrl } from '../utils/url'; +import { pmKeyRecipientsFromIds } from '../utils/recipient'; /** * Identify the account the notification is for, if possible. @@ -85,6 +86,7 @@ export const getAccountFromNotificationData = ( export const getNarrowFromNotificationData = ( data: Notification, usersById: Map, + ownUserId: number, ): Narrow | null => { if (!data.recipient_type) { // This condition is impossible if the value is rightly-typed; but in @@ -103,17 +105,9 @@ export const getNarrowFromNotificationData = ( return privateNarrow(data.sender_email); } - const emails = []; - const idStrs = data.pm_users.split(','); - for (let i = 0; i < idStrs.length; ++i) { - const id = parseInt(idStrs[i], 10); - const user = usersById.get(id); - if (!user) { - return null; - } - emails.push(user.email); - } - return groupNarrow(emails); + const ids = data.pm_users.split(',').map(s => parseInt(s, 10)); + const users = pmKeyRecipientsFromIds(ids, usersById, ownUserId); + return users && groupNarrow(users.map(u => u.email)); }; const getInitialNotification = async (): Promise => { diff --git a/src/notification/notificationActions.js b/src/notification/notificationActions.js index 5cfc754263c..8ec47feb406 100644 --- a/src/notification/notificationActions.js +++ b/src/notification/notificationActions.js @@ -13,7 +13,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 { getUsersById } from '../users/userSelectors'; +import { getOwnUserId, getUsersById } from '../users/userSelectors'; import { doNarrow } from '../message/messagesActions'; import { accountSwitch } from '../account/accountActions'; import { getIdentities } from '../account/accountsSelectors'; @@ -51,7 +51,7 @@ export const narrowToNotification = (data: ?Notification) => ( return; } - const narrow = getNarrowFromNotificationData(data, getUsersById(state)); + const narrow = getNarrowFromNotificationData(data, getUsersById(state), getOwnUserId(state)); if (narrow) { dispatch(doNarrow(narrow)); } diff --git a/src/notification/types.js b/src/notification/types.js index a1f76331a99..9ac56660d97 100644 --- a/src/notification/types.js +++ b/src/notification/types.js @@ -14,7 +14,7 @@ // 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 |} - // Group PM messages have `pm_users`, which is comma-separated IDs. + // Group PM messages have `pm_users`, which is sorted, comma-separated IDs. | {| recipient_type: 'private', pm_users: string, realm_uri?: string |} // 1:1 PM messages lack `pm_users`. | {| recipient_type: 'private', sender_email: string, realm_uri?: string |}; diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index 0ca8a09fd42..e9ea2d9ef21 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -27,43 +27,11 @@ describe('getRecentConversations', () => { }); test('returns unique list of recipients, includes conversations with self', () => { - const meAndJohnPm1 = eg.pmMessage({ - id: 1, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userJohn), - ], - }); - - const meAndMarkPm = eg.pmMessage({ - id: 2, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userMark), - ], - }); - - const meAndJohnPm2 = eg.pmMessage({ - id: 3, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userJohn), - ], - }); - - const meOnlyPm = eg.pmMessage({ - id: 4, - display_recipient: [eg.displayRecipientFromUser(eg.selfUser)], - }); - - const meJohnAndMarkPm = eg.pmMessage({ - id: 0, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userMark), - eg.displayRecipientFromUser(userJohn), - ], - }); + const meAndJohnPm1 = eg.pmMessage({ id: 1, recipients: [eg.selfUser, userJohn] }); + 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 state = eg.reduxState({ realm: eg.realmState({ email: eg.selfUser.email }), @@ -150,52 +118,12 @@ describe('getRecentConversations', () => { test('returns recipients sorted by last activity', () => { // Maybe we can share these definitions with the above test; // first, we have to sort out why the IDs are different. - - const meAndMarkPm1 = eg.pmMessage({ - id: 1, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userMark), - ], - }); - - const meAndJohnPm1 = eg.pmMessage({ - id: 2, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userJohn), - ], - }); - - const meAndMarkPm2 = eg.pmMessage({ - id: 3, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userMark), - ], - }); - - const meAndJohnPm2 = eg.pmMessage({ - id: 4, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userJohn), - ], - }); - - const meJohnAndMarkPm = eg.pmMessage({ - id: 5, - display_recipient: [ - eg.displayRecipientFromUser(eg.selfUser), - eg.displayRecipientFromUser(userJohn), - eg.displayRecipientFromUser(userMark), - ], - }); - - const meOnlyPm = eg.pmMessage({ - id: 6, - display_recipient: [eg.displayRecipientFromUser(eg.selfUser)], - }); + const meAndMarkPm1 = eg.pmMessage({ id: 1, recipients: [eg.selfUser, userMark] }); + const meAndJohnPm1 = eg.pmMessage({ id: 2, recipients: [eg.selfUser, userJohn] }); + const meAndMarkPm2 = eg.pmMessage({ id: 3, recipients: [eg.selfUser, userMark] }); + const meAndJohnPm2 = eg.pmMessage({ id: 4, recipients: [eg.selfUser, userJohn] }); + const meJohnAndMarkPm = eg.pmMessage({ id: 5, recipients: [eg.selfUser, userJohn, userMark] }); + const meOnlyPm = eg.pmMessage({ id: 6, recipients: [eg.selfUser] }); const state = eg.reduxState({ realm: eg.realmState({ email: eg.selfUser.email }), diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index f8efbd8ba21..b293321b023 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -19,8 +19,12 @@ export const getRecentConversations: Selector = createSele unreadHuddles: { [string]: number }, ): 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(msg.display_recipient, ownUser.email), + msgId: msg.id, })); diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 58f088ebbac..2710a95ce76 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -74,13 +74,15 @@ export type CaughtUp = {| * * See `CaughtUp` for details on what each value means. */ -export type CaughtUpState = {| +export type CaughtUpState = { + // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js. [narrow: string]: CaughtUp, -|}; +}; -export type DraftsState = {| +export type DraftsState = { + // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js. [narrow: string]: string, -|}; +}; export type Fetching = {| older: boolean, @@ -93,6 +95,7 @@ export type Fetching = {| * See also: `CaughtUpState`, `NarrowsState`. */ export type FetchingState = { + // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js. [narrow: string]: Fetching, }; @@ -154,8 +157,7 @@ export type FlagName = $Keys; * messages belonging to a given narrow. */ export type MessagesState = { - // MessagesState should be exact; we're waiting for Flow v0.126.0. See note - // in src/utils/jsonable.js. + // TODO(flow-v0.126): Should be exact. See note in src/utils/jsonable.js. [id: number]: $Exact, }; diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 80cd605e905..865fc783bc2 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -124,12 +124,10 @@ describe('decodeHashComponent', () => { }); describe('getNarrowFromLink', () => { - const [userA, userB, userC] = [eg.makeUser(), eg.makeUser(), eg.makeUser()]; - const usersById: Map = new Map([ - [userA.user_id, userA], - [userB.user_id, userB], - [userC.user_id, userC], - ]); + const [userB, userC] = [eg.makeUser(), eg.makeUser()]; + const usersById: Map = new Map( + [eg.selfUser, userB, userC].map(u => [u.user_id, u]), + ); const streamGeneral = eg.makeStream({ name: 'general' }); @@ -139,6 +137,7 @@ describe('getNarrowFromLink', () => { new URL('https://example.com'), usersById, new Map(streams.map(s => [s.stream_id, s])), + eg.selfUser.user_id, ); test('on link to realm domain but not narrow: return null', () => { @@ -256,15 +255,23 @@ describe('getNarrowFromLink', () => { }); test('on group PM link', () => { - const ids = `${userA.user_id},${userB.user_id},${userC.user_id}`; + const ids = `${userB.user_id},${userC.user_id}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual( - groupNarrow([userA.email, userB.email, userC.email]), + groupNarrow([userB.email, userC.email]), + ); + }); + + test('on group PM link including self', () => { + // 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( + groupNarrow([userB.email, userC.email]), ); }); test('if any of the user ids are not found: return null', () => { - const otherId = 1 + Math.max(userA.user_id, userB.user_id, userC.user_id); - const ids = `${userA.user_id},${userB.user_id},${otherId}`; + const otherId = 1 + Math.max(...usersById.keys()); + const ids = `${userB.user_id},${otherId}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(null); }); @@ -273,9 +280,9 @@ describe('getNarrowFromLink', () => { }); test('on a message link', () => { - const ids = `${userA.user_id},${userC.user_id}`; + const ids = `${userB.user_id},${userC.user_id}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group/near/2`)).toEqual( - groupNarrow([userA.email, userC.email]), + groupNarrow([userB.email, userC.email]), ); 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 b3ba527b9a8..d9f54001801 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -1,3 +1,5 @@ +/* @flow strict-local */ + import { HOME_NARROW, isHomeNarrow, @@ -24,6 +26,8 @@ import { MENTIONED_NARROW, } from '../narrow'; +import * as eg from '../../__tests__/lib/exampleData'; + describe('HOME_NARROW', () => { test('produces an empty list', () => { expect(HOME_NARROW).toEqual([]); @@ -45,8 +49,7 @@ describe('privateNarrow', () => { }); test('if operator is "pm-with" and only one email, then it is a private narrow', () => { - expect(isPrivateNarrow([])).toBe(false); - expect(isPrivateNarrow([{}, {}])).toBe(false); + expect(isPrivateNarrow(HOME_NARROW)).toBe(false); expect(isPrivateNarrow(privateNarrow('bob@example.com'))).toBe(true); expect( isPrivateNarrow([ @@ -70,8 +73,7 @@ describe('groupNarrow', () => { }); test('a group narrow is only private chat with more than one recipients', () => { - expect(isGroupNarrow([])).toBe(false); - expect(isGroupNarrow([{}, {}])).toBe(false); + expect(isGroupNarrow(HOME_NARROW)).toBe(false); expect(isGroupNarrow(privateNarrow('bob@example.com'))).toBe(false); expect(isGroupNarrow(groupNarrow(['bob@example.com', 'john@example.com']))).toBe(true); expect( @@ -96,8 +98,7 @@ describe('groupNarrow', () => { describe('isPrivateOrGroupNarrow', () => { test('a private or group narrow is any "pm-with" narrow', () => { expect(isPrivateOrGroupNarrow(undefined)).toBe(false); - expect(isPrivateOrGroupNarrow([])).toBe(false); - expect(isPrivateOrGroupNarrow([{}, {}])).toBe(false); + expect(isPrivateOrGroupNarrow(HOME_NARROW)).toBe(false); expect(isPrivateOrGroupNarrow(privateNarrow('bob@example.com'))).toBe(true); expect(isPrivateOrGroupNarrow(groupNarrow(['bob@example.com', 'john@example.com']))).toBe(true); expect( @@ -145,8 +146,7 @@ describe('specialNarrow', () => { test('only narrowing with the "is" operator is special narrow', () => { expect(isSpecialNarrow(undefined)).toBe(false); - expect(isSpecialNarrow([])).toBe(false); - expect(isSearchNarrow([{}, {}])).toBe(false); + expect(isSpecialNarrow(HOME_NARROW)).toBe(false); expect(isSpecialNarrow(streamNarrow('some stream'))).toBe(false); expect(isSpecialNarrow(STARRED_NARROW)).toBe(true); expect(isSpecialNarrow([{ operator: 'stream', operand: 'some stream' }])).toBe(false); @@ -166,8 +166,7 @@ describe('streamNarrow', () => { test('only narrow with operator of "stream" is a stream narrow', () => { expect(isStreamNarrow(undefined)).toBe(false); - expect(isStreamNarrow([])).toBe(false); - expect(isSearchNarrow([{}, {}])).toBe(false); + expect(isStreamNarrow(HOME_NARROW)).toBe(false); expect(isStreamNarrow(streamNarrow('some stream'))).toBe(true); expect(isStreamNarrow([{ operator: 'stream', operand: 'some stream' }])).toBe(true); }); @@ -183,7 +182,7 @@ describe('topicNarrow', () => { test('only narrow with two items, one for stream, one for topic is a topic narrow', () => { expect(isTopicNarrow(undefined)).toBe(false); - expect(isTopicNarrow([])).toBe(false); + expect(isTopicNarrow(HOME_NARROW)).toBe(false); expect(isTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true); expect( isTopicNarrow([ @@ -212,142 +211,107 @@ describe('SEARCH_NARROW', () => { test('narrow with "search" operand is a search narrow', () => { expect(isSearchNarrow(undefined)).toBe(false); - expect(isSearchNarrow([])).toBe(false); - expect(isSearchNarrow([{}, {}])).toBe(false); - expect(isSearchNarrow([{ operator: 'search' }])).toBe(true); + expect(isSearchNarrow(HOME_NARROW)).toBe(false); + expect(isSearchNarrow(SEARCH_NARROW('some query'))).toBe(true); }); }); describe('isMessageInNarrow', () => { - test('any message is in "Home"', () => { - const message = { - flags: [], - }; - const narrow = HOME_NARROW; - expect(isMessageInNarrow(message, narrow)).toBe(true); - }); - - test('message with type "private" is in private narrow if recipient matches', () => { - const message = { - flags: [], - type: 'private', - display_recipient: [{ email: 'me@example.com' }, { email: 'john@example.com' }], - }; - const narrow = privateNarrow('john@example.com'); - - expect(isMessageInNarrow(message, narrow, 'me@example.com')).toBe(true); - }); - - test('message to self is in "private" narrow with self', () => { - const message = { - flags: [], - type: 'private', - display_recipient: [{ email: 'me@example.com' }], - }; - const narrow = privateNarrow('me@example.com'); - - expect(isMessageInNarrow(message, narrow, 'me@example.com')).toBe(true); - }); - - test('message with type "private" is in group narrow if all recipients match ', () => { - const message = { - type: 'private', - flags: [], - display_recipient: [ - { email: 'me@example.com' }, - { email: 'john@example.com' }, - { email: 'mark@example.com' }, - ], - }; - const ownEmail = 'me@example.com'; - const narrow = groupNarrow(['john@example.com', 'mark@example.com']); - - expect(isMessageInNarrow(message, narrow, ownEmail)).toBe(true); - }); - - test('message with type "private" is always in "private messages" narrow', () => { - const message = { - flags: [], - type: 'private', - display_recipient: [{ email: 'me@example.com' }, { email: 'john@example.com' }], - }; - expect(isMessageInNarrow(message, ALL_PRIVATE_NARROW)).toBe(true); - }); - - test('message with type "stream" is in narrow if recipient and current stream match', () => { - const message = { - flags: [], - type: 'stream', - display_recipient: 'some stream', - }; - const narrow = streamNarrow('some stream'); - - expect(isMessageInNarrow(message, narrow)).toBe(true); - }); - - test('message with flags undefined throws an error', () => { - const message = { - // no flags key - }; - expect(() => isMessageInNarrow(message, MENTIONED_NARROW)).toThrow(); - }); - - test('message with flag "mentioned" is in is:mentioned narrow', () => { - const message = { - flags: ['mentioned'], - }; - expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(true); - }); - - test('message with flag "wildcard_mentioned" is in is:mentioned narrow', () => { - const message = { - flags: ['wildcard_mentioned'], - }; - expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(true); - }); - - test('message without flag "mentioned" or "wildcard_mentioned" is not in is:mentioned narrow', () => { - const message = { - flags: [], - }; - expect(isMessageInNarrow(message, MENTIONED_NARROW)).toBe(false); - }); - - test('message with flag "starred" is in is:starred narrow', () => { - const message = { - flags: ['starred'], - }; - expect(isMessageInNarrow(message, STARRED_NARROW)).toBe(true); - }); - - test('message without flag "starred" is not in is:starred narrow', () => { - const message = { - flags: [], - }; - expect(isMessageInNarrow(message, STARRED_NARROW)).toBe(false); - }); - - test('message with type stream is in topic narrow if current stream and topic match with its own', () => { - const message = { - type: 'stream', - subject: 'some topic', - display_recipient: 'some stream', - flags: [], - }; - const narrow = topicNarrow('some stream', 'some topic'); - - expect(isMessageInNarrow(message, narrow)).toBe(true); + const ownEmail = eg.selfUser.email; + const otherStream = eg.makeStream(); + + // prettier-ignore + for (const [narrowDescription, narrow, cases] of [ + ['all-messages ("home") narrow', HOME_NARROW, [ + ['a message', true, eg.streamMessage()], + ]], + + ['whole-stream narrow', streamNarrow(eg.stream.name), [ + ['matching stream message', true, eg.streamMessage()], + ['other-stream message', false, eg.streamMessage({ stream: otherStream })], + ['PM', false, eg.pmMessage()], + ]], + ['stream conversation', topicNarrow(eg.stream.name, 'cabbages'), [ + ['matching message', true, eg.streamMessage({ subject: 'cabbages' })], + ['message in same stream but other topic', false, eg.streamMessage({ subject: 'kings' })], + ['other-stream message', false, eg.streamMessage({ stream: otherStream })], + ['PM', false, eg.pmMessage()], + ]], + + ['1:1 PM conversation, non-self', privateNarrow(eg.otherUser.email), [ + ['matching PM, inbound', true, eg.pmMessage()], + ['matching PM, outbound', true, eg.pmMessage({ sender: eg.selfUser })], + ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], + ['group-PM including this user, inbound', false, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], + ['group-PM including this user, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], + ['stream message', false, eg.streamMessage()], + ]], + ['self-1:1 conversation', privateNarrow(eg.selfUser.email), [ + ['self-1:1 message', true, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], + ['other 1:1 message, inbound', false, eg.pmMessage()], + ['other 1:1 message, outbound', false, eg.pmMessage({ sender: eg.selfUser })], + ['group-PM, inbound', false, eg.pmMessage({ recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], + ['group-PM, outbound', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser, eg.thirdUser] })], + ['stream message', false, eg.streamMessage()], + ]], + ['group-PM conversation', groupNarrow([eg.otherUser.email, eg.thirdUser.email]), [ + ['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()], + ['1:1 within group, outbound', false, eg.pmMessage({ sender: eg.selfUser })], + ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], + ['stream message', false, eg.streamMessage()], + ]], + ['group-PM conversation, including self', groupNarrow([eg.selfUser.email, eg.otherUser.email, eg.thirdUser.email]), [ + ['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()], + ['1:1 within group, outbound', false, eg.pmMessage({ sender: eg.selfUser })], + ['self-1:1 message', false, eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser] })], + ['stream message', false, eg.streamMessage()], + ]], + ['all-PMs narrow', ALL_PRIVATE_NARROW, [ + ['a PM', true, eg.pmMessage()], + ['stream message', false, eg.streamMessage()], + ]], + + ['is:mentioned', MENTIONED_NARROW, [ + ['w/ mentioned flag', true, eg.streamMessage({ flags: ['mentioned'] })], + ['w/ wildcard_mentioned flag', true, eg.streamMessage({ flags: ['wildcard_mentioned'] })], + ['w/o flags', false, eg.streamMessage()], + ]], + ['is:starred', STARRED_NARROW, [ + ['w/ starred flag', true, eg.streamMessage({ flags: ['starred'] })], + ['w/o flags', false, eg.streamMessage()], + ]], + ]) { + describe(narrowDescription, () => { + for (const [messageDescription, expected, message] of cases) { + test(`${expected ? 'contains' : 'excludes'} ${messageDescription}`, () => { + expect( + isMessageInNarrow({ ...message, flags: message.flags ?? [] }, narrow, ownEmail), + ).toBe(expected); + }); + } + }); + } + + test('message with flags absent throws an error', () => { + const message = eg.streamMessage({ + // no flags + }); + expect(() => isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toThrow(); }); }); describe('getNarrowFromMessage', () => { - test('message with single recipient, returns a private narrow', () => { - const message = { - display_recipient: [{ email: 'bob@example.com' }], - }; - const ownEmail = 'hamlet@zulip.com'; + const ownEmail = eg.selfUser.email; - const expectedNarrow = privateNarrow('bob@example.com'); + test('message with single recipient, returns a private narrow', () => { + const message = eg.pmMessage({ + display_recipient: [eg.displayRecipientFromUser(eg.otherUser)], + }); + const expectedNarrow = privateNarrow(eg.otherUser.email); const actualNarrow = getNarrowFromMessage(message, ownEmail); @@ -355,11 +319,10 @@ describe('getNarrowFromMessage', () => { }); test('for message with multiple recipients, return a group narrow', () => { - const message = { - display_recipient: [{ email: 'bob@example.com' }, { email: 'john@example.com' }], - }; - const ownEmail = 'hamlet@zulip.com'; - const expectedNarrow = groupNarrow(['bob@example.com', 'john@example.com']); + const message = eg.pmMessage({ + display_recipient: [eg.otherUser, eg.thirdUser].map(eg.displayRecipientFromUser), + }); + const expectedNarrow = groupNarrow([eg.otherUser.email, eg.thirdUser.email]); const actualNarrow = getNarrowFromMessage(message, ownEmail); @@ -367,24 +330,19 @@ describe('getNarrowFromMessage', () => { }); test('if recipient of a message is string, returns a stream narrow', () => { - const message = { - display_recipient: 'stream', - }; - const expectedNarrow = streamNarrow('stream'); + const message = eg.streamMessage({ subject: '' }); + const expectedNarrow = streamNarrow(eg.stream.name); - const actualNarrow = getNarrowFromMessage(message); + const actualNarrow = getNarrowFromMessage(message, ownEmail); expect(actualNarrow).toEqual(expectedNarrow); }); test('if recipient is a string and there is a subject returns a topic narrow', () => { - const message = { - display_recipient: 'stream', - subject: 'subject', - }; - const expectedNarrow = topicNarrow('stream', 'subject'); + const message = eg.streamMessage(); + const expectedNarrow = topicNarrow(eg.stream.name, message.subject); - const actualNarrow = getNarrowFromMessage(message); + const actualNarrow = getNarrowFromMessage(message, ownEmail); expect(actualNarrow).toEqual(expectedNarrow); }); @@ -392,8 +350,6 @@ describe('getNarrowFromMessage', () => { describe('isSameNarrow', () => { test('Return true if two narrows are same', () => { - expect(isSameNarrow(undefined, undefined)).toBe(false); - expect(isSameNarrow(streamNarrow('stream'), undefined)).toBe(false); expect(isSameNarrow(streamNarrow('stream'), streamNarrow('stream'))).toBe(true); expect(isSameNarrow(streamNarrow('stream'), streamNarrow('stream1'))).toBe(false); expect(isSameNarrow(streamNarrow('stream'), topicNarrow('stream', 'topic'))).toBe(false); diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 8d2c4c00f83..d81097de323 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -2,6 +2,7 @@ import { addBreadcrumb } from '@sentry/react-native'; import type { Narrow, Stream, User } from '../types'; import { topicNarrow, streamNarrow, groupNarrow, specialNarrow } from './narrow'; +import { pmKeyRecipientsFromIds } from './recipient'; import { isUrlOnRealm } from './url'; // TODO: Work out what this does, write a jsdoc for its interface, and @@ -110,17 +111,9 @@ const parseStreamOperand = (operand, streamsById): string => { const parseTopicOperand = operand => decodeHashComponent(operand); /** Parse the operand of a `pm-with` operator. */ -const parsePmOperand = (operand, usersById) => { - const recipientIds = operand.split('-')[0].split(','); - const recipientEmails = []; - for (let i = 0; i < recipientIds.length; ++i) { - const user = usersById.get(parseInt(recipientIds[i], 10)); - if (user === undefined) { - return null; - } - recipientEmails.push(user.email); - } - return recipientEmails; +const parsePmOperand = (operand, usersById, ownUserId) => { + const idStrs = operand.split('-')[0].split(','); + return idStrs.map(s => parseInt(s, 10)); }; export const getNarrowFromLink = ( @@ -128,17 +121,16 @@ export const getNarrowFromLink = ( realm: URL, usersById: Map, streamsById: Map, + ownUserId: number, ): Narrow | null => { const type = getLinkType(url, realm); const paths = getPathsFromUrl(url, realm); switch (type) { case 'pm': { - const recipientEmails = parsePmOperand(paths[1], usersById); - if (recipientEmails === null) { - return null; - } - return groupNarrow(recipientEmails); + const ids = parsePmOperand(paths[1], usersById, ownUserId); + const users = pmKeyRecipientsFromIds(ids, usersById, ownUserId); + return users && groupNarrow(users.map(u => u.email)); } case 'topic': return topicNarrow(parseStreamOperand(paths[1], streamsById), parseTopicOperand(paths[3])); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 0ea5e25c45c..a6f92090e59 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -2,8 +2,8 @@ import isEqual from 'lodash.isequal'; import unescape from 'lodash.unescape'; -import type { Narrow, Message, Outbox } from '../types'; -import { normalizeRecipients } from './recipient'; +import type { Narrow, Message, Outbox, PmRecipientUser } from '../types'; +import { normalizeRecipientsSansMe } from './recipient'; export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => Array.isArray(narrow1) && Array.isArray(narrow2) && isEqual(narrow1, narrow2); @@ -21,6 +21,46 @@ export const privateNarrow = (email: string): Narrow => [ }, ]; +/** + * A group PM narrow. + * + * 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. + * + * They might not have a consistent sorting. (This would be good to fix.) + * Consumers of this data should sort for themselves when making comparisons. + */ +// 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: getNarrowFromMessage +// * 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 groupNarrow = (emails: string[]): Narrow => [ { operator: 'pm-with', @@ -210,10 +250,16 @@ export const isSearchNarrow = (narrow?: Narrow): boolean => /** (For search narrows, just returns false.) */ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: string): boolean => { - const matchRecipients = (emails: string[]) => { - const normalizedRecipients = normalizeRecipients(message.display_recipient); - const normalizedNarrow = [...emails, ownEmail].sort().join(','); - return normalizedRecipients === ownEmail || normalizedRecipients === normalizedNarrow; + const matchPmRecipients = (emails: string[]) => { + if (message.type !== 'private') { + return false; + } + const recipients: PmRecipientUser[] = message.display_recipient; + const narrowAsRecipients = emails.map(email => ({ email })); + return ( + normalizeRecipientsSansMe(recipients, ownEmail) + === normalizeRecipientsSansMe(narrowAsRecipients, ownEmail) + ); }; const { flags } = message; @@ -226,8 +272,8 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st stream: name => name === message.display_recipient, topic: (streamName, topic) => streamName === message.display_recipient && topic === message.subject, - pm: email => matchRecipients([email]), - groupPm: matchRecipients, + pm: email => matchPmRecipients([email]), + groupPm: matchPmRecipients, starred: () => flags.includes('starred'), mentioned: () => flags.includes('mentioned') || flags.includes('wildcard_mentioned'), allPrivate: () => message.type === 'private', diff --git a/src/utils/recipient.js b/src/utils/recipient.js index b6f8ee456c2..71e76ca09cc 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -22,6 +22,14 @@ export const normalizeRecipients = (recipients: $ReadOnlyArray<{ email: string, .sort() .join(','); +/** + * The same set of users as pmKeyRecipientsFromMessage, in quirkier form. + * + * Prefer normalizeRecipientsAsUserIdsSansMe over this; see #3764. + * See that function for further discussion. + * + * Users are sorted by email address. + */ export const normalizeRecipientsSansMe = ( recipients: $ReadOnlyArray<{ email: string, ... }>, ownEmail: string, @@ -38,6 +46,15 @@ export const normalizeRecipientsAsUserIds = ( .sort() .join(','); +/** + * The same set of users as pmKeyRecipientsFromMessage, in quirkier form. + * + * Sorted by user ID. + */ +// 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 +// server's behavior is quirkier... but we keep only one user for those +// anyway, so it doesn't matter. export const normalizeRecipientsAsUserIdsSansMe = ( recipients: $ReadOnlyArray<{ user_id: number, ... }>, ownUserId: number, @@ -87,6 +104,10 @@ 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: +// * 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. export const pmKeyRecipientsFromMessage = ( message: Message | Outbox, ownUser: User, @@ -97,6 +118,34 @@ export const pmKeyRecipientsFromMessage = ( return filterRecipients(message.display_recipient, ownUser.user_id); }; +/** + * The set of users to identify a PM conversation by in our data structures. + * + * This produces the same set of users as `pmKeyRecipientsFromMessage`, just + * from a different form of input. + * + * The input may either include or exclude self, without affecting the + * result. + */ +export const pmKeyRecipientsFromIds = ( + userIds: number[], + usersById: Map, + ownUserId: number, +): User[] | null => { + const users = []; + for (const id of userIds) { + if (id === ownUserId && userIds.length > 1) { + continue; + } + const user = usersById.get(id); + if (!user) { + return null; + } + users.push(user); + } + return users; +}; + /** * The key this PM is filed under in the "unread messages" data structure. *