-
-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Support muted users #1546
base: main
Are you sure you want to change the base?
Conversation
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>
ed83f2b
to
10ad48b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?)
ids=[ | ||
"zulip_feature_level:48", | ||
"zulip_feature_level:0", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits:
server_response
could be named more specifically?zulip_feature_level
is part of the setup, so better earlierids
are being asserted on, so would be cleaner later (see above), but also that's more obvious if named with anexpected_
prefix, and again perhaps more specifically :)
def _update_muted_users(self, muted_users: List[Dict[int, int]]) -> None: | ||
self._muted_users = {muted_user["id"] for muted_user in muted_users} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
def test_init_muted_users( | ||
self, mocker, initial_data, server_response, ids, zulip_feature_level | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this test 👍
@@ -903,6 +903,7 @@ def initial_data( | |||
} | |||
], | |||
"result": "success", | |||
"muted_users": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the co-author tags aren't showing in github since they're not quite formatted correctly.
self.model.is_muted_user.return_value = False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be inclined to add parentheses here for the new code, to make this clearer.
@@ -867,6 +869,7 @@ def test_main_view_generates_stream_header( | |||
"reactions": [], | |||
"sender_full_name": "Alice", | |||
"timestamp": 1532103879, | |||
"sender_id": 32900, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linting easy fix?
mocker.patch("zulipterminal.model.Model.is_muted_user", return_value=False) | ||
self.view.model.is_muted_user.return_value = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like duplication, ie. is one doing what you might think?
for user in users: | ||
if self.view.model.is_muted_user(user["user_id"]): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
topic
How did you test this?
Self-review checklist for each commit
Visual changes