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..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") @@ -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) } @@ -206,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 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. - // (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 15f006b7424..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")) @@ -250,20 +248,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,20 +285,12 @@ 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()) ) } - @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")) @@ -316,15 +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")) - - 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)) - } + assertParseFails(Example.hybrid.minus("user_id")) for (badIntList in sequenceOf( "abc,34", 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/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/api/messages/sendMessage.js b/src/api/messages/sendMessage.js index 371027ad715..90457dd8d0d 100644 --- a/src/api/messages/sendMessage.js +++ b/src/api/messages/sendMessage.js @@ -1,25 +1,26 @@ /* @flow strict-local */ import type { ApiResponse, Auth } from '../transportTypes'; +import type { UserId } from '../idTypes'; 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, - // TODO(server-2.0): Say "topic", not "subject" - subject?: string, - content: string, - localId?: number, - eventQueueId?: string, - |}, + params: + | {| ...CommonParams, type: 'stream', to: number, topic: string |} + | {| ...CommonParams, type: 'private', to: $ReadOnlyArray |}, ): Promise => apiPost(auth, 'messages', { type: params.type, - to: params.to, - subject: params.subject, + to: JSON.stringify(params.to), + topic: (params: { +topic?: string, ... }).topic, content: params.content, local_id: params.localId, queue_id: params.eventQueueId, 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/api/modelTypes.js b/src/api/modelTypes.js index 79e279fd2dd..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, /** @@ -1062,6 +1065,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 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/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/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); }); } diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 056bdf77b96..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(','), }; @@ -94,55 +98,46 @@ 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)) { - test(`${type} notification`, () => { + for (const [type, data] of objectEntries(cases)) { + describe(`${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 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 }); + test('baseline', () => expect(verify(msg)).toEqual(expected)); - // 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)); }); } }); @@ -153,56 +148,71 @@ 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(); expect(makeRaw({ initechData: 'everything' })).toThrow(/alien/); }); - test('very-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 + test('unsupported old-style messages', () => { + // pre-1.8 + 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, user_id, recipient_type: 'private', sender_email })()).toBeTruthy(); }); 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' })).toThrow( + expect(make({ ...identity, recipient_type: 'huddle' })).toThrow(/invalid/); + + expect( + make({ ...identity, recipient_type: 'stream', stream: 'stream name', topic: 'topic' })(), + ).toBeTruthy(); + 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: 'private', subject: 'topic' })).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/); }); 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', () => { 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; diff --git a/src/notification/notifOpen.js b/src/notification/notifOpen.js index 268b7ea2a12..856fd15448b 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) { @@ -78,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, |}; /** diff --git a/src/outbox/outboxActions.js b/src/outbox/outboxActions.js index ce7720c5553..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, streamNameOfStreamMessage } from '../utils/recipient'; +import { recipientIdsOfPrivateMessage } from '../utils/recipient'; export const messageSendStart = (outbox: Outbox): PerAccountAction => ({ type: MESSAGE_SEND_START, @@ -69,24 +69,17 @@ const trySendMessages = (dispatch, getState): boolean => { return; // i.e., continue } - // 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(',') - // 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)]); - - 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: recipientIdsOfPrivateMessage(item) } + : { ...commonParams, type: 'stream', to: item.stream_id, topic: item.subject }; + await api.sendMessage(auth, params); dispatch(messageSendComplete(item.timestamp)); }); return true; 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/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..e1bf790f074 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: { @@ -194,15 +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, - // TODO(server-2.0): switch to numeric stream ID (#3918), not name; - // then drop streamName from SendTo - to: sendTo.streamName, + topic: sendTo.topic || apiConstants.kNoTopicTopic, + to: sendTo.streamId, }; try { 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; } diff --git a/src/users/usersActions.js b/src/users/usersActions.js index 257c3c0a700..15a49c1fdb7 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, user_ids_array, 'start'); }, notify_server_stop: (user_ids_array: $ReadOnlyArray) => { - api.typing(auth, getRecipients(user_ids_array), 'stop'); + api.typing(auth, user_ids_array, 'stop'); }, }; }; 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') {