Repository: zulip/zulip-terminal
Pull request #1329 (2023-03-17 16:37:42)

Normalize update_stream_message behavior regarding notification messages
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->

This addresses the change in API behavior in update_stream introduced at ZFL 152, essentially requiring additional parameters to retain the previous behavior.
(ref https://zulip.com/api/update-message#parameter-send_notification_to_old_thread)

Prior to this change, another test for the same function is simplified and test-case ids added.

Further, to allow various names in the new function, the ruff exclusion list for naming linting is first updated to allow mixed case in general in tests/*.

After these changes, a commit applies typing to these parameters as defined at the above URL.

<!-- If fixing a filed bug or new feature, add 'Fixes #<issue>' or 'Partial fix for #<issue>' -->

<!-- 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! -->

**Notes & Questions** <!-- if any; add/delete/fill-in as appropriate -->
- Passing these parameters to the API will be necessary to use the new functionality in future, which this change enables
- This PR leaves topic editing as *not* giving a bot notification by default. This is both expected ZT behavior in previous server versions, as well as how the 'rename topic' behavior works with the web app.
- We could do a pop-up when only moving *one/some* messages from a topic, much like the web app does.

zulipbot: Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

<!-- areaLabelAddition -->


Patch:
diff --git a/README.md b/README.md
index d801787..0a9fcee 100644
--- a/README.md
+++ b/README.md
@@ -466,7 +466,7 @@ The source of truth is in the source code, so simply update the python file and
 run the relevant tool, as detailed below.
 
 Currently we have
-* `tools/generate-hotkeys.py` to regenerate docs/hotkeys.md from config/keys.py
+* `tools/lint-hotkeys --fix` to regenerate docs/hotkeys.md from config/keys.py
 * `tools/lint-docstring --fix` to regenerate docs/developer-file-overview.md from file docstrings
 
 (these tools are also used for the linting process to ensure that these files are synchronzed)
diff --git a/docs/hotkeys.md b/docs/hotkeys.md
index c49cb15..a71684b 100644
--- a/docs/hotkeys.md
+++ b/docs/hotkeys.md
@@ -1,4 +1,4 @@
-<!--- Generated automatically by tools/generate_hotkeys.py -->
+<!--- Generated automatically by tools/lint-hotkeys -->
 <!--- Do not modify -->
 
 # Hot Keys
diff --git a/pyproject.toml b/pyproject.toml
index 2e980fe..7a4c6b7 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -152,4 +152,6 @@ select = [
 # T20: Expect print output from CLI from run.py
 "zulipterminal/cli/run.py" = ["T20"]
 # N802: Allow upper-case in test function names, eg. for commands, such as test_keypress_ENTER
-"tests/*" = ["N802"]
+# N803: Allow upper-case in test function argument names, eg. ZFL, API
+# N806: Allow upper-case in test function variables
+"tests/*" = ["N802", "N803", "N806"]
diff --git a/tools/lint-all b/tools/lint-all
index 20287ad..1b148fc 100755
--- a/tools/lint-all
+++ b/tools/lint-all
@@ -20,10 +20,7 @@ tools = {
     "PEP8 & more (ruff)": ["ruff"] + python_sources,
     "Formatting (black)": ["black", "--check"] + python_sources,
     "Common spelling mistakes": "./tools/run-spellcheck",
-    "Hotkey linting & docs sync check": [
-        "./tools/generate_hotkeys.py",
-        "--check-only",
-    ],
+    "Hotkey linting & docs sync check": ["./tools/lint-hotkeys"],
     "Docstring linting & docs sync check (lint-docstring)": "./tools/lint-docstring",
 }
 
diff --git a/tools/generate_hotkeys.py b/tools/lint-hotkeys
similarity index 92%
rename from tools/generate_hotkeys.py
rename to tools/lint-hotkeys
index f79b7e9..68e9f8c 100755
--- a/tools/generate_hotkeys.py
+++ b/tools/lint-hotkeys
@@ -22,11 +22,11 @@ HELP_TEXT_STYLE = re.compile(r"^[a-zA-Z /()',&@#:_-]*$")
 KEYS_TO_EXCLUDE = ["q", "e", "m", "r"]
 
 
-def main(check_only: bool) -> None:
-    if check_only:
-        lint_hotkeys_file()
-    else:
+def main(fix: bool) -> None:
+    if fix:
         generate_hotkeys_file()
+    else:
+        lint_hotkeys_file()
 
 
 def lint_hotkeys_file() -> None:
@@ -144,14 +144,14 @@ def write_hotkeys_file(hotkeys_file_string: str) -> None:
 
 if __name__ == "__main__":
     parser = argparse.ArgumentParser(
-        description=f"Generate {OUTPUT_FILE_NAME} file by extracting key description and key "
-        f"combination from config/{KEYS_FILE_NAME} file"
+        description=f"Lint hotkeys by checking extracted key description style and key "
+        f"duplication from config/{KEYS_FILE_NAME} file"
     )
     parser.add_argument(
-        "--check-only",
+        "--fix",
         action="store_true",
-        help=f"Lint hotkeys by checking extracted key description style and key "
-        f"duplication from config/{KEYS_FILE_NAME} file",
+        help=f"Generate {OUTPUT_FILE_NAME} file by extracting key description and key "
+        f"combination from config/{KEYS_FILE_NAME} file",
     )
     args = parser.parse_args()
-    main(args.check_only)
+    main(args.fix)
diff --git a/zulipterminal/api_types.py b/zulipterminal/api_types.py
index 8e07823..a772c57 100644
--- a/zulipterminal/api_types.py
+++ b/zulipterminal/api_types.py
@@ -6,7 +6,7 @@ Types from the Zulip API, translated into python, to improve type checking
 
 from typing import Any, Dict, List, Optional, Union
 
-from typing_extensions import Literal, TypedDict
+from typing_extensions import Literal, NotRequired, TypedDict
 
 # These are documented in the zulip package (python-zulip-api repo)
 from zulip import EditPropagateMode  # one/all/later
@@ -36,9 +36,38 @@ class StreamComposition(TypedDict):
     subject: str  # TODO: Migrate to using topic
 
 
+# https://zulip.com/api/send-message
 Composition = Union[PrivateComposition, StreamComposition]
 
 
+class PrivateMessageUpdateRequest(TypedDict):
+    message_id: int
+    content: str
+
+
+class StreamMessageUpdateRequest(TypedDict):
+    message_id: int
+
+    # May update combination of content for specified message
+    # ...and/or topic of that message and potentially others (via mode)
+    # ...but content and stream may not be changed together
+    content: NotRequired[str]
+    topic: NotRequired[str]
+    propagate_mode: NotRequired[EditPropagateMode]
+
+    # Supported for stream moves in ZFL 9 (Zulip 3)
+    # Default values if not passed in ZFL 152 (Zulip 6)
+    send_notification_to_old_thread: NotRequired[bool]
+    send_notification_to_new_thread: NotRequired[bool]
+
+    # TODO: Implement message moves between streams
+    # stream_id: int
+
+
+# https://zulip.com/api/update-message
+MessageUpdateRequest = Union[PrivateMessageUpdateRequest, StreamMessageUpdateRequest]
+
+
 class Message(TypedDict, total=False):
     id: int
     sender_id: int
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index b727a6d..148e999 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -34,9 +34,11 @@ from zulipterminal.api_types import (
     EditPropagateMode,
     Event,
     PrivateComposition,
+    PrivateMessageUpdateRequest,
     RealmEmojiData,
     RealmUser,
     StreamComposition,
+    StreamMessageUpdateRequest,
     Subscription,
 )
 from zulipterminal.config.keys import primary_key_for_command
@@ -561,7 +563,7 @@ class Model:
         return message_was_sent
 
     def update_private_message(self, msg_id: int, content: str) -> bool:
-        request = {
+        request: PrivateMessageUpdateRequest = {
             "message_id": msg_id,
             "content": content,
         }
@@ -575,8 +577,10 @@ class Model:
         message_id: int,
         propagate_mode: EditPropagateMode,
         content: Optional[str] = None,
+        notify_old: bool = False,
+        notify_new: bool = False,
     ) -> bool:
-        request = {
+        request: StreamMessageUpdateRequest = {
             "message_id": message_id,
             "propagate_mode": propagate_mode,
             "topic": topic,
@@ -584,6 +588,10 @@ class Model:
         if content is not None:
             request["content"] = content
 
+        if self.server_feature_level is not None and self.server_feature_level >= 9:
+            request["send_notification_to_old_thread"] = notify_old
+            request["send_notification_to_new_thread"] = notify_new
+
         response = self.client.update_message(request)
         display_error_if_present(response, self.controller)
         if response["result"] == "success":


Test patch:
diff --git a/.github/workflows/lint-and-test.yml b/.github/workflows/lint-and-test.yml
index 371ef78..f52d1af 100644
--- a/.github/workflows/lint-and-test.yml
+++ b/.github/workflows/lint-and-test.yml
@@ -114,8 +114,8 @@ jobs:
           cache-dependency-path: 'setup.py'
       - name: Minimal install
         run: pip install .
-      - name: Run generate_hotkeys
-        run: ./tools/generate_hotkeys.py --check-only
+      - name: Run lint-hotkeys
+        run: ./tools/lint-hotkeys
 
   docstrings:
     runs-on: ubuntu-latest
diff --git a/tests/config/test_color.py b/tests/config/test_color.py
index c26f151..7191c3a 100644
--- a/tests/config/test_color.py
+++ b/tests/config/test_color.py
@@ -7,7 +7,7 @@ def test_color_properties() -> None:
     class Color(Enum):
         WHITE = "wh  #256  #24"
 
-    ExpandedColor = color_properties(Color, "BOLD", "ITALICS")  # noqa: N806
+    ExpandedColor = color_properties(Color, "BOLD", "ITALICS")
 
     assert ExpandedColor.WHITE in ExpandedColor
     assert ExpandedColor.WHITE.value == "wh  #256  #24"
diff --git a/tests/config/test_themes.py b/tests/config/test_themes.py
index c14fc89..73ae9cd 100644
--- a/tests/config/test_themes.py
+++ b/tests/config/test_themes.py
@@ -80,7 +80,7 @@ def test_builtin_theme_completeness(theme_name: str) -> None:
         fg, bg = style_conf
         assert fg in theme_colors and bg in theme_colors
     # Check completeness of META
-    expected_META = {"pygments": ["styles", "background", "overrides"]}  # noqa: N806
+    expected_META = {"pygments": ["styles", "background", "overrides"]}
     for metadata, config in expected_META.items():
         assert theme_meta[metadata]
         assert all(theme_meta[metadata][c] for c in config)
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index 8f64e0f..68bea8b 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -832,112 +832,105 @@ class TestModel:
     @pytest.mark.parametrize(
         "response, return_value",
         [
-            ({"result": "success"}, True),
-            ({"result": "some_failure"}, False),
+            case({"result": "success"}, True, id="API_success"),
+            case({"result": "some_failure"}, False, id="API_error"),
         ],
     )
     @pytest.mark.parametrize(
-        "req, old_topic, expected_report_success",
+        "kwargs, expected_report_success",
         [
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_one",
                     "content": "hi!",
-                    "topic": "Some topic",
+                    "topic": "old topic",
                 },
-                "Some topic",
                 None,  # None as footer is not updated.
+                id="topic_passed_but_same_so_not_changed:content_changed",
             ),
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_one",
                     "topic": "Topic change",
                 },
-                "Old topic",
                 [
                     "You changed one message's topic from ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
-                    ("footer_contrast", " Old topic "),
+                    ("footer_contrast", " old topic "),
                     " to ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
                     ("footer_contrast", " Topic change "),
                     " .",
                 ],
+                id="topic_changed[one]",
             ),
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_all",
-                    "topic": "Old topic",
+                    "topic": "old topic",
                 },
-                "Old topic",
                 None,  # None as footer is not updated.
+                id="topic_passed_but_same_so_not_changed[all]",
             ),
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_later",
                     "content": ":smile:",
-                    "topic": "terminal",
+                    "topic": "old topic",
                 },
-                "terminal",
                 None,  # None as footer is not updated.
+                id="topic_passed_but_same_so_not_changed[later]:content_changed",
             ),
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_later",
                     "content": ":smile:",
                     "topic": "new_terminal",
                 },
-                "old_terminal",
                 [
                     "You changed some messages' topic from ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
-                    ("footer_contrast", " old_terminal "),
+                    ("footer_contrast", " old topic "),
                     " to ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
                     ("footer_contrast", " new_terminal "),
                     " .",
                 ],
+                id="topic_changed[later]:content_changed",
             ),
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_one",
                     "content": "Hey!",
                     "topic": "grett",
                 },
-                "greet",
                 [
                     "You changed one message's topic from ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
-                    ("footer_contrast", " greet "),
+                    ("footer_contrast", " old topic "),
                     " to ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
                     ("footer_contrast", " grett "),
                     " .",
                 ],
+                id="topic_changed[one]:content_changed",
             ),
             case(
                 {
-                    "message_id": 1,
                     "propagate_mode": "change_all",
                     "content": "Lets party!",
                     "topic": "party",
                 },
-                "lets_party",
                 [
                     "You changed all messages' topic from ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
-                    ("footer_contrast", " lets_party "),
+                    ("footer_contrast", " old topic "),
                     " to ",
                     ("footer_contrast", f" stream {STREAM_TOPIC_SEPARATOR} "),
                     ("footer_contrast", " party "),
                     " .",
                 ],
+                id="topic_changed[all]:content_changed",
             ),
         ],
     )
@@ -947,26 +940,88 @@ class TestModel:
         model,
         response,
         return_value,
-        req,
-        old_topic,
+        kwargs,
         expected_report_success,
+        old_content="old content",
         old_stream_name="stream",
+        old_topic_name="old topic",
+        message_id=1,
     ):
         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)
+        model.index["messages"][message_id]["subject"] = old_topic_name
+        model.index["messages"][message_id]["display_recipient"] = old_stream_name
+        model.index["messages"][message_id]["content"] = old_content
+
+        result = model.update_stream_message(message_id=message_id, **kwargs)
+
         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 expected_report_success is not None:
             report_success.assert_called_once_with(expected_report_success, duration=6)
         else:
             report_success.assert_not_called()
 
+        # Implementation detail; only check here that it's always called
+        assert self.client.update_message.called
+
+    @pytest.mark.parametrize(
+        "ZFL, expect_API_notify_args",
+        [
+            (None, False),
+            (8, False),
+            (9, True),
+            (152, True),
+        ],
+    )
+    @pytest.mark.parametrize(
+        "notify_args, API_notify_old_thread, API_notify_new_thread",
+        [
+            ({}, False, False),
+            (dict(notify_new=False), False, False),
+            (dict(notify_old=False), False, False),
+            (dict(notify_old=True), True, False),
+            (dict(notify_new=True), False, True),
+            (dict(notify_new=True, notify_old=True), True, True),
+        ],
+    )
+    def test_update_stream_message__notify_thread_vs_ZFL(
+        self,
+        model,
+        ZFL,
+        expect_API_notify_args,
+        notify_args,
+        API_notify_old_thread,
+        API_notify_new_thread,
+        message_id=1,
+        old_content="old content",
+        old_topic="old topic",
+        old_stream_name="old stream name",
+        new_topic="new topic",
+        mode="change_all",
+    ) -> None:
+        model.server_feature_level = ZFL
+        model.index["messages"][message_id]["subject"] = old_topic
+        model.index["messages"][message_id]["display_recipient"] = old_stream_name
+        model.index["messages"][message_id]["content"] = old_content
+
+        base_kwargs = dict(message_id=message_id, topic=new_topic, propagate_mode=mode)
+
+        model.update_stream_message(**base_kwargs, **notify_args)
+
+        # Implementation detail, but only way of testing
+        if expect_API_notify_args:
+            expected_API_args = dict(
+                base_kwargs,
+                send_notification_to_old_thread=API_notify_old_thread,
+                send_notification_to_new_thread=API_notify_new_thread,
+            )
+        else:
+            expected_API_args = dict(base_kwargs)
+        self.client.update_message.assert_called_once_with(expected_API_args)
+
     @pytest.mark.parametrize(
         "response, return_value",
         [
@@ -2462,9 +2517,7 @@ class TestModel:
         Input is a list of pairs, of a message-id and a list of reaction tuples
         NOTE: reactions as None indicate not indexed, [] indicates no reaction
         """
-        MsgsType = List[  # noqa: N806
-            Tuple[int, Optional[List[Tuple[int, str, str, str]]]]
-        ]
+        MsgsType = List[Tuple[int, Optional[List[Tuple[int, str, str, str]]]]]
 
         def _factory(msgs: MsgsType):
             return {