Repository: zulip/zulip-terminal
Pull request #1245 (2022-08-15 04:34:23)

Skip muted topics and streams on n key
<!-- Please see https://github.com/zulip/zulip-terminal#contributor-guidelines ! -->

**What does this PR do?**  <!-- Overall description goes here -->

This is an adjusted version of the rebuild of #442 in #1239, so if merged will supersede both of those.

The motivation for this was upon considering the test adjustments which were absent in the draft updated PR (#1239), it seemed much more natural to migrate the appropriate function into the model itself. The main logic is therefore the same as #442, but the surrounding code is quite different, including tests.

I'm strongly considering merging this promptly, but I'd welcome a read-through, and testing by anyone who looked at #T1239.

This was discussed in #**zulip-terminal > Skip muted topics and streams on 'n' #T442 #T1239**
(may update topic name given this PR)

**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 -->

This explicitly adds what I believe is the current behavior of ordering by stream id and then topic name. As indicated in #1239, there is likely a lot of scope for improvement beyond that, assuming we want to mimic web app use of this feature.


Patch:
diff --git a/zulipterminal/model.py b/zulipterminal/model.py
index 4c30411..a8d0997 100644
--- a/zulipterminal/model.py
+++ b/zulipterminal/model.py
@@ -102,6 +102,7 @@ class Model:
         self.stream_id: Optional[int] = None
         self.recipients: FrozenSet[Any] = frozenset()
         self.index = initial_index
+        self._last_unread_topic = None
 
         self.user_id = -1
         self.user_email = ""
@@ -797,6 +798,26 @@ class Model:
         topic_to_search = (stream_name, topic)
         return topic_to_search in self._muted_topics.keys()
 
+    def get_next_unread_topic(self) -> Optional[Tuple[int, str]]:
+        unread_topics = sorted(self.unread_counts["unread_topics"].keys())
+        next_topic = False
+        if self._last_unread_topic not in unread_topics:
+            next_topic = True
+        # 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:
+                next_topic = True
+        return None
+
     def _fetch_initial_data(self) -> None:
         # Thread Processes to reduce start time.
         # NOTE: Exceptions do not work well with threads
diff --git a/zulipterminal/ui_tools/views.py b/zulipterminal/ui_tools/views.py
index 8f595f6..c212aa8 100644
--- a/zulipterminal/ui_tools/views.py
+++ b/zulipterminal/ui_tools/views.py
@@ -551,27 +551,11 @@ class MiddleColumnView(urwid.Frame):
         self.model = model
         self.controller = model.controller
         self.view = view
-        self.last_unread_topic = None
         self.last_unread_pm = None
         self.search_box = search_box
         view.message_view = message_view
         super().__init__(message_view, header=search_box, footer=write_box)
 
-    def get_next_unread_topic(self) -> Optional[Tuple[int, str]]:
-        topics = list(self.model.unread_counts["unread_topics"].keys())
-        next_topic = False
-        for topic in topics:
-            if next_topic is True:
-                self.last_unread_topic = topic
-                return topic
-            if topic == self.last_unread_topic:
-                next_topic = True
-        if len(topics) > 0:
-            topic = topics[0]
-            self.last_unread_topic = topic
-            return topic
-        return None
-
     def get_next_unread_pm(self) -> Optional[int]:
         pms = list(self.model.unread_counts["unread_pms"].keys())
         next_pm = False
@@ -631,7 +615,7 @@ class MiddleColumnView(urwid.Frame):
 
         elif is_command_key("NEXT_UNREAD_TOPIC", key):
             # narrow to next unread topic
-            stream_topic = self.get_next_unread_topic()
+            stream_topic = self.model.get_next_unread_topic()
             if stream_topic is None:
                 return key
             stream_id, topic = stream_topic


Test patch:
diff --git a/tests/model/test_model.py b/tests/model/test_model.py
index 6db23cf..7fe09f0 100644
--- a/tests/model/test_model.py
+++ b/tests/model/test_model.py
@@ -71,6 +71,7 @@ class TestModel:
         assert model.stream_dict == stream_dict
         assert model.recipients == frozenset()
         assert model.index == initial_index
+        assert model._last_unread_topic is None
         model.get_messages.assert_called_once_with(
             num_before=30, num_after=10, anchor=None
         )
@@ -3203,6 +3204,131 @@ class TestModel:
 
         assert return_value == is_muted
 
+    @pytest.mark.parametrize(
+        "unread_topics, last_unread_topic, next_unread_topic",
+        [
+            case(
+                {(1, "topic"), (2, "topic2")},
+                None,
+                (1, "topic"),
+                id="unread_present_after_no_state",
+            ),
+            case(
+                {(1, "topic"), (2, "topic2")},
+                (1, "topic"),
+                (2, "topic2"),
+                id="unread_present_after_previous_topic",
+            ),
+            case(
+                {(1, "topic"), (2, "topic2")},
+                (2, "topic2"),
+                (1, "topic"),
+                id="unread_present_before_previous_topic",
+            ),
+            case(
+                {(1, "topic")},
+                (1, "topic"),
+                (1, "topic"),
+                id="unread_still_present_in_topic",  # TODO Should be None? (2 other cases)
+            ),
+            case(
+                {},
+                (1, "topic"),
+                None,
+                id="no_unreads_with_previous_topic_state",
+            ),
+            case({}, None, None, id="no_unreads_with_no_previous_topic_state"),
+            case(
+                {(1, "topic"), (2, "topic2"), (3, "topic3")},
+                (2, "topic2"),
+                (1, "topic"),
+                id="unread_present_before_previous_topic_skipping_muted_stream",
+            ),
+            case(
+                {(1, "topic"), (2, "topic2"), (3, "topic3"), (4, "topic4")},
+                (2, "topic2"),
+                (4, "topic4"),
+                id="unread_present_after_previous_topic_skipping_muted_stream",
+            ),
+            case(
+                {(3, "topic3")},
+                (2, "topic2"),
+                None,
+                id="unread_present_only_in_muted_stream",
+            ),
+            case(
+                {(3, "topic3")},
+                (3, "topic3"),
+                None,
+                id="unread_present_starting_in_muted_stream",  # TODO See other 2 cases
+            ),
+            case(
+                {(3, "topic3"), (4, "topic4")},
+                None,
+                (4, "topic4"),
+                id="unread_present_skipping_first_muted_stream_unread",
+            ),
+            case(
+                {(2, "topic2 muted")},
+                None,
+                None,
+                id="unread_present_only_in_muted_topic",
+            ),
+            case(
+                {(2, "topic2 muted")},
+                (2, "topic2 muted"),
+                None,
+                id="unread_present_starting_in_muted_topic",  # TODO See other 2 cases
+            ),
+            case(
+                {(3, "topic3 muted")},
+                None,
+                None,
+                id="unread_present_only_in_muted_topic_in_muted_stream",
+            ),
+            case(
+                {(2, "topic2"), (2, "topic2 muted"), (4, "topic4")},
+                (2, "topic2"),
+                (4, "topic4"),
+                id="unread_present_after_previous_topic_skipping_muted_topic",
+            ),
+            case(
+                {(2, "topic2"), (2, "muted topic2")},
+                (2, "muted topic2"),
+                (2, "topic2"),
+                id="unread_present_after_previous_topic_muted",
+            ),
+        ],
+    )
+    def test_get_next_unread_topic(
+        self, model, unread_topics, last_unread_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
+
+        # Minimal extra streams for muted stream testing (should not exist otherwise)
+        assert {3, 4} & set(model.stream_dict) == set()
+        model.stream_dict[3] = {"name": "Stream 3"}
+        model.stream_dict[4] = {"name": "Stream 4"}
+        model.muted_streams = {3}
+
+        # date data unimportant (if present)
+        model._muted_topics = {
+            stream_topic: None
+            for stream_topic in [
+                ("Stream 2", "muted topic2"),
+                ("Stream 2", "topic2 muted"),
+                ("Stream 3", "topic3 muted"),
+            ]
+        }
+
+        unread_topic = model.get_next_unread_topic()
+
+        assert unread_topic == next_unread_topic
+
     @pytest.mark.parametrize(
         "stream_id, expected_response",
         [
diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py
index 1555d92..a527dd0 100644
--- a/tests/ui/test_ui_tools.py
+++ b/tests/ui/test_ui_tools.py
@@ -824,7 +824,6 @@ class TestMiddleColumnView:
     def test_init(self, mid_col_view):
         assert mid_col_view.model == self.model
         assert mid_col_view.controller == self.model.controller
-        assert mid_col_view.last_unread_topic is None
         assert mid_col_view.last_unread_pm is None
         assert mid_col_view.search_box == self.search_box
         assert self.view.message_view == "MSG_LIST"
@@ -832,25 +831,6 @@ class TestMiddleColumnView:
             "MSG_LIST", header=self.search_box, footer=self.write_box
         )
 
-    def test_get_next_unread_topic(self, mid_col_view):
-        mid_col_view.model.unread_counts = {"unread_topics": {1: 1, 2: 1}}
-        return_value = mid_col_view.get_next_unread_topic()
-        assert return_value == 1
-        assert mid_col_view.last_unread_topic == 1
-
-    def test_get_next_unread_topic_again(self, mid_col_view):
-        mid_col_view.model.unread_counts = {"unread_topics": {1: 1, 2: 1}}
-        mid_col_view.last_unread_topic = 1
-        return_value = mid_col_view.get_next_unread_topic()
-        assert return_value == 2
-        assert mid_col_view.last_unread_topic == 2
-
-    def test_get_next_unread_topic_no_unread(self, mid_col_view):
-        mid_col_view.model.unread_counts = {"unread_topics": {}}
-        return_value = mid_col_view.get_next_unread_topic()
-        assert return_value is None
-        assert mid_col_view.last_unread_topic is None
-
     def test_get_next_unread_pm(self, mid_col_view):
         mid_col_view.model.unread_counts = {"unread_pms": {1: 1, 2: 1}}
         return_value = mid_col_view.get_next_unread_pm()
@@ -940,17 +920,16 @@ class TestMiddleColumnView:
     ):
         size = widget_size(mid_col_view)
         mocker.patch(MIDCOLVIEW + ".focus_position")
-        mocker.patch(
-            MIDCOLVIEW + ".get_next_unread_topic",
-            return_value=("1", "topic"),
-        )
-        mid_col_view.model.stream_dict = {"1": {"name": "stream"}}
-        mid_col_view.keypress(size, key)
 
-        mid_col_view.get_next_unread_topic.assert_called_once_with()
+        mid_col_view.model.stream_dict = {1: {"name": "stream"}}
+        mid_col_view.model.get_next_unread_topic.return_value = (1, "topic")
+
+        return_value = mid_col_view.keypress(size, key)
+
         mid_col_view.controller.narrow_to_topic.assert_called_once_with(
             stream_name="stream", topic_name="topic"
         )
+        assert return_value == key
 
     @pytest.mark.parametrize("key", keys_for_command("NEXT_UNREAD_TOPIC"))
     def test_keypress_NEXT_UNREAD_TOPIC_no_stream(
@@ -958,9 +937,11 @@ class TestMiddleColumnView:
     ):
         size = widget_size(mid_col_view)
         mocker.patch(MIDCOLVIEW + ".focus_position")
-        mocker.patch(MIDCOLVIEW + ".get_next_unread_topic", return_value=None)
+        mid_col_view.model.get_next_unread_topic.return_value = None
 
         return_value = mid_col_view.keypress(size, key)
+
+        assert mid_col_view.controller.narrow_to_topic.called is False
         assert return_value == key
 
     @pytest.mark.parametrize("key", keys_for_command("NEXT_UNREAD_PM"))