Repository: zulip/zulip-terminal

Issue:
Support new `=` keyboard shortcut for 'reaction agreement'
This is the equivalent of zulip/zulip#24266 for this project.

Also see zulip/zulip#24828 for the related documentation, which should accompany this.

Discussion:
zulipbot: Hello @jagadeesh-akshaya, it seems like you have referenced #1369 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run `git commit --amend` in your command line client to amend your commit message description with `Fixes #1369.`.

An example of a correctly-formatted commit:

```
commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.
```

To learn how to write a great commit message, please refer to our [guide](https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages).

<!-- fixCommitWarning -->

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

<!-- areaLabelAddition -->

neiljp: @jagadeesh-akshaya You might also find the asciinema link in the PR template useful for videos!
neiljp: Regarding merging this:
- I've pushed back after squashing the commits together and updating the commit text
- The logic here looks reasonable, but could do with tests to demonstrate behavior
- Removal of agreement appears to succeed according to the server, but locally can fail due to an existing bug with removing reactions with multiple users (WIP)



Patch:
diff --git a/docs/hotkeys.md b/docs/hotkeys.md
index c9677e8..38ad92b 100644
--- a/docs/hotkeys.md
+++ b/docs/hotkeys.md
@@ -55,6 +55,7 @@
 |Narrow to the stream of the current message|<kbd>s</kbd>|
 |Narrow to the topic of the current message|<kbd>S</kbd>|
 |Narrow to a topic/direct-chat, or stream/all-direct-messages|<kbd>z</kbd>|
+|Toggle first emoji reaction on selected message|<kbd>=</kbd>|
 |Add/remove thumbs-up reaction to the current message|<kbd>+</kbd>|
 |Add/remove star status of the current message|<kbd>ctrl</kbd> + <kbd>s</kbd> / <kbd>*</kbd>|
 |Show/hide message information|<kbd>i</kbd>|
diff --git a/zulipterminal/config/keys.py b/zulipterminal/config/keys.py
index 27c9f35..f6f6c09 100644
--- a/zulipterminal/config/keys.py
+++ b/zulipterminal/config/keys.py
@@ -178,6 +178,11 @@ KEY_BINDINGS: Dict[str, KeyBinding] = {
             'Narrow to a topic/direct-chat, or stream/all-direct-messages',
         'key_category': 'msg_actions',
     },
+    'REACTION_AGREEMENT': {
+        'keys': ['='],
+        'help_text': 'Toggle first emoji reaction on selected message',
+        'key_category': 'msg_actions',
+    },
     'TOGGLE_TOPIC': {
         'keys': ['t'],
         'help_text': 'Toggle topics in a stream',
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 82d809a..7073e4f 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -2,6 +2,7 @@
 Defines the `Model`, fetching and storing data retrieved from the Zulip server
 """
 
+import bisect
 import itertools
 import json
 import time
@@ -111,7 +112,6 @@ class Model:
         self.stream_id: Optional[int] = None
         self.recipients: FrozenSet[Any] = frozenset()
         self.index = initial_index
-        self._last_unread_topic = None
         self.last_unread_pm = None
 
         self.user_id = -1
@@ -886,23 +886,67 @@ class Model:
         topic_to_search = (stream_name, topic)
         return topic_to_search in self._muted_topics
 
-    def get_next_unread_topic(self) -> Optional[Tuple[int, str]]:
+    def stream_topic_from_message_id(
+        self, message_id: int
+    ) -> Optional[Tuple[int, str]]:
+        """
+        Returns the stream and topic of a message of a given message id.
+        If the message is not a stream message or if it is not present in the index,
+        None is returned.
+        """
+        message = self.index["messages"].get(message_id, None)
+        if message is not None and message["type"] == "stream":
+            stream_id = message["stream_id"]
+            topic = message["subject"]
+            return (stream_id, topic)
+        return None
+
+    def next_unread_topic_from_message_id(
+        self, current_message: Optional[int]
+    ) -> Optional[Tuple[int, str]]:
+        if current_message:
+            current_topic = self.stream_topic_from_message_id(current_message)
+        else:
+            current_topic = (
+                self.stream_id_from_name(self.narrow[0][1]),
+                self.narrow[1][1],
+            )
         unread_topics = sorted(self.unread_counts["unread_topics"].keys())
         next_topic = False
-        if self._last_unread_topic not in unread_topics:
+        stream_start: Optional[Tuple[int, str]] = None
+        if current_topic is None:
             next_topic = True
+        elif current_topic not in unread_topics:
+            # insert current_topic in list of unread_topics for the case where
+            # current_topic is not in unread_topics, and the next unmuted topic
+            # is to be returned. This does not modify the original unread topics
+            # data, and is just used to compute the next unmuted topic to be returned.
+            bisect.insort(unread_topics, current_topic)
         # loop over unread_topics list twice for the case that last_unread_topic was
         # the last valid unread_topic in unread_topics list.
         for unread_topic in unread_topics * 2:
             stream_id, topic_name = unread_topic
-            if (
-                not self.is_muted_topic(stream_id, topic_name)
-                and not self.is_muted_stream(stream_id)
-                and next_topic
-            ):
-                self._last_unread_topic = unread_topic
-                return unread_topic
-            if unread_topic == self._last_unread_topic:
+            if not self.is_muted_topic(
+                stream_id, topic_name
+            ) and not self.is_muted_stream(stream_id):
+                if next_topic:
+                    if unread_topic == current_topic:
+                        return None
+                    if (
+                        current_topic is not None
+                        and unread_topic[0] != current_topic[0]
+                        and stream_start != current_topic
+                    ):
+                        return stream_start
+                    return unread_topic
+
+                if (
+                    stream_start is None
+                    and current_topic is not None
+                    and unread_topic[0] == current_topic[0]
+                ):
+                    stream_start = unread_topic
+            if unread_topic == current_topic:
                 next_topic = True
         return None
 
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index 64eebeb..1622114 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -236,6 +236,15 @@ class MessageView(urwid.ListBox):
             message = self.focus.original_widget.message
             self.model.toggle_message_star_status(message)
 
+        elif is_command_key("REACTION_AGREEMENT", key) and self.focus is not None:
+            message = self.focus.original_widget.message
+            message_reactions = message["reactions"]
+            if len(message_reactions) > 0:
+                for reaction in message_reactions:
+                    emoji = reaction["emoji_name"]
+                    self.model.toggle_message_reaction(message, emoji)
+                    break
+
         key = super().keypress(size, key)
         return key
 
@@ -584,9 +593,20 @@ class MiddleColumnView(urwid.Frame):
 
         elif is_command_key("NEXT_UNREAD_TOPIC", key):
             # narrow to next unread topic
-            stream_topic = self.model.get_next_unread_topic()
-            if stream_topic is None:
+            focus = self.view.message_view.focus
+            narrow = self.model.narrow
+            if focus:
+                current_msg_id = focus.original_widget.message["id"]
+                stream_topic = self.model.next_unread_topic_from_message_id(
+                    current_msg_id
+                )
+                if stream_topic is None:
+                    return key
+            elif narrow[0][0] == "stream" and narrow[1][0] == "topic":
+                stream_topic = self.model.next_unread_topic_from_message_id(None)
+            else:
                 return key
+
             stream_id, topic = stream_topic
             self.controller.narrow_to_topic(
                 stream_name=self.model.stream_dict[stream_id]["name"],


Test patch:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index 2bf150a..fe2ba55 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -72,7 +72,6 @@ class TestModel:
         assert model.stream_dict == stream_dict
         assert model.recipients == frozenset()
         assert model.index == initial_index
-        assert model._last_unread_topic is None
         assert model.last_unread_pm is None
         model.get_messages.assert_called_once_with(
             num_before=30, num_after=10, anchor=None
@@ -3900,7 +3899,7 @@ class TestModel:
         assert return_value == is_muted
 
     @pytest.mark.parametrize(
-        "unread_topics, last_unread_topic, next_unread_topic",
+        "unread_topics, current_topic, next_unread_topic",
         [
             case(
                 {(1, "topic"), (2, "topic2")},
@@ -3920,10 +3919,10 @@ class TestModel:
                 (1, "topic"),
                 id="unread_present_before_previous_topic",
             ),
-            case(  # TODO Should be None? (2 other cases)
+            case(
                 {(1, "topic")},
                 (1, "topic"),
-                (1, "topic"),
+                None,
                 id="unread_still_present_in_topic",
             ),
             case(
@@ -3993,16 +3992,42 @@ class TestModel:
                 (2, "topic2"),
                 id="unread_present_after_previous_topic_muted",
             ),
+            case(
+                {(1, "topic1"), (2, "topic2"), (2, "topic2 muted")},
+                (2, "topic1"),
+                (2, "topic2"),
+                id="unmuted_unread_present_in_same_stream_as_current_topic_not_in_unread_list",
+            ),
+            case(
+                {(1, "topic1"), (2, "topic2 muted"), (4, "topic4")},
+                (2, "topic1"),
+                (4, "topic4"),
+                id="unmuted_unread_present_in_next_stream_as_current_topic_not_in_unread_list",
+            ),
+            case(
+                {(1, "topic1"), (2, "topic2 muted"), (3, "topic3")},
+                (2, "topic1"),
+                (1, "topic1"),
+                id="unmuted_unread_not_present_in_next_stream_as_current_topic_not_in_unread_list",
+            ),
+            case(
+                {(1, "topic1"), (1, "topic11"), (2, "topic2")},
+                (1, "topic11"),
+                (1, "topic1"),
+                id="unread_present_in_same_stream_wrap_around",
+            ),
         ],
     )
-    def test_get_next_unread_topic(
-        self, model, unread_topics, last_unread_topic, next_unread_topic
+    def test_next_unread_topic_from_message(
+        self, mocker, model, unread_topics, current_topic, next_unread_topic
     ):
         # NOTE Not important how many unreads per topic, so just use '1'
         model.unread_counts = {
             "unread_topics": {stream_topic: 1 for stream_topic in unread_topics}
         }
-        model._last_unread_topic = last_unread_topic
+
+        current_message_id = 10  # Arbitrary value due to mock below
+        model.stream_topic_from_message_id = mocker.Mock(return_value=current_topic)
 
         # Minimal extra streams for muted stream testing (should not exist otherwise)
         assert {3, 4} & set(model.stream_dict) == set()
@@ -4020,7 +4045,38 @@ class TestModel:
             ]
         }
 
-        unread_topic = model.get_next_unread_topic()
+        unread_topic = model.next_unread_topic_from_message_id(current_message_id)
+
+        assert unread_topic == next_unread_topic
+
+    @pytest.mark.parametrize(
+        "unread_topics, empty_narrow, narrow_stream_id, next_unread_topic",
+        [
+            case(
+                {(1, "topic1"), (1, "topic2"), (2, "topic3")},
+                [["stream", "Stream 1"], ["topic", "topic1.5"]],
+                1,
+                (1, "topic2"),
+            ),
+        ],
+    )
+    def test_next_unread_topic_from_message__empty_narrow(
+        self,
+        mocker,
+        model,
+        unread_topics,
+        empty_narrow,
+        narrow_stream_id,
+        next_unread_topic,
+    ):
+        # NOTE Not important how many unreads per topic, so just use '1'
+        model.unread_counts = {
+            "unread_topics": {stream_topic: 1 for stream_topic in unread_topics}
+        }
+        model.stream_id_from_name = mocker.Mock(return_value=narrow_stream_id)
+        model.narrow = empty_narrow
+
+        unread_topic = model.next_unread_topic_from_message_id(None)
 
         assert unread_topic == next_unread_topic
 
@@ -4043,6 +4099,21 @@ class TestModel:
         assert return_value is None
         assert model.last_unread_pm is None
 
+    @pytest.mark.parametrize(
+        "message_id, expected_value",
+        [
+            case(537286, (205, "Test"), id="stream_message"),
+            case(537287, None, id="direct_message"),
+            case(537289, None, id="non-existent message"),
+        ],
+    )
+    def test_stream_topic_from_message_id(
+        self, mocker, model, message_id, expected_value, empty_index
+    ):
+        model.index = empty_index
+        current_topic = model.stream_topic_from_message_id(message_id)
+        assert current_topic == expected_value
+
     @pytest.mark.parametrize(
         "stream_id, expected_response",
         [
diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py
index c04c9c3..324ee04 100644
--- a/tests/ui/test_ui_tools.py
+++ b/tests/ui/test_ui_tools.py
@@ -887,9 +887,10 @@ class TestMiddleColumnView:
     ):
         size = widget_size(mid_col_view)
         mocker.patch(MIDCOLVIEW + ".focus_position")
+        mocker.patch.object(self.view, "message_view")
 
         mid_col_view.model.stream_dict = {1: {"name": "stream"}}
-        mid_col_view.model.get_next_unread_topic.return_value = (1, "topic")
+        mid_col_view.model.next_unread_topic_from_message_id.return_value = (1, "topic")
 
         return_value = mid_col_view.keypress(size, key)
 
@@ -904,7 +905,8 @@ class TestMiddleColumnView:
     ):
         size = widget_size(mid_col_view)
         mocker.patch(MIDCOLVIEW + ".focus_position")
-        mid_col_view.model.get_next_unread_topic.return_value = None
+        mocker.patch.object(self.view, "message_view")
+        mid_col_view.model.next_unread_topic_from_message_id.return_value = None
 
         return_value = mid_col_view.keypress(size, key)