Support muted users #1546

rsashank · 2024-09-20T14:04:17Z

What does this PR do, and why?

Adds support to handle muted users

TODO: Add support to mute and unmute users on ZT

Outstanding aspect(s)

[ ]

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 Implement muted users #1425
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. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

Update the muted_user list after a muted_user event. Add MutedUserEvent to api_types. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

Test added.

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. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

Skip user from showing in user list if user is muted. Test updated. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

neiljp

@rsashank Thanks for taking this PR forward 👍

Fixing the co-authored field will help this be more obvious which commits are yours, but it's worth noting what you've added too - eg tests in some cases :)

Most of the previous comments appear to have been addressed, and the feature broadly works. The remaining aspects are missing muted-users features (eg. see the API page), and that the updating of muting status in the UI relies upon some unrelated or indirect calls.

For the message update, I expect it is relying upon update_message_list_status_markers, which is indirectly updating the entire message list - including this message content. That makes for a rather confusing control flow right now, if so, and it would be good to make each part explicit.

For outstanding muted users features, I think we could build on that in a follow-up PR, most notably for

compressing multiple lines of message by the same muted user into one different message (does web do this?)
hiding 1:1 DM conversations (various views)
hiding in DM autocomplete
possibly hiding in message content mention autocomplete (web allows @ only?)

neiljp · 2024-09-23T01:09:44Z

tests/model/test_model.py

+        ids=[
+            "zulip_feature_level:48",
+            "zulip_feature_level:0",
+        ],


In other tests we've also handled an edge case near the ZFL when it's enabled; given the setup, it wouldn't be much to add, though there should be no significant difference.

neiljp · 2024-09-23T01:10:03Z

tests/model/test_model.py

@@ -190,6 +190,36 @@ def test_init_muted_topics(

        assert model._muted_topics == locally_processed_data

+    @pytest.mark.parametrize(
+        "server_response, ids, zulip_feature_level",


Nits:

server_response could be named more specifically?

zulip_feature_level is part of the setup, so better earlier

ids are being asserted on, so would be cleaner later (see above), but also that's more obvious if named with an expected_ prefix, and again perhaps more specifically :)

neiljp · 2024-09-23T01:19:48Z

zulipterminal/model.py

+    def _update_muted_users(self, muted_users: List[Dict[int, int]]) -> None:
+        self._muted_users = {muted_user["id"] for muted_user in muted_users}


I can see this is good preparation for the next commit, though you don't use the MutedUser type here, since you define it later - and that may be clearer.

For just this one commit, we don't need this function, so it could be inline in the earlier conditional - and then refactor it next with the new type, before then reusing the type and method in what is now the next commit (with the event).

However, pick the commit flow that you feel works well - I see this was broadly similar to the previous PR right now?

neiljp · 2024-09-23T01:26:51Z

tests/model/test_model.py

+    def test_init_muted_users(
+        self, mocker, initial_data, server_response, ids, zulip_feature_level
+    ):


Thanks for adding this test 👍

neiljp · 2024-09-23T01:28:19Z

tests/conftest.py

@@ -903,6 +903,7 @@ def initial_data(
            }
        ],
        "result": "success",
+        "muted_users": {},


Note that the co-author tags aren't showing in github since they're not quite formatted correctly.

neiljp · 2024-09-23T04:58:45Z

tests/ui_tools/test_messages.py

+        self.model.is_muted_user.return_value = False
+


I expect this adjusts the tests to pass, but doesn't test the new behavior? That is, this doesn't handle any muted-user cases, as well as any cases with mixed/combined messages of muted and not-muted? (before vs current)

neiljp · 2024-09-23T05:03:07Z

zulipterminal/ui_tools/messages.py

-                        recipient["full_name"]
+                        "Muted user"
+                        if self.model.is_muted_user(recipient["id"])
+                        else recipient["full_name"]
                        for recipient in self.message["display_recipient"]
                        if recipient["email"] != self.model.user_email


I'd be inclined to add parentheses here for the new code, to make this clearer.

neiljp · 2024-09-23T05:10:01Z

tests/ui_tools/test_messages.py

@@ -867,6 +869,7 @@ def test_main_view_generates_stream_header(
                "reactions": [],
                "sender_full_name": "Alice",
                "timestamp": 1532103879,
+                "sender_id": 32900,


Linting easy fix?

neiljp · 2024-09-23T05:13:20Z

tests/ui/test_ui_tools.py

+        mocker.patch("zulipterminal.model.Model.is_muted_user", return_value=False)
+        self.view.model.is_muted_user.return_value = False


This seems like duplication, ie. is one doing what you might think?

neiljp · 2024-09-23T05:37:45Z

zulipterminal/ui_tools/views.py

        for user in users:
+            if self.view.model.is_muted_user(user["user_id"]):
+                continue


This seems like a reasonable location for this to get this working, though it seems like a high-level location to be checking through each user. It would be clearer to have a users-without-muted list available from the model, in future, but may need reworking for that.

rsashank added 4 commits September 20, 2024 14:32

model: Add muted_users to intial_data_to_fetch.

68ab15e

The muted users are stored in muted_users list of Model class. Tests adapted. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

model/api_types: Handle muted_user event.

6f1fcce

Update the muted_user list after a muted_user event. Add MutedUserEvent to api_types. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

model: Add is_user_muted method.

a7d0d10

Test added.

zulipbot added the size: XL [Automatic label added by zulipbot] label Sep 20, 2024

views: Remove muted_users from user_list.

10ad48b

Skip user from showing in user list if user is muted. Test updated. Co-authored by: Subhasish-Behera <greendoor3000@gmail.com>

rsashank force-pushed the muted-users branch from ed83f2b to 10ad48b Compare September 20, 2024 14:15

neiljp reviewed Sep 23, 2024

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support muted users #1546

Support muted users #1546

rsashank commented Sep 20, 2024

neiljp left a comment

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

neiljp Sep 23, 2024

		def _update_muted_users(self, muted_users: List[Dict[int, int]]) -> None:
		self._muted_users = {muted_user["id"] for muted_user in muted_users}

		mocker.patch("zulipterminal.model.Model.is_muted_user", return_value=False)
		self.view.model.is_muted_user.return_value = False

Support muted users #1546

Are you sure you want to change the base?

Support muted users #1546

Conversation

rsashank commented Sep 20, 2024

What does this PR do, and why?

Outstanding aspect(s)

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