-
-
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?
Changes from all commits
68ab15e
6f1fcce
a7d0d10
3937ee2
10ad48b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Nits:
|
||
[ | ||
( | ||
[ | ||
{"id": 32323, "timestamp": 1726810359}, | ||
{"id": 37372, "timestamp": 214214214}, | ||
], | ||
{32323, 37372}, | ||
48, | ||
), | ||
([], set(), 0), | ||
], | ||
ids=[ | ||
"zulip_feature_level:48", | ||
"zulip_feature_level:0", | ||
], | ||
Comment on lines
+206
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
) | ||
def test_init_muted_users( | ||
self, mocker, initial_data, server_response, ids, zulip_feature_level | ||
): | ||
Comment on lines
+211
to
+213
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding this test 👍 |
||
mocker.patch(MODEL + ".get_messages", return_value="") | ||
initial_data["zulip_feature_level"] = zulip_feature_level | ||
initial_data["muted_users"] = server_response | ||
self.client.register = mocker.Mock(return_value=initial_data) | ||
|
||
model = Model(self.controller) | ||
|
||
assert model._muted_users == ids | ||
|
||
def test_init_InvalidAPIKey_response(self, mocker, initial_data): | ||
# Both network calls indicate the same response | ||
mocker.patch(MODEL + ".get_messages", return_value="Invalid API key") | ||
|
@@ -245,6 +275,7 @@ def test_register_initial_desired_events(self, mocker, initial_data): | |
"user_settings", | ||
"realm_emoji", | ||
"realm_user", | ||
"muted_users", | ||
] | ||
fetch_event_types = [ | ||
"realm", | ||
|
@@ -262,6 +293,7 @@ def test_register_initial_desired_events(self, mocker, initial_data): | |
"realm_emoji", | ||
"custom_profile_fields", | ||
"zulip_version", | ||
"muted_users", | ||
] | ||
model.client.register.assert_called_once_with( | ||
event_types=event_types, | ||
|
@@ -4080,6 +4112,17 @@ def test_is_muted_topic( | |
|
||
assert return_value == is_muted | ||
|
||
@pytest.mark.parametrize( | ||
"user, is_muted", | ||
[ | ||
case(1, False, id="unmuted_user"), | ||
case(2, True, id="muted_user"), | ||
], | ||
) | ||
def test_is_muted_user(self, user, is_muted, model): | ||
model._muted_users = {2} | ||
assert model.is_muted_user(user) == is_muted | ||
|
||
@pytest.mark.parametrize( | ||
"unread_topics, current_topic, next_unread_topic", | ||
[ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1078,6 +1078,8 @@ def test_users_view(self, users, users_btn_len, editor_mode, status, mocker): | |
user_btn = mocker.patch(VIEWS + ".UserButton") | ||
users_view = mocker.patch(VIEWS + ".UsersView") | ||
right_col_view = RightColumnView(self.view) | ||
mocker.patch("zulipterminal.model.Model.is_muted_user", return_value=False) | ||
self.view.model.is_muted_user.return_value = False | ||
Comment on lines
+1081
to
+1082
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like duplication, ie. is one doing what you might think? |
||
if status != "inactive": | ||
user_btn.assert_called_once_with( | ||
user=self.view.users[0], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -776,6 +776,7 @@ def test_main_view(self, mocker, message, last_message): | |
"reactions": [], | ||
"sender_full_name": "Alice", | ||
"timestamp": 1532103879, | ||
"sender_id": 32900, | ||
} | ||
], | ||
) | ||
|
@@ -814,6 +815,7 @@ def test_main_view_renders_slash_me(self, mocker, message, content, is_me_messag | |
"reactions": [], | ||
"sender_full_name": "Alice", | ||
"timestamp": 1532103879, | ||
"sender_id": 32900, | ||
} | ||
], | ||
) | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Linting easy fix? |
||
}, | ||
], | ||
) | ||
|
@@ -1057,6 +1060,7 @@ def test_msg_generates_search_and_header_bar( | |
"reactions": [], | ||
"sender_full_name": "alice", | ||
"timestamp": 1532103879, | ||
"sender_id": 32900, | ||
} | ||
], | ||
) | ||
|
@@ -1117,6 +1121,8 @@ def test_main_view_content_header_without_header( | |
|
||
msg_box = MessageBox(this_msg, self.model, last_msg) | ||
|
||
self.model.is_muted_user.return_value = False | ||
|
||
Comment on lines
+1124
to
+1125
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
expected_header[2] = output_date_time | ||
if current_year > 2018: | ||
expected_header[2] = "2018 - " + expected_header[2] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,7 @@ def __init__(self, controller: Any) -> None: | |
# zulip_version and zulip_feature_level are always returned in | ||
# POST /register from Feature level 3. | ||
"zulip_version", | ||
"muted_users", | ||
] | ||
|
||
# Events desired with their corresponding callback | ||
|
@@ -160,6 +161,7 @@ def __init__(self, controller: Any) -> None: | |
"user_settings": self._handle_user_settings_event, | ||
"realm_emoji": self._handle_update_emoji_event, | ||
"realm_user": self._handle_realm_user_event, | ||
"muted_users": self._handle_muted_users_event, | ||
} | ||
|
||
self.initial_data: Dict[str, Any] = {} | ||
|
@@ -208,6 +210,11 @@ def __init__(self, controller: Any) -> None: | |
) | ||
for stream_name, topic, *date_muted in muted_topics | ||
} | ||
# NOTE: muted_users also contains timestamps, but we only store the user IDs | ||
# muted_users was added in ZFL 48, Zulip 4.0 | ||
self._muted_users: Set[int] = set() | ||
if self.server_feature_level >= 48: | ||
self._update_muted_users(self.initial_data["muted_users"]) | ||
|
||
groups = self.initial_data["realm_user_groups"] | ||
self.user_group_by_id: Dict[int, Dict[str, Any]] = {} | ||
|
@@ -950,6 +957,9 @@ def is_muted_topic(self, stream_id: int, topic: str) -> bool: | |
topic_to_search = (stream_name, topic) | ||
return topic_to_search in self._muted_topics | ||
|
||
def is_muted_user(self, user_id: int) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: function name doesn't match commit message. |
||
return user_id in self._muted_users | ||
|
||
def stream_topic_from_message_id( | ||
self, message_id: int | ||
) -> Optional[Tuple[int, str]]: | ||
|
@@ -1204,6 +1214,9 @@ def get_user_info(self, user_id: int) -> Optional[TidiedUserInfo]: | |
|
||
return user_info | ||
|
||
def _update_muted_users(self, muted_users: List[Dict[int, int]]) -> None: | ||
self._muted_users = {muted_user["id"] for muted_user in muted_users} | ||
Comment on lines
+1217
to
+1218
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 _update_users_data_from_initial_data(self) -> None: | ||
# Dict which stores the active/idle status of users (by email) | ||
presences = self.initial_data["presences"] | ||
|
@@ -1666,6 +1679,13 @@ def notify_user(self, message: Message) -> str: | |
) | ||
return "" | ||
|
||
def _handle_muted_users_event(self, event: Event) -> None: | ||
""" | ||
Handle muting/unmuting of users | ||
""" | ||
assert event["type"] == "muted_users" | ||
self._update_muted_users(event["muted_users"]) | ||
|
||
def _handle_message_event(self, event: Event) -> None: | ||
""" | ||
Handle new messages (eg. add message to the end of the view) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,7 +96,9 @@ def __init__(self, message: Message, model: "Model", last_message: Any) -> None: | |
else: | ||
self.recipients_names = ", ".join( | ||
[ | ||
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 | ||
Comment on lines
-99
to
103
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
] | ||
|
@@ -687,7 +689,12 @@ 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"]) | ||
text["author"] = ( | ||
"msg_sender", | ||
"Muted user" | ||
if self.model.is_muted_user(self.message["sender_id"]) | ||
else message["this"]["author"], | ||
) | ||
Comment on lines
691
to
+697
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this only work for non-adjacent muted user messages? That is, does this cover the case where a muted user sends multiple messages in succession? |
||
|
||
# TODO: Refactor to use user ids for look up instead of emails. | ||
email = self.message.get("sender_email", "") | ||
|
@@ -729,10 +736,16 @@ def main_view(self) -> List[Any]: | |
"/me", f"<strong>{self.message['sender_full_name']}</strong>", 1 | ||
) | ||
|
||
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.model.is_muted_user(self.message["sender_id"]): | ||
content = (None, muted_message_text) | ||
Comment on lines
+746
to
+747
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're checking if it's a muted user every time, then it seems reasonable to place the check earlier, and only check for muting once?
Comment on lines
+739
to
+747
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd keep the muted_message_text close to the conditional. I was going to suggest the transform_content could be skipped, but it's also used for extracting eg. the links, so we should always do that - that may be worth a comment for future readers :) It'd be preferable to simplify the whole content-showing section, but I suspect that may need careful work, possibly with more tests, so for later. |
||
|
||
self.content.set_text(content) | ||
|
||
if self.message["id"] in self.model.index["edited_messages"]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -726,6 +726,8 @@ def users_view(self, users: Any = None) -> Any: | |
|
||
users_btn_list = list() | ||
for user in users: | ||
if self.view.model.is_muted_user(user["user_id"]): | ||
continue | ||
Comment on lines
728
to
+730
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
status = user["status"] | ||
# Only include `inactive` users in search result. | ||
if status == "inactive" and not self.view.controller.is_in_editor_mode(): | ||
|
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.