From bf1ad714da3487d6f1a1d6056683e73fc5ea5dea Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@zulipchat.com>
Date: Sat, 14 Jul 2018 10:08:35 +0530
Subject: [PATCH] actions.py: Refactor generate_topic_history_from_db_rows for
 clarity.

This refactors the generate_topic_history_from_db_rows function to not
depend upon the assumption of rows passed as parameter to be sorted in
reverse order of max_message_id field.

Additionally, we add sorting and some tests that verify correct
handling of these cases.
---
 zerver/lib/actions.py        | 27 +++++++++++++--------------
 zerver/tests/test_archive.py | 32 +++++++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py
index 2f17dca6ab..1ecff353c4 100644
--- a/zerver/lib/actions.py
+++ b/zerver/lib/actions.py
@@ -214,26 +214,25 @@ def activity_change_requires_seat_update(user: UserProfile) -> bool:
     return user.realm.has_seat_based_plan and not user.is_bot
 
 def generate_topic_history_from_db_rows(rows: List[Tuple[str, int]]) -> List[Dict[str, Any]]:
-    canonical_topic_names = set()  # type: Set[str]
-    history = []
-    # This algorithm relies on the fact that `rows` is reverse-sorted
-    # by `max_message_id`, so that we will correctly use the highest
-    # max_message_id where multiple topics have the same canonical
-    # name.
-    #
-    # Arguably, we should just reimplment this using a dict rather
-    # than a set to remove that assumption for better readability.
+    canonical_topic_names = {}  # type: Dict[str, Tuple[int, str]]
+
     for (topic_name, max_message_id) in rows:
         canonical_name = topic_name.lower()
-        if canonical_name in canonical_topic_names:
+
+        if canonical_name not in canonical_topic_names:
+            canonical_topic_names[canonical_name] = (max_message_id, topic_name)
             continue
 
-        canonical_topic_names.add(canonical_name)
+        if max_message_id > canonical_topic_names[canonical_name][0]:
+            canonical_topic_names[canonical_name] = (max_message_id, topic_name)
+
+    history = []
+    for canonical_topic, (max_message_id, topic_name) in canonical_topic_names.items():
         history.append(dict(
             name=topic_name,
-            max_id=max_message_id))
-
-    return history
+            max_id=max_message_id)
+        )
+    return sorted(history, key=lambda x: -x['max_id'])
 
 def get_topic_history_for_stream(user_profile: UserProfile,
                                  recipient: Recipient,
diff --git a/zerver/tests/test_archive.py b/zerver/tests/test_archive.py
index b5b4ba2e8d..734c952d31 100644
--- a/zerver/tests/test_archive.py
+++ b/zerver/tests/test_archive.py
@@ -109,11 +109,35 @@ class WebPublicTopicHistoryTest(ZulipTestCase):
         test_stream = self.make_stream('Test Public Archives')
         do_change_stream_web_public(test_stream, True)
 
+        self.send_stream_message(
+            self.example_email("iago"),
+            "Test Public Archives",
+            'Test Message 3',
+            topic_name='first_topic'
+        )
         self.send_stream_message(
             self.example_email("iago"),
             "Test Public Archives",
             'Test Message',
-            'TopicGlobal'
+            topic_name='TopicGlobal'
+        )
+        self.send_stream_message(
+            self.example_email("iago"),
+            "Test Public Archives",
+            'Test Message 2',
+            topic_name='topicglobal'
+        )
+        self.send_stream_message(
+            self.example_email("iago"),
+            "Test Public Archives",
+            'Test Message 3',
+            topic_name='second_topic'
+        )
+        self.send_stream_message(
+            self.example_email("iago"),
+            "Test Public Archives",
+            'Test Message 4',
+            topic_name='TopicGlobal'
         )
 
         result = self.client_get(
@@ -121,6 +145,8 @@ class WebPublicTopicHistoryTest(ZulipTestCase):
         )
         self.assert_json_success(result)
         history = result.json()['topics']
-        self.assert_length(history, 1)
-        self.assert_length(history[0], 2)
+        self.assert_length(history, 3)
+        # Should be sorted with latest topic first
         self.assertEqual(history[0]['name'], 'TopicGlobal')
+        self.assertEqual(history[1]['name'], 'second_topic')
+        self.assertEqual(history[2]['name'], 'first_topic')