Skip to content

Commit

Permalink
api_types/model: Update the mentioned narrow with message edit events.
Browse files Browse the repository at this point in the history 
This commit fixes a bug, allowing the mentioned narrow and the index
to be updated as soon as a message edit event with change in flags is
detected. Although, the changes are not rendered at the same time but
this updates the model so that a return to the mentioned messages
narrow will do so.

The behaviour is very similar to the starred message narrow and the
view is rendered only if a change in mentioned flag is detected from
the events.

The tests are also tried to be modified as less as possible, and the
present fixtures were mostly used to test for this new condition,
thus preventing the use of too many fixtures.

Tests added and amended.
  • Loading branch information
@zee-bit
zee-bit committed Jun 10, 2021
1 parent c1d019a commit 904c272
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 11 deletions.
133 changes: 123 additions & 10 deletions tests/model/test_model.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1313,7 +1313,8 @@ def test_notify_users_enabled(
assert notify.called == is_notify_called

@pytest.mark.parametrize(
"event, expected_times_messages_rerendered, expected_index, topic_view_enabled",
"event, expected_times_messages_rerendered, expected_index, topic_view_enabled,"
"mentioned_msg_ids",
[
case(
{ # Only subject of 1 message is updated.
Expand All @@ -1330,18 +1331,22 @@ def test_notify_users_enabled(
"stream_id": 10,
"content": "old content",
"subject": "new subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": {1},
"topics": {10: []},
"mentioned_msg_ids": set(),
},
False,
set(),
id="Only subject of 1 message is updated",
),
case(
Expand All @@ -1359,77 +1364,91 @@ def test_notify_users_enabled(
"stream_id": 10,
"content": "old content",
"subject": "new subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "new subject",
"flags": ["read"],
},
},
"edited_messages": {1},
"topics": {10: []},
"mentioned_msg_ids": set(),
},
False,
set(),
id="Subject of 2 messages is updated",
),
case(
{ # Message content is updated
{ # Message content is updated, wildcard-mention added
"message_id": 1,
"stream_id": 10,
"rendered_content": "<p>new content</p>",
"flags": ["read", "mentioned"],
},
1,
2,
{
"messages": {
1: {
"id": 1,
"stream_id": 10,
"content": "<p>new content</p>",
"subject": "old subject",
"flags": ["read", "mentioned"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": {1},
"topics": {10: ["old subject"]},
"mentioned_msg_ids": {1},
},
False,
id="Message content is updated",
set(),
id="Message content is updated, wildcard-mention added",
),
case(
{ # Both message content and subject is updated.
{ # Both message content and subject is updated, mention added.
"message_id": 1,
"rendered_content": "<p>new content</p>",
"subject": "new subject",
"stream_id": 10,
"message_ids": [1],
"flags": ["read", "mentioned"],
},
2,
3,
{ # 2=update of subject & content
"messages": {
1: {
"id": 1,
"stream_id": 10,
"content": "<p>new content</p>",
"subject": "new subject",
"flags": ["read", "mentioned"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": {1},
"topics": {10: []},
"mentioned_msg_ids": {1},
},
False,
id="Both message content and subject is updated",
set(),
id="Both message content and subject is updated, mention added",
),
case(
{ # Some new type of update which we don't handle yet.
Expand All @@ -1444,18 +1463,22 @@ def test_notify_users_enabled(
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": {1},
"topics": {10: ["old subject"]},
"mentioned_msg_ids": set(),
},
False,
set(),
id="Some new type of update which we don't handle yet",
),
case(
Expand All @@ -1474,18 +1497,22 @@ def test_notify_users_enabled(
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": set(),
"topics": {10: []}, # This resets the cache
"mentioned_msg_ids": set(),
},
False,
set(),
id="message_id not present in index, topic view closed",
),
case(
Expand All @@ -1504,49 +1531,128 @@ def test_notify_users_enabled(
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": set(),
"topics": {10: ["new subject", "old subject"]},
"mentioned_msg_ids": set(),
},
True,
set(),
id="message_id not present in index, topic view is enabled",
),
case(
{ # Message content is updated and topic view is enabled.
{ # Message content updated and topic view is enabled, mention removed.
"message_id": 1,
"rendered_content": "<p>new content</p>",
"subject": "new subject",
"stream_id": 10,
"message_ids": [1],
"flags": ["read"],
},
2,
3,
{
"messages": {
1: {
"id": 1,
"stream_id": 10,
"content": "<p>new content</p>",
"subject": "new subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"edited_messages": {1},
"topics": {10: ["new subject", "old subject"]},
"mentioned_msg_ids": set(),
},
True,
id="Message content is updated and topic view is enabled",
{1},
id="Message content updated and topic view is enabled, mention removed",
),
case(
{ # wildcard_mentioned flag added with no subject change
"message_id": 1,
"rendered_content": "<p>new content @**stream**</p>",
"subject": "old subject",
"stream_id": 10,
"message_ids": [1],
"flags": ["read", "wildcard_mentioned"],
},
3,
{
"messages": {
1: {
"id": 1,
"stream_id": 10,
"content": "<p>new content @**stream**</p>",
"subject": "old subject",
"flags": ["read", "wildcard_mentioned"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"mentioned_msg_ids": {1},
"edited_messages": {1},
"topics": {10: []},
},
False,
set(),
id="wildcard_mentioned flag added with no subject change",
),
case(
{ # mentioned flag removed with no subject change.
"message_id": 1,
"rendered_content": "<p>new content</p>",
"subject": "old subject",
"stream_id": 10,
"message_ids": [1],
"flags": ["read"],
},
3,
{
"messages": {
1: {
"id": 1,
"stream_id": 10,
"content": "<p>new content</p>",
"subject": "old subject",
"flags": ["read"],
},
2: {
"id": 2,
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": ["read"],
},
},
"mentioned_msg_ids": set(),
"edited_messages": {1},
"topics": {10: []},
},
False,
{1},
id="mentioned flag removed with no subject change",
),
],
)
Expand All @@ -1558,6 +1664,7 @@ def test__handle_update_message_event(
expected_index,
expected_times_messages_rerendered,
topic_view_enabled,
mentioned_msg_ids,
):
event["type"] = "update_message"

Expand All @@ -1568,9 +1675,15 @@ def test__handle_update_message_event(
"stream_id": 10,
"content": "old content",
"subject": "old subject",
"flags": (
["read"]
if message_id not in mentioned_msg_ids
else ["read", "mentioned"]
),
}
for message_id in [1, 2]
},
"mentioned_msg_ids": mentioned_msg_ids,
"edited_messages": set(),
"topics": {10: ["old subject"]},
}
Expand Down
1 change: 1 addition & 0 deletions zulipterminal/api_types.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class UpdateMessageEvent(TypedDict):
message_ids: List[int]
subject: str
stream_id: int
flags: List[MessageFlag]


class ReactionEvent(TypedDict):
Expand Down
19 changes: 18 additions & 1 deletion zulipterminal/model.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,7 @@ def _update_topic_index(self, stream_id: int, topic_name: str) -> None:

def _handle_update_message_event(self, event: Event) -> None:
"""
Handle updated (edited) messages (changed content/subject)
Handle updated (edited) messages (changed content/subject/flags)
"""
assert event["type"] == "update_message"
# Update edited message status from single message id
Expand All @@ -1183,6 +1183,23 @@ def _handle_update_message_event(self, event: Event) -> None:
self.index["messages"][message_id] = indexed_message
self._update_rendered_view(message_id)

# Update the flags if changed and message is indexed
mention_flag_set = {"mentioned", "wildcard_mentioned"}
if (
indexed_message
and "flags" in event
and set(event["flags"]) != set(indexed_message["flags"])
):
indexed_message["flags"] = event["flags"]
self.index["messages"][message_id] = indexed_message

if set(event["flags"]) & mention_flag_set:
self.index["mentioned_msg_ids"].add(message_id)
elif message_id in self.index["mentioned_msg_ids"]:
self.index["mentioned_msg_ids"].remove(message_id)

self._update_rendered_view(message_id)

# NOTE: This is independent of messages being indexed
# Previous assertion:
# * 'subject' is not present in update event if
Expand Down

0 comments on commit 904c272

Please sign in to comment.