From 072051f81e46b2a933ad9d6b9da73a0145909304 Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Mon, 14 Feb 2022 17:04:39 +0100 Subject: [PATCH] api: Add additional fields to `edit_history` entries. Since we've changed the database to contain these new fields, we just need to stop dropping them in the API code. This also changes the public API to match the database format again by removing `prev_subject` from edit history API. Adds an API changelog feature update for the renamed `prev_subject` field (to `prev_topic`) and new fields (`topic` and `stream`) in the message `edit_history`. Also, documents said `edit_history` in the `MessagesBase` schema in the api documentation, which is used by the `/get-messages`, `/get-events` and `/zulip-outgoing-webhooks` endpoints. Fixes #21076. Co-authored-by: Lauryn Menard --- templates/zerver/api/changelog.md | 15 +++ version.py | 2 +- zerver/lib/message.py | 27 +---- zerver/lib/types.py | 21 ---- zerver/openapi/zulip.yaml | 162 ++++++++++++++++++++++++++---- zerver/tests/test_message_edit.py | 18 ++-- zerver/views/message_edit.py | 4 +- 7 files changed, 173 insertions(+), 76 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index b03a00886d..c5797a5a15 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -20,6 +20,21 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 5.0 +**Feature level 118** + +* [`GET /messages`](/api/get-messages), [`GET + /events`](/api/get-events): Improved the format of the + `edit_history` object within message objects. Entries for stream + edits now include a both a `prev_stream` and `stream` field to + indicate the previous and current stream IDs. Entries for topic + edits now include both a `prev_topic` and `topic` field to indicate + the previous and current topic, replacing the `prev_subject` + field. These changes substantially simplify client complexity for + processing historical message edits. + +* [`GET messages/{message_id}/history`](/api/get-message-history): + Added `stream` field to message history `snapshot` indicating + the updated stream ID of messages moved to a new stream. **Feature level 117** diff --git a/version.py b/version.py index 569ba4a215..18bd9cf076 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 117 +API_FEATURE_LEVEL = 118 # 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 f13b19fd49..6f0ccb542c 100644 --- a/zerver/lib/message.py +++ b/zerver/lib/message.py @@ -38,12 +38,7 @@ from zerver.lib.streams import get_web_public_streams_queryset from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.topic import DB_TOPIC_NAME, MESSAGE__TOPIC, TOPIC_LINKS, TOPIC_NAME from zerver.lib.topic_mutes import build_topic_mute_checker, topic_is_muted -from zerver.lib.types import ( - APIEditHistoryEvent, - DisplayRecipientT, - EditHistoryEvent, - UserDisplayRecipient, -) +from zerver.lib.types import DisplayRecipientT, EditHistoryEvent, UserDisplayRecipient from zerver.models import ( MAX_TOPIC_NAME_LENGTH, Message, @@ -507,25 +502,7 @@ class MessageDict: if last_edit_time is not None: obj["last_edit_timestamp"] = datetime_to_timestamp(last_edit_time) assert edit_history_json is not None - raw_edit_history: List[EditHistoryEvent] = orjson.loads(edit_history_json) - edit_history: List[APIEditHistoryEvent] = [] - for edit_history_event in raw_edit_history: - # Drop fields we're not yet ready to have appear in the API - if "prev_topic" in edit_history_event: - # The prev_subject field has been renamed in the - # database, but not the API. - edit_history_event["prev_subject"] = edit_history_event["prev_topic"] # type: ignore # Temporary type-checking violation - - # New fields not consistently available, and thus - # intentionally not exposed to the API. - if "prev_topic" in edit_history_event: - del edit_history_event["prev_topic"] - if "stream" in edit_history_event: - del edit_history_event["stream"] - if "topic" in edit_history_event: - del edit_history_event["topic"] - - edit_history.append(edit_history_event) # type: ignore # Temporary type-checking violation + edit_history: List[EditHistoryEvent] = orjson.loads(edit_history_json) obj["edit_history"] = edit_history if Message.need_to_render_content( diff --git a/zerver/lib/types.py b/zerver/lib/types.py index 53fd941e7b..a765ab0adf 100644 --- a/zerver/lib/types.py +++ b/zerver/lib/types.py @@ -91,32 +91,11 @@ class UnspecifiedValue: pass -class APIEditHistoryEvent(TypedDict, total=False): - """Format of legacy edit history events in the API. Contains legacy - fields like LEGACY_PREV_TOPIC that we intend to remove from the - API eventually. - """ - - # Commented fields are fields we plan to add. - user_id: Optional[int] - timestamp: int - prev_stream: int - # stream: int - # TODO: Remove prev_subject from the API. - prev_subject: str - # prev_topic: str - # topic: str - prev_content: str - prev_rendered_content: Optional[str] - prev_rendered_content_version: Optional[int] - - class EditHistoryEvent(TypedDict, total=False): """ Database format for edit history events. """ - # Commented fields are fields we plan to add. user_id: Optional[int] timestamp: int prev_stream: int diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 6795c8cb21..b96d1ae5f1 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -4963,9 +4963,8 @@ paths: `topic`, `content`, `rendered_content`, `timestamp` and `user_id`. This snapshot will be the only one present if the message has never been edited. - Also note that if a message's content was edited (but not the topic) - or the topic was edited (but not the content), the snapshot object - will only contain data for the modified fields (e.g. if only the topic + Also note that each snapshot object will only contain additional data for the + modified fields for that particular edit (e.g. if only the topic or stream was edited, `prev_content`, `prev_rendered_content`, and `content_html_diff` will not appear). responses: @@ -4990,51 +4989,80 @@ paths: topic: type: string description: | - the topic for the message. + The topic of the message immediately + after this edit event. prev_topic: type: string description: | - the topic for the message before being edited. + Only present if message's topic was edited. + + The topic of the message immediately + prior to this edit event. + stream: + type: integer + description: | + Only present if message's stream was edited. + + The ID of the stream containing the message + immediately after this edit event. + + **Changes**: New in Zulip 5.0 (feature level 118). prev_stream: type: integer description: | Only present if message's stream was edited. - The stream ID of the message prior to being edited. + The ID of the stream containing the message immediately + prior to this edit event. content: type: string description: | - the body of the message. + The raw Markdown content of the message + immediately after this edit event. rendered_content: type: string description: | - the already rendered, HTML version of `content`. + The rendered HTML representation of `content`. prev_content: type: string description: | - the body of the message before being edited. + Only present if message's content was edited. + + The raw Markdown content of the message immediately + prior to this edit event. prev_rendered_content: type: string description: | - the already rendered, HTML version of - `prev_content`. + Only present if message's content was edited. + + The rendered HTML representation of `prev_content`. user_id: type: integer + nullable: true description: | - the ID of the user that made the edit. + The ID of the user that made the edit. + + Will be null only for edit history + events predating March 2017. + + Clients can display edit history events where this + is null as modified by either the sender (for content + edits) or an unknown user (for topic edits). content_html_diff: type: string description: | - an HTML diff between this version of the message + Only present if message's content was edited. + + An HTML diff between this version of the message and the previous one. timestamp: type: integer description: | - the UNIX timestamp for this edit. + The UNIX timestamp for this edit. description: | - A chronologically sorted array of `snapshot` - objects, each one with the values of the - message after the edit. + A chronologically sorted, oldest to newest, array + of `snapshot` objects, each one with the values of + the message after the edit. example: { "message_history": @@ -14625,6 +14653,106 @@ components: Data on the recipient of the message; either the name of a stream or a dictionary containing basic data on the users who received the message. + edit_history: + type: array + items: + type: object + additionalProperties: false + properties: + prev_content: + type: string + description: | + Only present if message's content was edited. + + The content of the message immediately prior to this + edit event. + prev_rendered_content: + type: string + description: | + Only present if message's content was edited. + + The rendered HTML representation of `prev_content`. + prev_rendered_content_version: + type: string + description: | + Only present if message's content was edited. + + The Markdown processor version number for the message + immediately prior to this edit event. + prev_stream: + type: integer + description: | + Only present if message's stream was edited. + + The stream ID of the message immediately prior to this + edit event. + prev_topic: + type: string + description: | + Only present if message's topic was edited. + + The topic of the message immediately prior to this + edit event. + + **Changes**: New in Zulip 5.0 (feature level 118). + Previously, this field was called `prev_subject`; + clients are recommended to rename `prev_subject` to + `prev_topic` if present for compatibility with + older Zulip servers. + stream: + type: integer + description: | + Only present if message's stream was edited. + + The ID of the stream containing the message + immediately after this edit event. + + **Changes**: New in Zulip 5.0 (feature level 118). + timestamp: + type: integer + description: | + The UNIX timestamp for the edit. + topic: + type: string + description: | + Only present if message's topic was edited. + + The topic of the message immediately after this edit event. + + **Changes**: New in Zulip 5.0 (feature level 118). + user_id: + type: integer + nullable: true + description: | + The ID of the user that made the edit. + + Will be null only for edit history + events predating March 2017. + + Clients can display edit history events where this + is null as modified by either the sender (for content + edits) or an unknown user (for topic edits). + required: + - user_id + - timestamp + description: | + An array of objects, with each object documenting the + changes made in a previous edit made to the the message, + ordered chronologically from most recent to least recent + edit. + + Not present if the message has never been edited or if the realm has + [disabled viewing of message edit history][disable-edit-history]. + + Every object will contain `user_id` and `timestamp`. + + The other fields are optional, and will be present or not + depending on which of the stream, topic, and message + content were modified in the edit event. For example, if + only the topic was edited, only `prev_topic` and `topic` + will be present in addition to `user_id` and `timestamp`. + + [disable-edit-history]: /help/disable-message-edit-history id: type: integer description: | diff --git a/zerver/tests/test_message_edit.py b/zerver/tests/test_message_edit.py index 1ee93402ff..7c8ebc1235 100644 --- a/zerver/tests/test_message_edit.py +++ b/zerver/tests/test_message_edit.py @@ -73,12 +73,11 @@ class EditMessageTestCase(ZulipTestCase): msg.sender_id, ) - # TODO: uncomment assertion when edit_history fields in API match fields in database. - # if msg.edit_history: - # self.assertEqual( - # fetch_message_dict["edit_history"], - # orjson.loads(msg.edit_history), - # ) + if msg.edit_history: + self.assertEqual( + fetch_message_dict["edit_history"], + orjson.loads(msg.edit_history), + ) def prepare_move_topics( self, @@ -879,14 +878,13 @@ class EditMessageTest(EditMessageTestCase): expected_entries.add("content_html_diff") if i in {0, 3}: expected_entries.add("prev_stream") - # TODO: uncomment when stream field is added to API - # expected_entries.add("stream") + expected_entries.add("stream") i += 1 self.assertEqual(expected_entries, set(entry.keys())) self.assert_length(message_history, 7) self.assertEqual(message_history[0]["topic"], "topic 4") self.assertEqual(message_history[0]["prev_topic"], "topic 3") - # self.assertEqual(message_history[0]["stream"], stream_3.id) + self.assertEqual(message_history[0]["stream"], stream_3.id) self.assertEqual(message_history[0]["prev_stream"], stream_2.id) self.assertEqual(message_history[0]["content"], "content 4") @@ -900,7 +898,7 @@ class EditMessageTest(EditMessageTestCase): self.assertEqual(message_history[2]["prev_content"], "content 2") self.assertEqual(message_history[3]["topic"], "topic 2") - # self.assertEqual(message_history[3]["stream"], stream_2.id) + self.assertEqual(message_history[3]["stream"], stream_2.id) self.assertEqual(message_history[3]["prev_stream"], stream_1.id) self.assertEqual(message_history[3]["content"], "content 2") diff --git a/zerver/views/message_edit.py b/zerver/views/message_edit.py index caa383675c..ef136a5894 100644 --- a/zerver/views/message_edit.py +++ b/zerver/views/message_edit.py @@ -16,7 +16,7 @@ from zerver.lib.message import access_message, access_web_public_message from zerver.lib.request import REQ, RequestNotes, has_request_variables from zerver.lib.response import json_success from zerver.lib.timestamp import datetime_to_timestamp -from zerver.lib.topic import LEGACY_PREV_TOPIC, REQ_topic +from zerver.lib.topic import REQ_topic from zerver.lib.types import EditHistoryEvent, FormattedEditHistoryEvent from zerver.lib.validator import check_bool, check_string_in, to_non_negative_int from zerver.models import Message, UserProfile @@ -70,7 +70,7 @@ def fill_edit_history_entries( if "prev_stream" in edit_history_event: formatted_entry["prev_stream"] = edit_history_event["prev_stream"] - # TODO: Include current stream here. + formatted_entry["stream"] = edit_history_event["stream"] formatted_edit_history.append(formatted_entry)