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
Open

Support (un)resolving topics #1544

Show file tree
Hide file tree
Changes from all commits
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
5 changes: 5 additions & 0 deletions docs/hotkeys.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@
|Show/hide user information|<kbd>i</kbd>|
|Narrow to direct messages with user|<kbd>Enter</kbd>|

## Topic list actions
|Command|Key Combination|
| :--- | :---: |
|Show/hide topic information & (un)resolve topic|<kbd>i</kbd>|

## Begin composing a message
|Command|Key Combination|
| :--- | :---: |
Expand Down
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
neiljp marked this conversation as resolved.
Show resolved Hide resolved
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
49 changes: 48 additions & 1 deletion tests/ui_tools/test_popups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from pytest_mock import MockerFixture
from urwid import Columns, Pile, Text, Widget

from zulipterminal.api_types import Message
from zulipterminal.api_types import RESOLVED_TOPIC_PREFIX, Message
from zulipterminal.config.keys import is_command_key, keys_for_command
from zulipterminal.config.ui_mappings import EDIT_MODE_CAPTIONS
from zulipterminal.helper import CustomProfileData, TidiedUserInfo
Expand All @@ -26,6 +26,7 @@
PopUpView,
StreamInfoView,
StreamMembersView,
TopicInfoView,
UserInfoView,
)
from zulipterminal.urwid_types import urwid_Size
Expand Down Expand Up @@ -1604,6 +1605,52 @@ def test_checkbox_toggle_visual_notification(
toggle_visual_notify_status.assert_called_once_with(stream_id)


class TestTopicInfoView:
@pytest.fixture(autouse=True)
def mock_external_classes(
self, mocker: MockerFixture, general_stream: Dict[str, Any], topics: List[str]
) -> None:
self.controller = mocker.Mock()

mocker.patch.object(
self.controller, "maximum_popup_dimensions", return_value=(64, 64)
)
mocker.patch(LISTWALKER, return_value=[])
self.stream_id = general_stream["stream_id"]
self.topic = topics[0]
self.controller.model.stream_dict = {self.stream_id: general_stream}

self.topic_info_view = TopicInfoView(
self.controller, self.stream_id, self.topic
)

@pytest.mark.parametrize(
"topic_name, expected_topic_button_label",
[
("hi!", "Resolve Topic"),
(f"{RESOLVED_TOPIC_PREFIX}" + "hi!", "Unresolve Topic"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Unless you plan to extract the 'hi!', this can be a combined f-string without the addition.

],
)
def test_topic_button_label(
self, topic_name: str, expected_topic_button_label: str
) -> None:
topic_info_view = TopicInfoView(self.controller, self.stream_id, topic_name)
assert (
topic_info_view.resolve_topic_setting_button_label
== expected_topic_button_label
)

def test_toggle_resolve_status(self) -> None:
resolve_button = self.topic_info_view.widgets[-1]
resolve_button._emit("click")

self.controller.model.toggle_topic_resolve_status.assert_called_once_with(
stream_id=self.stream_id, topic_name=self.topic
)

self.controller.exit_popup.assert_called_once()
Comment on lines +1643 to +1651
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you added this extra test, and the one above this?

I'm not sure about using _emit here, at least compared to our normal use of keypress, but that should be fine - what inspired _emit?

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 initially considered using keypress, but assumed the Enter key essentially uses ACTIVATE which triggers a click event in the context of an Urwid Button, just a NIT but thought it would be straightforward to use _emit.

I can change it back to use keypress with ACTIVATE_BUTTON

Urwid Docs

Send ‘click’ signal on ‘activate’ command.

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 fine, though I'll leave this comment unresolved since it may be useful to take this to a discussion of how best to integrate urwid and our own key handling further in this way.



class TestStreamMembersView:
@pytest.fixture(autouse=True)
def mock_external_classes(self, mocker: MockerFixture) -> None:
Expand Down
6 changes: 6 additions & 0 deletions zulipterminal/config/keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,11 @@ class KeyBinding(TypedDict):
'help_text': 'Show/hide stream information & modify settings',
'key_category': 'stream_list',
},
'TOPIC_INFO': {
'keys': ['i'],
'help_text': 'Show/hide topic information & (un)resolve topic',
'key_category': 'topic_list',
},
'STREAM_MEMBERS': {
'keys': ['m'],
'help_text': 'Show/hide stream members',
Expand Down Expand Up @@ -467,6 +472,7 @@ class KeyBinding(TypedDict):
"msg_actions": "Message actions",
"stream_list": "Stream list actions",
"user_list": "User list actions",
"topic_list": "Topic list actions",
"open_compose": "Begin composing a message",
"compose_box": "Writing a message",
"editor_navigation": "Editor: Navigation",
Expand Down
1 change: 1 addition & 0 deletions zulipterminal/config/themes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
'area:help' : 'standout',
'area:msg' : 'standout',
'area:stream' : 'standout',
'area:topic' : 'standout',
'area:error' : 'standout',
'area:user' : 'standout',
'search_error' : 'standout',
Expand Down
5 changes: 5 additions & 0 deletions zulipterminal/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
PopUpConfirmationView,
StreamInfoView,
StreamMembersView,
TopicInfoView,
UserInfoView,
)
from zulipterminal.version import ZT_VERSION
Expand Down Expand Up @@ -293,6 +294,10 @@ def show_stream_info(self, stream_id: int) -> None:
show_stream_view = StreamInfoView(self, stream_id)
self.show_pop_up(show_stream_view, "area:stream")

def show_topic_info(self, stream_id: int, topic_name: str) -> None:
show_topic_view = TopicInfoView(self, stream_id, topic_name)
self.show_pop_up(show_topic_view, "area:topic")

def show_stream_members(self, stream_id: int) -> None:
stream_members_view = StreamMembersView(self, stream_id)
self.show_pop_up(stream_members_view, "area:stream")
Comment on lines 295 to 303
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This would be better not between two stream-related functions.

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
1 change: 1 addition & 0 deletions zulipterminal/themes/gruvbox_dark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
'area:help' : (Color.DARK0_HARD, Color.BRIGHT_GREEN),
'area:msg' : (Color.DARK0_HARD, Color.NEUTRAL_PURPLE),
'area:stream' : (Color.DARK0_HARD, Color.BRIGHT_BLUE),
'area:topic' : (Color.DARK0_HARD, Color.BRIGHT_BLUE),
Comment on lines 70 to +71
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is useful to have distinct styles to enable changing them later, but I'm wondering if we should use different colors, rather than simply having duplicates of area:stream.

'area:error' : (Color.DARK0_HARD, Color.BRIGHT_RED),
'area:user' : (Color.DARK0_HARD, Color.BRIGHT_YELLOW),
'search_error' : (Color.BRIGHT_RED, Background.COLOR),
Expand Down
Loading
Loading