From deddc5843ee64944679b9696e7dc5799165f7a30 Mon Sep 17 00:00:00 2001 From: Isham Mahajan Date: Sun, 30 Jun 2019 13:29:18 +0530 Subject: [PATCH 1/7] Add `recent_private_conversations` key in `register` endpoint. The server added in zulip/zulip#11944 basic support for fetching the most recent private conversations that a user was involved in. The endpoint returns the private conversations a user has had within the past 1000 private messages they have sent/recieved. The endpoint returns data in the form of `[{user_ids, max_message_id}]` where user_ids lists all the people involved in the conversation, and max_message_id returns the message_id of the most recent message in the conversation. This commit introduces the reducers required to introduce this data into the Redux state, during `doInitialFetch`. It only catches the `REALM_INIT` action at the moment, and the rest of the actions will be added in a later commit with details. --- src/__tests__/lib/exampleData.js | 1 + src/api/initialDataTypes.js | 6 +++++ src/api/modelTypes.js | 23 +++++++++++++++++++ src/boot/reducers.js | 2 ++ src/boot/store.js | 6 ++--- src/config.js | 1 + src/directSelectors.js | 4 ++++ .../recentPmConversationsReducer.js | 18 +++++++++++++++ src/reduxTypes.js | 9 ++++++++ 9 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 src/pm-conversations/recentPmConversationsReducer.js diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 3abb814e224..9db6068ab5e 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -337,6 +337,7 @@ export const action = deepFreeze({ realm_users: [], user_id: 4, realm_user_groups: [], + recent_private_conversations: [], streams: [], never_subscribed: [], subscriptions: [], diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 4ec53caa21b..e24b5a03639 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -4,6 +4,7 @@ import type { CrossRealmBot, RealmEmojiById, RealmFilter, + RecentPrivateConversation, Stream, Subscription, User, @@ -107,6 +108,10 @@ export type InitialDataRealmUserGroups = {| realm_user_groups: UserGroup[], |}; +export type InitialDataRecentPmConversations = {| + recent_private_conversations: RecentPrivateConversation[], +|}; + type NeverSubscribedStream = {| description: string, invite_only: boolean, @@ -273,6 +278,7 @@ export type InitialData = {| ...InitialDataRealmFilters, ...InitialDataRealmUser, ...InitialDataRealmUserGroups, + ...InitialDataRecentPmConversations, ...InitialDataStream, ...InitialDataSubscription, ...InitialDataUpdateDisplaySettings, diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index ac0eec32c3e..960000f2ea0 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -536,3 +536,26 @@ export type Message = $ReadOnly<{ subject: string, subject_links: $ReadOnlyArray, }>; + +// +// +// +// =================================================================== +// Summaries of messages and conversations. +// +// + +/** + * Describes a recent PM conversation. + * + * See https://github.com/zulip/zulip/commit/4c3c669b4#diff-2c2aa234cb4a0120fa5f2eeaf8a94fa2R283 + * for the structure of this type and the meaning of its properties. + * + * `user_ids` does not contain the `user_id` of the current user. Consequently, + * a user's conversation with themselves will return [], which is unlike the + * behaviour found in other parts of codebase. + */ +export type RecentPrivateConversation = {| + max_message_id: number, + user_ids: number[], +|}; diff --git a/src/boot/reducers.js b/src/boot/reducers.js index a112ef84259..f418072e511 100644 --- a/src/boot/reducers.js +++ b/src/boot/reducers.js @@ -21,6 +21,7 @@ import nav from '../nav/navReducer'; import outbox from '../outbox/outboxReducer'; import presence from '../presence/presenceReducer'; import realm from '../realm/realmReducer'; +import recentPrivateConversations from '../pm-conversations/recentPmConversationsReducer'; import session from '../session/sessionReducer'; import settings from '../settings/settingsReducer'; import streams from '../streams/streamsReducer'; @@ -50,6 +51,7 @@ const reducers = { outbox, presence, realm, + recentPrivateConversations, session, settings, streams, diff --git a/src/boot/store.js b/src/boot/store.js index db6dbda2d58..c3a9ac398a2 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -44,9 +44,9 @@ export const storeKeys: Array<$Keys> = [ * don't have to re-download it. */ // prettier-ignore -export const cacheKeys: Array<$Keys> = [ - 'flags', 'messages', 'mute', 'narrows', 'realm', 'streams', - 'subscriptions', 'unread', 'userGroups', 'users', +export const cacheKeys = [ + 'flags', 'messages', 'mute', 'narrows', 'realm', 'recentPrivateConversations', + 'streams', 'subscriptions', 'unread', 'userGroups', 'users', ]; /** diff --git a/src/config.js b/src/config.js index 1a893b21223..e260a14e358 100644 --- a/src/config.js +++ b/src/config.js @@ -32,6 +32,7 @@ const config: Config = { 'realm_filters', 'realm_user', 'realm_user_groups', + 'recent_private_conversations', 'stream', 'subscription', 'update_display_settings', diff --git a/src/directSelectors.js b/src/directSelectors.js index 7bc77e89e82..201d07695c7 100644 --- a/src/directSelectors.js +++ b/src/directSelectors.js @@ -23,6 +23,7 @@ import type { Subscription, Stream, Outbox, + RecentPrivateConversation, User, UserGroup, UserStatusState, @@ -83,6 +84,9 @@ export const getSubscriptions = (state: GlobalState): Subscription[] => state.su */ export const getStreams = (state: GlobalState): Stream[] => state.streams; +export const getRecentPrivateConversations = (state: GlobalState): RecentPrivateConversation[] => + state.recentPrivateConversations; + export const getPresence = (state: GlobalState): PresenceState => state.presence; export const getOutbox = (state: GlobalState): Outbox[] => state.outbox; diff --git a/src/pm-conversations/recentPmConversationsReducer.js b/src/pm-conversations/recentPmConversationsReducer.js new file mode 100644 index 00000000000..7f9d79e49cb --- /dev/null +++ b/src/pm-conversations/recentPmConversationsReducer.js @@ -0,0 +1,18 @@ +/* @flow strict-local */ +import type { Action, RecentPrivateConversationsState } from '../types'; +import { REALM_INIT } from '../actionConstants'; +import { NULL_ARRAY } from '../nullObjects'; + +const initialState: RecentPrivateConversationsState = NULL_ARRAY; + +export default ( + state: RecentPrivateConversationsState = initialState, + action: Action, +): RecentPrivateConversationsState => { + switch (action.type) { + case REALM_INIT: + return action.data.recent_private_conversations || initialState; + default: + return state; + } +}; diff --git a/src/reduxTypes.js b/src/reduxTypes.js index aba9d0e4f95..ac444ece2da 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -22,6 +22,7 @@ import type { RealmEmojiById, RealmFilter, Narrow, + RecentPrivateConversation, Stream, StreamUnreadItem, Subscription, @@ -251,6 +252,13 @@ export type RealmState = {| isAdmin: boolean, |}; +/** + * The PM conversations (group or 1:1) found in the last 1000 PMs. + * + * Sorted by `max_message_id` descending. + */ +export type RecentPrivateConversationsState = RecentPrivateConversation[]; + export type ThemeName = 'default' | 'night'; export type SettingsState = {| @@ -338,6 +346,7 @@ export type GlobalState = {| outbox: OutboxState, presence: PresenceState, realm: RealmState, + recentPrivateConversations: RecentPrivateConversationsState, session: SessionState, settings: SettingsState, streams: StreamsState, From fb22b7cb296087f70a6a09b25589c31d3dbec118 Mon Sep 17 00:00:00 2001 From: Isham Mahajan Date: Sun, 30 Jun 2019 18:05:20 +0530 Subject: [PATCH 2/7] recent pms reducer: Add `EVENT_NEW_MESSAGE`. Since the `PmConversationsCard` needs to be updated when a new message comes in and the data it uses to be updated is in `recentPrivateConversations` key in the Redux state, this event catch adds/amends the data in the state to correctly match the most recent private messages. A unit test file has been created for this use-case as well. It is `strict-local`, and uses `exampleData.js` for its data source. --- src/__tests__/lib/exampleData.js | 2 +- .../recentPmConversationsReducer-test.js | 35 +++++++++++++++++++ .../recentPmConversationsReducer.js | 27 +++++++++++++- 3 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 src/pm-conversations/__tests__/recentPmConversationsReducer-test.js diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 9db6068ab5e..00295a1882c 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -147,7 +147,7 @@ export const stream: Stream = makeStream({ description: 'An example stream.', }); -const displayRecipientFromUser = (user: User): PmRecipientUser => { +export const displayRecipientFromUser = (user: User): PmRecipientUser => { const { email, full_name, user_id: id } = user; return deepFreeze({ email, diff --git a/src/pm-conversations/__tests__/recentPmConversationsReducer-test.js b/src/pm-conversations/__tests__/recentPmConversationsReducer-test.js new file mode 100644 index 00000000000..eb3aa4fcf15 --- /dev/null +++ b/src/pm-conversations/__tests__/recentPmConversationsReducer-test.js @@ -0,0 +1,35 @@ +/* @flow strict-local */ +import deepFreeze from 'deep-freeze'; + +import recentPmConversationsReducer from '../recentPmConversationsReducer'; +import { EVENT_NEW_MESSAGE } from '../../actionConstants'; +import * as eg from '../../__tests__/lib/exampleData'; + +describe('recentPmConversationsReducer', () => { + describe('EVENT_NEW_MESSAGE', () => { + test('reorder correctly upon recieving a new message', () => { + const users = [eg.makeUser({ name: 'john' }), eg.makeUser({ name: 'mark' })]; + const newMessage = eg.pmMessage({ + id: 2, + display_recipient: [eg.displayRecipientFromUser(users[1])], + }); + const initialState = deepFreeze([ + { max_message_id: 1, user_ids: [users[0].user_id] }, + { max_message_id: 0, user_ids: [users[1].user_id] }, + ]); + + const action = deepFreeze({ + type: EVENT_NEW_MESSAGE, + message: newMessage, + id: 0, + caughtUp: {}, + ownEmail: '', + }); + + expect(recentPmConversationsReducer(initialState, action)).toEqual([ + { max_message_id: 2, user_ids: [users[1].user_id] }, + { max_message_id: 1, user_ids: [users[0].user_id] }, + ]); + }); + }); +}); diff --git a/src/pm-conversations/recentPmConversationsReducer.js b/src/pm-conversations/recentPmConversationsReducer.js index 7f9d79e49cb..7b12f04a39e 100644 --- a/src/pm-conversations/recentPmConversationsReducer.js +++ b/src/pm-conversations/recentPmConversationsReducer.js @@ -1,10 +1,33 @@ /* @flow strict-local */ +import isEqual from 'lodash.isequal'; + import type { Action, RecentPrivateConversationsState } from '../types'; -import { REALM_INIT } from '../actionConstants'; +import { REALM_INIT, EVENT_NEW_MESSAGE } from '../actionConstants'; import { NULL_ARRAY } from '../nullObjects'; const initialState: RecentPrivateConversationsState = NULL_ARRAY; +const newMessage = (state, action) => { + if (action.message.type !== 'private') { + return state; + } + + const userIds = action.message.display_recipient.map(recipient => recipient.id); + const index = state.findIndex(item => isEqual(item.user_ids, userIds)); + const oldMaxMsgId = index < 0 ? null : state[index].max_message_id; + const item = { + user_ids: userIds, + max_message_id: Math.max(action.message.id, oldMaxMsgId ?? -Infinity), + }; + + const unsorted = + index < 0 + ? [item, ...state] /* force linebreak */ + : [item, ...state.slice(0, index), ...state.slice(index + 1)]; + + return unsorted.sort((a, b) => -(a.max_message_id - b.max_message_id)); +}; + export default ( state: RecentPrivateConversationsState = initialState, action: Action, @@ -12,6 +35,8 @@ export default ( switch (action.type) { case REALM_INIT: return action.data.recent_private_conversations || initialState; + case EVENT_NEW_MESSAGE: + return newMessage(state, action); default: return state; } From edbf6f2322ef96e85d6d94ca6be54dc3528ccade Mon Sep 17 00:00:00 2001 From: Isham Mahajan Date: Sun, 30 Jun 2019 13:06:11 +0530 Subject: [PATCH 3/7] pms selectors: Change selectors for `getRecentConversations`. The parent commits introduced a new key `recent_private_conversations` which fetches the private conversations from the most recent 1000 private messages. This commit changes the aforementioned selector to use the `getRecentPrivateConversations` direct selector also introduced in the same parent commit. The effect of this is that the `PmConversationsCard` now displays all the private conversations that a person might be participating in instead of only from the last 100 messages (which was the amount of messages that the previous iteration of the code fetched in `doInitialFetch`, which directly determined the messages which were scanned -- and how many conversations were returned). `ownEmail` was removed in favor of `getOwnUser` in order to enable the usage of `currentUser.user_id`. The tests for `pmConversationsSelectors` has also been changed to reflect the new data used and the way it is used. For example: we no longer need to use the timestamps in order to sort conversations by most recent because `max_message_id` satisifes that desire by itself, and we don't need the returned conversations to have the current user's ID in every `huddle` type conversation. --- .../pmConversationsSelectors-test.js | 113 +++--------------- .../pmConversationsSelectors.js | 46 ++++--- src/users/userSelectors.js | 9 ++ src/utils/recipient.js | 6 + 4 files changed, 50 insertions(+), 124 deletions(-) diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index f665209a1ac..a0a2782b447 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -1,16 +1,13 @@ import deepFreeze from 'deep-freeze'; import { getRecentConversations } from '../pmConversationsSelectors'; -import { ALL_PRIVATE_NARROW_STR } from '../../utils/narrow'; describe('getRecentConversations', () => { - test('when no messages, return no conversations', () => { + test('when no recent conversations, return no conversations', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, + recentPrivateConversations: [], users: [{ user_id: 0, email: 'me@example.com' }], - narrows: { - [ALL_PRIVATE_NARROW_STR]: [], - }, unread: { pms: [], huddles: [], @@ -25,54 +22,17 @@ describe('getRecentConversations', () => { test('returns unique list of recipients, includes conversations with self', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, + recentPrivateConversations: [ + { max_message_id: 3, user_ids: [1] }, + { max_message_id: 2, user_ids: [2] }, + { max_message_id: 0, user_ids: [1, 2] }, + { max_message_id: 4, user_ids: [] }, + ], users: [ { user_id: 0, email: 'me@example.com' }, { user_id: 1, email: 'john@example.com' }, { user_id: 2, email: 'mark@example.com' }, ], - narrows: { - [ALL_PRIVATE_NARROW_STR]: [0, 1, 2, 3, 4], - }, - messages: { - 1: { - id: 1, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 2: { - id: 2, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 3: { - id: 3, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 4: { - id: 4, - type: 'private', - display_recipient: [{ id: 0, email: 'me@example.com' }], - }, - 0: { - id: 0, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - }, unread: { pms: [ { @@ -132,62 +92,17 @@ describe('getRecentConversations', () => { test('returns recipients sorted by last activity', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, + recentPrivateConversations: [ + { max_message_id: 4, user_ids: [1] }, + { max_message_id: 3, user_ids: [2] }, + { max_message_id: 5, user_ids: [1, 2] }, + { max_message_id: 6, user_ids: [] }, + ], users: [ { user_id: 0, email: 'me@example.com' }, { user_id: 1, email: 'john@example.com' }, { user_id: 2, email: 'mark@example.com' }, ], - narrows: { - [ALL_PRIVATE_NARROW_STR]: [1, 2, 3, 4, 5, 6], - }, - messages: { - 2: { - id: 2, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 1: { - id: 1, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 4: { - id: 4, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - ], - }, - 3: { - id: 3, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 5: { - id: 5, - type: 'private', - display_recipient: [ - { id: 0, email: 'me@example.com' }, - { id: 1, email: 'john@example.com' }, - { id: 2, email: 'mark@example.com' }, - ], - }, - 6: { - id: 6, - type: 'private', - display_recipient: [{ id: 0, email: 'me@example.com' }], - }, - }, unread: { pms: [ { diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index f8efbd8ba21..fd06f433ae1 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -1,44 +1,40 @@ /* @flow strict-local */ import { createSelector } from 'reselect'; -import type { Message, PmConversationData, Selector, User } from '../types'; -import { getPrivateMessages } from '../message/messageSelectors'; -import { getOwnUser } from '../users/userSelectors'; +import type { RecentPrivateConversation, PmConversationData, Selector, User } from '../types'; +import { getRecentPrivateConversations } from '../directSelectors'; +import { getOwnUser, getAllUserEmails } from '../users/userSelectors'; import { getUnreadByPms, getUnreadByHuddles } from '../unread/unreadSelectors'; -import { normalizeRecipientsSansMe, pmUnreadsKeyFromMessage } from '../utils/recipient'; +import { normalizeRecipientsSansMe, sortIds } from '../utils/recipient'; export const getRecentConversations: Selector = createSelector( + getRecentPrivateConversations, + getAllUserEmails, getOwnUser, - getPrivateMessages, getUnreadByPms, getUnreadByHuddles, ( + recentPrivateConversations: RecentPrivateConversation[], + emails: $ReadOnly<{ [user_id: number]: string }>, ownUser: User, - messages: Message[], unreadPms: { [number]: number }, unreadHuddles: { [string]: number }, ): PmConversationData[] => { - const recipients = messages.map(msg => ({ - ids: pmUnreadsKeyFromMessage(msg, ownUser.user_id), - emails: normalizeRecipientsSansMe(msg.display_recipient, ownUser.email), - msgId: msg.id, - })); - - const latestByRecipient = new Map(); - recipients.forEach(recipient => { - const prev = latestByRecipient.get(recipient.emails); - if (!prev || recipient.msgId > prev.msgId) { - latestByRecipient.set(recipient.emails, { - ids: recipient.ids, - recipients: recipient.emails, - msgId: recipient.msgId, - }); + const recipients = recentPrivateConversations.map(conversation => { + const conversationUserIdsIncludeMe = conversation.user_ids.slice(); + if (conversationUserIdsIncludeMe.length !== 1) { + conversationUserIdsIncludeMe.push(ownUser.user_id); } + return { + ids: sortIds(conversationUserIdsIncludeMe), + recipients: normalizeRecipientsSansMe( + conversationUserIdsIncludeMe.map(id => ({ email: emails[id] })), + ownUser.email, + ), + msgId: conversation.max_message_id, + }; }); - - const sortedByMostRecent = Array.from(latestByRecipient.values()).sort( - (a, b) => +b.msgId - +a.msgId, - ); + const sortedByMostRecent = recipients.sort((a, b) => +b.msgId - +a.msgId); return sortedByMostRecent.map(recipient => ({ ...recipient, diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index 775e826ec47..f6ec91c7484 100644 --- a/src/users/userSelectors.js +++ b/src/users/userSelectors.js @@ -52,6 +52,15 @@ export const getAllUsersByEmail: Selector> = createSelect allUsers => new Map(allUsers.map(user => [user.email, user])), ); +export const getAllUserEmails: Selector<$ReadOnly<{ [user_id: number]: string }>> = createSelector( + getAllUsers, + (users = []) => + users.reduce((array, user) => { + array[user.user_id] = user.email; + return array; + }, {}), +); + /** * WARNING: despite the name, only (a) `is_active` users (b) excluding cross-realm bots. * diff --git a/src/utils/recipient.js b/src/utils/recipient.js index b6f8ee456c2..8ac0fa93105 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -97,6 +97,12 @@ export const pmKeyRecipientsFromMessage = ( return filterRecipients(message.display_recipient, ownUser.user_id); }; +export const sortIds = (ids: number[]): string => + ids + .slice() + .sort((a, b) => a - b) + .join(','); + /** * The key this PM is filed under in the "unread messages" data structure. * From bfea8be1a78c40881d75ec5b08b9f82aceec68db Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 10 Apr 2020 16:28:43 -0700 Subject: [PATCH 4/7] use getAllUsersById --- .../pmConversationsSelectors.js | 21 ++++++++++++------- src/users/userSelectors.js | 9 -------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index fd06f433ae1..e83cf6a1550 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -3,19 +3,19 @@ import { createSelector } from 'reselect'; import type { RecentPrivateConversation, PmConversationData, Selector, User } from '../types'; import { getRecentPrivateConversations } from '../directSelectors'; -import { getOwnUser, getAllUserEmails } from '../users/userSelectors'; +import { getOwnUser, getAllUsersById } from '../users/userSelectors'; import { getUnreadByPms, getUnreadByHuddles } from '../unread/unreadSelectors'; import { normalizeRecipientsSansMe, sortIds } from '../utils/recipient'; export const getRecentConversations: Selector = createSelector( getRecentPrivateConversations, - getAllUserEmails, + getAllUsersById, getOwnUser, getUnreadByPms, getUnreadByHuddles, ( recentPrivateConversations: RecentPrivateConversation[], - emails: $ReadOnly<{ [user_id: number]: string }>, + allUsersById, ownUser: User, unreadPms: { [number]: number }, unreadHuddles: { [string]: number }, @@ -25,12 +25,19 @@ export const getRecentConversations: Selector = createSele if (conversationUserIdsIncludeMe.length !== 1) { conversationUserIdsIncludeMe.push(ownUser.user_id); } + + const emails = []; + for (const userId of conversationUserIdsIncludeMe) { + const user = allUsersById.get(userId); + if (!user) { + throw new Error('getRecentConversations: unknown user id'); + } + emails.push(user.email); + } + return { ids: sortIds(conversationUserIdsIncludeMe), - recipients: normalizeRecipientsSansMe( - conversationUserIdsIncludeMe.map(id => ({ email: emails[id] })), - ownUser.email, - ), + recipients: normalizeRecipientsSansMe(emails.map(email => ({ email })), ownUser.email), msgId: conversation.max_message_id, }; }); diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index f6ec91c7484..775e826ec47 100644 --- a/src/users/userSelectors.js +++ b/src/users/userSelectors.js @@ -52,15 +52,6 @@ export const getAllUsersByEmail: Selector> = createSelect allUsers => new Map(allUsers.map(user => [user.email, user])), ); -export const getAllUserEmails: Selector<$ReadOnly<{ [user_id: number]: string }>> = createSelector( - getAllUsers, - (users = []) => - users.reduce((array, user) => { - array[user.user_id] = user.email; - return array; - }, {}), -); - /** * WARNING: despite the name, only (a) `is_active` users (b) excluding cross-realm bots. * From 734a5f6b872968d6b60628991c889c3d85d2a43a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 10 Apr 2020 16:35:33 -0700 Subject: [PATCH 5/7] simplify, using invariant --- .../pmConversationsSelectors-test.js | 6 ++-- .../pmConversationsSelectors.js | 35 +++++++++---------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index a0a2782b447..5ef69fee92b 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -23,10 +23,10 @@ describe('getRecentConversations', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, recentPrivateConversations: [ + { max_message_id: 4, user_ids: [] }, { max_message_id: 3, user_ids: [1] }, { max_message_id: 2, user_ids: [2] }, { max_message_id: 0, user_ids: [1, 2] }, - { max_message_id: 4, user_ids: [] }, ], users: [ { user_id: 0, email: 'me@example.com' }, @@ -93,10 +93,10 @@ describe('getRecentConversations', () => { const state = deepFreeze({ realm: { email: 'me@example.com' }, recentPrivateConversations: [ + { max_message_id: 6, user_ids: [] }, + { max_message_id: 5, user_ids: [1, 2] }, { max_message_id: 4, user_ids: [1] }, { max_message_id: 3, user_ids: [2] }, - { max_message_id: 5, user_ids: [1, 2] }, - { max_message_id: 6, user_ids: [] }, ], users: [ { user_id: 0, email: 'me@example.com' }, diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index e83cf6a1550..91bdc5d7ded 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -19,8 +19,8 @@ export const getRecentConversations: Selector = createSele ownUser: User, unreadPms: { [number]: number }, unreadHuddles: { [string]: number }, - ): PmConversationData[] => { - const recipients = recentPrivateConversations.map(conversation => { + ): PmConversationData[] => + recentPrivateConversations.map(conversation => { const conversationUserIdsIncludeMe = conversation.user_ids.slice(); if (conversationUserIdsIncludeMe.length !== 1) { conversationUserIdsIncludeMe.push(ownUser.user_id); @@ -35,27 +35,24 @@ export const getRecentConversations: Selector = createSele emails.push(user.email); } + const userIdsString = sortIds(conversationUserIdsIncludeMe); + return { - ids: sortIds(conversationUserIdsIncludeMe), + ids: userIdsString, recipients: normalizeRecipientsSansMe(emails.map(email => ({ email })), ownUser.email), msgId: conversation.max_message_id, + unread: + // This business of looking in one place and then the other is kind + // of messy. Fortunately it always works, because the key spaces + // are disjoint: all `unreadHuddles` keys contain a comma, and all + // `unreadPms` keys don't. + /* $FlowFixMe: The keys of unreadPms are logically numbers, but + because it's an object they end up converted to strings, so + this access with string keys works. We should probably use a + Map for this and similar maps. */ + unreadPms[userIdsString] || unreadHuddles[userIdsString], }; - }); - const sortedByMostRecent = recipients.sort((a, b) => +b.msgId - +a.msgId); - - return sortedByMostRecent.map(recipient => ({ - ...recipient, - unread: - // This business of looking in one place and then the other is kind - // of messy. Fortunately it always works, because the key spaces - // are disjoint: all `unreadHuddles` keys contain a comma, and all - // `unreadPms` keys don't. - /* $FlowFixMe: The keys of unreadPms are logically numbers, but because it's an object they - end up converted to strings, so this access with string keys works. We should probably use - a Map for this and similar maps. */ - unreadPms[recipient.ids] || unreadHuddles[recipient.ids], - })); - }, + }), ); export const getUnreadConversations: Selector = createSelector( From 534387209e79d954444d077a5a8d974f1038593e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 10 Apr 2020 16:36:59 -0700 Subject: [PATCH 6/7] note some TODOs, preferably for before merge --- src/pm-conversations/pmConversationsSelectors.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pm-conversations/pmConversationsSelectors.js b/src/pm-conversations/pmConversationsSelectors.js index 91bdc5d7ded..bda01858e5b 100644 --- a/src/pm-conversations/pmConversationsSelectors.js +++ b/src/pm-conversations/pmConversationsSelectors.js @@ -21,11 +21,13 @@ export const getRecentConversations: Selector = createSele unreadHuddles: { [string]: number }, ): PmConversationData[] => recentPrivateConversations.map(conversation => { + // TODO make recipient.js handle this nastiness const conversationUserIdsIncludeMe = conversation.user_ids.slice(); if (conversationUserIdsIncludeMe.length !== 1) { conversationUserIdsIncludeMe.push(ownUser.user_id); } + // TODO make recipient.js handle this nastiness too const emails = []; for (const userId of conversationUserIdsIncludeMe) { const user = allUsersById.get(userId); From 8e83e98a2f93eb5c87b19700111f35ae7334950e Mon Sep 17 00:00:00 2001 From: Isham Mahajan Date: Tue, 6 Aug 2019 19:10:26 +0530 Subject: [PATCH 7/7] Make `pmConversationsSelectors-test.js` `flow strict`. The tests for `pmConversationsSelectors` are now `@flow strict-local`. To aid this process, we make use of the 'new' way of making tests, which is using `src/__tests__/exampleData.js`. A few examples of such tests include the unit tests at `src/users/__tests__/userSelectors-test.js` and `src/webview/html/__tests__/messageHeaderAsHtml-test.js`. Note that this is just a translation, and not a behavorial change. --- .../pmConversationsSelectors-test.js | 206 +++++++++--------- 1 file changed, 100 insertions(+), 106 deletions(-) diff --git a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js index 5ef69fee92b..24d4bbd5da4 100644 --- a/src/pm-conversations/__tests__/pmConversationsSelectors-test.js +++ b/src/pm-conversations/__tests__/pmConversationsSelectors-test.js @@ -1,161 +1,155 @@ -import deepFreeze from 'deep-freeze'; +/* @flow strict-local */ import { getRecentConversations } from '../pmConversationsSelectors'; +import * as eg from '../../__tests__/lib/exampleData'; describe('getRecentConversations', () => { test('when no recent conversations, return no conversations', () => { - const state = deepFreeze({ - realm: { email: 'me@example.com' }, - recentPrivateConversations: [], - users: [{ user_id: 0, email: 'me@example.com' }], - unread: { - pms: [], - huddles: [], - }, + const state = eg.reduxState({ + realm: eg.realmState({ email: eg.selfUser.email }), + users: [eg.selfUser], }); - const actual = getRecentConversations(state); - - expect(actual).toEqual([]); + expect(getRecentConversations(state)).toEqual([]); }); test('returns unique list of recipients, includes conversations with self', () => { - const state = deepFreeze({ - realm: { email: 'me@example.com' }, - recentPrivateConversations: [ - { max_message_id: 4, user_ids: [] }, - { max_message_id: 3, user_ids: [1] }, - { max_message_id: 2, user_ids: [2] }, - { max_message_id: 0, user_ids: [1, 2] }, + const users = [eg.selfUser, eg.makeUser({ name: 'john' }), eg.makeUser({ name: 'mark' })]; + const recentPrivateConversations = [ + { max_message_id: 4, user_ids: [] }, + { max_message_id: 3, user_ids: [users[1].user_id] }, + { max_message_id: 2, user_ids: [users[2].user_id] }, + { max_message_id: 0, user_ids: [users[1].user_id, users[2].user_id] }, + ]; + const unread = { + streams: [], + huddles: [ + { + user_ids_string: [eg.selfUser.user_id, users[1].user_id, users[2].user_id] + .sort((a, b) => a - b) + .join(), + unread_message_ids: [5], + }, ], - users: [ - { user_id: 0, email: 'me@example.com' }, - { user_id: 1, email: 'john@example.com' }, - { user_id: 2, email: 'mark@example.com' }, + pms: [ + { + sender_id: eg.selfUser.user_id, + unread_message_ids: [4], + }, + { + sender_id: users[1].user_id, + unread_message_ids: [1, 3], + }, + { + sender_id: users[2].user_id, + unread_message_ids: [2], + }, ], - unread: { - pms: [ - { - sender_id: 0, - unread_message_ids: [4], - }, - { - sender_id: 1, - unread_message_ids: [1, 3], - }, - { - sender_id: 2, - unread_message_ids: [2], - }, - ], - huddles: [ - { - user_ids_string: '0,1,2', - unread_message_ids: [5], - }, - ], - }, + mentions: [], + }; + + const state = eg.reduxState({ + realm: eg.realmState({ email: eg.selfUser.email }), + users, + recentPrivateConversations, + unread, }); - const expectedResult = [ + expect(getRecentConversations(state)).toEqual([ { - ids: '0', - recipients: 'me@example.com', + ids: eg.selfUser.user_id.toString(), + recipients: eg.selfUser.email, msgId: 4, unread: 1, }, { - ids: '1', - recipients: 'john@example.com', + ids: users[1].user_id.toString(), + recipients: users[1].email, msgId: 3, unread: 2, }, { - ids: '2', - recipients: 'mark@example.com', + ids: users[2].user_id.toString(), + recipients: users[2].email, msgId: 2, unread: 1, }, { - ids: '0,1,2', - recipients: 'john@example.com,mark@example.com', + ids: [eg.selfUser.user_id, users[1].user_id, users[2].user_id].sort((a, b) => a - b).join(), + recipients: [users[1].email, users[2].email].join(), msgId: 0, unread: 1, }, - ]; - - const actual = getRecentConversations(state); - - expect(actual).toEqual(expectedResult); + ]); }); test('returns recipients sorted by last activity', () => { - const state = deepFreeze({ - realm: { email: 'me@example.com' }, - recentPrivateConversations: [ - { max_message_id: 6, user_ids: [] }, - { max_message_id: 5, user_ids: [1, 2] }, - { max_message_id: 4, user_ids: [1] }, - { max_message_id: 3, user_ids: [2] }, + const users = [eg.selfUser, eg.makeUser({ name: 'john' }), eg.makeUser({ name: 'mark' })]; + const recentPrivateConversations = [ + { max_message_id: 6, user_ids: [] }, + { max_message_id: 5, user_ids: [users[1].user_id, users[2].user_id] }, + { max_message_id: 4, user_ids: [users[1].user_id] }, + { max_message_id: 3, user_ids: [users[2].user_id] }, + ]; + const unread = { + streams: [], + huddles: [ + { + user_ids_string: [eg.selfUser.user_id, users[1].user_id, users[2].user_id] + .sort((a, b) => a - b) + .join(), + unread_message_ids: [5], + }, ], - users: [ - { user_id: 0, email: 'me@example.com' }, - { user_id: 1, email: 'john@example.com' }, - { user_id: 2, email: 'mark@example.com' }, + pms: [ + { + sender_id: eg.selfUser.user_id, + unread_message_ids: [4], + }, + { + sender_id: users[1].user_id, + unread_message_ids: [1, 3], + }, + { + sender_id: users[2].user_id, + unread_message_ids: [2], + }, ], - unread: { - pms: [ - { - sender_id: 0, - unread_message_ids: [4], - }, - { - sender_id: 1, - unread_message_ids: [1, 3], - }, - { - sender_id: 2, - unread_message_ids: [2], - }, - ], - huddles: [ - { - user_ids_string: '0,1,2', - unread_message_ids: [5], - }, - ], - }, + mentions: [], + }; + + const state = eg.reduxState({ + realm: eg.realmState({ email: eg.selfUser.email }), + users, + recentPrivateConversations, + unread, }); - const expectedResult = [ + expect(getRecentConversations(state)).toEqual([ { - ids: '0', - recipients: 'me@example.com', + ids: eg.selfUser.user_id.toString(), + recipients: eg.selfUser.email, msgId: 6, unread: 1, }, { - ids: '0,1,2', - recipients: 'john@example.com,mark@example.com', + ids: [eg.selfUser.user_id, users[1].user_id, users[2].user_id].sort((a, b) => a - b).join(), + recipients: [users[1].email, users[2].email].join(), msgId: 5, unread: 1, }, { - ids: '1', - recipients: 'john@example.com', + ids: users[1].user_id.toString(), + recipients: users[1].email, msgId: 4, unread: 2, }, { - ids: '2', - recipients: 'mark@example.com', + ids: users[2].user_id.toString(), + recipients: users[2].email, msgId: 3, unread: 1, }, - ]; - - const actual = getRecentConversations(state); - - expect(actual).toEqual(expectedResult); + ]); }); });