feature: Topic muting #914

mkp6781 · 2021-02-08T06:09:02Z

I have rebased @sumanthvrao branch for topic muting on to the main. I have modified some of the code and logic to accommodate the changes in data structures and added tests while maintaining the same commit structure as earlier.
In the 4th commit, I am yet to add the tests for the two new functions. I will be working on this soon.

Continues work on #467

mkp6781 · 2021-02-08T07:38:57Z

@zulipbot add "PR needs review"

Ezio-Sarthak

Thanks for taking this up 👍. This provisionally looks a great add-on! Leaving some comments inline :)

On manual testing, I observed that the unread count for All_msg changes even for muting topics in a muted stream. This edge case could be handled accordingly. (And potentially later in tests too)

minor: Typo in first commit message: API class => API call

Ezio-Sarthak · 2021-03-29T06:33:29Z

tests/model/test_model.py

+    def test_toggle_topic_muted_status(self, mocker, model,
+                                       stream_dict,
+                                       initial_muted_topics, op,
+                                       response={'result': 'success'}):


We could add tests for {result: error} too for non-existent topics perhaps?

Ezio-Sarthak · 2021-03-29T06:37:27Z

tests/model/test_model.py

+        model.toggle_topic_muted_status(1, 'Stream 1', 'party')
+        request = {
+            'stream': 'Stream 1',
+            'topic': 'party',


Maybe parameterize topics, so as to handle cases for non-existing topics? (See my above comment)

I made an API call with a non-existing topic in the request but it still returned a 'success' response. I'm not sure why that is the case. Maybe it's an issue in zulip api?

Ah right. I just looked over API events for topic muting and it seems like the response will be succeeded regardless of whether any messages have been sent to the specified topic.

The edge cases for error handling might be:

muting already muted topic

unumting an already unmuted topic

I had a look at this API events for topic muting but it did not mention anything about non-existent topics which you pointed out. I raised an issue regarding the same in the api repo.

The edge cases for error handling might be:

* muting already muted topic * unumting an already unmuted topic

request = { 'stream': stream_name, 'topic': topic_name, 'op': 'remove' if self.is_muted_topic(stream_id, topic_name) else 'add' }

This is the code for initializing the request variable for the api call. Depending on the topic's current state(muted/not muted), 'op' is assigned a value which can toggle the state. So there would'nt be an error case as such!
One thing I could do is rename 'op' in parametrize to 'expected_op'?

I don't think renaming is fruitful. As you said, the update event wouldn't be called in the edge cases I mentioned 👍

Ezio-Sarthak · 2021-03-29T06:56:04Z

zulipterminal/core.py

+        currently_muted = self.model.is_muted_topic(button.stream_id,
+                                                    button.topic_name)
+        type_of_action = "unmuting" if currently_muted else "muting"
+        question = urwid.Text(("bold", "Confirm " + type_of_action


On manual testing, I found the bold attribute isn't working?

Yea noticed that now. We don't need it to be bold anyways so i'll leave it as an empty string.

Ezio-Sarthak · 2021-03-29T08:18:28Z

zulipterminal/ui_tools/buttons.py

+                                self.stream_id, self.topic_name)] = self.count
+        self.model.unread_counts['unread_topics'].pop(
+                                    (self.stream_id, self.topic_name), None)
+        self.model.unread_counts['all_msg'] -= self.count


This needs an edge case handling. (See my review bullet)

I haven't checked other updates, though I could still reproduce this bug (i.e., subtracting unreads in the case of a muted stream)

Ezio-Sarthak · 2021-03-29T08:25:08Z

README.md

+### Topic list actions
+| Command                                               | Key Combination                               |
+|------------------------------------------------------ | --------------------------------------------- |
+| Mute/unmute Topics                                    | <kbd>M</kbd>                                  |
+


Note that we use auto-generated hotkeys now, thanks to your work in #926 :)

Ezio-Sarthak · 2021-03-29T08:31:02Z

zulipterminal/ui_tools/buttons.py

+    def keypress(self, size: urwid_Size, key: str) -> Optional[str]:
+        if is_command_key('TOGGLE_MUTE_TOPIC', key):
+            self.controller.topic_muting_confirmation_popup(self)
+        return super().keypress(size, key)


We could add a test for this?

Ezio-Sarthak · 2021-04-03T11:41:03Z

@zulipbot remove "PR needs review"
@zulipbot add "PR awaiting update"

This handles updating muting/unmuting topics in a stream by sending the request to the server with the help of an API call. Tests added.

We make use of the PopUpConfirmation for confirmation if the user wishes to continue performing muting/unmuting action of the displayed topic. The TopicButton's stream_name and topic_name attribute allow us to display the affected stream name and topic name in the view. Tests added.

We register the event_type 'muted_topics' to the event queue on startup, along with a handler method which responds to these events appropriately. Depending on whether the topic is already muted or not, the handler method calls the `mark_muted` or `mark_unmuted`. These two functions will update the data structures which keeps track of message counts in a later commit. Tests added

`model.unread_counts` and button counts are updated based on user action of muting or unmuting. During muting of a topic: * We decrease All_msg count by the unread count of topic. * If there are unread messages in the stream, we decrease its count by the same amount as well. During unmuting of a topic: * If there are unread messages in the stream, we replace 'M' with the correct unread count. * If there are no unread messages, we remove the marked 'M'. Tests to be added for mark_(un)muted.

Handles setting count of `model.unread_counts`. Functions `_set_count_in_model` and `_set_count_in_view` are modified to keep track of unread counts of muted topics when a new message is recieved or an existing message is read. This ensures that the count is in sync when the topic is unmuted later on.

New section 'Topic list actions' created in hotkeys.md. New 'topic_list' category created in keys for mapping keys belonging to topic list actions.

On pressing TOGGLE_MUTE_TOPIC, we now ask for confirmation before performing the action of muting/unmuting topics. Tests added.

mkp6781 · 2021-04-24T09:13:11Z

@zulipbot add "PR needs review"

zulipbot · 2021-04-24T09:13:13Z

ERROR: Label " PR needs review" does not exist and was thus not added to this pull request.

mkp6781 · 2021-04-24T09:14:27Z

@zulipbot add "PR needs review"

mkp6781 · 2021-04-24T09:14:54Z

@zulipbot remove "PR awaiting update"

mkp6781 · 2021-04-24T09:16:48Z

These commits deal with functionality aspects of this new feature.
In the 4th commit, I haven't added the test for mark_muted and mark_unmuted functions. I will do that in the next iteration.

zulipbot · 2021-08-30T23:02:48Z

Heads up @mkp6781, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added the size: XL [Automatic label added by zulipbot] label Feb 8, 2021

mkp6781 changed the title ~~Issue 745~~ Topic muting Feb 8, 2021

mkp6781 changed the title ~~Topic muting~~ feature: Topic muting Feb 8, 2021

mkp6781 force-pushed the issue_745 branch from e1429e9 to f2fe975 Compare February 8, 2021 06:43

zulipbot added the PR needs review PR requires feedback to proceed label Feb 8, 2021

mkp6781 force-pushed the issue_745 branch from f2fe975 to 0e90efd Compare February 12, 2021 11:00

Ezio-Sarthak suggested changes Mar 29, 2021

View reviewed changes

zulipbot added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Apr 3, 2021

Ezio-Sarthak mentioned this pull request Apr 12, 2021

TRACKING: Event handling improvements #993

Open

15 tasks

sumanthvrao and others added 7 commits April 24, 2021 09:42

model: Send toggle_muted_topic status request to mute_topic.

f96389a

This handles updating muting/unmuting topics in a stream by sending the request to the server with the help of an API call. Tests added.

hotkeys/keys: Add 'TOGGLE_MUTE_TOPIC' hotkey to 'mute/unmute' topics.

ecc30c9

New section 'Topic list actions' created in hotkeys.md. New 'topic_list' category created in keys for mapping keys belonging to topic list actions.

buttons: Connect PopUp confirmation display for topics via keypress.

02bd05c

On pressing TOGGLE_MUTE_TOPIC, we now ask for confirmation before performing the action of muting/unmuting topics. Tests added.

mkp6781 force-pushed the issue_745 branch from 0e90efd to effc9b8 Compare April 24, 2021 09:12

mkp6781 force-pushed the issue_745 branch from effc9b8 to 02bd05c Compare April 24, 2021 09:14

zulipbot added the PR needs review PR requires feedback to proceed label Apr 24, 2021

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Apr 24, 2021

zulipbot added the has conflicts label Aug 30, 2021

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Topic muting #914

feature: Topic muting #914

mkp6781 commented Feb 8, 2021 •

edited

mkp6781 commented Feb 8, 2021

Ezio-Sarthak left a comment

Ezio-Sarthak Mar 29, 2021

Ezio-Sarthak Mar 29, 2021

mkp6781 Apr 11, 2021

Ezio-Sarthak Apr 12, 2021

mkp6781 Apr 12, 2021

Ezio-Sarthak Apr 12, 2021

Ezio-Sarthak Mar 29, 2021

mkp6781 Apr 11, 2021

Ezio-Sarthak Mar 29, 2021

Ezio-Sarthak Apr 29, 2021 •

edited

Ezio-Sarthak Mar 29, 2021

Ezio-Sarthak Mar 29, 2021

Ezio-Sarthak commented Apr 3, 2021

mkp6781 commented Apr 24, 2021 •

edited

zulipbot commented Apr 24, 2021

mkp6781 commented Apr 24, 2021

mkp6781 commented Apr 24, 2021

mkp6781 commented Apr 24, 2021

zulipbot commented Aug 30, 2021

feature: Topic muting #914

Are you sure you want to change the base?

feature: Topic muting #914

Conversation

mkp6781 commented Feb 8, 2021 • edited

mkp6781 commented Feb 8, 2021

Ezio-Sarthak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak Apr 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezio-Sarthak commented Apr 3, 2021

mkp6781 commented Apr 24, 2021 • edited

zulipbot commented Apr 24, 2021

mkp6781 commented Apr 24, 2021

mkp6781 commented Apr 24, 2021

mkp6781 commented Apr 24, 2021

zulipbot commented Aug 30, 2021

mkp6781 commented Feb 8, 2021 •

edited

Ezio-Sarthak Apr 29, 2021 •

edited

mkp6781 commented Apr 24, 2021 •

edited