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.

Sign up for GitHub

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 (un)resolving topics #1544

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+368 −2
Open

Support (un)resolving topics #1544

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b7eb6b
model: Add toggle_topic_resolve_status function to (un)resolve topics.
rsashank Sep 5, 2024
59b9ab9
views: Add TopicInfoView to show pop up menu to change topic settings.
rsashank Sep 5, 2024
16cff3b
themes: Add 'area:topic' style for topic info popup menu.
srdeotarse Apr 8, 2023
7fdf686
core: Add show_topic_info function to show TopicInfoView pop up menu.
srdeotarse Apr 8, 2023
1c85d9d
keys: Add TOPIC_INFO shortcut using 'i' key to show/hide topic settings.
rsashank Sep 5, 2024
76fafdf
buttons: Add TOPIC_INFO shortcut to TopicButton.
srdeotarse Apr 8, 2023
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
195 changes: 195 additions & 0 deletions tests/model/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from pytest_mock import MockerFixture
from zulip import Client, ZulipError

from zulipterminal.api_types import RESOLVED_TOPIC_PREFIX
from zulipterminal.config.symbols import STREAM_TOPIC_SEPARATOR
from zulipterminal.helper import initial_index, powerset
from zulipterminal.model import (
Expand Down Expand Up @@ -1360,6 +1361,200 @@ def test_can_user_edit_topic(
else:
report_error.assert_called_once_with(expected_response[user_type][0])

@pytest.mark.parametrize(
"topic_name, latest_message_timestamp, server_feature_level,"
" topic_editing_limit_seconds, move_messages_within_stream_limit_seconds,"
" expected_new_topic_name",
[
case(
"hi!",
11662271397,
0,
None,
None,
RESOLVED_TOPIC_PREFIX + "hi!",
id="topic_resolved:Zulip2.1+:ZFL0",
),
case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
0,
None,
None,
"hi!",
id="topic_unresolved:Zulip2.1+:ZFL0",
),
Comment on lines +1369 to +1386
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in another comment, if we wish to fully test resolve/unresolve for each of these cases, the resolve/unresolve part could be a nested parametrize, since they are independent.

So you end up with a matrix of parameters from something like the equivalent of

@pytest.mark.parametrize("param1, param2", ...)
@pytest.mark.parametrize("a, b, c, d", ...)
def test_something(...)

case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
10,
86400,
None,
"hi!",
id="topic_unresolved:Zulip2.1+:ZFL10",
),
case(
"hi!",
11662271397,
12,
259200,
None,
RESOLVED_TOPIC_PREFIX + "hi!",
id="topic_resolved:Zulip2.1+:ZFL12",
),
case(
"hi!",
11662271397,
162,
86400,
86400,
RESOLVED_TOPIC_PREFIX + "hi!",
id="topic_resolved:Zulip7.0+:ZFL162",
),
case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
162,
259200,
259200,
"hi!",
id="topic_unresolved:Zulip7.0+:ZFL162",
),
case(
"hi!",
11662271397,
184,
259200,
259200,
RESOLVED_TOPIC_PREFIX + "hi!",
id="topic_unresolved:Zulip7.0+:ZFL184",
),
case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
184,
259200,
259200,
"hi!",
id="topic_unresolved:Zulip7.0+:ZFL184",
),
],
)
def test_toggle_topic_resolve_status_no_footer_error(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We've been switching towards a double underscore between the function name and the test meaning/motivation - it makes it clearer which part is which. (same elsewhere)

self,
mocker,
model,
initial_data,
topic_name,
latest_message_timestamp,
server_feature_level,
topic_editing_limit_seconds,
move_messages_within_stream_limit_seconds,
expected_new_topic_name,
stream_id=1,
message_id=1,
):
model.initial_data = initial_data
model.server_feature_level = server_feature_level
initial_data[
"realm_community_topic_editing_limit_seconds"
] = topic_editing_limit_seconds
initial_data[
"realm_move_messages_within_stream_limit_seconds"
] = move_messages_within_stream_limit_seconds
# If user can't edit topic, topic (un)resolve is disabled. Therefore,
# default return_value=True
model.can_user_edit_topic = mocker.Mock(return_value=True)
model.get_latest_message_in_topic = mocker.Mock(
return_value={
"subject": topic_name,
"timestamp": latest_message_timestamp,
"id": message_id,
}
)
model.update_stream_message = mocker.Mock(return_value={"result": "success"})

model.toggle_topic_resolve_status(stream_id, topic_name)

model.update_stream_message.assert_called_once_with(
message_id=message_id,
topic=expected_new_topic_name,
propagate_mode="change_all",
)

@pytest.mark.parametrize(
"topic_name, latest_message_timestamp, server_feature_level,"
" topic_editing_limit_seconds, move_messages_within_stream_limit_seconds,"
" expected_footer_error,",
[
case(
"hi!",
0,
12,
86400,
None,
" Time limit for editing topic has been exceeded.",
id="time_limit_exceeded:Zulip2.1+:ZFL12",
),
case(
"hi!",
0,
162,
259200,
259200,
" Time limit for editing topic has been exceeded.",
id="time_limit_exceeded:Zulip7.0+:ZFL162",
),
case(
"hi!",
0,
184,
None,
86400,
" Time limit for editing topic has been exceeded.",
id="topic_resolved:Zulip2.1+:ZFL184",
Comment on lines +1490 to +1515
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at these cases alone, it's not clear to me what's different in each one?

  • Except for the last case (with incorrect id?) the id starts with time_limit_exceeded; that's currently implied by the test, though might not be, if you extend the code to handle other errors.
  • Does the topic name need to be parametrized? We're not testing unresolving as it stands, but could parametrize over that separately instead (using a nested decorator).
  • Does the latest_message_timestamp matter? It appears to be zero in every case, which is constant among the cases (can it be a constant like message_id above?), but it's unclear what that value is trying to achieve?
  • Is the error string a constant between each case? (again, it may not be if the error handling is extended)

),
],
)
def test_toggle_topic_resolve_status_footer_error(
self,
mocker,
model,
initial_data,
topic_name,
latest_message_timestamp,
server_feature_level,
topic_editing_limit_seconds,
move_messages_within_stream_limit_seconds,
expected_footer_error,
stream_id=1,
message_id=1,
):
model.initial_data = initial_data
model.server_feature_level = server_feature_level
initial_data[
"realm_community_topic_editing_limit_seconds"
] = topic_editing_limit_seconds
initial_data[
"realm_move_messages_within_stream_limit_seconds"
] = move_messages_within_stream_limit_seconds
Comment on lines +1535 to +1540
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly this doesn't cover the ZFL 183+ (Zulip 7) situation, where the first of these parameters was removed?

We could treat the two parameters as a patch to initial data via a dict, but unfortunately that then leaves assumptions as to whether the two values are in initial_data, ie. for later versions we strictly want to remove the field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added cases for ZFL184, where the value of realm_community_topic_editing_limit_seconds is None.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that some fields are actually missing at some feature levels, not that they are None, or I'd at least expect that at early server versions - and if an option is actually removed at a later version?

# If user can't edit topic, topic (un)resolve is disabled. Therefore,
# default return_value=True
model.can_user_edit_topic = mocker.Mock(return_value=True)
model.get_latest_message_in_topic = mocker.Mock(
return_value={
"subject": topic_name,
"timestamp": latest_message_timestamp,
"id": message_id,
}
)
model.update_stream_message = mocker.Mock(return_value={"result": "success"})
report_error = model.controller.report_error

model.toggle_topic_resolve_status(stream_id, topic_name)

report_error.assert_called_once_with(expected_footer_error)
Copy link
Collaborator

@neiljp neiljp Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For comparison with the previous test, we can also assert that the update method is not called, and vice versa in the other test.

I mentioned this point indirectly when the tests were combined - each branch can assert on each element.


# NOTE: This tests only getting next-unread, not a fixed anchor
def test_success_get_messages(
self,
Expand Down
40 changes: 40 additions & 0 deletions zulipterminal/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
MAX_TOPIC_NAME_LENGTH,
PRESENCE_OFFLINE_THRESHOLD_SECS,
PRESENCE_PING_INTERVAL_SECS,
RESOLVED_TOPIC_PREFIX,
TYPING_STARTED_EXPIRY_PERIOD,
TYPING_STARTED_WAIT_PERIOD,
TYPING_STOPPED_WAIT_PERIOD,
Expand Down Expand Up @@ -709,6 +710,45 @@ def can_user_edit_topic(self) -> bool:
self.controller.report_error("User not found")
return False

def toggle_topic_resolve_status(self, stream_id: int, topic_name: str) -> None:
latest_msg = self.get_latest_message_in_topic(stream_id, topic_name)
if not self.can_user_edit_topic() or not latest_msg:
return
Comment on lines +714 to +716
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should report errors, based on how the function currently works - this was the error handling I mentioned was absent, and would be more obvious if restructured to be like this.


time_since_msg_sent = time.time() - latest_msg["timestamp"]

# ZFL >= 162, realm_move_messages_within_stream_limit_seconds was
# introduced in place of realm_community_topic_editing_limit_seconds
if self.server_feature_level >= 162:
edit_time_limit = self.initial_data.get(
"realm_move_messages_within_stream_limit_seconds", None
)
elif 11 <= self.server_feature_level < 162:
edit_time_limit = self.initial_data.get(
"realm_community_topic_editing_limit_seconds", None
)
# ZFL < 11, community_topic_editing_limit_seconds
# was hardcoded as int value in secs eg. 86400s (1 day) or None
else:
edit_time_limit = 86400
Comment on lines +730 to +733
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this comment was moved, but it doesn't map to the value being None?

My related question is whether edit_time_limit can end up being None for a well-specified server? It's checked against None below, which might just be related to the .get calls above, or if the 'seconds' values can actually have a None/null value that has a distinct meaning.


# Don't allow editing topic if time-limit exceeded.
if edit_time_limit is not None and time_since_msg_sent >= edit_time_limit:
self.controller.report_error(
" Time limit for editing topic has been exceeded."
)
return

if topic_name.startswith(RESOLVED_TOPIC_PREFIX):
topic_name = topic_name[2:]
else:
topic_name = RESOLVED_TOPIC_PREFIX + topic_name
self.update_stream_message(
message_id=latest_msg["id"],
topic=topic_name,
propagate_mode="change_all",
)

def generate_all_emoji_data(
self, custom_emoji: Dict[str, RealmEmojiData]
) -> Tuple[NamedEmojiData, List[str]]:
Expand Down