From e2d56db2a349e957184631aaa0f18c303d2a5748 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Thu, 5 Dec 2024 12:44:27 -0800 Subject: [PATCH] =?UTF-8?q?message=5Fcache:=20Use=20the=20sender=E2=80=99s?= =?UTF-8?q?=20recipient=5Fid=20for=20incoming=201:1=20DMs.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For an incoming 1:1 DM, the recipient’s own recipient_id is useless to the recipient themselves. Substitute the sender’s recipient_id, so the recipient can use recipient_id as documented to uniquely represent the set of 2 users in this conversation. Signed-off-by: Anders Kaseorg --- api_docs/changelog.md | 7 ++++++ version.py | 2 +- zerver/lib/message.py | 8 +++++- zerver/lib/message_cache.py | 14 +++++++++++ zerver/lib/outgoing_webhook.py | 1 + zerver/openapi/zulip.yaml | 5 ++++ zerver/tests/test_event_system.py | 26 +++++++++++++++++++- zerver/tests/test_message_dict.py | 12 ++++++--- zerver/tests/test_message_edit.py | 23 +++++++++++++---- zerver/tests/test_message_fetch.py | 19 ++++++++++++++ zerver/tests/test_message_send.py | 1 + zerver/tests/test_outgoing_webhook_system.py | 2 ++ zerver/tornado/event_queue.py | 11 ++++++++- zerver/tornado/views.py | 1 + 14 files changed, 120 insertions(+), 12 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 5976e570eb..af7f80fe03 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 327** + +* [`GET /messages`](/api/get-messages), [`GET + /messages/{message_id}`](/api/get-message), [`GET /events`](/api/get-events): + Adjusted the `recipient_id` field of an incoming 1:1 direct message to use the + same value that would be used for an outgoing message in that conversation. + **Feature level 326** * [`POST /register`](/api/register-queue): Removed `allow_owners_group` diff --git a/version.py b/version.py index 3ff4cdbeeb..3dc50321d4 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 = 326 # Last bumped for updating fields in server_supported_permission_settings +API_FEATURE_LEVEL = 327 # Last bumped to adjust recipient_id in incoming 1:1 DMs # 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/lib/message.py b/zerver/lib/message.py index 07cb28f6a4..ebfcbf4dd1 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -253,7 +253,13 @@ def messages_for_ids( msg_dict["can_access_sender"] = msg_dict["sender_id"] not in inaccessible_sender_ids message_list.append(msg_dict) - MessageDict.post_process_dicts(message_list, apply_markdown, client_gravatar, realm) + MessageDict.post_process_dicts( + message_list, + apply_markdown=apply_markdown, + client_gravatar=client_gravatar, + realm=realm, + user_recipient_id=None if user_profile is None else user_profile.recipient_id, + ) return message_list diff --git a/zerver/lib/message_cache.py b/zerver/lib/message_cache.py index 5c422a816b..36c9ac032a 100644 --- a/zerver/lib/message_cache.py +++ b/zerver/lib/message_cache.py @@ -175,9 +175,11 @@ class MessageDict: @staticmethod def post_process_dicts( objs: list[dict[str, Any]], + *, apply_markdown: bool, client_gravatar: bool, realm: Realm, + user_recipient_id: int | None, ) -> None: """ NOTE: This function mutates the objects in @@ -199,6 +201,7 @@ class MessageDict: skip_copy=True, can_access_sender=can_access_sender, realm_host=realm.host, + is_incoming_1_to_1=obj["recipient_id"] == user_recipient_id, ) @staticmethod @@ -211,6 +214,7 @@ class MessageDict: skip_copy: bool = False, can_access_sender: bool, realm_host: str, + is_incoming_1_to_1: bool, ) -> dict[str, Any]: """ By default, we make a shallow copy of the incoming dict to avoid @@ -247,12 +251,20 @@ class MessageDict: else: obj["content_type"] = "text/x-markdown" + if is_incoming_1_to_1: + # For an incoming 1:1 DM, the recipient’s own recipient_id is + # useless to the recipient themselves. Substitute the sender’s + # recipient_id, so the recipient can use recipient_id as documented + # to uniquely represent the set of 2 users in this conversation. + obj["recipient_id"] = obj["sender_recipient_id"] + for item in obj.get("edit_history", []): if "prev_rendered_content_version" in item: del item["prev_rendered_content_version"] if not keep_rendered_content: del obj["rendered_content"] + del obj["sender_recipient_id"] del obj["sender_realm_id"] del obj["sender_avatar_source"] del obj["sender_delivery_email"] @@ -472,6 +484,7 @@ class MessageDict: "full_name", "delivery_email", "email", + "recipient_id", "realm__string_id", "avatar_source", "avatar_version", @@ -486,6 +499,7 @@ class MessageDict: for obj in objs: sender_id = obj["sender_id"] user_row = sender_dict[sender_id] + obj["sender_recipient_id"] = user_row["recipient_id"] obj["sender_full_name"] = user_row["full_name"] obj["sender_email"] = user_row["email"] obj["sender_delivery_email"] = user_row["delivery_email"] diff --git a/zerver/lib/outgoing_webhook.py b/zerver/lib/outgoing_webhook.py index 727cad8ef8..8ebd917e9a 100644 --- a/zerver/lib/outgoing_webhook.py +++ b/zerver/lib/outgoing_webhook.py @@ -68,6 +68,7 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): get_user_profile_by_id(event["message"]["sender_id"]), self.user_profile ), realm_host=realm.host, + is_incoming_1_to_1=event["message"]["recipient_id"] == self.user_profile.recipient_id, ) request_data = { diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 0432863254..a557c74f5a 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -23510,6 +23510,11 @@ components: A unique ID for the set of users receiving the message (either a channel or group of users). Useful primarily for hashing. + + **Changes**: Before Zulip 10.0 (feature level 327), `recipient_id` + was the same across all incoming 1:1 direct messages. Now, each + incoming message uniquely shares a `recipient_id` with outgoing + messages in the same conversation. sender_email: type: string description: | diff --git a/zerver/tests/test_event_system.py b/zerver/tests/test_event_system.py index 97137ed1d4..ef4b871045 100644 --- a/zerver/tests/test_event_system.py +++ b/zerver/tests/test_event_system.py @@ -375,6 +375,7 @@ class GetEventsTest(ZulipTestCase): self.assertEqual(events[0]["local_message_id"], local_id) self.assertEqual(events[0]["message"]["display_recipient"][0]["is_mirror_dummy"], False) self.assertEqual(events[0]["message"]["display_recipient"][1]["is_mirror_dummy"], False) + self.assertEqual(events[0]["message"]["recipient_id"], recipient_user_profile.recipient_id) last_event_id = events[0]["id"] local_id = "10.02" @@ -407,6 +408,7 @@ class GetEventsTest(ZulipTestCase): self.assertEqual(events[0]["type"], "message") self.assertEqual(events[0]["message"]["sender_email"], email) self.assertEqual(events[0]["local_message_id"], local_id) + self.assertEqual(events[0]["message"]["recipient_id"], recipient_user_profile.recipient_id) # Test that the received message in the receiver's event queue # exists and does not contain a local id @@ -426,9 +428,12 @@ class GetEventsTest(ZulipTestCase): self.assertEqual(recipient_events[0]["type"], "message") self.assertEqual(recipient_events[0]["message"]["sender_email"], email) self.assertTrue("local_message_id" not in recipient_events[0]) + # Incoming DMs show the recipient_id that outgoing DMs would. + self.assertEqual(recipient_events[0]["message"]["recipient_id"], user_profile.recipient_id) self.assertEqual(recipient_events[1]["type"], "message") self.assertEqual(recipient_events[1]["message"]["sender_email"], email) self.assertTrue("local_message_id" not in recipient_events[1]) + self.assertEqual(recipient_events[1]["message"]["recipient_id"], user_profile.recipient_id) def test_get_events_narrow(self) -> None: user_profile = self.example_user("hamlet") @@ -861,6 +866,7 @@ class ClientDescriptorsTest(ZulipTestCase): queue_timeout=0, realm_id=realm.id, user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, ) client = allocate_client_descriptor(queue_data) @@ -915,6 +921,7 @@ class ClientDescriptorsTest(ZulipTestCase): queue_timeout=0, realm_id=realm.id, user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, ) client = allocate_client_descriptor(queue_data) @@ -959,9 +966,15 @@ class ClientDescriptorsTest(ZulipTestCase): class MockClient: def __init__( - self, user_profile_id: int, apply_markdown: bool, client_gravatar: bool + self, + *, + user_profile_id: int, + user_recipient_id: int | None, + apply_markdown: bool, + client_gravatar: bool, ) -> None: self.user_profile_id = user_profile_id + self.user_recipient_id = user_recipient_id self.apply_markdown = apply_markdown self.client_gravatar = client_gravatar self.client_type_name = "whatever" @@ -979,24 +992,28 @@ class ClientDescriptorsTest(ZulipTestCase): client1 = MockClient( user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, apply_markdown=True, client_gravatar=False, ) client2 = MockClient( user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, apply_markdown=False, client_gravatar=False, ) client3 = MockClient( user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, apply_markdown=True, client_gravatar=True, ) client4 = MockClient( user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, apply_markdown=False, client_gravatar=True, ) @@ -1028,11 +1045,13 @@ class ClientDescriptorsTest(ZulipTestCase): content="**hello**", rendered_content="hello", sender_id=sender.id, + recipient_id=1111, type="stream", client="website", # NOTE: Some of these fields are clutter, but some # will be useful when we let clients specify # that they can compute their own gravatar URLs. + sender_recipient_id=sender.recipient_id, sender_email=sender.email, sender_delivery_email=sender.delivery_email, sender_realm_id=sender.realm_id, @@ -1073,6 +1092,7 @@ class ClientDescriptorsTest(ZulipTestCase): sender_id=sender.id, sender_email=sender.email, id=999, + recipient_id=1111, content="hello", content_type="text/html", client="website", @@ -1092,6 +1112,7 @@ class ClientDescriptorsTest(ZulipTestCase): sender_id=sender.id, sender_email=sender.email, id=999, + recipient_id=1111, content="**hello**", content_type="text/x-markdown", client="website", @@ -1112,6 +1133,7 @@ class ClientDescriptorsTest(ZulipTestCase): sender_email=sender.email, avatar_url=None, id=999, + recipient_id=1111, content="hello", content_type="text/html", client="website", @@ -1132,6 +1154,7 @@ class ClientDescriptorsTest(ZulipTestCase): sender_email=sender.email, avatar_url=None, id=999, + recipient_id=1111, content="**hello**", content_type="text/x-markdown", client="website", @@ -1159,6 +1182,7 @@ class ReloadWebClientsTest(ZulipTestCase): queue_timeout=0, realm_id=realm.id, user_profile_id=hamlet.id, + user_recipient_id=hamlet.recipient_id, ) client = allocate_client_descriptor(queue_data) diff --git a/zerver/tests/test_message_dict.py b/zerver/tests/test_message_dict.py index a538db1270..9d6f85b1ff 100644 --- a/zerver/tests/test_message_dict.py +++ b/zerver/tests/test_message_dict.py @@ -71,7 +71,7 @@ class MessageDictTest(ZulipTestCase): return msg def get_send_message_payload( - msg_id: int, apply_markdown: bool, client_gravatar: bool + msg_id: int, *, apply_markdown: bool, client_gravatar: bool ) -> dict[str, Any]: msg = reload_message(msg_id) wide_dict = MessageDict.wide_dict(msg) @@ -82,11 +82,12 @@ class MessageDictTest(ZulipTestCase): client_gravatar=client_gravatar, can_access_sender=True, realm_host=get_realm("zulip").host, + is_incoming_1_to_1=False, ) return narrow_dict def get_fetch_payload( - msg_id: int, apply_markdown: bool, client_gravatar: bool + msg_id: int, *, apply_markdown: bool, client_gravatar: bool ) -> dict[str, Any]: msg = reload_message(msg_id) unhydrated_dict = MessageDict.messages_to_encoded_cache_helper([msg])[0] @@ -97,6 +98,7 @@ class MessageDictTest(ZulipTestCase): apply_markdown=apply_markdown, client_gravatar=client_gravatar, realm=get_realm("zulip"), + user_recipient_id=None, ) final_dict = unhydrated_dict return final_dict @@ -175,7 +177,11 @@ class MessageDictTest(ZulipTestCase): with self.assert_database_query_count(7): objs = MessageDict.ids_to_dict(ids) MessageDict.post_process_dicts( - objs, apply_markdown=False, client_gravatar=False, realm=realm + objs, + apply_markdown=False, + client_gravatar=False, + realm=realm, + user_recipient_id=None, ) self.assert_length(objs, num_ids) diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 5a74bce349..209d6fe0a1 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -171,18 +171,31 @@ class EditMessageTest(ZulipTestCase): self.assertEqual(Message.objects.get(id=msg_id).topic_name(), "edited") def test_fetch_message_from_id(self) -> None: - self.login("hamlet") + hamlet = self.example_user("hamlet") + cordelia = self.example_user("cordelia") + self.login_user(hamlet) + msg_id = self.send_personal_message( - from_user=self.example_user("hamlet"), - to_user=self.example_user("cordelia"), - content="Personal message", + from_user=hamlet, to_user=cordelia, content="Outgoing direct message" ) result = self.client_get("/json/messages/" + str(msg_id)) response_dict = self.assert_json_success(result) - self.assertEqual(response_dict["raw_content"], "Personal message") + self.assertEqual(response_dict["raw_content"], "Outgoing direct message") self.assertEqual(response_dict["message"]["id"], msg_id) + self.assertEqual(response_dict["message"]["recipient_id"], cordelia.recipient_id) self.assertEqual(response_dict["message"]["flags"], ["read"]) + msg_id = self.send_personal_message( + from_user=cordelia, to_user=hamlet, content="Incoming direct message" + ) + result = self.client_get("/json/messages/" + str(msg_id)) + response_dict = self.assert_json_success(result) + self.assertEqual(response_dict["raw_content"], "Incoming direct message") + self.assertEqual(response_dict["message"]["id"], msg_id) + # Incoming DMs show the recipient_id that outgoing DMs would. + self.assertEqual(response_dict["message"]["recipient_id"], cordelia.recipient_id) + self.assertEqual(response_dict["message"]["flags"], []) + # Send message to web-public stream where hamlet is not subscribed. # This will test case of user having no `UserMessage` but having access # to message. diff --git a/zerver/tests/test_message_fetch.py b/zerver/tests/test_message_fetch.py index 54fdb55b29..088e902236 100644 --- a/zerver/tests/test_message_fetch.py +++ b/zerver/tests/test_message_fetch.py @@ -4089,6 +4089,7 @@ class GetOldMessagesTest(ZulipTestCase): client_gravatar=False, can_access_sender=True, realm_host=get_realm("zulip").host, + is_incoming_1_to_1=False, ) self.assertEqual(final_dict["content"], "

test content

") @@ -4884,6 +4885,24 @@ WHERE user_profile_id = {hamlet_id} AND (content ILIKE '%jumping%' OR subject IL '@Othello, the Moor of Venice?

', ) + def test_dm_recipient_id(self) -> None: + hamlet = self.example_user("hamlet") + othello = self.example_user("othello") + self.login_user(hamlet) + + outgoing_message_id = self.send_personal_message(hamlet, othello) + incoming_message_id = self.send_personal_message(othello, hamlet) + + result = self.get_and_check_messages(dict(anchor="newest", num_before=2)) + self.assert_length(result["messages"], 2) + self.assertEqual(result["messages"][0]["id"], outgoing_message_id) + self.assertEqual(result["messages"][0]["sender_id"], hamlet.id) + self.assertEqual(result["messages"][0]["recipient_id"], othello.recipient_id) + self.assertEqual(result["messages"][1]["id"], incoming_message_id) + self.assertEqual(result["messages"][1]["sender_id"], othello.id) + # Incoming DMs show the recipient_id that outgoing DMs would. + self.assertEqual(result["messages"][1]["recipient_id"], othello.recipient_id) + class MessageHasKeywordsTest(ZulipTestCase): """Test for keywords like has_link, has_image, has_attachment.""" diff --git a/zerver/tests/test_message_send.py b/zerver/tests/test_message_send.py index c2cb330612..9730d1a418 100644 --- a/zerver/tests/test_message_send.py +++ b/zerver/tests/test_message_send.py @@ -1750,6 +1750,7 @@ class StreamMessagesTest(ZulipTestCase): apply_markdown=True, client_gravatar=False, realm=user_profile.realm, + user_recipient_id=None, ) self.assertEqual(dct["display_recipient"], "Denmark") diff --git a/zerver/tests/test_outgoing_webhook_system.py b/zerver/tests/test_outgoing_webhook_system.py index b0ca446964..9eb04d0e6a 100644 --- a/zerver/tests/test_outgoing_webhook_system.py +++ b/zerver/tests/test_outgoing_webhook_system.py @@ -53,7 +53,9 @@ class DoRestCallTests(ZulipTestCase): "message": { "display_recipient": "Verona", "stream_id": 999, + "recipient_id": 1111, "sender_id": bot_user.id, + "sender_recipient_id": bot_user.recipient_id, "sender_email": bot_user.email, "sender_realm_id": bot_user.realm.id, "sender_realm_str": bot_user.realm.string_id, diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 721d1b494a..7d13786123 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -63,6 +63,7 @@ class ClientDescriptor: self, *, user_profile_id: int, + user_recipient_id: int | None, realm_id: int, event_queue: "EventQueue", event_types: Sequence[str] | None, @@ -91,6 +92,7 @@ class ClientDescriptor: # added to load_event_queues() to update the restored objects. # Additionally, the to_dict and from_dict methods must be updated self.user_profile_id = user_profile_id + self.user_recipient_id = user_recipient_id self.realm_id = realm_id self.current_handler_id: int | None = None self.current_client_name: str | None = None @@ -165,6 +167,7 @@ class ClientDescriptor: ret = cls( user_profile_id=d["user_profile_id"], + user_recipient_id=d.get("user_recipient_id"), realm_id=d["realm_id"], event_queue=EventQueue.from_dict(d["event_queue"]), event_types=d["event_types"], @@ -1123,7 +1126,11 @@ def process_message_event( @cache def get_client_payload( - *, apply_markdown: bool, client_gravatar: bool, can_access_sender: bool + *, + apply_markdown: bool, + client_gravatar: bool, + can_access_sender: bool, + is_incoming_1_to_1: bool, ) -> dict[str, Any]: return MessageDict.finalize_payload( wide_dict, @@ -1131,6 +1138,7 @@ def process_message_event( client_gravatar=client_gravatar, can_access_sender=can_access_sender, realm_host=realm_host, + is_incoming_1_to_1=is_incoming_1_to_1, ) # Extra user-specific data to include @@ -1211,6 +1219,7 @@ def process_message_event( apply_markdown=client.apply_markdown, client_gravatar=client.client_gravatar, can_access_sender=can_access_sender, + is_incoming_1_to_1=wide_dict["recipient_id"] == client.user_recipient_id, ) # Make sure Zephyr mirroring bots know whether stream is invite-only diff --git a/zerver/tornado/views.py b/zerver/tornado/views.py index 3835cd8af3..457a9646b3 100644 --- a/zerver/tornado/views.py +++ b/zerver/tornado/views.py @@ -235,6 +235,7 @@ def get_events_backend( if queue_id is None: new_queue_data = dict( user_profile_id=user_profile.id, + user_recipient_id=user_profile.recipient_id, realm_id=user_profile.realm_id, event_types=event_types, client_type_name=valid_user_client_name,