From da596ef269040b5ed5a23f8dbeae643c9a6a9503 Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@zulip.com>
Date: Tue, 29 Jun 2021 09:43:52 -0700
Subject: [PATCH] message_edit: Fix live update bug in left sidebar.

We've had for years a subtle bug, where after editing a topic in the
left sidebar that had previously had unread messages (but doesn't
anymore), the old topic might still appear in the sidebar.

The bug was hard to notice except for new organizations or in the
development environment, because the pre-edit topic appeared with a
sort key of -Infinity (that being the max ID in an empty list of
message IDs). But this is an important onboarding bug in reducing
faith in Zulip's topic editing just working, so I'm glad to have it
fixed.

Fixes #11901.
---
 .../node_tests/stream_topic_history.js           |  7 ++++++-
 static/js/stream_topic_history.js                |  2 ++
 static/js/unread.js                              | 16 ++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/frontend_tests/node_tests/stream_topic_history.js b/frontend_tests/node_tests/stream_topic_history.js
index 4e568b554c..1a190cb518 100644
--- a/frontend_tests/node_tests/stream_topic_history.js
+++ b/frontend_tests/node_tests/stream_topic_history.js
@@ -256,10 +256,14 @@ test("test_unread_logic", () => {
     assert.deepEqual(history, ["toPic1", "topic2"]);
 
     const msgs = [
-        {id: 150, topic: "TOPIC2"}, // will be ignored
+        // This will be ignored as a case variant of `topic2` above.
+        {id: 150, topic: "TOPIC2"},
         {id: 61, topic: "unread1"},
         {id: 60, topic: "unread1"},
         {id: 20, topic: "UNREAD2"},
+        // We're going to mark this as read; this will verify the logic
+        // in unreads.js for only including topics with nonzero unreads.
+        {id: 79, topic: "to_mark_as_read"},
     ];
 
     for (const msg of msgs) {
@@ -269,6 +273,7 @@ test("test_unread_logic", () => {
     }
 
     unread.process_loaded_messages(msgs);
+    unread.mark_as_read(79);
 
     history = stream_topic_history.get_recent_topic_names(stream_id);
     assert.deepEqual(history, ["toPic1", "unread1", "topic2", "UNREAD2"]);
diff --git a/static/js/stream_topic_history.js b/static/js/stream_topic_history.js
index 715c50160c..03a4987310 100644
--- a/static/js/stream_topic_history.js
+++ b/static/js/stream_topic_history.js
@@ -201,6 +201,8 @@ export class PerStreamHistory {
     get_recent_topic_names() {
         const my_recents = Array.from(this.topics.values());
 
+        /* Add any older topics with unreads that may not be present
+         * in our local cache. */
         const missing_topics = unread.get_missing_topics({
             stream_id: this.stream_id,
             topic_dict: this.topics,
diff --git a/static/js/unread.js b/static/js/unread.js
index c218641ec3..08a98f5ba5 100644
--- a/static/js/unread.js
+++ b/static/js/unread.js
@@ -265,6 +265,15 @@ class UnreadTopicCounter {
     }
 
     get_missing_topics(opts) {
+        /* Clients have essentially complete unread data, but
+         * stream_topic_history.is_complete_for_stream_id() can be
+         * false. In that situation, this function helps ensure that
+         * we include all topics with unread messages in data that.
+         *
+         * It will return all topics in the provided stream with a
+         * nonzero unread count that are not already present in the
+         * topic_dict parameter.
+         */
         const stream_id = opts.stream_id;
         const topic_dict = opts.topic_dict;
 
@@ -275,6 +284,13 @@ class UnreadTopicCounter {
 
         let topic_names = Array.from(per_stream_bucketer.keys());
 
+        /* Include topics that have at least one unread. It would likely
+         * be better design for buckets to be deleted when emptied. */
+        topic_names = topic_names.filter((topic_name) => {
+            const messages = Array.from(per_stream_bucketer.get_bucket(topic_name));
+            return messages.length > 0;
+        });
+        /* And aren't already present in topic_dict. */
         topic_names = topic_names.filter((topic_name) => !topic_dict.has(topic_name));
 
         const result = topic_names.map((topic_name) => {