diff --git a/src/api/__tests__/rawModelTypes-test.js b/src/api/__tests__/rawModelTypes-test.js new file mode 100644 index 00000000000..5d0f82932c2 --- /dev/null +++ b/src/api/__tests__/rawModelTypes-test.js @@ -0,0 +1,116 @@ +/* @flow strict-local */ +import { + transformFetchedMessage, + type FetchedMessage, + type FetchedReaction, +} from '../rawModelTypes'; +import { identityOfAuth } from '../../account/accountMisc'; +import * as eg from '../../__tests__/lib/exampleData'; +import type { Message } from '../modelTypes'; +import { GravatarURL } from '../../utils/avatar'; + +describe('transformFetchedMessage', () => { + const reactingUser = eg.makeUser(); + + const serverReaction: FetchedReaction = { + emoji_name: '+1', + reaction_type: 'unicode_emoji', + emoji_code: '1f44d', + user: { + email: reactingUser.email, + full_name: reactingUser.full_name, + id: reactingUser.user_id, + }, + }; + + const fetchedMessage: FetchedMessage = { + ...eg.streamMessage(), + reactions: [serverReaction], + avatar_url: null, + edit_history: [ + { + prev_content: 'foo', + prev_rendered_content: '

foo

', + prev_stream: eg.stream.stream_id, + prev_topic: 'bar', + stream: eg.otherStream.stream_id, + timestamp: 0, + topic: 'bar!', + user_id: eg.selfUser.user_id, + }, + ], + }; + + describe('recent server', () => { + const input: FetchedMessage = fetchedMessage; + + const expectedOutput: Message = { + ...fetchedMessage, + reactions: [ + { + user_id: reactingUser.user_id, + emoji_name: serverReaction.emoji_name, + reaction_type: serverReaction.reaction_type, + emoji_code: serverReaction.emoji_code, + }, + ], + avatar_url: GravatarURL.validateAndConstructInstance({ email: fetchedMessage.sender_email }), + edit_history: + // $FlowIgnore[incompatible-cast] - See MessageEdit type + (fetchedMessage.edit_history: Message['edit_history']), + }; + + const actualOutput: Message = transformFetchedMessage( + input, + identityOfAuth(eg.selfAuth), + eg.recentZulipFeatureLevel, + true, + ); + + test('In reactions, replace user object with `user_id`', () => { + expect(actualOutput.reactions).toEqual(expectedOutput.reactions); + }); + + test('Converts avatar_url correctly', () => { + expect(actualOutput.avatar_url).toEqual(expectedOutput.avatar_url); + }); + + test('Keeps edit_history, if allowEditHistory is true', () => { + expect(actualOutput.edit_history).toEqual(expectedOutput.edit_history); + }); + + test('Drops edit_history, if allowEditHistory is false', () => { + expect( + transformFetchedMessage( + input, + identityOfAuth(eg.selfAuth), + eg.recentZulipFeatureLevel, + false, + ).edit_history, + ).toEqual(null); + }); + }); + + describe('drops edit_history from pre-118 server', () => { + expect( + transformFetchedMessage( + { + ...fetchedMessage, + edit_history: [ + { + prev_content: 'foo', + prev_rendered_content: '

foo

', + prev_stream: eg.stream.stream_id, + prev_subject: 'bar', + timestamp: 0, + user_id: eg.selfUser.user_id, + }, + ], + }, + identityOfAuth(eg.selfAuth), + 117, + true, + ).edit_history, + ).toEqual(null); + }); +}); diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index db852c4da7a..29d57e299c1 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -99,6 +99,10 @@ export type CustomProfileFieldsEvent = {| export type MessageEvent = $ReadOnly<{| ...EventCommon, type: typeof EventTypes.message, + + // TODO: This doesn't describe what we get from the server (see, e.g., + // avatar_url). Write a type that does; perhaps it can go in + // rawModelTypes.js. message: Message, /** See the same-named property on `Message`. */ diff --git a/src/api/index.js b/src/api/index.js index be8d8f7aa6a..a84055978c8 100644 --- a/src/api/index.js +++ b/src/api/index.js @@ -30,6 +30,7 @@ import deleteMessage from './messages/deleteMessage'; import deleteTopic from './messages/deleteTopic'; import getRawMessageContent from './messages/getRawMessageContent'; import getMessages from './messages/getMessages'; +import getSingleMessage from './messages/getSingleMessage'; import getMessageHistory from './messages/getMessageHistory'; import messagesFlags from './messages/messagesFlags'; import sendMessage from './messages/sendMessage'; @@ -78,6 +79,7 @@ export { deleteTopic, getRawMessageContent, getMessages, + getSingleMessage, getMessageHistory, messagesFlags, sendMessage, diff --git a/src/api/messages/__tests__/migrateMessages-test.js b/src/api/messages/__tests__/migrateMessages-test.js deleted file mode 100644 index 1e3913598e8..00000000000 --- a/src/api/messages/__tests__/migrateMessages-test.js +++ /dev/null @@ -1,112 +0,0 @@ -/* @flow strict-local */ -import { migrateMessages } from '../getMessages'; -import { identityOfAuth } from '../../../account/accountMisc'; -import * as eg from '../../../__tests__/lib/exampleData'; -import type { ServerMessage, ServerReaction } from '../getMessages'; -import type { Message } from '../../modelTypes'; -import { GravatarURL } from '../../../utils/avatar'; - -const reactingUser = eg.makeUser(); - -const serverReaction: ServerReaction = { - emoji_name: '+1', - reaction_type: 'unicode_emoji', - emoji_code: '1f44d', - user: { - email: reactingUser.email, - full_name: reactingUser.full_name, - id: reactingUser.user_id, - }, -}; - -const serverMessage: ServerMessage = { - ...eg.streamMessage(), - reactions: [serverReaction], - avatar_url: null, - edit_history: [ - { - prev_content: 'foo', - prev_rendered_content: '

foo

', - prev_stream: eg.stream.stream_id, - prev_topic: 'bar', - stream: eg.otherStream.stream_id, - timestamp: 0, - topic: 'bar!', - user_id: eg.selfUser.user_id, - }, - ], -}; - -describe('recent server', () => { - const input: $ReadOnlyArray = [serverMessage]; - - const expectedOutput: $ReadOnlyArray = [ - { - ...serverMessage, - reactions: [ - { - user_id: reactingUser.user_id, - emoji_name: serverReaction.emoji_name, - reaction_type: serverReaction.reaction_type, - emoji_code: serverReaction.emoji_code, - }, - ], - avatar_url: GravatarURL.validateAndConstructInstance({ email: serverMessage.sender_email }), - edit_history: - // $FlowIgnore[incompatible-cast] - See MessageEdit type - (serverMessage.edit_history: Message['edit_history']), - }, - ]; - - const actualOutput: $ReadOnlyArray = migrateMessages( - input, - identityOfAuth(eg.selfAuth), - eg.recentZulipFeatureLevel, - true, - ); - - test('In reactions, replace user object with `user_id`', () => { - expect(actualOutput.map(m => m.reactions)).toEqual(expectedOutput.map(m => m.reactions)); - }); - - test('Converts avatar_url correctly', () => { - expect(actualOutput.map(m => m.avatar_url)).toEqual(expectedOutput.map(m => m.avatar_url)); - }); - - test('Keeps edit_history, if allowEditHistory is true', () => { - expect(actualOutput.map(m => m.edit_history)).toEqual(expectedOutput.map(m => m.edit_history)); - }); - - test('Drops edit_history, if allowEditHistory is false', () => { - expect( - migrateMessages(input, identityOfAuth(eg.selfAuth), eg.recentZulipFeatureLevel, false).map( - m => m.edit_history, - ), - ).toEqual(expectedOutput.map(m => null)); - }); -}); - -describe('drops edit_history from pre-118 server', () => { - expect( - migrateMessages( - [ - { - ...serverMessage, - edit_history: [ - { - prev_content: 'foo', - prev_rendered_content: '

foo

', - prev_stream: eg.stream.stream_id, - prev_subject: 'bar', - timestamp: 0, - user_id: eg.selfUser.user_id, - }, - ], - }, - ], - identityOfAuth(eg.selfAuth), - 117, - true, - ).map(m => m.edit_history), - ).toEqual([null]); -}); diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index 048acb01916..651b5944c89 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -2,10 +2,9 @@ import type { Auth, ApiResponseSuccess } from '../transportTypes'; import type { Identity } from '../../types'; import type { Message, ApiNarrow } from '../apiTypes'; -import type { PmMessage, StreamMessage, Reaction, UserId, MessageEdit } from '../modelTypes'; +import { transformFetchedMessage, type FetchedMessage } from '../rawModelTypes'; import { apiGet } from '../apiFetch'; import { identityOfAuth } from '../../account/accountMisc'; -import { AvatarURL } from '../../utils/avatar'; type ApiResponseMessages = {| ...$Exact, @@ -16,102 +15,20 @@ type ApiResponseMessages = {| messages: $ReadOnlyArray, |}; -/** - * The variant of `Reaction` found in the actual server response. - * - * Note that reaction events have a *different* variation; see their - * handling in `eventToAction`. - */ -// We shouldn't have to rely on this format on servers at feature -// level 2+; those newer servers include a top-level `user_id` field -// in addition to the `user` object. See #4072. -// TODO(server-3.0): Simplify this away. -export type ServerReaction = $ReadOnly<{| - ...$Diff, - user: $ReadOnly<{| - email: string, - full_name: string, - id: UserId, - |}>, -|}>; - -/** - * The elements of Message.edit_history found in the actual server response. - * - * Accurate for supported servers before and after FL 118. For convenience, - * we drop objects of this type if the FL is <118, so that the modern shape - * at Message.edit_history is the only shape we store in Redux; see there. - */ -// TODO(server-5.0): Simplify this away. -export type ServerMessageEdit = $ReadOnly<{| - prev_content?: string, - prev_rendered_content?: string, - prev_rendered_content_version?: number, - prev_stream?: number, - prev_topic?: string, // New in FL 118, replacing `prev_subject`. - prev_subject?: string, // Replaced in FL 118 by `prev_topic`. - stream?: number, // New in FL 118. - timestamp: number, - topic?: string, // New in FL 118. - user_id: UserId | null, -|}>; - -// 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, - edit_history?: $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 = {| ...ApiResponseMessages, - messages: $ReadOnlyArray, + messages: $ReadOnlyArray, |}; -/** Exported for tests only. */ -export const migrateMessages = ( - messages: $ReadOnlyArray, - identity: Identity, - zulipFeatureLevel: number, - allowEditHistory: boolean, -): 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, - }; - }), - - // Why condition on allowEditHistory? See MessageBase['edit_history']. - // Why FL 118 condition? See MessageEdit type. - edit_history: - /* eslint-disable operator-linebreak */ - allowEditHistory && zulipFeatureLevel >= 118 - ? // $FlowIgnore[incompatible-cast] - See MessageEdit type - (message.edit_history: $ReadOnlyArray | void) - : null, - })); - const migrateResponse = (response, identity: Identity, zulipFeatureLevel, allowEditHistory) => { const { messages, ...restResponse } = response; return { ...restResponse, - messages: migrateMessages(messages, identity, zulipFeatureLevel, allowEditHistory), + messages: messages.map(message => + transformFetchedMessage(message, identity, zulipFeatureLevel, allowEditHistory), + ), }; }; diff --git a/src/api/messages/getSingleMessage.js b/src/api/messages/getSingleMessage.js new file mode 100644 index 00000000000..703652840a8 --- /dev/null +++ b/src/api/messages/getSingleMessage.js @@ -0,0 +1,55 @@ +/* @flow strict-local */ + +import type { Auth, ApiResponseSuccess } from '../transportTypes'; +import type { Message } from '../apiTypes'; +import { transformFetchedMessage, type FetchedMessage } from '../rawModelTypes'; +import { apiGet } from '../apiFetch'; +import { identityOfAuth } from '../../account/accountMisc'; + +// The actual response from the server. We convert the message to a proper +// Message before returning it to application code. +type ServerApiResponseSingleMessage = {| + ...$Exact, + -raw_content: string, // deprecated + + // Until we narrow FetchedMessage into its FL 120+ form, FetchedMessage + // will be a bit less precise than we could be here. That's because we + // only get this field from servers FL 120+. + // TODO(server-5.0): Make this field required, and remove FL-120 comment. + +message?: FetchedMessage, +|}; + +/** + * See https://zulip.com/api/get-message + * + * Gives undefined if the `message` field is missing, which it will be for + * FL <120. + */ +// TODO(server-5.0): Simplify FL-120 condition in jsdoc and implementation. +export default async ( + auth: Auth, + args: {| + +message_id: number, + |}, + + // TODO(#4659): Don't get this from callers. + zulipFeatureLevel: number, + + // TODO(#4659): Don't get this from callers? + allowEditHistory: boolean, +): Promise => { + const { message_id } = args; + const response: ServerApiResponseSingleMessage = await apiGet(auth, `messages/${message_id}`, { + apply_markdown: true, + }); + + return ( + response.message + && transformFetchedMessage( + response.message, + identityOfAuth(auth), + zulipFeatureLevel, + allowEditHistory, + ) + ); +}; diff --git a/src/api/rawModelTypes.js b/src/api/rawModelTypes.js new file mode 100644 index 00000000000..b764cebca92 --- /dev/null +++ b/src/api/rawModelTypes.js @@ -0,0 +1,99 @@ +/* @flow strict-local */ + +import type { Identity } from '../types'; +import type { + PmMessage, + StreamMessage, + Message, + MessageEdit, + Reaction, + UserId, +} from './modelTypes'; +import { AvatarURL } from '../utils/avatar'; + +/** + * The variant of `Reaction` found in a fetch-message(s) response. + * + * Note that reaction events have a *different* variation; see their + * handling in `eventToAction`. + */ +// We shouldn't have to rely on this format on servers at feature +// level 2+; those newer servers include a top-level `user_id` field +// in addition to the `user` object. See #4072. +// TODO(server-3.0): Simplify this away. +export type FetchedReaction = $ReadOnly<{| + ...$Diff, + user: $ReadOnly<{| + email: string, + full_name: string, + id: UserId, + |}>, +|}>; + +/** + * The elements of Message.edit_history found in a fetch-message(s) response. + * + * Accurate for supported servers before and after FL 118. For convenience, + * we drop objects of this type if the FL is <118, so that the modern shape + * at Message.edit_history is the only shape we store in Redux; see there. + */ +// TODO(server-5.0): Simplify this away. +export type FetchedMessageEdit = $ReadOnly<{| + prev_content?: string, + prev_rendered_content?: string, + prev_rendered_content_version?: number, + prev_stream?: number, + prev_topic?: string, // New in FL 118, replacing `prev_subject`. + prev_subject?: string, // Replaced in FL 118 by `prev_topic`. + stream?: number, // New in FL 118. + timestamp: number, + topic?: string, // New in FL 118. + user_id: UserId | null, +|}>; + +// How `FetchedMessage` relates to `Message`, in a way that applies +// uniformly to `Message`'s subtypes. +type FetchedMessageOf = $ReadOnly<{| + ...$Exact, + avatar_url: string | null, + reactions: $ReadOnlyArray, + + // Unlike Message['edit_history'], this can't be `null`. + edit_history?: $ReadOnlyArray, +|}>; + +export type FetchedMessage = FetchedMessageOf | FetchedMessageOf; + +/** + * Make a `Message` from `GET /messages` or `GET /message/{message_id}` + */ +export const transformFetchedMessage = ( + message: FetchedMessageOf, + identity: Identity, + zulipFeatureLevel: number, + allowEditHistory: boolean, +): 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, + }; + }), + + // Why condition on allowEditHistory? See MessageBase['edit_history']. + // Why FL 118 condition? See MessageEdit type. + edit_history: + /* eslint-disable operator-linebreak */ + allowEditHistory && zulipFeatureLevel >= 118 + ? // $FlowIgnore[incompatible-cast] - See MessageEdit type + (message.edit_history: $ReadOnlyArray | void) + : null, +}); diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 62d8d6e993e..6468bc7f6b5 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -15,7 +15,7 @@ import { tryFetch, } from '../fetchActions'; import { FIRST_UNREAD_ANCHOR } from '../../anchor'; -import type { ServerMessage } from '../../api/messages/getMessages'; +import type { FetchedMessage } from '../../api/rawModelTypes'; import { streamNarrow, HOME_NARROW, HOME_NARROW_STR, keyFromNarrow } from '../../utils/narrow'; import { GravatarURL } from '../../utils/avatar'; import * as eg from '../../__tests__/lib/exampleData'; @@ -242,8 +242,8 @@ describe('fetchActions', () => { // own transformations. // // TODO: Deduplicate this logic with similar logic in - // migrateMessages-test.js. - const serverMessage1: ServerMessage = { + // rawModelTypes-test.js. + const fetchedMessage1: FetchedMessage = { ...message1, reactions: [], avatar_url: null, // Null in server data will be transformed to a GravatarURL @@ -263,7 +263,7 @@ describe('fetchActions', () => { describe('success', () => { beforeEach(() => { const response = { - messages: [serverMessage1], + messages: [fetchedMessage1], result: 'success', }; // $FlowFixMe[prop-missing]: See mock in jest/globalFetch.js. @@ -353,21 +353,13 @@ describe('fetchActions', () => { }); test("rejects when validation-at-the-edge can't handle data, dispatches MESSAGE_FETCH_ERROR", async () => { - // This validation is done in `migrateMessages`. + // This validation is done in transformFetchedMessages in + // rawModelTypes. // - // TODO: See if we can mock `migrateMessages` to throw an - // error unconditionally. We don't want to care specifically - // how the data is malformed. Making this mock isn't - // straightforward, in part because Jest wants you to mock - // entire modules. `migrateMessages`'s caller is in the - // same file as `migrateMessages`; it doesn't import it. So we - // can't intercept such an import and have it give amock. - // - // For now, we simulate #4156, a real-life problem that a user - // at server commit 0af2f9d838 ran into [1], by having - // `user` be missing on reactions on a message. - // - // [1] https://github.com/zulip/zulip-mobile/issues/4156#issuecomment-655905093 + // Simulate #4156, a real-life problem that a user at server commit + // 0af2f9d838 ran into [1], by having `user` be missing on reactions + // on a message: + // https://github.com/zulip/zulip-mobile/issues/4156#issuecomment-655905093 const store = mockStore(baseState); // Missing `user` (and `user_id`, for good measure), to @@ -382,7 +374,7 @@ describe('fetchActions', () => { // Flow would complain at `faultyReaction` if it // type-checked `response`, but we should ignore it if that // day comes. It's badly shaped on purpose. - messages: [{ ...serverMessage1, reactions: [faultyReaction] }], + messages: [{ ...fetchedMessage1, reactions: [faultyReaction] }], result: 'success', }; // $FlowFixMe[prop-missing]: See mock in jest/globalFetch.js.