From 16b9e3e21d7656f86ea480a7ffc6a8dbbc00293b Mon Sep 17 00:00:00 2001 From: Ezio-Sarthak Date: Sun, 1 Aug 2021 14:42:24 +0530 Subject: [PATCH 1/5] refactor: tests: Use `case` for patching test ids of large frequency. --- tests/ui_tools/test_buttons.py | 72 +++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py index 2f4dd753c5..039cf7bc42 100644 --- a/tests/ui_tools/test_buttons.py +++ b/tests/ui_tools/test_buttons.py @@ -439,36 +439,40 @@ def test__decode_message_id( @pytest.mark.parametrize( "link, expected_parsed_link", [ - ( + case( SERVER_URL + "/#narrow/stream/1-Stream-1", ParsedNarrowLink( narrow="stream", stream=DecodedStream(stream_id=1, stream_name=None) ), + id="modern_stream_narrow_link", ), - ( + case( SERVER_URL + "/#narrow/stream/Stream.201", ParsedNarrowLink( narrow="stream", stream=DecodedStream(stream_id=None, stream_name="Stream 1"), ), + id="deprecated_stream_narrow_link", ), - ( + case( SERVER_URL + "/#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", ), - ( + case( SERVER_URL + "/#narrow/stream/1-Stream-1/near/1", ParsedNarrowLink( narrow="stream:near", message_id=1, stream=DecodedStream(stream_id=1, stream_name=None), ), + id="stream_near_narrow_link", ), - ( + case( SERVER_URL + "/#narrow/stream/1-Stream-1/topic/foo/near/1", ParsedNarrowLink( narrow="stream:topic:near", @@ -476,28 +480,34 @@ def test__decode_message_id( message_id=1, 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()), - ( + case( + SERVER_URL + "/#narrow/foo", + ParsedNarrowLink(), + id="invalid_narrow_link_1", + ), + case( + SERVER_URL + "/#narrow/stream/", + ParsedNarrowLink(), + id="invalid_narrow_link_2", + ), + case( + SERVER_URL + "/#narrow/stream/1-Stream-1/topic/", + ParsedNarrowLink(), + id="invalid_narrow_link_3", + ), + case( + SERVER_URL + "/#narrow/stream/1-Stream-1//near/", + ParsedNarrowLink(), + id="invalid_narrow_link_4", + ), + case( SERVER_URL + "/#narrow/stream/1-Stream-1/topic/foo/near/", ParsedNarrowLink(), + id="invalid_narrow_link_5", ), ], - 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 @@ -755,15 +765,16 @@ def test__validate_and_patch_stream_data( @pytest.mark.parametrize( "parsed_link, narrow_to_stream_called, narrow_to_topic_called", [ - ( + case( ParsedNarrowLink( narrow="stream", stream=DecodedStream(stream_id=1, stream_name="Stream 1"), ), True, False, + id="stream_narrow", ), - ( + case( ParsedNarrowLink( narrow="stream:topic", topic_name="Foo", @@ -771,8 +782,9 @@ def test__validate_and_patch_stream_data( ), False, True, + id="topic_narrow", ), - ( + case( ParsedNarrowLink( narrow="stream:near", message_id=1, @@ -780,8 +792,9 @@ def test__validate_and_patch_stream_data( ), True, False, + id="stream_near_narrow", ), - ( + case( ParsedNarrowLink( narrow="stream:topic:near", topic_name="Foo", @@ -790,14 +803,9 @@ def test__validate_and_patch_stream_data( ), False, True, + id="topic_near_narrow", ), ], - ids=[ - "stream_narrow", - "topic_narrow", - "stream_near_narrow", - "topic_near_narrow", - ], ) def test__switch_narrow_to( self, From db872dd467916ff03f014c1a7ad73f23238d6e0e Mon Sep 17 00:00:00 2001 From: Ezio-Sarthak Date: Sun, 1 Aug 2021 10:04:08 +0530 Subject: [PATCH 2/5] refactor: buttons: Use new helpers to patch and validate data. This commit adds the helpers: - `_patch_narrow_link` as a general method to patch validated data. This way, explicit patching of stream/pm data is ensured. - `_validate_stream_data` to validate stream data separately in the general `_validate_narrow_link`. Tests added. --- tests/ui_tools/test_buttons.py | 55 ++++--------------------------- zulipterminal/ui_tools/buttons.py | 35 +++++++++++++------- 2 files changed, 29 insertions(+), 61 deletions(-) diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py index 039cf7bc42..346210580a 100644 --- a/tests/ui_tools/test_buttons.py +++ b/tests/ui_tools/test_buttons.py @@ -676,91 +676,48 @@ def test__validate_narrow_link( @pytest.mark.parametrize( [ "parsed_link", - "is_user_subscribed_to_stream", - "is_valid_stream", "stream_id_from_name_return_value", "expected_parsed_link", - "expected_error", ], [ - ( + case( ParsedNarrowLink( stream=DecodedStream(stream_id=1, stream_name=None) ), # ... - True, - None, None, ParsedNarrowLink( stream=DecodedStream(stream_id=1, stream_name="Stream 1") ), - "", - ), - ( - ParsedNarrowLink( - stream=DecodedStream(stream_id=462, stream_name=None) - ), # ... - False, - None, - None, - ParsedNarrowLink(stream=DecodedStream(stream_id=462, stream_name=None)), - "The stream seems to be either unknown or unsubscribed", + id="stream_data_with_stream_id", ), - ( + case( ParsedNarrowLink( stream=DecodedStream(stream_id=None, stream_name="Stream 1") ), # ... - None, - True, 1, ParsedNarrowLink( stream=DecodedStream(stream_id=1, stream_name="Stream 1") ), - "", - ), - ( - ParsedNarrowLink( - stream=DecodedStream(stream_id=None, stream_name="foo") - ), # ... - None, - False, - None, - ParsedNarrowLink( - stream=DecodedStream(stream_id=None, stream_name="foo") - ), - "The stream seems to be either unknown or unsubscribed", + id="stream_data_with_stream_name", ), ], - ids=[ - "valid_stream_data_with_stream_id", - "invalid_stream_data_with_stream_id", - "valid_stream_data_with_stream_name", - "invalid_stream_data_with_stream_name", - ], ) - def test__validate_and_patch_stream_data( + def test__patch_narrow_link( self, stream_dict: Dict[int, Any], parsed_link: ParsedNarrowLink, - is_user_subscribed_to_stream: Optional[bool], - is_valid_stream: Optional[bool], stream_id_from_name_return_value: Optional[int], expected_parsed_link: ParsedNarrowLink, - expected_error: str, ) -> None: self.controller.model.stream_dict = stream_dict self.controller.model.stream_id_from_name.return_value = ( stream_id_from_name_return_value ) - self.controller.model.is_user_subscribed_to_stream.return_value = ( - is_user_subscribed_to_stream - ) - self.controller.model.is_valid_stream.return_value = is_valid_stream mocked_button = self.message_link_button() - error = mocked_button._validate_and_patch_stream_data(parsed_link) + mocked_button._patch_narrow_link(parsed_link) assert parsed_link == expected_parsed_link - assert error == expected_error @pytest.mark.parametrize( "parsed_link, narrow_to_stream_called, narrow_to_topic_called", diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py index ca51bd2395..a9a8ef1265 100644 --- a/zulipterminal/ui_tools/buttons.py +++ b/zulipterminal/ui_tools/buttons.py @@ -447,10 +447,10 @@ def _parse_narrow_link(cls, link: str) -> ParsedNarrowLink: return parsed_link - def _validate_and_patch_stream_data(self, parsed_link: ParsedNarrowLink) -> str: + def _validate_stream_data(self, parsed_link: ParsedNarrowLink) -> str: """ - Validates stream data and patches the optional value in the nested - DecodedStream dict. + Validates stream data and returns either an empty string for a successful + validation or an appropriate validation error. """ stream_id = parsed_link["stream"]["stream_id"] stream_name = parsed_link["stream"]["stream_name"] @@ -467,14 +467,6 @@ def _validate_and_patch_stream_data(self, parsed_link: ParsedNarrowLink) -> str: # report whether the stream id is invalid instead. return "The stream seems to be either unknown or unsubscribed" - # Patch the optional value. - if not stream_id: - stream_id = cast(int, model.stream_id_from_name(stream_name)) - parsed_link["stream"]["stream_id"] = stream_id - else: - stream_name = cast(str, model.stream_dict[stream_id]["name"]) - parsed_link["stream"]["stream_name"] = stream_name - return "" def _validate_narrow_link(self, parsed_link: ParsedNarrowLink) -> str: @@ -487,7 +479,7 @@ def _validate_narrow_link(self, parsed_link: ParsedNarrowLink) -> str: # Validate stream data. if "stream" in parsed_link: - error = self._validate_and_patch_stream_data(parsed_link) + error = self._validate_stream_data(parsed_link) if error: return error @@ -508,6 +500,24 @@ def _validate_narrow_link(self, parsed_link: ParsedNarrowLink) -> str: return "" + def _patch_narrow_link(self, parsed_link: ParsedNarrowLink) -> None: + """ + Patches the validated narrow link data. + """ + model = self.model + + if "stream" in parsed_link: + stream_id = parsed_link["stream"]["stream_id"] + stream_name = parsed_link["stream"]["stream_name"] + + # Patch the optional value. + if not stream_id: + stream_id = cast(int, model.stream_id_from_name(stream_name)) + parsed_link["stream"]["stream_id"] = stream_id + else: + stream_name = cast(str, model.stream_dict[stream_id]["name"]) + parsed_link["stream"]["stream_name"] = stream_name + def _switch_narrow_to(self, parsed_link: ParsedNarrowLink) -> None: """ Switches narrow via narrow_to_* methods. @@ -545,6 +555,7 @@ def handle_narrow_link(self) -> None: if error: self.controller.report_error(f" {error}") else: + self._patch_narrow_link(parsed_link) self._switch_narrow_to(parsed_link) # Exit pop-up if MessageLinkButton exists in one. From 21fa4e431ba838084e26c47341ee52d95e69b983 Mon Sep 17 00:00:00 2001 From: Ezio-Sarthak Date: Thu, 15 Jul 2021 19:00:35 +0530 Subject: [PATCH 3/5] buttons: Add method to decode PM data from MessageLinkButton. This commit adds a static method that decodes necessary information from a PM narrow message link, i.e., the list of PM recipient ids. TypedDict `DecodedPM` is added to return the required structured format of PM data. Currently, there are a bunch of exposed url formats supported by the Zulip server, such as (some seem ambigous too?): narrow/pm-with/1,2-pm narrow/pm-with/1,2-group narrow/pm-with/1,2,3-pm narrow/pm-with/1,2,3-group narrow/pm-with/1-user1 narrow/pm-with/1-bot-name (near parameter supported for each) Tests added. --- tests/ui_tools/test_buttons.py | 53 +++++++++++++++++++++++++++++++ zulipterminal/ui_tools/buttons.py | 27 ++++++++++++++-- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py index 346210580a..a50f68e22e 100644 --- a/tests/ui_tools/test_buttons.py +++ b/tests/ui_tools/test_buttons.py @@ -7,6 +7,7 @@ from zulipterminal.config.keys import keys_for_command from zulipterminal.ui_tools.buttons import ( + DecodedPM, DecodedStream, MessageLinkButton, ParsedNarrowLink, @@ -422,6 +423,58 @@ def test__decode_stream_data( assert return_value == expected_response + @pytest.mark.parametrize( + "pm_data, expected_response", + [ + case( + "1001,12-pm", + dict(type=None, recipient_ids=[1001, 12], recipient_emails=None), + id="pm_with_two_recipients", + ), + case( + "1001,12-group", + dict(type=None, recipient_ids=[1001, 12], recipient_emails=None), + id="group_pm_with_two_recipients", + ), + case( + "1001,11,12-pm", + dict(type=None, recipient_ids=[1001, 11, 12], recipient_emails=None), + id="pm_with_more_than_two_recipients", + ), + case( + "1001,11,12-group", + dict(type=None, recipient_ids=[1001, 11, 12], recipient_emails=None), + id="group_pm_with_more_than_two_recipients", + ), + case( + "11-user11", + dict(type=None, recipient_ids=[11], recipient_emails=None), + id="pm_exposed_format_1_ordinary", + ), + case( + "11-user2", + dict(type=None, recipient_ids=[11], recipient_emails=None), + id="pm_exposed_format_1_ambigous", + ), + case( + "5-bot-name", + dict(type=None, recipient_ids=[5], recipient_emails=None), + id="pm_with_bot_exposed_format_1_ordinary", + ), + case( + "5-bot;name", + dict(type=None, recipient_ids=[5], recipient_emails=None), + id="pm_with_bot_exposed_format_1_ambigous", + ), + ], + ) + def test__decode_pm_data( + self, mocker: MockerFixture, pm_data: str, expected_response: DecodedPM + ) -> None: + return_value = MessageLinkButton._decode_pm_data(pm_data) + + assert return_value == expected_response + @pytest.mark.parametrize( "message_id, expected_return_value", [ diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py index a9a8ef1265..c87f47f6c8 100644 --- a/zulipterminal/ui_tools/buttons.py +++ b/zulipterminal/ui_tools/buttons.py @@ -1,10 +1,10 @@ import re from functools import partial -from typing import Any, Callable, Dict, Optional, Tuple, Union, cast +from typing import Any, Callable, Dict, List, Optional, Tuple, Union, cast from urllib.parse import urljoin, urlparse import urwid -from typing_extensions import TypedDict +from typing_extensions import Literal, TypedDict from zulipterminal.api_types import EditPropagateMode from zulipterminal.config.keys import is_command_key, primary_key_for_command @@ -327,9 +327,16 @@ class DecodedStream(TypedDict): stream_name: Optional[str] +class DecodedPM(TypedDict): + type: Optional[Literal["pm", "group"]] + recipient_ids: List[int] + recipient_emails: Optional[List[str]] + + class ParsedNarrowLink(TypedDict, total=False): narrow: str stream: DecodedStream + pm_with: DecodedPM topic_name: str message_id: Optional[int] @@ -380,6 +387,16 @@ def _decode_stream_data(encoded_stream_data: str) -> DecodedStream: stream_name = hash_util_decode(encoded_stream_data) return DecodedStream(stream_id=None, stream_name=stream_name) + @staticmethod + def _decode_pm_data(encoded_pm_data: str) -> DecodedPM: + """ + Returns a dict with PM type and IDs of PM recipients. + """ + recipient_data, *_ = encoded_pm_data.split("-") + recipient_ids = list(map(int, recipient_data.split(","))) + + return DecodedPM(type=None, recipient_ids=recipient_ids, recipient_emails=None) + @staticmethod def _decode_message_id(message_id: str) -> Optional[int]: """ @@ -405,6 +422,12 @@ def _parse_narrow_link(cls, link: str) -> ParsedNarrowLink: # {encoded.20topic.20name} # d. narrow/stream/[{stream_id}-]{stream-name}/topic/ # {encoded.20topic.20name}/near/{message_id} + # e. narrow/pm-with/[{recipient_ids},]-{pm-type} + # f. narrow/pm-with/[{recipient_ids},]-{pm-type}/near/{message_id} + # g. narrow/pm-with/{user_id}-user{user_id} + # h. narrow/pm-with/{user_id}-user{user_id}/near/{message_id} + # i. narrow/pm-with/{bot_id}-{bot-name} + # j. narrow/pm-with/{bot_id}-{bot-name}/near/{message_id} fragments = urlparse(link.rstrip("/")).fragment.split("/") len_fragments = len(fragments) parsed_link = ParsedNarrowLink() From 7473a42b427458e20e47ca4ac499e17ec2a51a56 Mon Sep 17 00:00:00 2001 From: Ezio-Sarthak Date: Thu, 15 Jul 2021 20:11:43 +0530 Subject: [PATCH 4/5] buttons: Add support for parsing, detecting and narrowing PM links. This commit adds conditional checks in `_parse_narrow_link` to detect PM narrow links. On successful detection, the narrow is switched to the respective PM message via `_switch_narrow_to`. NOTE: A PM narrow can be either "all PMs" (not near) or a "specific PM conversation" (near). Tests added and amended. --- tests/ui_tools/test_buttons.py | 200 +++++++++++++++++++++++++++++- zulipterminal/ui_tools/buttons.py | 46 +++++++ 2 files changed, 244 insertions(+), 2 deletions(-) diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py index a50f68e22e..22988d24a8 100644 --- a/tests/ui_tools/test_buttons.py +++ b/tests/ui_tools/test_buttons.py @@ -3,6 +3,7 @@ import pytest from pytest import param as case from pytest_mock import MockerFixture +from typing_extensions import Literal from urwid import AttrMap, Overlay, Widget from zulipterminal.config.keys import keys_for_command @@ -324,11 +325,18 @@ def test_keypress_EXIT_TOGGLE_TOPIC( class TestMessageLinkButton: @pytest.fixture(autouse=True) - def mock_external_classes(self, mocker: MockerFixture) -> None: + def mock_external_classes( + self, mocker: MockerFixture, _all_users_by_id: Dict[str, Any] + ) -> None: self.controller = mocker.Mock() self.super_init = mocker.patch(MODULE + ".urwid.Button.__init__") self.connect_signal = mocker.patch(MODULE + ".urwid.connect_signal") + self.model = mocker.Mock() # To pass model data on potential calls. + self.model._all_users_by_id = _all_users_by_id + self.model.user_id = 1001 # User id of current logged in user + self.controller.model = self.model + def message_link_button( self, caption: str = "", link: str = "", display_attr: Optional[str] = None ) -> MessageLinkButton: @@ -535,6 +543,37 @@ def test__decode_message_id( ), id="topic_near_narrow_link", ), + case( + SERVER_URL + "/#narrow/pm-with/1001,12-pm", + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 12], recipient_emails=None + ), + ), + id="pm_narrow_link", + ), + case( + SERVER_URL + "/#narrow/pm-with/1001,12-group", + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 12], recipient_emails=None + ), + ), + id="group_pm_narrow_link", + ), + case( + SERVER_URL + "/#narrow/pm-with/1001,12-pm/near/1", + ParsedNarrowLink( + narrow="pm-with:near", + message_id=1, + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 12], recipient_emails=None + ), + ), + id="common_pm_near_narrow_link", + ), case( SERVER_URL + "/#narrow/foo", ParsedNarrowLink(), @@ -753,6 +792,86 @@ def test__validate_narrow_link( ), id="stream_data_with_stream_name", ), + case( + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 11], recipient_emails=None + ), + ), + None, + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=Literal["pm"], + recipient_ids=[1001, 11], + recipient_emails=["FOOBOO@gmail.com", "person1@example.com"], + ), + ), + id="normal_pm", + ), + case( + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 11, 12], recipient_emails=None + ), + ), + None, + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=Literal["group"], + recipient_ids=[1001, 11, 12], + recipient_emails=[ + "FOOBOO@gmail.com", + "person1@example.com", + "person2@example.com", + ], + ), + ), + id="group_pm", + ), + case( + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=None, recipient_ids=[11], recipient_emails=None + ), + ), + None, + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=Literal["pm"], + recipient_ids=[11, 1001], + recipient_emails=["person1@example.com", "FOOBOO@gmail.com"], + ), + ), + id="pm_with_user_id_not_specified", + ), + case( + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=None, recipient_ids=[11, 12], recipient_emails=None + ), + ), + None, + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=Literal["group"], + recipient_ids=[11, 12, 1001], + recipient_emails=[ + "person1@example.com", + "person2@example.com", + "FOOBOO@gmail.com", + ], + ), + ), + id="group_pm_with_user_id_not_specified", + ), ], ) def test__patch_narrow_link( @@ -773,7 +892,12 @@ def test__patch_narrow_link( assert parsed_link == expected_parsed_link @pytest.mark.parametrize( - "parsed_link, narrow_to_stream_called, narrow_to_topic_called", + [ + "parsed_link", + "narrow_to_stream_called", + "narrow_to_topic_called", + "narrow_to_user_called", + ], [ case( ParsedNarrowLink( @@ -782,6 +906,7 @@ def test__patch_narrow_link( ), True, False, + False, id="stream_narrow", ), case( @@ -792,6 +917,7 @@ def test__patch_narrow_link( ), False, True, + False, id="topic_narrow", ), case( @@ -802,6 +928,7 @@ def test__patch_narrow_link( ), True, False, + False, id="stream_near_narrow", ), case( @@ -813,8 +940,75 @@ def test__patch_narrow_link( ), False, True, + False, id="topic_near_narrow", ), + case( + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=Literal["pm"], + recipient_ids=[1001, 11], + recipient_emails=["FOOBOO@gmail.com", "person1@example.com"], + ), + ), + False, + False, + True, + id="pm_narrow", + ), + case( + ParsedNarrowLink( + narrow="pm-with", + pm_with=DecodedPM( + type=Literal["group"], + recipient_ids=[1001, 11, 12], + recipient_emails=[ + "FOOBOO@gmail.com", + "person1@example.com", + "person2@example.com", + ], + ), + ), + False, + False, + True, + id="group_pm_narrow", + ), + case( + ParsedNarrowLink( + narrow="pm-with:near", + message_id=1, + pm_with=DecodedPM( + type=Literal["pm"], + recipient_ids=[1001, 11], + recipient_emails=["FOOBOO@gmail.com", "person1@example.com"], + ), + ), + False, + False, + True, + id="pm_near_narrow", + ), + case( + ParsedNarrowLink( + narrow="pm-with:near", + message_id=1, + pm_with=DecodedPM( + type=Literal["group"], + recipient_ids=[1001, 11, 12], + recipient_emails=[ + "FOOBOO@gmail.com", + "person1@example.com", + "person2@example.com", + ], + ), + ), + False, + False, + True, + id="group_pm_near_narrow", + ), ], ) def test__switch_narrow_to( @@ -822,6 +1016,7 @@ def test__switch_narrow_to( parsed_link: ParsedNarrowLink, narrow_to_stream_called: bool, narrow_to_topic_called: bool, + narrow_to_user_called: bool, ) -> None: mocked_button = self.message_link_button() @@ -831,6 +1026,7 @@ def test__switch_narrow_to( mocked_button.controller.narrow_to_stream.called == narrow_to_stream_called ) assert mocked_button.controller.narrow_to_topic.called == narrow_to_topic_called + assert mocked_button.controller.narrow_to_user.called == narrow_to_user_called @pytest.mark.parametrize( "error, report_error_called, _switch_narrow_to_called, exit_popup_called", diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py index c87f47f6c8..726cf37259 100644 --- a/zulipterminal/ui_tools/buttons.py +++ b/zulipterminal/ui_tools/buttons.py @@ -468,6 +468,19 @@ def _parse_narrow_link(cls, link: str) -> ParsedNarrowLink: message_id=message_id, ) + elif ( + len_fragments == 5 and fragments[1] == "pm-with" and fragments[3] == "near" + ): + pm_data = cls._decode_pm_data(fragments[2]) + message_id = cls._decode_message_id(fragments[4]) + parsed_link = dict( + narrow="pm-with:near", pm_with=pm_data, message_id=message_id + ) + + elif len_fragments == 3 and fragments[1] == "pm-with": + pm_data = cls._decode_pm_data(fragments[2]) + parsed_link = dict(narrow="pm-with", pm_with=pm_data) + return parsed_link def _validate_stream_data(self, parsed_link: ParsedNarrowLink) -> str: @@ -541,6 +554,29 @@ def _patch_narrow_link(self, parsed_link: ParsedNarrowLink) -> None: stream_name = cast(str, model.stream_dict[stream_id]["name"]) parsed_link["stream"]["stream_name"] = stream_name + elif "pm_with" in parsed_link: + user_id = model.user_id + recipient_ids = parsed_link["pm_with"]["recipient_ids"] + + # Bump recipients to include current user_id if not already + # present (refer to URL formats in `_parse_narrow_link`) + if user_id not in recipient_ids: + recipient_ids.append(user_id) + + recipient_emails: List[str] = [] + for recipient_id in recipient_ids: + user = model._all_users_by_id.get(recipient_id, None) + email = user["email"] if user else "" + recipient_emails.append(email) + + # Currently webapp uses `pm` and `group` suffix interchangeably. + # Treat more-than-2 pms to group pms to avoid confusion. + pm_type = "pm" if len(recipient_ids) < 3 else "group" + + # Patch the PM type and recepient emails + parsed_link["pm_with"]["type"] = Literal[pm_type] + parsed_link["pm_with"]["recipient_emails"] = recipient_emails + def _switch_narrow_to(self, parsed_link: ParsedNarrowLink) -> None: """ Switches narrow via narrow_to_* methods. @@ -566,6 +602,16 @@ def _switch_narrow_to(self, parsed_link: ParsedNarrowLink) -> None: topic_name=parsed_link["topic_name"], contextual_message_id=parsed_link["message_id"], ) + elif "pm-with:near" == narrow: + self.controller.narrow_to_user( + recipient_emails=parsed_link["pm_with"]["recipient_emails"], + contextual_message_id=parsed_link["message_id"], + ) + + elif "pm-with" == narrow: + self.controller.narrow_to_user( + recipient_emails=parsed_link["pm_with"]["recipient_emails"] + ) def handle_narrow_link(self) -> None: """ From b8fb00f0afbe9840a8a2be9c0247f19d385b8ab9 Mon Sep 17 00:00:00 2001 From: Ezio-Sarthak Date: Sun, 1 Aug 2021 11:32:19 +0530 Subject: [PATCH 5/5] refactor: buttons: Validate PM data prior to narrowing. This commit adds validation helper for PM data, in an attempt to avoid and handle any potential exceptions before starting to switch to the PM narrow. The validation checks if all the recipients IDs are valid or not. Tests added and amended. --- tests/ui_tools/test_buttons.py | 105 ++++++++++++++++++++++++++++++ zulipterminal/ui_tools/buttons.py | 23 +++++++ 2 files changed, 128 insertions(+) diff --git a/tests/ui_tools/test_buttons.py b/tests/ui_tools/test_buttons.py index 22988d24a8..7ce5ebcfdf 100644 --- a/tests/ui_tools/test_buttons.py +++ b/tests/ui_tools/test_buttons.py @@ -612,6 +612,7 @@ def test__parse_narrow_link( [ "parsed_link", "is_user_subscribed_to_stream", + "is_valid_private_recipient", "is_valid_stream", "topics_in_stream", "expected_error", @@ -624,6 +625,7 @@ def test__parse_narrow_link( True, None, None, + None, "", id="valid_modern_stream_narrow_parsed_link", ), @@ -635,6 +637,7 @@ def test__parse_narrow_link( False, None, None, + None, "The stream seems to be either unknown or unsubscribed", id="invalid_modern_stream_narrow_parsed_link", ), @@ -644,6 +647,7 @@ def test__parse_narrow_link( stream=DecodedStream(stream_id=None, stream_name="Stream 1"), ), None, + None, True, None, "", @@ -655,6 +659,7 @@ def test__parse_narrow_link( stream=DecodedStream(stream_id=None, stream_name="foo"), ), None, + None, False, None, "The stream seems to be either unknown or unsubscribed", @@ -668,6 +673,7 @@ def test__parse_narrow_link( ), True, None, + None, ["Valid"], "", id="valid_topic_narrow_parsed_link", @@ -680,6 +686,7 @@ def test__parse_narrow_link( ), True, None, + None, [], "Invalid topic name", id="invalid_topic_narrow_parsed_link", @@ -693,6 +700,7 @@ def test__parse_narrow_link( True, None, None, + None, "", id="valid_stream_near_narrow_parsed_link", ), @@ -705,6 +713,7 @@ def test__parse_narrow_link( True, None, None, + None, "Invalid message ID", id="invalid_stream_near_narrow_parsed_link", ), @@ -717,6 +726,7 @@ def test__parse_narrow_link( ), True, None, + None, ["Valid"], "", id="valid_topic_near_narrow_parsed_link", @@ -730,6 +740,7 @@ def test__parse_narrow_link( ), True, None, + None, ["Valid"], "Invalid message ID", id="invalid_topic_near_narrow_parsed_link", @@ -739,9 +750,99 @@ def test__parse_narrow_link( None, None, None, + None, "The narrow link seems to be either broken or unsupported", id="invalid_narrow_link", ), + case( + ParsedNarrowLink( + narrow="stream", stream=DecodedStream(stream_id=1, stream_name=None) + ), # ... + True, + None, + None, + None, + "", + id="valid_stream_data_with_stream_id", + ), + case( + ParsedNarrowLink( + narrow="stream", + stream=DecodedStream(stream_id=462, stream_name=None), + ), # ... + False, + None, + None, + None, + "The stream seems to be either unknown or unsubscribed", + id="invalid_stream_data_with_stream_id", + ), + case( + ParsedNarrowLink( + narrow="stream", + stream=DecodedStream(stream_id=None, stream_name="Stream 1"), + ), # ... + None, + None, + True, + None, + "", + id="valid_stream_data_with_stream_name", + ), + case( + ParsedNarrowLink( + narrow="stream", + stream=DecodedStream(stream_id=None, stream_name="foo"), + ), # ... + None, + None, + False, + None, + "The stream seems to be either unknown or unsubscribed", + id="invalid_stream_data_with_stream_name", + ), + case( + ParsedNarrowLink( + narrow="pm_with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 11], recipient_emails=None + ), + ), # ... + None, + True, + None, + None, + "", + id="valid_pm_data", + ), + case( + ParsedNarrowLink( + narrow="pm_with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, 11, 12], recipient_emails=None + ), + ), # ... + None, + True, + None, + None, + "", + id="valid_group_pm_data", + ), + case( + ParsedNarrowLink( + narrow="pm_with", + pm_with=DecodedPM( + type=None, recipient_ids=[1001, -1], recipient_emails=None + ), + ), # ... + None, + False, + None, + None, + "The PM has one or more invalid recipient(s)", + id="invalid_recipient", + ), ], ) def test__validate_narrow_link( @@ -749,6 +850,7 @@ def test__validate_narrow_link( stream_dict: Dict[int, Any], parsed_link: ParsedNarrowLink, is_user_subscribed_to_stream: Optional[bool], + is_valid_private_recipient: Optional[bool], is_valid_stream: Optional[bool], topics_in_stream: Optional[List[str]], expected_error: str, @@ -757,6 +859,9 @@ def test__validate_narrow_link( self.controller.model.is_user_subscribed_to_stream.return_value = ( is_user_subscribed_to_stream ) + self.controller.model.is_valid_private_recipient.return_value = ( + is_valid_private_recipient + ) self.controller.model.is_valid_stream.return_value = is_valid_stream self.controller.model.topics_in_stream.return_value = topics_in_stream mocked_button = self.message_link_button() diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py index 726cf37259..f99e64a020 100644 --- a/zulipterminal/ui_tools/buttons.py +++ b/zulipterminal/ui_tools/buttons.py @@ -505,6 +505,23 @@ def _validate_stream_data(self, parsed_link: ParsedNarrowLink) -> str: return "" + def _validate_pm_data(self, parsed_link: ParsedNarrowLink) -> str: + """ + Validates PM data and returns either an empty string for a successful + validation or an appropriate validation error. + """ + recipient_ids = parsed_link["pm_with"]["recipient_ids"] + + for recipient_id in recipient_ids: + user = self.model._all_users_by_id.get(recipient_id, None) + email = user["email"] if user else "" + name = user["full_name"] if user else "" + + if not self.model.is_valid_private_recipient(email, name): + return "The PM has one or more invalid recipient(s)" + + return "" + def _validate_narrow_link(self, parsed_link: ParsedNarrowLink) -> str: """ Returns either an empty string for a successful validation or an @@ -519,6 +536,12 @@ def _validate_narrow_link(self, parsed_link: ParsedNarrowLink) -> str: if error: return error + # Validate PM data. + if "pm_with" in parsed_link: + error = self._validate_pm_data(parsed_link) + if error: + return error + # Validate topic name. if "topic_name" in parsed_link: topic_name = parsed_link["topic_name"]