From 6253954f437150c74fc83a0b72076904f7ef6852 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 22:15:40 -0800 Subject: [PATCH 01/11] narrow [nfc]: Use IDs, not emails, in a couple of trivial predicates. --- src/utils/narrow.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index c57793bee02..e603ce79486 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -337,10 +337,10 @@ export const isHomeNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { home: () => true }, () => false); export const is1to1PmNarrow = (narrow?: Narrow): boolean => - !!narrow && caseNarrowDefault(narrow, { pm: emails => emails.length === 1 }, () => false); + !!narrow && caseNarrowDefault(narrow, { pm: (emails, ids) => ids.length === 1 }, () => false); export const isGroupPmNarrow = (narrow?: Narrow): boolean => - !!narrow && caseNarrowDefault(narrow, { pm: emails => emails.length > 1 }, () => false); + !!narrow && caseNarrowDefault(narrow, { pm: (emails, ids) => ids.length > 1 }, () => false); /** * The recipients' emails if a group PM narrow; else error. From 0a5fa1d536a211aa2c249a3d4dbe0dcbb61baab3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 22 Dec 2020 22:27:32 -0800 Subject: [PATCH 02/11] narrow: Check IDs as well as emails in isNarrowValid. We use this function to avoid trying to load UI for a narrow where the UI is doomed to crash because we're lacking basic data about it: in particular, a PM conversation where one of the putative participants is a user we have no data for. One way that can happen is if a user has changed their email address, and either we learn about the narrow by their old email address, or we still have them by their old email address and learn about the narrow by their new one. In the course of migrating code from using the emails in a PM-conversation narrow to using the user IDs, we're about to start doing so with code that will look the users up in our data structures to get further information like full name or avatar. So, in the spirit of "crunchy shell", we should apply the same checks here to the user IDs. Then at the end of this migration series, when we've converted everything else that consumes a PM narrow so that it uses IDs instead of emails, we can drop the email part here because it'll no longer be possible for it to matter. --- src/chat/narrowsSelectors.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 7e5600dc0f4..f3c338c8662 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -22,7 +22,7 @@ import { getOutbox, } from '../directSelectors'; import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors'; -import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors'; +import { getAllUsersByEmail, getAllUsersById, getOwnUser } from '../users/userSelectors'; import { isStreamOrTopicNarrow, emailsOfGroupPmNarrow, @@ -149,13 +149,16 @@ export const isNarrowValid: Selector = createSelector( (state, narrow) => narrow, state => getStreams(state), state => getAllUsersByEmail(state), - (narrow, streams, allUsersByEmail) => + state => getAllUsersById(state), + (narrow, streams, allUsersByEmail, allUsersById) => caseNarrowDefault( narrow, { stream: streamName => streams.find(s => s.name === streamName) !== undefined, topic: streamName => streams.find(s => s.name === streamName) !== undefined, - pm: emails => emails.every(email => allUsersByEmail.get(email) !== undefined), + pm: (emails, ids) => + emails.every(email => allUsersByEmail.get(email) !== undefined) + && ids.every(id => allUsersById.get(id) !== undefined), }, () => true, ), From d4e7b97f4903da23f1733abbbfd39950513b0869 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 23:21:44 -0800 Subject: [PATCH 03/11] outbox [nfc]: Use IDs, not emails, to compute display_recipient. This could conceivably affect behavior if we reached the point of having composed, and attempting to send, a PM where we don't actually have data for one of the recipients by email, but do by user ID, or vice versa. That shouldn't be possible, though, because the UI to show the conversation in the first place would have already errored out by that point. --- src/outbox/outboxActions.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 1a1d06f5617..2c68722eaa2 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -12,7 +12,7 @@ import { } from '../actionConstants'; import { getAuth } from '../selectors'; import * as api from '../api'; -import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors'; +import { getAllUsersById, getOwnUser } from '../users/userSelectors'; import { getUsersAndWildcards } from '../users/userHelpers'; import { caseNarrowPartial } from '../utils/narrow'; import { BackoffMachine } from '../utils/async'; @@ -98,13 +98,13 @@ export const sendOutbox = () => async (dispatch: Dispatch, getState: GetState) = }; // A valid display_recipient with all the thread's users, sorted by ID. -const mapEmailsToUsers = (emails, allUsersByEmail, ownUser) => { - const result = emails.map(email => { - const user = allUsersByEmail.get(email); +const recipientsFromIds = (ids, allUsersById, ownUser) => { + const result = ids.map(id => { + const user = allUsersById.get(id); if (!user) { throw new Error('outbox: missing user when preparing to send PM'); } - return { email, id: user.user_id, full_name: user.full_name }; + return { id, email: user.email, full_name: user.full_name }; }); if (!result.some(r => r.id === ownUser.user_id)) { result.push({ email: ownUser.email, id: ownUser.user_id, full_name: ownUser.full_name }); @@ -120,13 +120,13 @@ type DataFromNarrow = SubsetProperties< const extractTypeToAndSubjectFromNarrow = ( narrow: Narrow, - allUsersByEmail: Map, + allUsersById: Map, ownUser: UserOrBot, ): DataFromNarrow => caseNarrowPartial(narrow, { - pm: emails => ({ + pm: (emails, ids) => ({ type: 'private', - display_recipient: mapEmailsToUsers(emails, allUsersByEmail, ownUser), + display_recipient: recipientsFromIds(ids, allUsersById, ownUser), subject: '', }), @@ -167,7 +167,7 @@ export const addToOutbox = (narrow: Narrow, content: string) => async ( dispatch( messageSendStart({ isSent: false, - ...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersByEmail(state), ownUser), + ...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersById(state), ownUser), markdownContent: content, content: getContentPreview(content, state), timestamp: localTime, From 0b58b91411c980488ea6cecd9fcfd3dfe330ef2c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 23:21:44 -0800 Subject: [PATCH 04/11] compose [nfc]: Use IDs, not emails, to compute placeholder. This could conceivably affect behavior if we're showing a 1:1 conversation with a user we lack data for by email, but have it by user ID, or vice versa. We'd switch from showing the generic "Type a message" to a specific "Message Greg Price" or back. That shouldn't be possible, though, because the UI to show the conversation in the first place would have already errored out by that point. --- src/compose/ComposeBox.js | 17 ++++++++--------- .../getComposeInputPlaceholder-test.js | 14 +++++++------- src/compose/getComposeInputPlaceholder.js | 14 +++++++------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 20f261dbaf6..8ed39ae54a6 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -44,7 +44,6 @@ import { getIsAdmin, getSession, getLastMessageTopic, - getActiveUsersByEmail, getCaughtUpForNarrow, getStreamInNarrow, getVideoChatProvider, @@ -56,12 +55,12 @@ import { import { getDraftForNarrow } from '../drafts/draftsSelectors'; import TopicAutocomplete from '../autocomplete/TopicAutocomplete'; import AutocompleteView from '../autocomplete/AutocompleteView'; -import { getOwnEmail } from '../users/userSelectors'; +import { getActiveUsersById, getOwnUserId } from '../users/userSelectors'; type SelectorProps = {| auth: Auth, - ownEmail: string, - usersByEmail: Map, + ownUserId: number, + usersById: Map, safeAreaInsets: Dimensions, isAdmin: boolean, isAnnouncementOnly: boolean, @@ -420,9 +419,9 @@ class ComposeBox extends PureComponent { render() { const { isTopicFocused, isMenuExpanded, height, message, topic, selection } = this.state; const { - ownEmail, + ownUserId, narrow, - usersByEmail, + usersById, editMessage, safeAreaInsets, isAdmin, @@ -441,7 +440,7 @@ class ComposeBox extends PureComponent { return ; } - const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); const style = { paddingBottom: safeAreaInsets.bottom, backgroundColor: 'hsla(0, 0%, 50%, 0.1)', @@ -519,8 +518,8 @@ class ComposeBox extends PureComponent { export default connect((state, props) => ({ auth: getAuth(state), - ownEmail: getOwnEmail(state), - usersByEmail: getActiveUsersByEmail(state), + ownUserId: getOwnUserId(state), + usersById: getActiveUsersById(state), safeAreaInsets: getSession(state).safeAreaInsets, isAdmin: getIsAdmin(state), isAnnouncementOnly: getIsActiveStreamAnnouncementOnly(state, props.narrow), diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index eb056545123..a661fa377f6 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -11,12 +11,12 @@ import { import * as eg from '../../__tests__/lib/exampleData'; describe('getComposeInputPlaceholder', () => { - const usersByEmail = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.email, u])); - const ownEmail = eg.selfUser.email; + const usersById = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.user_id, u])); + const ownUserId = eg.selfUser.user_id; test('returns "Message @ThisPerson" object for person narrow', () => { const narrow = deepFreeze(pm1to1NarrowFromUser(eg.otherUser)); - const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: `@${eg.otherUser.full_name}` }, @@ -25,25 +25,25 @@ describe('getComposeInputPlaceholder', () => { test('returns "Jot down something" object for self narrow', () => { const narrow = deepFreeze(pm1to1NarrowFromUser(eg.selfUser)); - const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Jot down something' }); }); test('returns "Message #streamName" for stream narrow', () => { const narrow = deepFreeze(streamNarrow('Denmark')); - const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); 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, ownEmail, usersByEmail); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Reply' }); }); test('returns "Message group" object for group narrow', () => { const narrow = deepFreeze(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); - const placeholder = getComposeInputPlaceholder(narrow, ownEmail, usersByEmail); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); expect(placeholder).toEqual({ text: 'Message group' }); }); }); diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index ff7a5563633..e7e15bcbde5 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -4,23 +4,23 @@ import { caseNarrowDefault } from '../utils/narrow'; export default ( narrow: Narrow, - ownEmail: string, - usersByEmail: Map, + ownUserId: number, + usersById: Map, ): LocalizableText => caseNarrowDefault( narrow, { - pm: emails => { - if (emails.length > 1) { + pm: (emails, ids) => { + if (ids.length > 1) { return { text: 'Message group' }; } - const email = emails[0]; + const userId = ids[0]; - if (email === ownEmail) { + if (userId === ownUserId) { return { text: 'Jot down something' }; } - const user = usersByEmail.get(email); + const user = usersById.get(userId); if (!user) { return { text: 'Type a message' }; } From 8cef87f06638ee123542e2240ee0070f0bd1f8c6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 12 Dec 2020 01:51:29 -0800 Subject: [PATCH 05/11] user [nfc]: Drop a disused ...ByEmail selector. We dropped the last use of this selector in the previous commit. --- src/users/__tests__/userSelectors-test.js | 17 ----------------- src/users/userSelectors.js | 14 +------------- 2 files changed, 1 insertion(+), 30 deletions(-) diff --git a/src/users/__tests__/userSelectors-test.js b/src/users/__tests__/userSelectors-test.js index 535c83e5444..a8d237166b4 100644 --- a/src/users/__tests__/userSelectors-test.js +++ b/src/users/__tests__/userSelectors-test.js @@ -1,6 +1,5 @@ /* @flow strict-local */ import { - getActiveUsersByEmail, getAllUsersByEmail, getAllUsersById, getUsersById, @@ -11,22 +10,6 @@ import { } from '../userSelectors'; import * as eg from '../../__tests__/lib/exampleData'; -describe('getActiveUsers', () => { - test('return users, bots, map by email and do not include inactive users', () => { - const state = eg.reduxState({ - users: [eg.selfUser], - realm: { - ...eg.baseReduxState.realm, - crossRealmBots: [eg.crossRealmBot], - nonActiveUsers: [eg.otherUser], - }, - }); - expect(getActiveUsersByEmail(state)).toEqual( - new Map([[eg.selfUser.email, eg.selfUser], [eg.crossRealmBot.email, eg.crossRealmBot]]), - ); - }); -}); - describe('getAllUsersByEmail', () => { test('return users mapped by their email', () => { const users = [eg.makeUser(), eg.makeUser(), eg.makeUser()]; diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index fbe1d622bc6..35a001ad06b 100644 --- a/src/users/userSelectors.js +++ b/src/users/userSelectors.js @@ -21,7 +21,7 @@ import { getUsers, getCrossRealmBots, getNonActiveUsers } from '../directSelecto * user to send a PM to -- deactivated users should be left out. * * See: - * * `getActiveUsersByEmail` for leaving out deactivated users + * * `getActiveUsersById` for leaving out deactivated users * * `User` for details on properties, and links to docs. */ const getAllUsers: Selector = createSelector( @@ -160,18 +160,6 @@ export const getUsersSansMe: Selector = createSelector( (users, ownEmail) => users.filter(user => user.email !== ownEmail), ); -/** - * Excludes deactivated users. See `getAllUsers` for discussion. - * - * Prefer `getActiveUsersById`; see #3764. - */ -export const getActiveUsersByEmail: Selector> = createSelector( - getUsers, - getCrossRealmBots, - (users = [], crossRealmBots = []) => - new Map([...users, ...crossRealmBots].map(user => [user.email, user])), -); - /** Excludes deactivated users. See `getAllUsers` for discussion. */ export const getActiveUsersById: Selector> = createSelector( getUsers, From a190063aefedd8d2dbf703045aa5255303b0ead3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 18 Dec 2020 21:04:34 -0800 Subject: [PATCH 06/11] avatar [nfc]: Drop the unused "shape" prop. This isn't used, and makes the code more complex. We always effectively use the default of "rounded", never the alternative of "square"; so just hard-wire the "rounded" behavior. Back in 805d35d6f, fixing #3398, we took out the old "circle" option here, which produced quite bad results when used. The "square" option could be OK in the right design context; but we don't actually use it, and haven't since 57f45f439 a couple of years ago. If we want that functionality in the future, we can add it back in the relevant places then. --- src/common/GroupAvatar.js | 10 ++-------- src/common/UserAvatar.js | 10 ++-------- src/common/UserAvatarWithPresence.js | 10 ++-------- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/common/GroupAvatar.js b/src/common/GroupAvatar.js index 29193e3d7cf..4f5f2909d62 100644 --- a/src/common/GroupAvatar.js +++ b/src/common/GroupAvatar.js @@ -20,7 +20,6 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| names: string[], size: number, - shape: 'rounded' | 'square', children?: React$Node, onPress?: () => void, |}>; @@ -43,22 +42,17 @@ export const initialsForGroupIcon = (names: string[]): string => { * * @prop names - The name of one or more users, used to extract their initials. * @prop size - Sets width and height in logical pixels. - * @prop [shape] * @prop children - If provided, will render inside the component body. * @prop onPress - Event fired on pressing the component. */ export default class GroupAvatar extends PureComponent { - static defaultProps = { - shape: 'rounded', - }; - render() { - const { children, names, size, shape, onPress } = this.props; + const { children, names, size, onPress } = this.props; const frameSize = { height: size, width: size, - borderRadius: shape === 'rounded' ? size / 8 : 0, + borderRadius: size / 8, backgroundColor: colorHashFromString(names.join(', ')), }; const textSize = { diff --git a/src/common/UserAvatar.js b/src/common/UserAvatar.js index 00e4fa73b9d..1c81ab9074f 100644 --- a/src/common/UserAvatar.js +++ b/src/common/UserAvatar.js @@ -8,7 +8,6 @@ import { AvatarURL } from '../utils/avatar'; type Props = $ReadOnly<{| avatarUrl: AvatarURL, size: number, - shape: 'rounded' | 'square', children?: React$Node, onPress?: () => void, |}>; @@ -18,18 +17,13 @@ type Props = $ReadOnly<{| * * @prop avatarUrl * @prop size - Sets width and height in logical pixels. - * @prop [shape] - 'rounded' (default) means a square with rounded corners. * @prop [children] - If provided, will render inside the component body. * @prop [onPress] - Event fired on pressing the component. */ export default class UserAvatar extends PureComponent { - static defaultProps = { - shape: 'rounded', - }; - render() { - const { avatarUrl, children, size, shape, onPress } = this.props; - const borderRadius = shape === 'rounded' ? size / 8 : 0; + const { avatarUrl, children, size, onPress } = this.props; + const borderRadius = size / 8; const style = { height: size, width: size, diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 62736946cba..a74d7273373 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -19,7 +19,6 @@ type Props = $ReadOnly<{| avatarUrl: AvatarURL, email: string, size: number, - shape: 'rounded' | 'square', onPress?: () => void, |}>; @@ -29,19 +28,14 @@ type Props = $ReadOnly<{| * @prop [avatarUrl] * @prop [email] - Sender's / user's email address, for the presence dot. * @prop [size] - Sets width and height in logical pixels. - * @prop [shape] - 'rounded' (default) means a square with rounded corners. * @prop [onPress] - Event fired on pressing the component. */ export default class UserAvatarWithPresence extends PureComponent { - static defaultProps = { - shape: 'rounded', - }; - render() { - const { avatarUrl, email, size, onPress, shape } = this.props; + const { avatarUrl, email, size, onPress } = this.props; return ( - + ); From fbb7153d8e37f5f42116f07afea4913f0bc64d62 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 23:35:15 -0800 Subject: [PATCH 07/11] user: Add "...ById" variant for UserAvatarWithPresence. We'll temporarily have two variants of this component -- an implementation in common but with slightly different interfaces -- as a way to smoothly migrate from callers using one interface to the other. The new interface keeps callers from having to know or care about emails, which we're moving away from. It also encapsulates the need to find the user's avatar URL; the caller only has to be concerned with identifying which user it wants represented, by user ID. This improves react-redux hygiene: it means that when that user's avatar changes, we no longer have to rerender the calling component, which fundamentally shouldn't care about the specific avatar URL. --- src/common/UserAvatarWithPresence.js | 50 +++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index a74d7273373..d35b534f085 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -1,11 +1,13 @@ /* @flow strict-local */ - -import React, { PureComponent } from 'react'; +import React, { type ComponentType, PureComponent } from 'react'; import { createStyleSheet } from '../styles'; +import type { Dispatch } from '../types'; import UserAvatar from './UserAvatar'; import PresenceStatusIndicator from './PresenceStatusIndicator'; import { AvatarURL } from '../utils/avatar'; +import { getUserForId } from '../users/userSelectors'; +import { connect } from '../react-redux'; const styles = createStyleSheet({ status: { @@ -17,20 +19,28 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| avatarUrl: AvatarURL, - email: string, size: number, onPress?: () => void, + email: string, |}>; /** - * Renders a user avatar with a PresenceStatusIndicator in the corner + * A user avatar with a PresenceStatusIndicator in the corner. + * + * Prefer `UserAvatarWithPresenceById` over this component: it does the same + * thing but avoids an email in the component's interface. Once all callers + * have migrated to that version, it'll replace this one. * * @prop [avatarUrl] * @prop [email] - Sender's / user's email address, for the presence dot. * @prop [size] - Sets width and height in logical pixels. * @prop [onPress] - Event fired on pressing the component. */ -export default class UserAvatarWithPresence extends PureComponent { +// The underlying class gets an inexact Props type to express that it's fine +// for it to be passed extra props by the implementation of …ById. +// We don't export it with that type, because we want exact Props types to +// get the most effective type-checking. +class UserAvatarWithPresence extends PureComponent<$ReadOnly<{ ...Props, ... }>> { render() { const { avatarUrl, email, size, onPress } = this.props; @@ -41,3 +51,33 @@ export default class UserAvatarWithPresence extends PureComponent { ); } } + +// Export the class with a tighter constraint on acceptable props, namely +// that the type is an exact object type as usual. +export default (UserAvatarWithPresence: ComponentType); + +/** + * A user avatar with a PresenceStatusIndicator in the corner. + * + * Use this in preference to the default export `UserAvatarWithPresence`. + * We're migrating from that one to this in order to avoid using emails. + * + * @prop [userId] + * @prop [size] + * @prop [onPress] + */ +export const UserAvatarWithPresenceById = connect<{| avatarUrl: AvatarURL, email: string |}, _, _>( + (state, props) => { + const user = getUserForId(state, props.userId); + return { avatarUrl: user.avatar_url, email: user.email }; + }, +)( + // The type inference embedded in our `connect` type relies on seeing the + // exact Props type for the underlying component, complete with all the + // expected props. Normally that's just how our Props types are in the + // first place, but here we have to provide it explicitly. + /* eslint-disable flowtype/generic-spacing */ + (UserAvatarWithPresence: ComponentType< + $ReadOnly<{| ...Props, dispatch: Dispatch, userId: number |}>, + >), +); From fe45ce2b5f5ee064ec29c7fa65972095f05353be Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 23:14:47 -0800 Subject: [PATCH 08/11] title [nfc]: Use UserAvatarByPresenceById to replace some email use. --- src/title/Title.js | 6 +++--- src/title/TitleGroup.js | 7 +++---- src/title/TitlePrivate.js | 7 +++---- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/title/Title.js b/src/title/Title.js index c25d81084b9..32e8e05c1e5 100644 --- a/src/title/Title.js +++ b/src/title/Title.js @@ -30,9 +30,9 @@ export default class Title extends PureComponent { allPrivate: () => , stream: () => , topic: () => , - pm: emails => - emails.length === 1 ? ( - + pm: (emails, ids) => + ids.length === 1 ? ( + ) : ( ), diff --git a/src/title/TitleGroup.js b/src/title/TitleGroup.js index f88b0ac0cfc..8ea1c26303b 100644 --- a/src/title/TitleGroup.js +++ b/src/title/TitleGroup.js @@ -7,7 +7,7 @@ import NavigationService from '../nav/NavigationService'; import type { Dispatch, UserOrBot, Narrow } from '../types'; import styles, { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; -import { UserAvatarWithPresence } from '../common'; +import { UserAvatarWithPresenceById } from '../common/UserAvatarWithPresence'; import { getRecipientsInGroupPmNarrow } from '../selectors'; import { navigateToAccountDetails } from '../nav/navActions'; @@ -40,11 +40,10 @@ class TitleGroup extends PureComponent { {recipients.map((user, index) => ( - this.handlePress(user)} size={32} - avatarUrl={user.avatar_url} - email={user.email} + userId={user.user_id} /> ))} diff --git a/src/title/TitlePrivate.js b/src/title/TitlePrivate.js index dc9eb4bc738..92b55d654f6 100644 --- a/src/title/TitlePrivate.js +++ b/src/title/TitlePrivate.js @@ -9,7 +9,7 @@ import styles, { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; import { Touchable, UserAvatarWithPresence, ViewPlaceholder } from '../common'; import ActivityText from './ActivityText'; -import { getAllUsersByEmail } from '../users/userSelectors'; +import { getAllUsersById } from '../users/userSelectors'; import { navigateToAccountDetails } from '../nav/navActions'; import * as logging from '../utils/logging'; @@ -18,7 +18,7 @@ type SelectorProps = $ReadOnly<{| |}>; type Props = $ReadOnly<{ - email: string, + userId: number, color: string, dispatch: Dispatch, @@ -70,6 +70,5 @@ class TitlePrivate extends PureComponent { } export default connect((state, props) => ({ - // TODO: use user_id, not email (https://github.com/zulip/zulip-mobile/issues/3764) - user: getAllUsersByEmail(state).get(props.email), + user: getAllUsersById(state).get(props.userId), }))(TitlePrivate); From 7cae3abd176cdca399464e351d430afb342b0a26 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 23:38:19 -0800 Subject: [PATCH 09/11] title [nfc]: Drop some more email-use in TitleGroup. --- src/title/TitleGroup.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/title/TitleGroup.js b/src/title/TitleGroup.js index 8ea1c26303b..d67065db7d7 100644 --- a/src/title/TitleGroup.js +++ b/src/title/TitleGroup.js @@ -23,8 +23,8 @@ type Props = $ReadOnly<{| |}>; class TitleGroup extends PureComponent { - handlePress = (user: UserOrBot) => { - NavigationService.dispatch(navigateToAccountDetails(user.user_id)); + handlePress = (userId: number) => { + NavigationService.dispatch(navigateToAccountDetails(userId)); }; styles = createStyleSheet({ @@ -38,10 +38,10 @@ class TitleGroup extends PureComponent { return ( - {recipients.map((user, index) => ( - + {recipients.map(user => ( + this.handlePress(user)} + onPress={() => this.handlePress(user.user_id)} size={32} userId={user.user_id} /> From 2b35d3b97129024fcd551aac68ddb67d34458f15 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 23:41:29 -0800 Subject: [PATCH 10/11] title [nfc]: Pass purely user IDs to TitleGroup. Now this component doesn't need to know or care about the state we have for these users in Redux. After all, it was never really using any of those details -- it's the child components, showing their avatars and presence dots, that actually care. --- src/title/Title.js | 2 +- src/title/TitleGroup.js | 29 +++++++---------------------- 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/title/Title.js b/src/title/Title.js index 32e8e05c1e5..f1c9ca07218 100644 --- a/src/title/Title.js +++ b/src/title/Title.js @@ -34,7 +34,7 @@ export default class Title extends PureComponent { ids.length === 1 ? ( ) : ( - + ), search: () => null, }); diff --git a/src/title/TitleGroup.js b/src/title/TitleGroup.js index d67065db7d7..01486f89c3d 100644 --- a/src/title/TitleGroup.js +++ b/src/title/TitleGroup.js @@ -4,25 +4,15 @@ import React, { PureComponent } from 'react'; import { View } from 'react-native'; import NavigationService from '../nav/NavigationService'; -import type { Dispatch, UserOrBot, Narrow } from '../types'; import styles, { createStyleSheet } from '../styles'; -import { connect } from '../react-redux'; import { UserAvatarWithPresenceById } from '../common/UserAvatarWithPresence'; -import { getRecipientsInGroupPmNarrow } from '../selectors'; import { navigateToAccountDetails } from '../nav/navActions'; -type SelectorProps = $ReadOnly<{| - recipients: UserOrBot[], -|}>; - type Props = $ReadOnly<{| - narrow: Narrow, - - dispatch: Dispatch, - ...SelectorProps, + userIds: $ReadOnlyArray, |}>; -class TitleGroup extends PureComponent { +export default class TitleGroup extends PureComponent { handlePress = (userId: number) => { NavigationService.dispatch(navigateToAccountDetails(userId)); }; @@ -34,16 +24,15 @@ class TitleGroup extends PureComponent { }); render() { - const { recipients } = this.props; - + const { userIds } = this.props; return ( - {recipients.map(user => ( - + {userIds.map(userId => ( + this.handlePress(user.user_id)} + onPress={() => this.handlePress(userId)} size={32} - userId={user.user_id} + userId={userId} /> ))} @@ -51,7 +40,3 @@ class TitleGroup extends PureComponent { ); } } - -export default connect((state, props) => ({ - recipients: getRecipientsInGroupPmNarrow(state, props.narrow), -}))(TitleGroup); From 4116f83afc1fcd6563f884ab4b122984a5955113 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 29 Dec 2020 13:13:32 -0800 Subject: [PATCH 11/11] user [nfc]: Use UserAvatarWithPresenceById more, where easy. This removes a few more references to emails. --- src/title/TitlePrivate.js | 5 +++-- src/users/UserItem.js | 10 +++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/title/TitlePrivate.js b/src/title/TitlePrivate.js index 92b55d654f6..69502a80cbe 100644 --- a/src/title/TitlePrivate.js +++ b/src/title/TitlePrivate.js @@ -7,7 +7,8 @@ import NavigationService from '../nav/NavigationService'; import type { Dispatch, UserOrBot } from '../types'; import styles, { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; -import { Touchable, UserAvatarWithPresence, ViewPlaceholder } from '../common'; +import { Touchable, ViewPlaceholder } from '../common'; +import { UserAvatarWithPresenceById } from '../common/UserAvatarWithPresence'; import ActivityText from './ActivityText'; import { getAllUsersById } from '../users/userSelectors'; import { navigateToAccountDetails } from '../nav/navActions'; @@ -55,7 +56,7 @@ class TitlePrivate extends PureComponent { return ( - + diff --git a/src/users/UserItem.js b/src/users/UserItem.js index 8f8ef6b6ea3..dcedc241e8f 100644 --- a/src/users/UserItem.js +++ b/src/users/UserItem.js @@ -3,7 +3,8 @@ import React, { PureComponent } from 'react'; import { View } from 'react-native'; import type { UserOrBot } from '../types'; -import { UserAvatarWithPresence, RawLabel, Touchable, UnreadCount } from '../common'; +import { RawLabel, Touchable, UnreadCount } from '../common'; +import { UserAvatarWithPresenceById } from '../common/UserAvatarWithPresence'; import styles, { createStyleSheet, BRAND_COLOR } from '../styles'; const componentStyles = createStyleSheet({ @@ -54,12 +55,7 @@ export default class UserItem extends PureComponent { return ( - +