Fix some more stream names to be IDs, and mark the rest

android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt

@@ -55,6 +55,8 @@ sealed class Recipient {
     }
 
     /** A stream message. */
+    // TODO(server-5.0): Require the stream ID (#3918).
+    // TODO(#3918): Get the stream ID, when present.
     data class Stream(val streamName: String, val topic: String) : Recipient()
 }
 

android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt

@@ -195,11 +195,13 @@ private fun extractGroupKey(identity: Identity): String {
 private fun extractConversationKey(fcmMessage: MessageFcmMessage): String {
     val groupKey = extractGroupKey(fcmMessage.identity)
     val conversation = when (fcmMessage.recipient) {
-        // TODO(#3918): Use the stream ID here instead of the stream name,
-        //   so things stay together if the stream is renamed.
-        // So long as this does use the stream name, we use `\u0000` as the delimiter because
+        // So long as this uses the stream name, we use `\u0000` as the delimiter because
         // it's the one character not allowed in Zulip stream names.
         // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.)
+        // TODO(server-5.0): Rely on the stream ID (#3918).
+        // TODO(#3918): When we have the stream ID, use that here instead of the
+        //   stream name, so things stay together if the stream is renamed.
+        //   See: https://github.com/zulip/zulip/issues/18067
         is Recipient.Stream -> "stream:${fcmMessage.recipient.streamName}\u0000${fcmMessage.recipient.topic}"
         is Recipient.GroupPm -> "groupPM:${fcmMessage.recipient.pmUsers.toString()}"
         is Recipient.Pm -> "private:${fcmMessage.sender.id}"

src/action-sheets/index.js

@@ -156,6 +156,7 @@ markTopicAsRead.errorMessage = 'Failed to mark topic as read';
 const unmuteTopic = async ({ auth, streamId, topic, streams }) => {
   const stream = streams.get(streamId);
   invariant(stream !== undefined, 'Stream with provided streamId must exist.');
+  // This still uses a stream name (#3918) because the API method does; see there.
   await api.setTopicMute(auth, stream.name, topic, false);
 };
 unmuteTopic.title = 'Unmute topic';
@@ -164,6 +165,7 @@ unmuteTopic.errorMessage = 'Failed to unmute topic';
 const muteTopic = async ({ auth, streamId, topic, streams }) => {
   const stream = streams.get(streamId);
   invariant(stream !== undefined, 'Stream with provided streamId must exist.');
+  // This still uses a stream name (#3918) because the API method does; see there.
   await api.setTopicMute(auth, stream.name, topic, true);
 };
 muteTopic.title = 'Mute topic';
@@ -223,6 +225,7 @@ showStreamSettings.errorMessage = 'Failed to show stream settings';
 const subscribe = async ({ auth, streamId, streams }) => {
   const stream = streams.get(streamId);
   invariant(stream !== undefined, 'Stream with provided streamId not found.');
+  // This still uses a stream name (#3918) because the API method does; see there.
   await api.subscriptionAdd(auth, [{ name: stream.name }]);
 };
 subscribe.title = 'Subscribe';
@@ -231,6 +234,7 @@ subscribe.errorMessage = 'Failed to subscribe';
 const unsubscribe = async ({ auth, streamId, subscriptions }) => {
   const sub = subscriptions.get(streamId);
   invariant(sub !== undefined, 'Subscription with provided streamId not found.');
+  // This still uses a stream name (#3918) because the API method does; see there.
   await api.subscriptionRemove(auth, [sub.name]);
 };
 unsubscribe.title = 'Unsubscribe';

src/actionTypes.js

@@ -288,7 +288,7 @@ type EventSubscriptionRemoveAction = $ReadOnly<{|
   ...ServerEvent,
   type: typeof EVENT_SUBSCRIPTION,
   op: 'remove',
-  subscriptions: $ReadOnlyArray<{| +stream_id: number, +name: string |}>,
+  subscriptions: $ReadOnlyArray<{| +stream_id: number, -name: string |}>,
 |}>;
 
 type EventSubscriptionUpdateAction = $ReadOnly<{|

src/api/modelTypes.js

@@ -304,20 +304,20 @@ export type MutedUser = $ReadOnly<{|
 //
 //
 
-export type Stream = $ReadOnly<{|
-  stream_id: number,
-  description: string,
-  name: string,
-  invite_only: boolean,
-  is_announcement_only: boolean,
+export type Stream = {|
+  +stream_id: number,
+  +description: string,
+  +name: string,
+  +invite_only: boolean,
+  +is_announcement_only: boolean,
   // TODO(server-2.1): is_web_public was added in Zulip version 2.1;
   //   absence implies the stream is not web-public.
-  is_web_public?: boolean,
-  history_public_to_subscribers: boolean,
-|}>;
+  +is_web_public?: boolean,
+  +history_public_to_subscribers: boolean,
+|};
 
 export type Subscription = {|
-  ...$ReadOnly<$Exact<Stream>>,
+  ...Stream,
   +color: string,
   +in_home_view: boolean,
   +pin_to_top: boolean,
@@ -369,7 +369,7 @@ export type NarrowElement =
  //  * `group-pm-with` since 2.1-dev-1813-gb338fd130
  //  * `sender` since 2.1-dev-1812-gc067c155a
  //  * `pm-with` since 2.1-dev-1350-gd7b4de234
- // TODO(server-2.1): Stop sending stream names or user emails here.
+ // TODO(server-2.1): Stop sending stream names (#3918) or user emails (#3764) here.
  | {| +operator: 'stream', +operand: string | number |} // stream ID
  | {| +operator: 'pm-with', +operand: string | $ReadOnlyArray<UserId> |}
  | {| +operator: 'sender', +operand: string | UserId |}

src/api/notificationTypes.js

@@ -2,6 +2,22 @@
 
 import { type UserId } from './idTypes';
 
+//
+// Note: No code actually refers to these type definitions!
+//
+// The code that consumes this part of the API is in:
+//   src/notification/extract.js
+//   android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt
+// of which the latter is used on Android, and the former on iOS.
+//
+// The Android-side code doesn't use these because it's not in JS.
+// (Moreover the data it receives is slightly different; see that code, or
+// the server at zerver/lib/push_notifications.py , for details.)
+//
+// The JS-side code has a signature taking an arbitrary JSON blob, so that
+// the type-checker can verify it systematically validates all the pieces it
+// uses.  But these types describe the form of data it *expects* to receive.
+
 // This format is as of 2020-02, commit 3.0~3347.
 type BaseData = {|
   /** The realm URL, same as in the `/server_settings` response. */
@@ -17,8 +33,8 @@ 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): Simplify comment.
-  +user_id: UserId,
+  // TODO(server-2.1): Mark required; simplify comment.
+  +user_id?: UserId,
 
   // The rest of the data is about the Zulip message we're being notified
   // about.
@@ -38,11 +54,12 @@ type StreamData = {|
 
   /**
    * The name of the message's stream.
-   *
-   * Sadly no stream ID: https://github.com/zulip/zulip/issues/18067
    */
   +stream: string,
 
+  // TODO(server-5.0): Require stream_id (#3918).
+  // +stream_id?: number, // TODO(#3918): Add this, and use it.
+
   +topic: string,
 |};
 
@@ -62,6 +79,8 @@ type PmData = {|
 |};
 
 /** An APNs message sent by the Zulip server. */
+// Note that no code actually refers to this type!  Rather it serves as
+// documentation.  See module comment.
 export type ApnsPayload = {|
   /** Our own data. */
   +zulip: StreamData | PmData,

src/api/streams/createStream.js

@@ -7,6 +7,7 @@ export default (
   auth: Auth,
   name: string,
   description?: string = '',
+  // TODO(server-3.0): Send numeric user IDs (#3764), not emails.
   principals?: $ReadOnlyArray<string> = [],
   inviteOnly?: boolean = false,
   announce?: boolean = false,

src/api/subscriptions/setTopicMute.js

@@ -5,6 +5,12 @@ import { apiPatch } from '../apiFetch';
 /** See https://zulip.com/api/mute-topic */
 export default async (
   auth: Auth,
+  // TODO(server-2.0): Switch to stream ID (#3918), instead of name.
+  //   (The version that was introduced in isn't documented:
+  //     https://github.com/zulip/zulip/issues/11136#issuecomment-1033046851
+  //   but see:
+  //     https://github.com/zulip/zulip-mobile/issues/3244#issuecomment-840200325
+  //   )
   stream: string,
   topic: string,
   value: boolean,

src/api/subscriptions/subscriptionAdd.js

@@ -3,13 +3,17 @@ import type { ApiResponse, Auth } from '../transportTypes';
 import { apiPost } from '../apiFetch';
 
 type SubscriptionObj = {|
+  // TODO(server-future): This should use a stream ID (#3918), not stream name.
+  //   Server issue: https://github.com/zulip/zulip/issues/10744
+  // TODO(#3918): Change example in docs/style.md to something without this issue.
   name: string,
 |};
 
 /** See https://zulip.com/api/subscribe */
 export default (
   auth: Auth,
   subscriptions: $ReadOnlyArray<SubscriptionObj>,
+  // TODO(server-3.0): Send numeric user IDs (#3764), not emails.
   principals?: $ReadOnlyArray<string>,
 ): Promise<ApiResponse> =>
   apiPost(auth, 'users/me/subscriptions', {

src/api/subscriptions/subscriptionRemove.js

@@ -5,7 +5,10 @@ import { apiDelete } from '../apiFetch';
 /** See https://zulip.com/api/unsubscribe */
 export default (
   auth: Auth,
+  // TODO(server-future): This should use a stream ID (#3918), not stream name.
+  //   Server issue: https://github.com/zulip/zulip/issues/10744
   subscriptions: $ReadOnlyArray<string>,
+  // TODO(server-3.0): Send numeric user IDs (#3764), not emails.
   principals?: $ReadOnlyArray<string>,
 ): Promise<ApiResponse> =>
   apiDelete(auth, 'users/me/subscriptions', {

src/autocomplete/StreamAutocomplete.js

@@ -18,12 +18,9 @@ export default function StreamAutocomplete(props: Props): Node {
   const { filter, onAutocomplete } = props;
   const subscriptions = useSelector(getSubscriptions);
 
-  const handleStreamItemAutocomplete = useCallback(
-    (streamId: number, streamName: string): void => {
-      onAutocomplete(`**${streamName}**`);
-    },
-    [onAutocomplete],
-  );
+  const handleStreamItemAutocomplete = useCallback(stream => onAutocomplete(`**${stream.name}**`), [
+    onAutocomplete,
+  ]);
 
   const isPrefixMatch = x => x.name.toLowerCase().startsWith(filter.toLowerCase());
 

src/chat/narrowsSelectors.js

@@ -12,13 +12,7 @@ import type {
   Stream,
   Subscription,
 } from '../types';
-import {
-  getAllNarrows,
-  getSubscriptions,
-  getMessages,
-  getOutbox,
-  getFlags,
-} from '../directSelectors';
+import { getAllNarrows, getMessages, getOutbox, getFlags } from '../directSelectors';
 import { getCaughtUpForNarrow } from '../caughtup/caughtUpSelectors';
 import { getAllUsersById, getOwnUserId } from '../users/userSelectors';
 import {
@@ -30,7 +24,6 @@ import {
   streamIdOfNarrow,
 } from '../utils/narrow';
 import { getMute, isTopicMuted } from '../mute/muteModel';
-import { streamNameOfStreamMessage } from '../utils/recipient';
 import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects';
 import * as logging from '../utils/logging';
 import { getStreamsById, getSubscriptionsById } from '../selectors';
@@ -96,10 +89,10 @@ export const getMessagesForNarrow: Selector<$ReadOnlyArray<Message | Outbox>, Na
 
 /** Whether this stream's messages should appear in the "all messages" narrow. */
 const showStreamInHomeNarrow = (
-  streamName: string,
-  subscriptions: $ReadOnlyArray<Subscription>,
+  streamId: number,
+  subscriptions: Map<number, Subscription>,
 ): boolean => {
-  const sub = subscriptions.find(x => x.name === streamName);
+  const sub = subscriptions.get(streamId);
   if (!sub) {
     // If there's no matching subscription, then the user must have
     // unsubscribed from the stream since the message was received.  Leave
@@ -122,7 +115,7 @@ export const getShownMessagesForNarrow: Selector<$ReadOnlyArray<Message | Outbox
   createSelector(
     (state, narrow) => narrow,
     getMessagesForNarrow,
-    state => getSubscriptions(state),
+    state => getSubscriptionsById(state),
     state => getMute(state),
     state => getFlags(state),
     (narrow, messagesForNarrow, subscriptions, mute, flags) =>
@@ -135,9 +128,8 @@ export const getShownMessagesForNarrow: Selector<$ReadOnlyArray<Message | Outbox
             if (flags.mentioned[message.id]) {
               return true;
             }
-            const streamName = streamNameOfStreamMessage(message);
             return (
-              showStreamInHomeNarrow(streamName, subscriptions)
+              showStreamInHomeNarrow(message.stream_id, subscriptions)
               && !isTopicMuted(message.stream_id, message.subject, mute)
             );
           }),

src/message/MentionedUserNotSubscribed.js

@@ -59,6 +59,7 @@ export default function MentionedUserNotSubscribed(props: Props): Node {
   }, [user, onDismiss]);
 
   const subscribeToStream = useCallback(() => {
+    // This still uses a stream name (#3918) because the API method does; see there.
     api.subscriptionAdd(auth, [{ name: stream.name }], [user.email]);
     handleDismiss();
   }, [auth, handleDismiss, stream, user]);

src/message/NotSubscribed.js

@@ -22,6 +22,7 @@ export default function NotSubscribed(props: Props): Node {
   );
 
   const subscribeToStream = useCallback(() => {
+    // This still uses a stream name (#3918) because the API method does; see there.
     api.subscriptionAdd(auth, [{ name: stream.name }]);
   }, [auth, stream]);
 

src/notification/extract.js

@@ -38,6 +38,9 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
   //
   // For the format this parses, see `ApnsPayload` in src/api/notificationTypes.js .
   //
+  // Though what it actually receives is more like this:
+  //   $Rest<ApnsPayload, {| aps: mixed |}>
+  // See comment below about PushNotificationsIOS.
 
   /** Helper function: fail. */
   const err = (style: string) =>

src/notification/index.js

@@ -160,6 +160,8 @@ export const getNarrowFromNotificationData = (
   //   we settle for not crashing.
 
   if (data.recipient_type === 'stream') {
+    // TODO(server-5.0): Always use the stream ID (#3918).
+    // TODO(#3918): Use the notification's own stream_id, where present.
     const stream = streamsByName.get(data.stream_name);
     return (stream && topicNarrow(stream.stream_id, data.topic)) ?? null;
   }

src/notification/types.js

@@ -19,6 +19,10 @@ type NotificationBase = {|
  */
 // NOTE: Keep the Android-side code in sync with this type definition.
 export type Notification =
+  // TODO(server-5.0): Rely on stream ID (#3918).  (We'll still want the
+  //   stream name, as a hint for display in case the stream is unknown;
+  //   see comment in getNarrowFromNotificationData.)
+  // TODO(#3918): Add stream_id.
   | {| ...NotificationBase, recipient_type: 'stream', stream_name: string, topic: string |}
   // Group PM messages have `pm_users`, which is sorted, comma-separated IDs.
   | {| ...NotificationBase, recipient_type: 'private', pm_users: string |}

src/outbox/outboxActions.js

@@ -72,9 +72,9 @@ const trySendMessages = (dispatch, getState): boolean => {
       // prettier-ignore
       const to =
         item.type === 'private'
-            // TODO(server-2.0): switch to numeric user IDs, not emails.
+            // 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, not names.
+            // 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)]);

src/sharing/ShareWrapper.js

@@ -23,7 +23,7 @@ import { IconAttachment, IconCancel } from '../common/Icons';
 
 type SendTo =
   | {| type: 'pm', selectedRecipients: $ReadOnlyArray<UserId> |}
-  // TODO(#3918): Drop streamName.  Used below for sending, and for narrow.
+  // TODO(#3918): Drop streamName.  Used below for sending.
   | {| type: 'stream', streamName: string, streamId: number, topic: string |};
 
 const styles = createStyleSheet({
@@ -174,7 +174,8 @@ class ShareWrapperInner extends React.Component<Props, State> {
             content: messageToSend,
             type: 'stream',
             subject: sendTo.topic || apiConstants.NO_TOPIC_TOPIC,
-            // TODO(server-2.0): switch to numeric stream ID, not name
+            // TODO(server-2.0): switch to numeric stream ID, not name;
+            //   then drop streamName from SendTo
             to: sendTo.streamName,
           };
 

src/streams/InviteUsersScreen.js

@@ -27,6 +27,7 @@ export default function InviteUsersScreen(props: Props): Node {
   const handleInviteUsers = useCallback(
     (selected: $ReadOnlyArray<UserOrBot>) => {
       const recipients = selected.map(user => user.email);
+      // This still uses a stream name (#3918) because the API method does; see there.
       api.subscriptionAdd(auth, [{ name: stream.name }], recipients);
       NavigationService.dispatch(navigateBack());
     },