From 826c2777b65434a890123d0daf6073a2b887b48a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 12:55:53 -0800 Subject: [PATCH 01/24] notif [nfc]: Drop now-redundant check for realm_uri Since 9620629a7, we've always had a realm_uri property in this object -- we've rejected any notifications where it was absent in the payload. So this check is no longer needed. --- src/notification/notifOpen.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/notification/notifOpen.js b/src/notification/notifOpen.js index 268b7ea2a12..9ad76ca9f1b 100644 --- a/src/notification/notifOpen.js +++ b/src/notification/notifOpen.js @@ -43,12 +43,6 @@ export const getAccountFromNotificationData = ( accounts: $ReadOnlyArray, ): Identity | null => { const { realm_uri, user_id } = data; - if (realm_uri == null) { - // Old server, no realm info included. This field appeared in - // Zulip 1.8, so we don't support these servers anyway. - logging.warn('notification missing field: realm_uri'); - return null; - } const realmUrl = tryParseUrl(realm_uri); if (realmUrl === undefined) { From a8af9339c70f4d9dedd4f0cc95b85afb66fde470 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 20:55:10 -0800 Subject: [PATCH 02/24] wip api: Use numeric stream ID in send-message, relying on Zulip 2.0+ TODO Perhaps make ServerCompatBanner more insistent? This is awfully core functionality that this breaks for ancient servers. Tested manually (on a current server) that sending to a stream, and sharing to a stream, still work. --- src/outbox/outboxActions.js | 7 ++----- src/sharing/ShareToStream.js | 1 - src/sharing/ShareWrapper.js | 7 ++----- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index ce7720c5553..14baa1a511f 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -29,7 +29,7 @@ import { getAllUsersById, getOwnUser } from '../users/userSelectors'; import { makeUserId } from '../api/idTypes'; import { caseNarrowPartial, isConversationNarrow } from '../utils/narrow'; import { BackoffMachine } from '../utils/async'; -import { recipientsOfPrivateMessage, streamNameOfStreamMessage } from '../utils/recipient'; +import { recipientsOfPrivateMessage } from '../utils/recipient'; export const messageSendStart = (outbox: Outbox): PerAccountAction => ({ type: MESSAGE_SEND_START, @@ -74,10 +74,7 @@ const trySendMessages = (dispatch, getState): boolean => { item.type === 'private' // TODO(server-2.0): switch to numeric user IDs (#3764), not emails. ? recipientsOfPrivateMessage(item).map(r => r.email).join(',') - // TODO(server-2.0): switch to numeric stream IDs (#3918), not names. - // HACK: the server attempts to interpret this argument as JSON, then - // CSV, then a literal. To avoid misparsing, always use JSON. - : JSON.stringify([streamNameOfStreamMessage(item)]); + : JSON.stringify(item.stream_id); await api.sendMessage(auth, { type: item.type, diff --git a/src/sharing/ShareToStream.js b/src/sharing/ShareToStream.js index 22b881e9003..c58bc98449a 100644 --- a/src/sharing/ShareToStream.js +++ b/src/sharing/ShareToStream.js @@ -144,7 +144,6 @@ class ShareToStreamInner extends React.PureComponent { const { streamName, streamId, topic, isStreamFocused, isTopicFocused } = this.state; const narrow = streamId !== null ? streamNarrow(streamId) : null; const sendTo = { - streamName, /* $FlowFixMe[incompatible-cast]: ShareWrapper will only look at this * if getValidationErrors returns empty, so only if streamId is * indeed not null. Should make that logic less indirected and more diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index 01c5fe671c0..ceea013da81 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -26,8 +26,7 @@ import { ApiError, RequestError } from '../api/apiErrors'; type SendTo = | {| type: 'pm', selectedRecipients: $ReadOnlyArray |} - // TODO(server-2.0): Drop streamName (#3918). Used below for sending. - | {| type: 'stream', streamName: string, streamId: number, topic: string |}; + | {| type: 'stream', streamId: number, topic: string |}; const styles = createStyleSheet({ wrapper: { @@ -200,9 +199,7 @@ class ShareWrapperInner extends React.PureComponent { content: messageToSend, type: 'stream', subject: sendTo.topic || apiConstants.kNoTopicTopic, - // TODO(server-2.0): switch to numeric stream ID (#3918), not name; - // then drop streamName from SendTo - to: sendTo.streamName, + to: JSON.stringify(sendTo.streamId), }; try { From ae56cfada1af714e6cbb3784f4ff668263dc0afe Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 20:56:46 -0800 Subject: [PATCH 03/24] api: Use numeric user IDs in send-message, relying on Zulip 2.0+ Tested manually that sending a PM still works. --- src/outbox/outboxActions.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 14baa1a511f..52c4e71df0a 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -72,8 +72,7 @@ const trySendMessages = (dispatch, getState): boolean => { // prettier-ignore const to = item.type === 'private' - // TODO(server-2.0): switch to numeric user IDs (#3764), not emails. - ? recipientsOfPrivateMessage(item).map(r => r.email).join(',') + ? JSON.stringify(recipientsOfPrivateMessage(item).map(r => r.id)) : JSON.stringify(item.stream_id); await api.sendMessage(auth, { From 5f74f6806c67934eff8e147f0e5f3ac050ec1966 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 21:03:57 -0800 Subject: [PATCH 04/24] api: Stop sending (empty) subject/topic on PMs The server just ignores this. At the other api.sendMessage call site, in ShareWrapper, we already don't send it. --- src/outbox/outboxActions.js | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 52c4e71df0a..a29f7fbd8da 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -69,20 +69,26 @@ const trySendMessages = (dispatch, getState): boolean => { return; // i.e., continue } - // prettier-ignore - const to = - item.type === 'private' - ? JSON.stringify(recipientsOfPrivateMessage(item).map(r => r.id)) - : JSON.stringify(item.stream_id); - - await api.sendMessage(auth, { - type: item.type, - to, - subject: item.subject, + const commonParams = { content: item.markdownContent, localId: item.timestamp, eventQueueId: state.session.eventQueueId ?? undefined, - }); + }; + + const params = + item.type === 'private' + ? { + ...commonParams, + type: 'private', + to: JSON.stringify(recipientsOfPrivateMessage(item).map(r => r.id)), + } + : { + ...commonParams, + type: 'stream', + to: JSON.stringify(item.stream_id), + subject: item.subject, + }; + await api.sendMessage(auth, params); dispatch(messageSendComplete(item.timestamp)); }); return true; From 759022c291f1dcaeeb435bb53d7bb3534f0031e2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 21:04:51 -0800 Subject: [PATCH 05/24] api types: Disallow topic on PMs in send-message The server actually accepts and ignores these, but best to make the interface clear. --- src/api/messages/sendMessage.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/api/messages/sendMessage.js b/src/api/messages/sendMessage.js index 371027ad715..2cc7fd314de 100644 --- a/src/api/messages/sendMessage.js +++ b/src/api/messages/sendMessage.js @@ -3,23 +3,25 @@ import type { ApiResponse, Auth } from '../transportTypes'; import { apiPost } from '../apiFetch'; +type CommonParams = {| + content: string, + localId?: number, + eventQueueId?: string, +|}; + /** See https://zulip.com/api/send-message */ export default async ( auth: Auth, - params: {| - type: 'private' | 'stream', - to: string, + // prettier-ignore + params: // TODO(server-2.0): Say "topic", not "subject" - subject?: string, - content: string, - localId?: number, - eventQueueId?: string, - |}, + | {| ...CommonParams, type: 'stream', to: string, subject: string |} + | {| ...CommonParams, type: 'private', to: string |}, ): Promise => apiPost(auth, 'messages', { type: params.type, to: params.to, - subject: params.subject, + subject: (params: { +subject?: string, ... }).subject, content: params.content, local_id: params.localId, queue_id: params.eventQueueId, From 0bc075db4ae97fb559250c85b33926ad2c0a6620 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 21:07:54 -0800 Subject: [PATCH 06/24] api [nfc]: In send-message, push JSON.stringify(to) to the inside This is really part of the details of how things are encoded in the API, not something application code should have to concern itself with. --- src/api/messages/sendMessage.js | 7 ++++--- src/outbox/outboxActions.js | 9 ++------- src/sharing/ShareWrapper.js | 4 ++-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/api/messages/sendMessage.js b/src/api/messages/sendMessage.js index 2cc7fd314de..3b77968d51f 100644 --- a/src/api/messages/sendMessage.js +++ b/src/api/messages/sendMessage.js @@ -1,6 +1,7 @@ /* @flow strict-local */ import type { ApiResponse, Auth } from '../transportTypes'; +import type { UserId } from '../idTypes'; import { apiPost } from '../apiFetch'; type CommonParams = {| @@ -15,12 +16,12 @@ export default async ( // prettier-ignore params: // TODO(server-2.0): Say "topic", not "subject" - | {| ...CommonParams, type: 'stream', to: string, subject: string |} - | {| ...CommonParams, type: 'private', to: string |}, + | {| ...CommonParams, type: 'stream', to: number, subject: string |} + | {| ...CommonParams, type: 'private', to: $ReadOnlyArray |}, ): Promise => apiPost(auth, 'messages', { type: params.type, - to: params.to, + to: JSON.stringify(params.to), subject: (params: { +subject?: string, ... }).subject, content: params.content, local_id: params.localId, diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index a29f7fbd8da..09396ef5bc6 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -80,14 +80,9 @@ const trySendMessages = (dispatch, getState): boolean => { ? { ...commonParams, type: 'private', - to: JSON.stringify(recipientsOfPrivateMessage(item).map(r => r.id)), + to: recipientsOfPrivateMessage(item).map(r => r.id), } - : { - ...commonParams, - type: 'stream', - to: JSON.stringify(item.stream_id), - subject: item.subject, - }; + : { ...commonParams, type: 'stream', to: item.stream_id, subject: item.subject }; await api.sendMessage(auth, params); dispatch(messageSendComplete(item.timestamp)); }); diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index ceea013da81..299b17b3979 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -193,13 +193,13 @@ class ShareWrapperInner extends React.PureComponent { ? { content: messageToSend, type: 'private', - to: JSON.stringify(sendTo.selectedRecipients), + to: sendTo.selectedRecipients, } : { content: messageToSend, type: 'stream', subject: sendTo.topic || apiConstants.kNoTopicTopic, - to: JSON.stringify(sendTo.streamId), + to: sendTo.streamId, }; try { From 6fbe2fd542e0267f1f6449bfcb715f2bcd6755cb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 13:19:43 -0800 Subject: [PATCH 07/24] api [nfc]: Rename send-message parameter to "topic", from "subject" And add a TODO for changing what we actually send to the server. (Which in turn we'll take care of shortly.) --- src/api/messages/sendMessage.js | 7 +++---- src/outbox/outboxActions.js | 2 +- src/sharing/ShareWrapper.js | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/api/messages/sendMessage.js b/src/api/messages/sendMessage.js index 3b77968d51f..8db8fcc8610 100644 --- a/src/api/messages/sendMessage.js +++ b/src/api/messages/sendMessage.js @@ -13,16 +13,15 @@ type CommonParams = {| /** See https://zulip.com/api/send-message */ export default async ( auth: Auth, - // prettier-ignore params: - // TODO(server-2.0): Say "topic", not "subject" - | {| ...CommonParams, type: 'stream', to: number, subject: string |} + | {| ...CommonParams, type: 'stream', to: number, topic: string |} | {| ...CommonParams, type: 'private', to: $ReadOnlyArray |}, ): Promise => apiPost(auth, 'messages', { type: params.type, to: JSON.stringify(params.to), - subject: (params: { +subject?: string, ... }).subject, + // TODO(server-2.0): say "topic" instead + subject: (params: { +topic?: string, ... }).topic, content: params.content, local_id: params.localId, queue_id: params.eventQueueId, diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index 09396ef5bc6..ee00fd74d75 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -82,7 +82,7 @@ const trySendMessages = (dispatch, getState): boolean => { type: 'private', to: recipientsOfPrivateMessage(item).map(r => r.id), } - : { ...commonParams, type: 'stream', to: item.stream_id, subject: item.subject }; + : { ...commonParams, type: 'stream', to: item.stream_id, topic: item.subject }; await api.sendMessage(auth, params); dispatch(messageSendComplete(item.timestamp)); }); diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index 299b17b3979..e1bf790f074 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -198,7 +198,7 @@ class ShareWrapperInner extends React.PureComponent { : { content: messageToSend, type: 'stream', - subject: sendTo.topic || apiConstants.kNoTopicTopic, + topic: sendTo.topic || apiConstants.kNoTopicTopic, to: sendTo.streamId, }; From 3267916b27e8a7352a6d27dfa3a1961dd3250262 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 13:21:58 -0800 Subject: [PATCH 08/24] api: Say "topic" in send-message, relying on Zulip 2.0+ --- src/api/messages/sendMessage.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/api/messages/sendMessage.js b/src/api/messages/sendMessage.js index 8db8fcc8610..90457dd8d0d 100644 --- a/src/api/messages/sendMessage.js +++ b/src/api/messages/sendMessage.js @@ -20,8 +20,7 @@ export default async ( apiPost(auth, 'messages', { type: params.type, to: JSON.stringify(params.to), - // TODO(server-2.0): say "topic" instead - subject: (params: { +topic?: string, ... }).topic, + topic: (params: { +topic?: string, ... }).topic, content: params.content, local_id: params.localId, queue_id: params.eventQueueId, From 502110f82b13e5ebb28c95ee834fb9a170b57e76 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 13:24:11 -0800 Subject: [PATCH 09/24] api: Say "topic" in update-message, relying on Zulip 2.0+ --- src/action-sheets/index.js | 2 +- src/api/messages/updateMessage.js | 4 +--- src/chat/ChatScreen.js | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/action-sheets/index.js b/src/action-sheets/index.js index c3a53abbabc..e644b168f96 100644 --- a/src/action-sheets/index.js +++ b/src/action-sheets/index.js @@ -343,7 +343,7 @@ const toggleResolveTopic = async ({ auth, streamId, topic, _, streams, zulipFeat await api.updateMessage(auth, messageId, { propagate_mode: 'change_all', - subject: newTopic, + topic: newTopic, ...(zulipFeatureLevel >= 9 && { send_notification_to_old_thread: false, send_notification_to_new_thread: true, diff --git a/src/api/messages/updateMessage.js b/src/api/messages/updateMessage.js index af3f92c8c7d..401359d58d4 100644 --- a/src/api/messages/updateMessage.js +++ b/src/api/messages/updateMessage.js @@ -17,9 +17,7 @@ export default async ( auth: Auth, messageId: number, params: $ReadOnly<{| - // TODO(server-2.0): Say "topic", not "subject" - subject?: string, - + topic?: string, propagate_mode?: PropagateMode, content?: string, diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 7c64e888cc5..b306311ce3a 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -201,7 +201,7 @@ export default function ChatScreen(props: Props): Node { ); if ((content !== undefined && content !== '') || (topic !== undefined && topic !== '')) { - api.updateMessage(auth, editMessage.id, { content, subject: topic }).catch(error => { + api.updateMessage(auth, editMessage.id, { content, topic }).catch(error => { showErrorAlert(_('Failed to edit message'), error.message); }); } From 4c15ed24ba97ce7f3e90bd6731a85f61cf326f0a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 21:15:47 -0800 Subject: [PATCH 10/24] api: Use user IDs in set-typing-status, relying on Zulip 2.0+ Tested manually that this feature still works fine. --- src/users/usersActions.js | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/users/usersActions.js b/src/users/usersActions.js index 257c3c0a700..81cd8aeae42 100644 --- a/src/users/usersActions.js +++ b/src/users/usersActions.js @@ -4,9 +4,8 @@ import * as typing_status from '@zulip/shared/lib/typing_status'; import type { Auth, PerAccountState, Narrow, UserId, ThunkAction } from '../types'; import * as api from '../api'; import { PRESENCE_RESPONSE } from '../actionConstants'; -import { getAuth, getServerVersion } from '../selectors'; +import { getAuth } from '../selectors'; import { isPmNarrow, userIdsOfPmNarrow } from '../utils/narrow'; -import { getUserForId } from './userSelectors'; export const reportPresence = (isActive: boolean): ThunkAction> => @@ -28,28 +27,15 @@ export const reportPresence = const typingWorker = (state: PerAccountState) => { const auth: Auth = getAuth(state); - // User ID arrays are only supported in server versions >= 2.0.0-rc1 - // (zulip/zulip@2f634f8c0). For versions before this, email arrays - // are used. - // TODO(server-2.0): Simplify this away. - const useEmailArrays = !getServerVersion(state).isAtLeast('2.0.0-rc1'); - - const getRecipients = user_ids_array => { - if (useEmailArrays) { - return JSON.stringify(user_ids_array.map(userId => getUserForId(state, userId).email)); - } - return JSON.stringify(user_ids_array); - }; - return { get_current_time: () => new Date().getTime(), notify_server_start: (user_ids_array: $ReadOnlyArray) => { - api.typing(auth, getRecipients(user_ids_array), 'start'); + api.typing(auth, JSON.stringify(user_ids_array), 'start'); }, notify_server_stop: (user_ids_array: $ReadOnlyArray) => { - api.typing(auth, getRecipients(user_ids_array), 'stop'); + api.typing(auth, JSON.stringify(user_ids_array), 'stop'); }, }; }; From 725227c06876a24ca17375ddd36e113ef56e99ed Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 21:17:32 -0800 Subject: [PATCH 11/24] api [nfc]: Require numeric user IDs in set-typing-status --- src/api/typing.js | 9 +++++++-- src/users/usersActions.js | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/api/typing.js b/src/api/typing.js index f3c25dd2acf..8ea0c0d592e 100644 --- a/src/api/typing.js +++ b/src/api/typing.js @@ -1,12 +1,17 @@ /* @flow strict-local */ import type { ApiResponse, Auth } from './transportTypes'; +import type { UserId } from './idTypes'; import { apiPost } from './apiFetch'; type TypingOperation = 'start' | 'stop'; /** See https://zulip.com/api/set-typing-status */ -export default (auth: Auth, recipients: string, operation: TypingOperation): Promise => +export default ( + auth: Auth, + recipients: $ReadOnlyArray, + operation: TypingOperation, +): Promise => apiPost(auth, 'typing', { - to: recipients, + to: JSON.stringify(recipients), op: operation, }); diff --git a/src/users/usersActions.js b/src/users/usersActions.js index 81cd8aeae42..15a49c1fdb7 100644 --- a/src/users/usersActions.js +++ b/src/users/usersActions.js @@ -31,11 +31,11 @@ const typingWorker = (state: PerAccountState) => { get_current_time: () => new Date().getTime(), notify_server_start: (user_ids_array: $ReadOnlyArray) => { - api.typing(auth, JSON.stringify(user_ids_array), 'start'); + api.typing(auth, user_ids_array, 'start'); }, notify_server_stop: (user_ids_array: $ReadOnlyArray) => { - api.typing(auth, JSON.stringify(user_ids_array), 'stop'); + api.typing(auth, user_ids_array, 'stop'); }, }; }; From 4a90fa326430f02443f9e335d465efc7623e4fe1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 13:46:45 -0800 Subject: [PATCH 12/24] wip api types: Update Message to comment on "subject" (previously took care of topic_links and subject_links too, but that's been done since) These descriptions are copied from the corresponding properties on an `update_message` event, at EventUpdateMessageAction; the same changes happened on messages at the same time: https://zulip.com/api/get-messages#response https://zulip.com/api/get-events#message --- src/api/modelTypes.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 79e279fd2dd..d5e3e47edcc 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -1062,6 +1062,7 @@ export type StreamMessage = $ReadOnly<{| * https://chat.zulip.org/#narrow/stream/19-documentation/topic/.60orig_subject.60.20in.20.60update_message.60.20events/near/1112709 * (see point 4). We assume this has always been the case. */ + // Still "subject", as of 2022: https://github.com/zulip/zulip/issues/1192 subject: string, // We don't actually use this property. If and when we do, we'll probably From 0b4107615fc552eca90c0bd1fc591a11a6b72389 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 12 Jan 2022 21:39:20 -0800 Subject: [PATCH 13/24] recipient [nfc]: Factor out recipientIdsOfPrivateMessage This encapsulates a bit more into this module of how we represent this list of recipients. Expand jsdoc on the remaining ways we expose the other details, to point out the pitfall in them. Also note with a TODO an opportunity this opens up for improving performance. --- src/api/modelTypes.js | 3 ++ src/outbox/outboxActions.js | 8 ++---- src/pm-conversations/pmConversationsModel.js | 7 ++--- src/utils/narrow.js | 8 ++---- src/utils/recipient.js | 30 ++++++++++++-------- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index d5e3e47edcc..7f98035fcde 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -1029,6 +1029,9 @@ export type PmMessage = $ReadOnly<{| * * The ordering is less well specified; if it matters, sort first. */ + // TODO: We frequently do `.map(r => r.id)` on this, via `recipientIdsOfPrivateMessage`. + // Instead of making a new array every time, it'd be good to do that + // once, up front, at the edge. display_recipient: $ReadOnlyArray, /** diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index ee00fd74d75..42b306cc1e4 100644 --- a/src/outbox/outboxActions.js +++ b/src/outbox/outboxActions.js @@ -29,7 +29,7 @@ import { getAllUsersById, getOwnUser } from '../users/userSelectors'; import { makeUserId } from '../api/idTypes'; import { caseNarrowPartial, isConversationNarrow } from '../utils/narrow'; import { BackoffMachine } from '../utils/async'; -import { recipientsOfPrivateMessage } from '../utils/recipient'; +import { recipientIdsOfPrivateMessage } from '../utils/recipient'; export const messageSendStart = (outbox: Outbox): PerAccountAction => ({ type: MESSAGE_SEND_START, @@ -77,11 +77,7 @@ const trySendMessages = (dispatch, getState): boolean => { const params = item.type === 'private' - ? { - ...commonParams, - type: 'private', - to: recipientsOfPrivateMessage(item).map(r => r.id), - } + ? { ...commonParams, type: 'private', to: recipientIdsOfPrivateMessage(item) } : { ...commonParams, type: 'stream', to: item.stream_id, topic: item.subject }; await api.sendMessage(auth, params); dispatch(messageSendComplete(item.timestamp)); diff --git a/src/pm-conversations/pmConversationsModel.js b/src/pm-conversations/pmConversationsModel.js index d85a8c8206f..ec6ac858f6d 100644 --- a/src/pm-conversations/pmConversationsModel.js +++ b/src/pm-conversations/pmConversationsModel.js @@ -10,7 +10,7 @@ import { import { makeUserId } from '../api/idTypes'; import type { PerAccountApplicableAction, PmMessage, PmOutbox, UserId } from '../types'; -import { recipientsOfPrivateMessage } from '../utils/recipient'; +import { recipientIdsOfPrivateMessage } from '../utils/recipient'; import { ZulipVersion } from '../utils/zulipVersion'; /** The minimum server version to expect this data to be available. */ @@ -45,10 +45,7 @@ function keyOfUsers(ids: $ReadOnlyArray, ownUserId: UserId): PmConversat } function keyOfPrivateMessage(msg: PmMessage | PmOutbox, ownUserId: UserId): PmConversationKey { - return keyOfUsers( - recipientsOfPrivateMessage(msg).map(r => r.id), - ownUserId, - ); + return keyOfUsers(recipientIdsOfPrivateMessage(msg), ownUserId); } /** The users in the conversation, other than self. */ diff --git a/src/utils/narrow.js b/src/utils/narrow.js index a25fea4ea40..553716f2d2d 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -5,7 +5,7 @@ import type { ApiNarrow, Message, Outbox, Stream, UserId, UserOrBot } from '../t import { normalizeRecipientsAsUserIdsSansMe, pmKeyRecipientsFromMessage, - recipientsOfPrivateMessage, + recipientIdsOfPrivateMessage, type PmKeyRecipients, type PmKeyUsers, pmKeyRecipientsFromPmKeyUsers, @@ -475,11 +475,9 @@ export const isMessageInNarrow = ( if (message.type !== 'private') { return false; } - const recipients = recipientsOfPrivateMessage(message).map(r => r.id); - const narrowAsRecipients = ids; return ( - normalizeRecipientsAsUserIdsSansMe(recipients, ownUserId) - === normalizeRecipientsAsUserIdsSansMe(narrowAsRecipients, ownUserId) + normalizeRecipientsAsUserIdsSansMe(recipientIdsOfPrivateMessage(message), ownUserId) + === normalizeRecipientsAsUserIdsSansMe(ids, ownUserId) ); }, starred: () => flags.includes('starred'), diff --git a/src/utils/recipient.js b/src/utils/recipient.js index d3992fbbb51..d2e4138ff30 100644 --- a/src/utils/recipient.js +++ b/src/utils/recipient.js @@ -36,6 +36,12 @@ export const recipientsOfPrivateMessage = ( message: PmMessage | PmOutbox, ): $ReadOnlyArray => message.display_recipient; +/** The recipients of a PM. */ +// TODO: This makes a fresh array every time. It would be good to do that +// once, when first ingesting a message. +export const recipientIdsOfPrivateMessage = (message: PmMessage | PmOutbox): Array => + recipientsOfPrivateMessage(message).map(r => r.id); + /** * A list of users identifying a PM conversation, as per pmKeyRecipientsFromMessage. * @@ -147,12 +153,18 @@ export const normalizeRecipientsAsUserIdsSansMe = ( /** * The set of users to show in the UI to identify a PM conversation. * + * Note that the details here, other than user IDs, may be out of date! + * They reflect the respective users' emails and full names (etc.) as of + * when we learned about this message; they do not reflect any changes we've + * heard since then. See #5208. + * * See also: * * `pmKeyRecipientsFromMessage`, which should be used when a consistent, * unique key is needed for identifying different PM conversations in our * data structures. * * `pmUiRecipientsFromKeyRecipients`, which takes a `PmKeyRecipients` - * as input instead of a message. + * as input instead of a message, returns only user IDs, and is therefore + * immune to #5208. * * BUG(#5208): This is as of whenever we learned about the message; * it doesn't get updated if one of these users changes their @@ -228,11 +240,7 @@ export const pmKeyRecipientsFromIds = ( export const pmKeyRecipientsFromMessage = ( message: PmMessage | PmOutbox, ownUserId: UserId, -): PmKeyRecipients => - pmKeyRecipientsFromIds( - recipientsOfPrivateMessage(message).map(r => r.id), - ownUserId, - ); +): PmKeyRecipients => pmKeyRecipientsFromIds(recipientIdsOfPrivateMessage(message), ownUserId); /** * The list of users to identify a PM conversation by in our data structures. @@ -271,7 +279,7 @@ export const pmKeyRecipientUsersFromMessage = ( allUsersById: Map, ownUserId: UserId, ): PmKeyUsers | null => { - const userIds = recipientsOfPrivateMessage(message).map(r => r.id); + const userIds = recipientIdsOfPrivateMessage(message); return pmKeyRecipientUsersFromIds(userIds, allUsersById, ownUserId); }; @@ -310,10 +318,8 @@ export const pmKeyRecipientsFromUsers = ( // is sorted numerically and encoded in ASCII-decimal, comma-separated. // See the `unread_msgs` data structure in `src/api/initialDataTypes.js`. export const pmUnreadsKeyFromMessage = (message: PmMessage, ownUserId?: UserId): string => { - const recipients = recipientsOfPrivateMessage(message); - // This includes all users in the thread; see `Message#display_recipient`. - const userIds = recipients.map(r => r.id); + const userIds = recipientIdsOfPrivateMessage(message); if (userIds.length === 1) { // Self-PM. @@ -443,8 +449,8 @@ export const isSameRecipient = ( // identify its conversation, and sort when computing that. Until // then, we just tolerate this glitch in that edge case. return isEqual( - recipientsOfPrivateMessage(message1).map(r => r.id), - recipientsOfPrivateMessage(message2).map(r => r.id), + recipientIdsOfPrivateMessage(message1), + recipientIdsOfPrivateMessage(message2), ); case 'stream': if (message2.type !== 'stream') { From d5ebeae085c267255778203bae74fef511d99f0a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 14:01:20 -0800 Subject: [PATCH 14/24] wip android notif: Stop parsing pre-2.0 zulip_message_id field on remove TODO clarify in server that the plural field is the only one that counts, as a matter of API --- .../zulipmobile/notifications/FcmMessage.kt | 3 --- .../notifications/FcmMessageTest.kt | 26 +++---------------- 2 files changed, 4 insertions(+), 25 deletions(-) diff --git a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt index 37237ab727a..4405399f46b 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt @@ -183,9 +183,6 @@ data class RemoveFcmMessage( companion object { fun fromFcmData(data: Map): RemoveFcmMessage { val messageIds = HashSet() - data["zulip_message_id"]?.parseInt("zulip_message_id")?.let { - messageIds.add(it) - } data["zulip_message_ids"]?.parseCommaSeparatedInts("zulip_message_ids")?.let { messageIds.addAll(it) } diff --git a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt index 15f006b7424..9b803820542 100644 --- a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt +++ b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt @@ -250,20 +250,14 @@ class RemoveFcmMessageTest : FcmMessageTestBase() { "event" to "remove" )) - /// The Zulip server before v2.0 sends this form (plus some irrelevant fields). - // TODO(server-2.0): Drop this, and the logic it tests. - val singular = base.plus(sequenceOf( - "zulip_message_id" to "123" - )) - /// Zulip servers starting at v2.0 (released 2019-02-28; commit 9869153ae) - /// send a hybrid form. (In practice the singular field has one of the - /// same IDs found in the batch.) + /// send a hybrid singular-plural form. The singular field has one of the + /// same IDs found in the batch. /// /// We started consuming the batch field in 23.2.111 (released 2019-02-28; /// commit 4acd07376). val hybrid = base.plus(sequenceOf( - "zulip_message_ids" to "234,345", + "zulip_message_ids" to "123,234,345", "zulip_message_id" to "123" )) @@ -293,10 +287,7 @@ class RemoveFcmMessageTest : FcmMessageTestBase() { expect.that(parse(Example.batched)).isEqualTo( RemoveFcmMessage(Example.identity, setOf(123, 234, 345)) ) - expect.that(parse(Example.singular)).isEqualTo( - RemoveFcmMessage(Example.identity, setOf(123)) - ) - expect.that(parse(Example.singular.minus("zulip_message_id"))).isEqualTo( + expect.that(parse(Example.base)).isEqualTo( // This doesn't seem very useful to send, but harmless. RemoveFcmMessage(Example.identity, setOf()) ) @@ -317,15 +308,6 @@ class RemoveFcmMessageTest : FcmMessageTestBase() { assertParseFails(Example.hybrid.plus("realm_uri" to "zulip.example.com")) assertParseFails(Example.hybrid.plus("realm_uri" to "/examplecorp")) - for (badInt in sequenceOf( - "12,34", - "abc", - "" - )) { - assertParseFails(Example.singular.plus("zulip_message_id" to badInt)) - assertParseFails(Example.hybrid.plus("zulip_message_id" to badInt)) - } - for (badIntList in sequenceOf( "abc,34", "12,abc", From ea27eff7123721b85dd618ccd36ece2626ac611c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 14:10:05 -0800 Subject: [PATCH 15/24] android notif [nfc]: Update API comment on "user" This comment mentioned server 2.0, but just as the latest as of when it was written. This field was deleted at a later version, so note that down; and the real prompt for deleting the comment will be when "user_id" is always present and can be relied on. So copy the "TODO(server-2.1)" from there. --- .../java/com/zulipmobile/notifications/FcmMessage.kt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt index 4405399f46b..3fbd716ab82 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt @@ -203,12 +203,12 @@ private fun extractIdentity(data: Map): Identity = // `realm_uri` was added in server version 1.9.0 realmUri = data.require("realm_uri").parseUrl("realm_uri"), - // Server versions from 1.6.0 through 2.0.0 (and possibly earlier - // and later) send the user's email address, as `user`. We *could* - // use this as a substitute for `user_id` when that's missing... - // but it'd be inherently buggy, and the bug it'd introduce seems - // likely to affect more users than the bug it'd fix. So just ignore. - // TODO(server-2.0): Delete this comment. + // Server versions from 1.6.0 up to 3.0~6772 send the user's email + // address, as `user`. We *could* use this as a substitute for + // `user_id` when that's missing... but it'd be inherently buggy, + // and the bug it'd introduce seems likely to affect more users + // than the bug it'd fix. So just ignore. + // TODO(server-2.1): Delete this comment, relying on user_id. // (data["user"] ignored) // `user_id` was added in server version 2.1.0 (released 2019-12-12; From bc1cccbb73326ff436e10a2fec73c0ed6038c02a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 20:22:52 -0800 Subject: [PATCH 16/24] wip api: Make user_status required in initial data, relying on Zulip 2.0+ TODO cut the fallback, too; and its test case. --- src/api/initialDataTypes.js | 7 +------ src/user-statuses/userStatusesModel.js | 2 ++ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/api/initialDataTypes.js b/src/api/initialDataTypes.js index 9f11c5c00e0..181a9f72384 100644 --- a/src/api/initialDataTypes.js +++ b/src/api/initialDataTypes.js @@ -663,14 +663,9 @@ export type InitialDataUserStatus = $ReadOnly<{| * status is the zero status), the server may omit that user's record * entirely. * - * New in Zulip 2.0. Older servers don't send this, so it's natural - * to treat them as if all users have the zero status; and that gives - * correct behavior for this feature. - * * See UserStatusEvent for the event that carries updates to this data. */ - // TODO(server-2.0): Make required. - user_status?: $ReadOnly<{| + user_status: $ReadOnly<{| // Keys are UserId encoded as strings (just because JS objects are // string-keyed). [userId: string]: UserStatusUpdate, diff --git a/src/user-statuses/userStatusesModel.js b/src/user-statuses/userStatusesModel.js index 7be0906088e..e41dfff7de0 100644 --- a/src/user-statuses/userStatusesModel.js +++ b/src/user-statuses/userStatusesModel.js @@ -69,6 +69,8 @@ export const reducer = ( case REGISTER_COMPLETE: { const { user_status } = action.data; if (!user_status) { + // This fallback is unnecessary for server 2.0+, which includes all + // versions we support. But it's so cheap. // TODO(server-2.0): Drop this. return initialState; } From 55f75bfe402ef18fa6eacda23018c9c10bfc88a7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 10:51:26 -0800 Subject: [PATCH 17/24] notif tests [nfc]: Make "modern" payloads the baseline --- .../__tests__/notification-test.js | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 056bdf77b96..602a50255e2 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -94,47 +94,45 @@ describe('getNarrowFromNotificationData', () => { }); describe('extract iOS notification data', () => { - const barebones = deepFreeze({ + const identity = { realm_uri, user_id }; + const cases = deepFreeze({ // TODO(server-5.0): this will become an error case 'stream, no ID': { recipient_type: 'stream', stream: 'announce', topic: 'New channel', - realm_uri, + ...identity, }, stream: { recipient_type: 'stream', stream_id: 234, stream: 'announce', topic: 'New channel', - realm_uri, + ...identity, }, - '1:1 PM': { recipient_type: 'private', sender_email: 'nobody@example.com', realm_uri }, - 'group PM': { recipient_type: 'private', pm_users: '54,321', realm_uri }, + '1:1 PM': { recipient_type: 'private', sender_email: 'nobody@example.com', ...identity }, + 'group PM': { recipient_type: 'private', pm_users: '54,321', ...identity }, }); describe('success', () => { /** Helper function: test data immediately. */ const verify = (data: JSONableDict) => extractIosNotificationData({ zulip: data }); - for (const [type, data] of objectEntries(barebones)) { + for (const [type, data] of objectEntries(cases)) { test(`${type} notification`, () => { const expected = (() => { const { stream: stream_name = undefined, ...rest } = data; return stream_name !== undefined ? { ...rest, stream_name } : data; })(); - // barebones 1.9.0-style message is accepted + // baseline message is accepted const msg = data; expect(verify(msg)).toEqual(expected); - // new(-ish) optional user_id is accepted and copied - // TODO: Rewrite so modern-style payloads are the baseline, e.g., - // with a `modern` variable instead of `barebones`. Write - // individual tests for supporting older-style payloads, and mark - // those for future deletion, like with `TODO(1.9.0)`. - const msg1 = { ...msg, user_id }; - expect(verify(msg1)).toEqual({ ...expected, user_id }); + // pre-2.1 message missing user_id is accepted + const { user_id: _ignore, ...msg1 } = msg; // eslint-disable-line no-unused-vars + const { user_id: _ignore_, ...expected1 } = expected; // eslint-disable-line no-unused-vars + expect(verify(msg1)).toEqual(expected1); // unused fields are not copied const msg2 = { ...msg, realm_id: 8675309 }; @@ -197,12 +195,12 @@ describe('extract iOS notification data', () => { }); test('optional data is typechecked', () => { - expect(make({ ...barebones.stream, realm_uri: null })).toThrow(/invalid/); - expect(make({ ...barebones.stream, stream_id: '234' })).toThrow(/invalid/); - expect(make({ ...barebones['group PM'], realm_uri: ['array', 'of', 'string'] })).toThrow( + expect(make({ ...cases.stream, realm_uri: null })).toThrow(/invalid/); + expect(make({ ...cases.stream, stream_id: '234' })).toThrow(/invalid/); + expect(make({ ...cases['group PM'], realm_uri: ['array', 'of', 'string'] })).toThrow( /invalid/, ); - expect(make({ ...barebones.stream, user_id: 'abc' })).toThrow(/invalid/); + expect(make({ ...cases.stream, user_id: 'abc' })).toThrow(/invalid/); }); test('hypothetical future: different event types', () => { From e1e53e98e2b3094d92a1a3186aa1772bcb62ca31 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 10:54:37 -0800 Subject: [PATCH 18/24] notif tests: Split up cases better as distinct Jest tests This lets Jest give more informative output when some of them break. --- src/notification/__tests__/notification-test.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 602a50255e2..c6f00a13582 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -119,28 +119,25 @@ describe('extract iOS notification data', () => { const verify = (data: JSONableDict) => extractIosNotificationData({ zulip: data }); for (const [type, data] of objectEntries(cases)) { - test(`${type} notification`, () => { + describe(`${type} notification`, () => { const expected = (() => { const { stream: stream_name = undefined, ...rest } = data; return stream_name !== undefined ? { ...rest, stream_name } : data; })(); - // baseline message is accepted const msg = data; - expect(verify(msg)).toEqual(expected); + test('baseline', () => expect(verify(msg)).toEqual(expected)); - // pre-2.1 message missing user_id is accepted const { user_id: _ignore, ...msg1 } = msg; // eslint-disable-line no-unused-vars const { user_id: _ignore_, ...expected1 } = expected; // eslint-disable-line no-unused-vars - expect(verify(msg1)).toEqual(expected1); + test('pre-2.1 message missing user_id', () => expect(verify(msg1)).toEqual(expected1)); - // unused fields are not copied const msg2 = { ...msg, realm_id: 8675309 }; - expect(verify(msg2)).toEqual(expected); + test('unused fields are not copied', () => expect(verify(msg2)).toEqual(expected)); - // unknown fields are ignored and not copied const msg2a = { ...msg, unknown_data: ['unknown_data'] }; - expect(verify(msg2a)).toEqual(expected); + test('unknown fields are ignored and not copied', () => + expect(verify(msg2a)).toEqual(expected)); }); } }); From 8057ccaae1c8f3beeda35855bb2ebc091887cb7e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 11:00:48 -0800 Subject: [PATCH 19/24] notif tests: Test actual old styles when testing handling of old styles This reflects what this was testing before 144dbb195, where we started expecting realm_uri. The ancient payloads that didn't have recipient_type didn't have that either. --- src/notification/__tests__/notification-test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index c6f00a13582..3e441c2339c 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -154,14 +154,14 @@ describe('extract iOS notification data', () => { expect(makeRaw({ initechData: 'everything' })).toThrow(/alien/); }); - test('very-old-style messages', () => { + test('unsupported old-style messages', () => { const sender_email = 'nobody@example.com'; - // baseline - expect(make({ realm_uri, recipient_type: 'private', sender_email })()).toBeTruthy(); - // missing recipient_type - expect(make({ realm_uri, sender_email })).toThrow(/archaic/); - // missing realm_uri + // pre-1.8 + expect(make({ sender_email })).toThrow(/archaic/); + // pre-1.9 expect(make({ recipient_type: 'private', sender_email })).toThrow(/archaic/); + // baseline, for comparison + expect(make({ realm_uri, recipient_type: 'private', sender_email })()).toBeTruthy(); }); test('broken or partial messages', () => { From 7a83937a24f8c31860f143b673ded02cb94596cb Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 11:12:12 -0800 Subject: [PATCH 20/24] notif tests: Be more systematic in "broken" examples --- .../__tests__/notification-test.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 3e441c2339c..bc6fb8e5c79 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -148,6 +148,8 @@ describe('extract iOS notification data', () => { const make = (data: JSONableDict) => () => extractIosNotificationData({ zulip: data }); describe('failure', () => { + const sender_email = 'nobody@example.com'; + test('completely malformed or inappropriate messages', () => { expect(makeRaw({})).toThrow(); expect(makeRaw({ message_ids: [1] })).toThrow(); @@ -155,7 +157,6 @@ describe('extract iOS notification data', () => { }); test('unsupported old-style messages', () => { - const sender_email = 'nobody@example.com'; // pre-1.8 expect(make({ sender_email })).toThrow(/archaic/); // pre-1.9 @@ -166,12 +167,25 @@ describe('extract iOS notification data', () => { test('broken or partial messages', () => { expect(make({ realm_uri, recipient_type: 'huddle' })).toThrow(/invalid/); - expect(make({ realm_uri, recipient_type: 'stream' })).toThrow(/invalid/); + + expect( + make({ realm_uri, recipient_type: 'stream', stream: 'stream name', topic: 'topic' })(), + ).toBeTruthy(); expect(make({ realm_uri, recipient_type: 'stream', stream: 'stream name' })).toThrow( /invalid/, ); expect(make({ realm_uri, recipient_type: 'stream', subject: 'topic' })).toThrow(/invalid/); + expect(make({ realm_uri, recipient_type: 'stream' })).toThrow(/invalid/); + + expect(make({ realm_uri, recipient_type: 'private', sender_email })()).toBeTruthy(); + expect(make({ realm_uri, recipient_type: 'private' })).toThrow(/invalid/); expect(make({ realm_uri, recipient_type: 'private', subject: 'topic' })).toThrow(/invalid/); + + expect(make({ realm_uri, recipient_type: 'private', pm_users: '12,345' })()).toBeTruthy(); + expect(make({ realm_uri, recipient_type: 'private', pm_users: 123 })).toThrow(/invalid/); + expect(make({ realm_uri, recipient_type: 'private', pm_users: [1, 23] })).toThrow(/invalid/); + expect(make({ realm_uri, recipient_type: 'private', pm_users: '12,ab' })).toThrow(/invalid/); + expect(make({ realm_uri, recipient_type: 'private', pm_users: '12,' })).toThrow(/invalid/); }); test('values of incorrect type', () => { From ff09c95a27376e44a4a9ab6682477c44b4e8347d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 11:16:50 -0800 Subject: [PATCH 21/24] notif tests [nfc]: Make an example more compact in formatting --- src/notification/__tests__/notification-test.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index bc6fb8e5c79..f5858b9b839 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -196,12 +196,7 @@ describe('extract iOS notification data', () => { /invalid/, ); expect( - make({ - realm_uri, - recipient_type: 'stream', - stream: { name: 'somewhere' }, - topic: 'no', - }), + make({ realm_uri, recipient_type: 'stream', stream: { name: 'somewhere' }, topic: 'no' }), ).toThrow(/invalid/); }); From d812b10b87a73d647c8efba108fc998a9a495dd9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 13 Jan 2022 14:49:08 -0800 Subject: [PATCH 22/24] wip android notif: Require user_id, relying on Zulip 2.1+ TODO test manually --- .../zulipmobile/notifications/FcmMessage.kt | 18 ++++-------------- .../notifications/FcmMessageTest.kt | 10 ++-------- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt index 3fbd716ab82..759198468b0 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt @@ -24,7 +24,7 @@ data class Identity( /// This user's ID within the server. Useful mainly in the case where /// the user has multiple accounts in the same org. - val userId: Int?, + val userId: Int, ) /** @@ -120,7 +120,7 @@ data class MessageFcmMessage( // NOTE: Keep the JS-side type definition in sync with this code. buildArray { list -> list.add("realm_uri" to identity.realmUri.toString()) - identity.userId?.let { list.add("user_id" to it) } + list.add("user_id" to identity.userId) when (recipient) { is Recipient.Stream -> { list.add("recipient_type" to "stream") @@ -203,18 +203,8 @@ private fun extractIdentity(data: Map): Identity = // `realm_uri` was added in server version 1.9.0 realmUri = data.require("realm_uri").parseUrl("realm_uri"), - // Server versions from 1.6.0 up to 3.0~6772 send the user's email - // address, as `user`. We *could* use this as a substitute for - // `user_id` when that's missing... but it'd be inherently buggy, - // and the bug it'd introduce seems likely to affect more users - // than the bug it'd fix. So just ignore. - // TODO(server-2.1): Delete this comment, relying on user_id. - // (data["user"] ignored) - - // `user_id` was added in server version 2.1.0 (released 2019-12-12; - // commit 447a517e6, PR #12172.) - // TODO(server-2.1): Require this. - userId = data["user_id"]?.parseInt("user_id") + // `user_id` was added in server version 2.1.0. + userId = data.require("user_id").parseInt("user_id") ) private fun Map.require(key: String): String = diff --git a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt index 9b803820542..ecdd4cb1428 100644 --- a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt +++ b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt @@ -179,7 +179,6 @@ class MessageFcmMessageTest : FcmMessageTestBase() { streamName = Example.stream["stream"]!!, topic = Example.stream["topic"]!! )) - expect.that(parse(Example.pm.minus("user_id")).identity.userId).isNull() } @Test @@ -192,8 +191,6 @@ class MessageFcmMessageTest : FcmMessageTestBase() { "stream_name" to Example.stream["stream"]!!, "topic" to Example.stream["topic"]!!, ) - expect.that(dataForOpen(Example.stream.minus("user_id"))) - .isEqualTo(baseExpected.minus("user_id")) expect.that(dataForOpen(Example.stream.minus("stream_id"))) .isEqualTo(baseExpected.minus("stream_id")) } @@ -215,6 +212,7 @@ class MessageFcmMessageTest : FcmMessageTestBase() { assertParseFails(Example.pm.minus("realm_uri")) assertParseFails(Example.pm.plus("realm_uri" to "zulip.example.com")) assertParseFails(Example.pm.plus("realm_uri" to "/examplecorp")) + assertParseFails(Example.pm.minus("user_id")) assertParseFails(Example.stream.minus("recipient_type")) assertParseFails(Example.stream.plus("stream_id" to "12,34")) @@ -293,11 +291,6 @@ class RemoveFcmMessageTest : FcmMessageTestBase() { ) } - @Test - fun `optional fields missing cause no error`() { - expect.that(parse(Example.hybrid.minus("user_id")).identity.userId).isNull() - } - @Test fun `parse failures on malformed data`() { assertParseFails(Example.hybrid.minus("server")) @@ -307,6 +300,7 @@ class RemoveFcmMessageTest : FcmMessageTestBase() { assertParseFails(Example.hybrid.minus("realm_uri")) assertParseFails(Example.hybrid.plus("realm_uri" to "zulip.example.com")) assertParseFails(Example.hybrid.plus("realm_uri" to "/examplecorp")) + assertParseFails(Example.hybrid.minus("user_id")) for (badIntList in sequenceOf( "abc,34", From 9fa2bbabe2df902763049e90f45a579018725c4d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 11:18:10 -0800 Subject: [PATCH 23/24] wip notif: Expect user_id, relying on Zulip 2.1+ TODO test manually; perhaps squash with Android side --- src/api/notificationTypes.js | 4 +- .../__tests__/notification-test.js | 48 ++++++++++--------- src/notification/extract.js | 11 ++--- 3 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/api/notificationTypes.js b/src/api/notificationTypes.js index 483cfe74da5..67a685007ca 100644 --- a/src/api/notificationTypes.js +++ b/src/api/notificationTypes.js @@ -32,9 +32,7 @@ type BaseData = {| * * (This lets us distinguish different accounts in the same realm.) */ - // added 2.1-dev-540-g447a517e6f, release 2.1.0+ - // TODO(server-2.1): Mark required; simplify comment. - +user_id?: UserId, + +user_id: UserId, // The rest of the data is about the Zulip message we're being notified // about. diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index f5858b9b839..2ce9cd3bca5 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -128,10 +128,6 @@ describe('extract iOS notification data', () => { const msg = data; test('baseline', () => expect(verify(msg)).toEqual(expected)); - const { user_id: _ignore, ...msg1 } = msg; // eslint-disable-line no-unused-vars - const { user_id: _ignore_, ...expected1 } = expected; // eslint-disable-line no-unused-vars - test('pre-2.1 message missing user_id', () => expect(verify(msg1)).toEqual(expected1)); - const msg2 = { ...msg, realm_id: 8675309 }; test('unused fields are not copied', () => expect(verify(msg2)).toEqual(expected)); @@ -161,42 +157,48 @@ describe('extract iOS notification data', () => { expect(make({ sender_email })).toThrow(/archaic/); // pre-1.9 expect(make({ recipient_type: 'private', sender_email })).toThrow(/archaic/); + // pre-2.1 + expect(make({ realm_uri, recipient_type: 'private', sender_email })).toThrow(/archaic/); // baseline, for comparison - expect(make({ realm_uri, recipient_type: 'private', sender_email })()).toBeTruthy(); + expect(make({ realm_uri, user_id, recipient_type: 'private', sender_email })()).toBeTruthy(); }); test('broken or partial messages', () => { - expect(make({ realm_uri, recipient_type: 'huddle' })).toThrow(/invalid/); + expect(make({ ...identity, recipient_type: 'huddle' })).toThrow(/invalid/); expect( - make({ realm_uri, recipient_type: 'stream', stream: 'stream name', topic: 'topic' })(), + make({ ...identity, recipient_type: 'stream', stream: 'stream name', topic: 'topic' })(), ).toBeTruthy(); - expect(make({ realm_uri, recipient_type: 'stream', stream: 'stream name' })).toThrow( + expect(make({ ...identity, recipient_type: 'stream', stream: 'stream name' })).toThrow( + /invalid/, + ); + expect(make({ ...identity, recipient_type: 'stream', subject: 'topic' })).toThrow(/invalid/); + expect(make({ ...identity, recipient_type: 'stream' })).toThrow(/invalid/); + + expect(make({ ...identity, recipient_type: 'private', sender_email })()).toBeTruthy(); + expect(make({ ...identity, recipient_type: 'private' })).toThrow(/invalid/); + expect(make({ ...identity, recipient_type: 'private', subject: 'topic' })).toThrow(/invalid/); + + expect(make({ ...identity, recipient_type: 'private', pm_users: '12,345' })()).toBeTruthy(); + expect(make({ ...identity, recipient_type: 'private', pm_users: 123 })).toThrow(/invalid/); + expect(make({ ...identity, recipient_type: 'private', pm_users: [1, 23] })).toThrow( + /invalid/, + ); + expect(make({ ...identity, recipient_type: 'private', pm_users: '12,ab' })).toThrow( /invalid/, ); - expect(make({ realm_uri, recipient_type: 'stream', subject: 'topic' })).toThrow(/invalid/); - expect(make({ realm_uri, recipient_type: 'stream' })).toThrow(/invalid/); - - expect(make({ realm_uri, recipient_type: 'private', sender_email })()).toBeTruthy(); - expect(make({ realm_uri, recipient_type: 'private' })).toThrow(/invalid/); - expect(make({ realm_uri, recipient_type: 'private', subject: 'topic' })).toThrow(/invalid/); - - expect(make({ realm_uri, recipient_type: 'private', pm_users: '12,345' })()).toBeTruthy(); - expect(make({ realm_uri, recipient_type: 'private', pm_users: 123 })).toThrow(/invalid/); - expect(make({ realm_uri, recipient_type: 'private', pm_users: [1, 23] })).toThrow(/invalid/); - expect(make({ realm_uri, recipient_type: 'private', pm_users: '12,ab' })).toThrow(/invalid/); - expect(make({ realm_uri, recipient_type: 'private', pm_users: '12,' })).toThrow(/invalid/); + expect(make({ ...identity, recipient_type: 'private', pm_users: '12,' })).toThrow(/invalid/); }); test('values of incorrect type', () => { - expect(make({ realm_uri, recipient_type: 'private', pm_users: [1, 2, 3] })).toThrow( + expect(make({ ...identity, recipient_type: 'private', pm_users: [1, 2, 3] })).toThrow( /invalid/, ); - expect(make({ realm_uri, recipient_type: 'stream', stream: [], topic: 'yes' })).toThrow( + expect(make({ ...identity, recipient_type: 'stream', stream: [], topic: 'yes' })).toThrow( /invalid/, ); expect( - make({ realm_uri, recipient_type: 'stream', stream: { name: 'somewhere' }, topic: 'no' }), + make({ ...identity, recipient_type: 'stream', stream: { name: 'somewhere' }, topic: 'no' }), ).toThrow(/invalid/); }); diff --git a/src/notification/extract.js b/src/notification/extract.js index 2a4422571e5..2c9bde8d148 100644 --- a/src/notification/extract.js +++ b/src/notification/extract.js @@ -103,14 +103,13 @@ export const fromAPNsImpl = (data: ?JSONableDict): Notification | void => { if (typeof realm_uri !== 'string') { throw err('invalid'); } - if (user_id !== undefined && typeof user_id !== 'number') { + if (user_id === undefined) { + throw err('archaic (pre-2.1)'); + } + if (typeof user_id !== 'number') { throw err('invalid'); } - - const identity = { - realm_uri, - ...(user_id === undefined ? Object.freeze({}) : { user_id: makeUserId(user_id) }), - }; + const identity = { realm_uri, user_id: makeUserId(user_id) }; if (recipient_type === 'stream') { const { stream: stream_name, stream_id, topic } = zulip; From 5431c297c61695ab052b104e42df617ef4cce1ee Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 1 Feb 2022 11:21:45 -0800 Subject: [PATCH 24/24] wip notif nfc rely on user_id in finding account TODO perhaps rearrange/squash this, the Android, and the iOS sides --- .../__tests__/notification-test.js | 4 ++++ src/notification/notifOpen.js | 18 ------------------ src/notification/types.js | 2 +- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 2ce9cd3bca5..fbbaad66c47 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -29,6 +29,7 @@ describe('getNarrowFromNotificationData', () => { const streamsByName = new Map([[stream.name, stream]]); const notification = { realm_uri, + user_id, recipient_type: 'stream', stream_id: eg.stream.stream_id, // Name points to some other stream, but the ID prevails. @@ -45,6 +46,7 @@ describe('getNarrowFromNotificationData', () => { const streamsByName = new Map([[stream.name, stream]]); const notification = { realm_uri, + user_id, recipient_type: 'stream', stream_name: 'some stream', topic: 'some topic', @@ -58,6 +60,7 @@ describe('getNarrowFromNotificationData', () => { const allUsersByEmail: Map = new Map(users.map(u => [u.email, u])); const notification = { realm_uri, + user_id, recipient_type: 'private', sender_email: eg.otherUser.email, }; @@ -76,6 +79,7 @@ describe('getNarrowFromNotificationData', () => { const notification = { realm_uri, + user_id, recipient_type: 'private', pm_users: users.map(u => u.user_id).join(','), }; diff --git a/src/notification/notifOpen.js b/src/notification/notifOpen.js index 9ad76ca9f1b..856fd15448b 100644 --- a/src/notification/notifOpen.js +++ b/src/notification/notifOpen.js @@ -72,24 +72,6 @@ export const getAccountFromNotificationData = ( return null; } - // TODO(server-2.1): Remove this, because user_id will always be present - if (user_id === undefined) { - if (urlMatches.length > 1) { - logging.warn( - 'notification realm_uri ambiguous; multiple matches found; user_id missing (old server)', - { - realm_uri, - parsed_url: realmUrl.toString(), - match_count: urlMatches.length, - unique_identities_count: new Set(urlMatches.map(account => account.email)).size, - }, - ); - return null; - } else { - return identityOfAccount(urlMatches[0]); - } - } - // There may be multiple accounts in the notification's realm. Pick one // based on the notification's `user_id`. const userMatch = urlMatches.find(account => account.userId === user_id); diff --git a/src/notification/types.js b/src/notification/types.js index a6bc92f949f..f698b8026bf 100644 --- a/src/notification/types.js +++ b/src/notification/types.js @@ -3,7 +3,7 @@ import type { UserId } from '../types'; type NotificationBase = {| realm_uri: string, - user_id?: UserId, + user_id: UserId, |}; /**