From 998437c123b4533536cc7a3b1a4d1c53551537dd Mon Sep 17 00:00:00 2001 From: Eeshan Garg Date: Mon, 28 Jan 2019 00:58:29 -0330 Subject: [PATCH] check_message: Support sending stream messages by ID. This commit also contains the following auxiliary changes: * Adds a custom exception, StreamWithIDDoesNotExist for when a stream with a given ID does not exist because the error message returned by StreamDoesNotExist only makes with stream names, not IDs. * Adds a new helper, get_stream_by_id_in_realm, which is similar to get_user_profile_by_id_in_realm (introduced in #10391). * Adds a helper, validate_stream_id_with_pm_notification, which returns the Stream object associated with a given ID and also handles PM notifications to the bot owner if the message was sent by a bot and if the stream does not exist or has no subscribers. * Modifies the message sent by send_pm_if_empty_stream to accommodate stream IDs. Note that all of the above changes are required before check_message can be modified to support stream IDs. --- zerver/lib/actions.py | 43 ++++++++++++++++++++++++++++------- zerver/lib/addressee.py | 3 +-- zerver/lib/exceptions.py | 11 +++++++++ zerver/models.py | 3 +++ zerver/tests/test_messages.py | 42 ++++++++++++++++++++++++++++++++++ 5 files changed, 92 insertions(+), 10 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 42bcb50f44..8b581c54ec 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -35,7 +35,8 @@ from zerver.lib.cache import ( ) from zerver.lib.context_managers import lockfile from zerver.lib.emoji import emoji_name_to_emoji_code, get_emoji_file_name -from zerver.lib.exceptions import StreamDoesNotExistError +from zerver.lib.exceptions import StreamDoesNotExistError, \ + StreamWithIDDoesNotExistError from zerver.lib.hotspots import get_next_hotspots from zerver.lib.message import ( access_message, @@ -104,7 +105,8 @@ from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity, UserGroup, UserGroupMembership, get_default_stream_groups, \ get_bot_services, get_bot_dicts_in_realm, DomainNotAllowedForRealmError, \ DisposableEmailError, EmailContainsPlusError, \ - get_user_including_cross_realm, get_user_by_id_in_realm_including_cross_realm + get_user_including_cross_realm, get_user_by_id_in_realm_including_cross_realm, \ + get_stream_by_id_in_realm from zerver.lib.alert_words import alert_words_in_realm from zerver.lib.avatar import avatar_url, avatar_url_from_dict @@ -2066,7 +2068,8 @@ def send_rate_limited_pm_notification_to_bot_owner(sender: UserProfile, def send_pm_if_empty_stream(sender: UserProfile, stream: Optional[Stream], stream_name: str, - realm: Realm) -> None: + realm: Realm, + stream_id: Optional[int]=None) -> None: """If a bot sends a message to a stream that doesn't exist or has no subscribers, sends a notification to the bot owner (if not a cross-realm bot) so that the owner can correct the issue.""" @@ -2084,10 +2087,17 @@ def send_pm_if_empty_stream(sender: UserProfile, # num_subscribers == 0 error_msg = "there are no subscribers to that stream. To join it, " - content = ("Hi there! We thought you'd like to know that your bot **%s** just " - "tried to send a message to stream `%s`, but %s" - "click the gear in the left-side stream list." % - (sender.full_name, stream_name, error_msg)) + if stream_id is not None: + stream_info = "with ID {stream_id}".format(stream_id=stream_id) + else: + stream_info = "`{stream_name}`".format(stream_name=stream_name) + + content = ("Hi there! We thought you'd like to know that your bot **{sender}** just " + "tried to send a message to stream {stream_info}, but {error_msg}" + "click the gear in the left-side stream list.") + content = content.format(sender=sender.full_name, + stream_info=stream_info, + error_msg=error_msg) send_rate_limited_pm_notification_to_bot_owner(sender, realm, content) @@ -2144,6 +2154,17 @@ def validate_stream_name_with_pm_notification(stream_name: str, realm: Realm, return stream +def validate_stream_id_with_pm_notification(stream_id: int, realm: Realm, + sender: UserProfile) -> Stream: + try: + stream = get_stream_by_id_in_realm(stream_id, realm) + send_pm_if_empty_stream(sender, stream, stream.name, realm) + except Stream.DoesNotExist: + send_pm_if_empty_stream(sender, None, "", realm, stream_id=stream_id) + raise StreamWithIDDoesNotExistError(stream_id) + + return stream + # check_message: # Returns message ready for sending with do_send_message on success or the error message (string) on error. def check_message(sender: UserProfile, client: Client, addressee: Addressee, @@ -2175,7 +2196,13 @@ def check_message(sender: UserProfile, client: Client, addressee: Addressee, topic_name = truncate_topic(topic_name) stream_name = addressee.stream_name() - stream = validate_stream_name_with_pm_notification(stream_name, realm, sender) + stream_id = addressee.stream_id() + + if stream_name is not None: + stream = validate_stream_name_with_pm_notification(stream_name, realm, sender) + else: + stream = validate_stream_id_with_pm_notification(stream_id, realm, sender) + recipient = get_stream_recipient(stream.id) # This will raise JsonableError if there are problems. diff --git a/zerver/lib/addressee.py b/zerver/lib/addressee.py index 86799ffa27..46c8a80598 100644 --- a/zerver/lib/addressee.py +++ b/zerver/lib/addressee.py @@ -85,9 +85,8 @@ class Addressee: assert(self.is_private()) return self._user_profiles # type: ignore # assertion protects us - def stream_name(self) -> str: + def stream_name(self) -> Optional[str]: assert(self.is_stream()) - assert(self._stream_name is not None) return self._stream_name def stream_id(self) -> Optional[int]: diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index d7507d45e7..39cd8d5e82 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -142,6 +142,17 @@ class StreamDoesNotExistError(JsonableError): def msg_format() -> str: return _("Stream '{stream}' does not exist") +class StreamWithIDDoesNotExistError(JsonableError): + code = ErrorCode.STREAM_DOES_NOT_EXIST + data_fields = ['stream_id'] + + def __init__(self, stream_id: int) -> None: + self.stream_id = stream_id + + @staticmethod + def msg_format() -> str: + return _("Stream with ID '{stream_id}' does not exist") + class CannotDeactivateLastUserError(JsonableError): code = ErrorCode.CANNOT_DEACTIVATE_LAST_USER data_fields = ['is_last_admin', 'entity'] diff --git a/zerver/models.py b/zerver/models.py index d9338c2b52..e2f4c6f3a4 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1279,6 +1279,9 @@ def get_stream(stream_name: str, realm: Realm) -> Stream: ''' return get_realm_stream(stream_name, realm.id) +def get_stream_by_id_in_realm(stream_id: int, realm: Realm) -> Stream: + return Stream.objects.select_related().get(id=stream_id, realm=realm) + def bulk_get_streams(realm: Realm, stream_names: STREAM_NAMES) -> Dict[str, Any]: def fetch_streams_by_name(stream_names: List[str]) -> Sequence[Stream]: diff --git a/zerver/tests/test_messages.py b/zerver/tests/test_messages.py index e32a41653d..10713cdfe6 100644 --- a/zerver/tests/test_messages.py +++ b/zerver/tests/test_messages.py @@ -1308,6 +1308,48 @@ class MessagePOSTTest(ZulipTestCase): "topic": "Test topic"}) self.assert_json_success(result) + def test_message_to_stream_with_nonexistent_id(self) -> None: + cordelia = self.example_user('cordelia') + bot = self.create_test_bot( + short_name='whatever', + user_profile=cordelia, + ) + result = self.api_post( + bot.email, "/api/v1/messages", + { + "type": "stream", + "to": ujson.dumps([99999]), + "client": "test suite", + "content": "Stream message by ID.", + "topic": "Test topic for stream ID message" + } + ) + self.assert_json_error(result, "Stream with ID '99999' does not exist") + + msg = self.get_last_message() + expected = ("Hi there! We thought you'd like to know that your bot **{sender}** just " + "tried to send a message to stream {stream_info}, but that stream does not " + "yet exist. To create it, click the gear in the left-side stream list.") + expected = expected.format(sender=bot.full_name, stream_info='with ID 99999') + self.assertEqual(msg.content, expected) + + def test_message_to_stream_by_id(self) -> None: + """ + Sending a message to a stream (by stream ID) to which you are + subscribed is successful. + """ + self.login(self.example_email("hamlet")) + realm = get_realm('zulip') + stream = get_stream('Verona', realm) + result = self.client_post("/json/messages", {"type": "stream", + "to": ujson.dumps([stream.id]), + "client": "test suite", + "content": "Stream message by ID.", + "topic": "Test topic for stream ID message"}) + self.assert_json_success(result) + sent_message = self.get_last_message() + self.assertEqual(sent_message.content, "Stream message by ID.") + def test_message_to_announce(self) -> None: """ Sending a message to an announcement_only stream by a realm admin