diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index d2dd8b554ec..e1ee73534c9 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -6,6 +6,8 @@ import Immutable from 'immutable'; import type { CrossRealmBot, Message, + PmMessage, + StreamMessage, PmRecipientUser, Reaction, Stream, @@ -296,7 +298,6 @@ const messagePropertiesBase = deepFreeze({ is_me_message: false, // last_edit_timestamp omitted reactions: [], - subject_links: [], submessages: [], }); @@ -324,11 +325,11 @@ const randMessageId: () => number = makeUniqueRandInt('message ID', 10000000); * Beware! These values may not be representative. */ export const pmMessage = (args?: {| - ...$Rest, + ...$Rest, sender?: User, recipients?: User[], sender_id?: number, // accept a plain number, for convenience in tests -|}): Message => { +|}): PmMessage => { // The `Object.freeze` is to work around a Flow issue: // https://github.com/facebook/flow/issues/2386#issuecomment-695064325 const { @@ -338,7 +339,7 @@ export const pmMessage = (args?: {| ...extra } = args ?? Object.freeze({}); - const baseMessage: Message = { + const baseMessage: PmMessage = { ...messagePropertiesBase, ...messagePropertiesFromSender(sender), @@ -348,8 +349,6 @@ export const pmMessage = (args?: {| // in real messages. (See comments on the Message type.) display_recipient: recipients.map(displayRecipientFromUser), id: randMessageId(), - recipient_id: 2342, - stream_id: -1, subject: '', timestamp: 1556579160, type: 'private', @@ -361,13 +360,15 @@ export const pmMessage = (args?: {| }); }; -export const pmMessageFromTo = (from: User, to: User[], extra?: $Rest): Message => - pmMessage({ sender: from, recipients: [from, ...to], ...extra }); +export const pmMessageFromTo = ( + from: User, + to: User[], + extra?: $Rest, +): PmMessage => pmMessage({ sender: from, recipients: [from, ...to], ...extra }); const messagePropertiesFromStream = (stream1: Stream) => { const { stream_id, name: display_recipient } = stream1; return deepFreeze({ - recipient_id: 2567, display_recipient, stream_id, }); @@ -379,15 +380,15 @@ const messagePropertiesFromStream = (stream1: Stream) => { * Beware! These values may not be representative. */ export const streamMessage = (args?: {| - ...$Rest, + ...$Rest, stream?: Stream, sender?: User, -|}): Message => { +|}): StreamMessage => { // The `Object.freeze` is to work around a Flow issue: // https://github.com/facebook/flow/issues/2386#issuecomment-695064325 const { stream: streamInner = stream, sender = otherUser, ...extra } = args ?? Object.freeze({}); - const baseMessage: Message = { + const baseMessage: StreamMessage = { ...messagePropertiesBase, ...messagePropertiesFromSender(sender), ...messagePropertiesFromStream(streamInner), @@ -396,6 +397,7 @@ export const streamMessage = (args?: {| content_type: 'text/markdown', id: randMessageId(), subject: 'example topic', + subject_links: [], timestamp: 1556579727, type: 'stream', }; diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index d230106409b..7c152cb6454 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -2,7 +2,7 @@ import type { Auth, ApiResponseSuccess } from '../transportTypes'; import type { Identity } from '../../types'; import type { Message, ApiNarrow } from '../apiTypes'; -import type { Reaction, UserId } from '../modelTypes'; +import type { PmMessage, StreamMessage, Reaction, UserId } from '../modelTypes'; import { apiGet } from '../apiFetch'; import { identityOfAuth } from '../../account/accountMisc'; import { AvatarURL } from '../../utils/avatar'; @@ -31,12 +31,16 @@ export type ServerReaction = $ReadOnly<{| |}>, |}>; -export type ServerMessage = $ReadOnly<{| - ...$Exact, +// How `ServerMessage` relates to `Message`, in a way that applies +// uniformly to `Message`'s subtypes. +type ServerMessageOf = $ReadOnly<{| + ...$Exact, avatar_url: string | null, reactions: $ReadOnlyArray, |}>; +export type ServerMessage = ServerMessageOf | ServerMessageOf; + // The actual response from the server. We convert the data from this to // `ApiResponseMessages` before returning it to application code. type ServerApiResponseMessages = {| @@ -45,27 +49,26 @@ type ServerApiResponseMessages = {| |}; /** Exported for tests only. */ -export const migrateMessages = (messages: ServerMessage[], identity: Identity): Message[] => - messages.map(message => { - const { reactions, avatar_url: rawAvatarUrl, ...restMessage } = message; - - return { - ...restMessage, - avatar_url: AvatarURL.fromUserOrBotData({ - rawAvatarUrl, - email: message.sender_email, - userId: message.sender_id, - realm: identity.realm, - }), - reactions: reactions.map(reaction => { - const { user, ...restReaction } = reaction; - return { - ...restReaction, - user_id: user.id, - }; - }), - }; - }); +export const migrateMessages = ( + messages: $ReadOnlyArray, + identity: Identity, +): Message[] => + messages.map((message: ServerMessageOf): M => ({ + ...message, + avatar_url: AvatarURL.fromUserOrBotData({ + rawAvatarUrl: message.avatar_url, + email: message.sender_email, + userId: message.sender_id, + realm: identity.realm, + }), + reactions: message.reactions.map(reaction => { + const { user, ...restReaction } = reaction; + return { + ...restReaction, + user_id: user.id, + }; + }), + })); const migrateResponse = (response, identity: Identity) => { const { messages, ...restResponse } = response; diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 3d14bddd8e3..8165c5b07f8 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -445,48 +445,10 @@ export type Submessage = $ReadOnly<{| |}>; /** - * A Zulip message. - * - * This type is mainly intended to represent the data the server sends as - * the `message` property of an event of type `message`. Caveat lector: we - * pass these around to a lot of places, and do a lot of further munging, so - * this type may not quite represent that. Any differences should - * definitely be commented, and perhaps refactored. - * - * The server's behavior here is undocumented and the source is very - * complex; this is naturally a place where a large fraction of all the - * features of Zulip have to appear. - * - * Major appearances of this type include - * * `message: Message` on a server event of type `message`, and our - * `EVENT_NEW_MESSAGE` Redux action for the event; - * * `messages: Message[]` in a `/messages` (our `getMessages`) response, - * and our resulting `MESSAGE_FETCH_COMPLETE` Redux action; - * * `messages: {| [id]: Message |}` in our global Redux state. - * - * References include: - * * the two example events at https://zulip.com/api/get-events-from-queue - * * `process_message_event` in zerver/tornado/event_queue.py; the call - * `client.add_event(user_event)` makes the final determination of what - * goes into the event, so `message_dict` is the final value of `message` - * * `MessageDict.wide_dict` and its helpers in zerver/lib/message.py; - * via `do_send_messages` in `zerver/lib/actions.py`, these supply most - * of the data ultimately handled by `process_message_event` - * * `messages_for_ids` and its helpers in zerver/lib/message.py; via - * `get_messages_backend`, these supply the data ultimately returned by - * `/messages` - * * the `Message` and `AbstractMessage` models in zerver/models.py, but - * with caution; many fields are adjusted between the DB row and the event - * * empirical study looking at Redux events logged [to the - * console](docs/howto/debugging.md). - * - * See also `Outbox`, which is deliberately similar so that we can use - * the type `Message | Outbox` in many places. - * - * See also `MessagesState` for discussion of how we fetch and store message - * data. + * Properties in common among the two different flavors of a + * `Message`: `PmMessage` and `StreamMessage`. */ -export type Message = $ReadOnly<{| +type MessageBase = $ReadOnly<{| /** Our own flag; if true, really type `Outbox`. */ isOutbox: false, @@ -544,10 +506,14 @@ export type Message = $ReadOnly<{| timestamp: number, - // - // Properties that behave differently for stream vs. private messages. + /** Deprecated; a server implementation detail not useful in a client. */ + // recipient_id: number, +|}>; - type: 'stream' | 'private', +export type PmMessage = $ReadOnly<{| + ...MessageBase, + + type: 'private', // Notes from studying the server code: // * Notes are primarily from the server as of 2020-04 at cb85763c7, but @@ -567,27 +533,81 @@ export type Message = $ReadOnly<{| // it sorted by user ID; so, best not to assume current behavior. // /** - * The set of all users in the thread, for a PM; else the stream name. + * The set of all users in the thread. * - * For a private message, this lists the sender as well as all (other) - * recipients, and it lists each user just once. In particular the - * self-user is always included. + * This lists the sender as well as all (other) recipients, and it + * lists each user just once. In particular the self-user is always + * included. * * The ordering is less well specified; if it matters, sort first. - * - * For stream messages, prefer `stream_id`; see #3918. */ - display_recipient: string | $ReadOnlyArray, // `string` for type stream, else PmRecipientUser[] + display_recipient: $ReadOnlyArray, - /** Deprecated; a server implementation detail not useful in a client. */ - recipient_id: number, + subject: '', +|}>; + +export type StreamMessage = $ReadOnly<{| + ...MessageBase, - stream_id: number, // FixMe: actually only for type `stream`, else absent. + type: 'stream', + + /** + * The stream name. + * + * Prefer `stream_id`; see #3918. + */ + display_recipient: string, + + stream_id: number, subject: string, subject_links: $ReadOnlyArray, |}>; +/** + * A Zulip message. + * + * This type is mainly intended to represent the data the server sends as + * the `message` property of an event of type `message`. Caveat lector: we + * pass these around to a lot of places, and do a lot of further munging, so + * this type may not quite represent that. Any differences should + * definitely be commented, and perhaps refactored. + * + * The server's behavior here is undocumented and the source is very + * complex; this is naturally a place where a large fraction of all the + * features of Zulip have to appear. + * + * Major appearances of this type include + * * `message: Message` on a server event of type `message`, and our + * `EVENT_NEW_MESSAGE` Redux action for the event; + * * `messages: Message[]` in a `/messages` (our `getMessages`) response, + * and our resulting `MESSAGE_FETCH_COMPLETE` Redux action; + * * `messages: {| [id]: Message |}` in our global Redux state. + * + * References include: + * * the two example events at https://zulip.com/api/get-events-from-queue + * * `process_message_event` in zerver/tornado/event_queue.py; the call + * `client.add_event(user_event)` makes the final determination of what + * goes into the event, so `message_dict` is the final value of `message` + * * `MessageDict.wide_dict` and its helpers in zerver/lib/message.py; + * via `do_send_messages` in `zerver/lib/actions.py`, these supply most + * of the data ultimately handled by `process_message_event` + * * `messages_for_ids` and its helpers in zerver/lib/message.py; via + * `get_messages_backend`, these supply the data ultimately returned by + * `/messages` + * * the `Message` and `AbstractMessage` models in zerver/models.py, but + * with caution; many fields are adjusted between the DB row and the event + * * empirical study looking at Redux events logged [to the + * console](docs/howto/debugging.md). + * + * See also `Outbox`, which is deliberately similar so that we can use + * the type `Message | Outbox` in many places. + * + * See also `MessagesState` for discussion of how we fetch and store message + * data. + */ +export type Message = PmMessage | StreamMessage; + // // // diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index f65c13363ae..a631aca77c9 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -2,7 +2,6 @@ import invariant from 'invariant'; import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; -import omit from 'lodash.omit'; import Immutable from 'immutable'; import type { GlobalState } from '../../reduxTypes'; @@ -15,7 +14,6 @@ import { tryFetch, } from '../fetchActions'; import { FIRST_UNREAD_ANCHOR } from '../../anchor'; -import type { Message } from '../../api/modelTypes'; import type { ServerMessage } from '../../api/messages/getMessages'; import { streamNarrow, HOME_NARROW, HOME_NARROW_STR, keyFromNarrow } from '../../utils/narrow'; import { GravatarURL } from '../../utils/avatar'; @@ -118,15 +116,13 @@ describe('fetchActions', () => { }; const message1 = eg.streamMessage({ id: 1, sender }); - type CommonFields = $Diff; - // message1 exactly as we receive it from the server, before our // own transformations. // // TODO: Deduplicate this logic with similar logic in // migrateMessages-test.js. const serverMessage1: ServerMessage = { - ...(omit(message1, 'reactions', 'avatar_url'): CommonFields), + ...message1, reactions: [], avatar_url: null, // Null in server data will be transformed to a GravatarURL }; diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js index 93a13910dbd..59bca0f0976 100644 --- a/src/message/messagesReducer.js +++ b/src/message/messagesReducer.js @@ -2,7 +2,7 @@ import omit from 'lodash.omit'; import Immutable from 'immutable'; -import type { MessagesState, Action } from '../types'; +import type { MessagesState, Message, Action } from '../types'; import { REALM_INIT, LOGOUT, @@ -83,9 +83,9 @@ export default (state: MessagesState = initialState, action: Action): MessagesSt case EVENT_REACTION_ADD: return state.update( action.message_id, - oldMessage => + (oldMessage: M): M => oldMessage && { - ...oldMessage, + ...(oldMessage: M), reactions: oldMessage.reactions.concat({ emoji_name: action.emoji_name, user_id: action.user_id, @@ -98,9 +98,9 @@ export default (state: MessagesState = initialState, action: Action): MessagesSt case EVENT_REACTION_REMOVE: return state.update( action.message_id, - oldMessage => + (oldMessage: M): M => oldMessage && { - ...oldMessage, + ...(oldMessage: M), reactions: oldMessage.reactions.filter( x => !(x.emoji_name === action.emoji_name && x.user_id === action.user_id), ), @@ -113,9 +113,9 @@ export default (state: MessagesState = initialState, action: Action): MessagesSt case EVENT_SUBMESSAGE: return state.update( action.message_id, - message => + (message: M): M => message && { - ...message, + ...(message: M), submessages: [ ...(message.submessages || []), { @@ -133,40 +133,49 @@ export default (state: MessagesState = initialState, action: Action): MessagesSt return state.deleteAll(action.messageIds); case EVENT_UPDATE_MESSAGE: - return state.update( - action.message_id, - oldMessage => - oldMessage && { - ...oldMessage, - content: action.rendered_content || oldMessage.content, - subject: action.subject || oldMessage.subject, - subject_links: action.subject_links || oldMessage.subject_links, - edit_history: [ - action.orig_rendered_content - ? action.orig_subject !== undefined - ? { - prev_rendered_content: action.orig_rendered_content, - prev_subject: oldMessage.subject, - timestamp: action.edit_timestamp, - prev_rendered_content_version: action.prev_rendered_content_version, - user_id: action.user_id, - } - : { - prev_rendered_content: action.orig_rendered_content, - timestamp: action.edit_timestamp, - prev_rendered_content_version: action.prev_rendered_content_version, - user_id: action.user_id, - } - : { + return state.update(action.message_id, (oldMessage: M): M => { + if (!oldMessage) { + return oldMessage; + } + const messageWithNewCommonFields: M = { + ...(oldMessage: M), + content: action.rendered_content || oldMessage.content, + edit_history: [ + action.orig_rendered_content + ? action.orig_subject !== undefined + ? { + prev_rendered_content: action.orig_rendered_content, prev_subject: oldMessage.subject, timestamp: action.edit_timestamp, + prev_rendered_content_version: action.prev_rendered_content_version, user_id: action.user_id, - }, - ...(oldMessage.edit_history || NULL_ARRAY), - ], - last_edit_timestamp: action.edit_timestamp, - }, - ); + } + : { + prev_rendered_content: action.orig_rendered_content, + timestamp: action.edit_timestamp, + prev_rendered_content_version: action.prev_rendered_content_version, + user_id: action.user_id, + } + : { + prev_subject: oldMessage.subject, + timestamp: action.edit_timestamp, + user_id: action.user_id, + }, + ...(oldMessage.edit_history || NULL_ARRAY), + ], + last_edit_timestamp: action.edit_timestamp, + }; + + return messageWithNewCommonFields.type === 'stream' + ? { + ...messageWithNewCommonFields, + subject: action.subject || messageWithNewCommonFields.subject, + subject_links: action.subject_links || messageWithNewCommonFields.subject_links, + } + : { + ...messageWithNewCommonFields, + }; + }); default: return state;