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

Jump to bottom

Support (un)resolving topics #1544

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
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
82e3b87
model: Add toggle_topic_resolve_status function to (un)resolve topics.
rsashank Sep 5, 2024
2242552
views: Add TopicInfoView to show pop up menu to change topic settings.
rsashank Sep 5, 2024
5630d9e
themes: Add 'area:topic' style for topic info popup menu.
rsashank Sep 5, 2024
b84dda6
core: Add show_topic_info function to show TopicInfoView pop up menu.
rsashank Sep 5, 2024
81c8f93
keys: Add TOPIC_INFO shortcut using 'i' key to show/hide topic settings.
rsashank Sep 5, 2024
a570d28
buttons: Add TOPIC_INFO shortcut to TopicButton.
rsashank Sep 5, 2024
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
134 changes: 134 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move the 'Fix' line from the first commit until the one when we complete the feature.

A tip I saw recently was to drop the '#' until it's merged, as it avoids spam on github issues.

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,139 @@ def test_can_user_edit_topic(
else:
report_error.assert_called_once_with(expected_response[user_type][0])

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

Choose a reason for hiding this comment

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

If the ZFL-dependent code isn't moved to the initial setup as per another comment, it would be clearer to have these in order of increasing ZFL to improve readability.

),
case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
0,
None,
None,
"hi!",
None,
id="topic_unresolved:Zulip2.1+:ZFL0",
),
case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
10,
86400,
None,
"hi!",
None,
id="topic_unresolved:Zulip2.1+:ZFL10",
),
case(
"hi!",
11662271397,
162,
None,
86400,
RESOLVED_TOPIC_PREFIX + "hi!",
None,
id="topic_resolved:Zulip7.0+:ZFL162",
),
case(
RESOLVED_TOPIC_PREFIX + "hi!",
11662271397,
162,
None,
259200,
"hi!",
None,
id="topic_unresolved:Zulip7.0+:ZFL162",
),
case(
"hi!",
0,
12,
86400,
None,
"hi!",
" Time limit for editing topic has been exceeded.",
id="time_limit_exceeded:Zulip2.1+:ZFL12",
),
case(
"hi!",
0,
162,
None,
259200,
"hi!",
" Time limit for editing topic has been exceeded.",
id="time_limit_exceeded:Zulip7.0+:ZFL162",
),
],
)
def test_toggle_topic_resolve_status(
self,
mocker,
model,
initial_data,
topic_name,
timestamp,
server_feature_level,
topic_editing_limit_seconds,
move_messages_within_stream_limit_seconds,
expected_new_topic_name,
expected_footer_error,
stream_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 +1467 to +1472
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.

# 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": timestamp,
Comment on lines +1476 to +1479
Copy link
Collaborator

Choose a reason for hiding this comment

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

This suggests the timestamp in the test should be a latest_message_timestamp, to make the field clearer?

The subject/topic is for the entire topic already, so that's clear enough.

"id": 1,
}
)
Comment on lines +1476 to +1482
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the building of the object here, given the id is considered the same, compared to the previous PR 👍 It's difficult to know what reads best in test cases in some situations though - eg. for the server settings (initial data).

Note that for this fixed id, it can be useful setting it as a default parameter, like the stream_id. That reduces the duplication in eg. the later assert here.

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)

if not expected_footer_error:
model.update_stream_message.assert_called_once_with(
message_id=1,
topic=expected_new_topic_name,
propagate_mode="change_all",
)
else:
report_error.assert_called_once_with(expected_footer_error)
Comment on lines +1488 to +1495
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can assert on each function in each branch.

You could split this test into two, which would avoid this conditional, but let's focus on the other comments on this commit first though.


# NOTE: This tests only getting next-unread, not a fixed anchor
def test_success_get_messages(
self,
Expand Down
39 changes: 39 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,44 @@ 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:
if self.can_user_edit_topic():
latest_msg = self.get_latest_message_in_topic(stream_id, topic_name)
if latest_msg:
time_since_msg_sent = time.time() - latest_msg["timestamp"]
# ZFL < 11, community_topic_editing_limit_seconds
# was hardcoded as int value in secs eg. 86400s (1 day) or None
Comment on lines +718 to +719
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment seems a little out of position relative to the code? I'd consider reordering the conditional to start with the new maximum, and leave the <11 case to the end with this comment?

if 11 <= self.server_feature_level < 162:
Comment on lines +714 to +720
Copy link
Collaborator

Choose a reason for hiding this comment

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

To reduce the level of nesting here, consider the matching else blocks for these early conditionals - they're basically error conditions (and we're not handling them, right now!)

In these cases, exiting early after performing those checks means the rest of the function knows it's in a good state and can continue. In terms of indenting, the current indented sections can then become an 'else' but at a non-indented level. (The absence of error handling then becomes fairly obvious too!)

edit_time_limit = self.initial_data.get(
"realm_community_topic_editing_limit_seconds", None
)
# ZFL >= 162, realm_move_messages_within_stream_limit_seconds was
# introduced in place of realm_community_topic_editing_limit_seconds
elif self.server_feature_level >= 162:
edit_time_limit = self.initial_data.get(
"realm_move_messages_within_stream_limit_seconds", None
)
else:
Comment on lines +724 to +730
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the new section, and the matching new test case, compared to the previous PR?

edit_time_limit = 86400
Comment on lines +718 to +731
Copy link
Collaborator

Choose a reason for hiding this comment

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

One aspect that would make the rest of this commit simpler would be to move this conditional so that it's run once when parsing initial_data, and putting it into a 'modern' form, a bit like we do with modernize_ing messages. For this work that could be via something like the _store_* methods, though they'll want tidying up and combining in future.

Right now the tests need to cover (un)resolving and time elapse checks, and all versus ZFL. With this change the ZFL test can be separate, in a small commit before this, and this function and these tests will be much smaller :)

# 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."
)
else:
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