From c6c392bcc714f88e32f96a2f0c4729471842d4e4 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Sat, 30 Mar 2024 14:26:26 +0530 Subject: [PATCH] custom_profile_fields: Make name & hint optional during update. Fixes #23022. Added a new validation function that validates updates to custom profile fields. --- api_docs/changelog.md | 7 +++++ zerver/actions/custom_profile_fields.py | 11 ++++--- zerver/tests/test_custom_profile_data.py | 21 +++++++------ zerver/tests/test_events.py | 10 ++++-- zerver/views/custom_profile_fields.py | 40 ++++++++++++++++-------- 5 files changed, 60 insertions(+), 29 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 4f49975805..7d1f0a132b 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 9.0 +**Feature level 252** + +* `PATCH /realm/profile_fields/{field_id}`: `name` and `hint` fields are now + optional during an update. Previously we required the clients to populate + the fields in the PATCH request even if there was no change to those fields' + values. + **Feature level 251** * [`POST /register`](/api/register-queue): Fixed `realm_upload_quota_mib` diff --git a/zerver/actions/custom_profile_fields.py b/zerver/actions/custom_profile_fields.py index 6b2aa6a063..e534f57af4 100644 --- a/zerver/actions/custom_profile_fields.py +++ b/zerver/actions/custom_profile_fields.py @@ -101,14 +101,17 @@ def remove_custom_profile_field_value_if_required( def try_update_realm_custom_profile_field( realm: Realm, field: CustomProfileField, - name: str, - hint: str = "", + name: Optional[str] = None, + hint: Optional[str] = None, field_data: Optional[ProfileFieldData] = None, display_in_profile_summary: bool = False, required: bool = False, ) -> None: - field.name = name - field.hint = hint + if name is not None: + field.name = name + if hint is not None: + field.hint = hint + field.display_in_profile_summary = display_in_profile_summary field.required = required if field.field_type in (CustomProfileField.SELECT, CustomProfileField.EXTERNAL_ACCOUNT): diff --git a/zerver/tests/test_custom_profile_data.py b/zerver/tests/test_custom_profile_data.py index 73ea04b6c8..f9481e64fb 100644 --- a/zerver/tests/test_custom_profile_data.py +++ b/zerver/tests/test_custom_profile_data.py @@ -494,7 +494,6 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ - "name": "New phone number", "hint": "*" * 81, }, ) @@ -504,8 +503,6 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ - "name": "New phone number", - "hint": "New contact number", "display_in_profile_summary": "invalid value", }, ) @@ -515,8 +512,6 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ - "name": "New phone number", - "hint": "New contact number", "required": "invalid value", }, ) @@ -554,7 +549,7 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): field = CustomProfileField.objects.get(name="Favorite editor", realm=realm) result = self.client_patch( f"/json/realm/profile_fields/{field.id}", - info={"name": "Favorite editor", "field_data": "invalid"}, + info={"field_data": "invalid"}, ) self.assert_json_error(result, 'Argument "field_data" is not valid JSON.') @@ -566,7 +561,7 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): ).decode() result = self.client_patch( f"/json/realm/profile_fields/{field.id}", - info={"name": "Favorite editor", "field_data": field_data}, + info={"field_data": field_data}, ) self.assert_json_error(result, "field_data is not a dict") @@ -580,7 +575,6 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ - "name": "Favorite editor", "field_data": field_data, "display_in_profile_summary": "true", }, @@ -591,8 +585,6 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ - "name": field.name, - "hint": field.hint, "display_in_profile_summary": "true", }, ) @@ -600,6 +592,15 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): result, "Only 2 custom profile fields can be displayed in the profile summary." ) + # Empty string for hint should set it to an empty string + result = self.client_patch( + f"/json/realm/profile_fields/{field.id}", + info={"hint": ""}, + ) + self.assert_json_success(result) + field.refresh_from_db() + self.assertEqual(field.hint, "") + def test_update_is_aware_of_uniqueness(self) -> None: self.login("iago") realm = get_realm("zulip") diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index ad5f04306c..3e8c944e91 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1390,7 +1390,11 @@ class NormalActionsTest(BaseAction): events = self.verify_action( lambda: try_update_realm_custom_profile_field( - realm, field, name, hint=hint, display_in_profile_summary=display_in_profile_summary + realm=realm, + field=field, + name=name, + hint=hint, + display_in_profile_summary=display_in_profile_summary, ) ) check_custom_profile_fields("events[0]", events[0]) @@ -1416,7 +1420,9 @@ class NormalActionsTest(BaseAction): hint = "What pronouns should people use to refer you?" events = self.verify_action( - lambda: try_update_realm_custom_profile_field(realm, field, name, hint=hint), + lambda: try_update_realm_custom_profile_field( + realm=realm, field=field, name=name, hint=hint + ), pronouns_field_type_supported=False, ) check_custom_profile_fields("events[0]", events[0]) diff --git a/zerver/views/custom_profile_fields.py b/zerver/views/custom_profile_fields.py index 8db3729d0d..ebe4b29818 100644 --- a/zerver/views/custom_profile_fields.py +++ b/zerver/views/custom_profile_fields.py @@ -116,20 +116,36 @@ def validate_custom_profile_field( validate_display_in_profile_summary_field(field_type, display_in_profile_summary) +def validate_custom_profile_field_update( + field: CustomProfileField, + field_data: ProfileFieldData, + display_in_profile_summary: bool, + name: Optional[str] = None, + hint: Optional[str] = None, +) -> None: + if name is None: + name = field.name + if hint is None: + hint = field.hint + validate_custom_profile_field( + name, hint, field.field_type, field_data, display_in_profile_summary + ) + + check_profile_field_data: Validator[ProfileFieldData] = check_dict( value_validator=check_union([check_dict(value_validator=check_string), check_string]) ) def update_only_display_in_profile_summary( - requested_name: str, - requested_hint: str, requested_field_data: ProfileFieldData, existing_field: CustomProfileField, + requested_name: Optional[str] = None, + requested_hint: Optional[str] = None, ) -> bool: if ( - requested_name != existing_field.name - or requested_hint != existing_field.hint + (requested_name is not None and requested_name != existing_field.name) + or (requested_hint is not None and requested_hint != existing_field.hint) or requested_field_data != orjson.loads(existing_field.field_data) ): return False @@ -208,8 +224,8 @@ def update_realm_custom_profile_field( request: HttpRequest, user_profile: UserProfile, field_id: int, - name: str = REQ(default="", converter=lambda var_name, x: x.strip()), - hint: str = REQ(default=""), + name: Optional[str] = REQ(default=None, converter=lambda var_name, x: x.strip()), + hint: Optional[str] = REQ(default=None), field_data: ProfileFieldData = REQ(default={}, json_validator=check_profile_field_data), display_in_profile_summary: bool = REQ(default=False, json_validator=check_bool), required: bool = REQ(default=False, json_validator=check_bool), @@ -235,18 +251,16 @@ def update_realm_custom_profile_field( # TODO: Make the name/hint/field_data parameters optional, and # just require that None was passed for all of them for this case. and is_default_external_field(field.field_type, orjson.loads(field.field_data)) - and not update_only_display_in_profile_summary(name, hint, field_data, field) + and not update_only_display_in_profile_summary(field_data, field, name, hint) ): raise JsonableError(_("Default custom field cannot be updated.")) - validate_custom_profile_field( - name, hint, field.field_type, field_data, display_in_profile_summary - ) + validate_custom_profile_field_update(field, field_data, display_in_profile_summary, name, hint) try: try_update_realm_custom_profile_field( - realm, - field, - name, + realm=realm, + field=field, + name=name, hint=hint, field_data=field_data, display_in_profile_summary=display_in_profile_summary,