From 009d0c8d9cdfe0080de6a95cfa8e661d42f90f72 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Dec 2020 15:50:42 -0800 Subject: [PATCH 01/17] narrow [nfc]: Update comment classifying PM-constructor call stacks. In 034da03aa the other day, we converted getNarrowForReply (then called getNarrowFromMessage) to use pmKeyRecipientsFromMessage. Then in 436d9711c we added getNarrowsForMessage, which relies on the same helper. Those are the only new call sites I see with `git log --stat -p -S`, searching for `groupNarrow` or `pmNarrowFromEmails`, since this comment was originally compiled in the series ending at ff872f600. --- src/utils/narrow.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 209c36aa509..5d4a504695f 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -71,11 +71,12 @@ const pmNarrowByString = (emails: string): Narrow => [ // 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 +// * Good: getNarrowForReply: also pmKeyRecipientsFromMessage +// * Good: getNarrowsForMessage: also pmKeyRecipientsFromMessage export const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); /** Convenience wrapper for `pmNarrowFromEmails`. */ From e4dfdb12b7b0ea3f5108fccdf225cdb45e789984 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 15:15:21 -0800 Subject: [PATCH 02/17] narrow [nfc]: Add constructor taking Flow-verified proper filtered PM data. The type on this function's argument, an "opaque type alias", guarantees that the data comes from one of our helper functions in `recipient.js`. See jsdoc within. This is one of a series of commits which will ultimately let us ensure that all our PM narrows are constructed with data that (a) observes the right convention for whether to include the self user, (b) is sorted consistently, and (c) contains user IDs as well as emails, so that we can subsequently switch to using user IDs in the Narrow values themselves. To enlist Flow to help us with (a) and (b), we'll use these opaque types to ensure that the data always passes through one of a few helper functions responsible for those conditions. --- src/utils/narrow.js | 20 +++++++++++-- src/utils/recipient.js | 37 ++++++++++++++++++++++++- src/webview/html/messageHeaderAsHtml.js | 4 +-- 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 5d4a504695f..36d9053ef16 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -8,6 +8,7 @@ import { pmKeyRecipientsFromMessage, recipientsOfPrivateMessage, streamNameOfStreamMessage, + type PmKeyRecipients, } from './recipient'; export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => @@ -60,7 +61,7 @@ const pmNarrowByString = (emails: string): Narrow => [ // merely latent only because it doesn't (as far as we know) have any // user-visible effect. // -// Known call stacks: +// Known call stacks not using the validating helpers: // * 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 @@ -73,6 +74,9 @@ const pmNarrowByString = (emails: string): Narrow => [ // to give someone else in it a link to a particular message, say. // * Good: getNarrowFromNotificationData: filters, and starts from // notification's pm_users, which is sorted. +// +// Known call stacks using the validating helpers -- still listed here +// because sorting can still vary, for now: // * Good: messageHeaderAsHtml: comes from pmKeyRecipientsFromMessage, // which filters and sorts by ID // * Good: getNarrowForReply: also pmKeyRecipientsFromMessage @@ -82,6 +86,16 @@ export const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString /** 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. + */ +export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow => + pmNarrowFromEmails(recipients.map(r => r.email)); + export const specialNarrow = (operand: string): Narrow => [ { operator: 'is', @@ -346,7 +360,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)); @@ -375,7 +389,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..b03437b44c2 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -26,6 +26,41 @@ export const recipientsOfPrivateMessage = ( return recipients; }; +/** + * A set 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; + // Filter a list of PM recipients in the quirky way that we do. // // Specifically: all users, except the self-user, except if it's the @@ -157,7 +192,7 @@ export const pmUiRecipientsFromMessage = ( export const pmKeyRecipientsFromMessage = ( message: Message | Outbox, ownUser: User, -): $ReadOnlyArray => { +): PmKeyRecipients => { if (message.type !== 'private') { throw new Error('pmKeyRecipientsFromMessage: expected PM, got stream message'); } 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); From bed2f2a418c3194fcc67694f6f4bb629492162c3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 15:44:31 -0800 Subject: [PATCH 03/17] narrow [nfc]: Add another checked-input constructor, pmNarrowFromUsers. --- src/notification/index.js | 4 ++-- src/utils/internalLinks.js | 4 ++-- src/utils/narrow.js | 19 ++++++++++++++++--- src/utils/recipient.js | 12 +++++++++++- 4 files changed, 31 insertions(+), 8 deletions(-) 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/utils/internalLinks.js b/src/utils/internalLinks.js index 055ceb0c5a8..06357d7f349 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'; @@ -130,7 +130,7 @@ export const getNarrowFromLink = ( case 'pm': { 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 36d9053ef16..12bc74a9d40 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -9,6 +9,7 @@ import { recipientsOfPrivateMessage, streamNameOfStreamMessage, type PmKeyRecipients, + type PmKeyUsers, } from './recipient'; export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => @@ -68,15 +69,15 @@ const pmNarrowByString = (emails: string): Narrow => [ // * OK, email: PmConversationList < PmConversationCard: the data comes // from `getRecentConversations`, which filters and sorts by email // * OK, email: PmConversationList < UnreadCards: ditto +// +// Known call stacks using the validating helpers -- still listed here +// because sorting can still vary, for now: // * 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. // * Good: getNarrowFromNotificationData: filters, and starts from // notification's pm_users, which is sorted. -// -// Known call stacks using the validating helpers -- still listed here -// because sorting can still vary, for now: // * Good: messageHeaderAsHtml: comes from pmKeyRecipientsFromMessage, // which filters and sorts by ID // * Good: getNarrowForReply: also pmKeyRecipientsFromMessage @@ -92,10 +93,22 @@ export const pmNarrowFromEmail = (email: string): Narrow => pmNarrowFromEmails([ * 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. + */ +export const pmNarrowFromUsers = (recipients: PmKeyUsers): Narrow => + pmNarrowFromEmails(recipients.map(r => r.email)); + export const specialNarrow = (operand: string): Narrow => [ { operator: 'is', diff --git a/src/utils/recipient.js b/src/utils/recipient.js index b03437b44c2..943c138d06e 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -61,6 +61,16 @@ export const recipientsOfPrivateMessage = ( */ export opaque type PmKeyRecipients: $ReadOnlyArray = $ReadOnlyArray; +/** + * A set 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. // // Specifically: all users, except the self-user, except if it's the @@ -212,7 +222,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) { From accccba38f5b93c5b4b6342407f491012e8c14f5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 17:14:09 -0800 Subject: [PATCH 04/17] narrow [nfc]: Add constructor pm1to1NarrowFromUser. This is just like the old pmNarrowFromEmail, but takes a whole user object. Using this function guarantees that we *have* a user object on hand -- and therefore not just an email but a user ID. Once we're using this and other such constructors exclusively, that will enable us to start identifying PM narrows by user IDs, rather than by emails. --- src/account-info/AccountDetailsScreen.js | 4 ++-- src/pm-conversations/PmConversationList.js | 4 ++-- src/users/UsersCard.js | 4 ++-- src/utils/narrow.js | 22 ++++++++++++++++++++-- 4 files changed, 26 insertions(+), 8 deletions(-) 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/pm-conversations/PmConversationList.js b/src/pm-conversations/PmConversationList.js index f6d335bc9be..e02e6e8da6c 100644 --- a/src/pm-conversations/PmConversationList.js +++ b/src/pm-conversations/PmConversationList.js @@ -4,7 +4,7 @@ import { FlatList } from 'react-native'; import type { Dispatch, PmConversationData, UserOrBot } from '../types'; import { createStyleSheet } from '../styles'; -import { pmNarrowFromEmail, pmNarrowFromEmails } from '../utils/narrow'; +import { pm1to1NarrowFromUser, pmNarrowFromEmails } from '../utils/narrow'; import UserItem from '../users/UserItem'; import GroupPmConversationItem from './GroupPmConversationItem'; import { doNarrow } from '../actions'; @@ -27,7 +27,7 @@ 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) => { 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/utils/narrow.js b/src/utils/narrow.js index 12bc74a9d40..8db438299b2 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -2,7 +2,7 @@ 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, @@ -84,7 +84,13 @@ const pmNarrowByString = (emails: string): Narrow => [ // * Good: getNarrowsForMessage: also pmKeyRecipientsFromMessage export const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); -/** Convenience wrapper for `pmNarrowFromEmails`. */ +/** + * DEPRECATED. Convenience wrapper for `pmNarrowFromEmails`. + * + * 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. + */ export const pmNarrowFromEmail = (email: string): Narrow => pmNarrowFromEmails([email]); /** @@ -105,10 +111,22 @@ export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow => * * 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)); +/** + * 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', From 4dc644b9b900f78345fa962c923968eecd098f3f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 17:30:30 -0800 Subject: [PATCH 05/17] narrow [nfc]: Migrate some tests pmNarrowFromEmail -> pm1to1NarrowFromUser. Done like so: $ perl -i -0pe ' s/pmNarrowFromEmail\((.*?)\.email\)/pm1to1NarrowFromUser($1)/g; s/\bpmNarrowFromEmail\b/pm1to1NarrowFromUser/g ' src/typing/__tests__/typingSelectors-test.js src/chat/__tests__/narrowsReducer-test.js $ tools/fmt The file limitation is mainly to keep it to files where the old can be eliminated entirely this way. A few other files call it with literal strings, so need a bit more work first. --- src/chat/__tests__/narrowsReducer-test.js | 10 +++++----- src/typing/__tests__/typingSelectors-test.js | 11 +++++------ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 337af9263f7..6d000858820 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -6,7 +6,7 @@ import narrowsReducer from '../narrowsReducer'; import { HOME_NARROW, HOME_NARROW_STR, - pmNarrowFromEmail, + pm1to1NarrowFromUser, ALL_PRIVATE_NARROW_STR, pmNarrowFromEmails, streamNarrow, @@ -23,7 +23,7 @@ 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 privateNarrowStr = JSON.stringify(pm1to1NarrowFromUser(eg.otherUser)); const groupNarrowStr = JSON.stringify( pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]), ); @@ -147,7 +147,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 +373,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 +383,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/typing/__tests__/typingSelectors-test.js b/src/typing/__tests__/typingSelectors-test.js index 5cabaee5d68..4cff4ab5e95 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, pmNarrowFromEmails } 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]); }); @@ -63,7 +63,7 @@ describe('getCurrentTypingUsers', () => { users: [user1, user2], }); - const typingUsers = getCurrentTypingUsers(state, pmNarrowFromEmail(user2.email)); + const typingUsers = getCurrentTypingUsers(state, pm1to1NarrowFromUser(user2)); expect(typingUsers).toEqual(NULL_ARRAY); }); @@ -99,7 +99,7 @@ describe('getCurrentTypingUsers', () => { }); const getTypingUsers = () => - getCurrentTypingUsers(state, pmNarrowFromEmail(deactivatedUser.email)); + getCurrentTypingUsers(state, pm1to1NarrowFromUser(deactivatedUser)); expect(getTypingUsers).not.toThrow(); expect(getTypingUsers()).toEqual([]); @@ -112,8 +112,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([]); From 7d15c5b0b7f86b2709e7f13c69ca67b10cfe2c5a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Dec 2020 17:57:08 -0800 Subject: [PATCH 06/17] narrow [nfc]: Add pmNarrowFromUsersUnsafe, for use in tests. Using this instead of pmNarrowFromEmails will do two things for us: * It helps us reduce to zero the usage of the email-based constructors, so that we can switch to using user IDs in our Narrow objects. * It sorts the users by ID automatically. As we move over the next few commits toward doing the same thing consistently in app code, we can have tests switch to this function to follow along, saving us from manually writing out the sort in a bunch of spots in our tests. --- src/utils/narrow.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 8db438299b2..60a551dff97 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -118,6 +118,25 @@ export const pmNarrowFromRecipients = (recipients: PmKeyRecipients): Narrow => 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. * From 957672780ae7258d8764956c0a874270ef3469ad Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Dec 2020 16:27:13 -0800 Subject: [PATCH 07/17] recipient [nfc]: Sort (by user ID) in pmKeyRecipientsFromMessage. This also starts us sorting by ID in pmUiRecipientsFromMessage. But pmKeyRecipientsFromMessage is more interesting, because there it'll shortly make sense to make that part of its interface. In practice, for both functions it's already sorted because that's how a group PM's display_recipient comes from the server. The similar change in filterRecipientsAsUserIds is there for parallelism, but has no effect because its caller already sorts the same way anyway. On filterRecipientsByEmail we don't have the user IDs, so we can't do our standard sorting. This is OK because its only caller does its own sorting anyway. --- src/utils/__tests__/narrow-test.js | 5 +++-- src/utils/recipient.js | 10 ++++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 81496b9689f..3466400bb72 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -6,6 +6,7 @@ import { pmNarrowFromEmail, is1to1PmNarrow, pmNarrowFromEmails, + pmNarrowFromUsersUnsafe, isGroupPmNarrow, specialNarrow, isSpecialNarrow, @@ -388,7 +389,7 @@ describe('getNarrowsForMessage', () => { expectedNarrows: [ HOME_NARROW, ALL_PRIVATE_NARROW, - pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]), + pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]), ], }, ]; @@ -421,7 +422,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/recipient.js b/src/utils/recipient.js index 943c138d06e..7563fe05e0e 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -83,16 +83,22 @@ 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 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, From d50c34c1b5cf144b435a5e1bfdf8327444c10527 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Dec 2020 17:07:38 -0800 Subject: [PATCH 08/17] recipient: Almost always sort recipient lists, by user ID. This is "almost" because we don't do so in the deprecated `normalizeRecipientsSansMe`, which only takes emails as input. There's also still one codepath (through CreateGroupScreen) where we construct a PM narrow without going through the recipient helpers at all. In upcoming commits we'll make this rule universal for constructing PM narrows. --- .../__tests__/notification-test.js | 4 +-- src/utils/__tests__/internalLinks-test.js | 8 +++--- src/utils/narrow.js | 22 +++++++-------- src/utils/recipient.js | 28 ++++++++++--------- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 3254ee0d98b..6e65549588e 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); 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/narrow.js b/src/utils/narrow.js index 60a551dff97..098ce3084d7 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -70,18 +70,16 @@ const pmNarrowByString = (emails: string): Narrow => [ // from `getRecentConversations`, which filters and sorts by email // * OK, email: PmConversationList < UnreadCards: ditto // -// Known call stacks using the validating helpers -- still listed here -// because sorting can still vary, for now: -// * 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. -// * Good: getNarrowFromNotificationData: filters, and starts from -// notification's pm_users, which is sorted. -// * Good: messageHeaderAsHtml: comes from pmKeyRecipientsFromMessage, -// which filters and sorts by ID -// * Good: getNarrowForReply: also pmKeyRecipientsFromMessage -// * Good: getNarrowsForMessage: also pmKeyRecipientsFromMessage +// Known call stacks using the validating helpers -- which guarantee not +// only filtering but sorting by ID, hooray: +// * 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. +// * getNarrowFromNotificationData +// * messageHeaderAsHtml +// * getNarrowForReply +// * getNarrowsForMessage export const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); /** diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 7563fe05e0e..d6fdd8f0a32 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -27,7 +27,7 @@ export const recipientsOfPrivateMessage = ( }; /** - * A set of users identifying a PM conversation, as per pmKeyRecipientsFromMessage. + * 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/ @@ -62,7 +62,7 @@ export const recipientsOfPrivateMessage = ( export opaque type PmKeyRecipients: $ReadOnlyArray = $ReadOnlyArray; /** - * A set of users identifying a PM conversation, as per pmKeyRecipientsFromMessage. + * 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. @@ -71,10 +71,10 @@ export opaque type PmKeyRecipients: $ReadOnlyArray = $ReadOnlyA */ export opaque type PmKeyUsers: $ReadOnlyArray = $ReadOnlyArray; -// Filter a list of PM recipients in the quirky way that we do. +// 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 @@ -133,7 +133,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. @@ -149,9 +149,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 @@ -179,7 +177,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 @@ -201,10 +201,12 @@ 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, @@ -216,9 +218,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 @@ -240,7 +242,7 @@ export const pmKeyRecipientsFromIds = ( } users.push(user); } - return users; + return users.sort((a, b) => a.user_id - b.user_id); }; /** From dd464e06db34425df9ee30a2e8e36239bf5fe445 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Dec 2020 17:15:07 -0800 Subject: [PATCH 09/17] CreateGroupScreen: Sort users for constructing PM narrow. Better yet will be to make this use whole User objects, and go through a central, validating helper. We'll do that a bit later in the branch, but first this is a very easy change that gets this callsite out of the way of universally consistent sorting. --- src/user-groups/CreateGroupScreen.js | 2 +- src/utils/narrow.js | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/user-groups/CreateGroupScreen.js b/src/user-groups/CreateGroupScreen.js index e5a181894ed..2eb4196d886 100644 --- a/src/user-groups/CreateGroupScreen.js +++ b/src/user-groups/CreateGroupScreen.js @@ -34,7 +34,7 @@ class CreateGroupScreen extends PureComponent { handleCreateGroup = (selected: User[]) => { const { dispatch } = this.props; - const recipients = selected.map(user => user.email); + const recipients = selected.sort((a, b) => a.user_id - b.user_id).map(user => user.email); NavigationService.dispatch(navigateBack()); dispatch(doNarrow(pmNarrowFromEmails(recipients))); }; diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 098ce3084d7..057ab1890fb 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -63,9 +63,8 @@ const pmNarrowByString = (emails: string): Narrow => [ // user-visible effect. // // Known call stacks not using the validating helpers: -// * 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, perilously: CreateGroupScreen: the self user isn't offered in the +// UI, so effectively the list is filtered; does sort by ID // * OK, email: PmConversationList < PmConversationCard: the data comes // from `getRecentConversations`, which filters and sorts by email // * OK, email: PmConversationList < UnreadCards: ditto From c78e5052083ff0161bc9bc8ac3e0282aaaa7c220 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 17:55:00 -0800 Subject: [PATCH 10/17] users [nfc]: Rename some usersBy* -> allUsersBy* where that's already true. This doesn't actually change what data we use in any of these codepaths -- just the names. Ultimately we probably want to be have just one map, namely "all users, by ID", and drop the "all" from its name because we'll stop having to think about the distinction. But until we're using "all users" maps everywhere, the names will help us track where we are vs. where we aren't yet. A few of these were recently changed to be "all users"; most of them have been for a long time. --- src/notification/__tests__/notification-test.js | 4 ++-- src/pm-conversations/GroupPmConversationItem.js | 6 +++--- src/pm-conversations/PmConversationList.js | 8 ++++---- src/pm-conversations/PmConversationsCard.js | 8 ++++---- src/unread/UnreadCards.js | 4 ++-- src/unread/unreadSelectors.js | 6 +++--- src/users/usersActions.js | 4 ++-- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 6e65549588e..c48bf1ed312 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -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/pm-conversations/GroupPmConversationItem.js b/src/pm-conversations/GroupPmConversationItem.js index 65e844c2c9f..57ec9981739 100644 --- a/src/pm-conversations/GroupPmConversationItem.js +++ b/src/pm-conversations/GroupPmConversationItem.js @@ -16,7 +16,7 @@ const componentStyles = createStyleSheet({ type Props = $ReadOnly<{| email: string, - usersByEmail: Map, + allUsersByEmail: Map, unreadCount: number, onPress: (emails: string) => void, |}>; @@ -31,8 +31,8 @@ export default class GroupPmConversationItem extends PureComponent { }; render() { - const { email, usersByEmail, unreadCount } = this.props; - const allUsers = email.split(',').map(e => usersByEmail.get(e)); + const { email, allUsersByEmail, unreadCount } = this.props; + const allUsers = email.split(',').map(e => allUsersByEmail.get(e)); const allUsersFound = allUsers.every(user => user); diff --git a/src/pm-conversations/PmConversationList.js b/src/pm-conversations/PmConversationList.js index e02e6e8da6c..ad292c8f5b6 100644 --- a/src/pm-conversations/PmConversationList.js +++ b/src/pm-conversations/PmConversationList.js @@ -19,7 +19,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| dispatch: Dispatch, conversations: PmConversationData[], - usersByEmail: Map, + allUsersByEmail: Map, |}>; /** @@ -35,7 +35,7 @@ export default class PmConversationList extends PureComponent { }; render() { - const { conversations, usersByEmail } = this.props; + const { conversations, allUsersByEmail } = this.props; return ( { keyExtractor={item => item.recipients} renderItem={({ item }) => { if (item.recipients.indexOf(',') === -1) { - const user = usersByEmail.get(item.recipients); + const user = allUsersByEmail.get(item.recipients); if (!user) { return null; @@ -60,7 +60,7 @@ export default class PmConversationList extends PureComponent { ); 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/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/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'); } From 4ed0c138b8fc93f0c6a1894b695cd9d07591308d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Dec 2020 13:43:44 -0800 Subject: [PATCH 11/17] pm-conversations tests: Drop reliance on specific user IDs. This ensures that we can adequately test whether and how we're sorting the users in the output. --- .../__tests__/pmConversationsSelectors-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index 7759bfa8e52..e06333033da 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({ From 077574ef8410a22b268b54e1dd2b4f19be34fe6e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 18:06:24 -0800 Subject: [PATCH 12/17] PmConversationList [nfc]: Pass real Users, not joined-emails string. We're still getting joined-email strings as input here, though; we'll convert those to real user objects too in a subsequent commit. --- .../GroupPmConversationItem.js | 26 +++++---------- src/pm-conversations/PmConversationList.js | 32 ++++++++++--------- 2 files changed, 25 insertions(+), 33 deletions(-) diff --git a/src/pm-conversations/GroupPmConversationItem.js b/src/pm-conversations/GroupPmConversationItem.js index 57ec9981739..c9c2093b097 100644 --- a/src/pm-conversations/GroupPmConversationItem.js +++ b/src/pm-conversations/GroupPmConversationItem.js @@ -15,10 +15,9 @@ const componentStyles = createStyleSheet({ }); type Props = $ReadOnly<{| - email: string, - allUsersByEmail: Map, + users: $ReadOnlyArray, unreadCount: number, - onPress: (emails: string) => void, + onPress: (users: $ReadOnlyArray) => void, |}>; /** @@ -26,32 +25,23 @@ type Props = $ReadOnly<{| * */ export default class GroupPmConversationItem extends PureComponent { handlePress = () => { - const { email, onPress } = this.props; - onPress(email); + const { users, onPress } = this.props; + onPress(users); }; render() { - const { email, allUsersByEmail, unreadCount } = this.props; - const allUsers = email.split(',').map(e => allUsersByEmail.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 ad292c8f5b6..e195e03639b 100644 --- a/src/pm-conversations/PmConversationList.js +++ b/src/pm-conversations/PmConversationList.js @@ -30,8 +30,8 @@ export default class PmConversationList extends PureComponent { this.props.dispatch(doNarrow(pm1to1NarrowFromUser(user))); }; - handleGroupNarrow = (email: string) => { - this.props.dispatch(doNarrow(pmNarrowFromEmails(email.split(',')))); + handleGroupNarrow = (users: $ReadOnlyArray) => { + this.props.dispatch(doNarrow(pmNarrowFromEmails(users.map(u => u.email)))); }; render() { @@ -44,26 +44,28 @@ export default class PmConversationList extends PureComponent { data={conversations} keyExtractor={item => item.recipients} renderItem={({ item }) => { - if (item.recipients.indexOf(',') === -1) { - const user = allUsersByEmail.get(item.recipients); - + const users = []; + for (const email of item.recipients.split(',')) { + const user = allUsersByEmail.get(email); if (!user) { return null; } + users.push(user); + } + if (users.length === 1) { return ( - + + ); + } else { + return ( + ); } - - return ( - - ); }} /> ); From 3a23c4952ae93b470ec49aa11f903cb460b7b0c3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 19:17:43 -0800 Subject: [PATCH 13/17] PmConversationList: Take real users as our view-model data, too. In the previous commit, we stopped passing these joined-email strings down to GroupPmConversationItem. This commit continues that by eliminating them at the next step back, where we were generating them; and it lets us construct the chosen PM narrow using real users, with a type that guarantees they've been appropriately filtered and sorted by our recipient helpers. By relying on our centralized recipient-processing helpers, this also gets the users sorted by ID -- which eliminates the last remaining mismatch between how we sort users to describe a PM conversation, fixing a latent bug. --- .../GroupPmConversationItem.js | 10 +++-- src/pm-conversations/PmConversationList.js | 21 ++++------ .../pmConversationsSelectors-test.js | 34 +++++++-------- .../pmConversationsSelectors.js | 41 ++++++++----------- src/types.js | 5 ++- src/utils/narrow.js | 20 +++------ src/utils/recipient.js | 12 ++++++ 7 files changed, 68 insertions(+), 75 deletions(-) diff --git a/src/pm-conversations/GroupPmConversationItem.js b/src/pm-conversations/GroupPmConversationItem.js index c9c2093b097..13d7863ce4b 100644 --- a/src/pm-conversations/GroupPmConversationItem.js +++ b/src/pm-conversations/GroupPmConversationItem.js @@ -14,16 +14,18 @@ const componentStyles = createStyleSheet({ }, }); -type Props = $ReadOnly<{| - users: $ReadOnlyArray, +type Props = $ReadOnly<{| + users: U, unreadCount: number, - onPress: (users: $ReadOnlyArray) => 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 { users, onPress } = this.props; onPress(users); diff --git a/src/pm-conversations/PmConversationList.js b/src/pm-conversations/PmConversationList.js index e195e03639b..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 { pm1to1NarrowFromUser, 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'; @@ -30,29 +31,21 @@ export default class PmConversationList extends PureComponent { this.props.dispatch(doNarrow(pm1to1NarrowFromUser(user))); }; - handleGroupNarrow = (users: $ReadOnlyArray) => { - this.props.dispatch(doNarrow(pmNarrowFromEmails(users.map(u => u.email)))); + handleGroupNarrow = (users: PmKeyUsers) => { + this.props.dispatch(doNarrow(pmNarrowFromUsers(users))); }; render() { - const { conversations, allUsersByEmail } = this.props; + const { conversations } = this.props; return ( item.recipients} + keyExtractor={item => item.key} renderItem={({ item }) => { - const users = []; - for (const email of item.recipients.split(',')) { - const user = allUsersByEmail.get(email); - if (!user) { - return null; - } - users.push(user); - } - + const users = item.keyRecipients; if (users.length === 1) { return ( diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index e06333033da..cd1dfe96c47 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -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/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/utils/narrow.js b/src/utils/narrow.js index 057ab1890fb..4358b403095 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -48,29 +48,19 @@ const pmNarrowByString = (emails: string): Narrow => [ * 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 +// Ideally, all callers should agree on how they're sorted, too. If not, 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. +// group PM conversation. But it appears that we now do sort consistently, +// by user ID. // // Known call stacks not using the validating helpers: // * OK, perilously: CreateGroupScreen: the self user isn't offered in the // UI, so effectively the list is filtered; does sort by ID -// * OK, email: PmConversationList < PmConversationCard: the data comes -// from `getRecentConversations`, which filters and sorts by email -// * OK, email: PmConversationList < UnreadCards: ditto // // Known call stacks using the validating helpers -- which guarantee not // only filtering but sorting by ID, hooray: +// * PmConversationList < PmConversationCard +// * PmConversationList < UnreadCards // * 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 diff --git a/src/utils/recipient.js b/src/utils/recipient.js index d6fdd8f0a32..2a166cb3af0 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -245,6 +245,18 @@ export const pmKeyRecipientsFromIds = ( 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); +}; + /** * The key this PM is filed under in the "unread messages" data structure. * From 6343d051b3fcf5e3321b4dc759ead1e49fd4f0a8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Dec 2020 17:31:22 -0800 Subject: [PATCH 14/17] recipient [nfc]: One more validating helper; convert the last PM callsite! We're now down to zero callsites (in non-test code) that call pmNarrowFromEmails directly! All callers that construct a PM narrow now use one of our new constructors that require types containing user IDs as well as emails, and constructible only by helpers that guarantee they properly filter and sort the list. This change is NFC because this callsite was already sorting the list, and the users in it are chosen from a UI which doesn't include the self user in the first place. --- src/user-groups/CreateGroupScreen.js | 19 +++++++++++++------ src/utils/narrow.js | 4 ++-- src/utils/recipient.js | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/user-groups/CreateGroupScreen.js b/src/user-groups/CreateGroupScreen.js index 2eb4196d886..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.sort((a, b) => a.user_id - b.user_id).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/utils/narrow.js b/src/utils/narrow.js index 4358b403095..ab3c8088e45 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -54,11 +54,11 @@ const pmNarrowByString = (emails: string): Narrow => [ // by user ID. // // Known call stacks not using the validating helpers: -// * OK, perilously: CreateGroupScreen: the self user isn't offered in the -// UI, so effectively the list is filtered; does sort by ID +// none! // // Known call stacks using the validating helpers -- which guarantee not // only filtering but sorting by ID, hooray: +// * CreateGroupScreen // * PmConversationList < PmConversationCard // * PmConversationList < UnreadCards // * getNarrowFromLink. Though there's basically a bug in the webapp, diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 2a166cb3af0..bc8483ebb2b 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -87,6 +87,15 @@ const filterRecipients = ( ? 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, @@ -257,6 +266,14 @@ export const pmKeyRecipientUsersFromMessage = ( 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. * From 5814cb541224417b6eb3215dc97672c735b9fa32 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Dec 2020 16:47:05 -0800 Subject: [PATCH 15/17] tests [nfc]: Well-type a few more tests, and avoid magic-coincidence strings. These are prompted by having calls to pmNarrowFromEmails, which we're about to systematically eliminate. --- .../getComposeInputPlaceholder-test.js | 56 ++++++------------- src/title/__tests__/titleSelectors-test.js | 35 ++++-------- src/utils/__tests__/narrow-test.js | 34 +++++------ 3 files changed, 47 insertions(+), 78 deletions(-) diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index 9132e52dfd2..b9b9a8daef5 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'; @@ -7,65 +8,42 @@ import { topicNarrow, pmNarrowFromEmails, } 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(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])); + const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Message group' }); }); }); diff --git a/src/title/__tests__/titleSelectors-test.js b/src/title/__tests__/titleSelectors-test.js index 2ace21ff47b..e780acb8b2d 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 * 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, pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])), ).toEqual(DEFAULT_TITLE_BACKGROUND_COLOR); }); }); diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 3466400bb72..8cb15b95769 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -42,22 +42,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', + operand: eg.otherUser.email, }, ]), ).toBe(true); @@ -66,23 +66,25 @@ describe('pmNarrowFromEmail', () => { describe('pmNarrowFromEmails', () => { test('returns a narrow with specified recipients', () => { - expect(pmNarrowFromEmails(['bob@example.com', 'john@example.com'])).toEqual([ + expect(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])).toEqual([ { operator: 'pm-with', - operand: 'bob@example.com,john@example.com', + operand: [eg.otherUser.email, eg.thirdUser.email].join(','), }, ]); }); 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(pmNarrowFromEmail(eg.otherUser.email))).toBe(false); + expect(isGroupPmNarrow(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]))).toBe( + true, + ); expect( isGroupPmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com', + operand: eg.otherUser.email, }, ]), ).toBe(false); @@ -90,7 +92,7 @@ describe('pmNarrowFromEmails', () => { isGroupPmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com,john@example.com', + operand: [eg.otherUser.email, eg.thirdUser.email].join(','), }, ]), ).toBe(true); @@ -101,13 +103,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(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]))).toBe(true); expect( isPmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com', + operand: eg.otherUser.email, }, ]), ).toBe(true); @@ -115,7 +117,7 @@ describe('isPmNarrow', () => { isPmNarrow([ { operator: 'pm-with', - operand: 'bob@example.com,john@example.com', + operand: [eg.otherUser.email, eg.thirdUser.email].join(','), }, ]), ).toBe(true); @@ -128,9 +130,9 @@ 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(pmNarrowFromEmail(eg.otherUser.email))).toBe(false); expect( - isStreamOrTopicNarrow(pmNarrowFromEmails(['john@example.com', 'mark@example.com'])), + isStreamOrTopicNarrow(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])), ).toBe(false); expect(isStreamOrTopicNarrow(STARRED_NARROW)).toBe(false); }); From bb121e4cebf63698317e4170b1d950fef5b6f4b6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 9 Dec 2020 19:46:31 -0800 Subject: [PATCH 16/17] narrow [nfc]: Cut uses of pmNarrowFromEmails from tests, too. Now the only calls to this function are in the higher-level Narrow constructors, and in its own tests. Done automatically with these commands: $ perl -i -0pe ' s#pmNarrowFromEmails\(\[(\S+\.email,?\s*)*\]\)# $& =~ s/(\S+)\.email/$1/gr =~ s/^\w+\(\[(.*)\]\)/pmNarrowFromUsersUnsafe([$1])/r #eg && s/pmNarrowFromEmails(?=[, ])/pmNarrowFromUsersUnsafe/g ' src/**/__tests__/*.js $ tools/fmt followed by restoring the handful of references in its own tests. --- src/chat/__tests__/narrowsReducer-test.js | 6 ++---- src/chat/__tests__/narrowsSelectors-test.js | 6 +++--- .../__tests__/getComposeInputPlaceholder-test.js | 4 ++-- src/title/__tests__/titleSelectors-test.js | 4 ++-- src/typing/__tests__/typingSelectors-test.js | 9 +++------ src/utils/__tests__/narrow-test.js | 12 ++++++------ 6 files changed, 18 insertions(+), 23 deletions(-) diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 6d000858820..f4700739a2a 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -8,7 +8,7 @@ import { HOME_NARROW_STR, pm1to1NarrowFromUser, ALL_PRIVATE_NARROW_STR, - pmNarrowFromEmails, + pmNarrowFromUsersUnsafe, streamNarrow, topicNarrow, STARRED_NARROW_STR, @@ -24,9 +24,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('narrowsReducer', () => { const privateNarrowStr = JSON.stringify(pm1to1NarrowFromUser(eg.otherUser)); - const groupNarrowStr = JSON.stringify( - pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]), - ); + 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)); 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 b9b9a8daef5..d93deef2c8d 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -6,7 +6,7 @@ import { pmNarrowFromEmail, streamNarrow, topicNarrow, - pmNarrowFromEmails, + pmNarrowFromUsersUnsafe, } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; @@ -42,7 +42,7 @@ describe('getComposeInputPlaceholder', () => { }); test('returns "Message group" object for group narrow', () => { - const narrow = deepFreeze(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])); + const narrow = deepFreeze(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); expect(placeholder).toEqual({ text: 'Message group' }); }); diff --git a/src/title/__tests__/titleSelectors-test.js b/src/title/__tests__/titleSelectors-test.js index e780acb8b2d..c944bdaafce 100644 --- a/src/title/__tests__/titleSelectors-test.js +++ b/src/title/__tests__/titleSelectors-test.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { DEFAULT_TITLE_BACKGROUND_COLOR, getTitleBackgroundColor } from '../titleSelectors'; -import { pmNarrowFromEmails, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow'; +import { pmNarrowFromUsersUnsafe, streamNarrow, pmNarrowFromEmail } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; describe('getTitleBackgroundColor', () => { @@ -28,7 +28,7 @@ describe('getTitleBackgroundColor', () => { DEFAULT_TITLE_BACKGROUND_COLOR, ); expect( - getTitleBackgroundColor(state, pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])), + getTitleBackgroundColor(state, pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])), ).toEqual(DEFAULT_TITLE_BACKGROUND_COLOR); }); }); diff --git a/src/typing/__tests__/typingSelectors-test.js b/src/typing/__tests__/typingSelectors-test.js index 4cff4ab5e95..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, pm1to1NarrowFromUser, 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'; @@ -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]); }); @@ -85,7 +82,7 @@ describe('getCurrentTypingUsers', () => { const typingUsers = getCurrentTypingUsers( state, - pmNarrowFromEmails([expectedUser.email, anotherUser.email]), + pmNarrowFromUsersUnsafe([expectedUser, anotherUser]), ); expect(typingUsers).toEqual([expectedUser]); diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 8cb15b95769..80f55d89057 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -104,7 +104,7 @@ describe('isPmNarrow', () => { expect(isPmNarrow(undefined)).toBe(false); expect(isPmNarrow(HOME_NARROW)).toBe(false); expect(isPmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); - expect(isPmNarrow(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]))).toBe(true); + expect(isPmNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe(true); expect( isPmNarrow([ { @@ -131,9 +131,9 @@ describe('isStreamOrTopicNarrow', () => { expect(isStreamOrTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true); expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false); expect(isStreamOrTopicNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(false); - expect( - isStreamOrTopicNarrow(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])), - ).toBe(false); + expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe( + false, + ); expect(isStreamOrTopicNarrow(STARRED_NARROW)).toBe(false); }); }); @@ -258,7 +258,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()], @@ -266,7 +266,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()], From 998947d8073f4b339d8086824e5cfc8703577c33 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Dec 2020 17:18:08 -0800 Subject: [PATCH 17/17] narrow [nfc]: Unexport pmNarrowFromEmails! The only remaining calls to this function are the ones within its own module, including the higher-level, validating constructors. As a result: * All PM narrows we construct are validated to have the right set of users, including or excluding self consistently (except some in test data that use pmNarrowFromUsersUnsafe for convenience.) * All PM narrows we construct are validated to have consistent sorting of the users. * Almost all PM narrows we construct are through constructors that receive user IDs as well as emails, setting us up to start recording user IDs in the Narrow objects in the future. The remaining exceptions are some 1:1 narrows constructed through pmNarrowFromEmail; these consist of some test data and one remaining non-test callsite. --- src/utils/__tests__/narrow-test.js | 37 ------------------------- src/utils/internalLinks.js | 5 ++++ src/utils/narrow.js | 44 +++++++++++------------------- 3 files changed, 21 insertions(+), 65 deletions(-) diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 80f55d89057..70be9f78ae9 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -5,9 +5,7 @@ import { isHomeNarrow, pmNarrowFromEmail, is1to1PmNarrow, - pmNarrowFromEmails, pmNarrowFromUsersUnsafe, - isGroupPmNarrow, specialNarrow, isSpecialNarrow, ALL_PRIVATE_NARROW, @@ -64,41 +62,6 @@ describe('pmNarrowFromEmail', () => { }); }); -describe('pmNarrowFromEmails', () => { - test('returns a narrow with specified recipients', () => { - expect(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email])).toEqual([ - { - operator: 'pm-with', - operand: [eg.otherUser.email, eg.thirdUser.email].join(','), - }, - ]); - }); - - test('a group narrow is only private chat with more than one recipients', () => { - expect(isGroupPmNarrow(HOME_NARROW)).toBe(false); - expect(isGroupPmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(false); - expect(isGroupPmNarrow(pmNarrowFromEmails([eg.otherUser.email, eg.thirdUser.email]))).toBe( - true, - ); - expect( - isGroupPmNarrow([ - { - operator: 'pm-with', - operand: eg.otherUser.email, - }, - ]), - ).toBe(false); - expect( - isGroupPmNarrow([ - { - operator: 'pm-with', - operand: [eg.otherUser.email, eg.thirdUser.email].join(','), - }, - ]), - ).toBe(true); - }); -}); - describe('isPmNarrow', () => { test('a private or group narrow is any "pm-with" narrow', () => { expect(isPmNarrow(undefined)).toBe(false); diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 06357d7f349..3abbb7aaffe 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -128,6 +128,11 @@ 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 === null ? null : pmNarrowFromUsers(users); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index ab3c8088e45..bba9bf09192 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -41,38 +41,26 @@ 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. */ -// Ideally, all callers should agree on how they're sorted, too. If not, we -// can wind up with several distinct narrows that are actually the same -// group PM conversation. But it appears that we now do sort consistently, -// by user ID. -// -// Known call stacks not using the validating helpers: -// none! -// -// Known call stacks using the validating helpers -- which guarantee not -// only filtering but sorting by ID, hooray: -// * CreateGroupScreen -// * PmConversationList < PmConversationCard -// * PmConversationList < UnreadCards -// * 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. -// * getNarrowFromNotificationData -// * messageHeaderAsHtml -// * getNarrowForReply -// * getNarrowsForMessage -export const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); +const pmNarrowFromEmails = (emails: string[]): Narrow => pmNarrowByString(emails.join()); /** - * DEPRECATED. Convenience wrapper for `pmNarrowFromEmails`. + * 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