From 7714f449787fbdbf5ab3efb8ebeeb30b9ae9a74b Mon Sep 17 00:00:00 2001 From: Zeeshan Date: Fri, 28 May 2021 15:23:50 +0530 Subject: [PATCH 1/2] bugfix: api_types/model: Update mentioned narrow on message edit event. This commit updates the index and unread_count if the message was indexed when an update message event with change in mention flag(s) is detected. If the message was not in index, we just update the unread_count. In this case, the count is updated by comparing with initial_data's "unread_msgs" field which does not stay synced with the server, hence the logic is buggy. Changes in mentioned narrow after this event are not rendered at the same time but it updates the model so that a return to the mentioned messages narrow will do so. 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 amended. --- tests/model/test_model.py | 140 ++++++++++++++++++++++++++++++++++--- zulipterminal/api_types.py | 1 + zulipterminal/model.py | 49 ++++++++++++- 3 files changed, 179 insertions(+), 11 deletions(-) diff --git a/tests/model/test_model.py b/tests/model/test_model.py index 02be6ce0ad..45372ba07b 100644 --- a/tests/model/test_model.py +++ b/tests/model/test_model.py @@ -1311,7 +1311,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," + "initial_flags, mentioned_msg_ids", [ case( { # Only subject of 1 message is updated. @@ -1328,18 +1329,23 @@ 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, + ["read"], + set(), id="Only subject of 1 message is updated", ), case( @@ -1357,27 +1363,33 @@ 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, + ["read"], + 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": "

new content

", + "flags": ["read", "wildcard_mentioned"], }, - 1, + 2, { "messages": { 1: { @@ -1385,29 +1397,35 @@ def test_notify_users_enabled( "stream_id": 10, "content": "

new content

", "subject": "old subject", + "flags": ["read", "wildcard_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", + ["read"], + 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": "

new content

", "subject": "new subject", "stream_id": 10, "message_ids": [1], + "flags": ["read", "mentioned"], }, - 2, + 3, { # 2=update of subject & content "messages": { 1: { @@ -1415,19 +1433,24 @@ def test_notify_users_enabled( "stream_id": 10, "content": "

new content

", "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", + ["read"], + set(), + id="Both message content and subject is updated, mention added", ), case( { # Some new type of update which we don't handle yet. @@ -1442,18 +1465,23 @@ 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, + ["read"], + set(), id="Some new type of update which we don't handle yet", ), case( @@ -1472,18 +1500,23 @@ 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, + ["read"], + set(), id="message_id not present in index, topic view closed", ), case( @@ -1502,29 +1535,35 @@ 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, + ["read"], + 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": "

new content

", "subject": "new subject", "stream_id": 10, "message_ids": [1], + "flags": ["read"], }, - 2, + 3, { "messages": { 1: { @@ -1532,19 +1571,96 @@ def test_notify_users_enabled( "stream_id": 10, "content": "

new 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: ["new subject", "old subject"]}, + "mentioned_msg_ids": set(), }, True, - id="Message content is updated and topic view is enabled", + ["read", "mentioned"], + {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": "

new content @**stream**

", + "subject": "old subject", + "stream_id": 10, + "message_ids": [1], + "flags": ["read", "wildcard_mentioned"], + }, + 3, + { + "messages": { + 1: { + "id": 1, + "stream_id": 10, + "content": "

new content @**stream**

", + "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, + ["read"], + set(), + id="wildcard_mentioned flag added with no subject change", + ), + case( + { # wildcard_mentioned flag removed with no subject change. + "message_id": 1, + "rendered_content": "

new content

", + "subject": "old subject", + "stream_id": 10, + "message_ids": [1], + "flags": ["read"], + }, + 3, + { + "messages": { + 1: { + "id": 1, + "stream_id": 10, + "content": "

new content

", + "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, + ["read", "wildcard_mentioned"], + {1}, + id="wildcard_mentioned flag removed with no subject change", ), ], ) @@ -1556,6 +1672,8 @@ def test__handle_update_message_event( expected_index, expected_times_messages_rerendered, topic_view_enabled, + initial_flags, + mentioned_msg_ids, ): event["type"] = "update_message" @@ -1566,9 +1684,11 @@ def test__handle_update_message_event( "stream_id": 10, "content": "old content", "subject": "old subject", + "flags": initial_flags if message_id == 1 else ["read"], } for message_id in [1, 2] }, + "mentioned_msg_ids": mentioned_msg_ids, "edited_messages": set(), "topics": {10: ["old subject"]}, } diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py index 2ff75fa72f..3da648369a 100644 --- a/zulipterminal/api_types.py +++ b/zulipterminal/api_types.py @@ -114,6 +114,7 @@ class UpdateMessageEvent(TypedDict): message_ids: List[int] subject: str stream_id: int + flags: List[MessageFlag] class ReactionEvent(TypedDict): diff --git a/zulipterminal/model.py b/zulipterminal/model.py index 03b6835286..1848002f2d 100644 --- a/zulipterminal/model.py +++ b/zulipterminal/model.py @@ -1164,7 +1164,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 @@ -1182,6 +1182,53 @@ def _handle_update_message_event(self, event: Event) -> None: self.index["messages"][message_id] = indexed_message self._update_rendered_view(message_id) + # Update the index and unread_count if flag changed and message is indexed + mention_flags_set = {"mentioned", "wildcard_mentioned"} + if ( + indexed_message + and "flags" in event + and set(event["flags"]) != set(indexed_message["flags"]) + ): + mentioned_before = bool(set(indexed_message["flags"]) & mention_flags_set) + mentioned_after = bool(set(event["flags"]) & mention_flags_set) + self.index["messages"][message_id]["flags"] = event["flags"] + + if not mentioned_before and mentioned_after: + self.index["mentioned_msg_ids"].add(message_id) + if "read" not in indexed_message["flags"]: + self.controller.view.mentioned_button.update_count( + self.controller.view.mentioned_button.count + 1 + ) + elif mentioned_before and not mentioned_after: + self.index["mentioned_msg_ids"].discard(message_id) + if "read" not in indexed_message["flags"]: + self.controller.view.mentioned_button.update_count( + self.controller.view.mentioned_button.count - 1 + ) + + self._update_rendered_view(message_id) + + # Update the unread_count if message is not indexed. We can't index the message + # since we don't get the full message structure from server for this event. + elif not indexed_message and "flags" in event: + unread_mentions = self.initial_data["unread_msgs"]["mentions"] + unread_mentioned_before = message_id in unread_mentions + mentioned_after = bool(set(event["flags"]) & mention_flags_set) + + if not unread_mentioned_before and mentioned_after: + self.controller.view.mentioned_button.update_count( + self.controller.view.mentioned_button.count + 1 + ) + if message_id not in unread_mentions: + unread_mentions.append(message_id) + elif unread_mentioned_before and not mentioned_after: + self.controller.view.mentioned_button.update_count( + self.controller.view.mentioned_button.count - 1 + ) + unread_mentions.remove(message_id) + + self.controller.update_screen() + # NOTE: This is independent of messages being indexed # Previous assertion: # * 'subject' is not present in update event if From f75075afa712545514db991b9b0591d09ed07845 Mon Sep 17 00:00:00 2001 From: Zeeshan Date: Thu, 27 May 2021 01:57:12 +0530 Subject: [PATCH 2/2] tests: model: Extend other tests to incorporate both mentioned flags. This commit expands some of the fixtures of present tests with the mentioned/wildcard_mentioned flags to make the tests more robust and generalized. --- tests/model/test_model.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/model/test_model.py b/tests/model/test_model.py index 45372ba07b..be11432426 100644 --- a/tests/model/test_model.py +++ b/tests/model/test_model.py @@ -888,6 +888,8 @@ def test_toggle_stream_muted_status( (["read"], "add"), (["read", "starred"], "remove"), (["starred", "read"], "remove"), + (["read", "mentioned", "wildcard_mentioned"], "add"), + (["mentioned", "wildcard_mentioned", "starred"], "remove"), ], ) def test_toggle_message_star_status( @@ -1990,11 +1992,18 @@ def test_update_star_status_invalid_operation( ("add", 1, ["read"], ["read", "starred"]), ("add", 1, ["starred"], ["starred"]), ("add", 1, ["read", "starred"], ["read", "starred"]), + ("add", 1, ["mentioned"], ["mentioned", "starred"]), + ("add", 1, ["mentioned", "starred"], ["mentioned", "starred"]), + ("add", 1, ["wildcard_mentioned"], ["wildcard_mentioned", "starred"]), ("remove", -1, [], []), ("remove", -1, ["read"], ["read"]), ("remove", -1, ["starred"], []), ("remove", -1, ["read", "starred"], ["read"]), ("remove", -1, ["starred", "read"], ["read"]), + ("remove", -1, ["mentioned"], ["mentioned"]), + ("remove", -1, ["wildcard_mentioned"], ["wildcard_mentioned"]), + ("remove", -1, ["mentioned", "starred"], ["mentioned"]), + ("remove", -1, ["wildcard_mentioned", "starred"], ["wildcard_mentioned"]), ], ) def test_update_star_status( @@ -2073,11 +2082,19 @@ def test_update_star_status( ("add", ["read"], ["read"]), ("add", ["starred"], ["starred", "read"]), ("add", ["read", "starred"], ["read", "starred"]), + ("add", ["mentioned"], ["mentioned", "read"]), + ("add", ["read", "mentioned"], ["read", "mentioned"]), + ("add", ["mentioned", "starred"], ["mentioned", "starred", "read"]), + ("add", ["wildcard_mentioned"], ["wildcard_mentioned", "read"]), ("remove", [], []), ("remove", ["read"], ["read"]), # msg cannot be marked 'unread' ("remove", ["starred"], ["starred"]), + ("remove", ["mentioned"], ["mentioned"]), ("remove", ["starred", "read"], ["starred", "read"]), ("remove", ["read", "starred"], ["read", "starred"]), + ("remove", ["read", "mentioned"], ["read", "mentioned"]), + ("remove", ["wildcard_mentioned"], ["wildcard_mentioned"]), + ("remove", ["read", "wildcard_mentioned"], ["read", "wildcard_mentioned"]), ], ) def test_update_read_status(