Skip to content

Update index's topic_msg_ids accurately with topic updates. #1130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Update index's topic_msg_ids accurately with topic updates. #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c91f80f
api_types: Extend UpdateMessageEvent to accomodate topic updates.
prah23 Jun 14, 2021
a52236b
bugfix: model: Remove updated `message_id`s from their old topic's set.
prah23 Jul 3, 2021
3d07db2
bugfix: model: Add updated `message_id`s to their new topic's set.
prah23 Feb 22, 2022
6749809
refactor: model: Extract variable in _handle_update_message_event.
neiljp Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1624,6 +1624,7 @@ def test_notify_users_enabled(
case(
{ # Only subject of 1 message is updated.
"message_id": 1,
"orig_subject": "old subject",
"subject": "new subject",
"stream_id": 10,
"message_ids": [1],
Expand All @@ -1644,6 +1645,9 @@ def test_notify_users_enabled(
"subject": "old subject",
},
},
"topic_msg_ids": {
10: {"new subject": {1}, "old subject": {2}},
},
"edited_messages": {1},
"topics": {10: []},
},
Expand All @@ -1653,6 +1657,7 @@ def test_notify_users_enabled(
case(
{ # Subject of 2 messages is updated
"message_id": 1,
"orig_subject": "old subject",
"subject": "new subject",
"stream_id": 10,
"message_ids": [1, 2],
Expand All @@ -1673,6 +1678,9 @@ def test_notify_users_enabled(
"subject": "new subject",
},
},
"topic_msg_ids": {
10: {"new subject": {1, 2}, "old subject": set()},
},
"edited_messages": {1},
"topics": {10: []},
},
Expand Down Expand Up @@ -1701,8 +1709,11 @@ def test_notify_users_enabled(
"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",
Expand All @@ -1711,6 +1722,7 @@ def test_notify_users_enabled(
{ # 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],
Expand All @@ -1731,6 +1743,9 @@ def test_notify_users_enabled(
"subject": "old subject",
},
},
"topic_msg_ids": {
10: {"new subject": {1}, "old subject": {2}},
},
"edited_messages": {1},
"topics": {10: []},
},
Expand Down Expand Up @@ -1758,8 +1773,11 @@ def test_notify_users_enabled(
"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",
Expand All @@ -1768,6 +1786,7 @@ def test_notify_users_enabled(
{ # 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],
Expand All @@ -1788,6 +1807,9 @@ def test_notify_users_enabled(
"subject": "old subject",
},
},
"topic_msg_ids": {
10: {"new subject": {3}, "old subject": {1, 2}},
},
"edited_messages": set(),
"topics": {10: []}, # This resets the cache
},
Expand All @@ -1798,6 +1820,7 @@ def test_notify_users_enabled(
{ # 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],
Expand All @@ -1818,6 +1841,9 @@ def test_notify_users_enabled(
"subject": "old subject",
},
},
"topic_msg_ids": {
10: {"new subject": {3}, "old subject": {1, 2}},
},
"edited_messages": set(),
"topics": {10: ["new subject", "old subject"]},
},
Expand All @@ -1828,6 +1854,7 @@ def test_notify_users_enabled(
{ # 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],
Expand All @@ -1848,6 +1875,9 @@ def test_notify_users_enabled(
"subject": "old subject",
},
},
"topic_msg_ids": {
10: {"new subject": {1}, "old subject": {2}},
},
"edited_messages": {1},
"topics": {10: ["new subject", "old subject"]},
},
Expand Down Expand Up @@ -1877,8 +1907,11 @@ def test__handle_update_message_event(
}
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")

Expand Down
2 changes: 2 additions & 0 deletions zulipterminal/api_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
19 changes: 18 additions & 1 deletion zulipterminal/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1384,9 +1384,26 @@ def _handle_update_message_event(self, event: Event) -> None:
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
Expand Down