Fix all remaining stream names to be IDs, modulo server support

android/app/build.gradle

@@ -224,6 +224,7 @@ repositories {
 dependencies {
     implementation fileTree(dir: "libs", include: ["*.jar"])
     implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
+    implementation "androidx.core:core-ktx:1.7.0"
     implementation 'androidx.appcompat:appcompat:1.0.0'
     implementation "androidx.swiperefreshlayout:swiperefreshlayout:1.0.0"
     implementation "com.google.firebase:firebase-messaging:17.3.4"

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

@@ -1,6 +1,5 @@
 package com.zulipmobile.notifications
 
-import android.os.Bundle
 import java.net.MalformedURLException
 import java.net.URL
 import java.util.*
@@ -56,8 +55,7 @@ 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()
+    data class Stream(val streamId: Int?, val streamName: String, val topic: String) : Recipient()
 }
 
 /**
@@ -87,6 +85,13 @@ sealed class FcmMessage {
     }
 }
 
+// This exists mainly to give a properly-typed wrapper around ArrayList#toArray.
+inline fun <reified T> buildArray(block: (ArrayList<T>) -> Unit): Array<T> {
+    val result = arrayListOf<T>()
+    block(result)
+    return result.toArray(arrayOf<T>())
+}
+
 /**
  * Parsed version of an FCM message of type `message`.
  *
@@ -111,33 +116,36 @@ data class MessageFcmMessage(
      * For the corresponding type definition on the JS side, see `Notification`
      * in `src/notification/types.js`.
      */
-    fun dataForOpen(): Bundle = Bundle().apply {
+    fun dataForOpen(): Array<Pair<String, Any?>> =
         // NOTE: Keep the JS-side type definition in sync with this code.
-        putString("realm_uri", identity.realmUri.toString())
-        identity.userId?.let { putInt("user_id", it) }
-        when (recipient) {
-            is Recipient.Stream -> {
-                putString("recipient_type", "stream")
-                putString("stream_name", recipient.streamName)
-                putString("topic", recipient.topic)
-            }
-            is Recipient.GroupPm -> {
-                putString("recipient_type", "private")
-                putString("pm_users", recipient.getPmUsersString())
-            }
-            is Recipient.Pm -> {
-                putString("recipient_type", "private")
-                putString("sender_email", sender.email)
+        buildArray { list ->
+            list.add("realm_uri" to identity.realmUri.toString())
+            identity.userId?.let { list.add("user_id" to it) }
+            when (recipient) {
+                is Recipient.Stream -> {
+                    list.add("recipient_type" to "stream")
+                    recipient.streamId?.let { list.add("stream_id" to it) }
+                    list.add("stream_name" to recipient.streamName)
+                    list.add("topic" to recipient.topic)
+                }
+                is Recipient.GroupPm -> {
+                    list.add("recipient_type" to "private")
+                    list.add("pm_users" to recipient.getPmUsersString())
+                }
+                is Recipient.Pm -> {
+                    list.add("recipient_type" to "private")
+                    list.add("sender_email" to sender.email)
+                }
             }
         }
-    }
 
     companion object {
         fun fromFcmData(data: Map<String, String>): MessageFcmMessage {
             val recipientType = data.require("recipient_type")
             val recipient = when (recipientType) {
                 "stream" ->
                     Recipient.Stream(
+                        data["stream_id"]?.parseInt("stream_id"),
                         data.require("stream"),
                         data.require("topic")
                     )

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

@@ -16,6 +16,7 @@ import androidx.core.app.NotificationCompat
 import androidx.core.app.NotificationManagerCompat
 import androidx.core.app.Person
 import androidx.core.graphics.drawable.IconCompat
+import androidx.core.os.bundleOf
 import com.zulipmobile.BuildConfig
 import com.zulipmobile.MainActivity
 import com.zulipmobile.R
@@ -192,14 +193,19 @@ private fun extractGroupKey(identity: Identity): String {
 private fun extractConversationKey(fcmMessage: MessageFcmMessage): String {
     val groupKey = extractGroupKey(fcmMessage.identity)
     val conversation = when (fcmMessage.recipient) {
-        // 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.Stream -> when (fcmMessage.recipient.streamId) {
+            // When using 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.)
+            null -> "stream:${fcmMessage.recipient.streamName}\u0000${fcmMessage.recipient.topic}"
+
+            // The conditional use of streamId means a slight glitch: when upgrading either the
+            // client or server (whichever comes later) so that we start using stream IDs, any
+            // existing notifications from before the upgrade won't get threaded together with new
+            // ones.  That seems OK; after all it's inherently transient.
+            else -> "stream:${fcmMessage.recipient.streamId}:${fcmMessage.recipient.topic}"
+        }
         is Recipient.GroupPm -> "groupPM:${fcmMessage.recipient.pmUsers.toString()}"
         is Recipient.Pm -> "private:${fcmMessage.sender.id}"
     }
@@ -296,10 +302,10 @@ private fun updateNotification(
 
         setNumber(messagingStyle.messages.size)
 
-        extras = Bundle().apply {
+        setExtras(bundleOf(
             // We use this for deciding when a RemoveFcmMessage should clear this notification.
-            putInt("lastZulipMessageId", fcmMessage.zulipMessageId)
-        }
+            "lastZulipMessageId" to fcmMessage.zulipMessageId
+        ))
 
         // Our own code doesn't look at the message details in this URL,
         // the "data URL" we put on the intent.  Instead, we get data from
@@ -334,7 +340,7 @@ private fun updateNotification(
                         //   all the activities on top of the target one.
                         Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP
                     )
-                    .putExtra(EXTRA_NOTIFICATION_DATA, fcmMessage.dataForOpen()),
+                    .putExtra(EXTRA_NOTIFICATION_DATA, bundleOf(*fcmMessage.dataForOpen())),
                 PendingIntent.FLAG_IMMUTABLE))
         setAutoCancel(true)
     }.build()

android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt

@@ -86,6 +86,7 @@ class MessageFcmMessageTest : FcmMessageTestBase() {
 
         val stream = base.plus(sequenceOf(
             "recipient_type" to "stream",
+            "stream_id" to "42",
             "stream" to "denmark",
             "topic" to "play",
 
@@ -115,6 +116,9 @@ class MessageFcmMessageTest : FcmMessageTestBase() {
     private fun parse(data: Map<String, String>) =
         FcmMessage.fromFcmData(data) as MessageFcmMessage
 
+    private fun dataForOpen(data: Map<String, String>) =
+        mapOf(*parse(data).dataForOpen())
+
     @Test
     fun `fields get parsed right in 'message' happy path`() {
         expect.that(parse(Example.stream)).isEqualTo(
@@ -128,6 +132,7 @@ class MessageFcmMessageTest : FcmMessageTestBase() {
                 ),
                 zulipMessageId = 12345,
                 recipient = Recipient.Stream(
+                    streamId = Example.stream["stream_id"]!!.toInt(),
                     streamName = Example.stream["stream"]!!,
                     topic = Example.stream["topic"]!!
                 ),
@@ -143,11 +148,56 @@ class MessageFcmMessageTest : FcmMessageTestBase() {
         )
     }
 
+    @Test
+    fun `dataForOpen works right in happy path`() {
+        expect.that(dataForOpen(Example.stream)).isEqualTo(mapOf(
+            "realm_uri" to Example.stream["realm_uri"]!!,
+            "user_id" to Example.stream["user_id"]!!.toInt(),
+            "recipient_type" to "stream",
+            "stream_id" to Example.stream["stream_id"]!!.toInt(),
+            "stream_name" to Example.stream["stream"]!!,
+            "topic" to Example.stream["topic"]!!,
+        ))
+        expect.that(dataForOpen(Example.groupPm)).isEqualTo(mapOf(
+            "realm_uri" to Example.groupPm["realm_uri"]!!,
+            "user_id" to Example.groupPm["user_id"]!!.toInt(),
+            "recipient_type" to "private",
+            "pm_users" to "123,234,345",
+        ))
+        expect.that(dataForOpen(Example.pm)).isEqualTo(mapOf(
+            "realm_uri" to Example.pm["realm_uri"]!!,
+            "user_id" to Example.pm["user_id"]!!.toInt(),
+            "recipient_type" to "private",
+            "sender_email" to Example.pm["sender_email"]!!,
+        ))
+    }
+
     @Test
     fun `optional fields missing cause no error`() {
+        expect.that(parse(Example.stream.minus("stream_id")).recipient).isEqualTo(Recipient.Stream(
+            streamId = null,
+            streamName = Example.stream["stream"]!!,
+            topic = Example.stream["topic"]!!
+        ))
         expect.that(parse(Example.pm.minus("user_id")).identity.userId).isNull()
     }
 
+    @Test
+    fun `dataForOpen leaves out optional fields missing in input`() {
+        val baseExpected = mapOf(
+            "realm_uri" to Example.stream["realm_uri"]!!,
+            "user_id" to Example.stream["user_id"]!!.toInt(),
+            "recipient_type" to "stream",
+            "stream_id" to Example.stream["stream_id"]!!.toInt(),
+            "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"))
+    }
+
     @Test
     fun `obsolete or novel fields have no effect`() {
         expect.that(parse(Example.pm.plus("user" to "client@example.com")))
@@ -167,6 +217,8 @@ class MessageFcmMessageTest : FcmMessageTestBase() {
         assertParseFails(Example.pm.plus("realm_uri" to "/examplecorp"))
 
         assertParseFails(Example.stream.minus("recipient_type"))
+        assertParseFails(Example.stream.plus("stream_id" to "12,34"))
+        assertParseFails(Example.stream.plus("stream_id" to "abc"))
         assertParseFails(Example.stream.minus("stream"))
         assertParseFails(Example.stream.minus("topic"))
         assertParseFails(Example.groupPm.minus("recipient_type"))

docs/style.md

@@ -654,20 +654,20 @@ import * as api from '../api';
 
 // …
 
-    api.subscriptionAdd(auth, [{ name: stream.name }]);
+    const response = await api.uploadFile(auth, url, name);
 
 ```
 
 rather than like this:
 
 ```js
 // BAD
-import subscriptionAdd from '../api/subscriptions/subscriptionAdd';
+import uploadFile from '../api/uploadFile.js;
 
 // …
 
     // BAD
-    subscriptionAdd(auth, [{ name: stream.name }]);
+    const response = await uploadFile(auth, url, name);
 ```
 
 We do this because a lot of the names in our API bindings are also

src/api/notificationTypes.js

@@ -58,7 +58,7 @@ type StreamData = {|
   +stream: string,
 
   // TODO(server-5.0): Require stream_id (#3918).
-  // +stream_id?: number, // TODO(#3918): Add this, and use it.
+  +stream_id?: number,
 
   +topic: string,
 |};

src/api/subscriptions/subscriptionAdd.js

@@ -5,7 +5,6 @@ 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,
 |};
 

src/diagnostics/StorageScreen.js

@@ -31,7 +31,6 @@ export default function StorageScreen(props: Props): Node {
     <Screen title="Storage" scrollEnabled={false}>
       <FlatList
         data={storageSizes}
-        keyExtractor={item => item.key}
         renderItem={({ item }) => <SizeItem text={item.key} size={item.size} />}
       />
     </Screen>

src/notification/__tests__/notification-test.js

@@ -25,6 +25,22 @@ describe('getNarrowFromNotificationData', () => {
   });
 
   test('recognizes stream notifications and returns topic narrow', () => {
+    const stream = eg.makeStream({ name: 'some stream' });
+    const streamsByName = new Map([[stream.name, stream]]);
+    const notification = {
+      realm_uri,
+      recipient_type: 'stream',
+      stream_id: eg.stream.stream_id,
+      // Name points to some other stream, but the ID prevails.
+      stream_name: 'some stream',
+      topic: 'some topic',
+    };
+    const narrow = getNarrowFromNotificationData(notification, new Map(), streamsByName, ownUserId);
+    expect(narrow).toEqual(topicNarrow(eg.stream.stream_id, 'some topic'));
+  });
+
+  test('recognizes stream notification missing stream_id', () => {
+    // TODO(server-5.0): this test's data will become ill-typed; delete it
     const stream = eg.makeStream({ name: 'some stream' });
     const streamsByName = new Map([[stream.name, stream]]);
     const notification = {
@@ -79,7 +95,20 @@ describe('getNarrowFromNotificationData', () => {
 
 describe('extract iOS notification data', () => {
   const barebones = deepFreeze({
-    stream: { recipient_type: 'stream', stream: 'announce', topic: 'New channel', realm_uri },
+    // TODO(server-5.0): this will become an error case
+    'stream, no ID': {
+      recipient_type: 'stream',
+      stream: 'announce',
+      topic: 'New channel',
+      realm_uri,
+    },
+    stream: {
+      recipient_type: 'stream',
+      stream_id: 234,
+      stream: 'announce',
+      topic: 'New channel',
+      realm_uri,
+    },
     '1:1 PM': { recipient_type: 'private', sender_email: 'nobody@example.com', realm_uri },
     'group PM': { recipient_type: 'private', pm_users: '54,321', realm_uri },
   });
@@ -168,26 +197,12 @@ describe('extract iOS notification data', () => {
     });
 
     test('optional data is typechecked', () => {
-      expect(
-        make({
-          ...barebones.stream,
-          realm_uri: null,
-        }),
-      ).toThrow(/invalid/);
-
-      expect(
-        make({
-          ...barebones['group PM'],
-          realm_uri: ['array', 'of', 'string'],
-        }),
-      ).toThrow(/invalid/);
-
-      expect(
-        make({
-          ...barebones.stream,
-          user_id: 'abc',
-        }),
-      ).toThrow(/invalid/);
+      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(
+        /invalid/,
+      );
+      expect(make({ ...barebones.stream, user_id: 'abc' })).toThrow(/invalid/);
     });
 
     test('hypothetical future: different event types', () => {

src/notification/extract.js

@@ -112,13 +112,17 @@ export const fromAPNsImpl = (data: ?JSONableDict): Notification | void => {
   };
 
   if (recipient_type === 'stream') {
-    const { stream: stream_name, topic } = zulip;
+    const { stream: stream_name, stream_id, topic } = zulip;
     if (typeof stream_name !== 'string' || typeof topic !== 'string') {
       throw err('invalid');
     }
+    if (stream_id !== undefined && typeof stream_id !== 'number') {
+      throw err('invalid');
+    }
     return {
       ...identity,
       recipient_type: 'stream',
+      ...((stream_id !== undefined ? { stream_id } : Object.freeze({})): { stream_id?: number }),
       stream_name,
       topic,
     };