Implement muted users #1425

Subhasish-Behera · 2023-08-23T18:43:45Z

What does this PR do, and why?

External discussion & connections

Discussed in #zulip-terminal in topic
Fully fixes #
Partially fixes issue Add support for muted users #1001
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on #

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

The muted users are stored in muted_users list of Model class. Tests adapted.

Update the muted_user list after a muted_user event. Add MutedUserEvent to api_types.

Add "muted_messages" to index. It gets updated when sender_id of a messsage is in model.muted_users list.

The content shown for the message is changed. The recepient header shows muted_user instead of original name. The name of the author is also changed to muted_user. Tests Adapted.

skip the user if in muted_users while iterating. Tests adapted.

neiljp

@Subhasish-Behera Thanks for getting this into a PR; the quicker there is code to give a manual try, and potentially to read, the quicker feedback can be given, allowing quicker updates :)

From manual testing I can tell that the core functionality appears to be working fine: fetching the initial state, updating it, with the latter demonstrated by the user list updating (every <60s) when I mute and unmute users via the web app.

Ideally we might hide and unhide users in the user list when we receive the events, but right now we don't have a generalized handler for that, so the slight delay to the user list update works fine until we have that. So, I think we could certainly get some of the earlier commits on the model merged sooner, in combination with the user list update, once those commits are slightly polished. Having the last commit next to what is currently the first two would make it easier to merge from the start of the PR if we do that.

The styling of messages sent by muted users does also work, if less completely - it's a more complex feature :) Right now if a user is muted when we enter a narrow, then the content and user is hidden 👍 However, this will need a little more care:

if the muting status of a user is changed, going to a different narrow shows the message list with that new muting status, but doesn't affect going back to the previous narrow
if the muting status of a user is changed while in a message list including from a muted user, the visibility of those messages are not updated
while it is possible to view the content of a message indirectly via i r, there is no other way to quickly temporarily view the content, like the web app 'click to reveal'
we may wish to make these hidden messages more compact, possibly in the content, but also with a fixed presence marker and maybe no sender?
reactions should also be substituted -> "Muted user"

I left inline feedback, which would be good to have addressed via code changes and/or your own comments, but let's discuss the challenges with the message styling in the stream, as well as any longer queries you have.

neiljp · 2023-08-24T23:01:59Z

zulipterminal/model.py

@@ -205,6 +206,10 @@ def __init__(self, controller: Any) -> None:
            for stream_name, topic, *date_muted in muted_topics
        }

+        self.muted_users = [


Please add a type for this; mypy guesses List[Any], likely since we don't have a type for initial_data.

I'd suggest a leading underscore in the attribute name, to indicate this is intended for use internal to the model.

neiljp · 2023-08-25T00:10:07Z

zulipterminal/model.py

@@ -205,6 +206,10 @@ def __init__(self, controller: Any) -> None:
            for stream_name, topic, *date_muted in muted_topics
        }

+        self.muted_users = [
+            muted_user["id"] for muted_user in self.initial_data["muted_users"]


Note that this line will fail on older servers where muted_users is not present in initial_data, so you'll need to handle that situation.

I'm not sure if we want to have test_init depend upon feature level (or server version), or have another test for this. I'd expect self.muted_users to be checked in test_init for now in any case.

neiljp · 2023-08-25T00:16:35Z

zulipterminal/api_types.py

@@ -594,6 +594,11 @@ class UpdateDisplaySettingsEvent(TypedDict):
    setting: bool


+class MutedUserEvent(TypedDict):
+    type: Literal["muted_users"]
+    muted_users: List[Dict[str, int]]


Ideally we would pin down the Dict further by using a TypedDict. That would ensure that only the two specific str keys are allowed, even if all the values are of type int!

neiljp · 2023-08-25T00:24:00Z

zulipterminal/model.py

        }

+        self.muted_users: List[int] = list()


This same attribute is set later on already, so you shouldn't need this - it doesn't cause a problem, but it's confusing.

neiljp · 2023-08-25T00:24:55Z

zulipterminal/model.py

@@ -205,6 +206,10 @@ def __init__(self, controller: Any) -> None:
            for stream_name, topic, *date_muted in muted_topics
        }

+        self.muted_users = [


If you don't include the timestamp data, it's worth including a comment noting it's available but unused.

neiljp · 2023-08-25T01:09:21Z

zulipterminal/ui_tools/views.py

@@ -691,6 +691,8 @@ def users_view(self, users: Any = None) -> Any:

        users_btn_list = list()
        for user in users:
+            if user["user_id"] in self.view.model.muted_users:


This should use the accessor as well, and the test should mock it.

neiljp · 2023-08-25T01:46:49Z

zulipterminal/ui_tools/messages.py

+                for recipient in self.message["display_recipient"]:
+                    if recipient["email"] != self.model.user_email:
+                        if self.message["id"] in self.model.index["muted_messages"]:
+                            self.recipients_list.append("muted_user")


This works to indicate that the message is from a muted user, but we'll want this text to be something more user-friendly, and if we use it many places here the string should be a module-level constant.

neiljp · 2023-08-25T01:52:30Z

zulipterminal/ui_tools/messages.py

+                self.recipients_list = []
+                for recipient in self.message["display_recipient"]:
+                    if recipient["email"] != self.model.user_email:
+                        if self.message["id"] in self.model.index["muted_messages"]:
+                            self.recipients_list.append("muted_user")
+                        else:
+                            self.recipients_list.append(recipient["full_name"])
+                self.recipients_names = ", ".join(self.recipients_list)


You should be able to retain the original code structure for the loop with a small change. It'd be ok to switch to this form for the loop if it was a lot more complex, but that doesn't seem necessary here.

Also, note that you've introduced self.recipients_list, which is used nowhere else, so if this were to be the new form of the loop, there is no need to add this new object attribute, it can simply be a local variable.

neiljp · 2023-08-25T03:09:32Z

zulipterminal/ui_tools/messages.py

@@ -672,7 +673,10 @@ def main_view(self) -> List[Any]:
            text: Dict[str, urwid_MarkupTuple] = {key: (None, " ") for key in text_keys}

            if any(different[key] for key in ("recipients", "author", "24h")):
-                text["author"] = ("msg_sender", message["this"]["author"])
+                if self.message["id"] in self.model.index["muted_messages"]:
+                    text["author"] = ("msg_sender", "muted_user")


Similar to above, we'll want this to be a more user-friendly text.

neiljp · 2023-08-25T03:18:41Z

zulipterminal/ui_tools/messages.py

+        muted_message_text = "This message was hidden because you have muted the sender"
        # Transform raw message content into markup (As needed by urwid.Text)
-        content, self.message_links, self.time_mentions = self.transform_content(
-            self.message["content"], self.model.server_url
-        )
+        if self.message["id"] in self.model.index["muted_messages"]:
+            content, self.message_links, self.time_mentions = (
+                (None, muted_message_text),
+                {},
+                [],
+            )
+        else:
+            content, self.message_links, self.time_mentions = self.transform_content(


The comment should remain with the relevant code.

If you were unpacking once into the same variables, from an object that was dependent on the result of the conditional, unpacking overall would seem more reasonable. I'd suggest doing that, or simply assigning each individually in the top branch of the conditional.

mounilKshah

Thank you @Subhasish-Behera for working on this. I tried running this on my local machine and it worked fine. As for any feedback/changes, for the current iteration, Neil has already given quite apt suggestions.

The current version of the PR hides the message sent by the muted user. After implementing Neil's suggestions, I think a later iteration can be to unhide the message by the muted user.

mounilKshah · 2023-09-17T18:54:51Z

zulipterminal/model.py

+        Handle muting/unmuting of users
+        """
+        assert event["type"] == "muted_users"
+        self.muted_users = [user["id"] for user in event["muted_users"]]


True. However, I had a doubt regarding how to set muted_users for on load situation?
The function which you have suggested to create essentially work when an event occurs, but when the initial_data is first loaded, how will that be set by such a centralized method?

Subhasish-Behera added 5 commits August 23, 2023 18:25

model: Add muted_users to intial_data_to_fetch.

561a756

The muted users are stored in muted_users list of Model class. Tests adapted.

model/api_types: Handle muted_user event.

bc0f807

Update the muted_user list after a muted_user event. Add MutedUserEvent to api_types.

helper: Add "muted_messages" to mark messages which should be muted.

1e50a08

Add "muted_messages" to index. It gets updated when sender_id of a messsage is in model.muted_users list.

messages: Changes made in MessageBox for muted_messages.

99b0c3d

The content shown for the message is changed. The recepient header shows muted_user instead of original name. The name of the author is also changed to muted_user. Tests Adapted.

views: Remove muted_users from user_list.

53ecc17

skip the user if in muted_users while iterating. Tests adapted.

zulipbot added the size: L [Automatic label added by zulipbot] label Aug 23, 2023

neiljp reviewed Aug 25, 2023

View reviewed changes

mounilKshah reviewed Sep 17, 2023

View reviewed changes

mounilKshah added enhancement New feature or request area: UI General user interface update area: event handling How events from the server are responded to PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Sep 17, 2023

neiljp changed the title ~~Implement muted users2~~ Implement muted users Sep 19, 2023

neiljp added missing feature: user A missing feature for all users, present in another Zulip client PR completion candidate PR with reviews that may unblock merging and removed enhancement New feature or request labels Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement muted users #1425

Implement muted users #1425

Subhasish-Behera commented Aug 23, 2023 •

edited by neiljp

neiljp left a comment

neiljp Aug 24, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

neiljp Aug 25, 2023

mounilKshah left a comment

mounilKshah Sep 17, 2023

Implement muted users #1425

Are you sure you want to change the base?

Implement muted users #1425

Conversation

Subhasish-Behera commented Aug 23, 2023 • edited by neiljp

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

neiljp 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

Choose a reason for hiding this comment

mounilKshah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Subhasish-Behera commented Aug 23, 2023 •

edited by neiljp