Repository: zulip/zulip-terminal
Pull request #1178 (2022-03-24 13:31:35)

model: Improve reporting upon moving/splitting topic.
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->
This PR improves reporting upon moving a message by editing it's topic.

<!-- If fixing a filed bug or new feature, add 'Fixes #<issue>' or 'Partial fix for #<issue>' -->
Partial fix for #1172 

<!-- 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] New tests added (for any new behavior)
- [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
-->

**Notes & Questions** <!-- if any; add/delete/fill-in as appropriate -->
<!-- For example:
- this doesn't include feature X (yet?)
- unsure about Y
- should this do Z?
-->

**Interactions** <!-- if any; add/delete/fill-in as appropriate -->
<!-- eg.
- Waiting on #<PR>
- Blocks #<PR>
-->

**Visual changes** <!-- if any; add/delete/fill-in with screenshot/diagram as appropriate -->

https://user-images.githubusercontent.com/53873549/159926734-3f04ffb8-c35f-4836-8d76-ae7401e6fca6.mp4



srdeotarse: Rebased with current main branch and updated mypy. Still showing error below @neiljp -
```
Successfully installed autoflake-1.4 autopep8-1.6.0 flake8-4.0.1 isort-5.10.1 jedi-0.18.1 lxml-stubs-0.4.0 mypy-0.942 parso-0.8.3 pudb-2022.1.1 pycodestyle-2.8.0 pyflakes-2.4.0 pytz-2022.1 snakeviz-2.1.1 types-docutils-0.18.0 types-pygments-2.9.17 types-setuptools-57.4.11 zulip-term-0.6.0+git
(zt_venv) srdeotarse@LAPTOP-VC2MC73M:~/zulip-terminal$ make check
================================================================================
Import order (isort)
--------------------------------------------------------------------------------
All files have imports sorted according to rules.
================================================================================
Type consistency (mypy)
--------------------------------------------------------------------------------
Running mypy for `zulipterminal`.
zulipterminal/model.py:584: error: Invalid index type "Optional[int]" for "Dict[int, Set[int]]"; expected type "int"
Found 1 error in 1 file (checked 37 source files)
Running mypy for `tests`.
Success: no issues found in 15 source files
================================================================================
LINTING FAILED
make: *** [makefile:22: lint] Error 1
```
srdeotarse: Rebased with current main branch and updated mypy. Still showing error below @neiljp -

```
Successfully installed autoflake-1.4 autopep8-1.6.0 flake8-4.0.1 isort-5.10.1 jedi-0.18.1 lxml-stubs-0.4.0 mypy-0.942 parso-0.8.3 pudb-2022.1.1 pycodestyle-2.8.0 pyflakes-2.4.0 pytz-2022.1 snakeviz-2.1.1 types-docutils-0.18.0 types-pygments-2.9.17 types-setuptools-57.4.11 zulip-term-0.6.0+git
(zt_venv) srdeotarse@LAPTOP-VC2MC73M:~/zulip-terminal$ make check
================================================================================
Import order (isort)
--------------------------------------------------------------------------------
All files have imports sorted according to rules.
================================================================================
Type consistency (mypy)
--------------------------------------------------------------------------------
Running mypy for `zulipterminal`.
zulipterminal/model.py:584: error: Invalid index type "Optional[int]" for "Dict[int, Set[int]]"; expected type "int"
Found 1 error in 1 file (checked 37 source files)
Running mypy for `tests`.
Success: no issues found in 15 source files
================================================================================
LINTING FAILED
make: *** [makefile:22: lint] Error 1
```
neiljp: @srdeotarse Was there a reason you closed/reopened this? Generally there's no need to do so.
srdeotarse: @neiljp I was trying to rebase with main to solve mypy error for Optional[int] and accidentally deleted my local branch.

Patch:
diff --git a/zulipterminal/core.py b/zulipterminal/core.py
index d24f893..bde77a4 100644
--- a/zulipterminal/core.py
+++ b/zulipterminal/core.py
@@ -437,23 +437,23 @@ class Controller:
         self.is_typing_notification_in_progress = False
         self.view.set_footer_text()
 
-    def report_error(self, text: str) -> None:
+    def report_error(self, text: str, duration: int = 3) -> None:
         """
         Helper to show an error message in footer
         """
-        self.view.set_footer_text(text, "task:error", 3)
+        self.view.set_footer_text(text, "task:error", duration)
 
-    def report_success(self, text: str) -> None:
+    def report_success(self, text: str, duration: int = 3) -> None:
         """
         Helper to show a success message in footer
         """
-        self.view.set_footer_text(text, "task:success", 3)
+        self.view.set_footer_text(text, "task:success", duration)
 
-    def report_warning(self, text: str) -> None:
+    def report_warning(self, text: str, duration: int = 3) -> None:
         """
         Helper to show a warning message in footer
         """
-        self.view.set_footer_text(text, "task:warning", 3)
+        self.view.set_footer_text(text, "task:warning", duration)
 
     def search_messages(self, text: str) -> None:
         # Search for a text in messages
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index b0ed291..7eb45fc 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -576,10 +576,24 @@ class Model:
         response = self.client.update_message(request)
         display_error_if_present(response, self.controller)
         if response["result"] == "success":
-            old_topic = self.index["messages"][message_id].get("subject", None)
+            message = self.index["messages"][message_id]
+            stream_name = message.get("display_recipient", None)
+            old_topic = message.get("subject", None)
             new_topic = request["topic"]
+
+            recent_moved_msgs = ""
             if old_topic != new_topic:
-                self.controller.report_success("You changed a message's topic.")
+                if propagate_mode == "change_one":
+                    recent_moved_msgs = "one"
+                    one_message = f"You changed {recent_moved_msgs} message's topic from #{stream_name} > {old_topic} to #{stream_name} > {new_topic}."
+                    self.controller.report_success(one_message)
+                else:
+                    if propagate_mode == "change_all":
+                        recent_moved_msgs = "all"
+                    if propagate_mode == "change_later":
+                        recent_moved_msgs = "some"
+                    some_all_messages = f"You changed {recent_moved_msgs} messages' topic from #{stream_name} > {old_topic} to #{stream_name} > {new_topic}."
+                    self.controller.report_success(some_all_messages)
 
         return response["result"] == "success"
 


Test patch:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index ebf7651..cf0596d 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -834,9 +834,9 @@ class TestModel:
         ],
     )
     @pytest.mark.parametrize(
-        "req, old_topic, footer_updated",
+        "req, old_topic, expected_report_success",
         [
-            (
+            case(
                 {
                     "message_id": 1,
                     "propagate_mode": "change_one",
@@ -844,23 +844,27 @@ class TestModel:
                     "topic": "Some topic",
                 },
                 "Some topic",
-                False,
+                None,  # None as footer is not updated.
             ),
-            (
+            case(
                 {
                     "message_id": 1,
                     "propagate_mode": "change_one",
                     "topic": "Topic change",
                 },
                 "Old topic",
-                True,
+                "You changed one message's topic from #stream > Old topic to #stream > Topic change.",
             ),
-            (
-                {"message_id": 1, "propagate_mode": "change_all", "topic": "Old topic"},
+            case(
+                {
+                    "message_id": 1,
+                    "propagate_mode": "change_all",
+                    "topic": "Old topic",
+                },
                 "Old topic",
-                False,
+                None,  # None as footer is not updated.
             ),
-            (
+            case(
                 {
                     "message_id": 1,
                     "propagate_mode": "change_later",
@@ -868,9 +872,19 @@ class TestModel:
                     "topic": "terminal",
                 },
                 "terminal",
-                False,
+                None,  # None as footer is not updated.
             ),
-            (
+            case(
+                {
+                    "message_id": 1,
+                    "propagate_mode": "change_later",
+                    "content": ":smile:",
+                    "topic": "new_terminal",
+                },
+                "old_terminal",
+                "You changed some messages' topic from #stream > old_terminal to #stream > new_terminal.",
+            ),
+            case(
                 {
                     "message_id": 1,
                     "propagate_mode": "change_one",
@@ -878,9 +892,9 @@ class TestModel:
                     "topic": "grett",
                 },
                 "greet",
-                True,
+                "You changed one message's topic from #stream > greet to #stream > grett.",
             ),
-            (
+            case(
                 {
                     "message_id": 1,
                     "propagate_mode": "change_all",
@@ -888,24 +902,33 @@ class TestModel:
                     "topic": "party",
                 },
                 "lets_party",
-                True,
+                "You changed all messages' topic from #stream > lets_party to #stream > party.",
             ),
         ],
     )
     def test_update_stream_message(
-        self, mocker, model, response, return_value, req, old_topic, footer_updated
+        self,
+        mocker,
+        model,
+        response,
+        return_value,
+        req,
+        old_topic,
+        expected_report_success,
+        old_stream_name="stream",
     ):
         self.client.update_message = mocker.Mock(return_value=response)
         model.index["messages"][req["message_id"]]["subject"] = old_topic
-
+        model.index["messages"][req["message_id"]][
+            "display_recipient"
+        ] = old_stream_name
         result = model.update_stream_message(**req)
-
         self.client.update_message.assert_called_once_with(req)
         assert result == return_value
         self.display_error_if_present.assert_called_once_with(response, self.controller)
         report_success = model.controller.report_success
-        if result and footer_updated:
-            report_success.assert_called_once_with("You changed a message's topic.")
+        if result and expected_report_success is not None:
+            report_success.assert_called_once_with(expected_report_success)
         else:
             report_success.assert_not_called()