-
-
Notifications
You must be signed in to change notification settings - Fork 274
Add support for (un)resolving topics in ZT. #1235
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
base: main
Are you sure you want to change the base?
Changes from all commits
2b30a1d
0433e30
595260d
d3ebc5f
9f2c0da
b8e2a84
8504cfc
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
from pytest import param as case | ||
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 ( | ||
|
@@ -1097,11 +1098,11 @@ def test_get_latest_message_in_topic( | |
}, | ||
{ | ||
"_": ["User not found", False], | ||
"_owner": ["Editing messages is disabled", False], | ||
"_admin": ["Editing messages is disabled", False], | ||
"_moderator": ["Editing messages is disabled", False], | ||
"_member": ["Editing messages is disabled", False], | ||
"_guest": ["Editing messages is disabled", False], | ||
"_owner": [" Editing messages is disabled", False], | ||
"_admin": [" Editing messages is disabled", False], | ||
"_moderator": [" Editing messages is disabled", False], | ||
"_member": [" Editing messages is disabled", False], | ||
"_guest": [" Editing messages is disabled", False], | ||
}, | ||
id="editing_msg_disabled:PreZulip4.0", | ||
), | ||
|
@@ -1114,9 +1115,9 @@ def test_get_latest_message_in_topic( | |
"_": ["User not found", False], | ||
"_owner": ["", True], | ||
"_admin": ["", True], | ||
"_moderator": ["Editing topic is disabled", False], | ||
"_member": ["Editing topic is disabled", False], | ||
"_guest": ["Editing topic is disabled", False], | ||
"_moderator": [" Editing topic is disabled", False], | ||
"_member": [" Editing topic is disabled", False], | ||
"_guest": [" Editing topic is disabled", False], | ||
}, | ||
id="editing_topic_disabled:PreZulip4.0", | ||
), | ||
|
@@ -1135,18 +1136,6 @@ def test_get_latest_message_in_topic( | |
}, | ||
id="editing_topic_and_msg_enabled:PreZulip4.0", | ||
), | ||
case( | ||
{"allow_message_editing": False, "edit_topic_policy": 1}, | ||
{ | ||
"_": ["User not found", False], | ||
"_owner": ["Editing messages is disabled", False], | ||
"_admin": ["Editing messages is disabled", False], | ||
"_moderator": ["Editing messages is disabled", False], | ||
"_member": ["Editing messages is disabled", False], | ||
"_guest": ["Editing messages is disabled", False], | ||
}, | ||
id="all_but_no_editing:Zulip4.0+:ZFL75", | ||
), | ||
case( | ||
{"allow_message_editing": True, "edit_topic_policy": 1}, | ||
{ | ||
|
@@ -1156,8 +1145,8 @@ def test_get_latest_message_in_topic( | |
"_moderator": ["", True], | ||
"_member": ["", True], | ||
"_guest": [ | ||
"Only organization administrators, moderators, full members and" | ||
" members can edit topic", | ||
"Only organization administrators, moderators, full members" | ||
" and members can edit topic", | ||
False, | ||
], | ||
}, | ||
|
@@ -1208,13 +1197,13 @@ def test_get_latest_message_in_topic( | |
"_admin": ["", True], | ||
"_moderator": ["", True], | ||
"_member": [ | ||
"Only organization administrators and moderators" | ||
" can edit topic", | ||
"Only organization administrators and moderators can edit" | ||
" topic", | ||
False, | ||
], | ||
"_guest": [ | ||
"Only organization administrators and moderators" | ||
" can edit topic", | ||
"Only organization administrators and moderators can edit" | ||
" topic", | ||
False, | ||
], | ||
}, | ||
|
@@ -1246,9 +1235,9 @@ def test_can_user_edit_topic( | |
): | ||
model.get_user_info = mocker.Mock(return_value=user_role) | ||
model.initial_data = initial_data | ||
initial_data["realm_allow_message_editing"] = realm_editing_settings[ | ||
initial_data["realm_allow_message_editing"] = realm_editing_settings.get( | ||
"allow_message_editing" | ||
] | ||
) | ||
allow_community_topic_editing = realm_editing_settings.get( | ||
"allow_community_topic_editing", None | ||
) | ||
|
@@ -1271,6 +1260,87 @@ def test_can_user_edit_topic( | |
else: | ||
report_error.assert_called_once_with(expected_response[user_type][0]) | ||
|
||
@pytest.mark.parametrize( | ||
"topic_name, msg_response, server_feature_level, topic_editing_limit_seconds," | ||
" expected_new_topic_name, expected_footer_error", | ||
[ | ||
case( | ||
"hi!", | ||
{ | ||
"subject": "hi!", | ||
"timestamp": 11662271397, | ||
"id": 1, | ||
}, | ||
Comment on lines
+1269
to
+1273
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 dict appears identical in each case, except for the time, so let's build it in the function to keep it simpler? You could test for the |
||
12, | ||
259200, | ||
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. Is the timestamp within this time difference? The test case doesn't make it clear that's why we expect it to resolve. |
||
RESOLVED_TOPIC_PREFIX + "hi!", | ||
neiljp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
None, | ||
id="topic_resolved:Zulip2.1+:ZFL12", | ||
), | ||
neiljp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case( | ||
"hi!", | ||
{ | ||
"subject": "hi!", | ||
"timestamp": 0, | ||
"id": 1, | ||
}, | ||
None, | ||
None, | ||
RESOLVED_TOPIC_PREFIX + "hi!", | ||
None, | ||
id="no_time_limit:Zulip2.1+:None", | ||
), | ||
case( | ||
RESOLVED_TOPIC_PREFIX + "hi!", | ||
{ | ||
"subject": RESOLVED_TOPIC_PREFIX + "hi!", | ||
"timestamp": 11662271397, | ||
"id": 1, | ||
}, | ||
10, | ||
86400, | ||
"hi!", | ||
None, | ||
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 value is always |
||
id="topic_unresolved:Zulip2.1+:ZFL10", | ||
), | ||
], | ||
) | ||
def test_toggle_topic_resolve_status( | ||
self, | ||
mocker, | ||
model, | ||
initial_data, | ||
topic_name, | ||
msg_response, | ||
server_feature_level, | ||
topic_editing_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 | ||
# 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=msg_response) | ||
model.update_stream_message = mocker.Mock(return_value={"result": "success"}) | ||
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. See #1235 (comment) This does not need a return value. |
||
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=msg_response["id"], | ||
topic=expected_new_topic_name, | ||
propagate_mode="change_all", | ||
) | ||
else: | ||
report_error.assert_called_once_with(expected_footer_error) | ||
|
||
# NOTE: This tests only getting next-unread, not a fixed anchor | ||
def test_success_get_messages( | ||
self, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
|
||
from zulipterminal import unicode_emojis | ||
from zulipterminal.api_types import ( | ||
RESOLVED_TOPIC_PREFIX, | ||
Composition, | ||
EditPropagateMode, | ||
Event, | ||
|
@@ -648,7 +649,7 @@ def can_user_edit_topic(self) -> bool: | |
user_info = self.get_user_info(self.user_id) | ||
neiljp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if user_info is not None: | ||
if not self.initial_data.get("realm_allow_message_editing"): | ||
self.controller.report_error("Editing messages is disabled") | ||
self.controller.report_error(" Editing messages is disabled") | ||
return False | ||
role = user_info["role"] | ||
if role <= 200: | ||
|
@@ -661,7 +662,7 @@ def can_user_edit_topic(self) -> bool: | |
if allow_community_topic_editing is True: | ||
return True | ||
elif allow_community_topic_editing is False: | ||
self.controller.report_error("Editing topic is disabled") | ||
self.controller.report_error(" Editing topic is disabled") | ||
return False | ||
else: | ||
edit_topic_policy = self.initial_data.get("realm_edit_topic_policy") | ||
|
@@ -693,12 +694,44 @@ def can_user_edit_topic(self) -> bool: | |
else: | ||
self.controller.report_error(EDIT_TOPIC_POLICY[1]) | ||
return False | ||
else: # edit_topic_policy == 5 (or None) | ||
else: | ||
# All users including guests | ||
return True | ||
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 | ||
if self.server_feature_level is None or self.server_feature_level >= 11: | ||
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. See #1235 (comment) For server 2.1, this will set edit_time_limit to None. My impression is that the expected server behavior was
This appears to generate
Is this going to fail for 2.1? If so, could you update the test to fail first? |
||
edit_time_limit = self.initial_data.get( | ||
"realm_community_topic_editing_limit_seconds", None | ||
) | ||
else: | ||
edit_time_limit = 86400 | ||
# 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]]: | ||
|
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.
Why is this test case being removed?
Generally I think the changes in this revised first commit are otherwise minor enough that we can drop them? The rest appear to be:
In any case, if we do include parts in some way, since this would be a modification of the previous function, the commit title would need adjusting.