Skip to content
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

Support muted users #1546

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,7 @@ def initial_data(
}
],
"result": "success",
"muted_users": {},
Copy link
Collaborator

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.

"queue_id": "1522420755:786",
"realm_users": users_fixture,
"cross_realm_bots": [
Expand Down
31 changes: 31 additions & 0 deletions tests/model/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Collaborator

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 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 :)

[
(
[
{"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
Copy link
Collaborator

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.

)
def test_init_muted_users(
self, mocker, initial_data, server_response, ids, zulip_feature_level
):
Comment on lines +211 to +213
Copy link
Collaborator

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 👍

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")
Expand Down Expand Up @@ -262,6 +292,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,
Expand Down
9 changes: 9 additions & 0 deletions zulipterminal/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -208,6 +209,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]] = {}
Expand Down Expand Up @@ -1204,6 +1210,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
Copy link
Collaborator

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 _update_users_data_from_initial_data(self) -> None:
# Dict which stores the active/idle status of users (by email)
presences = self.initial_data["presences"]
Expand Down