Skip to content

Commit

Permalink
pms / recipient: Revert PR #4040.
Browse files Browse the repository at this point in the history
This reverts the following commits (which constitute PR #4040):
 * a111cfc
 * d8f4512
 * d464e33
 * 79c8671
 * 423cd19
 * 2a2a2e1

Commit 79c8671 introduced a crash bug. `getOwnUser` relies on the
presence of the user database which is provided by REALM_INIT, but
it's being called as part of the instantiation of `UnreadCards` before
REALM_INIT occurs.
  • Loading branch information
rk-for-zulip committed Apr 27, 2020
1 parent 88063e5 commit 084d726
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 61 deletions.
11 changes: 0 additions & 11 deletions src/pm-conversations/__tests__/pmConversationsSelectors-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ describe('getRecentConversations', () => {
test('when no messages, return no conversations', () => {
const state = deepFreeze({
realm: { email: 'me@example.com' },
users: [{ user_id: 0, email: 'me@example.com' }],
narrows: {
[ALL_PRIVATE_NARROW_STR]: [],
},
Expand All @@ -25,11 +24,6 @@ describe('getRecentConversations', () => {
test('returns unique list of recipients, includes conversations with self', () => {
const state = deepFreeze({
realm: { email: 'me@example.com' },
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],
},
Expand Down Expand Up @@ -132,11 +126,6 @@ describe('getRecentConversations', () => {
test('returns recipients sorted by last activity', () => {
const state = deepFreeze({
realm: { email: 'me@example.com' },
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],
},
Expand Down
18 changes: 7 additions & 11 deletions src/pm-conversations/pmConversationsSelectors.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
/* @flow strict-local */
import { createSelector } from 'reselect';

import type { Message, PmConversationData, Selector, User } from '../types';
import type { Message, PmConversationData, Selector } from '../types';
import { getPrivateMessages } from '../message/messageSelectors';
import { getOwnUser } from '../users/userSelectors';
import { getOwnEmail } from '../users/userSelectors';
import { getUnreadByPms, getUnreadByHuddles } from '../unread/unreadSelectors';
import { normalizeRecipientsSansMe, pmUnreadsKeyFromMessage } from '../utils/recipient';
import { normalizeRecipientsSansMe, getRecipientsIds } from '../utils/recipient';

export const getRecentConversations: Selector<PmConversationData[]> = createSelector(
getOwnUser,
getOwnEmail,
getPrivateMessages,
getUnreadByPms,
getUnreadByHuddles,
(
ownUser: User,
ownEmail: string,
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),
ids: getRecipientsIds(msg, ownEmail),
emails: normalizeRecipientsSansMe(msg.display_recipient, ownEmail),
msgId: msg.id,
}));

Expand All @@ -43,10 +43,6 @@ export const getRecentConversations: Selector<PmConversationData[]> = createSele
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. */
Expand Down
4 changes: 2 additions & 2 deletions src/unread/unreadHuddlesReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
EVENT_MESSAGE_DELETE,
EVENT_UPDATE_MESSAGE_FLAGS,
} from '../actionConstants';
import { pmUnreadsKeyFromMessage } from '../utils/recipient';
import { getRecipientsIds } from '../utils/recipient';
import { addItemsToHuddleArray, removeItemsDeeply } from './unreadHelpers';
import { NULL_ARRAY } from '../nullObjects';

Expand All @@ -27,7 +27,7 @@ const eventNewMessage = (state, action) => {
return state;
}

return addItemsToHuddleArray(state, [action.message.id], pmUnreadsKeyFromMessage(action.message));
return addItemsToHuddleArray(state, [action.message.id], getRecipientsIds(action.message));
};

const eventUpdateMessageFlags = (state, action) => {
Expand Down
50 changes: 13 additions & 37 deletions src/utils/recipient.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,8 @@ export const pmUiRecipientsFromMessage = (
* including stream and topic narrows.
*
* * `normalizeRecipients`, `normalizeRecipientsSansMe`, and
* `pmUnreadsKeyFromMessage`, which do the same job as this function with
* slight variations, and which we variously use in different places in
* the app.
* `getRecipientsIds`, which do the same job as this function with slight
* variations, and which we variously use in different places in the app.
*
* It would be great to unify on a single version, as the variation is a
* possible source of bugs.
Expand All @@ -97,44 +96,21 @@ export const pmKeyRecipientsFromMessage = (
return filterRecipients(message.display_recipient, ownUser.user_id);
};

/**
* The key this PM is filed under in the "unread messages" data structure.
*
* Note this diverges slightly from pmKeyRecipientsFromMessage in its
* behavior -- it encodes a different set of users.
*
* See also:
* * `pmKeyRecipientsFromMessage`, which we use for other data structures.
* * `UnreadState`, the type of `state.unread`, which is the data structure
* these keys appear in.
*
* @param ownUserId - Required if the message could be a 1:1 PM; optional if
* it is definitely a group PM.
*/
// Specifically, this includes all user IDs for group PMs and self-PMs,
// 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 getRecipientsIds = (message: Message, ownEmail?: string): string => {
if (message.type !== 'private') {
throw new Error('pmUnreadsKeyFromMessage: expected PM, got stream message');
throw new Error('getRecipientsIds: expected PM, got stream message');
}
const recipients: PmRecipientUser[] = message.display_recipient;
// This includes all users in the thread; see `Message#display_recipient`.
const userIds = recipients.map(r => r.id);

if (userIds.length === 1) {
// Self-PM.
return userIds[0].toString();
} else if (userIds.length === 2) {
// Non-self 1:1 PM. Unlike display_recipient, leave out the self user.
if (ownUserId === undefined) {
throw new Error('getRecipientsIds: got 1:1 PM, but ownUserId omitted');
const recipients = message.display_recipient;
if (recipients.length === 2) {
if (ownEmail === undefined) {
throw new Error('getRecipientsIds: got 1:1 PM, but ownEmail omitted');
}
return userIds.filter(userId => userId !== ownUserId)[0].toString();
return recipients.filter(r => r.email !== ownEmail)[0].id.toString();
} else {
// Group PM.
return userIds.sort((a, b) => a - b).join(',');
return recipients
.map(s => s.id)
.sort((a, b) => a - b)
.join(',');
}
};

Expand Down

0 comments on commit 084d726

Please sign in to comment.