Skip to content

recipient: Finish reconciling the ways we identify a PM conversation #4356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gnprice merged 6 commits into from
Dec 28, 2020
Merged

recipient: Finish reconciling the ways we identify a PM conversation #4356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
af84897
narrow [nfc]: Take whole own-user in isMessageInNarrow, to have ID.
gnprice Dec 12, 2020
aee9c5e
narrow: Use user IDs in isMessageInNarrow, instead of emails.
gnprice Dec 12, 2020
7e769af
recipient [nfc]: Delete normalizeRecipientsSansMe!
gnprice Dec 12, 2020
723635b
recipient: Use IDs in isSameRecipient; cut out normalizeRecipients!
gnprice Dec 12, 2020
fe0a2e9
unread: Use IDs, not emails, in getUnreadCountForNarrow.
gnprice Dec 12, 2020
e7315cd
recipient [nfc]: Add explicit helper pmUnreadsKeyFromPmKeyIds.
gnprice Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/chat/__tests__/narrowsSelectors-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ describe('getMessagesForNarrow', () => {
}),
messages,
outbox: [],
realm: eg.realmState({ email: eg.selfUser.email }),
users: [eg.selfUser],
realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }),
});

const result = getMessagesForNarrow(state, HOME_NARROW);
Expand All @@ -55,7 +56,8 @@ describe('getMessagesForNarrow', () => {
caughtUp: {
[HOME_NARROW_STR]: { older: false, newer: true },
},
realm: eg.realmState({ email: eg.selfUser.email }),
users: [eg.selfUser],
realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }),
});

const result = getMessagesForNarrow(state, HOME_NARROW);
Expand All @@ -70,7 +72,8 @@ describe('getMessagesForNarrow', () => {
}),
messages,
outbox: [outboxMessage],
realm: eg.realmState({ email: eg.selfUser.email }),
users: [eg.selfUser],
realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }),
});

const result = getMessagesForNarrow(state, HOME_NARROW);
Expand All @@ -85,7 +88,8 @@ describe('getMessagesForNarrow', () => {
}),
messages,
outbox: [outboxMessage],
realm: eg.realmState({ email: eg.selfUser.email }),
users: [eg.selfUser],
realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }),
});

const result = getMessagesForNarrow(state, pm1to1NarrowFromUser(eg.otherUser));
Expand Down
8 changes: 4 additions & 4 deletions src/chat/narrowsSelectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
getOutbox,
} from '../directSelectors';
import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors';
import { getAllUsersByEmail, getOwnEmail } from '../users/userSelectors';
import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors';
import {
isStreamOrTopicNarrow,
emailsOfGroupPmNarrow,
Expand All @@ -38,8 +38,8 @@ export const outboxMessagesForNarrow: Selector<Outbox[], Narrow> = createSelecto
(state, narrow) => narrow,
getCaughtUpForNarrow,
state => getOutbox(state),
getOwnEmail,
(narrow, caughtUp, outboxMessages, ownEmail) => {
getOwnUser,
(narrow, caughtUp, outboxMessages, ownUser) => {
if (!caughtUp.newer) {
return NULL_ARRAY;
}
Expand All @@ -52,7 +52,7 @@ export const outboxMessagesForNarrow: Selector<Outbox[], Narrow> = createSelecto
// messages can't be starred, so "no flags" gives that the right answer.
const fakeFlags = [];
const filtered = outboxMessages.filter(message =>
isMessageInNarrow(message, fakeFlags, narrow, ownEmail),
isMessageInNarrow(message, fakeFlags, narrow, ownUser),
);
return isEqual(filtered, outboxMessages) ? outboxMessages : filtered;
},
Expand Down
3 changes: 2 additions & 1 deletion src/events/doEventActionSideEffects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { getActiveAccount, getChatScreenParams, getOwnEmail } from '../selectors
import { playMessageSound } from '../utils/sound';
import { NULL_ARRAY } from '../nullObjects';
import { ensureTypingStatusExpiryLoop } from '../typing/typingActions';
import { getOwnUser } from '../users/userSelectors';

/**
* React to incoming `MessageEvent`s.
Expand All @@ -34,7 +35,7 @@ const messageEvent = (state: GlobalState, message: Message): void => {
activeAccount
&& narrow !== undefined // chat screen is not at top
&& !isHomeNarrow(narrow)
&& isMessageInNarrow(message, flags, narrow, activeAccount.email);
&& isMessageInNarrow(message, flags, narrow, getOwnUser(state));
const isSenderSelf = getOwnEmail(state) === message.sender_email;
if (!isUserInSameNarrow && !isSenderSelf) {
playMessageSound();
Expand Down
6 changes: 4 additions & 2 deletions src/topics/__tests__/topicsSelectors-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ describe('getLastMessageTopic', () => {
test('when no messages in narrow return an empty string', () => {
const state = eg.reduxState({
narrows: Immutable.Map({}),
realm: eg.realmState({ email: eg.selfUser.email }),
users: [eg.selfUser],
realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }),
});

const topic = getLastMessageTopic(state, HOME_NARROW);
Expand All @@ -53,7 +54,8 @@ describe('getLastMessageTopic', () => {
[message1.id]: message1,
[message2.id]: message2,
},
realm: eg.realmState({ email: eg.selfUser.email }),
users: [eg.selfUser],
realm: eg.realmState({ user_id: eg.selfUser.user_id, email: eg.selfUser.email }),
});

const topic = getLastMessageTopic(state, narrow);
Expand Down
38 changes: 11 additions & 27 deletions src/unread/unreadSelectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ import {
getUnreadHuddles,
getUnreadMentions,
} from '../directSelectors';
import { getOwnEmail, getAllUsersByEmail } from '../users/userSelectors';
import { getOwnUserId } from '../users/userSelectors';
import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors';
import { isTopicMuted } from '../utils/message';
import { caseNarrow } from '../utils/narrow';
import { NULL_SUBSCRIPTION, NULL_USER } from '../nullObjects';
import { NULL_SUBSCRIPTION } from '../nullObjects';
import { pmUnreadsKeyFromPmKeyIds } from '../utils/recipient';

/** The number of unreads in each stream, excluding muted topics, by stream ID. */
export const getUnreadByStream: Selector<{ [number]: number }> = createSelector(
Expand Down Expand Up @@ -221,24 +222,13 @@ export const getUnreadByHuddlesMentionsAndPMs: Selector<number> = createSelector
export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
(state, narrow) => narrow,
state => getStreams(state),
state => getAllUsersByEmail(state),
state => getOwnEmail(state),
state => getOwnUserId(state),
state => getUnreadTotal(state),
state => getUnreadStreams(state),
state => getUnreadHuddles(state),
state => getUnreadPms(state),
state => getMute(state),
(
narrow,
streams,
allUsersByEmail,
ownEmail,
unreadTotal,
unreadStreams,
unreadHuddles,
unreadPms,
mute,
) => {
(narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) => {
const sumLengths = unreads => unreads.reduce((sum, x) => sum + x.unread_message_ids.length, 0);

return caseNarrow(narrow, {
Expand Down Expand Up @@ -266,20 +256,14 @@ export const getUnreadCountForNarrow: Selector<number, Narrow> = createSelector(
);
},

pm: emails => {
if (emails.length > 1) {
const userIds = [...emails, ownEmail]
.map(email => (allUsersByEmail.get(email) || NULL_USER).user_id)
.sort((a, b) => a - b)
.join(',');
const unread = unreadHuddles.find(x => x.user_ids_string === userIds);
pm: (emails, ids) => {
if (ids.length > 1) {
const unreadsKey = pmUnreadsKeyFromPmKeyIds(ids, ownUserId);
const unread = unreadHuddles.find(x => x.user_ids_string === unreadsKey);
return unread ? unread.unread_message_ids.length : 0;
} else {
const sender = allUsersByEmail.get(emails[0]);
if (!sender) {
return 0;
}
const unread = unreadPms.find(x => x.sender_id === sender.user_id);
const senderId = ids[0];
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe Dec 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could call pmUnreadsKeyFromPmKeyIds to get senderId here, I suppose—as long as senderId and unreadPms[].sender_id are made to agree in being a number or a string (for the .find that they're used in here).

Copy link
Copy Markdown
Member Author

@gnprice gnprice Dec 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think that will most naturally come after converting these to our own Immutable.Map or similar data structure, and perhaps after converting the both of these (state.unread.pms and state.unread.huddles) into a single such data structure.

With the data structures as they are, with .sender_id a number, if we ran ids through pmUnreadsKeyFromPmKeyIds we'd have to convert it back from string to number in order to do the search (or else convert every sender_id into a string to do the comparison, which would be inefficient.) That conversion would make sense only because we know the key isn't just any string but has a particular structure... but the same facts about its structure mean we can just use the number directly and skip converting to a string and back.

Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe Dec 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think that will most naturally come after converting these to our own Immutable.Map or similar data structure, and perhaps after converting the both of these (state.unread.pms and state.unread.huddles) into a single such data structure.

Makes sense—I figured that was in the plan. 🙂

const unread = unreadPms.find(x => x.sender_id === senderId);
return unread ? unread.unread_message_ids.length : 0;
}
},
Expand Down
3 changes: 1 addition & 2 deletions src/utils/__tests__/narrow-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ describe('SEARCH_NARROW', () => {
});

describe('isMessageInNarrow', () => {
const ownEmail = eg.selfUser.email;
const otherStream = eg.makeStream();

// prettier-ignore
Expand Down Expand Up @@ -191,7 +190,7 @@ describe('isMessageInNarrow', () => {
for (const [messageDescription, expected, message] of cases) {
test(`${expected ? 'contains' : 'excludes'} ${messageDescription}`, () => {
expect(
isMessageInNarrow(message, message.flags ?? [], narrow, ownEmail),
isMessageInNarrow(message, message.flags ?? [], narrow, eg.selfUser),
).toBe(expected);
});
}
Expand Down
53 changes: 0 additions & 53 deletions src/utils/__tests__/recipient-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,61 +1,8 @@
import {
normalizeRecipients,
normalizeRecipientsAsUserIds,
normalizeRecipientsSansMe,
normalizeRecipientsAsUserIdsSansMe,
isSameRecipient,
} from '../recipient';
import * as logging from '../logging';

describe('normalizeRecipients', () => {
test('joins emails from recipients, sorted, trimmed, not including missing ones', () => {
const recipients = [
{ email: '' },
{ email: 'abc@example.com' },
{ email: 'xyz@example.com' },
{ email: ' def@example.com ' },
];
const expectedResult = 'abc@example.com,def@example.com,xyz@example.com';

logging.error.mockReturnValue();

const normalized = normalizeRecipients(recipients);
expect(normalized).toEqual(expectedResult);

expect(logging.error.mock.calls).toHaveLength(2);
logging.error.mockReset();
});
});

describe('normalizeRecipientsSansMe', () => {
test('if only self email provided return unmodified', () => {
const recipients = [{ email: 'me@example.com' }];
const ownEmail = 'me@example.com';
const expectedResult = 'me@example.com';

const normalized = normalizeRecipientsSansMe(recipients, ownEmail);

expect(normalized).toEqual(expectedResult);
});

test('when more than one emails normalize but filter out self email', () => {
const recipients = [
{ email: 'abc@example.com' },
{ email: 'me@example.com' },
{ email: ' def@example.com ' },
];
const ownEmail = 'me@example.com';
const expectedResult = 'abc@example.com,def@example.com';

logging.error.mockReturnValue();

const normalized = normalizeRecipientsSansMe(recipients, ownEmail);
expect(normalized).toEqual(expectedResult);

expect(logging.error.mock.calls).toHaveLength(1);
logging.error.mockReset();
});
});

describe('normalizeRecipientsAsUserIds', () => {
test('joins user IDs from recipients, sorted', () => {
Expand Down
14 changes: 7 additions & 7 deletions src/utils/narrow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import invariant from 'invariant';

import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types';
import {
normalizeRecipientsSansMe,
normalizeRecipientsAsUserIdsSansMe,
pmKeyRecipientsFromMessage,
recipientsOfPrivateMessage,
streamNameOfStreamMessage,
Expand Down Expand Up @@ -468,7 +468,7 @@ export const isMessageInNarrow = (
message: Message | Outbox,
flags: $ReadOnlyArray<string>,
narrow: Narrow,
ownEmail: string,
ownUser: User,
): boolean =>
caseNarrow(narrow, {
home: () => true,
Expand All @@ -477,15 +477,15 @@ export const isMessageInNarrow = (
message.type === 'stream'
&& streamName === streamNameOfStreamMessage(message)
&& topic === message.subject,
pm: emails => {
pm: (emails, ids) => {
if (message.type !== 'private') {
return false;
}
const recipients = recipientsOfPrivateMessage(message);
const narrowAsRecipients = emails.map(email => ({ email }));
const recipients = recipientsOfPrivateMessage(message).map(r => r.id);
const narrowAsRecipients = ids;
return (
normalizeRecipientsSansMe(recipients, ownEmail)
=== normalizeRecipientsSansMe(narrowAsRecipients, ownEmail)
normalizeRecipientsAsUserIdsSansMe(recipients, ownUser.user_id)
=== normalizeRecipientsAsUserIdsSansMe(narrowAsRecipients, ownUser.user_id)
);
},
starred: () => flags.includes('starred'),
Expand Down
Loading