Repository: zulip/zulip-terminal

Issue:
Support old format for narrow links (subject in addition to topic)
As documented in buttons.py (currently `_parse_narrow_link`), we support links formatted in the form:
```
... narrow/stream/[{stream-id}-]{stream-name}/topic/{encoded.20.topic.20name}[/near/{message_id}]
```

As I noted in [**#api documentation > Narrow URL formats**](https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/Narrow.20URL.20formats) this doesn't handle an old format where `subject` was used in place of `topic`, which makes the URL in the quoted message fail to be identified and link properly in ZT.

Servers running newer versions of Zulip will not generate links of this format, which limits the impact of this issue, but equally anyone who has upgraded an older server will potentially still have messages with links of this form.

Discussion:
neiljp: @mounilKshah Thanks for the refactor :+1: Adjusted very slightly and added in a small followup commit for message vs stream id distinction I referred to.

Patch:
diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py
index fe21169..3fd8b97 100644
--- a/zulipterminal/ui_tools/buttons.py
+++ b/zulipterminal/ui_tools/buttons.py
@@ -493,12 +493,17 @@ class MessageLinkButton(urwid.Button):
         """
         # NOTE: The optional stream_id link version is deprecated. The extended
         # support is for old messages.
+        # NOTE: Support for narrow links with subject instead of topic is also added
         # We expect the fragment to be one of the following types:
         # a. narrow/stream/[{stream_id}-]{stream-name}
         # b. narrow/stream/[{stream_id}-]{stream-name}/near/{message_id}
         # c. narrow/stream/[{stream_id}-]{stream-name}/topic/
         #    {encoded.20topic.20name}
-        # d. narrow/stream/[{stream_id}-]{stream-name}/topic/
+        # d. narrow/stream/[{stream_id}-]{stream-name}/subject/
+        #    {encoded.20topic.20name}
+        # e. narrow/stream/[{stream_id}-]{stream-name}/topic/
+        #    {encoded.20topic.20name}/near/{message_id}
+        # f. narrow/stream/[{stream_id}-]{stream-name}/subject/
         #    {encoded.20topic.20name}/near/{message_id}
         fragments = urlparse(link.rstrip("/")).fragment.split("/")
         len_fragments = len(fragments)
@@ -509,7 +514,9 @@ class MessageLinkButton(urwid.Button):
             parsed_link = dict(narrow="stream", stream=stream_data)
 
         elif (
-            len_fragments == 5 and fragments[1] == "stream" and fragments[3] == "topic"
+            len_fragments == 5
+            and fragments[1] == "stream"
+            and (fragments[3] == "topic" or fragments[3] == "subject")
         ):
             stream_data = cls._decode_stream_data(fragments[2])
             topic_name = hash_util_decode(fragments[4])
@@ -527,7 +534,7 @@ class MessageLinkButton(urwid.Button):
         elif (
             len_fragments == 7
             and fragments[1] == "stream"
-            and fragments[3] == "topic"
+            and (fragments[3] == "topic" or fragments[3] == "subject")
             and fragments[5] == "near"
         ):
             stream_data = cls._decode_stream_data(fragments[2])


Test patch:
diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py
index b29e88a..3705bba 100644
--- a/tests/ui_tools/test_buttons.py
+++ b/tests/ui_tools/test_buttons.py
@@ -653,70 +653,109 @@ class TestMessageLinkButton:
     @pytest.mark.parametrize(
         "link, expected_parsed_link",
         [
-            (
-                SERVER_URL + "/#narrow/stream/1-Stream-1",
+            case(
+                "/#narrow/stream/1-Stream-1",
                 ParsedNarrowLink(
                     narrow="stream", stream=DecodedStream(stream_id=1, stream_name=None)
                 ),
+                id="modern_stream_narrow_link",
             ),
-            (
-                SERVER_URL + "/#narrow/stream/Stream.201",
+            case(
+                "/#narrow/stream/Stream.201",
                 ParsedNarrowLink(
                     narrow="stream",
                     stream=DecodedStream(stream_id=None, stream_name="Stream 1"),
                 ),
+                id="deprecated_stream_narrow_link",
             ),
-            (
-                SERVER_URL + "/#narrow/stream/1-Stream-1/topic/foo.20bar",
+            case(
+                "/#narrow/stream/1-Stream-1/topic/foo.20bar",
                 ParsedNarrowLink(
                     narrow="stream:topic",
                     topic_name="foo bar",
                     stream=DecodedStream(stream_id=1, stream_name=None),
                 ),
+                id="topic_narrow_link",
             ),
-            (
-                SERVER_URL + "/#narrow/stream/1-Stream-1/near/1",
+            case(
+                "/#narrow/stream/1-Stream-1/subject/foo.20bar",
+                ParsedNarrowLink(
+                    narrow="stream:topic",
+                    topic_name="foo bar",
+                    stream=DecodedStream(stream_id=1, stream_name=None),
+                ),
+                id="subject_narrow_link",
+            ),
+            case(
+                "/#narrow/stream/1-Stream-1/near/987",
                 ParsedNarrowLink(
                     narrow="stream:near",
-                    message_id=1,
+                    message_id=987,
                     stream=DecodedStream(stream_id=1, stream_name=None),
                 ),
+                id="stream_near_narrow_link",
             ),
-            (
-                SERVER_URL + "/#narrow/stream/1-Stream-1/topic/foo/near/1",
+            case(
+                "/#narrow/stream/1-Stream-1/topic/foo/near/789",
                 ParsedNarrowLink(
                     narrow="stream:topic:near",
                     topic_name="foo",
-                    message_id=1,
+                    message_id=789,
                     stream=DecodedStream(stream_id=1, stream_name=None),
                 ),
+                id="topic_near_narrow_link",
             ),
-            (SERVER_URL + "/#narrow/foo", ParsedNarrowLink()),
-            (SERVER_URL + "/#narrow/stream/", ParsedNarrowLink()),
-            (SERVER_URL + "/#narrow/stream/1-Stream-1/topic/", ParsedNarrowLink()),
-            (SERVER_URL + "/#narrow/stream/1-Stream-1//near/", ParsedNarrowLink()),
-            (
-                SERVER_URL + "/#narrow/stream/1-Stream-1/topic/foo/near/",
+            case(
+                "/#narrow/stream/1-Stream-1/subject/foo/near/654",
+                ParsedNarrowLink(
+                    narrow="stream:topic:near",
+                    topic_name="foo",
+                    message_id=654,
+                    stream=DecodedStream(stream_id=1, stream_name=None),
+                ),
+                id="subject_near_narrow_link",
+            ),
+            case(
+                "/#narrow/foo",
                 ParsedNarrowLink(),
+                id="invalid_narrow_link_1",
+            ),
+            case(
+                "/#narrow/stream/",
+                ParsedNarrowLink(),
+                id="invalid_narrow_link_2",
+            ),
+            case(
+                "/#narrow/stream/1-Stream-1/topic/",
+                ParsedNarrowLink(),
+                id="invalid_narrow_link_3",
+            ),
+            case(
+                "/#narrow/stream/1-Stream-1/subject/",
+                ParsedNarrowLink(),
+                id="invalid_narrow_link_4",
+            ),
+            case(
+                "/#narrow/stream/1-Stream-1//near/",
+                ParsedNarrowLink(),
+                id="invalid_narrow_link_5",
+            ),
+            case(
+                "/#narrow/stream/1-Stream-1/topic/foo/near/",
+                ParsedNarrowLink(),
+                id="invalid_narrow_link_6",
+            ),
+            case(
+                "/#narrow/stream/1-Stream-1/subject/foo/near/",
+                ParsedNarrowLink(),
+                id="invalid_narrow_link_7",
             ),
-        ],
-        ids=[
-            "modern_stream_narrow_link",
-            "deprecated_stream_narrow_link",
-            "topic_narrow_link",
-            "stream_near_narrow_link",
-            "topic_near_narrow_link",
-            "invalid_narrow_link_1",
-            "invalid_narrow_link_2",
-            "invalid_narrow_link_3",
-            "invalid_narrow_link_4",
-            "invalid_narrow_link_5",
         ],
     )
     def test__parse_narrow_link(
         self, link: str, expected_parsed_link: ParsedNarrowLink
     ) -> None:
-        return_value = MessageLinkButton._parse_narrow_link(link)
+        return_value = MessageLinkButton._parse_narrow_link(SERVER_URL + link)
 
         assert return_value == expected_parsed_link