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/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 6513cca5b02..865fc783bc2 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -124,8 +124,10 @@ describe('decodeHashComponent', () => { }); describe('getNarrowFromLink', () => { - const [userA, userB, userC] = [eg.makeUser(), eg.makeUser(), eg.makeUser()]; - const usersById: Map = new Map([userA, userB, userC].map(u => [u.user_id, u])); + 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' }); @@ -135,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', () => { @@ -252,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(...usersById.keys()); - const ids = `${userA.user_id},${userB.user_id},${otherId}`; + const ids = `${userB.user_id},${otherId}`; expect(get(`https://example.com/#narrow/pm-with/${ids}-group`)).toEqual(null); }); @@ -269,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/internalLinks.js b/src/utils/internalLinks.js index 5800efe1122..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,20 +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; - } - // BUG: should normalize recipients; see comment on groupNarrow. - // (We're parsing a link someone wrote in a message, so the server - // gives us no guarantees here.) - 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 80231050bac..a6f92090e59 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -25,8 +25,8 @@ 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 there may be - * bugs where they don't; some consumers of this data re-normalize to be sure. + * `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. @@ -45,21 +45,17 @@ export const privateNarrow = (email: string): Narrow => [ // merely latent only because it doesn't (as far as we know) have any // user-visible effect. // -// But we also have some callers that don't even ensure the set is the right -// one, with the self-user properly there or not. Known call stacks: -// * BUG, ish: getNarrowFromLink doesn't ensure this precondition is met. -// And... 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. But conversely I guess it means -// that the mobile app actually works just as well as the webapp on the -// links people generate from the webapp. +// 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.