Repository: zulip/zulip-terminal

Issue:
Narrowing with a contextual message id should select that message
If using a link such as generated in a quoted message, then the UI doesn't select that message - which seems like the behavior to be expected.

To reproduce:
* quote a message and send it
* move cursor onto message with quote
* open message info popup, move cursor onto link to original message & press enter

I'd expect the original message to be selected, but we don't do this.

Current behavior is:
* if narrow is loaded/open, do nothing?
* otherwise, load messages around message id; unclear which message is selected?

Discussion:
zee-bit: @zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"
zee-bit: @zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update"
zee-bit: There's a prep PR for this that might simplify the multiple-message index generation in `conftest` here : #1022.
neiljp: @zee-bit I adjusted some commit text, added test cases for the the anchor being another value in the narrow, and moved the expected value into the parametrize block in the last commit (like in the earlier one) - merging now :tada:

Patch:
diff --git a/zulipterminal/core.py b/zulipterminal/core.py
index c503aa4..8486b4d 100644
--- a/zulipterminal/core.py
+++ b/zulipterminal/core.py
@@ -386,13 +386,16 @@ class Controller:
 
     def _narrow_to(self, anchor: Optional[int], **narrow: Any) -> None:
         already_narrowed = self.model.set_narrow(**narrow)
-        if already_narrowed:
+
+        if already_narrowed and anchor is None:
             return
 
         msg_id_list = self.model.get_message_ids_in_current_narrow()
 
-        # if no messages are found get more messages
-        if len(msg_id_list) == 0:
+        # If no messages are found in the current narrow
+        # OR, given anchor is not present in msg_id_list
+        # then, get more messages.
+        if len(msg_id_list) == 0 or (anchor is not None and anchor not in msg_id_list):
             self.model.get_messages(num_before=30, num_after=10, anchor=anchor)
             msg_id_list = self.model.get_message_ids_in_current_narrow()
 


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 9fdaede..b29b15a 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -356,6 +356,14 @@ def stream_msg_template():
     return msg_template
 
 
+@pytest.fixture
+def extra_stream_msg_template():
+    msg_template = msg_template_factory(
+        537289, "stream", 1520918740, subject="Test", stream_id=205
+    )
+    return msg_template
+
+
 @pytest.fixture
 def pm_template():
     recipients = display_recipient_factory([(5179, "Boo Boo"), (5140, "Foo Foo")])
@@ -730,6 +738,20 @@ def index_topic(empty_index):
     return dict(empty_index, **diff)
 
 
+@pytest.fixture
+def index_multiple_topic_msg(empty_index, extra_stream_msg_template):
+    """
+    Index of initial_data with multiple message when model.narrow = [['stream, '7'],
+                                                                     ['topic', 'Test']]
+    """
+    empty_index_with_multiple_topic_msg = empty_index
+    empty_index_with_multiple_topic_msg["messages"].update(
+        {extra_stream_msg_template["id"]: extra_stream_msg_template}
+    )
+    diff = {"topic_msg_ids": defaultdict(dict, {205: {"Test": {537286, 537289}}})}
+    return dict(empty_index_with_multiple_topic_msg, **diff)
+
+
 @pytest.fixture
 def index_user(empty_index):
     """
diff --git a/tests/core/test_core.py b/tests/core/test_core.py
index 9056ce8..86416f9 100644
--- a/tests/core/test_core.py
+++ b/tests/core/test_core.py
@@ -126,13 +126,37 @@ class TestController:
         id_list = index_stream["stream_msg_ids_by_stream_id"][stream_id]
         assert {widget.original_widget.message["id"]} == id_list
 
-    def test_narrow_to_topic(self, mocker, controller, msg_box, index_topic):
+    @pytest.mark.parametrize(
+        ["initial_narrow", "initial_stream_id", "anchor", "expected_final_focus"],
+        [
+            ([], None, None, 537289),
+            ([["stream", "PTEST"], ["topic", "Test"]], 205, 537286, 537286),
+            ([["stream", "PTEST"], ["topic", "Test"]], 205, 537289, 537289),
+        ],
+        ids=[
+            "all-messages_to_topic_narrow_no_anchor",
+            "topic_narrow_to_same_topic_narrow_with_anchor",
+            "topic_narrow_to_same_topic_narrow_with_other_anchor",
+        ],
+    )
+    def test_narrow_to_topic(
+        self,
+        mocker,
+        controller,
+        msg_box,
+        index_multiple_topic_msg,
+        initial_narrow,
+        initial_stream_id,
+        anchor,
+        expected_final_focus,
+    ):
         expected_narrow = [
             ["stream", msg_box.stream_name],
             ["topic", msg_box.topic_name],
         ]
-        controller.model.narrow = []
-        controller.model.index = index_topic
+        controller.model.narrow = initial_narrow
+        controller.model.index = index_multiple_topic_msg
+        controller.model.stream_id = initial_stream_id
         controller.view.message_view = mocker.patch("urwid.ListBox")
         controller.model.stream_dict = {
             205: {
@@ -143,16 +167,23 @@ class TestController:
         controller.model.muted_streams = []
         controller.model.is_muted_topic = mocker.Mock(return_value=False)
 
-        controller.narrow_to_topic(stream_name="PTEST", topic_name=msg_box.topic_name)
+        controller.narrow_to_topic(
+            stream_name="PTEST",
+            topic_name=msg_box.topic_name,
+            contextual_message_id=anchor,
+        )
 
         assert controller.model.stream_id == msg_box.stream_id
         assert controller.model.narrow == expected_narrow
         controller.view.message_view.log.clear.assert_called_once_with()
 
-        widget = controller.view.message_view.log.extend.call_args_list[0][0][0][0]
+        widgets, focus = controller.view.message_view.log.extend.call_args_list[0][0]
         stream_id, topic_name = msg_box.stream_id, msg_box.topic_name
-        id_list = index_topic["topic_msg_ids"][stream_id][topic_name]
-        assert {widget.original_widget.message["id"]} == id_list
+        id_list = index_multiple_topic_msg["topic_msg_ids"][stream_id][topic_name]
+        msg_ids = {widget.original_widget.message["id"] for widget in widgets}
+        final_focus_msg_id = widgets[focus].original_widget.message["id"]
+        assert msg_ids == id_list
+        assert final_focus_msg_id == expected_final_focus
 
     def test_narrow_to_user(self, mocker, controller, user_button, index_user):
         controller.model.narrow = []
@@ -176,7 +207,18 @@ class TestController:
         id_list = index_user["private_msg_ids_by_user_ids"][recipients]
         assert {widget.original_widget.message["id"]} == id_list
 
-    def test_narrow_to_all_messages(self, mocker, controller, index_all_messages):
+    @pytest.mark.parametrize(
+        "anchor, expected_final_focus_msg_id",
+        [(None, 537288), (537286, 537286), (537288, 537288)],
+    )
+    def test_narrow_to_all_messages(
+        self,
+        mocker,
+        controller,
+        index_all_messages,
+        anchor,
+        expected_final_focus_msg_id,
+    ):
         controller.model.narrow = [["stream", "PTEST"]]
         controller.model.index = index_all_messages
         controller.view.message_view = mocker.patch("urwid.ListBox")
@@ -190,15 +232,17 @@ class TestController:
         controller.model.muted_streams = []
         controller.model.is_muted_topic = mocker.Mock(return_value=False)
 
-        controller.narrow_to_all_messages()  # FIXME: Add id narrowing test
+        controller.narrow_to_all_messages(contextual_message_id=anchor)
 
         assert controller.model.narrow == []
         controller.view.message_view.log.clear.assert_called_once_with()
 
-        widgets = controller.view.message_view.log.extend.call_args_list[0][0][0]
+        widgets, focus = controller.view.message_view.log.extend.call_args_list[0][0]
         id_list = index_all_messages["all_msg_ids"]
         msg_ids = {widget.original_widget.message["id"] for widget in widgets}
+        final_focus_msg_id = widgets[focus].original_widget.message["id"]
         assert msg_ids == id_list
+        assert final_focus_msg_id == expected_final_focus_msg_id
 
     def test_narrow_to_all_pm(self, mocker, controller, index_user):
         controller.model.narrow = []