From d4dc9c27b8eade68fa787dc7e5e7beba93fdf812 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 16 Dec 2020 09:56:27 -0800 Subject: [PATCH 01/24] typing-status tests: Make test data less ad-hoc. That is, construct it more systematically, and get data on test users from the exampleData code. --- src/actionTypes.js | 2 +- src/typing/__tests__/typingReducer-test.js | 130 +++++++++------------ 2 files changed, 53 insertions(+), 79 deletions(-) diff --git a/src/actionTypes.js b/src/actionTypes.js index d91129d3266..24b935ce8f5 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -374,7 +374,7 @@ type EventPresenceAction = {| type EventTypingCommon = {| ...ServerEvent, ownUserId: number, - recipients: Array<{ + recipients: $ReadOnlyArray<{ user_id: number, email: string, }>, diff --git a/src/typing/__tests__/typingReducer-test.js b/src/typing/__tests__/typingReducer-test.js index 1481c5569ba..49dc9212a32 100644 --- a/src/typing/__tests__/typingReducer-test.js +++ b/src/typing/__tests__/typingReducer-test.js @@ -2,30 +2,42 @@ import deepFreeze from 'deep-freeze'; +import type { Action, User } from '../../types'; import { EVENT_TYPING_START, EVENT_TYPING_STOP } from '../../actionConstants'; import typingReducer from '../typingReducer'; import { NULL_OBJECT } from '../../nullObjects'; +import * as eg from '../../__tests__/lib/exampleData'; describe('typingReducer', () => { + const user1 = { ...eg.otherUser, user_id: 1 }; + const user2 = { ...eg.thirdUser, user_id: 2 }; + + const egTypingAction = (args: {| + op: 'start' | 'stop', + sender: User, + recipients: $ReadOnlyArray, + time: number, + |}): Action => { + const { op, sender, recipients, time } = args; + const base = { id: 123, ownUserId: eg.selfUser.user_id, sender, recipients, time }; + return op === 'start' + ? { ...base, op: 'start', type: EVENT_TYPING_START } + : { ...base, op: 'stop', type: EVENT_TYPING_STOP }; + }; + describe('EVENT_TYPING_START', () => { test('adds sender as currently typing user', () => { const initialState = NULL_OBJECT; - const action = deepFreeze({ - type: EVENT_TYPING_START, + const action = egTypingAction({ op: 'start', - sender: { email: 'john@example.com', user_id: 1 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'me@example.com', user_id: 2 }, - ], - ownUserId: 2, + sender: user1, + recipients: [user1, eg.selfUser], time: 123456789, - id: 123, }); const expectedState = { - '1': { time: 123456789, userIds: [1] }, + '1': { time: 123456789, userIds: [user1.user_id] }, }; const newState = typingReducer(initialState, action); @@ -35,24 +47,18 @@ describe('typingReducer', () => { test('if user is already typing, no change in userIds but update time', () => { const initialState = deepFreeze({ - '1': { time: 123456789, userIds: [1] }, + '1': { time: 123456789, userIds: [user1.user_id] }, }); - const action = deepFreeze({ - type: EVENT_TYPING_START, + const action = egTypingAction({ op: 'start', - sender: { email: 'john@example.com', user_id: 1 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'me@example.com', user_id: 2 }, - ], - ownUserId: 2, + sender: user1, + recipients: [user1, eg.selfUser], time: 123456889, - id: 123, }); const expectedState = { - '1': { time: 123456889, userIds: [1] }, + '1': { time: 123456889, userIds: [user1.user_id] }, }; const newState = typingReducer(initialState, action); @@ -62,26 +68,19 @@ describe('typingReducer', () => { test('if other people are typing in other narrows, add, do not affect them', () => { const initialState = deepFreeze({ - '1': { time: 123489, userIds: [1] }, + '1': { time: 123489, userIds: [user1.user_id] }, }); - const action = deepFreeze({ - type: EVENT_TYPING_START, + const action = egTypingAction({ op: 'start', - sender: { email: 'mark@example.com', user_id: 2 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'mark@example.com', user_id: 2 }, - { email: 'me@example.com', user_id: 3 }, - ], - ownUserId: 3, + sender: user2, + recipients: [user1, user2, eg.selfUser], time: 123456789, - id: 123, }); const expectedState = { - '1': { time: 123489, userIds: [1] }, - '1,2': { time: 123456789, userIds: [2] }, + '1': { time: 123489, userIds: [user1.user_id] }, + '1,2': { time: 123456789, userIds: [user2.user_id] }, }; const newState = typingReducer(initialState, action); @@ -91,25 +90,18 @@ describe('typingReducer', () => { test('if another user is typing already, append new one', () => { const initialState = deepFreeze({ - '1,2': { time: 123489, userIds: [1] }, + '1,2': { time: 123489, userIds: [user1.user_id] }, }); - const action = deepFreeze({ - type: EVENT_TYPING_START, + const action = egTypingAction({ op: 'start', - sender: { email: 'mark@example.com', user_id: 2 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'mark@example.com', user_id: 2 }, - { email: 'me@example.com', user_id: 3 }, - ], - ownUserId: 3, + sender: user2, + recipients: [user1, user2, eg.selfUser], time: 123456789, - id: 123, }); const expectedState = { - '1,2': { time: 123456789, userIds: [1, 2] }, + '1,2': { time: 123456789, userIds: [user1.user_id, user2.user_id] }, }; const newState = typingReducer(initialState, action); @@ -121,25 +113,19 @@ describe('typingReducer', () => { describe('EVENT_TYPING_STOP', () => { test('if after removing, key is an empty list, key is removed', () => { const initialState = deepFreeze({ - '1': { time: 123489, userIds: [1] }, - '3': { time: 123489, userIds: [2] }, + '1': { time: 123489, userIds: [user1.user_id] }, + '3': { time: 123489, userIds: [eg.selfUser.user_id] }, }); - const action = deepFreeze({ - type: EVENT_TYPING_STOP, + const action = egTypingAction({ op: 'stop', - sender: { email: 'john@example.com', user_id: 1 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'me@example.com', user_id: 2 }, - ], - ownUserId: 2, + sender: user1, + recipients: [user1, eg.selfUser], time: 123456789, - id: 123, }); const expectedState = { - '3': { time: 123489, userIds: [2] }, + '3': { time: 123489, userIds: [eg.selfUser.user_id] }, }; const newState = typingReducer(initialState, action); @@ -149,24 +135,18 @@ describe('typingReducer', () => { test('if two people are typing, just one is removed', () => { const initialState = deepFreeze({ - '1': { time: 123489, userIds: [1, 2] }, + '1': { time: 123489, userIds: [user1.user_id, eg.selfUser.user_id] }, }); - const action = deepFreeze({ - type: EVENT_TYPING_STOP, + const action = egTypingAction({ op: 'stop', - sender: { email: 'john@example.com', user_id: 1 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'me@example.com', user_id: 2 }, - ], - ownUserId: 2, + sender: user1, + recipients: [user1, eg.selfUser], time: 123456789, - id: 123, }); const expectedState = { - '1': { time: 123456789, userIds: [2] }, + '1': { time: 123456789, userIds: [eg.selfUser.user_id] }, }; const newState = typingReducer(initialState, action); @@ -177,17 +157,11 @@ describe('typingReducer', () => { test('if typing state does not exist, no change is made', () => { const initialState = NULL_OBJECT; - const action = deepFreeze({ - type: EVENT_TYPING_STOP, + const action = egTypingAction({ op: 'stop', - sender: { email: 'john@example.com', user_id: 1 }, - recipients: [ - { email: 'john@example.com', user_id: 1 }, - { email: 'me@example.com', user_id: 2 }, - ], - ownUserId: 2, + sender: user1, + recipients: [user1, eg.selfUser], time: 123456789, - id: 123, }); const expectedState = {}; From d11911aab5275e0411f827222c6ff02b2ebdb984 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 20:43:52 -0800 Subject: [PATCH 02/24] recipient [nfc]: Mark an array parameter read-only. --- src/utils/recipient.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/recipient.js b/src/utils/recipient.js index bacd7dfe808..31eed997b70 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -200,7 +200,7 @@ export const pmKeyRecipientsFromMessage = ( * Returns null when a user couldn't be found in the given `allUsersById`. */ export const pmKeyRecipientsFromIds = ( - userIds: number[], + userIds: $ReadOnlyArray, allUsersById: Map, ownUserId: number, ): PmKeyUsers | null => { From a171a051f66e7b871317545d9fab869c8dcb232f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 19 Jan 2021 23:33:27 -0800 Subject: [PATCH 03/24] api: Cut unused, unimplemented user-groups bindings. Many of these can't possibly work in the first place -- how can we edit a user group, for example, without providing any data on what we want to change about it? In any case, we don't use them, and have no plans to in the short or medium term. When we do, it's not hard to add bindings, and really that will only be made easier by not having broken ones lying around. --- src/api/index.js | 12 ------------ src/api/user_groups/createUserGroup.js | 5 ----- src/api/user_groups/deleteUserGroup.js | 6 ------ src/api/user_groups/editUserGroup.js | 6 ------ src/api/user_groups/editUserGroupMembers.js | 6 ------ src/api/user_groups/getUserGroupById.js | 5 ----- src/api/user_groups/getUserGroups.js | 5 ----- 7 files changed, 45 deletions(-) delete mode 100644 src/api/user_groups/createUserGroup.js delete mode 100644 src/api/user_groups/deleteUserGroup.js delete mode 100644 src/api/user_groups/editUserGroup.js delete mode 100644 src/api/user_groups/editUserGroupMembers.js delete mode 100644 src/api/user_groups/getUserGroupById.js delete mode 100644 src/api/user_groups/getUserGroups.js diff --git a/src/api/index.js b/src/api/index.js index a65263be063..cf951c16ea4 100644 --- a/src/api/index.js +++ b/src/api/index.js @@ -45,12 +45,6 @@ import toggleStreamNotifications from './subscriptions/toggleStreamNotifications import getSubscriptionToStream from './subscriptions/getSubscriptionToStream'; import unmuteTopic from './subscriptions/unmuteTopic'; import tryGetFileTemporaryUrl from './tryGetFileTemporaryUrl'; -import createUserGroup from './user_groups/createUserGroup'; -import deleteUserGroup from './user_groups/deleteUserGroup'; -import editUserGroup from './user_groups/editUserGroup'; -import editUserGroupMembers from './user_groups/editUserGroupMembers'; -import getUserGroupById from './user_groups/getUserGroupById'; -import getUserGroups from './user_groups/getUserGroups'; import getUsers from './users/getUsers'; import createUser from './users/createUser'; import getUserProfile from './users/getUserProfile'; @@ -104,12 +98,6 @@ export { toggleStreamNotifications, unmuteTopic, tryGetFileTemporaryUrl, - createUserGroup, - deleteUserGroup, - editUserGroup, - editUserGroupMembers, - getUserGroupById, - getUserGroups, getUsers, createUser, getUserProfile, diff --git a/src/api/user_groups/createUserGroup.js b/src/api/user_groups/createUserGroup.js deleted file mode 100644 index 3d62f9c7821..00000000000 --- a/src/api/user_groups/createUserGroup.js +++ /dev/null @@ -1,5 +0,0 @@ -/* @flow strict-local */ -import type { ApiResponse, Auth } from '../transportTypes'; -import { apiPost } from '../apiFetch'; - -export default async (auth: Auth): Promise => apiPost(auth, 'user_groups/create'); diff --git a/src/api/user_groups/deleteUserGroup.js b/src/api/user_groups/deleteUserGroup.js deleted file mode 100644 index e1d549dca8f..00000000000 --- a/src/api/user_groups/deleteUserGroup.js +++ /dev/null @@ -1,6 +0,0 @@ -/* @flow strict-local */ -import type { ApiResponse, Auth } from '../transportTypes'; -import { apiDelete } from '../apiFetch'; - -export default async (auth: Auth, id: number): Promise => - apiDelete(auth, `user_groups/${id}`); diff --git a/src/api/user_groups/editUserGroup.js b/src/api/user_groups/editUserGroup.js deleted file mode 100644 index 2bdf57dc575..00000000000 --- a/src/api/user_groups/editUserGroup.js +++ /dev/null @@ -1,6 +0,0 @@ -/* @flow strict-local */ -import type { ApiResponse, Auth } from '../transportTypes'; -import { apiPatch } from '../apiFetch'; - -export default async (auth: Auth, id: number): Promise => - apiPatch(auth, `user_groups/${id}`); diff --git a/src/api/user_groups/editUserGroupMembers.js b/src/api/user_groups/editUserGroupMembers.js deleted file mode 100644 index df66f3e37c6..00000000000 --- a/src/api/user_groups/editUserGroupMembers.js +++ /dev/null @@ -1,6 +0,0 @@ -/* @flow strict-local */ -import type { ApiResponse, Auth } from '../transportTypes'; -import { apiPost } from '../apiFetch'; - -export default async (auth: Auth, id: number): Promise => - apiPost(auth, `user_groups/${id}/members`); diff --git a/src/api/user_groups/getUserGroupById.js b/src/api/user_groups/getUserGroupById.js deleted file mode 100644 index 7c7271b3ce8..00000000000 --- a/src/api/user_groups/getUserGroupById.js +++ /dev/null @@ -1,5 +0,0 @@ -/* @flow strict-local */ -import type { Auth } from '../transportTypes'; -import { apiGet } from '../apiFetch'; - -export default (auth: Auth, id: number): Promise => apiGet(auth, `realm/user_groups/${id}`); diff --git a/src/api/user_groups/getUserGroups.js b/src/api/user_groups/getUserGroups.js deleted file mode 100644 index 9a3878c777d..00000000000 --- a/src/api/user_groups/getUserGroups.js +++ /dev/null @@ -1,5 +0,0 @@ -/* @flow strict-local */ -import type { Auth } from '../transportTypes'; -import { apiGet } from '../apiFetch'; - -export default (auth: Auth): Promise => apiGet(auth, 'users/me/user_groups'); From d4c57b691507b0b36d54dbda15541371c664eb2c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 15 Dec 2020 12:47:57 -0800 Subject: [PATCH 04/24] api types [nfc]: Move import to above a section heading. The imports belong to the whole file, and this had the import placed inside the first section of it. --- src/api/modelTypes.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 956d703155e..a5ef07b1088 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -4,6 +4,8 @@ * @flow strict-local */ +import type { AvatarURL } from '../utils/avatar'; + // // // @@ -12,8 +14,6 @@ // // -import type { AvatarURL } from '../utils/avatar'; - export type ImageEmojiType = $ReadOnly<{| author?: $ReadOnly<{| email: string, From c4c39caf2133c97682253569f858b42eb71e4701 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 12 Dec 2020 01:02:23 -0800 Subject: [PATCH 05/24] api types: Add UserId, an opaque type alias; don't use it yet. We'll use this in waves over the coming series of commits. Because it's public that `UserId` is a subtype of `number`, other code can freely take a `UserId` and use it as if it were simply a `number`. That means we can do the conversion gradually so long as we start with places where a user ID is made, and follow the data flow to places where it's passed to and ultimately consumed: while we're in the middle of the conversion, the boundary will always look like some code providing a `UserId` and some other code accepting it as a `number`. --- src/api/idTypes.js | 54 +++++++++++++++++++++++++++++++++++++++++++ src/api/modelTypes.js | 2 ++ 2 files changed, 56 insertions(+) create mode 100644 src/api/idTypes.js diff --git a/src/api/idTypes.js b/src/api/idTypes.js new file mode 100644 index 00000000000..421f10b1808 --- /dev/null +++ b/src/api/idTypes.js @@ -0,0 +1,54 @@ +/* @flow strict-local */ + +/** + * A user ID. + * + * This is a number that identifies a particular Zulip user. Different + * users on the same Zulip server will have different user IDs. On the + * other hand, between different Zulip servers the same user ID may be used + * to refer to completely unrelated users. + * + * In general, if something calls for a value of this type, then you should + * be getting it from something that already has this type: like the + * `user_id` property of a `User` object, or a data structure that stores + * user IDs. + * + * The only other way to create a `UserId` is to call the `makeUserId` + * function provided by this module. + * + * See also type `User`, for the thing that one of these identifies. + */ +// How this type works: it's an "opaque type alias" for simply a number. +// See Flow docs: https://flow.org/en/docs/types/opaque-types/ +// +// This means that: +// * At runtime, a `UserId` value is just a number. The use of `UserId` +// involves zero runtime overhead compared with simply `number`. +// * Because we've written a "bound" of `: number`, all code that has one +// of these values can freely use it as if it were simply `number`. +// * On the other hand, the only way to *create* such a value is to invoke +// something from this module to do it for you. +// +// For more background discussion of opaque types, see `PmKeyRecipients`. +export opaque type UserId: number = number; + +/** + * Take a number, and declare that it truly is a user ID. + * + * This does nothing at all at runtime, just returning the value it's + * passed. Its only effect is to inform the type-checker that it's OK to + * use this value where a user ID is required. + * + * In general the only legitimate use case for this function, outside of + * tests, is when parsing a user ID from a string. When getting a user ID + * from any other source, if the values really are user IDs then the type of + * that source should be adjusted to say so. + */ +export const makeUserId = (id: number): UserId => id; + +/* Possible future work: + export opaque type StreamId: number = number; + export opaque type MessageId: number = number; + export const makeStreamId = (id: number): StreamId => id; + export const makeMessageId = (id: number): MessageId => id; +*/ diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index a5ef07b1088..b5d9b202c1a 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -6,6 +6,8 @@ import type { AvatarURL } from '../utils/avatar'; +export type * from './idTypes'; + // // // From f59cd486e1adee2ce1fd0811aed88df6a226990b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 20:40:15 -0800 Subject: [PATCH 06/24] user types: Use the new UserId alias in exampleData. In places where these test helpers accept a user ID, we'll keep accepting plain numbers, to just reduce the number of places where test code has to say `makeUserId`. But where they provide a user ID, start specifying that it's not just any number but a genuine `UserId`. --- src/__tests__/lib/exampleData.js | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 179061be089..b4cbec10110 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -12,7 +12,9 @@ import type { Subscription, User, UserGroup, + UserId, } from '../../api/modelTypes'; +import { makeUserId } from '../../api/idTypes'; import type { Action, GlobalState, MessagesState, RealmState } from '../../reduxTypes'; import type { Auth, Account, Outbox } from '../../types'; import { UploadedAvatarURL } from '../../utils/avatar'; @@ -104,10 +106,12 @@ export const diverseCharacters = type UserOrBotPropertiesArgs = {| name?: string, - user_id?: number, + user_id?: number, // accept a plain number, for convenience in tests |}; -const randUserId: () => number = makeUniqueRandInt('user IDs', 10000); +const randUserId: () => UserId = (mk => () => makeUserId(mk()))( + makeUniqueRandInt('user IDs', 10000), +); const userOrBotProperties = ({ name: _name, user_id }: UserOrBotPropertiesArgs) => { const name = _name ?? randString(); const capsName = name.substring(0, 1).toUpperCase() + name.substring(1); @@ -125,7 +129,7 @@ const userOrBotProperties = ({ name: _name, user_id }: UserOrBotPropertiesArgs) full_name: `${capsName} User`, is_admin: false, timezone: 'UTC', - user_id: user_id ?? randUserId(), + user_id: user_id != null ? makeUserId(user_id) : randUserId(), }); }; @@ -321,11 +325,16 @@ export const pmMessage = (args?: {| ...$Rest, sender?: User, recipients?: User[], + sender_id?: number, // accept a plain number, for convenience in tests |}): Message => { // The `Object.freeze` is to work around a Flow issue: // https://github.com/facebook/flow/issues/2386#issuecomment-695064325 - const { sender = otherUser, recipients = [otherUser, selfUser], ...extra } = - args ?? Object.freeze({}); + const { + sender = otherUser, + recipients = [otherUser, selfUser], + sender_id = undefined, + ...extra + } = args ?? Object.freeze({}); const baseMessage: Message = { ...messagePropertiesBase, @@ -344,7 +353,11 @@ export const pmMessage = (args?: {| type: 'private', }; - return deepFreeze({ ...baseMessage, ...extra }); + return deepFreeze({ + ...baseMessage, + ...(sender_id != null && { sender_id: makeUserId(sender_id) }), + ...extra, + }); }; export const pmMessageFromTo = (from: User, to: User[], extra?: $Rest): Message => @@ -537,7 +550,7 @@ export const action = deepFreeze({ is_admin: false, realm_non_active_users: [], realm_users: [], - user_id: 4, + user_id: makeUserId(4), realm_user_groups: [], recent_private_conversations: [], streams: [], From 62d48a70d6163d6f705b4e32d664238f992ec2f9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 16 Dec 2020 09:30:53 -0800 Subject: [PATCH 07/24] user types: Use makeUserId in test code. Where we make up a user ID in test code, mark it in the type system as being a user ID and not just any old number. We'll soon adjust the types of our many functions that expect data containing a user ID, so that they require this marking. This change prepares for that. --- src/boot/__tests__/replaceRevive-test.js | 3 ++- src/message/__tests__/messagesReducer-test.js | 7 +++--- .../__tests__/pmConversationsModel-test.js | 7 +++--- src/typing/__tests__/typingReducer-test.js | 5 ++-- .../__tests__/unreadHuddlesReducer-test.js | 25 ++++++++++--------- src/unread/__tests__/unreadPmsReducer-test.js | 21 ++++++++-------- src/utils/__tests__/recipient-test.js | 13 +++++----- 7 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/boot/__tests__/replaceRevive-test.js b/src/boot/__tests__/replaceRevive-test.js index 896c72406c7..735c0acf54c 100644 --- a/src/boot/__tests__/replaceRevive-test.js +++ b/src/boot/__tests__/replaceRevive-test.js @@ -8,6 +8,7 @@ import { FallbackAvatarURL, GravatarURL, UploadedAvatarURL } from '../../utils/a import { ZulipVersion } from '../../utils/zulipVersion'; import { stringify, parse, SERIALIZED_TYPE_FIELD_NAME } from '../replaceRevive'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeUserId } from '../../api/idTypes'; const data = { list: Immutable.List([1, 2, 'a', null]), @@ -31,7 +32,7 @@ const data = { }), fallbackAvatarURL: FallbackAvatarURL.validateAndConstructInstance({ realm: eg.realm, - userId: 1, + userId: makeUserId(1), }), plainObjectWithTypeKey: { a: 1, diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js index 2525bb4f49d..3b03a974956 100644 --- a/src/message/__tests__/messagesReducer-test.js +++ b/src/message/__tests__/messagesReducer-test.js @@ -14,6 +14,7 @@ import { } from '../../actionConstants'; import * as eg from '../../__tests__/lib/exampleData'; import { ALL_PRIVATE_NARROW, HOME_NARROW } from '../../utils/narrow'; +import { makeUserId } from '../../api/idTypes'; describe('messagesReducer', () => { describe('EVENT_NEW_MESSAGE', () => { @@ -344,19 +345,19 @@ describe('messagesReducer', () => { ...eg.unicodeEmojiReaction, emoji_code: '1f44b', emoji_name: 'wave', - user_id: 1, + user_id: makeUserId(1), }; const reaction2 = { ...eg.unicodeEmojiReaction, emoji_code: '1f44b', emoji_name: 'wave', - user_id: 2, + user_id: makeUserId(2), }; const reaction3 = { ...eg.unicodeEmojiReaction, emoji_code: '1f6e0', emoji_name: 'working_on_it', - user_id: 1, + user_id: makeUserId(1), }; const message1 = eg.streamMessage({ id: 1, reactions: [reaction1, reaction2, reaction3] }); diff --git a/src/pm-conversations/__tests__/pmConversationsModel-test.js b/src/pm-conversations/__tests__/pmConversationsModel-test.js index c7e2708e5f1..410c6ef5aa1 100644 --- a/src/pm-conversations/__tests__/pmConversationsModel-test.js +++ b/src/pm-conversations/__tests__/pmConversationsModel-test.js @@ -3,6 +3,7 @@ import Immutable from 'immutable'; import { usersOfKey, keyOfExactUsers, reducer } from '../pmConversationsModel'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeUserId } from '../../api/idTypes'; describe('usersOfKey', () => { for (const [desc, ids] of [ @@ -11,7 +12,7 @@ describe('usersOfKey', () => { ['group PM', [123, 345, 567]], ]) { test(desc, () => { - expect(usersOfKey(keyOfExactUsers(ids))).toEqual(ids); + expect(usersOfKey(keyOfExactUsers(ids.map(makeUserId)))).toEqual(ids); }); } }); @@ -33,8 +34,8 @@ describe('reducer', () => { // Out of order. const recent_private_conversations = [ { user_ids: [], max_message_id: 234 }, - { user_ids: [1], max_message_id: 123 }, - { user_ids: [2, 1], max_message_id: 345 }, // user_ids out of order + { user_ids: [makeUserId(1)], max_message_id: 123 }, + { user_ids: [2, 1].map(makeUserId), max_message_id: 345 }, // user_ids out of order ]; const expected = { map: Immutable.Map([['', 234], ['1', 123], ['1,2', 345]]), diff --git a/src/typing/__tests__/typingReducer-test.js b/src/typing/__tests__/typingReducer-test.js index 49dc9212a32..4ac73012642 100644 --- a/src/typing/__tests__/typingReducer-test.js +++ b/src/typing/__tests__/typingReducer-test.js @@ -7,10 +7,11 @@ import { EVENT_TYPING_START, EVENT_TYPING_STOP } from '../../actionConstants'; import typingReducer from '../typingReducer'; import { NULL_OBJECT } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeUserId } from '../../api/idTypes'; describe('typingReducer', () => { - const user1 = { ...eg.otherUser, user_id: 1 }; - const user2 = { ...eg.thirdUser, user_id: 2 }; + const user1 = { ...eg.otherUser, user_id: makeUserId(1) }; + const user2 = { ...eg.thirdUser, user_id: makeUserId(2) }; const egTypingAction = (args: {| op: 'start' | 'stop', diff --git a/src/unread/__tests__/unreadHuddlesReducer-test.js b/src/unread/__tests__/unreadHuddlesReducer-test.js index b381fb2e10e..82b67a6ff69 100644 --- a/src/unread/__tests__/unreadHuddlesReducer-test.js +++ b/src/unread/__tests__/unreadHuddlesReducer-test.js @@ -9,6 +9,7 @@ import { } from '../../actionConstants'; import { NULL_ARRAY } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeUserId } from '../../api/idTypes'; describe('unreadHuddlesReducer', () => { describe('ACCOUNT_SWITCH', () => { @@ -71,9 +72,9 @@ describe('unreadHuddlesReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('if message id already exists, do not mutate state', () => { - const user1 = { ...eg.makeUser(), user_id: 1 }; - const user2 = { ...eg.makeUser(), user_id: 2 }; - const user3 = { ...eg.makeUser(), user_id: 3 }; + const user1 = { ...eg.makeUser(), user_id: makeUserId(1) }; + const user2 = { ...eg.makeUser(), user_id: makeUserId(2) }; + const user3 = { ...eg.makeUser(), user_id: makeUserId(3) }; const message1 = eg.pmMessage({ id: 1, recipients: [user1, user2, user3] }); const message2 = eg.pmMessage({ id: 2, recipients: [user1, user2, user3] }); @@ -118,9 +119,9 @@ describe('unreadHuddlesReducer', () => { }); test('if message is sent by self, do not mutate state', () => { - const selfUser = { ...eg.selfUser, user_id: 1 }; - const user2 = { ...eg.otherUser, user_id: 2 }; - const user3 = { ...eg.thirdUser, user_id: 3 }; + const selfUser = { ...eg.selfUser, user_id: makeUserId(1) }; + const user2 = { ...eg.otherUser, user_id: makeUserId(2) }; + const user3 = { ...eg.thirdUser, user_id: makeUserId(3) }; const initialState = deepFreeze([]); @@ -142,9 +143,9 @@ describe('unreadHuddlesReducer', () => { }); test('if message id does not exist, append to state', () => { - const selfUser = { ...eg.selfUser, user_id: 1 }; - const user2 = { ...eg.otherUser, user_id: 2 }; - const user3 = { ...eg.thirdUser, user_id: 3 }; + const selfUser = { ...eg.selfUser, user_id: makeUserId(1) }; + const user2 = { ...eg.otherUser, user_id: makeUserId(2) }; + const user3 = { ...eg.thirdUser, user_id: makeUserId(3) }; const message4 = eg.pmMessage({ id: 4, recipients: [selfUser, user2, user3] }); @@ -174,9 +175,9 @@ describe('unreadHuddlesReducer', () => { }); test('if sender-ids string does not exist, append to state as new', () => { - const user1 = { ...eg.selfUser, user_id: 1 }; - const user2 = { ...eg.otherUser, user_id: 2 }; - const user3 = { ...eg.thirdUser, user_id: 3 }; + const user1 = { ...eg.selfUser, user_id: makeUserId(1) }; + const user2 = { ...eg.otherUser, user_id: makeUserId(2) }; + const user3 = { ...eg.thirdUser, user_id: makeUserId(3) }; const message4 = eg.pmMessage({ id: 4, recipients: [user1, user2, user3] }); const initialState = deepFreeze([ diff --git a/src/unread/__tests__/unreadPmsReducer-test.js b/src/unread/__tests__/unreadPmsReducer-test.js index 9ab1f95f3b2..4cf958ca7f8 100644 --- a/src/unread/__tests__/unreadPmsReducer-test.js +++ b/src/unread/__tests__/unreadPmsReducer-test.js @@ -5,13 +5,14 @@ import unreadPmsReducer from '../unreadPmsReducer'; import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants'; import { NULL_ARRAY } from '../../nullObjects'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeUserId } from '../../api/idTypes'; describe('unreadPmsReducer', () => { describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3], }, ]); @@ -95,7 +96,7 @@ describe('unreadPmsReducer', () => { const message4 = eg.streamMessage({ id: 4 }); const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3], }, ]); @@ -162,7 +163,7 @@ describe('unreadPmsReducer', () => { const message4 = eg.pmMessage({ id: 4, sender_id: 2 }); const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3], }, ]); @@ -174,7 +175,7 @@ describe('unreadPmsReducer', () => { const expectedState = [ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3], }, { @@ -211,11 +212,11 @@ describe('unreadPmsReducer', () => { test('if id does not exist do not mutate state', () => { const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3, 4, 5], }, { - sender_id: 2, + sender_id: makeUserId(2), unread_message_ids: [4, 5], }, ]); @@ -238,11 +239,11 @@ describe('unreadPmsReducer', () => { test('if ids are in state remove them', () => { const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3], }, { - sender_id: 2, + sender_id: makeUserId(2), unread_message_ids: [4, 5], }, ]); @@ -272,7 +273,7 @@ describe('unreadPmsReducer', () => { test('when operation is "remove" do nothing', () => { const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3, 4, 5], }, ]); @@ -295,7 +296,7 @@ describe('unreadPmsReducer', () => { test('when "all" is true reset state', () => { const initialState = deepFreeze([ { - sender_id: 1, + sender_id: makeUserId(1), unread_message_ids: [1, 2, 3, 4, 5], }, ]); diff --git a/src/utils/__tests__/recipient-test.js b/src/utils/__tests__/recipient-test.js index 5664459a8cc..a3133cb294f 100644 --- a/src/utils/__tests__/recipient-test.js +++ b/src/utils/__tests__/recipient-test.js @@ -7,10 +7,11 @@ import { pmKeyRecipientsFromIds, } from '../recipient'; import * as eg from '../../__tests__/lib/exampleData'; +import { makeUserId } from '../../api/idTypes'; describe('normalizeRecipientsAsUserIds', () => { test('joins user IDs from recipients, sorted', () => { - const recipients = [22, 1, 5, 3, 4]; + const recipients = [22, 1, 5, 3, 4].map(makeUserId); const expectedResult = '1,3,4,5,22'; const normalized = normalizeRecipientsAsUserIds(recipients); @@ -19,7 +20,7 @@ describe('normalizeRecipientsAsUserIds', () => { }); test('for a single recipient, returns the user ID as string', () => { - const recipients = [1]; + const recipients = [1].map(makeUserId); const expectedResult = '1'; const normalized = normalizeRecipientsAsUserIds(recipients); @@ -30,8 +31,8 @@ describe('normalizeRecipientsAsUserIds', () => { describe('normalizeRecipientsAsUserIdsSansMe', () => { test('if only self user ID provided return unmodified', () => { - const recipients = [1]; - const ownUserId = 1; + const recipients = [1].map(makeUserId); + const ownUserId = makeUserId(1); const expectedResult = '1'; const normalized = normalizeRecipientsAsUserIdsSansMe(recipients, ownUserId); @@ -40,9 +41,9 @@ describe('normalizeRecipientsAsUserIdsSansMe', () => { }); test('when more than one user IDs normalize but filter out self user ID', () => { - const recipients = [22, 1, 5, 3, 4]; + const recipients = [22, 1, 5, 3, 4].map(makeUserId); const expectedResult = '3,4,5,22'; - const ownUserId = 1; + const ownUserId = makeUserId(1); const normalized = normalizeRecipientsAsUserIdsSansMe(recipients, ownUserId); From 11f5b83d50944b6e4bca85b6a940c509836291b7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 20:46:21 -0800 Subject: [PATCH 08/24] user types: Use makeUserId when parsing a user ID from a string. --- src/compose/MentionWarnings.js | 3 ++- src/notification/index.js | 3 ++- src/pm-conversations/pmConversationsModel.js | 7 ++++--- src/utils/internalLinks.js | 3 ++- src/utils/narrow.js | 3 ++- src/webview/handleOutboundEvents.js | 6 +++--- src/webview/js/generatedEs3.js | 8 ++++++-- src/webview/js/js.js | 5 +++-- 8 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/compose/MentionWarnings.js b/src/compose/MentionWarnings.js index 9f1a2fa1200..bbc9b3010fd 100644 --- a/src/compose/MentionWarnings.js +++ b/src/compose/MentionWarnings.js @@ -11,6 +11,7 @@ import * as api from '../api'; import { showToast } from '../utils/info'; import MentionedUserNotSubscribed from '../message/MentionedUserNotSubscribed'; +import { makeUserId } from '../api/idTypes'; type State = {| unsubscribedMentions: Array, @@ -57,7 +58,7 @@ class MentionWarnings extends PureComponent { } if (userIdRaw !== undefined) { - const userId = Number.parseInt(userIdRaw, 10); + const userId = makeUserId(Number.parseInt(userIdRaw, 10)); return usersById.get(userId); } diff --git a/src/notification/index.js b/src/notification/index.js index 52300f52b23..91c889c615f 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -18,6 +18,7 @@ import { identityOfAuth } from '../account/accountMisc'; import { fromAPNs } from './extract'; import { tryParseUrl } from '../utils/url'; import { pmKeyRecipientsFromIds } from '../utils/recipient'; +import { makeUserId } from '../api/idTypes'; /** * Identify the account the notification is for, if possible. @@ -107,7 +108,7 @@ export const getNarrowFromNotificationData = ( return (user && pm1to1NarrowFromUser(user)) ?? null; } - const ids = data.pm_users.split(',').map(s => parseInt(s, 10)); + const ids = data.pm_users.split(',').map(s => makeUserId(parseInt(s, 10))); const users = pmKeyRecipientsFromIds(ids, allUsersById, ownUserId); return users === null ? null : pmNarrowFromUsers(users); }; diff --git a/src/pm-conversations/pmConversationsModel.js b/src/pm-conversations/pmConversationsModel.js index c504692a135..7d6224e4a0c 100644 --- a/src/pm-conversations/pmConversationsModel.js +++ b/src/pm-conversations/pmConversationsModel.js @@ -9,8 +9,9 @@ import { MESSAGE_FETCH_COMPLETE, REALM_INIT, } from '../actionConstants'; +import { makeUserId } from '../api/idTypes'; -import type { Action, Message, Outbox } from '../types'; +import type { Action, Message, Outbox, UserId } from '../types'; import { recipientsOfPrivateMessage } from '../utils/recipient'; import { ZulipVersion } from '../utils/zulipVersion'; @@ -47,8 +48,8 @@ function keyOfPrivateMessage(msg: Message | Outbox, ownUserId: number): PmConver } /** The users in the conversation, other than self. */ -export function usersOfKey(key: PmConversationKey): number[] { - return key ? key.split(',').map(s => Number.parseInt(s, 10)) : []; +export function usersOfKey(key: PmConversationKey): UserId[] { + return key ? key.split(',').map(s => makeUserId(Number.parseInt(s, 10))) : []; } // diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 68e37e6783c..b65c8ff69e7 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import { addBreadcrumb } from '@sentry/react-native'; +import { makeUserId } from '../api/idTypes'; import type { Narrow, Stream, UserOrBot } from '../types'; import { topicNarrow, streamNarrow, specialNarrow, pmNarrowFromUsers } from './narrow'; import { pmKeyRecipientsFromIds } from './recipient'; @@ -139,7 +140,7 @@ const parseTopicOperand = operand => decodeHashComponent(operand); /** Parse the operand of a `pm-with` operator. */ const parsePmOperand = operand => { const idStrs = operand.split('-')[0].split(','); - return idStrs.map(s => parseInt(s, 10)); + return idStrs.map(s => makeUserId(parseInt(s, 10))); }; /** diff --git a/src/utils/narrow.js b/src/utils/narrow.js index f8e28e9f149..3c557d70769 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,5 +1,6 @@ /* @flow strict-local */ +import { makeUserId } from '../api/idTypes'; import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types'; import { normalizeRecipientsAsUserIdsSansMe, @@ -294,7 +295,7 @@ export const parseNarrow = (narrowStr: string): Narrow => { } case 'pm:': { - const ids = rest.split(',').map(s => Number.parseInt(s, 10)); + const ids = rest.split(',').map(s => makeUserId(Number.parseInt(s, 10))); return pmNarrowInternal(ids); } diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index 2773a2be46d..e28efe5b0df 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -4,7 +4,7 @@ import { Clipboard, Alert } from 'react-native'; import * as NavigationService from '../nav/NavigationService'; import * as api from '../api'; import config from '../config'; -import type { Dispatch, GetText, Message, Narrow, Outbox, EditMessage } from '../types'; +import type { Dispatch, GetText, Message, Narrow, Outbox, EditMessage, UserId } from '../types'; import type { BackgroundData } from './MessageList'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; import type { JSONableDict } from '../utils/jsonable'; @@ -55,7 +55,7 @@ type WebViewOutboundEventScroll = {| type WebViewOutboundEventAvatar = {| type: 'avatar', - fromUserId: number, + fromUserId: UserId, |}; type WebViewOutboundEventNarrow = {| @@ -121,7 +121,7 @@ type WebViewOutboundEventReactionDetails = {| type WebViewOutboundEventMention = {| type: 'mention', - userId: number, + userId: UserId, |}; type WebViewOutboundEventTimeDetails = {| diff --git a/src/webview/js/generatedEs3.js b/src/webview/js/generatedEs3.js index 8ff8e4146bf..af21cb561cc 100644 --- a/src/webview/js/generatedEs3.js +++ b/src/webview/js/generatedEs3.js @@ -85,6 +85,10 @@ var compiledWebviewJs = (function (exports) { return target; } + var makeUserId = function makeUserId(id) { + return id; + }; + var sendMessage = (function (msg) { window.ReactNativeWebView.postMessage(JSON.stringify(msg)); }); @@ -863,7 +867,7 @@ var compiledWebviewJs = (function (exports) { if (target.matches('.avatar-img')) { sendMessage({ type: 'avatar', - fromUserId: requireNumericAttribute(target, 'data-sender-id') + fromUserId: makeUserId(requireNumericAttribute(target, 'data-sender-id')) }); return; } @@ -879,7 +883,7 @@ var compiledWebviewJs = (function (exports) { if (target.matches('.user-mention')) { sendMessage({ type: 'mention', - userId: requireNumericAttribute(target, 'data-user-id') + userId: makeUserId(requireNumericAttribute(target, 'data-user-id')) }); return; } diff --git a/src/webview/js/js.js b/src/webview/js/js.js index 13901a8bf00..5c9a0b9f8ac 100644 --- a/src/webview/js/js.js +++ b/src/webview/js/js.js @@ -9,6 +9,7 @@ import type { WebViewInboundEventReady, WebViewInboundEventMessagesRead, } from '../generateInboundEvents'; +import { makeUserId } from '../../api/idTypes'; import InboundEventLogger from './InboundEventLogger'; import sendMessage from './sendMessage'; @@ -726,7 +727,7 @@ documentBody.addEventListener('click', (e: MouseEvent) => { if (target.matches('.avatar-img')) { sendMessage({ type: 'avatar', - fromUserId: requireNumericAttribute(target, 'data-sender-id'), + fromUserId: makeUserId(requireNumericAttribute(target, 'data-sender-id')), }); return; } @@ -742,7 +743,7 @@ documentBody.addEventListener('click', (e: MouseEvent) => { if (target.matches('.user-mention')) { sendMessage({ type: 'mention', - userId: requireNumericAttribute(target, 'data-user-id'), + userId: makeUserId(requireNumericAttribute(target, 'data-user-id')), }); return; } From 117049992d5cbe5ee668e8ebfa5f2776822564d1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 22:38:58 -0800 Subject: [PATCH 09/24] user types: Use makeUserId in our fake-yet-not-test data. This NULL_USER object should really go away, and perhaps it will soon. Until then, mark this fake user-ID value of -1 as indeed being meant as a (fake) user ID. --- src/nullObjects.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nullObjects.js b/src/nullObjects.js index a7ae5766370..2132f7059a2 100644 --- a/src/nullObjects.js +++ b/src/nullObjects.js @@ -1,4 +1,5 @@ /* @flow strict-local */ +import { makeUserId } from './api/idTypes'; import type { User, Subscription } from './types'; import { GravatarURL } from './utils/avatar'; @@ -38,7 +39,7 @@ export const NULL_USER: User = { is_admin: false, is_bot: false, timezone: '', - user_id: -1, + user_id: makeUserId(-1), }; /** DEPRECATED; don't add new uses. See block comment above definition. */ From 31e0e3ffa70ee48d0b0e461b023fccf4399629b8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 12 Dec 2020 01:19:41 -0800 Subject: [PATCH 10/24] api types: Use UserId in most of the API. Specifically, use it in all the places that are purely an "output position" (or nearly so), i.e. where we consume a value from this type but don't try to put a value back into it. This leaves out a few parts of the API we'll get to in subsequent commits: * a few places which are "input positions", meaning the API code accepts values of this type instead of providing them -- in particular when we're providing user IDs as parameters; * a few places where we reuse the type for new values we construct for our own internal data structures, so that the type is in an input position there as well as an output position. Those will require updating more of the app too, so that we can supply UserId values. --- src/actionTypes.js | 17 +++++++++-------- src/api/eventTypes.js | 4 ++-- src/api/initialDataTypes.js | 5 +++-- src/api/messages/getMessages.js | 4 ++-- src/api/modelTypes.js | 17 +++++++++-------- src/api/users/getUserProfile.js | 3 ++- src/events/eventToAction.js | 4 ++-- src/notification/extract.js | 4 ++-- src/types.js | 8 ++++---- src/utils/avatar.js | 6 +++--- 10 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/actionTypes.js b/src/actionTypes.js index 24b935ce8f5..901dce303b5 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -80,6 +80,7 @@ import type { CaughtUpState, MuteState, AlertWordsState, + UserId, UserStatusEvent, } from './types'; import type { ZulipVersion } from './utils/zulipVersion'; @@ -285,7 +286,7 @@ type EventSubscriptionPeerAddAction = {| type: typeof EVENT_SUBSCRIPTION, op: 'peer_add', subscriptions: string[], - user_id: number, + user_id: UserId, |}; type EventSubscriptionPeerRemoveAction = {| @@ -293,7 +294,7 @@ type EventSubscriptionPeerRemoveAction = {| type: typeof EVENT_SUBSCRIPTION, op: 'peer_remove', subscriptions: string[], - user_id: number, + user_id: UserId, |}; type GenericEventAction = {| @@ -345,7 +346,7 @@ type EventUpdateMessageAction = {| rendered_content: string, subject_links: string[], subject: string, - user_id: number, + user_id: UserId, |}; type EventReactionCommon = {| @@ -375,11 +376,11 @@ type EventTypingCommon = {| ...ServerEvent, ownUserId: number, recipients: $ReadOnlyArray<{ - user_id: number, + user_id: UserId, email: string, }>, sender: { - user_id: number, + user_id: UserId, email: string, }, time: number, @@ -422,7 +423,7 @@ type EventUserRemoveAction = {| type EventUserUpdateAction = {| ...ServerEvent, type: typeof EVENT_USER_UPDATE, - userId: number, + userId: UserId, // Include only the fields that should be overwritten. person: $Shape, |}; @@ -460,7 +461,7 @@ type EventUserGroupAddMembersAction = {| type: typeof EVENT_USER_GROUP_ADD_MEMBERS, op: 'add_members', group_id: number, - user_ids: number[], + user_ids: UserId[], |}; type EventUserGroupRemoveMembersAction = {| @@ -468,7 +469,7 @@ type EventUserGroupRemoveMembersAction = {| type: typeof EVENT_USER_GROUP_REMOVE_MEMBERS, op: 'remove_members', group_id: number, - user_ids: number[], + user_ids: UserId[], |}; type EventRealmEmojiUpdateAction = {| diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index d9f552d53ba..92dac6a1938 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -10,7 +10,7 @@ * @flow strict-local */ -import type { Message, Stream, UserPresence } from './modelTypes'; +import type { Message, Stream, UserId, UserPresence } from './modelTypes'; export class EventTypes { static alert_words: 'alert_words' = 'alert_words'; @@ -75,7 +75,7 @@ export type SubmessageEvent = {| type: typeof EventTypes.submessage, submessage_id: number, message_id: number, - sender_id: number, + sender_id: UserId, msg_type: 'widget', content: string, |}; diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 05cefb7ded4..c807df3d55d 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -9,6 +9,7 @@ import type { Subscription, User, UserGroup, + UserId, UserPresence, UserStatusMapObject, } from './apiTypes'; @@ -113,7 +114,7 @@ export type RawInitialDataRealmUser = {| is_admin: boolean, realm_users: Array<{| ...User, avatar_url?: string | null |}>, realm_non_active_users: Array<{| ...User, avatar_url?: string | null |}>, - user_id: number, + user_id: UserId, |}; export type InitialDataRealmUser = {| @@ -204,7 +205,7 @@ export type StreamUnreadItem = {| unread_message_ids: number[], /** All distinct senders of these messages; sorted. */ - // sender_ids: number[], + // sender_ids: UserId[], |}; export type HuddlesUnreadItem = {| diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index 7e5d114b03a..d230106409b 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -2,7 +2,7 @@ import type { Auth, ApiResponseSuccess } from '../transportTypes'; import type { Identity } from '../../types'; import type { Message, ApiNarrow } from '../apiTypes'; -import type { Reaction } from '../modelTypes'; +import type { Reaction, UserId } from '../modelTypes'; import { apiGet } from '../apiFetch'; import { identityOfAuth } from '../../account/accountMisc'; import { AvatarURL } from '../../utils/avatar'; @@ -27,7 +27,7 @@ export type ServerReaction = $ReadOnly<{| user: $ReadOnly<{| email: string, full_name: string, - id: number, + id: UserId, |}>, |}>; diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index b5d9b202c1a..ea3f363b91d 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -5,6 +5,7 @@ */ import type { AvatarURL } from '../utils/avatar'; +import type { UserId } from './idTypes'; export type * from './idTypes'; @@ -77,7 +78,7 @@ export type DevUser = {| * * `UserOrBot` for a convenience union of the two */ export type User = {| - user_id: number, + user_id: UserId, email: string, full_name: string, @@ -158,7 +159,7 @@ export type CrossRealmBot = {| full_name: string, is_admin: boolean, is_bot: true, - user_id: number, + user_id: UserId, // The timezone field has been included since commit 58ee3fa8c (in 1.9.0). Tim // mentions in 2020-02, at @@ -186,7 +187,7 @@ export type UserOrBot = User | CrossRealmBot; export type UserGroup = {| description: string, id: number, - members: number[], + members: UserId[], name: string, |}; @@ -353,7 +354,7 @@ export type ReactionType = 'unicode_emoji' | 'realm_emoji' | 'zulip_extra_emoji' * normalize it to this form. */ export type Reaction = $ReadOnly<{| - user_id: number, + user_id: UserId, emoji_name: string, reaction_type: ReactionType, @@ -374,7 +375,7 @@ export type Reaction = $ReadOnly<{| * See also `MessageEdit`. */ export type MessageSnapshot = $ReadOnly<{| - user_id: number, + user_id: UserId, timestamp: number, /** Docs unclear but suggest absent if only content edited. */ @@ -402,7 +403,7 @@ export type MessageEdit = $ReadOnly<{| prev_rendered_content_version?: number, prev_subject?: string, timestamp: number, - user_id: number, + user_id: UserId, |}>; /** A user, as seen in the `display_recipient` of a PM `Message`. */ @@ -433,7 +434,7 @@ export type PmRecipientUser = $ReadOnly<{| export type Submessage = $ReadOnly<{| id: number, message_id: number, - sender_id: number, + sender_id: UserId, msg_type: 'widget', // only this type is currently available content: string, // JSON string |}>; @@ -529,7 +530,7 @@ export type Message = $ReadOnly<{| reactions: $ReadOnlyArray, sender_email: string, sender_full_name: string, - sender_id: number, + sender_id: UserId, sender_realm_str: string, sender_short_name: string, diff --git a/src/api/users/getUserProfile.js b/src/api/users/getUserProfile.js index 916ecc9e7d9..21a9c84db6e 100644 --- a/src/api/users/getUserProfile.js +++ b/src/api/users/getUserProfile.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import type { Auth, ApiResponseSuccess } from '../transportTypes'; +import { type UserId } from '../idTypes'; import { apiGet } from '../apiFetch'; type ApiResponseUserProfile = {| @@ -11,7 +12,7 @@ type ApiResponseUserProfile = {| is_bot: boolean, max_message_id: number, short_name: string, - user_id: number, + user_id: UserId, // pointer: number, /* deprecated 2020-02; see zulip/zulip#8994 */ |}; diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index e198780e7d9..691ba6cfa41 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -210,8 +210,8 @@ export default (state: GlobalState, event: $FlowFixMe): EventAction => { ...event, // Raw reaction events from the server have a variation on the - // properties of `Reaction`: instead of `user_id: number`, they have - // `user: {| email: string, full_name: string, user_id: number |}`. + // properties of `Reaction`: instead of `user_id: UserId`, they have + // `user: {| email: string, full_name: string, user_id: UserId |}`. // NB this is different from the reactions in a `/messages` response; // see `getMessages` to compare. user_id: event.user.user_id, diff --git a/src/notification/extract.js b/src/notification/extract.js index 19e41dea761..25b593609d7 100644 --- a/src/notification/extract.js +++ b/src/notification/extract.js @@ -54,7 +54,7 @@ const asDict = (obj: JSONableInput | void): JSONableInputDict | void => { // added 1.7.0-1351-g98943a8333, release 1.8.0+ sender_email: string, - sender_id: number, + sender_id: UserId, server: string, // settings.EXTERNAL_HOST realm_id: number, // server-internal realm identifier @@ -62,7 +62,7 @@ const asDict = (obj: JSONableInput | void): JSONableInputDict | void => { realm_uri: string, // as in `/server_settings` response // added 2.1-dev-540-g447a517e6f, release 2.1.0+ - user_id: number, // recipient id + user_id: UserId, // recipient id ...(StreamData | PmData), } }; diff --git a/src/types.js b/src/types.js index b0f3060387b..164cb8cc3d4 100644 --- a/src/types.js +++ b/src/types.js @@ -3,7 +3,7 @@ import type { IntlShape } from 'react-intl'; import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; import type { SubsetProperties } from './generics'; -import type { Auth, Topic, Message, ReactionType } from './api/apiTypes'; +import type { Auth, Topic, Message, ReactionType, UserId } from './api/apiTypes'; import type { ZulipVersion } from './utils/zulipVersion'; import type { PmKeyUsers } from './utils/recipient'; @@ -121,7 +121,7 @@ export type AggregatedReaction = {| name: string, selfReacted: boolean, type: ReactionType, - users: $ReadOnlyArray, + users: $ReadOnlyArray, |}; export type EditMessage = {| @@ -184,7 +184,7 @@ export type Outbox = $ReadOnly<{| // values that lack it; which is fine once the release that adds it has // been out for a few weeks. // (Also drop the hack line about it in MessageLike.) - sender_id?: number, + sender_id?: UserId, /* eslint-disable flowtype/generic-spacing */ ...SubsetProperties< @@ -237,7 +237,7 @@ export type MessageLike = | $ReadOnly<{ // $Shape is unsound, per Flow docs, but $ReadOnly<$Shape> is not ...$Shape<{ [$Keys]: void }>, - sender_id?: number, // TODO: Drop this once required in Outbox. + sender_id?: UserId, // TODO: Drop this once required in Outbox. ...Outbox, }>; diff --git a/src/utils/avatar.js b/src/utils/avatar.js index f2a931da8c8..94a8c9eab3c 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -4,7 +4,7 @@ import md5 from 'blueimp-md5'; import * as logging from './logging'; -import { ensureUnreachable } from '../types'; +import { ensureUnreachable, type UserId } from '../types'; import { isUrlAbsolute, isUrlPathAbsolute } from './url'; /** @@ -18,7 +18,7 @@ export class AvatarURL { */ static fromUserOrBotData(args: {| rawAvatarUrl: string | void | null, - userId: number, + userId: UserId, email: string, realm: URL, |}): AvatarURL { @@ -240,7 +240,7 @@ export class FallbackAvatarURL extends AvatarURL { // very expensive, and their use in these pseudo-constructors (which // process data at the edge, just as it's received from the server) // used to slow down `api.registerForEvents` quite a lot. - static validateAndConstructInstance(args: {| realm: URL, userId: number |}): FallbackAvatarURL { + static validateAndConstructInstance(args: {| realm: URL, userId: UserId |}): FallbackAvatarURL { const { realm, userId } = args; // Thankfully, this string concatenation is quite safe: we know // enough about our inputs here to compose a properly formatted From 075d971db4dc9414c6ba426f801382361029e7ef Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:04:48 -0800 Subject: [PATCH 11/24] user types: Use UserId for PM recipients. This PmRecipientUser type is one we reuse in some data structures we construct ourselves, in particular outbox messages. So to tighten its type, we need to propagate the fact that these values are user IDs through the various code that handles them on their way there, in particular the Narrow type and its constructors. --- src/api/modelTypes.js | 2 +- src/utils/narrow.js | 10 +++++----- src/utils/recipient.js | 18 +++++++++--------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index ea3f363b91d..d6d3fa4ca56 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -410,7 +410,7 @@ export type MessageEdit = $ReadOnly<{| export type PmRecipientUser = $ReadOnly<{| // These five fields (id, email, full_name, short_name, is_mirror_dummy) // have all been present since server commit 6b13f4a3c, in 2014. - id: number, + id: UserId, email: string, full_name: string, // We mark short_name and is_mirror_dummy optional so we can leave them diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 3c557d70769..91be48decad 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { makeUserId } from '../api/idTypes'; -import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types'; +import type { ApiNarrow, Message, Outbox, User, UserId, UserOrBot } from '../types'; import { normalizeRecipientsAsUserIdsSansMe, pmKeyRecipientsFromMessage, @@ -33,7 +33,7 @@ import { export opaque type Narrow = | {| type: 'stream', streamName: string |} | {| type: 'topic', streamName: string, topic: string |} - | {| type: 'pm', userIds: $ReadOnlyArray |} + | {| type: 'pm', userIds: $ReadOnlyArray |} | {| type: 'search', query: string |} | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; @@ -60,7 +60,7 @@ export const HOME_NARROW_STR: string = keyFromNarrow(HOME_NARROW); * accidentally disagreeing on whether to include the self-user, or on how * to sort the list (by user ID vs. email), or neglecting to sort it at all. */ -const pmNarrowInternal = (userIds: $ReadOnlyArray): Narrow => +const pmNarrowInternal = (userIds: $ReadOnlyArray): Narrow => Object.freeze({ type: 'pm', userIds }); /** @@ -153,7 +153,7 @@ export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 's type NarrowCases = {| home: () => T, - pm: (ids: $ReadOnlyArray) => T, + pm: (ids: $ReadOnlyArray) => T, starred: () => T, mentioned: () => T, allPrivate: () => T, @@ -338,7 +338,7 @@ export const isGroupPmNarrow = (narrow?: Narrow): boolean => * This is the same list of users that can appear in a `PmKeyRecipients` or * `PmKeyUsers`, but contains only their user IDs. */ -export const userIdsOfPmNarrow = (narrow: Narrow): $ReadOnlyArray => +export const userIdsOfPmNarrow = (narrow: Narrow): $ReadOnlyArray => caseNarrowPartial(narrow, { pm: ids => ids }); /** diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 31eed997b70..a2bf264ee5f 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -4,7 +4,7 @@ import isEqual from 'lodash.isequal'; import { mapOrNull } from '../collections'; import * as logging from './logging'; -import type { PmRecipientUser, Message, Outbox, User, UserOrBot } from '../types'; +import type { PmRecipientUser, Message, Outbox, User, UserId, UserOrBot } from '../types'; /** The stream name a stream message was sent to. Throws if a PM. */ export const streamNameOfStreamMessage = (message: Message | Outbox): string => { @@ -61,7 +61,7 @@ export const recipientsOfPrivateMessage = ( * * See also `pmNarrowFromRecipients`, which requires a value of this type. */ -export opaque type PmKeyRecipients: $ReadOnlyArray = $ReadOnlyArray; +export opaque type PmKeyRecipients: $ReadOnlyArray = $ReadOnlyArray; /** * A list of users identifying a PM conversation, as per pmKeyRecipientsFromMessage. @@ -100,9 +100,9 @@ const filterRecipientUsers = ( // Like filterRecipients, but on user IDs directly. const filterRecipientsAsUserIds = ( - recipients: $ReadOnlyArray, + recipients: $ReadOnlyArray, ownUserId: number, -): number[] => +): UserId[] => // prettier-ignore recipients.length === 1 // The spread is so that we always return a fresh array. This allows @@ -111,7 +111,7 @@ const filterRecipientsAsUserIds = ( ? [...recipients] : recipients.filter(r => r !== ownUserId).sort((a, b) => a - b); -export const normalizeRecipientsAsUserIds = (recipients: number[]) => +export const normalizeRecipientsAsUserIds = (recipients: UserId[]) => recipients.sort((a, b) => a - b).join(','); /** @@ -122,7 +122,7 @@ export const normalizeRecipientsAsUserIds = (recipients: number[]) => // server's behavior is quirkier... but we keep only one user for those // anyway, so it doesn't matter. export const normalizeRecipientsAsUserIdsSansMe = ( - recipients: $ReadOnlyArray, + recipients: $ReadOnlyArray, ownUserId: number, ) => normalizeRecipientsAsUserIds(filterRecipientsAsUserIds(recipients, ownUserId)); @@ -290,7 +290,7 @@ export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: number): s */ // See comment on pmUnreadsKeyFromMessage for details on this form. export const pmUnreadsKeyFromPmKeyIds = ( - userIds: $ReadOnlyArray, + userIds: $ReadOnlyArray, ownUserId: number, ): string => { if (userIds.length === 1) { @@ -319,7 +319,7 @@ export const pmUnreadsKeyFromPmKeyIds = ( // TODO: It'd be neat to have another opaque type like PmKeyIds, for this // and pmUnreadsKeyFromPmKeyIds to consume. Perhaps simplest to do after // Narrow no longer contains emails. -export const pmTypingKeyFromPmKeyIds = (userIds: $ReadOnlyArray): string => +export const pmTypingKeyFromPmKeyIds = (userIds: $ReadOnlyArray): string => userIds.join(','); /** @@ -337,7 +337,7 @@ export const pmTypingKeyFromPmKeyIds = (userIds: $ReadOnlyArray): string // for these events.) // * Self-PMs don't have typing-status events in the first place. export const pmTypingKeyFromRecipients = ( - recipients: $ReadOnlyArray, + recipients: $ReadOnlyArray, ownUserId: number, ): string => pmTypingKeyFromPmKeyIds(filterRecipientsAsUserIds(recipients, ownUserId)); From 19c8189fad4447d189aac678b1a91f92eb9fdeb7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Jan 2021 01:17:32 -0800 Subject: [PATCH 12/24] user types: Use UserId for "own user ID" data. This propagates the UserId type through where we store in Redux the self user's user ID, to the `getOwnUserId` selector that gets that information, and on to all the values fed by that selector. We already have a pretty consistent convention of calling those values `ownUserId`, so this is an easy change to make. --- src/actionTypes.js | 4 ++-- src/compose/ComposeBox.js | 3 ++- src/compose/getComposeInputPlaceholder.js | 4 ++-- src/message/fetchActions.js | 4 ++-- src/notification/index.js | 4 ++-- src/pm-conversations/pmConversationsModel.js | 4 ++-- src/reactions/MessageReactionList.js | 4 ++-- src/reactions/aggregateReactions.js | 4 ++-- src/reduxTypes.js | 3 ++- src/user-groups/CreateGroupScreen.js | 4 ++-- src/users/userSelectors.js | 4 ++-- src/utils/internalLinks.js | 4 ++-- src/utils/recipient.js | 20 ++++++++++---------- src/webview/html/messageAsHtml.js | 3 ++- 14 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/actionTypes.js b/src/actionTypes.js index 901dce303b5..aa29b28f94e 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -219,7 +219,7 @@ type MessageFetchCompleteAction = {| numAfter: number, foundNewest: boolean | void, foundOldest: boolean | void, - ownUserId: number, + ownUserId: UserId, |}; type InitialFetchStartAction = {| @@ -374,7 +374,7 @@ type EventPresenceAction = {| type EventTypingCommon = {| ...ServerEvent, - ownUserId: number, + ownUserId: UserId, recipients: $ReadOnlyArray<{ user_id: UserId, email: string, diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index dc7bae366b9..c1893c084fc 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -20,6 +20,7 @@ import type { GetText, Subscription, Stream, + UserId, VideoChatProvider, } from '../types'; import { connect } from '../react-redux'; @@ -60,7 +61,7 @@ import { getActiveUsersById, getOwnUserId } from '../users/userSelectors'; type SelectorProps = {| auth: Auth, - ownUserId: number, + ownUserId: UserId, usersById: Map, isAdmin: boolean, isAnnouncementOnly: boolean, diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index 0f833bced96..4aed77acbbb 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -1,10 +1,10 @@ /* @flow strict-local */ -import type { Narrow, UserOrBot, LocalizableText } from '../types'; +import type { Narrow, UserId, UserOrBot, LocalizableText } from '../types'; import { caseNarrowDefault } from '../utils/narrow'; export default ( narrow: Narrow, - ownUserId: number, + ownUserId: UserId, usersById: Map, ): LocalizableText => caseNarrowDefault( diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index a7d695abc97..aef9abc7305 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -1,6 +1,6 @@ /* @flow strict-local */ import * as NavigationService from '../nav/NavigationService'; -import type { Narrow, Dispatch, GetState, GlobalState, Message, Action } from '../types'; +import type { Narrow, Dispatch, GetState, GlobalState, Message, Action, UserId } from '../types'; import type { ApiResponseServerSettings } from '../api/settings/getServerSettings'; import type { InitialData } from '../api/initialDataTypes'; import * as api from '../api'; @@ -59,7 +59,7 @@ const messageFetchComplete = (args: {| numAfter: number, foundNewest?: boolean, foundOldest?: boolean, - ownUserId: number, + ownUserId: UserId, |}): Action => { const { messages, diff --git a/src/notification/index.js b/src/notification/index.js index 91c889c615f..4ec4e23d34c 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -3,7 +3,7 @@ import { DeviceEventEmitter, NativeModules, Platform, PushNotificationIOS } from import NotificationsIOS from 'react-native-notifications'; import type { Notification } from './types'; -import type { Auth, Dispatch, Identity, Narrow, UserOrBot } from '../types'; +import type { Auth, Dispatch, Identity, Narrow, UserId, UserOrBot } from '../types'; import { topicNarrow, pmNarrowFromUsers, pm1to1NarrowFromUser } from '../utils/narrow'; import type { JSONable, JSONableDict } from '../utils/jsonable'; import * as api from '../api'; @@ -88,7 +88,7 @@ export const getNarrowFromNotificationData = ( data: Notification, allUsersById: Map, allUsersByEmail: Map, - ownUserId: number, + ownUserId: UserId, ): Narrow | null => { if (!data.recipient_type) { // This condition is impossible if the value is rightly-typed; but in diff --git a/src/pm-conversations/pmConversationsModel.js b/src/pm-conversations/pmConversationsModel.js index 7d6224e4a0c..5a4c3b451d4 100644 --- a/src/pm-conversations/pmConversationsModel.js +++ b/src/pm-conversations/pmConversationsModel.js @@ -38,12 +38,12 @@ export function keyOfExactUsers(ids: number[]): PmConversationKey { } // Input may contain self or not, and needn't be sorted. -function keyOfUsers(ids: number[], ownUserId: number): PmConversationKey { +function keyOfUsers(ids: number[], ownUserId: UserId): PmConversationKey { return keyOfExactUsers(ids.filter(id => id !== ownUserId)); } // Input must indeed be a PM, else throws. -function keyOfPrivateMessage(msg: Message | Outbox, ownUserId: number): PmConversationKey { +function keyOfPrivateMessage(msg: Message | Outbox, ownUserId: UserId): PmConversationKey { return keyOfUsers(recipientsOfPrivateMessage(msg).map(r => r.id), ownUserId); } diff --git a/src/reactions/MessageReactionList.js b/src/reactions/MessageReactionList.js index 6b9dd6ebd73..c9c8584b6b0 100644 --- a/src/reactions/MessageReactionList.js +++ b/src/reactions/MessageReactionList.js @@ -8,7 +8,7 @@ import * as NavigationService from '../nav/NavigationService'; import * as logging from '../utils/logging'; import ReactionUserList from './ReactionUserList'; import { connect } from '../react-redux'; -import type { Dispatch, EmojiType, Message, ReactionType } from '../types'; +import type { Dispatch, EmojiType, Message, ReactionType, UserId } from '../types'; import { Screen, Label, RawLabel } from '../common'; import { getOwnUser } from '../selectors'; import aggregateReactions from './aggregateReactions'; @@ -28,7 +28,7 @@ const emojiTypeFromReactionType = (reactionType: ReactionType): EmojiType => { type SelectorProps = $ReadOnly<{| message: Message | void, - ownUserId: number, + ownUserId: UserId, |}>; type Props = $ReadOnly<{| diff --git a/src/reactions/aggregateReactions.js b/src/reactions/aggregateReactions.js index 017ef013925..0b888f9cfe6 100644 --- a/src/reactions/aggregateReactions.js +++ b/src/reactions/aggregateReactions.js @@ -1,9 +1,9 @@ /* @flow strict-local */ -import type { Reaction, AggregatedReaction } from '../types'; +import type { Reaction, AggregatedReaction, UserId } from '../types'; export default ( reactions: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): $ReadOnlyArray => { const reactionMap = new Map(); reactions.forEach(x => { diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 85cdbb26b90..9db65d319a7 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -23,6 +23,7 @@ import type { Subscription, User, UserGroup, + UserId, UserPresence, UserStatusMapObject, } from './api/apiTypes'; @@ -261,7 +262,7 @@ export type RealmState = {| videoChatProvider: VideoChatProvider | null, email: string | void, - user_id: number | void, + user_id: UserId | void, twentyFourHourTime: boolean, canCreateStreams: boolean, isAdmin: boolean, diff --git a/src/user-groups/CreateGroupScreen.js b/src/user-groups/CreateGroupScreen.js index 5d85fa227be..7120793ed32 100644 --- a/src/user-groups/CreateGroupScreen.js +++ b/src/user-groups/CreateGroupScreen.js @@ -3,7 +3,7 @@ import React, { PureComponent } from 'react'; import type { AppNavigationProp, AppNavigationRouteProp } from '../nav/AppNavigator'; import * as NavigationService from '../nav/NavigationService'; -import type { Dispatch, User } from '../types'; +import type { Dispatch, User, UserId } from '../types'; import { connect } from '../react-redux'; import { Screen } from '../common'; import { doNarrow, navigateBack } from '../actions'; @@ -13,7 +13,7 @@ import UserPickerCard from '../user-picker/UserPickerCard'; import { getOwnUserId } from '../users/userSelectors'; type SelectorProps = {| - +ownUserId: number, + +ownUserId: UserId, |}; type Props = $ReadOnly<{| diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index 3843505f7b7..ab33a261afd 100644 --- a/src/users/userSelectors.js +++ b/src/users/userSelectors.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { createSelector } from 'reselect'; -import type { GlobalState, UserOrBot, Selector, User } from '../types'; +import type { GlobalState, UserOrBot, Selector, User, UserId } from '../types'; import { NULL_USER } from '../nullObjects'; import { getUsers, getCrossRealmBots, getNonActiveUsers } from '../directSelectors'; @@ -96,7 +96,7 @@ export const getSortedUsers: Selector = createSelector( */ // Not currently used, but should replace uses of `getOwnEmail` (e.g. inside // `getOwnUser`). See #3764. -export const getOwnUserId = (state: GlobalState): number => { +export const getOwnUserId = (state: GlobalState): UserId => { const { user_id } = state.realm; if (user_id === undefined) { throw new Error('No server data found'); diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index b65c8ff69e7..34cf8e0488b 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 { makeUserId } from '../api/idTypes'; -import type { Narrow, Stream, UserOrBot } from '../types'; +import type { Narrow, Stream, UserId, UserOrBot } from '../types'; import { topicNarrow, streamNarrow, specialNarrow, pmNarrowFromUsers } from './narrow'; import { pmKeyRecipientsFromIds } from './recipient'; @@ -155,7 +155,7 @@ export const getNarrowFromLink = ( realm: URL, allUsersById: Map, streamsById: Map, - ownUserId: number, + ownUserId: UserId, ): Narrow | null => { const type = getLinkType(url, realm); const paths = getPathsFromUrl(url, realm); diff --git a/src/utils/recipient.js b/src/utils/recipient.js index a2bf264ee5f..d6fa2c7d835 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -83,7 +83,7 @@ export opaque type PmKeyUsers: $ReadOnlyArray = $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): $ReadOnlyArray => recipients.length === 1 ? recipients @@ -92,7 +92,7 @@ const filterRecipients = ( // Like filterRecipients, but on User objects. const filterRecipientUsers = ( recipients: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): $ReadOnlyArray => recipients.length === 1 ? recipients @@ -101,7 +101,7 @@ const filterRecipientUsers = ( // Like filterRecipients, but on user IDs directly. const filterRecipientsAsUserIds = ( recipients: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): UserId[] => // prettier-ignore recipients.length === 1 @@ -123,7 +123,7 @@ export const normalizeRecipientsAsUserIds = (recipients: UserId[]) => // anyway, so it doesn't matter. export const normalizeRecipientsAsUserIdsSansMe = ( recipients: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ) => normalizeRecipientsAsUserIds(filterRecipientsAsUserIds(recipients, ownUserId)); /** @@ -202,7 +202,7 @@ export const pmKeyRecipientsFromMessage = ( export const pmKeyRecipientsFromIds = ( userIds: $ReadOnlyArray, allUsersById: Map, - ownUserId: number, + ownUserId: UserId, ): PmKeyUsers | null => { const resultIds = userIds.filter(id => id !== ownUserId); if (resultIds.length === 0) { @@ -223,7 +223,7 @@ export const pmKeyRecipientsFromIds = ( export const pmKeyRecipientUsersFromMessage = ( message: Message | Outbox, allUsersById: Map, - ownUserId: number, + ownUserId: UserId, ): PmKeyUsers | null => { const userIds = recipientsOfPrivateMessage(message).map(r => r.id); return pmKeyRecipientsFromIds(userIds, allUsersById, ownUserId); @@ -234,7 +234,7 @@ export const pmKeyRecipientUsersFromMessage = ( */ export const pmKeyRecipientsFromUsers = ( users: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): PmKeyUsers => filterRecipientUsers(users, ownUserId); /** @@ -257,7 +257,7 @@ export const pmKeyRecipientsFromUsers = ( // and just the other user ID for non-self 1:1s; and in each case the list // is sorted numerically and encoded in ASCII-decimal, comma-separated. // See the `unread_msgs` data structure in `src/api/initialDataTypes.js`. -export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: number): string => { +export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: UserId): string => { if (message.type !== 'private') { throw new Error('pmUnreadsKeyFromMessage: expected PM, got stream message'); } @@ -291,7 +291,7 @@ export const pmUnreadsKeyFromMessage = (message: Message, ownUserId?: number): s // See comment on pmUnreadsKeyFromMessage for details on this form. export const pmUnreadsKeyFromPmKeyIds = ( userIds: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): string => { if (userIds.length === 1) { // A 1:1 PM. Both forms include just one user: the other user if any, @@ -338,7 +338,7 @@ export const pmTypingKeyFromPmKeyIds = (userIds: $ReadOnlyArray): string // * Self-PMs don't have typing-status events in the first place. export const pmTypingKeyFromRecipients = ( recipients: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, ): string => pmTypingKeyFromPmKeyIds(filterRecipientsAsUserIds(recipients, ownUserId)); export const isSameRecipient = ( diff --git a/src/webview/html/messageAsHtml.js b/src/webview/html/messageAsHtml.js index b11b48b572f..09de4c8299e 100644 --- a/src/webview/html/messageAsHtml.js +++ b/src/webview/html/messageAsHtml.js @@ -10,6 +10,7 @@ import type { Outbox, Reaction, ImageEmojiType, + UserId, } from '../../types'; import type { BackgroundData } from '../MessageList'; import { shortTime } from '../../utils/date'; @@ -44,7 +45,7 @@ const messageReactionAsHtml = ( const messageReactionListAsHtml = ( reactions: $ReadOnlyArray, - ownUserId: number, + ownUserId: UserId, allImageEmojiById: $ReadOnly<{ [id: string]: ImageEmojiType }>, ): string => { if (reactions.length === 0) { From 15ab4d8d8ae88bdd40ed10281da57f1a167cb7fa Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:21:20 -0800 Subject: [PATCH 13/24] user types: Use UserId in initial PM-conversations data. This change in the API types is tied together with a small change in our code that consumes it, for a bit of a silly reason: we want to sort this array, and we'd rather do so in place rather than making an unnecessary copy, and that means it can't be `$ReadOnlyArray`. In principle this could be avoided by Flow having something intermediate between plain `Array` and `$ReadOnlyArray`, with a name like `$CovariantArray`. Like `$ReadOnlyArray`, this would exclude methods like `push` that accept a new item, and that therefore aren't safe to use on an array that some other code might expect to have a more specific type of element. But unlike `$ReadOnlyArray`, it would include `sort`, and `pop`, and other methods that mutate the array but only provide elements as output, never taking them as input, and therefore are just fine for that situation. But Flow doesn't currently have that feature, so just treat this type covariantly and update both the provider and consumers in lockstep. --- src/api/modelTypes.js | 2 +- src/pm-conversations/pmConversationsModel.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index d6d3fa4ca56..0ca9449d747 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -606,5 +606,5 @@ export type RecentPrivateConversation = {| max_message_id: number, // When received from the server, these are guaranteed to be sorted only after // 2.2-dev-53-g405a529340. To be safe, we always sort them on receipt. - user_ids: number[], + user_ids: UserId[], |}; diff --git a/src/pm-conversations/pmConversationsModel.js b/src/pm-conversations/pmConversationsModel.js index 5a4c3b451d4..2cd1c708b5b 100644 --- a/src/pm-conversations/pmConversationsModel.js +++ b/src/pm-conversations/pmConversationsModel.js @@ -33,12 +33,12 @@ export opaque type PmConversationKey = string; /** PRIVATE. Exported only for tests. */ // Input must have the exact right (multi-)set of users. Needn't be sorted. -export function keyOfExactUsers(ids: number[]): PmConversationKey { +export function keyOfExactUsers(ids: UserId[]): PmConversationKey { return ids.sort((a, b) => a - b).join(','); } // Input may contain self or not, and needn't be sorted. -function keyOfUsers(ids: number[], ownUserId: UserId): PmConversationKey { +function keyOfUsers(ids: UserId[], ownUserId: UserId): PmConversationKey { return keyOfExactUsers(ids.filter(id => id !== ownUserId)); } From 78cdd5eef84131f69eb99d1f4a773367a938331e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:10:21 -0800 Subject: [PATCH 14/24] user types: Use UserId in user-status data. We do this only partially, due to a Flow bug in the interaction of indexer types (i.e. the types of objects-as-maps) and opaque types. --- src/api/eventTypes.js | 2 +- src/api/modelTypes.js | 5 +++++ src/user-status/userStatusReducer.js | 6 +++++- src/user-status/userStatusSelectors.js | 4 ++-- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index 92dac6a1938..2323f21bbbb 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -103,7 +103,7 @@ export type PresenceEvent = {| export type UserStatusEvent = {| ...EventCommon, type: typeof EventTypes.user_status, - user_id: number, + user_id: UserId, away?: boolean, status_text?: string, |}; diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 0ca9449d747..99ca54af57c 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -205,6 +205,11 @@ export type UserStatus = {| |}; export type UserStatusMapObject = {| + // TODO(flow): The key here is really UserId, not just any number; but + // this Flow bug: + // https://github.com/facebook/flow/issues/5407 + // means that doesn't work right, and the best workaround is to + // leave it as `number`. [userId: number]: UserStatus, |}; diff --git a/src/user-status/userStatusReducer.js b/src/user-status/userStatusReducer.js index bd32221bb43..48cb57d4ca8 100644 --- a/src/user-status/userStatusReducer.js +++ b/src/user-status/userStatusReducer.js @@ -39,7 +39,11 @@ export default (state: UserStatusState = initialState, action: Action): UserStat } return { ...state, - [action.user_id]: newUserStatus, + // TODO(flow): The cast here is because we've left this data + // structure's type with plain `number` for the key, to work + // around a Flow bug. See the definition of the type + // `UserStatusMapObject`. + [(action.user_id: number)]: newUserStatus, }; } diff --git a/src/user-status/userStatusSelectors.js b/src/user-status/userStatusSelectors.js index 9a27f1a4e5e..01bd77140ea 100644 --- a/src/user-status/userStatusSelectors.js +++ b/src/user-status/userStatusSelectors.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import type { GlobalState, Selector, UserStatus } from '../types'; +import type { GlobalState, Selector, UserId, UserStatus } from '../types'; import { getUserStatus } from '../directSelectors'; import { getSelfUserDetail } from '../users/userSelectors'; @@ -38,7 +38,7 @@ export const getSelfUserStatusText = (state: GlobalState): string => { * Returns the `status text` value of the user with the given userId. * We return `undefined` if no value is set. */ -export const getUserStatusTextForUser = (state: GlobalState, userId: number): string | void => { +export const getUserStatusTextForUser = (state: GlobalState, userId: UserId): string | void => { const userStatus = getUserStatus(state); return userStatus[userId] && userStatus[userId].status_text; }; From 109bc35e40bfe0123935e80c704bcfbe4286f0cb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:11:09 -0800 Subject: [PATCH 15/24] user types: Use UserId for unreads data. We reuse this PmsUnreadItem type for the data structures we maintain over time for this data, so tightening its type requires us to adjust a type in the code that maintains it. --- src/api/initialDataTypes.js | 2 +- src/unread/unreadHelpers.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index c807df3d55d..62686e294c7 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -227,7 +227,7 @@ export type PmsUnreadItem = {| * the normal thing even then would be to make a bot user to send the * messages as.) See server commit ca74cd6e3. */ - sender_id: number, + sender_id: UserId, // Sorted. unread_message_ids: number[], diff --git a/src/unread/unreadHelpers.js b/src/unread/unreadHelpers.js index c62518e0699..8ffbdb0c14f 100644 --- a/src/unread/unreadHelpers.js +++ b/src/unread/unreadHelpers.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import type { HuddlesUnreadItem, PmsUnreadItem, StreamUnreadItem } from '../types'; +import type { HuddlesUnreadItem, PmsUnreadItem, StreamUnreadItem, UserId } from '../types'; import { addItemsToArray, removeItemsFromArray, filterArray } from '../utils/immutability'; type SomeUnreadItem = { unread_message_ids: number[] }; @@ -50,7 +50,7 @@ function addItemsDeeply(input: T[], itemsToAdd: number[], ind export const addItemsToPmArray = ( input: PmsUnreadItem[], itemsToAdd: number[], - senderId: number, + senderId: UserId, ): PmsUnreadItem[] => { const index = input.findIndex(sender => sender.sender_id === senderId); From b3b2599eb9daea91adf131094daa26e53e6251f0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Jan 2021 00:41:28 -0800 Subject: [PATCH 16/24] user types: Use UserId for typing-status state. --- src/reduxTypes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 9db65d319a7..ad36c7e3944 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -293,7 +293,7 @@ export type TopicsState = {| export type TypingState = { [normalizedRecipients: string]: { time: number, - userIds: number[], + userIds: UserId[], }, }; From b950e2504174dcbcaf99113460d5cc05b46c2cf2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:27:34 -0800 Subject: [PATCH 17/24] user types: Use UserId in MentionWarnings component. This makes the meaning of the `unsubscribedMentions` state-property rather clearer! The elements are user IDs. --- src/compose/MentionWarnings.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/compose/MentionWarnings.js b/src/compose/MentionWarnings.js index bbc9b3010fd..08ec4d244ec 100644 --- a/src/compose/MentionWarnings.js +++ b/src/compose/MentionWarnings.js @@ -3,7 +3,16 @@ import React, { PureComponent } from 'react'; import { connect } from 'react-redux'; -import type { Auth, Stream, Dispatch, Narrow, UserOrBot, Subscription, GetText } from '../types'; +import type { + Auth, + Stream, + Dispatch, + Narrow, + UserOrBot, + Subscription, + GetText, + UserId, +} from '../types'; import { TranslationContext } from '../boot/TranslationProvider'; import { getActiveUsersById, getAuth } from '../selectors'; import { is1to1PmNarrow } from '../utils/narrow'; @@ -14,12 +23,12 @@ import MentionedUserNotSubscribed from '../message/MentionedUserNotSubscribed'; import { makeUserId } from '../api/idTypes'; type State = {| - unsubscribedMentions: Array, + unsubscribedMentions: Array, |}; type SelectorProps = {| auth: Auth, - usersById: Map, + usersById: Map, |}; type Props = $ReadOnly<{| @@ -124,9 +133,7 @@ class MentionWarnings extends PureComponent { handleMentionWarningDismiss = (user: UserOrBot) => { this.setState(prevState => ({ - unsubscribedMentions: prevState.unsubscribedMentions.filter( - (x: number) => x !== user.user_id, - ), + unsubscribedMentions: prevState.unsubscribedMentions.filter(x => x !== user.user_id), })); }; From 91b857204f6de6f9d7d46e4ee59497319ef8b286 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Sat, 12 Dec 2020 02:48:32 -0800 Subject: [PATCH 18/24] user types: Switch to UserId in most React props and state. All these changes are pretty boring. Many of them are independent of each other, but others aren't because one component passes a user ID to another, either as a child or by navigation. This change is limited to the places where a user ID, or list of them, appears in order to specify particular users that this component, this time, should do its work with respect to. Upcoming commits will take care of places where we pass general information that happens to contain user IDs. --- src/common/UserAvatarWithPresence.js | 4 ++-- src/nav/AppNavigator.js | 6 +++--- src/nav/navActions.js | 6 +++--- src/reactions/ReactionUserList.js | 4 ++-- src/sharing/ChooseRecipientsScreen.js | 4 ++-- src/sharing/ShareToPm.js | 4 ++-- src/title-buttons/InfoNavButtonGroup.js | 4 ++-- src/title-buttons/InfoNavButtonPrivate.js | 3 ++- src/title/TitleGroup.js | 3 ++- src/title/TitlePrivate.js | 3 ++- src/users/UserItem.js | 4 ++-- 11 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index df8c5a9d65e..a872917079c 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -1,8 +1,8 @@ /* @flow strict-local */ import React, { type ComponentType, PureComponent } from 'react'; +import type { Dispatch, UserId } from '../types'; import { createStyleSheet } from '../styles'; -import type { Dispatch } from '../types'; import UserAvatar from './UserAvatar'; import PresenceStatusIndicator from './PresenceStatusIndicator'; import { AvatarURL } from '../utils/avatar'; @@ -83,6 +83,6 @@ export const UserAvatarWithPresenceById = connect<{| avatarUrl: AvatarURL, email // first place, but here we have to provide it explicitly. /* eslint-disable flowtype/generic-spacing */ (UserAvatarWithPresence: ComponentType< - $ReadOnly<{| ...Props, dispatch: Dispatch, userId: number |}>, + $ReadOnly<{| ...Props, dispatch: Dispatch, userId: UserId |}>, >), ); diff --git a/src/nav/AppNavigator.js b/src/nav/AppNavigator.js index 540ad9c9f5b..25ec12c42e1 100644 --- a/src/nav/AppNavigator.js +++ b/src/nav/AppNavigator.js @@ -12,7 +12,7 @@ import { useSelector } from '../react-redux'; import { hasAuth as getHasAuth, getAccounts, getHaveServerData } from '../selectors'; import getInitialRouteInfo from './getInitialRouteInfo'; import type { GlobalParamList } from './globalTypes'; -import type { Narrow, Message, SharedData } from '../types'; +import type { Narrow, Message, SharedData, UserId } from '../types'; import type { ApiResponseServerSettings } from '../api/settings/getServerSettings'; import AccountPickScreen from '../account/AccountPickScreen'; import RealmScreen from '../start/RealmScreen'; @@ -48,8 +48,8 @@ import SharingScreen from '../sharing/SharingScreen'; export type AppNavigatorParamList = {| account: void, - 'account-details': {| userId: number |}, - 'group-details': {| recipients: $ReadOnlyArray |}, + 'account-details': {| userId: UserId |}, + 'group-details': {| recipients: $ReadOnlyArray |}, auth: {| serverSettings: ApiResponseServerSettings |}, chat: {| narrow: Narrow |}, dev: void, diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 4e0f136c817..015fcfc35f6 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -5,7 +5,7 @@ import { type GenericNavigationAction, } from '@react-navigation/native'; -import type { Message, Narrow, SharedData } from '../types'; +import type { Message, Narrow, SharedData, UserId } from '../types'; import type { ApiResponseServerSettings } from '../api/settings/getServerSettings'; import { getSameRoutesCount } from '../selectors'; @@ -53,11 +53,11 @@ export const navigateToPassword = (requireEmailFormat: boolean): GenericNavigati export const navigateToAccountPicker = (): GenericNavigationAction => StackActions.push('account'); -export const navigateToAccountDetails = (userId: number): GenericNavigationAction => +export const navigateToAccountDetails = (userId: UserId): GenericNavigationAction => StackActions.push('account-details', { userId }); export const navigateToGroupDetails = ( - recipients: $ReadOnlyArray, + recipients: $ReadOnlyArray, ): GenericNavigationAction => StackActions.push('group-details', { recipients }); export const navigateToRealmScreen = ( diff --git a/src/reactions/ReactionUserList.js b/src/reactions/ReactionUserList.js index 4205d6139d7..f493b135016 100644 --- a/src/reactions/ReactionUserList.js +++ b/src/reactions/ReactionUserList.js @@ -3,12 +3,12 @@ import React from 'react'; import { FlatList } from 'react-native'; import * as NavigationService from '../nav/NavigationService'; -import type { UserOrBot } from '../types'; +import type { UserId, UserOrBot } from '../types'; import UserItem from '../users/UserItem'; import { navigateToAccountDetails } from '../actions'; type Props = $ReadOnly<{| - reactedUserIds: $ReadOnlyArray, + reactedUserIds: $ReadOnlyArray, |}>; /** diff --git a/src/sharing/ChooseRecipientsScreen.js b/src/sharing/ChooseRecipientsScreen.js index 32ed51bf386..826d8685fd9 100644 --- a/src/sharing/ChooseRecipientsScreen.js +++ b/src/sharing/ChooseRecipientsScreen.js @@ -1,13 +1,13 @@ /* @flow strict-local */ import React, { PureComponent } from 'react'; -import type { User, Dispatch } from '../types'; +import type { User, Dispatch, UserId } from '../types'; import { connect } from '../react-redux'; import { Screen } from '../common'; import UserPickerCard from '../user-picker/UserPickerCard'; type Props = $ReadOnly<{| dispatch: Dispatch, - onComplete: ($ReadOnlyArray) => void, + onComplete: ($ReadOnlyArray) => void, |}>; type State = {| diff --git a/src/sharing/ShareToPm.js b/src/sharing/ShareToPm.js index 815b436df6e..45c39469894 100644 --- a/src/sharing/ShareToPm.js +++ b/src/sharing/ShareToPm.js @@ -4,7 +4,7 @@ import { View, Image, ScrollView, Modal, BackHandler } from 'react-native'; import type { SharingNavigationProp, SharingRouteProp } from './SharingScreen'; import * as NavigationService from '../nav/NavigationService'; -import type { Dispatch, Auth, GetText } from '../types'; +import type { Dispatch, Auth, GetText, UserId } from '../types'; import { createStyleSheet } from '../styles'; import { TranslationContext } from '../boot/TranslationProvider'; import { connect } from '../react-redux'; @@ -63,7 +63,7 @@ type Props = $ReadOnly<{| |}>; type State = $ReadOnly<{| - selectedRecipients: $ReadOnlyArray, + selectedRecipients: $ReadOnlyArray, message: string, choosingRecipients: boolean, sending: boolean, diff --git a/src/title-buttons/InfoNavButtonGroup.js b/src/title-buttons/InfoNavButtonGroup.js index 2afba05d6a8..0cc5190b3f8 100644 --- a/src/title-buttons/InfoNavButtonGroup.js +++ b/src/title-buttons/InfoNavButtonGroup.js @@ -1,14 +1,14 @@ /* @flow strict-local */ - import React from 'react'; +import type { UserId } from '../types'; import * as NavigationService from '../nav/NavigationService'; import NavButton from '../nav/NavButton'; import { navigateToGroupDetails } from '../actions'; type Props = $ReadOnly<{| color: string, - userIds: $ReadOnlyArray, + userIds: $ReadOnlyArray, |}>; export default function InfoNavButtonGroup(props: Props) { diff --git a/src/title-buttons/InfoNavButtonPrivate.js b/src/title-buttons/InfoNavButtonPrivate.js index 79148121597..0a12f36965f 100644 --- a/src/title-buttons/InfoNavButtonPrivate.js +++ b/src/title-buttons/InfoNavButtonPrivate.js @@ -1,13 +1,14 @@ /* @flow strict-local */ import React from 'react'; +import type { UserId } from '../types'; import * as NavigationService from '../nav/NavigationService'; import NavButton from '../nav/NavButton'; import { navigateToAccountDetails } from '../actions'; type Props = $ReadOnly<{| color: string, - userId: number, + userId: UserId, |}>; export default function InfoNavButtonPrivate(props: Props) { diff --git a/src/title/TitleGroup.js b/src/title/TitleGroup.js index 0ef3c5a80f2..79bf1a86f9c 100644 --- a/src/title/TitleGroup.js +++ b/src/title/TitleGroup.js @@ -3,13 +3,14 @@ import React from 'react'; import { View } from 'react-native'; +import type { UserId } from '../types'; import * as NavigationService from '../nav/NavigationService'; import styles, { createStyleSheet } from '../styles'; import { UserAvatarWithPresenceById } from '../common/UserAvatarWithPresence'; import { navigateToAccountDetails } from '../nav/navActions'; type Props = $ReadOnly<{| - userIds: $ReadOnlyArray, + userIds: $ReadOnlyArray, |}>; const componentStyles = createStyleSheet({ diff --git a/src/title/TitlePrivate.js b/src/title/TitlePrivate.js index f46dd9f3549..f471a439902 100644 --- a/src/title/TitlePrivate.js +++ b/src/title/TitlePrivate.js @@ -4,6 +4,7 @@ import React from 'react'; import { Text, View } from 'react-native'; import * as NavigationService from '../nav/NavigationService'; +import type { UserId } from '../types'; import styles, { createStyleSheet } from '../styles'; import { useSelector } from '../react-redux'; import { Touchable, ViewPlaceholder } from '../common'; @@ -13,7 +14,7 @@ import { getAllUsersById } from '../users/userSelectors'; import { navigateToAccountDetails } from '../nav/navActions'; type Props = $ReadOnly<{ - userId: number, + userId: UserId, color: string, }>; diff --git a/src/users/UserItem.js b/src/users/UserItem.js index 263a60bf571..7bab11637a4 100644 --- a/src/users/UserItem.js +++ b/src/users/UserItem.js @@ -2,7 +2,7 @@ import React, { type ComponentType, type ElementConfig, PureComponent } from 'react'; import { View } from 'react-native'; -import type { UserOrBot } from '../types'; +import type { UserId, UserOrBot } from '../types'; import { RawLabel, Touchable, UnreadCount } from '../common'; import { UserAvatarWithPresenceById } from '../common/UserAvatarWithPresence'; import styles, { createStyleSheet, BRAND_COLOR } from '../styles'; @@ -102,7 +102,7 @@ export const UserItemRaw = (UserItem: ComponentType<$Exact, { user: mixed }>>, - userId: number, + userId: UserId, |}>; /** From 9f53018f59f049c9078474c5509f3832350c5933 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Jan 2021 12:33:40 -0800 Subject: [PATCH 19/24] user types: Use UserId in pmKeyRecipientsFromIds. --- src/utils/recipient.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/recipient.js b/src/utils/recipient.js index d6fa2c7d835..84c41a844d3 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -200,7 +200,7 @@ export const pmKeyRecipientsFromMessage = ( * Returns null when a user couldn't be found in the given `allUsersById`. */ export const pmKeyRecipientsFromIds = ( - userIds: $ReadOnlyArray, + userIds: $ReadOnlyArray, allUsersById: Map, ownUserId: UserId, ): PmKeyUsers | null => { From 32197105a82958d98699246209b3a536dda8d785 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:38:46 -0800 Subject: [PATCH 20/24] user types: Propagate UserId type through outbound typing-status. The data we're passing to this code comes from PM recipients, where we've already marked the user IDs as type `UserId`. So when we tighten the type here, Flow already accepts that. This in turn causes Flow to see that this code's local helper `getRecipients` operates on `UserId`s. It passes the user IDs to one of our Redux selectors; so in an upcoming commit, when we tighten those selectors to expect `UserId`, Flow will see that this change is in fact required. --- src/users/usersActions.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/users/usersActions.js b/src/users/usersActions.js index 9a66cfb5b60..66758e7df76 100644 --- a/src/users/usersActions.js +++ b/src/users/usersActions.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import * as typing_status from '@zulip/shared/js/typing_status'; -import type { Auth, Dispatch, GetState, GlobalState, Narrow } from '../types'; +import type { Auth, Dispatch, GetState, GlobalState, Narrow, UserId } from '../types'; import * as api from '../api'; import { PRESENCE_RESPONSE } from '../actionConstants'; import { getAuth, tryGetAuth, getServerVersion } from '../selectors'; @@ -46,11 +46,11 @@ const typingWorker = (state: GlobalState) => { return { get_current_time: () => new Date().getTime(), - notify_server_start: (user_ids_array: $ReadOnlyArray) => { + notify_server_start: (user_ids_array: $ReadOnlyArray) => { api.typing(auth, getRecipients(user_ids_array), 'start'); }, - notify_server_stop: (user_ids_array: $ReadOnlyArray) => { + notify_server_stop: (user_ids_array: $ReadOnlyArray) => { api.typing(auth, getRecipients(user_ids_array), 'stop'); }, }; From 9d12a961e42de67cb339addce149a1417a58998a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Jan 2021 12:35:06 -0800 Subject: [PATCH 21/24] user types: Consume UserId in two user-data selectors. This comes after converting React props and route props, which provide the arguments to these selectors at most call sites, and before converting the `get*UsersById` selectors which provide the data structures these look up data in. --- src/users/userSelectors.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index ab33a261afd..f145b01ff5e 100644 --- a/src/users/userSelectors.js +++ b/src/users/userSelectors.js @@ -176,7 +176,7 @@ export const getActiveUsersById: Selector> = createSelect * * Throws if no such user exists. */ -export const getUserForId = (state: GlobalState, userId: number): UserOrBot => { +export const getUserForId = (state: GlobalState, userId: UserId): UserOrBot => { const user = getAllUsersById(state).get(userId); if (!user) { throw new Error(`getUserForId: missing user: id ${userId}`); @@ -197,7 +197,7 @@ export const getUserForId = (state: GlobalState, userId: number): UserOrBot => { */ // To understand this implementation, see the comment about `is_active` in // the `User` type definition. -export const getUserIsActive = (state: GlobalState, userId: number): boolean => +export const getUserIsActive = (state: GlobalState, userId: UserId): boolean => !!getActiveUsersById(state).get(userId); /** From a8b3a211434a94f88073442b721309ade8be0b15 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Jan 2021 01:31:01 -0800 Subject: [PATCH 22/24] user types: Switch to UserId in getAllUsersById and friends. In this commit, we update the selectors that return maps of users by ID to mark the keys as being specifically user IDs, not just any numbers; and we propagate that change through all the values that just pass on these selectors' return values. --- src/compose/ComposeBox.js | 2 +- src/compose/getComposeInputPlaceholder.js | 2 +- src/notification/__tests__/notification-test.js | 8 ++++---- src/notification/index.js | 2 +- src/outbox/outboxActions.js | 13 +++++++++++-- src/users/userSelectors.js | 6 +++--- src/utils/__tests__/internalLinks-test.js | 4 ++-- src/utils/internalLinks.js | 2 +- src/utils/narrow.js | 2 +- src/utils/recipient.js | 4 ++-- 10 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index c1893c084fc..b84c68e616e 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -62,7 +62,7 @@ import { getActiveUsersById, getOwnUserId } from '../users/userSelectors'; type SelectorProps = {| auth: Auth, ownUserId: UserId, - usersById: Map, + usersById: Map, isAdmin: boolean, isAnnouncementOnly: boolean, isSubscribed: boolean, diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index 4aed77acbbb..9fcab3c46bf 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -5,7 +5,7 @@ import { caseNarrowDefault } from '../utils/narrow'; export default ( narrow: Narrow, ownUserId: UserId, - usersById: Map, + usersById: Map, ): LocalizableText => caseNarrowDefault( narrow, diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 2dd72b7f7dc..6aa546c3844 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -2,7 +2,7 @@ import deepFreeze from 'deep-freeze'; import type { Notification } from '../types'; -import type { UserOrBot } from '../../api/modelTypes'; +import type { UserId, UserOrBot } from '../../api/modelTypes'; import type { JSONableDict } from '../../utils/jsonable'; import { getNarrowFromNotificationData } from '..'; import { topicNarrow, pm1to1NarrowFromUser, pmNarrowFromUsersUnsafe } from '../../utils/narrow'; @@ -12,7 +12,7 @@ import { fromAPNsImpl as extractIosNotificationData } from '../extract'; import objectEntries from '../../utils/objectEntries'; describe('getNarrowFromNotificationData', () => { - const DEFAULT_MAP = new Map(); + const DEFAULT_MAP = new Map(); const ownUserId = eg.selfUser.user_id; test('unknown notification data returns null', () => { @@ -34,7 +34,7 @@ describe('getNarrowFromNotificationData', () => { test('on notification for a private message returns a PM narrow', () => { const users = [eg.selfUser, eg.otherUser]; - const allUsersById: Map = new Map(users.map(u => [u.user_id, u])); + const allUsersById: Map = new Map(users.map(u => [u.user_id, u])); const allUsersByEmail: Map = new Map(users.map(u => [u.email, u])); const notification = { recipient_type: 'private', @@ -51,7 +51,7 @@ describe('getNarrowFromNotificationData', () => { test('on notification for a group message returns a group narrow', () => { const users = [eg.selfUser, eg.makeUser(), eg.makeUser(), eg.makeUser()]; - const allUsersById: Map = new Map(users.map(u => [u.user_id, u])); + const allUsersById: Map = new Map(users.map(u => [u.user_id, u])); const allUsersByEmail: Map = new Map(users.map(u => [u.email, u])); const notification = { diff --git a/src/notification/index.js b/src/notification/index.js index 4ec4e23d34c..d91528fb1f1 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -86,7 +86,7 @@ export const getAccountFromNotificationData = ( export const getNarrowFromNotificationData = ( data: Notification, - allUsersById: Map, + allUsersById: Map, allUsersByEmail: Map, ownUserId: UserId, ): Narrow | null => { diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index dde0cf423e6..474febaf166 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -2,7 +2,16 @@ import parseMarkdown from 'zulip-markdown-parser'; import * as logging from '../utils/logging'; -import type { Dispatch, GetState, GlobalState, Narrow, Outbox, UserOrBot, Action } from '../types'; +import type { + Dispatch, + GetState, + GlobalState, + Narrow, + Outbox, + UserOrBot, + UserId, + Action, +} from '../types'; import type { SubsetProperties } from '../generics'; import { MESSAGE_SEND_START, @@ -120,7 +129,7 @@ type DataFromNarrow = SubsetProperties< const extractTypeToAndSubjectFromNarrow = ( narrow: Narrow, - allUsersById: Map, + allUsersById: Map, ownUser: UserOrBot, ): DataFromNarrow => caseNarrowPartial(narrow, { diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index f145b01ff5e..280c8fca581 100644 --- a/src/users/userSelectors.js +++ b/src/users/userSelectors.js @@ -36,7 +36,7 @@ const getAllUsers: Selector = createSelector( ); /** See `getAllUsers` for discussion. */ -export const getAllUsersById: Selector> = createSelector( +export const getAllUsersById: Selector> = createSelector( getAllUsers, allUsers => new Map(allUsers.map(user => [user.user_id, user])), ); @@ -57,7 +57,7 @@ export const getAllUsersByEmail: Selector> = createSelect * * See `getAllUsersById`, and `getAllUsers` for discussion. */ -export const getUsersById: Selector> = createSelector( +export const getUsersById: Selector> = createSelector( getUsers, (users = []) => new Map(users.map(user => [user.user_id, user])), ); @@ -161,7 +161,7 @@ export const getUsersSansMe: Selector = createSelector( ); /** Excludes deactivated users. See `getAllUsers` for discussion. */ -export const getActiveUsersById: Selector> = createSelector( +export const getActiveUsersById: Selector> = createSelector( getUsers, getCrossRealmBots, (users = [], crossRealmBots = []) => diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 49a7cbf660e..530458fd4d6 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -1,6 +1,6 @@ /* @flow strict-local */ -import type { UserOrBot } from '../../api/modelTypes'; +import type { UserId, UserOrBot } from '../../types'; import { streamNarrow, topicNarrow, pmNarrowFromUsersUnsafe, STARRED_NARROW } from '../narrow'; import { isInternalLink, @@ -252,7 +252,7 @@ describe('decodeHashComponent', () => { describe('getNarrowFromLink', () => { const [userB, userC] = [eg.makeUser(), eg.makeUser()]; - const allUsersById: Map = new Map( + const allUsersById: Map = new Map( [eg.selfUser, userB, userC].map(u => [u.user_id, u]), ); diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 34cf8e0488b..81ef8ca01d2 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -153,7 +153,7 @@ const parsePmOperand = operand => { export const getNarrowFromLink = ( url: string, realm: URL, - allUsersById: Map, + allUsersById: Map, streamsById: Map, ownUserId: UserId, ): Narrow | null => { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 91be48decad..736d053e45c 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -392,7 +392,7 @@ export const isSearchNarrow = (narrow?: Narrow): boolean => */ export const apiNarrowOfNarrow = ( narrow: Narrow, - allUsersById: Map, + allUsersById: Map, ): ApiNarrow => caseNarrow(narrow, { stream: streamName => [{ operator: 'stream', operand: streamName }], diff --git a/src/utils/recipient.js b/src/utils/recipient.js index 84c41a844d3..49e9816bae3 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -201,7 +201,7 @@ export const pmKeyRecipientsFromMessage = ( */ export const pmKeyRecipientsFromIds = ( userIds: $ReadOnlyArray, - allUsersById: Map, + allUsersById: Map, ownUserId: UserId, ): PmKeyUsers | null => { const resultIds = userIds.filter(id => id !== ownUserId); @@ -222,7 +222,7 @@ export const pmKeyRecipientsFromIds = ( */ export const pmKeyRecipientUsersFromMessage = ( message: Message | Outbox, - allUsersById: Map, + allUsersById: Map, ownUserId: UserId, ): PmKeyUsers | null => { const userIds = recipientsOfPrivateMessage(message).map(r => r.id); From 7fb49af75514083937efbf38cb9cbe2b81bfa01d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 20 Jan 2021 01:02:01 -0800 Subject: [PATCH 23/24] user types: Use UserId for sending a PM, in sharing code. --- src/sharing/send.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sharing/send.js b/src/sharing/send.js index fb68e65358c..b31667c3659 100644 --- a/src/sharing/send.js +++ b/src/sharing/send.js @@ -1,6 +1,6 @@ /* @flow strict-local */ -import type { SharedData, Auth, GetText } from '../types'; +import type { SharedData, Auth, GetText, UserId } from '../types'; import { showToast } from '../utils/info'; import { sendMessage, uploadFile } from '../api'; @@ -13,7 +13,7 @@ type SendStream = {| |}; type SendPm = {| - selectedRecipients: $ReadOnlyArray, + selectedRecipients: $ReadOnlyArray, message: string, sharedData: SharedData, type: 'pm', From 1680f7b76e1dcec075fe6c1a7cd34f454b3718dc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 21:16:41 -0800 Subject: [PATCH 24/24] api types: Require UserId on input to the API, completing the conversion. This takes us full circle from the early part of this series, where we started using UserId where the API provides a user ID. After this change, a search like `rg -i user.*number`, or similar searches with "recipient" or "sender" instead of "user", turns up only a handful of places where we have something intended to be a user ID that's still of type `number`; and each of them has a comment explaining why. --- src/api/modelTypes.js | 6 +++--- src/api/subscriptions/getSubscriptionToStream.js | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 99ca54af57c..b3cd66efc65 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -315,9 +315,9 @@ export type NarrowElement = // * `sender` since 2.1-dev-1812-gc067c155a // * `pm-with` since 2.1-dev-1350-gd7b4de234 | {| +operator: 'stream', +operand: string | number |} // stream ID - | {| +operator: 'pm-with', +operand: string | $ReadOnlyArray |} // user IDs - | {| +operator: 'sender', +operand: string | number |} // user ID - | {| +operator: 'group-pm-with', +operand: string | number |} // user ID + | {| +operator: 'pm-with', +operand: string | $ReadOnlyArray |} + | {| +operator: 'sender', +operand: string | UserId |} + | {| +operator: 'group-pm-with', +operand: string | UserId |} | {| +operator: 'near' | 'id', +operand: number |} // message ID ; diff --git a/src/api/subscriptions/getSubscriptionToStream.js b/src/api/subscriptions/getSubscriptionToStream.js index 1c521d58f7b..8c906531703 100644 --- a/src/api/subscriptions/getSubscriptionToStream.js +++ b/src/api/subscriptions/getSubscriptionToStream.js @@ -1,5 +1,6 @@ /* @flow strict-local */ import type { Auth, ApiResponseSuccess } from '../transportTypes'; +import { type UserId } from '../idTypes'; import { apiGet } from '../apiFetch'; type ApiResponseSubscriptionStatus = {| @@ -15,7 +16,7 @@ type ApiResponseSubscriptionStatus = {| */ export default ( auth: Auth, - userId: number, + userId: UserId, streamId: number, ): Promise => apiGet(auth, `users/${userId}/subscriptions/${streamId}`);