diff --git a/zerver/actions/scheduled_messages.py b/zerver/actions/scheduled_messages.py index bda0ff9e32..8a6c073b4d 100644 --- a/zerver/actions/scheduled_messages.py +++ b/zerver/actions/scheduled_messages.py @@ -17,14 +17,16 @@ from zerver.actions.message_send import ( from zerver.actions.uploads import check_attachment_reference_change, do_claim_attachments from zerver.lib.addressee import Addressee from zerver.lib.exceptions import JsonableError, RealmDeactivatedError, UserDeactivatedError -from zerver.lib.message import SendMessageRequest, render_markdown +from zerver.lib.message import SendMessageRequest, render_markdown, truncate_topic from zerver.lib.scheduled_messages import access_scheduled_message +from zerver.lib.string_validation import check_stream_topic from zerver.models import ( Client, Realm, ScheduledMessage, Subscription, UserProfile, + get_recipient_ids, get_system_bot, ) from zerver.tornado.django_api import send_event @@ -141,8 +143,16 @@ def notify_update_scheduled_message( def edit_scheduled_message( - scheduled_message_id: int, send_request: SendMessageRequest, sender: UserProfile -) -> int: + sender: UserProfile, + client: Client, + scheduled_message_id: int, + recipient_type_name: Optional[str], + message_to: Optional[str], + topic_name: Optional[str], + message_content: Optional[str], + deliver_at: Optional[datetime.datetime], + realm: Realm, +) -> None: with transaction.atomic(): scheduled_message_object = access_scheduled_message(sender, scheduled_message_id) @@ -150,24 +160,94 @@ def edit_scheduled_message( if scheduled_message_object.delivered is True: raise JsonableError(_("Scheduled message was already sent")) - # Only override fields that user can change. - scheduled_message_object.recipient = send_request.message.recipient - topic_name = send_request.message.topic_name() - scheduled_message_object.set_topic_name(topic_name=topic_name) - rendering_result = render_markdown( - send_request.message, send_request.message.content, send_request.realm - ) - scheduled_message_object.content = send_request.message.content - scheduled_message_object.rendered_content = rendering_result.rendered_content - scheduled_message_object.sending_client = send_request.message.sending_client - scheduled_message_object.stream = send_request.stream - assert send_request.deliver_at is not None - scheduled_message_object.scheduled_timestamp = send_request.deliver_at + # If the server failed to send the scheduled message, a new scheduled + # delivery timestamp (`deliver_at`) is required. + if scheduled_message_object.failed and deliver_at is None: + raise JsonableError(_("Scheduled delivery time must be in the future.")) - scheduled_message_object.has_attachment = check_attachment_reference_change( - scheduled_message_object, rendering_result + # Get existing scheduled message's recipient IDs and recipient_type_name. + existing_recipient, existing_recipient_type_name = get_recipient_ids( + scheduled_message_object.recipient, sender.id ) + # If any recipient information or message content has been updated, + # we check the message again. + if recipient_type_name is not None or message_to is not None or message_content is not None: + # Update message type if changed. + if recipient_type_name is not None: + updated_recipient_type_name = recipient_type_name + else: + updated_recipient_type_name = existing_recipient_type_name + + # Update message recipient if changed. + if message_to is not None: + if updated_recipient_type_name == "stream": + updated_recipient = extract_stream_id(message_to) + else: + updated_recipient = extract_direct_message_recipient_ids(message_to) + else: + updated_recipient = existing_recipient + + # Update topic name if changed. + if topic_name is not None: + updated_topic = topic_name + else: + # This will be ignored in Addressee.legacy_build if type + # is being changed from stream to direct. + updated_topic = scheduled_message_object.topic_name() + + # Update message content if changed. + if message_content is not None: + updated_content = message_content + else: + updated_content = scheduled_message_object.content + + # Check message again. + addressee = Addressee.legacy_build( + sender, updated_recipient_type_name, updated_recipient, updated_topic + ) + send_request = check_message( + sender, + client, + addressee, + updated_content, + realm=realm, + forwarder_user_profile=sender, + ) + + if recipient_type_name is not None or message_to is not None: + # User has updated the scheduled message's recipient. + scheduled_message_object.recipient = send_request.message.recipient + scheduled_message_object.stream = send_request.stream + # Update the topic based on the new recipient information. + new_topic_name = send_request.message.topic_name() + scheduled_message_object.set_topic_name(topic_name=new_topic_name) + elif topic_name is not None and existing_recipient_type_name == "stream": + # User has updated the scheduled message's topic, but not + # the existing recipient information. We ignore topics sent + # for scheduled direct messages. + check_stream_topic(topic_name) + new_topic_name = truncate_topic(topic_name) + scheduled_message_object.set_topic_name(topic_name=new_topic_name) + + if message_content is not None: + # User has updated the scheduled messages's content. + rendering_result = render_markdown( + send_request.message, send_request.message.content, send_request.realm + ) + scheduled_message_object.content = send_request.message.content + scheduled_message_object.rendered_content = rendering_result.rendered_content + scheduled_message_object.has_attachment = check_attachment_reference_change( + scheduled_message_object, rendering_result + ) + + if deliver_at is not None: + # User has updated the scheduled message's send timestamp. + scheduled_message_object.scheduled_timestamp = deliver_at + + # Update for most recent Client information. + scheduled_message_object.sending_client = client + # If the user is editing a scheduled message that the server tried # and failed to send, we need to update the `failed` boolean field # as well as the associated `failure_message` field. @@ -178,7 +258,6 @@ def edit_scheduled_message( scheduled_message_object.save() notify_update_scheduled_message(sender, scheduled_message_object) - return scheduled_message_id def notify_remove_scheduled_message(user_profile: UserProfile, scheduled_message_id: int) -> None: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 196b93b015..712a4c4568 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -80,6 +80,7 @@ from zerver.actions.realm_settings import ( from zerver.actions.scheduled_messages import ( check_schedule_message, delete_scheduled_message, + edit_scheduled_message, ) from zerver.actions.streams import ( bulk_add_subscriptions, @@ -3348,11 +3349,12 @@ class ScheduledMessagesEventsTest(BaseAction): convert_to_UTC(dateparser("2023-04-19 18:24:56")), self.user_profile.realm, ) - action = lambda: check_schedule_message( + action = lambda: edit_scheduled_message( self.user_profile, get_client("website"), - "stream", - [self.get_stream_id("Verona")], + scheduled_message_id, + None, + None, "Edited test topic", "Edited stream message", convert_to_UTC(dateparser("2023-04-20 18:24:56")), diff --git a/zerver/tests/test_scheduled_messages.py b/zerver/tests/test_scheduled_messages.py index 854bee69f7..ccf1e1e5b8 100644 --- a/zerver/tests/test_scheduled_messages.py +++ b/zerver/tests/test_scheduled_messages.py @@ -2,7 +2,7 @@ import datetime import re import time from io import StringIO -from typing import TYPE_CHECKING, List, Union +from typing import TYPE_CHECKING, Any, Dict, List, Union from unittest import mock import orjson @@ -19,7 +19,7 @@ from zerver.lib.exceptions import JsonableError from zerver.lib.test_classes import ZulipTestCase from zerver.lib.test_helpers import most_recent_message from zerver.lib.timestamp import timestamp_to_datetime -from zerver.models import Attachment, Message, ScheduledMessage, UserMessage +from zerver.models import Attachment, Message, Recipient, ScheduledMessage, UserMessage if TYPE_CHECKING: from django.test.client import _MonkeyPatchedWSGIResponse as TestHttpResponse @@ -202,12 +202,13 @@ class ScheduledMessageTest(ZulipTestCase): # message is successfully sent. new_delivery_datetime = timezone_now() + datetime.timedelta(minutes=7) new_delivery_timestamp = int(new_delivery_datetime.timestamp()) - updated_response = self.do_schedule_message( - "direct", - [othello.id], - "New content!", - new_delivery_timestamp, - scheduled_message_id=str(scheduled_message.id), + content = "New message content" + payload = { + "content": content, + "scheduled_delivery_timestamp": new_delivery_timestamp, + } + updated_response = self.client_patch( + f"/json/scheduled_messages/{scheduled_message.id}", payload ) self.assert_json_error(updated_response, "Scheduled message was already sent") @@ -428,20 +429,25 @@ class ScheduledMessageTest(ZulipTestCase): scheduled_message, logger, expected_failure_message ) - # After verifying the scheduled message failed to be sent, confirm - # editing the scheduled message with that ID for a future time is + # After verifying the scheduled message failed to be sent: + # Confirm not updating the scheduled delivery timestamp for + # the scheduled message with that ID returns an error. + payload_without_timestamp = {"topic": "Failed to send"} + response = self.client_patch( + f"/json/scheduled_messages/{scheduled_message.id}", payload_without_timestamp + ) + self.assert_json_error(response, "Scheduled delivery time must be in the future.") + + # Editing the scheduled message with that ID for a future time is # successful and resets the `failed` and `failure_message` fields. new_delivery_datetime = timezone_now() + datetime.timedelta(minutes=60) new_delivery_timestamp = int(new_delivery_datetime.timestamp()) - content = "Test message" - verona_stream_id = self.get_stream_id("Verona") scheduled_message_id = scheduled_message.id - response = self.do_schedule_message( - "stream", - verona_stream_id, - content + " 1", - new_delivery_timestamp, - scheduled_message_id=str(scheduled_message_id), + payload_with_timestamp = { + "scheduled_delivery_timestamp": new_delivery_timestamp, + } + response = self.client_patch( + f"/json/scheduled_messages/{scheduled_message.id}", payload_with_timestamp ) self.assert_json_success(response) @@ -472,30 +478,124 @@ class ScheduledMessageTest(ZulipTestCase): ) scheduled_message = self.last_scheduled_message() self.assert_json_success(result) + self.assertEqual(scheduled_message.recipient.type, Recipient.STREAM) self.assertEqual(scheduled_message.content, "Original test message") self.assertEqual(scheduled_message.topic_name(), "Test topic") self.assertEqual( scheduled_message.scheduled_timestamp, timestamp_to_datetime(scheduled_delivery_timestamp), ) + scheduled_message_id = scheduled_message.id + payload: Dict[str, Any] - # Edit content and time of scheduled message. + # Sending request with only scheduled message ID returns an error + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}") + self.assert_json_error(result, "Nothing to change") + + # Trying to edit only message `type` returns an error + payload = { + "type": "direct", + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) + self.assert_json_error( + result, "Recipient required when updating type of scheduled message." + ) + + # Edit message `type` with valid `to` parameter succeeds + othello = self.example_user("othello") + to = [othello.id] + payload = {"type": "direct", "to": orjson.dumps(to).decode()} + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) + self.assert_json_success(result) + + scheduled_message = self.get_scheduled_message(str(scheduled_message_id)) + self.assertNotEqual(scheduled_message.recipient.type, Recipient.STREAM) + + # Trying to edit `topic` for direct message is ignored + payload = { + "topic": "Private message topic", + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) + self.assert_json_success(result) + + scheduled_message = self.get_scheduled_message(str(scheduled_message_id)) + self.assertEqual(scheduled_message.topic_name(), "") + + # Trying to edit `type` to stream message type without a `topic` returns an error + payload = { + "type": "stream", + "to": orjson.dumps(verona_stream_id).decode(), + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) + self.assert_json_error( + result, "Topic required when updating scheduled message type to stream." + ) + + # Edit message `type` to stream with valid `to` and `topic` succeeds + payload = { + "type": "stream", + "to": orjson.dumps(verona_stream_id).decode(), + "topic": "New test topic", + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) + self.assert_json_success(result) + + scheduled_message = self.get_scheduled_message(str(scheduled_message_id)) + self.assertEqual(scheduled_message.recipient.type, Recipient.STREAM) + self.assertEqual(scheduled_message.topic_name(), "New test topic") + + # Trying to edit with timestamp in the past returns an error + new_scheduled_delivery_timestamp = int(time.time() - 86400) + payload = { + "scheduled_delivery_timestamp": new_scheduled_delivery_timestamp, + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) + self.assert_json_error(result, "Scheduled delivery time must be in the future.") + + # Edit content and time of scheduled message succeeds edited_content = "Edited test message" new_scheduled_delivery_timestamp = scheduled_delivery_timestamp + int( time.time() + (3 * 86400) ) - - result = self.do_schedule_message( - "stream", - verona_stream_id, - edited_content, - new_scheduled_delivery_timestamp, - scheduled_message_id=str(scheduled_message.id), - ) - scheduled_message = self.get_scheduled_message(str(scheduled_message.id)) + payload = { + "content": edited_content, + "scheduled_delivery_timestamp": new_scheduled_delivery_timestamp, + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload) self.assert_json_success(result) + + scheduled_message = self.get_scheduled_message(str(scheduled_message_id)) self.assertEqual(scheduled_message.content, edited_content) - self.assertEqual(scheduled_message.topic_name(), "Test topic") + self.assertEqual(scheduled_message.topic_name(), "New test topic") + self.assertEqual( + scheduled_message.scheduled_timestamp, + timestamp_to_datetime(new_scheduled_delivery_timestamp), + ) + + # Edit topic and content of scheduled stream message + edited_content = "Final content edit for test" + payload = { + "topic": "Another topic for test", + "content": edited_content, + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload) + self.assert_json_success(result) + + scheduled_message = self.get_scheduled_message(str(scheduled_message.id)) + self.assertEqual(scheduled_message.content, edited_content) + self.assertEqual(scheduled_message.topic_name(), "Another topic for test") + + # Edit only topic of scheduled stream message + payload = { + "topic": "Final topic for test", + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload) + self.assert_json_success(result) + + scheduled_message = self.get_scheduled_message(str(scheduled_message.id)) + self.assertEqual(scheduled_message.recipient.type, Recipient.STREAM) + self.assertEqual(scheduled_message.content, edited_content) + self.assertEqual(scheduled_message.topic_name(), "Final topic for test") self.assertEqual( scheduled_message.scheduled_timestamp, timestamp_to_datetime(new_scheduled_delivery_timestamp), @@ -602,13 +702,11 @@ class ScheduledMessageTest(ZulipTestCase): # Test editing to change attachmment edited_content = f"Test [zulip.txt](http://{hamlet.realm.host}/user_uploads/{path_id2})" - result = self.do_schedule_message( - "stream", - verona_stream_id, - edited_content, - scheduled_delivery_timestamp, - scheduled_message_id=str(scheduled_message.id), - ) + payload = { + "content": edited_content, + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload) + scheduled_message = self.get_scheduled_message(str(scheduled_message.id)) self.assertEqual( list(attachment_object1.scheduled_messages.all().values_list("id", flat=True)), [] @@ -621,13 +719,11 @@ class ScheduledMessageTest(ZulipTestCase): # Test editing to no longer reference any attachments edited_content = "No more attachments" - result = self.do_schedule_message( - "stream", - verona_stream_id, - edited_content, - scheduled_delivery_timestamp, - scheduled_message_id=str(scheduled_message.id), - ) + payload = { + "content": edited_content, + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload) + scheduled_message = self.get_scheduled_message(str(scheduled_message.id)) self.assertEqual( list(attachment_object1.scheduled_messages.all().values_list("id", flat=True)), [] @@ -641,13 +737,11 @@ class ScheduledMessageTest(ZulipTestCase): edited_content = ( f"Attachment is back! [zulip.txt](http://{hamlet.realm.host}/user_uploads/{path_id2})" ) - result = self.do_schedule_message( - "stream", - verona_stream_id, - edited_content, - scheduled_delivery_timestamp, - scheduled_message_id=str(scheduled_message.id), - ) + payload = { + "content": edited_content, + } + result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload) + scheduled_message = self.get_scheduled_message(str(scheduled_message.id)) self.assertEqual( list(attachment_object1.scheduled_messages.all().values_list("id", flat=True)), [] diff --git a/zerver/views/scheduled_messages.py b/zerver/views/scheduled_messages.py index 7f8c1ce13e..c3ed3db3be 100644 --- a/zerver/views/scheduled_messages.py +++ b/zerver/views/scheduled_messages.py @@ -7,6 +7,7 @@ from django.utils.translation import gettext as _ from zerver.actions.scheduled_messages import ( check_schedule_message, delete_scheduled_message, + edit_scheduled_message, extract_direct_message_recipient_ids, extract_stream_id, ) @@ -16,7 +17,7 @@ from zerver.lib.response import json_success from zerver.lib.scheduled_messages import get_undelivered_scheduled_messages from zerver.lib.timestamp import timestamp_to_datetime from zerver.lib.topic import REQ_topic -from zerver.lib.validator import check_int, check_string_in +from zerver.lib.validator import check_int, check_string_in, to_non_negative_int from zerver.models import Message, UserProfile @@ -36,7 +37,76 @@ def delete_scheduled_messages( @has_request_variables -def scheduled_messages_backend( +def update_scheduled_message_backend( + request: HttpRequest, + user_profile: UserProfile, + scheduled_message_id: int = REQ(converter=to_non_negative_int, path_only=True), + req_type: Optional[str] = REQ( + "type", str_validator=check_string_in(Message.API_RECIPIENT_TYPES), default=None + ), + req_to: Optional[str] = REQ("to", default=None), + topic_name: Optional[str] = REQ_topic(), + message_content: Optional[str] = REQ("content", default=None), + scheduled_delivery_timestamp: Optional[int] = REQ(json_validator=check_int, default=None), +) -> HttpResponse: + if ( + req_type is None + and req_to is None + and topic_name is None + and message_content is None + and scheduled_delivery_timestamp is None + ): + raise JsonableError(_("Nothing to change")) + + recipient_type_name = None + if req_type: + if req_to is None: + raise JsonableError(_("Recipient required when updating type of scheduled message.")) + else: + recipient_type_name = req_type + + if recipient_type_name is not None and recipient_type_name == "stream" and topic_name is None: + raise JsonableError(_("Topic required when updating scheduled message type to stream.")) + + if recipient_type_name is not None and recipient_type_name == "direct": + # For now, use "private" from Message.API_RECIPIENT_TYPES. + # TODO: Use "direct" here, as well as in events and + # scheduled message objects/dicts. + recipient_type_name = "private" + + message_to = None + if req_to is not None: + # Because the recipient_type_name may not be updated/changed, + # we extract these updated recipient IDs in edit_scheduled_message. + message_to = req_to + + deliver_at = None + if scheduled_delivery_timestamp is not None: + deliver_at = timestamp_to_datetime(scheduled_delivery_timestamp) + if deliver_at <= timezone_now(): + raise JsonableError(_("Scheduled delivery time must be in the future.")) + + sender = user_profile + client = RequestNotes.get_notes(request).client + assert client is not None + + edit_scheduled_message( + sender, + client, + scheduled_message_id, + recipient_type_name, + message_to, + topic_name, + message_content, + deliver_at, + realm=user_profile.realm, + ) + + return json_success(request) + + +@has_request_variables +def create_scheduled_message_backend( request: HttpRequest, user_profile: UserProfile, req_type: str = REQ("type", str_validator=check_string_in(Message.API_RECIPIENT_TYPES)), diff --git a/zproject/urls.py b/zproject/urls.py index e537b2d2c1..e32deb041c 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -134,9 +134,10 @@ from zerver.views.report import ( report_csp_violations, ) from zerver.views.scheduled_messages import ( + create_scheduled_message_backend, delete_scheduled_messages, fetch_scheduled_messages, - scheduled_messages_backend, + update_scheduled_message_backend, ) from zerver.views.sentry import sentry_tunnel from zerver.views.storage import get_storage, remove_storage, update_storage @@ -324,8 +325,14 @@ v1_api_and_json_patterns = [ rest_path("drafts", GET=fetch_drafts, POST=create_drafts), rest_path("drafts/", PATCH=edit_draft, DELETE=delete_draft), # New scheduled messages are created via send_message_backend. - rest_path("scheduled_messages", GET=fetch_scheduled_messages, POST=scheduled_messages_backend), - rest_path("scheduled_messages/", DELETE=delete_scheduled_messages), + rest_path( + "scheduled_messages", GET=fetch_scheduled_messages, POST=create_scheduled_message_backend + ), + rest_path( + "scheduled_messages/", + DELETE=delete_scheduled_messages, + PATCH=update_scheduled_message_backend, + ), # messages -> zerver.views.message* # GET returns messages, possibly filtered, POST sends a message rest_path(