From b7e183089e715d97ac552926b2ef198e6d46a059 Mon Sep 17 00:00:00 2001 From: Preet Mishra Date: Sat, 1 Aug 2020 11:40:56 +0530 Subject: [PATCH 1/5] model: Decouple stream muting from is_muted_topic method. This decouples stream muting from is_muted_topic to allow independent muted_topics lookups. Additionally, this adds a docstring to clarify the method further. The method had been here for a while without any use-case. Besides, the current behaviour can easily be achieved by using is_muted_topic with is_muted_stream at any point. Test amended. --- tests/model/test_model.py | 9 +++++---- zulipterminal/model.py | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/model/test_model.py b/tests/model/test_model.py index 9684bf6bef..ead6f8e310 100644 --- a/tests/model/test_model.py +++ b/tests/model/test_model.py @@ -1578,17 +1578,18 @@ def test_is_muted_stream(self, muted_streams, stream_id, is_muted, assert model.is_muted_stream(stream_id) == is_muted @pytest.mark.parametrize('topic, is_muted', [ - ((1, 'stream muted & unmuted topic'), True), + ((1, 'stream muted & unmuted topic'), False), ((2, 'muted topic'), True), ((1, 'muted stream muted topic'), True), ((2, 'unmuted topic'), False), ]) def test_is_muted_topic(self, topic, is_muted, stream_dict, model): model.stream_dict = stream_dict - model.muted_streams = [1] model.muted_topics = [ ['Stream 2', 'muted topic'], ['Stream 1', 'muted stream muted topic'], ] - assert model.is_muted_topic(stream_id=topic[0], - topic=topic[1]) == is_muted + + return_value = model.is_muted_topic(stream_id=topic[0], topic=topic[1]) + + assert return_value == is_muted diff --git a/zulipterminal/model.py b/zulipterminal/model.py index 8e8e34b436..a3a97edcb6 100644 --- a/zulipterminal/model.py +++ b/zulipterminal/model.py @@ -411,8 +411,9 @@ def is_muted_stream(self, stream_id: int) -> bool: return stream_id in self.muted_streams def is_muted_topic(self, stream_id: int, topic: str) -> bool: - if stream_id in self.muted_streams: - return True + """ + Returns True if topic is muted via muted_topics. + """ stream_name = self.stream_dict[stream_id]['name'] topic_to_search = [stream_name, topic] # type: List[str] return topic_to_search in self.muted_topics From 9d889ea35dd3b7df5830f09dfef6ae3853ba3092 Mon Sep 17 00:00:00 2001 From: Preet Mishra Date: Sat, 1 Aug 2020 14:59:40 +0530 Subject: [PATCH 2/5] refactor: buttons/helper/utils: Use is_muted_topic for lookups. Tests amended with simplified parametrize blocks wherever feasible. --- tests/core/test_core.py | 12 +++++----- tests/helper/test_helper.py | 5 ++++- tests/ui/test_ui_tools.py | 23 ++++++++++++------- tests/ui/test_utils.py | 37 +++++++++---------------------- zulipterminal/helper.py | 6 ++--- zulipterminal/ui_tools/buttons.py | 3 +-- zulipterminal/ui_tools/utils.py | 2 +- 7 files changed, 40 insertions(+), 48 deletions(-) diff --git a/tests/core/test_core.py b/tests/core/test_core.py index 6777a7421d..4404983026 100644 --- a/tests/core/test_core.py +++ b/tests/core/test_core.py @@ -85,7 +85,7 @@ def test_narrow_to_stream(self, mocker, controller, } } controller.model.muted_streams = [] - controller.model.muted_topics = [] + controller.model.is_muted_topic = mocker.Mock(return_value=False) controller.narrow_to_stream(stream_button) @@ -113,7 +113,7 @@ def test_narrow_to_topic(self, mocker, controller, } } controller.model.muted_streams = [] - controller.model.muted_topics = [] + controller.model.is_muted_topic = mocker.Mock(return_value=False) controller.narrow_to_topic(msg_box) assert controller.model.stream_id == msg_box.stream_id assert controller.model.narrow == expected_narrow @@ -158,7 +158,7 @@ def test_show_all_messages(self, mocker, controller, index_all_messages): } } controller.model.muted_streams = [] - controller.model.muted_topics = [] + controller.model.is_muted_topic = mocker.Mock(return_value=False) controller.show_all_messages('') @@ -194,7 +194,8 @@ def test_show_all_starred(self, mocker, controller, index_all_starred): controller.model.index = index_all_starred controller.model.muted_streams = set() # FIXME Expand upon this controller.model.user_id = 1 - controller.model.muted_topics = [] # FIXME Expand upon this + # FIXME: Expand upon is_muted_topic(). + controller.model.is_muted_topic = mocker.Mock(return_value=False) controller.model.user_email = "some@email" controller.model.stream_dict = { 205: { @@ -218,7 +219,8 @@ def test_show_all_mentions(self, mocker, controller, index_all_mentions): controller.model.narrow = [] controller.model.index = index_all_mentions controller.model.muted_streams = set() # FIXME Expand upon this - controller.model.muted_topics = [] # FIXME Expand upon this + # FIXME: Expand upon is_muted_topic(). + controller.model.is_muted_topic = mocker.Mock(return_value=False) controller.model.user_email = "some@email" controller.model.user_id = 1 controller.model.stream_dict = { diff --git a/tests/helper/test_helper.py b/tests/helper/test_helper.py index 05bb0cbc5a..d5888a6e8a 100644 --- a/tests/helper/test_helper.py +++ b/tests/helper/test_helper.py @@ -212,7 +212,10 @@ def test_classify_unread_counts(mocker, initial_data, stream_dict, model = mocker.Mock() model.stream_dict = stream_dict model.initial_data = initial_data - model.muted_topics = muted_topics + model.is_muted_topic = mocker.Mock(side_effect=( + lambda stream_id, topic: + [model.stream_dict[stream_id]['name'], topic] in muted_topics + )) model.muted_streams = muted_streams assert classify_unread_counts(model) == dict(classified_unread_counts, **vary_in_unreads) diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py index 711ce5da17..ac55a23666 100644 --- a/tests/ui/test_ui_tools.py +++ b/tests/ui/test_ui_tools.py @@ -629,7 +629,9 @@ def test_update_topics_list(self, mocker, topic_view, topic_name, topic_view.view.controller.model.stream_dict = { 86: {'name': 'PTEST'} } - topic_view.view.controller.model.muted_topics = [] + topic_view.view.controller.model.is_muted_topic = ( + mocker.Mock(return_value=False) + ) topic_view.log = [mocker.Mock(topic_name=topic_name) for topic_name in topic_initial_log] @@ -2660,7 +2662,7 @@ def test_init_calls_top_button(self, mocker, width, count, title, 86: {'name': 'Django'}, 14: {'name': 'GSoC'}, } - controller.model.muted_topics = [] + controller.model.is_muted_topic = mocker.Mock(return_value=False) top_button = mocker.patch(TOPBUTTON + '.__init__') params = dict(controller=controller, width=width, @@ -2677,22 +2679,27 @@ def test_init_calls_top_button(self, mocker, width, count, title, assert topic_button.stream_id == stream_id assert topic_button.topic_name == title - @pytest.mark.parametrize(['stream_name', 'title', 'muted_topics', + @pytest.mark.parametrize(['stream_name', 'title', + 'is_muted_topic_return_value', 'is_muted_called'], [ - ('Django', 'topic1', [['Django', 'topic1']], True), - ('Django', 'topic2', [['Django', 'topic1']], False), - ('GSoC', 'topic1', [['Django', 'topic1']], False), + ('Django', 'topic1', True, True), + ('Django', 'topic2', False, False), + ('GSoC', 'topic1', False, False), ], ids=[ + # Assuming 'Django', 'topic1' is muted via muted_topics. 'stream_and_topic_match', 'topic_mismatch', 'stream_mismatch', ]) def test_init_calls_mark_muted(self, mocker, stream_name, title, - muted_topics, is_muted_called): + is_muted_topic_return_value, + is_muted_called): mark_muted = mocker.patch( 'zulipterminal.ui_tools.buttons.TopicButton.mark_muted') controller = mocker.Mock() - controller.model.muted_topics = muted_topics + controller.model.is_muted_topic = ( + mocker.Mock(return_value=is_muted_topic_return_value) + ) controller.model.stream_dict = { 205: {'name': stream_name} } diff --git a/tests/ui/test_utils.py b/tests/ui/test_utils.py index 3aafccfa7e..1398b270a4 100644 --- a/tests/ui/test_utils.py +++ b/tests/ui/test_utils.py @@ -3,8 +3,8 @@ from zulipterminal.ui_tools.utils import create_msg_box_list, is_muted -@pytest.mark.parametrize(['msg', 'narrow', 'muted_streams', 'muted_topics', - 'muted'], [ +@pytest.mark.parametrize(['msg', 'narrow', 'muted_streams', + 'is_muted_topic_return_value', 'muted'], [ ( # PM TEST { 'type': 'private', @@ -12,10 +12,7 @@ }, [], [1, 2], - [ - ['foo', 'boo foo'], - ['boo', 'foo boo'], - ], + False, False ), ( @@ -25,10 +22,7 @@ }, [['stream', 'foo'], ['topic', 'boo']], [1, 2], - [ - ['foo', 'boo foo'], - ['boo', 'foo boo'], - ], + False, False ), ( @@ -39,52 +33,41 @@ }, [['stream', 'foo']], [1, 2], - [ - ['foo', 'boo foo'], - ['boo', 'foo boo'], - ], + False, True ), ( { 'type': 'stream', 'stream_id': 2, - 'display_recipient': 'boo', - 'subject': 'foo boo', # ... }, [], [1, 2], - [ - ['foo', 'boo foo'], - ['boo', 'foo boo'], - ], + True, True ), ( { 'type': 'stream', 'stream_id': 3, - 'display_recipient': 'zoo', 'subject': 'foo koo', # ... }, [], [1, 2], - [ - ['foo', 'boo foo'], - ['boo', 'foo boo'], - ], + False, False ), ]) -def test_is_muted(mocker, msg, narrow, muted_streams, muted_topics, muted): +def test_is_muted(mocker, msg, narrow, muted_streams, + is_muted_topic_return_value, muted): model = mocker.Mock() model.is_muted_stream = ( mocker.Mock(return_value=(msg.get('stream_id', '') in muted_streams)) ) model.narrow = narrow - model.muted_topics = muted_topics + model.is_muted_topic.return_value = is_muted_topic_return_value return_value = is_muted(msg, model) assert return_value is muted diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py index 2b30d5df87..846adf167a 100644 --- a/zulipterminal/helper.py +++ b/zulipterminal/helper.py @@ -187,8 +187,7 @@ def _set_count_in_view(controller: Any, new_count: int, + new_count) break # FIXME: Update unread_counts['unread_topics']? - if ([message['display_recipient'], msg_topic] in - controller.model.muted_topics): + if controller.model.is_muted_topic(stream_id, msg_topic): add_to_counts = False if is_open_topic_view and stream_id == toggled_stream_id: # If topic_view is open for incoming messages's stream, @@ -441,8 +440,7 @@ def classify_unread_counts(model: Any) -> UnreadCounts: # unsubscribed streams may be in unreads, but not in stream_dict if stream_id not in model.stream_dict: continue - if [model.stream_dict[stream_id]['name'], - stream['topic']] in model.muted_topics: + if model.is_muted_topic(stream_id, stream['topic']): continue stream_topic = (stream_id, stream['topic']) unread_counts['unread_topics'][stream_topic] = count diff --git a/zulipterminal/ui_tools/buttons.py b/zulipterminal/ui_tools/buttons.py index a09125ae19..e804cf93eb 100644 --- a/zulipterminal/ui_tools/buttons.py +++ b/zulipterminal/ui_tools/buttons.py @@ -242,8 +242,7 @@ def __init__(self, stream_id: int, topic: str, width=width, count=count) - topic_pair = [self.stream_name, self.topic_name] - if topic_pair in controller.model.muted_topics: + if controller.model.is_muted_topic(self.stream_id, self.topic_name): self.mark_muted() def mark_muted(self) -> None: diff --git a/zulipterminal/ui_tools/utils.py b/zulipterminal/ui_tools/utils.py index 754089d303..d9d3431ef8 100644 --- a/zulipterminal/ui_tools/utils.py +++ b/zulipterminal/ui_tools/utils.py @@ -60,7 +60,7 @@ def is_muted(msg: Message, model: Any) -> bool: return False elif model.is_muted_stream(msg['stream_id']): return True - elif [msg['display_recipient'], msg['subject']] in model.muted_topics: + elif model.is_muted_topic(msg['stream_id'], msg['subject']): return True return False From 433fa40b827354dee1659f25c99865c403f3af95 Mon Sep 17 00:00:00 2001 From: Preet Mishra Date: Sat, 1 Aug 2020 15:24:50 +0530 Subject: [PATCH 3/5] model: Restrict muted_topics access using a protected access modifier. Test amended. --- tests/model/test_model.py | 2 +- zulipterminal/model.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/model/test_model.py b/tests/model/test_model.py index ead6f8e310..d3d5bd1dd7 100644 --- a/tests/model/test_model.py +++ b/tests/model/test_model.py @@ -1585,7 +1585,7 @@ def test_is_muted_stream(self, muted_streams, stream_id, is_muted, ]) def test_is_muted_topic(self, topic, is_muted, stream_dict, model): model.stream_dict = stream_dict - model.muted_topics = [ + model._muted_topics = [ ['Stream 2', 'muted topic'], ['Stream 1', 'muted stream muted topic'], ] diff --git a/zulipterminal/model.py b/zulipterminal/model.py index a3a97edcb6..b1346ed183 100644 --- a/zulipterminal/model.py +++ b/zulipterminal/model.py @@ -116,7 +116,7 @@ def __init__(self, controller: Any) -> None: (self.stream_dict, self.muted_streams, self.pinned_streams, self.unpinned_streams) = stream_data - self.muted_topics = ( + self._muted_topics = ( self.initial_data['muted_topics']) # type: List[List[str]] groups = self.initial_data['realm_user_groups'] @@ -416,7 +416,7 @@ def is_muted_topic(self, stream_id: int, topic: str) -> bool: """ stream_name = self.stream_dict[stream_id]['name'] topic_to_search = [stream_name, topic] # type: List[str] - return topic_to_search in self.muted_topics + return topic_to_search in self._muted_topics def _update_initial_data(self) -> None: # Thread Processes to reduce start time. From af346497460739144eba4ba8796a89f4efce911e Mon Sep 17 00:00:00 2001 From: Preet Mishra Date: Sun, 9 Aug 2020 20:35:40 +0530 Subject: [PATCH 4/5] bugfix: conftest/model: Reflect API migration for muted topics. This reflects the expected server response change for muted_topics, from [stream_name, topic] to [stream_name, topic, date_muted] in Zulip version 3.0, Feature level 1 (see /~https://github.com/zulip/zulip/commit/9340cd1a0b7af7af9b83a622e37d02f1a2340609). Test updated along with the addition of processed_muted_topics() fixture in conftest.py and added for muted topic's local conversion. --- tests/conftest.py | 22 ++++++++++++++++++++++ tests/model/test_model.py | 37 ++++++++++++++++++++++++++++++++----- zulipterminal/model.py | 16 ++++++++++++---- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 18f0cdb42c..af3dee2f44 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -752,6 +752,28 @@ def stream_dict(streams_fixture): return {stream['stream_id']: stream for stream in streams_fixture} +@pytest.fixture(params=[ + { + ('Stream 1', 'muted stream muted topic'): None, + ('Stream 2', 'muted topic'): None, + }, + { + ('Stream 1', 'muted stream muted topic'): 1530129122, + ('Stream 2', 'muted topic'): 1530129122, + }, + ], + ids=[ + 'zulip_feature_level:None', + 'zulip_feature_level:1', + ] +) +def processed_muted_topics(request): + """ + Locally processed muted topics data (see _muted_topics in Model.__init__). + """ + return request.param + + @pytest.fixture def classified_unread_counts(): """ diff --git a/tests/model/test_model.py b/tests/model/test_model.py index d3d5bd1dd7..365396039f 100644 --- a/tests/model/test_model.py +++ b/tests/model/test_model.py @@ -72,6 +72,35 @@ def test_init(self, model, initial_data, user_profile): self.classify_unread_counts.assert_called_once_with(model) assert model.unread_counts == [] + @pytest.mark.parametrize(['server_response', 'locally_processed_data', + 'zulip_feature_level'], [ + ( + [['Stream 1', 'muted stream muted topic']], + {('Stream 1', 'muted stream muted topic'): None}, + None, + ), + ( + [['Stream 2', 'muted topic', 1530129122]], + {('Stream 2', 'muted topic'): 1530129122}, + 1, + ), + ], + ids=[ + 'zulip_feature_level:None', + 'zulip_feature_level:1', + ] + ) + def test_init_muted_topics(self, mocker, initial_data, server_response, + locally_processed_data, zulip_feature_level): + mocker.patch('zulipterminal.model.Model.get_messages', return_value='') + initial_data['zulip_feature_level'] = zulip_feature_level + initial_data['muted_topics'] = server_response + self.client.register = mocker.Mock(return_value=initial_data) + + model = Model(self.controller) + + assert model._muted_topics == locally_processed_data + def test_init_InvalidAPIKey_response(self, mocker, initial_data): # Both network calls indicate the same response mocker.patch('zulipterminal.model.Model.get_messages', @@ -1583,12 +1612,10 @@ def test_is_muted_stream(self, muted_streams, stream_id, is_muted, ((1, 'muted stream muted topic'), True), ((2, 'unmuted topic'), False), ]) - def test_is_muted_topic(self, topic, is_muted, stream_dict, model): + def test_is_muted_topic(self, topic, is_muted, stream_dict, model, + processed_muted_topics): model.stream_dict = stream_dict - model._muted_topics = [ - ['Stream 2', 'muted topic'], - ['Stream 1', 'muted stream muted topic'], - ] + model._muted_topics = processed_muted_topics return_value = model.is_muted_topic(stream_id=topic[0], topic=topic[1]) diff --git a/zulipterminal/model.py b/zulipterminal/model.py index b1346ed183..1761739a03 100644 --- a/zulipterminal/model.py +++ b/zulipterminal/model.py @@ -116,8 +116,16 @@ def __init__(self, controller: Any) -> None: (self.stream_dict, self.muted_streams, self.pinned_streams, self.unpinned_streams) = stream_data - self._muted_topics = ( - self.initial_data['muted_topics']) # type: List[List[str]] + # NOTE: The expected response has been upgraded from + # [stream_name, topic] to [stream_name, topic, date_muted] in + # feature level 1, server version 3.0. + muted_topics = self.initial_data['muted_topics'] + assert set(map(len, muted_topics)) in (set(), {2}, {3}) + self._muted_topics = { + (stream_name, topic): (None if self.server_feature_level is None + else date_muted[0]) + for stream_name, topic, *date_muted in muted_topics + } # type: Dict[Tuple[str, str], Optional[int]] groups = self.initial_data['realm_user_groups'] self.user_group_by_id = {} # type: Dict[int, Dict[str, Any]] @@ -415,8 +423,8 @@ def is_muted_topic(self, stream_id: int, topic: str) -> bool: Returns True if topic is muted via muted_topics. """ stream_name = self.stream_dict[stream_id]['name'] - topic_to_search = [stream_name, topic] # type: List[str] - return topic_to_search in self._muted_topics + topic_to_search = (stream_name, topic) + return topic_to_search in self._muted_topics.keys() def _update_initial_data(self) -> None: # Thread Processes to reduce start time. From 9989b913196d77116619b015d867d8e203fc0f87 Mon Sep 17 00:00:00 2001 From: Preet Mishra Date: Tue, 11 Aug 2020 21:10:54 +0530 Subject: [PATCH 5/5] refactor: tests: ui_tools: Simplify test_init_calls_mark_muted(). This simplifies the test parametrization and its body. --- tests/ui/test_ui_tools.py | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/tests/ui/test_ui_tools.py b/tests/ui/test_ui_tools.py index ac55a23666..e6466fbcdb 100644 --- a/tests/ui/test_ui_tools.py +++ b/tests/ui/test_ui_tools.py @@ -2679,34 +2679,24 @@ def test_init_calls_top_button(self, mocker, width, count, title, assert topic_button.stream_id == stream_id assert topic_button.topic_name == title - @pytest.mark.parametrize(['stream_name', 'title', - 'is_muted_topic_return_value', - 'is_muted_called'], [ - ('Django', 'topic1', True, True), - ('Django', 'topic2', False, False), - ('GSoC', 'topic1', False, False), - ], ids=[ - # Assuming 'Django', 'topic1' is muted via muted_topics. - 'stream_and_topic_match', - 'topic_mismatch', - 'stream_mismatch', + @pytest.mark.parametrize(['is_muted_topic_return_value', + 'is_mark_muted_called'], [ + (True, True), + (False, False), ]) - def test_init_calls_mark_muted(self, mocker, stream_name, title, - is_muted_topic_return_value, - is_muted_called): - mark_muted = mocker.patch( - 'zulipterminal.ui_tools.buttons.TopicButton.mark_muted') + def test_init_calls_mark_muted(self, mocker, is_muted_topic_return_value, + is_mark_muted_called): + mocker.patch('zulipterminal.ui_tools.buttons.TopicButton.mark_muted') controller = mocker.Mock() controller.model.is_muted_topic = ( mocker.Mock(return_value=is_muted_topic_return_value) ) controller.model.stream_dict = { - 205: {'name': stream_name} + 205: {'name': 'Stream 1'} } + topic_button = TopicButton(stream_id=205, - topic=title, controller=controller, + topic='Topic', controller=controller, width=40, count=0) - if is_muted_called: - mark_muted.assert_called_once_with() - else: - mark_muted.assert_not_called() + + assert topic_button.mark_muted.called == is_mark_muted_called