From 3ba198e79acb0bf60d7a1af486c5210b125eaa82 Mon Sep 17 00:00:00 2001 From: Prakhar Pratyush Date: Tue, 12 Nov 2024 19:16:51 +0530 Subject: [PATCH] message_send: Add support to send message with an empty topic name. This commit is a part of the work to support empty string as a topic name. Previously, empty string was not a valid topic name. Adds a `empty_topic_name` client capability to allow client to specify whether it supports empty string as a topic name. Adds backward compatibility for: - `subject` field in the `message` event type --- api_docs/changelog.md | 14 +++++++ tools/linter_lib/custom_check.py | 1 + version.py | 2 +- zerver/actions/message_send.py | 2 +- zerver/lib/addressee.py | 4 ++ zerver/lib/events.py | 6 ++- zerver/lib/home.py | 1 + zerver/lib/string_validation.py | 3 -- zerver/lib/topic.py | 6 +++ zerver/models/messages.py | 4 ++ zerver/openapi/zulip.yaml | 29 ++++++++++++- zerver/tests/test_event_system.py | 1 + zerver/tests/test_message_move_topic.py | 5 ++- zerver/tests/test_message_send.py | 36 +++++++++-------- zerver/tests/test_message_topics.py | 54 +++++++++++++++++++++++++ zerver/tornado/django_api.py | 2 + zerver/tornado/event_queue.py | 16 +++++++- zerver/tornado/views.py | 5 +++ 18 files changed, 165 insertions(+), 26 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 13d48cea29..cac8101472 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -26,6 +26,20 @@ format used by the Zulip server that they are interacting with. `realm_empty_topic_display_name` field for clients to use while adding support for empty string as topic name. +* [`POST /register`](/api/register-queue): Added `empty_topic_name` + [client capability](/api/register-queue#parameter-client_capabilities) + to allow client to specify whether it supports empty string as a topic name + in `register` response or events involving topic names. + Clients that don't support this client capability receive + `realm_empty_topic_display_name` field value as the topic name replacing + the empty string. + +* [`GET /events`](/api/get-events): For clients that don't support + the `empty_topic_name` [client capability](/api/register-queue#parameter-client_capabilities), + the following fields will have the value of `realm_empty_topic_display_name` + field replacing the empty string for channel messages: + * `subject` field in the `message` event type + **Feature level 333** * [Message formatting](/api/message-formatting): System groups can now diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index ad9f183431..5bc3decb28 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -37,6 +37,7 @@ FILES_WITH_LEGACY_SUBJECT = { # This has lots of query data embedded, so it's hard # to fix everything until we migrate the DB to "topic". "zerver/tests/test_message_fetch.py", + "zerver/tests/test_message_topics.py", } shebang_rules: list["Rule"] = [ diff --git a/version.py b/version.py index e7368f9242..305423246d 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 333 # Last bumped for can_send_message_group setting. +API_FEATURE_LEVEL = 334 # Last bumped for adding empty_topic_name client capability. # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/actions/message_send.py b/zerver/actions/message_send.py index ce8a2eb105..639131ef86 100644 --- a/zerver/actions/message_send.py +++ b/zerver/actions/message_send.py @@ -1767,7 +1767,7 @@ def check_message( # else can sneak past the access check. assert sender.bot_type == sender.OUTGOING_WEBHOOK_BOT - if realm.mandatory_topics and topic_name == "(no topic)": + if realm.mandatory_topics and topic_name in ("(no topic)", ""): raise JsonableError(_("Topics are required in this organization")) elif addressee.is_private(): diff --git a/zerver/lib/addressee.py b/zerver/lib/addressee.py index e4ba5140ac..1924cc5644 100644 --- a/zerver/lib/addressee.py +++ b/zerver/lib/addressee.py @@ -5,6 +5,7 @@ from django.utils.translation import gettext as _ from zerver.lib.exceptions import JsonableError from zerver.lib.string_validation import check_stream_topic +from zerver.lib.topic import maybe_rename_general_chat_to_empty_topic from zerver.models import Realm, Stream, UserProfile from zerver.models.users import ( get_user_by_id_in_realm_including_cross_realm, @@ -146,6 +147,7 @@ class Addressee: @staticmethod def for_stream(stream: Stream, topic_name: str) -> "Addressee": topic_name = topic_name.strip() + topic_name = maybe_rename_general_chat_to_empty_topic(topic_name) check_stream_topic(topic_name) return Addressee( msg_type="stream", @@ -156,6 +158,7 @@ class Addressee: @staticmethod def for_stream_name(stream_name: str, topic_name: str) -> "Addressee": topic_name = topic_name.strip() + topic_name = maybe_rename_general_chat_to_empty_topic(topic_name) check_stream_topic(topic_name) return Addressee( msg_type="stream", @@ -166,6 +169,7 @@ class Addressee: @staticmethod def for_stream_id(stream_id: int, topic_name: str) -> "Addressee": topic_name = topic_name.strip() + topic_name = maybe_rename_general_chat_to_empty_topic(topic_name) check_stream_topic(topic_name) return Addressee( msg_type="stream", diff --git a/zerver/lib/events.py b/zerver/lib/events.py index 5dbd1b6497..f8a46412ca 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -80,6 +80,7 @@ from zerver.models import ( Client, CustomProfileField, Draft, + Message, NamedUserGroup, Realm, RealmUserDefault, @@ -479,7 +480,7 @@ def fetch_initial_state_data( settings.MAX_DEACTIVATED_REALM_DELETION_DAYS ) - state["realm_empty_topic_display_name"] = "testing general chat" + state["realm_empty_topic_display_name"] = Message.EMPTY_TOPIC_FALLBACK_NAME if want("realm_user_settings_defaults"): realm_user_default = RealmUserDefault.objects.get(realm=realm) state["realm_user_settings_defaults"] = {} @@ -1751,6 +1752,7 @@ class ClientCapabilities(TypedDict): user_list_incomplete: NotRequired[bool] include_deactivated_groups: NotRequired[bool] archived_channels: NotRequired[bool] + empty_topic_name: NotRequired[bool] DEFAULT_CLIENT_CAPABILITIES = ClientCapabilities(notification_settings_null=False) @@ -1792,6 +1794,7 @@ def do_events_register( user_list_incomplete = client_capabilities.get("user_list_incomplete", False) include_deactivated_groups = client_capabilities.get("include_deactivated_groups", False) archived_channels = client_capabilities.get("archived_channels", False) + empty_topic_name = client_capabilities.get("empty_topic_name", False) if fetch_event_types is not None: event_types_set: set[str] | None = set(fetch_event_types) @@ -1865,6 +1868,7 @@ def do_events_register( user_list_incomplete=user_list_incomplete, include_deactivated_groups=include_deactivated_groups, archived_channels=archived_channels, + empty_topic_name=empty_topic_name, ) if queue_id is None: diff --git a/zerver/lib/home.py b/zerver/lib/home.py index f2787c1cf2..bcceba2295 100644 --- a/zerver/lib/home.py +++ b/zerver/lib/home.py @@ -157,6 +157,7 @@ def build_page_params_for_home_page_load( user_list_incomplete=True, include_deactivated_groups=True, archived_channels=True, + empty_topic_name=True, ) if user_profile is not None: diff --git a/zerver/lib/string_validation.py b/zerver/lib/string_validation.py index ba0a95ffef..6761c1a3ee 100644 --- a/zerver/lib/string_validation.py +++ b/zerver/lib/string_validation.py @@ -56,9 +56,6 @@ def check_stream_name(stream_name: str) -> None: def check_stream_topic(topic_name: str) -> None: - if topic_name.strip() == "": - raise JsonableError(_("Topic can't be empty!")) - invalid_character_pos = check_string_is_printable(topic_name) if invalid_character_pos is not None: raise JsonableError( diff --git a/zerver/lib/topic.py b/zerver/lib/topic.py index 7e3c7d4b89..6e48c1af98 100644 --- a/zerver/lib/topic.py +++ b/zerver/lib/topic.py @@ -321,3 +321,9 @@ def participants_for_topic(realm_id: int, recipient_id: int, topic_name: str) -> ).values_list("id", flat=True) ) return participants + + +def maybe_rename_general_chat_to_empty_topic(topic_name: str) -> str: + if topic_name == Message.EMPTY_TOPIC_FALLBACK_NAME: + topic_name = "" + return topic_name diff --git a/zerver/models/messages.py b/zerver/models/messages.py index 559fcd8969..5de34749f3 100644 --- a/zerver/models/messages.py +++ b/zerver/models/messages.py @@ -157,6 +157,10 @@ class Message(AbstractMessage): DEFAULT_SELECT_RELATED = ["sender", "realm", "recipient", "sending_client"] + # Name to be used for the empty topic with clients that have not + # yet migrated to have the `empty_topic_name` client capability. + EMPTY_TOPIC_FALLBACK_NAME = "test general chat" + class Meta: indexes = [ GinIndex("search_tsvector", fastupdate=False, name="zerver_message_search_tsvector"), diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 3603bde6d2..cbe2a314ab 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -7067,7 +7067,14 @@ paths: [`POST /register`](/api/register-queue) endpoint to determine the maximum topic length. - **Changes**: New in Zulip 2.0.0. Previous Zulip releases encoded + Note: When the value of `realm_empty_topic_display_name` found in + the [POST /register](/api/register-queue) response is used for this + parameter, it is interpreted as an empty string. + + **Changes**: Before Zulip 10.0 (feature level 334), empty string + was not a valid topic name for channel messages. + + New in Zulip 2.0.0. Previous Zulip releases encoded this as `subject`, which is currently a deprecated alias. type: string example: Castle @@ -14138,6 +14145,16 @@ paths: access archived channels, without breaking backwards-compatibility for existing clients. + - `empty_topic_name`: Boolean for whether the client supports processing + the empty string as a topic name. Clients not declaring this capability + will be sent the value of `realm_empty_topic_display_name` found in the + [POST /register](/api/register-queue) response instead of the empty string + wherever topic names appear in the register response or events involving + topic names. +
+ **Changes**: New in Zulip 10.0 (feature level 334). Previously, + the empty string was not a valid topic name. + [help-linkifiers]: /help/add-a-custom-linkifier [rfc6570]: https://www.rfc-editor.org/rfc/rfc6570.html [events-linkifiers]: /api/get-events#realm_linkifiers @@ -23445,6 +23462,16 @@ components: The field name is a legacy holdover from when topics were called "subjects" and will eventually change. + + For clients that don't support the `empty_topic_name` [client capability][client-capabilities], + the empty string value is replaced with the value of `realm_empty_topic_display_name` + found in the [POST /register](/api/register-queue) response, for channel messages. + + **Changes**: Before Zulip 10.0 (feature level 334), `empty_topic_name` + client capability didn't exist and empty string as the topic name for + channel messages wasn't allowed. + + [client-capabilities]: /api/register-queue#parameter-client_capabilities submessages: type: array description: | diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index c70b48f2cf..058752219a 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -979,6 +979,7 @@ class ClientDescriptorsTest(ZulipTestCase): self.client_gravatar = client_gravatar self.client_type_name = "whatever" self.events: list[dict[str, Any]] = [] + self.empty_topic_name = True def accepts_messages(self) -> bool: return True diff --git a/zerver/tests/test_message_move_topic.py b/zerver/tests/test_message_move_topic.py index 8a9f745a19..16e20768a6 100644 --- a/zerver/tests/test_message_move_topic.py +++ b/zerver/tests/test_message_move_topic.py @@ -93,7 +93,7 @@ class MessageMoveTopicTest(ZulipTestCase): 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: + def test_edit_message_empty_topic_with_extra_space(self) -> None: self.login("hamlet") msg_id = self.send_stream_message( self.example_user("hamlet"), "Denmark", topic_name="editing", content="before edit" @@ -104,7 +104,8 @@ class MessageMoveTopicTest(ZulipTestCase): "topic": " ", }, ) - self.assert_json_error(result, "Topic can't be empty!") + self.assert_json_success(result) + self.check_topic(msg_id, "") def test_edit_message_invalid_topic(self) -> None: self.login("hamlet") diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index 9a867da56e..cb7ddd7e46 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -789,22 +789,6 @@ class MessagePOSTTest(ZulipTestCase): ) self.assert_json_error(result, "Message must not be empty") - def test_empty_string_topic(self) -> None: - """ - Sending a message that has empty string topic should fail - """ - self.login("hamlet") - result = self.client_post( - "/json/messages", - { - "type": "channel", - "to": "Verona", - "content": "Test message", - "topic": "", - }, - ) - self.assert_json_error(result, "Topic can't be empty!") - def test_missing_topic(self) -> None: """ Sending a message without topic should fail @@ -3342,3 +3326,23 @@ class CheckMessageTest(ZulipTestCase): realm.refresh_from_db() ret = check_message(sender, client, addressee, message_content, realm) self.assertEqual(ret.message.sender.id, sender.id) + + def test_empty_topic_message(self) -> None: + realm = get_realm("zulip") + sender = self.example_user("iago") + client = make_client(name="test suite") + stream = get_stream("Denmark", realm) + topic_name = "" + message_content = "whatever" + addressee = Addressee.for_stream(stream, topic_name) + + do_set_realm_property(realm, "mandatory_topics", True, acting_user=None) + realm.refresh_from_db() + + with self.assertRaisesRegex(JsonableError, "Topics are required in this organization"): + check_message(sender, client, addressee, message_content, realm) + + do_set_realm_property(realm, "mandatory_topics", False, acting_user=None) + realm.refresh_from_db() + ret = check_message(sender, client, addressee, message_content, realm) + self.assertEqual(ret.message.topic_name(), topic_name) diff --git a/zerver/tests/test_message_topics.py b/zerver/tests/test_message_topics.py index de54efa2c8..3d6f242fb1 100644 --- a/zerver/tests/test_message_topics.py +++ b/zerver/tests/test_message_topics.py @@ -1,3 +1,4 @@ +import time from unittest import mock from django.utils.timezone import now as timezone_now @@ -9,6 +10,7 @@ from zerver.models import Message, UserMessage, UserTopic from zerver.models.clients import get_client from zerver.models.realms import get_realm from zerver.models.streams import get_stream +from zerver.tornado.event_queue import allocate_client_descriptor class TopicHistoryTest(ZulipTestCase): @@ -362,3 +364,55 @@ class TopicDeleteTest(ZulipTestCase): ) result_dict = self.assert_json_success(result) self.assertFalse(result_dict["complete"]) + + +class EmptyTopicNameTest(ZulipTestCase): + def test_client_supports_empty_topic_name(self) -> None: + iago = self.example_user("iago") + hamlet = self.example_user("hamlet") + queue_data = dict( + all_public_streams=True, + apply_markdown=True, + client_type_name="website", + empty_topic_name=True, + event_types=["message"], + last_connection_time=time.time(), + queue_timeout=600, + realm_id=hamlet.realm.id, + user_profile_id=hamlet.id, + ) + client = allocate_client_descriptor(queue_data) + self.assertTrue(client.event_queue.empty()) + + self.send_stream_message(iago, "Denmark", topic_name="") + events = client.event_queue.contents() + self.assertEqual(events[0]["message"]["subject"], "") + + self.send_stream_message(iago, "Denmark", topic_name=Message.EMPTY_TOPIC_FALLBACK_NAME) + events = client.event_queue.contents() + self.assertEqual(events[1]["message"]["subject"], "") + + def test_client_not_supports_empty_topic_name(self) -> None: + iago = self.example_user("iago") + hamlet = self.example_user("hamlet") + queue_data = dict( + all_public_streams=True, + apply_markdown=True, + client_type_name="zulip-mobile", + empty_topic_name=False, + event_types=["message"], + last_connection_time=time.time(), + queue_timeout=600, + realm_id=hamlet.realm.id, + user_profile_id=hamlet.id, + ) + client = allocate_client_descriptor(queue_data) + self.assertTrue(client.event_queue.empty()) + + self.send_stream_message(iago, "Denmark", topic_name="") + events = client.event_queue.contents() + self.assertEqual(events[0]["message"]["subject"], Message.EMPTY_TOPIC_FALLBACK_NAME) + + self.send_stream_message(iago, "Denmark", topic_name=Message.EMPTY_TOPIC_FALLBACK_NAME) + events = client.event_queue.contents() + self.assertEqual(events[1]["message"]["subject"], Message.EMPTY_TOPIC_FALLBACK_NAME) diff --git a/zerver/tornado/django_api.py b/zerver/tornado/django_api.py index e3a8865af5..5095f0612c 100644 --- a/zerver/tornado/django_api.py +++ b/zerver/tornado/django_api.py @@ -92,6 +92,7 @@ def request_event_queue( user_list_incomplete: bool = False, include_deactivated_groups: bool = False, archived_channels: bool = False, + empty_topic_name: bool = False, ) -> str | None: if not settings.USING_TORNADO: return None @@ -117,6 +118,7 @@ def request_event_queue( "user_list_incomplete": orjson.dumps(user_list_incomplete), "include_deactivated_groups": orjson.dumps(include_deactivated_groups), "archived_channels": orjson.dumps(archived_channels), + "empty_topic_name": orjson.dumps(empty_topic_name), } if event_types is not None: diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 7d13786123..8ef9b524fc 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -29,7 +29,7 @@ from zerver.lib.narrow_predicate import build_narrow_predicate from zerver.lib.notification_data import UserMessageNotificationsData from zerver.lib.queue import queue_json_publish_rollback_unsafe, retry_event from zerver.middleware import async_request_timer_restart -from zerver.models import CustomProfileField +from zerver.models import CustomProfileField, Message from zerver.tornado.descriptors import clear_descriptor_by_handler_id, set_descriptor_by_handler_id from zerver.tornado.exceptions import BadEventQueueIdError from zerver.tornado.handlers import finish_handler, get_handler_by_id, handler_stats_string @@ -82,6 +82,7 @@ class ClientDescriptor: user_list_incomplete: bool, include_deactivated_groups: bool, archived_channels: bool, + empty_topic_name: bool, ) -> None: # TODO: We eventually want to upstream this code to the caller, but # serialization concerns make it a bit difficult. @@ -115,6 +116,7 @@ class ClientDescriptor: self.user_list_incomplete = user_list_incomplete self.include_deactivated_groups = include_deactivated_groups self.archived_channels = archived_channels + self.empty_topic_name = empty_topic_name # Default for lifespan_secs is DEFAULT_EVENT_QUEUE_TIMEOUT_SECS; # but users can set it as high as MAX_QUEUE_TIMEOUT_SECS. @@ -147,6 +149,7 @@ class ClientDescriptor: user_list_incomplete=self.user_list_incomplete, include_deactivated_groups=self.include_deactivated_groups, archived_channels=self.archived_channels, + empty_topic_name=self.empty_topic_name, ) @override @@ -186,6 +189,7 @@ class ClientDescriptor: user_list_incomplete=d.get("user_list_incomplete", False), include_deactivated_groups=d.get("include_deactivated_groups", False), archived_channels=d.get("archived_channels", False), + empty_topic_name=d.get("empty_topic_name", False), ) ret.last_connection_time = d["last_connection_time"] return ret @@ -1222,6 +1226,16 @@ def process_message_event( is_incoming_1_to_1=wide_dict["recipient_id"] == client.user_recipient_id, ) + # Compatibility code to change subject="" to subject=Message.EMPTY_TOPIC_FALLBACK_NAME + # for older clients with no UI support for empty topic name. + if ( + recipient_type_name == "stream" + and not client.empty_topic_name + and wide_dict["subject"] == "" + ): + message_dict = message_dict.copy() + message_dict["subject"] = Message.EMPTY_TOPIC_FALLBACK_NAME + # Make sure Zephyr mirroring bots know whether stream is invite-only if "mirror" in client.client_type_name and event_template.get("invite_only"): message_dict = message_dict.copy() diff --git a/zerver/tornado/views.py b/zerver/tornado/views.py index 457a9646b3..8278eb62cc 100644 --- a/zerver/tornado/views.py +++ b/zerver/tornado/views.py @@ -214,6 +214,10 @@ def get_events_backend( Json[bool], ApiParamConfig(documentation_status=DocumentationStatus.INTENTIONALLY_UNDOCUMENTED), ] = False, + empty_topic_name: Annotated[ + Json[bool], + ApiParamConfig(documentation_status=DocumentationStatus.INTENTIONALLY_UNDOCUMENTED), + ] = False, ) -> HttpResponse: if narrow is None: narrow = [] @@ -254,6 +258,7 @@ def get_events_backend( user_list_incomplete=user_list_incomplete, include_deactivated_groups=include_deactivated_groups, archived_channels=archived_channels, + empty_topic_name=empty_topic_name, ) result = in_tornado_thread(fetch_events)(