diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 00679e75174..64095cab165 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -370,10 +370,9 @@ const outboxMessageBase: $Diff = deep avatar_url: selfUser.avatar_url, content: '

Test.

', - display_recipient: 'test', + display_recipient: stream.name, // id: ..., markdownContent: 'Test.', - narrow: [{ operator: 'stream', operand: 'test' }], reactions: [], sender_email: selfUser.email, sender_full_name: selfUser.full_name, diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 6c3e0db1744..12205a382d4 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -25,9 +25,7 @@ describe('getMessagesForNarrow', () => { // them to be numbers. [123]: message /* eslint-disable-line no-useless-computed-key */, }; - const outboxMessage = eg.makeOutboxMessage({ - narrow: HOME_NARROW, - }); + const outboxMessage = eg.makeOutboxMessage({}); test('if no outbox messages returns messages with no change', () => { const state = eg.reduxState({ @@ -36,6 +34,7 @@ describe('getMessagesForNarrow', () => { }, messages, outbox: [], + realm: eg.realmState({ email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -53,6 +52,7 @@ describe('getMessagesForNarrow', () => { caughtUp: { [HOME_NARROW_STR]: { older: false, newer: true }, }, + realm: eg.realmState({ email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -67,6 +67,7 @@ describe('getMessagesForNarrow', () => { }, messages, outbox: [outboxMessage], + realm: eg.realmState({ email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, HOME_NARROW); @@ -80,7 +81,8 @@ describe('getMessagesForNarrow', () => { [JSON.stringify(privateNarrow('john@example.com'))]: [123], }, messages, - outbox: [{ ...outboxMessage, narrow: streamNarrow('denmark') }], + outbox: [outboxMessage], + realm: eg.realmState({ email: eg.selfUser.email }), }); const result = getMessagesForNarrow(state, privateNarrow('john@example.com')); diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 21d09f03ea0..c360d1d86a6 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -47,7 +47,11 @@ const eventNewMessage = (state, action) => { let stateChange = false; const newState: NarrowsState = {}; Object.keys(state).forEach(key => { - const isInNarrow = isMessageInNarrow(action.message, JSON.parse(key), action.ownEmail); + const { flags } = action.message; + if (!flags) { + throw new Error('EVENT_NEW_MESSAGE message missing flags'); + } + const isInNarrow = isMessageInNarrow(action.message, flags, JSON.parse(key), action.ownEmail); const isCaughtUp = action.caughtUp[key] && action.caughtUp[key].newer; const messageDoesNotExist = state[key].find(id => action.message.id === id) === undefined; diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index 2d13ce3e945..52ebb6fdcbd 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -21,12 +21,12 @@ import { getOutbox, } from '../directSelectors'; import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors'; -import { getAllUsersByEmail } from '../users/userSelectors'; +import { getAllUsersByEmail, getOwnEmail } from '../users/userSelectors'; import { isPrivateNarrow, isStreamOrTopicNarrow, emailsOfGroupNarrow, - narrowContains, + isMessageInNarrow, } from '../utils/narrow'; import { shouldBeMuted } from '../utils/message'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; @@ -35,11 +35,22 @@ export const outboxMessagesForNarrow: Selector = createSelecto (state, narrow) => narrow, getCaughtUpForNarrow, state => getOutbox(state), - (narrow, caughtUp, outboxMessages) => { + getOwnEmail, + (narrow, caughtUp, outboxMessages, ownEmail) => { if (!caughtUp.newer) { return NULL_ARRAY; } - const filtered = outboxMessages.filter(item => narrowContains(narrow, item.narrow)); + // TODO?: Handle @-mention flags in outbox messages. As is, if you + // @-mention yourself (or a wildcard) and then go look at the + // is:mentioned view while your message is still unsent, we wrongly + // leave it out. Pretty uncommon edge case, though. + // + // No other narrows rely on flags except the "starred" narrow. Outbox + // 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), + ); return isEqual(filtered, outboxMessages) ? outboxMessages : filtered; }, ); diff --git a/src/events/doEventActionSideEffects.js b/src/events/doEventActionSideEffects.js index 58beaaecf66..c7719164db6 100644 --- a/src/events/doEventActionSideEffects.js +++ b/src/events/doEventActionSideEffects.js @@ -35,7 +35,7 @@ const messageEvent = (state: GlobalState, message: Message): void => { activeAccount && narrow !== undefined // chat screen is not at top && !isHomeNarrow(narrow) - && isMessageInNarrow(message, narrow, activeAccount.email); + && isMessageInNarrow(message, flags, narrow, activeAccount.email); const isSenderSelf = getOwnEmail(state) === message.sender_email; if (!isUserInSameNarrow && !isSenderSelf) { playMessageSound(); diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 3112bea9c87..87dea0e752e 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -9,7 +9,7 @@ import type { NamedUser, Narrow, Outbox, - User, + UserOrBot, Action, } from '../types'; import { @@ -20,11 +20,10 @@ import { } from '../actionConstants'; import { getAuth } from '../selectors'; import * as api from '../api'; -import { getSelfUserDetail, getUsersByEmail } from '../users/userSelectors'; +import { getAllUsersByEmail, getOwnUser } from '../users/userSelectors'; import { getUsersAndWildcards } from '../users/userHelpers'; -import { isStreamNarrow, isPrivateOrGroupNarrow } from '../utils/narrow'; +import { caseNarrowPartial } from '../utils/narrow'; import { BackoffMachine } from '../utils/async'; -import { NULL_USER } from '../nullObjects'; export const messageSendStart = (outbox: Outbox): Action => ({ type: MESSAGE_SEND_START, @@ -64,17 +63,16 @@ export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean return; // i.e., continue } - const to = ((): string => { - const { narrow } = item; - // TODO: can this test be `if (item.type === private)`? - if (isPrivateOrGroupNarrow(narrow)) { - return narrow[0].operand; - } else { - // HACK: the server attempts to interpret this argument as JSON, then - // CSV, then a literal. To avoid misparsing, always use JSON. - return JSON.stringify([item.display_recipient]); - } - })(); + // prettier-ignore + const to = + item.type === 'private' + // TODO(server-2.0): switch to numeric user IDs, not emails. + ? item.display_recipient.map(r => r.email).join(',') + // TODO(server-2.0): switch to numeric stream IDs, not names. + // (This will require wiring the stream ID through to here.) + // HACK: the server attempts to interpret this argument as JSON, then + // CSV, then a literal. To avoid misparsing, always use JSON. + : JSON.stringify([item.display_recipient]); await api.sendMessage(auth, { type: item.type, @@ -106,14 +104,21 @@ export const sendOutbox = () => async (dispatch: Dispatch, getState: GetState) = dispatch(toggleOutboxSending(false)); }; -const mapEmailsToUsers = (usersByEmail, narrow, selfDetail) => - narrow[0].operand - .split(',') - .map(item => { - const user = usersByEmail.get(item) || NULL_USER; - return { email: item, id: user.user_id, full_name: user.full_name }; - }) - .concat({ email: selfDetail.email, id: selfDetail.user_id, full_name: selfDetail.full_name }); +// A valid display_recipient with all the thread's users, sorted by ID. +const mapEmailsToUsers = (emails, allUsersByEmail, ownUser) => { + const result = emails.map(email => { + const user = allUsersByEmail.get(email); + if (!user) { + throw new Error('outbox: missing user when preparing to send PM'); + } + return { email, id: user.user_id, full_name: user.full_name }; + }); + if (!result.some(r => r.id === ownUser.user_id)) { + result.push({ email: ownUser.email, id: ownUser.user_id, full_name: ownUser.full_name }); + } + result.sort((r1, r2) => r1.id - r2.id); + return result; +}; // TODO type: `string | NamedUser[]` is a bit confusing. type DataFromNarrow = {| @@ -124,19 +129,28 @@ type DataFromNarrow = {| const extractTypeToAndSubjectFromNarrow = ( narrow: Narrow, - usersByEmail: Map, - selfDetail: { email: string, user_id: number, full_name: string }, + allUsersByEmail: Map, + ownUser: UserOrBot, ): DataFromNarrow => { - if (isPrivateOrGroupNarrow(narrow)) { - return { - type: 'private', - display_recipient: mapEmailsToUsers(usersByEmail, narrow, selfDetail), - subject: '', - }; - } else if (isStreamNarrow(narrow)) { - return { type: 'stream', display_recipient: narrow[0].operand, subject: '(no topic)' }; - } - return { type: 'stream', display_recipient: narrow[0].operand, subject: narrow[1].operand }; + const forPm = emails => ({ + type: 'private', + display_recipient: mapEmailsToUsers(emails, allUsersByEmail, ownUser), + subject: '', + }); + return caseNarrowPartial(narrow, { + pm: email => forPm([email]), + groupPm: forPm, + + // TODO: we shouldn't ever be passing a whole-stream narrow here; + // ensure we don't, then remove this case + stream: name => ({ type: 'stream', display_recipient: name, subject: '(no topic)' }), + + topic: (streamName, topic) => ({ + type: 'stream', + display_recipient: streamName, + subject: topic, + }), + }); }; const getContentPreview = (content: string, state: GlobalState): string => { @@ -159,21 +173,20 @@ export const addToOutbox = (narrow: Narrow, content: string) => async ( getState: GetState, ) => { const state = getState(); - const userDetail = getSelfUserDetail(state); + const ownUser = getOwnUser(state); const localTime = Math.round(new Date().getTime() / 1000); dispatch( messageSendStart({ - narrow, isSent: false, - ...extractTypeToAndSubjectFromNarrow(narrow, getUsersByEmail(state), userDetail), + ...extractTypeToAndSubjectFromNarrow(narrow, getAllUsersByEmail(state), ownUser), markdownContent: content, content: getContentPreview(content, state), timestamp: localTime, id: localTime, - sender_full_name: userDetail.full_name, - sender_email: userDetail.email, - avatar_url: userDetail.avatar_url, + sender_full_name: ownUser.full_name, + sender_email: ownUser.email, + avatar_url: ownUser.avatar_url, isOutbox: true, reactions: [], }), diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index ede0a76e10a..65e3de63ad2 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -31,6 +31,7 @@ describe('getLastMessageTopic', () => { test('when no messages in narrow return an empty string', () => { const state = eg.reduxState({ narrows: {}, + realm: eg.realmState({ email: eg.selfUser.email }), }); const topic = getLastMessageTopic(state, HOME_NARROW); @@ -50,6 +51,7 @@ describe('getLastMessageTopic', () => { [message1.id]: message1, [message2.id]: message2, }, + realm: eg.realmState({ email: eg.selfUser.email }), }); const topic = getLastMessageTopic(state, narrow); diff --git a/src/types.js b/src/types.js index a5d7f362dc3..6ad9e2b893a 100644 --- a/src/types.js +++ b/src/types.js @@ -2,7 +2,7 @@ import type { IntlShape } from 'react-intl'; import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; -import type { Auth, Topic, Message, Reaction, ReactionType, Narrow } from './api/apiTypes'; +import type { Auth, Topic, Message, Reaction, ReactionType } from './api/apiTypes'; import type { ZulipVersion } from './utils/zulipVersion'; export type * from './generics'; @@ -171,10 +171,9 @@ export type Outbox = {| */ isSent: boolean, - // These fields don't exist in `Message`. - // They're used for sending the message to the server. + // `markdownContent` doesn't exist in `Message`. + // It's used for sending the message to the server. markdownContent: string, - narrow: Narrow, // These fields are modeled on `Message`. avatar_url: string | null, diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index d9f54001801..969a9bac9a8 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -289,19 +289,12 @@ describe('isMessageInNarrow', () => { for (const [messageDescription, expected, message] of cases) { test(`${expected ? 'contains' : 'excludes'} ${messageDescription}`, () => { expect( - isMessageInNarrow({ ...message, flags: message.flags ?? [] }, narrow, ownEmail), + isMessageInNarrow(message, message.flags ?? [], narrow, ownEmail), ).toBe(expected); }); } }); } - - test('message with flags absent throws an error', () => { - const message = eg.streamMessage({ - // no flags - }); - expect(() => isMessageInNarrow(message, MENTIONED_NARROW, ownEmail)).toThrow(); - }); }); describe('getNarrowFromMessage', () => { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index a6f92090e59..c0277ab3f96 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -248,8 +248,23 @@ export const isStreamOrTopicNarrow = (narrow?: Narrow): boolean => export const isSearchNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { search: () => true }, () => false); -/** (For search narrows, just returns false.) */ -export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: string): boolean => { +/** + * True just if the given message is part of the given narrow. + * + * This function does not support search narrows, and for them always + * returns false. + * + * The message's flags must be in `flags`; `message.flags` is ignored. This + * makes it the caller's responsibility to deal with the ambiguity in our + * Message type of whether the message's flags live in a `flags` property or + * somewhere else. + */ +export const isMessageInNarrow = ( + message: Message | Outbox, + flags: $ReadOnlyArray, + narrow: Narrow, + ownEmail: string, +): boolean => { const matchPmRecipients = (emails: string[]) => { if (message.type !== 'private') { return false; @@ -262,11 +277,6 @@ export const isMessageInNarrow = (message: Message, narrow: Narrow, ownEmail: st ); }; - const { flags } = message; - if (!flags) { - throw new Error('`message.flags` should be defined.'); - } - return caseNarrow(narrow, { home: () => true, stream: name => name === message.display_recipient, @@ -294,20 +304,6 @@ export const canSendToNarrow = (narrow: Narrow): boolean => search: () => false, }); -/** True just if `haystack` contains all possible messages in `needle`. */ -export const narrowContains = (haystack: Narrow, needle: Narrow): boolean => { - if (isHomeNarrow(haystack)) { - return true; - } - if (isAllPrivateNarrow(haystack) && isPrivateOrGroupNarrow(needle)) { - return true; - } - if (isStreamNarrow(haystack) && needle[0].operand === haystack[0].operand) { - return true; - } - return JSON.stringify(needle) === JSON.stringify(haystack); -}; - export const getNarrowFromMessage = (message: Message | Outbox, ownEmail: string) => { if (Array.isArray(message.display_recipient)) { const recipient =