diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index ce317b60513..40a57b86877 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -18,7 +18,7 @@ import type { } from '../../api/modelTypes'; import { makeUserId } from '../../api/idTypes'; import type { Action, GlobalState, MessagesState, RealmState } from '../../reduxTypes'; -import type { Auth, Account, Outbox } from '../../types'; +import type { Auth, Account, OutboxBase, StreamOutbox } from '../../types'; import { UploadedAvatarURL } from '../../utils/avatar'; import { ZulipVersion } from '../../utils/zulipVersion'; import { @@ -415,8 +415,11 @@ export const makeMessagesState = (messages: Message[]): MessagesState => * (Only stream messages for now. Feel free to add PMs, if you need them.) */ -/** An outbox message with no interesting data. */ -const outboxMessageBase: $Diff = deepFreeze({ +/** + * Properties in common among PM and stream outbox messages, with no + * interesting data. + */ +const outboxMessageBase: $Diff = deepFreeze({ isOutbox: true, isSent: false, avatar_url: selfUser.avatar_url, @@ -434,11 +437,12 @@ const outboxMessageBase: $Diff = deep }); /** - * Create an outbox message from an interesting subset of its data. + * Create a stream outbox message from an interesting subset of its + * data. * * `.id` is always identical to `.timestamp` and should not be supplied. */ -export const makeOutboxMessage = (data: $Shape<$Diff>): Outbox => { +export const streamOutbox = (data: $Shape<$Diff>): StreamOutbox => { const { timestamp } = data; const outputTimestamp = timestamp ?? makeTime() / 1000; diff --git a/src/boot/store.js b/src/boot/store.js index 69ded1cf663..4b3454c9a45 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -283,6 +283,7 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = { accounts: state.accounts.filter(a => a.email !== ''), }), + // Add "open links with in-app browser" setting. '28': state => ({ ...state, settings: { @@ -291,6 +292,12 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = { }, }), + // Make `sender_id` on `Outbox` required. + '29': state => ({ + ...state, + outbox: state.outbox.filter(o => o.sender_id !== undefined), + }), + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index 8dac38d685c..76643129c79 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -24,7 +24,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('getMessagesForNarrow', () => { const message = eg.streamMessage({ id: 123 }); const messages = eg.makeMessagesState([message]); - const outboxMessage = eg.makeOutboxMessage({}); + const outboxMessage = eg.streamOutbox({}); test('if no outbox messages returns messages with no change', () => { const state = eg.reduxState({ diff --git a/src/message/__tests__/getHtmlPieceDescriptors-test.js b/src/message/__tests__/getHtmlPieceDescriptors-test.js index 61456bb9915..42c26d327d6 100644 --- a/src/message/__tests__/getHtmlPieceDescriptors-test.js +++ b/src/message/__tests__/getHtmlPieceDescriptors-test.js @@ -1,5 +1,8 @@ +/* @flow strict-local */ import deepFreeze from 'deep-freeze'; +import invariant from 'invariant'; +import * as eg from '../../__tests__/lib/exampleData'; import { HOME_NARROW } from '../../utils/narrow'; import getHtmlPieceDescriptors from '../getHtmlPieceDescriptors'; @@ -12,13 +15,7 @@ describe('getHtmlPieceDescriptors', () => { }); test('renders time, header and message for a single input', () => { - const messages = deepFreeze([ - { - timestamp: 123, - avatar_url: '', - id: 12345, - }, - ]); + const messages = deepFreeze([{ ...eg.streamMessage({ id: 12345 }), timestamp: 123 }]); const htmlPieceDescriptors = getHtmlPieceDescriptors(messages, narrow); @@ -28,115 +25,59 @@ describe('getHtmlPieceDescriptors', () => { }); test('several messages in same stream, from same person result in time row, header for the stream, three messages, only first of which is full detail', () => { + const stream = eg.stream; + const sender = eg.otherUser; + const messages = deepFreeze([ - { - timestamp: 123, - type: 'stream', - id: 1, - sender_email: 'john@example.com', - sender_full_name: 'John Doe', - display_recipient: 'general', - subject: '', - avatar_url: '', - }, - { - timestamp: 124, - type: 'stream', - id: 2, - sender_email: 'john@example.com', - sender_full_name: 'John Doe', - display_recipient: 'general', - subject: '', - avatar_url: '', - }, - { - timestamp: 125, - type: 'stream', - id: 3, - sender_email: 'john@example.com', - sender_full_name: 'John Doe', - display_recipient: 'general', - subject: '', - avatar_url: '', - }, + eg.streamMessage({ stream, sender, id: 1 }), + eg.streamMessage({ stream, sender, id: 2 }), + eg.streamMessage({ stream, sender, id: 3 }), ]); const htmlPieceDescriptors = getHtmlPieceDescriptors(messages, narrow); const messageKeys = htmlPieceDescriptors[1].data.map(x => x.key); - const messageBriefs = htmlPieceDescriptors[1].data.map(x => x.isBrief); + const messageBriefs = htmlPieceDescriptors[1].data.map(x => { + invariant(x.type === 'message', 'expected message item'); + return x.isBrief; + }); expect(messageKeys).toEqual([1, 2, 3]); expect(messageBriefs).toEqual([false, true, true]); }); test('several messages in same stream, from different people result in time row, header for the stream, three messages, only all full detail', () => { + const stream = eg.stream; + const messages = deepFreeze([ - { - timestamp: 123, - type: 'stream', - id: 1, - sender_email: 'john@example.com', - sender_full_name: 'John', - display_recipient: 'general', - subject: '', - avatar_url: '', - }, - { - timestamp: 124, - type: 'stream', - id: 2, - sender_email: 'mark@example.com', - sender_full_name: 'Mark', - display_recipient: 'general', - subject: '', - avatar_url: '', - }, - { - timestamp: 125, - type: 'stream', - id: 3, - sender_email: 'peter@example.com', - sender_full_name: 'Peter', - display_recipient: 'general', - subject: '', - avatar_url: '', - }, + eg.streamMessage({ stream, sender: eg.selfUser, id: 1 }), + eg.streamMessage({ stream, sender: eg.otherUser, id: 2 }), + eg.streamMessage({ stream, sender: eg.thirdUser, id: 3 }), ]); const htmlPieceDescriptors = getHtmlPieceDescriptors(messages, narrow); const messageKeys = htmlPieceDescriptors[1].data.map(x => x.key); - const messageBriefs = htmlPieceDescriptors[1].data.map(x => x.isBrief); + const messageBriefs = htmlPieceDescriptors[1].data.map(x => { + invariant(x.type === 'message', 'expected message item'); + return x.isBrief; + }); expect(messageKeys).toEqual([1, 2, 3]); expect(messageBriefs).toEqual([false, false, false]); }); test('private messages between two people, results in time row, header and two full messages', () => { const messages = deepFreeze([ - { - timestamp: 123, - type: 'private', - id: 1, - sender_email: 'john@example.com', - sender_full_name: 'John', - avatar_url: '', - display_recipient: [{ email: 'john@example.com' }, { email: 'mark@example.com' }], - }, - { - timestamp: 123, - type: 'private', - id: 2, - sender_email: 'mark@example.com', - sender_full_name: 'Mark', - avatar_url: '', - display_recipient: [{ email: 'john@example.com' }, { email: 'mark@example.com' }], - }, + eg.pmMessage({ sender: eg.selfUser, recipients: [eg.selfUser, eg.otherUser], id: 1 }), + eg.pmMessage({ sender: eg.otherUser, recipients: [eg.selfUser, eg.otherUser], id: 2 }), ]); const htmlPieceDescriptors = getHtmlPieceDescriptors(messages, narrow); const messageKeys = htmlPieceDescriptors[1].data.map(x => x.key); - const messageBriefs = htmlPieceDescriptors[1].data.map(x => x.isBrief); + const messageBriefs = htmlPieceDescriptors[1].data.map(x => { + invariant(x.type === 'message', 'expected message item'); + return x.isBrief; + }); expect(messageKeys).toEqual([1, 2]); expect(messageBriefs).toEqual([false, false]); }); diff --git a/src/message/getHtmlPieceDescriptors.js b/src/message/getHtmlPieceDescriptors.js index 089b4c5588e..57446a841f1 100644 --- a/src/message/getHtmlPieceDescriptors.js +++ b/src/message/getHtmlPieceDescriptors.js @@ -34,15 +34,8 @@ export default ( }); } - // TODO(#3764): Use sender_id, not sender_email. Needs making - // Outbox#sender_id required; which needs a migration to drop Outbox - // values that lack it; which is fine once the release that adds it - // has been out for a few weeks. const shouldGroupWithPrev = - !diffRecipient - && !diffDays - && !!prevMessage - && prevMessage.sender_email === message.sender_email; + !diffRecipient && !diffDays && !!prevMessage && prevMessage.sender_id === message.sender_id; sections[sections.length - 1].data.push({ key: message.id, diff --git a/src/outbox/__tests__/outboxReducer-test.js b/src/outbox/__tests__/outboxReducer-test.js index 168692d432f..4dbf7cf51ac 100644 --- a/src/outbox/__tests__/outboxReducer-test.js +++ b/src/outbox/__tests__/outboxReducer-test.js @@ -9,9 +9,9 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('outboxReducer', () => { describe('INITIAL_FETCH_COMPLETE', () => { test('filters out isSent', () => { - const message1 = eg.makeOutboxMessage({ content: 'New one' }); - const message2 = eg.makeOutboxMessage({ content: 'Another one' }); - const message3 = eg.makeOutboxMessage({ content: 'Message already sent', isSent: true }); + const message1 = eg.streamOutbox({ content: 'New one' }); + const message2 = eg.streamOutbox({ content: 'Another one' }); + const message3 = eg.streamOutbox({ content: 'Message already sent', isSent: true }); const initialState = deepFreeze([message1, message2, message3]); const action = deepFreeze({ @@ -28,7 +28,7 @@ describe('outboxReducer', () => { describe('MESSAGE_SEND_START', () => { test('add a new message to the outbox', () => { - const message = eg.makeOutboxMessage({ content: 'New one' }); + const message = eg.streamOutbox({ content: 'New one' }); const initialState = deepFreeze([]); @@ -45,8 +45,8 @@ describe('outboxReducer', () => { }); test('do not add a message with a duplicate timestamp to the outbox', () => { - const message1 = eg.makeOutboxMessage({ content: 'hello', timestamp: 123 }); - const message2 = eg.makeOutboxMessage({ content: 'hello twice', timestamp: 123 }); + const message1 = eg.streamOutbox({ content: 'hello', timestamp: 123 }); + const message2 = eg.streamOutbox({ content: 'hello twice', timestamp: 123 }); const initialState = deepFreeze([message1]); @@ -63,7 +63,7 @@ describe('outboxReducer', () => { describe('EVENT_NEW_MESSAGE', () => { test('do not mutate state if a message is not removed', () => { - const initialState = deepFreeze([eg.makeOutboxMessage({ timestamp: 546 })]); + const initialState = deepFreeze([eg.streamOutbox({ timestamp: 546 })]); const message = eg.streamMessage({ timestamp: 123 }); @@ -78,9 +78,9 @@ describe('outboxReducer', () => { }); test('remove message if local_message_id matches', () => { - const message1 = eg.makeOutboxMessage({ timestamp: 546 }); - const message2 = eg.makeOutboxMessage({ timestamp: 150238512430 }); - const message3 = eg.makeOutboxMessage({ timestamp: 150238594540 }); + const message1 = eg.streamOutbox({ timestamp: 546 }); + const message2 = eg.streamOutbox({ timestamp: 150238512430 }); + const message3 = eg.streamOutbox({ timestamp: 150238594540 }); const initialState = deepFreeze([message1, message2, message3]); const action = deepFreeze({ @@ -97,9 +97,9 @@ describe('outboxReducer', () => { }); test("remove nothing if local_message_id doesn't match", () => { - const message1 = eg.makeOutboxMessage({ timestamp: 546 }); - const message2 = eg.makeOutboxMessage({ timestamp: 150238512430 }); - const message3 = eg.makeOutboxMessage({ timestamp: 150238594540 }); + const message1 = eg.streamOutbox({ timestamp: 546 }); + const message2 = eg.streamOutbox({ timestamp: 150238512430 }); + const message3 = eg.streamOutbox({ timestamp: 150238594540 }); const initialState = deepFreeze([message1, message2, message3]); const action = deepFreeze({ diff --git a/src/types.js b/src/types.js index ee301f7c487..a2ec2429ecc 100644 --- a/src/types.js +++ b/src/types.js @@ -3,7 +3,15 @@ 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, UserId } from './api/apiTypes'; +import type { + Auth, + Topic, + Message, + PmMessage, + StreamMessage, + ReactionType, + UserId, +} from './api/apiTypes'; import type { ZulipVersion } from './utils/zulipVersion'; import type { PmKeyUsers } from './utils/recipient'; @@ -145,27 +153,10 @@ export type TopicExtended = {| |}; /** - * A message we're in the process of sending. - * - * We use these objects for two purposes: - * - * (a) They make up the queue of messages the user has asked us to send, and - * which we'll retry sending if initial attempts fail. See - * `trySendMessages`. - * - * (b) We show them immediately in the message list, even before we've - * successfully gotten them to the server (but with an activity - * indicator to show we're still working on them.) - * - * Even after (a) is complete for a given message, we still need the - * `Outbox` object for the sake of (b), until we hear an `EVENT_NEW_MESSAGE` - * event from the server that lets us replace it with the corresponding - * `Message` object. - * - * This type most often appears in the union `Message | Outbox`, and so its - * properties are deliberately similar to those of `Message`. + * Properties in common among the two different flavors of a + * `Outbox`: `PmOutbox` and `StreamOutbox`. */ -export type Outbox = $ReadOnly<{| +export type OutboxBase = $ReadOnly<{| /** Used for distinguishing from a `Message` object. */ isOutbox: true, @@ -182,16 +173,10 @@ export type Outbox = $ReadOnly<{| // It's used for sending the message to the server. markdownContent: string, - // The remaining fields are modeled on `Message`. - - // TODO(#3764): Make sender_id required. Needs a migration to drop Outbox - // 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?: UserId, - /* eslint-disable flowtype/generic-spacing */ ...SubsetProperties< + // Could use `MessageBase` here, but Flow would complain anyway if + // we tried to put something that's not in `MessageBase` below. Message, {| avatar_url: mixed, @@ -199,6 +184,7 @@ export type Outbox = $ReadOnly<{| display_recipient: mixed, id: mixed, reactions: mixed, + sender_id: mixed, sender_email: mixed, sender_full_name: mixed, subject: mixed, @@ -208,6 +194,51 @@ export type Outbox = $ReadOnly<{| >, |}>; +export type PmOutbox = {| + ...OutboxBase, + + ...SubsetProperties, +|}; + +export type StreamOutbox = {| + ...OutboxBase, + + // TODO(#3764): Make stream_id required. Needs a migration to drop + // StreamOutbox values that lack it; that'll be fine once the + // release that adds it has been out for a few weeks. (Also drop + // the hack line about it in MessageLike.) + // + // Once it is required, it + // should go in the second type argument passed to + // `SubsetProperties` of `StreamMessage`, below. + stream_id?: number, + + ...SubsetProperties, +|}; + +/** + * A message we're in the process of sending. + * + * We use these objects for two purposes: + * + * (a) They make up the queue of messages the user has asked us to send, and + * which we'll retry sending if initial attempts fail. See + * `trySendMessages`. + * + * (b) We show them immediately in the message list, even before we've + * successfully gotten them to the server (but with an activity + * indicator to show we're still working on them.) + * + * Even after (a) is complete for a given message, we still need the + * `Outbox` object for the sake of (b), until we hear an `EVENT_NEW_MESSAGE` + * event from the server that lets us replace it with the corresponding + * `Message` object. + * + * This type most often appears in the union `Message | Outbox`, and so its + * properties are deliberately similar to those of `Message`. + */ +export type Outbox = PmOutbox | StreamOutbox; + /** * MessageLike: Imprecise alternative to `Message | Outbox`. * @@ -241,7 +272,7 @@ export type MessageLike = | $ReadOnly<{| // $Shape is unsound, per Flow docs, but $ReadOnly<$Shape> is not ...$Shape<{| [$Keys]: void |}>, - sender_id?: UserId, // TODO: Drop this once required in Outbox. + stream_id?: number, // TODO: Drop this once required in StreamOutbox. ...Outbox, |}>;