mirror of
https://github.com/zulip/zulip.git
synced 2026-07-09 21:21:47 +08:00
message edit: Extract data validity checks from check_update_message.
These checks are more related to the API than the editability or permissions logic, so it makes sense to handle them first before further processing the request. Also split the main test class to separate out the tests for this logic. This also simplifies some tests by reducing the data setup required to reach failure. Tweaked by tabbott to avoid losing the topic_name.strip().
This commit is contained in:
Abhijeet Prasad Bodas
Tim Abbott
parent
10be798a3c
commit
4b30fc01e4
@ -2703,6 +2703,37 @@ def check_schedule_message(
|
||||
return do_schedule_messages([send_request])[0]
|
||||
|
||||
|
||||
def validate_message_edit_payload(
|
||||
message: Message,
|
||||
stream_id: Optional[int],
|
||||
topic_name: Optional[str],
|
||||
propagate_mode: Optional[str],
|
||||
content: Optional[str],
|
||||
) -> None:
|
||||
"""
|
||||
Checks that the data sent is well-formed. Does not handle editability, permissions etc.
|
||||
"""
|
||||
if topic_name is None and content is None and stream_id is None:
|
||||
raise JsonableError(_("Nothing to change"))
|
||||
|
||||
if not message.is_stream_message():
|
||||
if stream_id is not None:
|
||||
raise JsonableError(_("Private messages cannot be moved to streams."))
|
||||
|
||||
if propagate_mode != "change_one" and topic_name is None and stream_id is None:
|
||||
raise JsonableError(_("Invalid propagate_mode without topic edit"))
|
||||
|
||||
if topic_name == "":
|
||||
raise JsonableError(_("Topic can't be empty"))
|
||||
|
||||
if stream_id is not None and content is not None:
|
||||
raise JsonableError(_("Cannot change message content while changing stream"))
|
||||
|
||||
# Right now, we prevent users from editing widgets.
|
||||
if content is not None and is_widget_message(message):
|
||||
raise JsonableError(_("Widgets cannot be edited."))
|
||||
|
||||
|
||||
def check_update_message(
|
||||
user_profile: UserProfile,
|
||||
message_id: int,
|
||||
@ -2718,13 +2749,19 @@ def check_update_message(
|
||||
and raises a JsonableError if otherwise.
|
||||
It returns the number changed.
|
||||
"""
|
||||
message, ignored_user_message = access_message(user_profile, message_id)
|
||||
|
||||
if not user_profile.realm.allow_message_editing:
|
||||
raise JsonableError(_("Your organization has turned off message editing"))
|
||||
|
||||
if propagate_mode != "change_one" and topic_name is None and stream_id is None:
|
||||
raise JsonableError(_("Invalid propagate_mode without topic edit"))
|
||||
# The zerver/views/message_edit.py callpoint already strips this
|
||||
# via REQ_topic; so we can delete this line if we arrange a
|
||||
# contract where future callers in the embedded bots system strip
|
||||
# use REQ_topic as well (or otherwise are guaranteed to strip input).
|
||||
if topic_name is not None:
|
||||
topic_name = topic_name.strip()
|
||||
validate_message_edit_payload(message, stream_id, topic_name, propagate_mode, content)
|
||||
|
||||
message, ignored_user_message = access_message(user_profile, message_id)
|
||||
is_no_topic_msg = message.topic_name() == "(no topic)"
|
||||
|
||||
# You only have permission to edit a message if:
|
||||
@ -2744,10 +2781,6 @@ def check_update_message(
|
||||
else:
|
||||
raise JsonableError(_("You don't have permission to edit this message"))
|
||||
|
||||
# Right now, we prevent users from editing widgets.
|
||||
if content is not None and is_widget_message(message):
|
||||
raise JsonableError(_("Widgets cannot be edited."))
|
||||
|
||||
# If there is a change to the content, check that it hasn't been too long
|
||||
# Allow an extra 20 seconds since we potentially allow editing 15 seconds
|
||||
# past the limit, and in case there are network issues, etc. The 15 comes
|
||||
@ -2773,12 +2806,6 @@ def check_update_message(
|
||||
if (timezone_now() - message.date_sent) > datetime.timedelta(seconds=deadline_seconds):
|
||||
raise JsonableError(_("The time limit for editing this message has passed"))
|
||||
|
||||
if topic_name is None and content is None and stream_id is None:
|
||||
raise JsonableError(_("Nothing to change"))
|
||||
if topic_name is not None:
|
||||
topic_name = topic_name.strip()
|
||||
if topic_name == "":
|
||||
raise JsonableError(_("Topic can't be empty"))
|
||||
rendered_content = None
|
||||
links_for_embed: Set[str] = set()
|
||||
prior_mention_user_ids: Set[int] = set()
|
||||
@ -2815,8 +2842,7 @@ def check_update_message(
|
||||
number_changed = 0
|
||||
|
||||
if stream_id is not None:
|
||||
if not message.is_stream_message():
|
||||
raise JsonableError(_("Message must be a stream message"))
|
||||
assert message.is_stream_message()
|
||||
if not user_profile.can_move_messages_between_streams():
|
||||
raise JsonableError(_("You don't have permission to move this message"))
|
||||
try:
|
||||
@ -2827,8 +2853,6 @@ def check_update_message(
|
||||
"You don't have permission to move this message due to missing access to its stream"
|
||||
)
|
||||
)
|
||||
if content is not None:
|
||||
raise JsonableError(_("Cannot change message content while changing stream"))
|
||||
|
||||
new_stream = access_stream_by_id(user_profile, stream_id, require_active=True)[0]
|
||||
check_stream_access_based_on_stream_post_policy(user_profile, new_stream)
|
||||
|
||||
@ -95,6 +95,117 @@ class EditMessageTestCase(ZulipTestCase):
|
||||
return (user_profile, stream, new_stream, msg_id, msg_id_lt)
|
||||
|
||||
|
||||
class EditMessagePayloadTest(EditMessageTestCase):
|
||||
def test_edit_message_no_changes(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit"
|
||||
)
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Nothing to change")
|
||||
|
||||
def test_move_message_cant_move_private_message(self) -> None:
|
||||
hamlet = self.example_user("hamlet")
|
||||
self.login("hamlet")
|
||||
cordelia = self.example_user("cordelia")
|
||||
msg_id = self.send_personal_message(hamlet, cordelia)
|
||||
|
||||
verona = get_stream("Verona", hamlet.realm)
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"stream_id": verona.id,
|
||||
},
|
||||
)
|
||||
|
||||
self.assert_json_error(result, "Private messages cannot be moved to streams.")
|
||||
|
||||
def test_propagate_invalid(self) -> None:
|
||||
self.login("hamlet")
|
||||
id1 = self.send_stream_message(self.example_user("hamlet"), "Scotland", topic_name="topic1")
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(id1),
|
||||
{
|
||||
"topic": "edited",
|
||||
"propagate_mode": "invalid",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Invalid propagate_mode")
|
||||
self.check_topic(id1, topic_name="topic1")
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(id1),
|
||||
{
|
||||
"content": "edited",
|
||||
"propagate_mode": "change_all",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Invalid propagate_mode without topic edit")
|
||||
self.check_topic(id1, topic_name="topic1")
|
||||
|
||||
def test_edit_message_no_topic(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit"
|
||||
)
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"topic": " ",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Topic can't be empty")
|
||||
|
||||
def test_move_message_to_stream_with_content(self) -> None:
|
||||
(user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
|
||||
"iago", "test move stream", "new stream", "test"
|
||||
)
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"stream_id": new_stream.id,
|
||||
"propagate_mode": "change_all",
|
||||
"content": "Not allowed",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Cannot change message content while changing stream")
|
||||
|
||||
messages = get_topic_messages(user_profile, old_stream, "test")
|
||||
self.assert_length(messages, 3)
|
||||
|
||||
messages = get_topic_messages(user_profile, new_stream, "test")
|
||||
self.assert_length(messages, 0)
|
||||
|
||||
# Right now, we prevent users from editing widgets.
|
||||
def test_edit_submessage(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
self.example_user("hamlet"),
|
||||
"Scotland",
|
||||
topic_name="editing",
|
||||
content="/poll Games?\nYES\nNO",
|
||||
)
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"content": "/poll Games?\nYES\nNO\nMaybe",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Widgets cannot be edited.")
|
||||
|
||||
|
||||
class EditMessageTest(EditMessageTestCase):
|
||||
def test_query_count_on_to_dict_uncached(self) -> None:
|
||||
# `to_dict_uncached` method is used by the mechanisms
|
||||
@ -211,24 +322,6 @@ class EditMessageTest(EditMessageTestCase):
|
||||
result = self.client_get("/json/messages/" + str(msg_id))
|
||||
self.assert_json_error(result, "Invalid message(s)")
|
||||
|
||||
# Right now, we prevent users from editing widgets.
|
||||
def test_edit_submessage(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
self.example_user("hamlet"),
|
||||
"Scotland",
|
||||
topic_name="editing",
|
||||
content="/poll Games?\nYES\nNO",
|
||||
)
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"content": "/poll Games?\nYES\nNO\nMaybe",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Widgets cannot be edited.")
|
||||
|
||||
def test_edit_message_no_permission(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
@ -243,33 +336,6 @@ class EditMessageTest(EditMessageTestCase):
|
||||
)
|
||||
self.assert_json_error(result, "You don't have permission to edit this message")
|
||||
|
||||
def test_edit_message_no_changes(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit"
|
||||
)
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Nothing to change")
|
||||
|
||||
def test_edit_message_no_topic(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
self.example_user("hamlet"), "Scotland", topic_name="editing", content="before edit"
|
||||
)
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"topic": " ",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Topic can't be empty")
|
||||
|
||||
def test_edit_message_no_content(self) -> None:
|
||||
self.login("hamlet")
|
||||
msg_id = self.send_stream_message(
|
||||
@ -1094,30 +1160,6 @@ class EditMessageTest(EditMessageTestCase):
|
||||
self.check_topic(id3, topic_name="topiC1")
|
||||
self.check_topic(id4, topic_name="edited")
|
||||
|
||||
def test_propagate_invalid(self) -> None:
|
||||
self.login("hamlet")
|
||||
id1 = self.send_stream_message(self.example_user("hamlet"), "Scotland", topic_name="topic1")
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(id1),
|
||||
{
|
||||
"topic": "edited",
|
||||
"propagate_mode": "invalid",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Invalid propagate_mode")
|
||||
self.check_topic(id1, topic_name="topic1")
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(id1),
|
||||
{
|
||||
"content": "edited",
|
||||
"propagate_mode": "change_all",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Invalid propagate_mode without topic edit")
|
||||
self.check_topic(id1, topic_name="topic1")
|
||||
|
||||
def test_move_message_to_stream(self) -> None:
|
||||
(user_profile, old_stream, new_stream, msg_id, msg_id_lt) = self.prepare_move_topics(
|
||||
"iago", "test move stream", "new stream", "test"
|
||||
@ -1270,29 +1312,6 @@ class EditMessageTest(EditMessageTestCase):
|
||||
private_stream.recipient_id,
|
||||
)
|
||||
|
||||
def test_move_message_cant_move_private_message(
|
||||
self,
|
||||
) -> None:
|
||||
user_profile = self.example_user("iago")
|
||||
self.assertEqual(user_profile.role, UserProfile.ROLE_REALM_ADMINISTRATOR)
|
||||
self.login("iago")
|
||||
|
||||
hamlet = self.example_user("hamlet")
|
||||
msg_id = self.send_personal_message(user_profile, hamlet)
|
||||
|
||||
verona = get_stream("Verona", user_profile.realm)
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"stream_id": verona.id,
|
||||
"propagate_mode": "change_all",
|
||||
},
|
||||
)
|
||||
|
||||
self.assert_json_error(result, "Message must be a stream message")
|
||||
|
||||
def test_move_message_to_stream_change_later(self) -> None:
|
||||
(user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
|
||||
"iago", "test move stream", "new stream", "test"
|
||||
@ -1493,28 +1512,6 @@ class EditMessageTest(EditMessageTestCase):
|
||||
)
|
||||
check_move_message_to_stream(UserProfile.ROLE_MEMBER)
|
||||
|
||||
def test_move_message_to_stream_with_content(self) -> None:
|
||||
(user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
|
||||
"iago", "test move stream", "new stream", "test"
|
||||
)
|
||||
|
||||
result = self.client_patch(
|
||||
"/json/messages/" + str(msg_id),
|
||||
{
|
||||
"message_id": msg_id,
|
||||
"stream_id": new_stream.id,
|
||||
"propagate_mode": "change_all",
|
||||
"content": "Not allowed",
|
||||
},
|
||||
)
|
||||
self.assert_json_error(result, "Cannot change message content while changing stream")
|
||||
|
||||
messages = get_topic_messages(user_profile, old_stream, "test")
|
||||
self.assert_length(messages, 3)
|
||||
|
||||
messages = get_topic_messages(user_profile, new_stream, "test")
|
||||
self.assert_length(messages, 0)
|
||||
|
||||
def test_move_message_to_stream_and_topic(self) -> None:
|
||||
(user_profile, old_stream, new_stream, msg_id, msg_id_later) = self.prepare_move_topics(
|
||||
"iago", "test move stream", "new stream", "test"
|
||||
|
||||
Loading…
Reference in New Issue
Block a user