Repository: zulip/zulip-terminal
Pull request #1403 (2023-05-22 06:28:21)

bugfix: Correctly rerender when /me status changes.
<!-- See README for documentation, or ask in #zulip-terminal if unclear -->
### What does this PR do, and why?

This bug was identified via the additional flags added in comments in #1401.

Previously the is_me_message flag in messages were not updated locally, if the content was updated to be recognized as such by the server. Such a flag is provided directly in update_message events (is_me_message) as in message objects fetched from the server, but was not synchronized until now.

This had no effect on /me messages which were edited to lose that status, since the message rendering code did not find the matching /me message content and so updated the rendering to show as a regular message.

However, for messages which were initially not /me messages, the /me rendering path could never be entered if they were subsequently edited to become /me messages, since the property of the message was not updated to allow this.

Tests updated and new test case added for switching to /me message.

### Outstanding aspect(s)    <!-- DELETE SECTION IF EMPTY -->
<!-- In what ways is this not fully implemented/functioning? Compared to a discussion/issue? -->
<!-- Do you not understand something? Are you unsure about a certain approach? Want feedback? -->
- [x] Add test for currently working case of /me message -> not /me message

### External discussion & connections
<!-- [x] all that apply, specifying topic and adding numbers after # for issues/PRs -->
- [ ] Discussed in **#zulip-terminal** in `topic`
- [ ] Fully fixes #
- [ ] Partially fixes issue #
- [ ] Builds upon previous unmerged work in PR #
- [x] Is a follow-up to work in PR #1401 
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #

### How did you test this?
<!-- [x] all that apply -->
- [x] Manually - Behavioral changes
- [x] Manually - Visual changes
- [x] Adapting existing automated tests
- [x] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (*only a refactor of tested code*)

### Self-review checklist for each commit
- [x] It is a [minimal coherent idea](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development)
- [x] It has a commit summary following the [documented style](https://github.com/zulip/zulip-terminal#structuring-commits---speeding-up-reviews-merging--development) (title & body)
- [x] It has a commit summary describing the  motivation and reasoning for the change
- [x] It individually passes linting and tests
- [x] It contains test additions for any new behavior
- [ ] It flows clearly from a previous branch commit, and/or prepares for the next commit


Patch:
diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py
index bd3fae7..e9d75e2 100644
--- a/zulipterminal/api_types.py
+++ b/zulipterminal/api_types.py
@@ -335,7 +335,7 @@ class UpdateMessageContentEvent(BaseUpdateMessageEvent):
     # orig_content: str
     # content: str
 
-    # is_me_message: bool
+    is_me_message: bool
 
 
 class UpdateMessagesLocationEvent(BaseUpdateMessageEvent):
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 48ddf96..906660d 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -1604,6 +1604,7 @@ class Model:
         if "rendered_content" in event and indexed_message:
             content_event = cast(UpdateMessageContentEvent, event)
             indexed_message["content"] = content_event["rendered_content"]
+            indexed_message["is_me_message"] = content_event["is_me_message"]
             self.index["messages"][message_id] = indexed_message
             self._update_rendered_view(message_id)
 


Test patch:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index 2319069..f7bfdc6 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -2081,7 +2081,8 @@ class TestModel:
         )
 
     @pytest.mark.parametrize(
-        "event, expected_times_messages_rerendered, expected_index, topic_view_enabled",
+        "event, initially_me_message,"
+        "expected_times_messages_rerendered, expected_index, topic_view_enabled",
         [
             case(
                 {  # Only subject of 1 message is updated.
@@ -2091,6 +2092,7 @@ class TestModel:
                     "stream_id": 10,
                     "message_ids": [1],
                 },
+                False,
                 1,
                 {
                     "messages": {
@@ -2099,12 +2101,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "new subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2124,6 +2128,7 @@ class TestModel:
                     "stream_id": 10,
                     "message_ids": [1, 2],
                 },
+                False,
                 2,
                 {
                     "messages": {
@@ -2132,12 +2137,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "new subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "new subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2154,7 +2161,9 @@ class TestModel:
                     "message_id": 1,
                     "stream_id": 10,
                     "rendered_content": "<p>new content</p>",
+                    "is_me_message": False,
                 },
+                False,
                 1,
                 {
                     "messages": {
@@ -2163,12 +2172,49 @@ class TestModel:
                             "stream_id": 10,
                             "content": "<p>new content</p>",
                             "subject": "old subject",
+                            "is_me_message": False,
+                        },
+                        2: {
+                            "id": 2,
+                            "stream_id": 10,
+                            "content": "old content",
+                            "subject": "old subject",
+                            "is_me_message": False,
+                        },
+                    },
+                    "topic_msg_ids": {
+                        10: {"new subject": set(), "old subject": {1, 2}},
+                    },
+                    "edited_messages": {1},
+                    "topics": {10: ["new subject", "old subject"]},
+                },
+                False,
+                id="Message content is updated; both not me-messages",
+            ),
+            case(
+                {
+                    "message_id": 1,
+                    "stream_id": 10,
+                    "rendered_content": "<p>/me has new content</p>",
+                    "is_me_message": True,
+                },
+                False,
+                1,
+                {
+                    "messages": {
+                        1: {
+                            "id": 1,
+                            "stream_id": 10,
+                            "content": "<p>/me has new content</p>",
+                            "subject": "old subject",
+                            "is_me_message": True,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2178,17 +2224,54 @@ class TestModel:
                     "topics": {10: ["new subject", "old subject"]},
                 },
                 False,
-                id="Message content is updated",
+                id="Message content is updated; now a me-message",
+            ),
+            case(
+                {
+                    "message_id": 1,
+                    "stream_id": 10,
+                    "rendered_content": "<p>new content</p>",
+                    "is_me_message": False,
+                },
+                True,
+                1,
+                {
+                    "messages": {
+                        1: {
+                            "id": 1,
+                            "stream_id": 10,
+                            "content": "<p>new content</p>",
+                            "subject": "old subject",
+                            "is_me_message": False,
+                        },
+                        2: {
+                            "id": 2,
+                            "stream_id": 10,
+                            "content": "/me dances (old)",
+                            "subject": "old subject",
+                            "is_me_message": True,
+                        },
+                    },
+                    "topic_msg_ids": {
+                        10: {"new subject": set(), "old subject": {1, 2}},
+                    },
+                    "edited_messages": {1},
+                    "topics": {10: ["new subject", "old subject"]},
+                },
+                False,
+                id="Message content is updated; was a me-message, not now",
             ),
             case(
                 {  # Both message content and subject is updated.
                     "message_id": 1,
                     "rendered_content": "<p>new content</p>",
+                    "is_me_message": False,
                     "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
                 },
+                False,
                 2,
                 {  # 2=update of subject & content
                     "messages": {
@@ -2197,12 +2280,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "<p>new content</p>",
                             "subject": "new subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2219,6 +2304,7 @@ class TestModel:
                     "message_id": 1,
                     "foo": "boo",
                 },
+                False,
                 0,
                 {
                     "messages": {
@@ -2227,12 +2313,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2248,11 +2336,13 @@ class TestModel:
                 {  # message_id not present in index, topic view closed.
                     "message_id": 3,
                     "rendered_content": "<p>new content</p>",
+                    "is_me_message": False,
                     "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [3],
                 },
+                False,
                 0,
                 {
                     "messages": {
@@ -2261,12 +2351,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2282,11 +2374,13 @@ class TestModel:
                 {  # message_id not present in index, topic view is enabled.
                     "message_id": 3,
                     "rendered_content": "<p>new content</p>",
+                    "is_me_message": False,
                     "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [3],
                 },
+                False,
                 0,
                 {
                     "messages": {
@@ -2295,12 +2389,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2316,11 +2412,13 @@ class TestModel:
                 {  # Message content is updated and topic view is enabled.
                     "message_id": 1,
                     "rendered_content": "<p>new content</p>",
+                    "is_me_message": False,
                     "orig_subject": "old subject",
                     "subject": "new subject",
                     "stream_id": 10,
                     "message_ids": [1],
                 },
+                False,
                 2,
                 {
                     "messages": {
@@ -2329,12 +2427,14 @@ class TestModel:
                             "stream_id": 10,
                             "content": "<p>new content</p>",
                             "subject": "new subject",
+                            "is_me_message": False,
                         },
                         2: {
                             "id": 2,
                             "stream_id": 10,
                             "content": "old content",
                             "subject": "old subject",
+                            "is_me_message": False,
                         },
                     },
                     "topic_msg_ids": {
@@ -2353,20 +2453,23 @@ class TestModel:
         mocker,
         model,
         event,
+        initially_me_message,
         expected_index,
         expected_times_messages_rerendered,
         topic_view_enabled,
     ):
         event["type"] = "update_message"
 
+        initial_message_data = {  # for all messages in index, base data
+            "stream_id": 10,
+            "content": "/me dances (old)" if initially_me_message else "old content",
+            "subject": "old subject",
+            "is_me_message": initially_me_message,
+        }
+
         model.index = {
             "messages": {
-                message_id: {
-                    "id": message_id,
-                    "stream_id": 10,
-                    "content": "old content",
-                    "subject": "old subject",
-                }
+                message_id: dict(initial_message_data, id=message_id)
                 for message_id in [1, 2]
             },
             "topic_msg_ids": {  # FIXME? consider test for eg. absence of empty set