From 96e035a2f01e081de562b5adbe4589e350dcb7e9 Mon Sep 17 00:00:00 2001 From: Gaurav Pandey Date: Sat, 8 May 2021 22:11:54 +0530 Subject: [PATCH] api: Fix encoding of strings in streams endpoint. * Remove unnecessary json_validator for string parameters. * Update frontend to pass right parameter. Bump api feature level and highlight the fix for `emojiset` parameter of `settings/display` endpoint in zulip.yaml file. Fixes part of #18035. --- static/js/stream_edit.js | 4 +- templates/zerver/api/changelog.md | 7 ++++ version.py | 2 +- zerver/openapi/zulip.yaml | 63 +++++++++++++++++-------------- zerver/tests/test_subs.py | 54 ++++++++------------------ zerver/views/streams.py | 4 +- 6 files changed, 62 insertions(+), 72 deletions(-) diff --git a/static/js/stream_edit.js b/static/js/stream_edit.js index bc67c9e800..e80c92aa01 100644 --- a/static/js/stream_edit.js +++ b/static/js/stream_edit.js @@ -652,7 +652,7 @@ export function change_stream_name(e) { channel.patch({ // Stream names might contain unsafe characters so we must encode it first. url: "/json/streams/" + stream_id, - data: {new_name: JSON.stringify(new_name)}, + data: {new_name}, success() { new_name_box.val(""); ui_report.success( @@ -705,7 +705,7 @@ export function change_stream_description(e) { // Description might contain unsafe characters so we must encode it first. url: "/json/streams/" + stream_id, data: { - description: JSON.stringify(description), + description, }, success() { // The event from the server will update the rest of the UI diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 0358680faa..51c972b062 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,13 @@ below features are supported. ## Changes in Zulip 4.0 +**Feature level 64** + +* `PATCH /streams/{stream_id}`: Removed unnecessary JSON-encoding of string + parameters `new_name` and `description`. +* `PATCH /settings/display`: Removed unnecessary JSON-encoding of string + parameters `default_view`, `emojiset` and `timezone`. + **Feature level 63** * `PATCH /settings/notifications`: Removed unnecessary JSON-encoding of string diff --git a/version.py b/version.py index b90a70f7c3..d6306e5c02 100644 --- a/version.py +++ b/version.py @@ -30,7 +30,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 63 +API_FEATURE_LEVEL = 64 # 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/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 26329e2f4e..1f5a87597f 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -9323,11 +9323,11 @@ paths: in: query description: | Notification sound name. - content: - application/json: - schema: - type: string - example: ding + + **Changes**: Removed unnecessary JSON-encoding of parameter in Zulip 4.0 (feature level 63). + schema: + type: string + example: ding - name: enable_desktop_notifications in: query description: | @@ -9598,12 +9598,12 @@ paths: This controls both the Zulip UI as well as email notifications sent to the user. The value needs to be a standard language code that the Zulip server has - translation data for; for example, `"en"` for English` or `"de"` for German. - content: - application/json: - schema: - type: string - example: "en" + translation data for; for example, `"en"` for English or `"de"` for German. + + **Changes**: Removed unnecessary JSON-encoding of parameter in Zulip 4.0 (feature level 63). + schema: + type: string + example: en - name: default_view in: query description: | @@ -9612,9 +9612,11 @@ paths: * "recent_topics" - Recent topics view * "all_messages" - All messages view + + **Changes**: Removed unnecessary JSON-encoding of parameter in Zulip 4.0 (feature level 64). schema: type: string - example: '"all_messages"' + example: all_messages - name: left_side_userlist in: query description: | @@ -9634,6 +9636,8 @@ paths: * "google-blob" - Google classic * "twitter" - Twitter * "text" - Plain text + + **Changes**: Removed unnecessary JSON-encoding of parameter in Zulip 4.0 (feature level 64). schema: type: string example: "google" @@ -9661,11 +9665,11 @@ paths: Timezone values supported by the server are served at [/static/generated/timezones.json](/static/generated/timezones.json). - content: - application/json: - schema: - type: string - example: "Asia/Kolkata" + + **Changes**: Removed unnecessary JSON-encoding of parameter in Zulip 4.0 (feature level 64). + schema: + type: string + example: "Asia/Kolkata" responses: "200": description: Success @@ -9950,22 +9954,25 @@ paths: - name: description in: query description: | - The new description for the stream. - content: - application/json: - schema: - type: string - example: This stream is related to football dicsussions. + The new description for the stream. Limited Zulip markdown is allowed in this + field. + + **Changes**: Removed unnecessary JSON-encoding of this parameter in + Zulip 4.0 (feature level 64). + schema: + type: string + example: "Discuss Italian history and travel destinations." required: false - name: new_name in: query description: | The new name for the stream. - content: - application/json: - schema: - type: string - example: Italy + + **Changes**: Removed unnecessary JSON-encoding of this parameter in + Zulip 4.0 (feature level 64). + schema: + type: string + example: Italy required: false - name: is_private in: query diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 5f4c0043a8..2e2eca1584 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -759,31 +759,21 @@ class StreamAdminTest(ZulipTestCase): stream = self.subscribe(user_profile, "stream_name1") do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None) - result = self.client_patch( - f"/json/streams/{stream.id}", {"new_name": orjson.dumps("stream_name1").decode()} - ) + result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "stream_name1"}) self.assert_json_error(result, "Stream already has that name!") - result = self.client_patch( - f"/json/streams/{stream.id}", {"new_name": orjson.dumps("Denmark").decode()} - ) + result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "Denmark"}) self.assert_json_error(result, "Stream name 'Denmark' is already taken.") - result = self.client_patch( - f"/json/streams/{stream.id}", {"new_name": orjson.dumps("denmark ").decode()} - ) + result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "denmark "}) self.assert_json_error(result, "Stream name 'denmark' is already taken.") # Do a rename that is case-only--this should succeed. - result = self.client_patch( - f"/json/streams/{stream.id}", {"new_name": orjson.dumps("sTREAm_name1").decode()} - ) + result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "sTREAm_name1"}) self.assert_json_success(result) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): stream_id = get_stream("stream_name1", user_profile.realm).id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("stream_name2").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "stream_name2"}) self.assert_json_success(result) event = events[1]["event"] self.assertEqual( @@ -813,9 +803,7 @@ class StreamAdminTest(ZulipTestCase): # *NOTE: Here encoding is needed when Unicode string is passed as an argument* with tornado_redirected_to_list(events): stream_id = stream_name2_exists.id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("नया नाम").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "नया नाम"}) self.assert_json_success(result) # While querying, system can handle Unicode strings. stream_name_uni_exists = get_stream("नया नाम", realm) @@ -828,7 +816,7 @@ class StreamAdminTest(ZulipTestCase): stream_id = stream_name_uni_exists.id result = self.client_patch( f"/json/streams/{stream_id}", - {"new_name": orjson.dumps("नाम में क्या रक्खा हे").decode()}, + {"new_name": "नाम में क्या रक्खा हे"}, ) self.assert_json_success(result) # While querying, system can handle Unicode strings. @@ -840,9 +828,7 @@ class StreamAdminTest(ZulipTestCase): # Test case to change name from one language to other. with tornado_redirected_to_list(events): stream_id = stream_name_new_uni_exists.id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("français").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français"}) self.assert_json_success(result) stream_name_fr_exists = get_stream("français", realm) self.assertTrue(stream_name_fr_exists) @@ -850,9 +836,7 @@ class StreamAdminTest(ZulipTestCase): # Test case to change name to mixed language name. with tornado_redirected_to_list(events): stream_id = stream_name_fr_exists.id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("français name").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français name"}) self.assert_json_success(result) stream_name_mixed_exists = get_stream("français name", realm) self.assertTrue(stream_name_mixed_exists) @@ -867,7 +851,7 @@ class StreamAdminTest(ZulipTestCase): stream_id = get_stream("stream_private_name1", realm).id result = self.client_patch( f"/json/streams/{stream_id}", - {"new_name": orjson.dumps("stream_private_name2").decode()}, + {"new_name": "stream_private_name2"}, ) self.assert_json_success(result) notified_user_ids = set(events[1]["users"]) @@ -896,7 +880,7 @@ class StreamAdminTest(ZulipTestCase): with tornado_redirected_to_list(events): result = self.client_patch( f"/json/streams/{new_stream.id}", - {"new_name": orjson.dumps("stream_rename").decode()}, + {"new_name": "stream_rename"}, ) self.assert_json_success(result) self.assertEqual(len(events), 3) @@ -914,9 +898,7 @@ class StreamAdminTest(ZulipTestCase): self.assertFalse(sub.is_stream_admin) stream_id = get_stream("stream_name1", user_profile.realm).id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("stream_name2").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "stream_name2"}) self.assert_json_error(result, "Must be an organization or stream administrator") def test_notify_on_stream_rename(self) -> None: @@ -926,9 +908,7 @@ class StreamAdminTest(ZulipTestCase): stream = self.subscribe(user_profile, "stream_name1") do_change_user_role(user_profile, UserProfile.ROLE_REALM_ADMINISTRATOR, acting_user=None) - result = self.client_patch( - f"/json/streams/{stream.id}", {"new_name": orjson.dumps("stream_name2").decode()} - ) + result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "stream_name2"}) self.assert_json_success(result) # Inspect the notification message sent @@ -956,9 +936,7 @@ class StreamAdminTest(ZulipTestCase): self.assert_json_success(result) stream_id = get_stream("private_stream", iago.realm).id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("new_private_stream").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "new_private_stream"}) self.assert_json_success(result) result = self.client_patch( @@ -992,9 +970,7 @@ class StreamAdminTest(ZulipTestCase): stream_id = get_stream("private_stream_1", hamlet.realm).id - result = self.client_patch( - f"/json/streams/{stream_id}", {"new_name": orjson.dumps("private_stream_2").decode()} - ) + result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "private_stream_2"}) self.assert_json_error(result, "Invalid stream id") result = self.client_patch( diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 23d0c638b1..070bd8e18c 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -244,7 +244,7 @@ def update_stream_backend( user_profile: UserProfile, stream_id: int, description: Optional[str] = REQ( - json_validator=check_capped_string(Stream.MAX_DESCRIPTION_LENGTH), default=None + str_validator=check_capped_string(Stream.MAX_DESCRIPTION_LENGTH), default=None ), is_private: Optional[bool] = REQ(json_validator=check_bool, default=None), is_announcement_only: Optional[bool] = REQ(json_validator=check_bool, default=None), @@ -252,7 +252,7 @@ def update_stream_backend( json_validator=check_int_in(Stream.STREAM_POST_POLICY_TYPES), default=None ), history_public_to_subscribers: Optional[bool] = REQ(json_validator=check_bool, default=None), - new_name: Optional[str] = REQ(json_validator=check_string, default=None), + new_name: Optional[str] = REQ(default=None), message_retention_days: Optional[Union[int, str]] = REQ( json_validator=check_string_or_int, default=None ),