scheduled-messages: Split out edit scheduled message endpoint.

Part of splitting creating and editing scheduled messages.
Should be merged with final commit in series. Breaks tests.

Splits out editing an existing scheduled message into a new
view function and updated `edit_scheduled_message` function.

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
+    sender: UserProfile,
-) -> int:
+    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.
+        # If the server failed to send the scheduled message, a new scheduled
-        scheduled_message_object.recipient = send_request.message.recipient
+        # delivery timestamp (`deliver_at`) is required.
-        topic_name = send_request.message.topic_name()
+        if scheduled_message_object.failed and deliver_at is None:
-        scheduled_message_object.set_topic_name(topic_name=topic_name)
+            raise JsonableError(_("Scheduled delivery time must be in the future."))
-        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
 
-        scheduled_message_object.has_attachment = check_attachment_reference_change(
+        # Get existing scheduled message's recipient IDs and recipient_type_name.
-            scheduled_message_object, rendering_result
+        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:

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",
+            scheduled_message_id,
-            [self.get_stream_id("Verona")],
+            None,
+            None,
             "Edited test topic",
             "Edited stream message",
             convert_to_UTC(dateparser("2023-04-20 18:24:56")),

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(
+        content = "New message content"
-            "direct",
+        payload = {
-            [othello.id],
+            "content": content,
-            "New content!",
+            "scheduled_delivery_timestamp": new_delivery_timestamp,
-            new_delivery_timestamp,
+        }
-            scheduled_message_id=str(scheduled_message.id),
+        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
+            # After verifying the scheduled message failed to be sent:
-        # editing the scheduled message with that ID for a future time is
+            # 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(
+        payload_with_timestamp = {
-            "stream",
+            "scheduled_delivery_timestamp": new_delivery_timestamp,
-            verona_stream_id,
+        }
-            content + " 1",
+        response = self.client_patch(
-            new_delivery_timestamp,
+            f"/json/scheduled_messages/{scheduled_message.id}", payload_with_timestamp
-            scheduled_message_id=str(scheduled_message_id),
         )
         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)
         )
-
+        payload = {
-        result = self.do_schedule_message(
+            "content": edited_content,
-            "stream",
+            "scheduled_delivery_timestamp": new_scheduled_delivery_timestamp,
-            verona_stream_id,
+        }
-            edited_content,
+        result = self.client_patch(f"/json/scheduled_messages/{scheduled_message_id}", payload)
-            new_scheduled_delivery_timestamp,
-            scheduled_message_id=str(scheduled_message.id),
-        )
-        scheduled_message = self.get_scheduled_message(str(scheduled_message.id))
         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(
+        payload = {
-            "stream",
+            "content": edited_content,
-            verona_stream_id,
+        }
-            edited_content,
+        result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload)
-            scheduled_delivery_timestamp,
+
-            scheduled_message_id=str(scheduled_message.id),
-        )
         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(
+        payload = {
-            "stream",
+            "content": edited_content,
-            verona_stream_id,
+        }
-            edited_content,
+        result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload)
-            scheduled_delivery_timestamp,
+
-            scheduled_message_id=str(scheduled_message.id),
-        )
         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(
+        payload = {
-            "stream",
+            "content": edited_content,
-            verona_stream_id,
+        }
-            edited_content,
+        result = self.client_patch(f"/json/scheduled_messages/{scheduled_message.id}", payload)
-            scheduled_delivery_timestamp,
+
-            scheduled_message_id=str(scheduled_message.id),
-        )
         scheduled_message = self.get_scheduled_message(str(scheduled_message.id))
         self.assertEqual(
             list(attachment_object1.scheduled_messages.all().values_list("id", flat=True)), []

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)),

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/<int:draft_id>", 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(
-    rest_path("scheduled_messages/<int:scheduled_message_id>", DELETE=delete_scheduled_messages),
+        "scheduled_messages", GET=fetch_scheduled_messages, POST=create_scheduled_message_backend
+    ),
+    rest_path(
+        "scheduled_messages/<int:scheduled_message_id>",
+        DELETE=delete_scheduled_messages,
+        PATCH=update_scheduled_message_backend,
+    ),
     # messages -> zerver.views.message*
     # GET returns messages, possibly filtered, POST sends a message
     rest_path(