Repository: zulip/zulip-terminal
Pull request #1130 (2021-08-23 13:07:21)

Update index's `topic_msg_ids` accurately with topic updates.
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->
Prior to this PR, if a message's topic was changed, the
corresponding `message_id` was not removed from the old topic's set
and added to the new one under the "topic_msg_ids" key of the model's
index. This PR updates the relevant sets to ensure accuracy.

<!-- Add a link to a discussion on chat.zulip.org, if relevant -->

**Tested?** <!-- Fine to leave some of these unchecked if this is a draft/work-in-progress -->
- [x] Manually
- [x] Existing tests (adapted, if necessary)
- [x] Passed linting & tests (each commit)
<!-- Code must pass CI (GitHub Actions) before merging - look for the green tick! -->

<!-- See https://github.com/zulip/zulip-terminal#commit-style -->
**Commit flow** <!-- if more than one commit; add/delete/fill-in as appropriate -->
<!-- For example:
- first commit doing some thing
- maybe multiple commits doing similar things
-->
- api_types: Include the orig_subject field in UpdateMessageEvent. 
This commit introduces another field to the UpdateMessageEvent class
in `api_types.py` to accommodate this field in the event handler
method for update message events.

- bugfix: model: Remove updated message_ids from their old topic's set.
Prior to this commit, if a message's topic was changed, the
corresponding `message_id` was not removed from the old topic's set
under the "topic_msg_ids" key of the model's index. This commit removes
the updated `message_id` from that set, ensuring the accuracy of the
set.
Tests updated.

neiljp: @prah23 This change looks reasonable, though I'll read the tests again before merging, but as per `#zulip-terminal>Topic move bug? #T1130` does this fix a particular situation that can also be manually detected/tested?

Patch:
diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py
index a47d5ed..55237c4 100644
--- a/zulipterminal/api_types.py
+++ b/zulipterminal/api_types.py
@@ -151,7 +151,9 @@ class UpdateMessageEvent(TypedDict):
     rendered_content: str
     # B: Subject of these message ids needs updating?
     message_ids: List[int]
+    orig_subject: str
     subject: str
+    propagate_mode: EditPropagateMode
     stream_id: int
 
 
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 142a41a..a53d2a6 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -1384,9 +1384,26 @@ class Model:
         if "subject" in event:
             new_subject = event["subject"]
             stream_id = event["stream_id"]
+            old_subject = event["orig_subject"]
+            msg_ids_by_topic = self.index["topic_msg_ids"][stream_id]
 
-            # Update any indexed messages & re-render them
+            # Remove each message_id from the old topic's `topic_msg_ids` set
+            # if it exists, and update & re-render the message if it is indexed.
             for msg_id in event["message_ids"]:
+                # Ensure that the new topic is not the same as the old one
+                # (no-op topic edit).
+                if new_subject != old_subject:
+                    # Remove the msg_id from the relevant `topic_msg_ids` set,
+                    # if that topic's set has already been initiated.
+                    if old_subject in msg_ids_by_topic:
+                        msg_ids_by_topic[old_subject].discard(msg_id)
+
+                    # Add the msg_id to the new topic's set, if the set has
+                    # already been initiated.
+                    if new_subject in msg_ids_by_topic:
+                        msg_ids_by_topic[new_subject].add(msg_id)
+
+                # Update and re-render indexed messages.
                 indexed_msg = self.index["messages"].get(msg_id)
                 if indexed_msg:
                     indexed_msg["subject"] = new_subject


Test patch:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index bf0f887..7aab54f 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -1624,6 +1624,7 @@ class TestModel:
             case(
                 {  # Only subject of 1 message is updated.
                     "message_id": 1,
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
@@ -1644,6 +1645,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1}, "old subject": {2}},
+                    },
                     "edited_messages": {1},
                     "topics": {10: []},
                 },
@@ -1653,6 +1657,7 @@ class TestModel:
             case(
                 {  # Subject of 2 messages is updated
                     "message_id": 1,
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1, 2],
@@ -1673,6 +1678,9 @@ class TestModel:
                             "subject": "new subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1, 2}, "old subject": set()},
+                    },
                     "edited_messages": {1},
                     "topics": {10: []},
                 },
@@ -1701,8 +1709,11 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": set(), "old subject": {1, 2}},
+                    },
                     "edited_messages": {1},
-                    "topics": {10: ["old subject"]},
+                    "topics": {10: ["new subject", "old subject"]},
                 },
                 False,
                 id="Message content is updated",
@@ -1711,6 +1722,7 @@ class TestModel:
                 {  # Both message content and subject is updated.
                     "message_id": 1,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
@@ -1731,6 +1743,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1}, "old subject": {2}},
+                    },
                     "edited_messages": {1},
                     "topics": {10: []},
                 },
@@ -1758,8 +1773,11 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": set(), "old subject": {1, 2}},
+                    },
                     "edited_messages": {1},
-                    "topics": {10: ["old subject"]},
+                    "topics": {10: ["new subject", "old subject"]},
                 },
                 False,
                 id="Some new type of update which we don't handle yet",
@@ -1768,6 +1786,7 @@ class TestModel:
                 {  # message_id not present in index, topic view closed.
                     "message_id": 3,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [3],
@@ -1788,6 +1807,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {3}, "old subject": {1, 2}},
+                    },
                     "edited_messages": set(),
                     "topics": {10: []},  # This resets the cache
                 },
@@ -1798,6 +1820,7 @@ class TestModel:
                 {  # message_id not present in index, topic view is enabled.
                     "message_id": 3,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [3],
@@ -1818,6 +1841,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {3}, "old subject": {1, 2}},
+                    },
                     "edited_messages": set(),
                     "topics": {10: ["new subject", "old subject"]},
                 },
@@ -1828,6 +1854,7 @@ class TestModel:
                 {  # Message content is updated and topic view is enabled.
                     "message_id": 1,
                     "rendered_content": "<p>new content</p>",
+                    "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
@@ -1848,6 +1875,9 @@ class TestModel:
                             "subject": "old subject",
                         },
                     },
+                    "topic_msg_ids": {
+                        10: {"new subject": {1}, "old subject": {2}},
+                    },
                     "edited_messages": {1},
                     "topics": {10: ["new subject", "old subject"]},
                 },
@@ -1877,8 +1907,11 @@ class TestModel:
                 }
                 for message_id in [1, 2]
             },
+            "topic_msg_ids": {  # FIXME? consider test for eg. absence of empty set
+                10: {"new subject": set(), "old subject": {1, 2}},
+            },
             "edited_messages": set(),
-            "topics": {10: ["old subject"]},
+            "topics": {10: ["new subject", "old subject"]},
         }
         mocker.patch(MODEL + "._update_rendered_view")