From aaa627229edd7d85a4ea98b68d3261852e2c818d Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Fri, 14 Jan 2022 15:23:49 +0100 Subject: [PATCH] api: Update `update_message` event required fields. Makes `edit_timestamp` and `user_id` required fields for all `update_message` events. Adds `rendering_only` as another required field to signal if events are only updating the rendered content of the message, which is currently the case for adding inline url previews. Updates `test_event.py` so that `do_update_message` and `do_update_embedded_data` refer to the same testing schema for `update_message` events, and therefore reflect the same required fields for the `update_message` event. The OpenAPI definition for `update_message` events is also updated to reflect the required field and descriptions of various properties are updated for the addition of the `rendering_only` property. --- templates/zerver/api/changelog.md | 13 ++++++ version.py | 2 +- zerver/lib/actions.py | 10 ++++- zerver/lib/event_schema.py | 35 +++++++-------- zerver/openapi/zulip.yaml | 72 ++++++++++++++++++++++++------- zerver/tests/test_events.py | 16 ++++++- 6 files changed, 111 insertions(+), 37 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 29e260a9bf..cfd1df729b 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -20,6 +20,19 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 5.0 +**Feature level 114** + +* [`GET /events`](/api/get-events): Added `rendering_only` field to + `update_message` event type to indicate if the message change only + updated the rendering of the message or if it was the result of a + user interaction. + +* [`GET /events`](/api/get-events): Updated `update_message` event type + to always include `edit_timestamp` and `user_id` fields. If the event + only updates the rendering of the message, then the `user_id` field + will be present, but with a value of null as the update was not the + result of a user interaction. + **Feature level 113** * `GET /realm/emoji`, `POST /realm/emoji/{emoji_name}`, [`GET diff --git a/version.py b/version.py index 9862173b3a..18816eda0b 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 = 113 +API_FEATURE_LEVEL = 114 # 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/actions.py b/zerver/lib/actions.py index 054c77123c..fcac293ad9 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -6561,7 +6561,14 @@ def do_update_embedded_data( content: Optional[str], rendering_result: MessageRenderingResult, ) -> None: - event: Dict[str, Any] = {"type": "update_message", "message_id": message.id} + timestamp = timezone_now() + event: Dict[str, Any] = { + "type": "update_message", + "user_id": None, + "edit_timestamp": datetime_to_timestamp(timestamp), + "message_id": message.id, + "rendering_only": True, + } changed_messages = [message] rendered_content: Optional[str] = None @@ -6630,6 +6637,7 @@ def do_update_message( "user_id": user_profile.id, "edit_timestamp": datetime_to_timestamp(timestamp), "message_id": target_message.id, + "rendering_only": False, } edit_history_event: Dict[str, Any] = { diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 7dc4d2c5ec..ac97c21446 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -1516,13 +1516,15 @@ def check_update_global_notifications( assert isinstance(setting, setting_type) +# user_id field is null for embedded variant of update_message update_message_required_fields = [ ("type", Equals("update_message")), - ("user_id", int), + ("user_id", OptionalType(int)), ("edit_timestamp", int), ("message_id", int), ("flags", ListType(str)), ("message_ids", ListType(int)), + ("rendering_only", bool), ] update_message_stream_fields: List[Tuple[str, object]] = [ @@ -1531,11 +1533,14 @@ update_message_stream_fields: List[Tuple[str, object]] = [ ] update_message_content_fields: List[Tuple[str, object]] = [ - ("content", str), ("is_me_message", bool), ("orig_content", str), ("orig_rendered_content", str), ("prev_rendered_content_version", int), +] + +update_message_content_or_embedded_data_fields: List[Tuple[str, object]] = [ + ("content", str), ("rendered_content", str), ] @@ -1566,14 +1571,13 @@ update_message_change_stream_or_topic_fields: List[Tuple[str, object]] = [ update_message_optional_fields = ( update_message_stream_fields + update_message_content_fields + + update_message_content_or_embedded_data_fields + update_message_topic_fields + update_message_change_stream_fields + update_message_change_stream_or_topic_fields ) -# The schema here does not include the "embedded" -# variant of update_message; it is for message -# and topic editing. +# The schema here includes the embedded variant of update_message update_message_event = event_dict_type( required_keys=update_message_required_fields, optional_keys=update_message_optional_fields, @@ -1588,6 +1592,7 @@ def check_update_message( has_content: bool, has_topic: bool, has_new_stream_id: bool, + is_embedded_update_only: bool, ) -> None: # Always check the basic schema first. _check_update_message(var_name, event) @@ -1601,6 +1606,7 @@ def check_update_message( if has_content: expected_keys.update(tup[0] for tup in update_message_content_fields) + expected_keys.update(tup[0] for tup in update_message_content_or_embedded_data_fields) if has_topic: expected_keys.update(tup[0] for tup in update_message_topic_fields) @@ -1610,21 +1616,16 @@ def check_update_message( expected_keys.update(tup[0] for tup in update_message_change_stream_fields) expected_keys.update(tup[0] for tup in update_message_change_stream_or_topic_fields) + if is_embedded_update_only: + expected_keys.update(tup[0] for tup in update_message_content_or_embedded_data_fields) + assert event["user_id"] is None + else: + assert isinstance(event["user_id"], int) + + assert event["rendering_only"] == is_embedded_update_only assert expected_keys == actual_keys -update_message_embedded_event = event_dict_type( - required_keys=[ - ("type", Equals("update_message")), - ("flags", ListType(str)), - ("content", str), - ("message_id", int), - ("message_ids", ListType(int)), - ("rendered_content", str), - ] -) -check_update_message_embedded = make_checker(update_message_embedded_event) - update_message_flags_add_event = event_dict_type( required_keys=[ ("type", Equals("update_message_flags")), diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 12b9c2feca..d007185b10 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1966,9 +1966,14 @@ paths: - type: object additionalProperties: false description: | - Event sent when a message has been edited. + Event sent when a message's content, topic and/or + stream has been edited or when a message's content + has a rendering update, such as for an + [inline URL preview][inline-url-previews]. Sent to all users who had received the original message. + + [inline-url-previews]: https://zulip.readthedocs.io/en/latest/subsystems/sending-messages.html#inline-url-previews properties: id: $ref: "#/components/schemas/EventIdSchema" @@ -1979,20 +1984,42 @@ paths: - update_message user_id: type: integer + nullable: true description: | The ID of the user who sent the message. - Not present if update_message event is for providing inline URL - preview in message. + Null when event is for a rendering update of the original message, + such as for an [inline URL preview][inline-url-previews]. + + **Changes**: As of Zulip 5.0 (feature level 114), this field + is present for all `update_message` events. Previously, this + field was omitted for [inline URL preview][inline-url-previews] + updates. + rendering_only: + type: boolean + description: | + Whether the event only updates the rendered content of the message. + + This field should be used by clients to determine if the event + only provides a rendering update to the message content, + such as for an [inline URL preview][inline-url-previews]. + When `True`, the event does not reflect a user-generated edit + and does not modify the message history. + + **Changes**: New in Zulip 5.0 (feature level 114). Clients can + correctly identify these rendering update event with earlier + Zulip versions by checking whether the `user_id` field was omitted. message_id: type: integer description: | - The ID of the message which was edited. + The ID of the message which was edited or updated. This field should be used to apply content edits to the client's - cached message history. If the stream or topic was changed, the - set of moved messages is encoded in the separate `message_ids` - field, which is guaranteed to include `message_id`. + cached message history, or to apply rendered content updates. + + If the stream or topic was changed, the set of moved messages is + encoded in the separate `message_ids` field, which is guaranteed + to include `message_id`. message_ids: type: array items: @@ -2033,19 +2060,21 @@ paths: The time when this message edit operation was processed by the server. - Not present if update_message event is for providing inline URL - preview in message. + **Changes**: As of Zulip 5.0 (feature level 114), this field + is present for all `update_message` events. Previously, this + field was omitted for [inline URL preview][inline-url-previews] + updates. stream_name: type: string description: | - Only present if the message was originally sent to a stream. + Only present if the message was edited and originally sent to a stream. The name of the stream that the message was sent to. Clients are recommended to use the `stream_id` field instead. stream_id: type: integer description: | - Only present if the message was originally sent to a stream. + Only present if the message was edited and originally sent to a stream. The pre-edit stream for all of the messages with IDs in `message_ids`. @@ -2068,9 +2097,12 @@ paths: topic and/or stream. The choice the editing user made about which messages should be - affected by a stream/topic edit: just the one indicated in - `message_id`, messages in the same topic that had been sent - after this one, or all messages in that topic. + affected by a stream/topic edit: + + - `change_one` => Just change the one indicated in `message_id`. + - `change_later` => Change messages in the same topic that had + been sent after this one. + - `change_all`=> Change all messages in that topic. This parameter should be used to decide whether to change navigation and compose box state in response to the edit. For @@ -2159,14 +2191,18 @@ paths: content: type: string description: | - Only present if this event changed the message content. + Only present if this event changed the message content or + updated the message content for an + [inline URL preview][inline-url-previews]. The new content of the message with ID `message_id`, in the original Markdown. rendered_content: type: string description: | - Only present if this event changed the message content. + Only present if this event changed the message content or + updated the message content for an + [inline URL preview][inline-url-previews]. The new content of the message with ID `message_id`, rendered in HTML. @@ -2182,9 +2218,12 @@ paths: required: - "type" - "id" + - "user_id" - "message_id" - "message_ids" - "flags" + - "edit_timestamp" + - "rendering_only" example: { "type": "update_message", @@ -2205,6 +2244,7 @@ paths: "topic_links": [], "message_ids": [58, 57], "flags": [], + "rendering_only": false, "id": 0, } - type: object diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 40b889128a..c5604cfaad 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -154,7 +154,6 @@ from zerver.lib.event_schema import ( check_update_display_settings, check_update_global_notifications, check_update_message, - check_update_message_embedded, check_update_message_flags_add, check_update_message_flags_remove, check_user_group_add, @@ -466,6 +465,7 @@ class NormalActionsTest(BaseAction): has_content=True, has_topic=False, has_new_stream_id=False, + is_embedded_update_only=False, ) def test_huddle_send_message_events(self) -> None: @@ -526,6 +526,7 @@ class NormalActionsTest(BaseAction): has_content=True, has_topic=False, has_new_stream_id=False, + is_embedded_update_only=False, ) # Verify stream message editing - topic only @@ -555,15 +556,25 @@ class NormalActionsTest(BaseAction): has_content=False, has_topic=True, has_new_stream_id=False, + is_embedded_update_only=False, ) + # Verify special case of embedded content update content = "embed_content" rendering_result = render_markdown(message, content) events = self.verify_action( lambda: do_update_embedded_data(self.user_profile, message, content, rendering_result), state_change_expected=False, ) - check_update_message_embedded("events[0]", events[0]) + check_update_message( + "events[0]", + events[0], + is_stream_message=False, + has_content=False, + has_topic=False, + has_new_stream_id=False, + is_embedded_update_only=True, + ) # Verify move topic to different stream. @@ -602,6 +613,7 @@ class NormalActionsTest(BaseAction): has_content=False, has_topic=False, has_new_stream_id=True, + is_embedded_update_only=False, ) def test_update_message_flags(self) -> None: