diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 7d1f0a132b..e5e5bf225e 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -22,8 +22,8 @@ format used by the Zulip server that they are interacting with. **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 +* `PATCH /realm/profile_fields/{field_id}`: `name`, `hint` and `field_data` 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. diff --git a/zerver/actions/custom_profile_fields.py b/zerver/actions/custom_profile_fields.py index e534f57af4..e17fec2ad6 100644 --- a/zerver/actions/custom_profile_fields.py +++ b/zerver/actions/custom_profile_fields.py @@ -114,11 +114,20 @@ def try_update_realm_custom_profile_field( field.display_in_profile_summary = display_in_profile_summary field.required = required - if field.field_type in (CustomProfileField.SELECT, CustomProfileField.EXTERNAL_ACCOUNT): - if field.field_type == CustomProfileField.SELECT: - assert field_data is not None + + if field.field_type in ( + CustomProfileField.SELECT, + CustomProfileField.EXTERNAL_ACCOUNT, + ): + # If field_data is None, field_data is unchanged and there is no need for + # comparing field_data values. + if field_data is not None and field.field_type == CustomProfileField.SELECT: remove_custom_profile_field_value_if_required(field, field_data) - field.field_data = orjson.dumps(field_data or {}).decode() + + # If field.field_data is the default empty string, we will set field_data + # to an empty dict. + if field_data is not None or field.field_data == "": + field.field_data = orjson.dumps(field_data or {}).decode() field.save() notify_realm_custom_profile_fields(realm) diff --git a/zerver/tests/test_custom_profile_data.py b/zerver/tests/test_custom_profile_data.py index f9481e64fb..2205c9ecdc 100644 --- a/zerver/tests/test_custom_profile_data.py +++ b/zerver/tests/test_custom_profile_data.py @@ -601,6 +601,17 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): field.refresh_from_db() self.assertEqual(field.hint, "") + field = CustomProfileField.objects.get(name="Favorite editor", realm=realm) + + # Empty field_data should not be allowed + result = self.client_patch( + f"/json/realm/profile_fields/{field.id}", + info={ + "field_data": {}, + }, + ) + self.assert_json_error(result, "Field must have at least one choice.") + def test_update_is_aware_of_uniqueness(self) -> None: self.login("iago") realm = get_realm("zulip") diff --git a/zerver/views/custom_profile_fields.py b/zerver/views/custom_profile_fields.py index ebe4b29818..5abbbf30b2 100644 --- a/zerver/views/custom_profile_fields.py +++ b/zerver/views/custom_profile_fields.py @@ -118,8 +118,8 @@ def validate_custom_profile_field( def validate_custom_profile_field_update( field: CustomProfileField, - field_data: ProfileFieldData, display_in_profile_summary: bool, + field_data: Optional[ProfileFieldData] = None, name: Optional[str] = None, hint: Optional[str] = None, ) -> None: @@ -127,8 +127,21 @@ def validate_custom_profile_field_update( name = field.name if hint is None: hint = field.hint + if field_data is None: + if field.field_data == "": + # We're passing this just for validation, sinec the function won't + # accept a string. This won't change the actual value. + field_data = {} + else: + field_data = orjson.loads(field.field_data) + + assert field_data is not None validate_custom_profile_field( - name, hint, field.field_type, field_data, display_in_profile_summary + name, + hint, + field.field_type, + field_data, + display_in_profile_summary, ) @@ -138,15 +151,18 @@ check_profile_field_data: Validator[ProfileFieldData] = check_dict( def update_only_display_in_profile_summary( - requested_field_data: ProfileFieldData, existing_field: CustomProfileField, + requested_field_data: Optional[ProfileFieldData] = None, requested_name: Optional[str] = None, requested_hint: Optional[str] = None, ) -> bool: if ( (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) + or ( + requested_field_data is not None + and requested_field_data != orjson.loads(existing_field.field_data) + ) ): return False return True @@ -226,7 +242,9 @@ def update_realm_custom_profile_field( field_id: int, 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), + field_data: Optional[ProfileFieldData] = REQ( + default=None, 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), ) -> HttpResponse: @@ -249,13 +267,16 @@ def update_realm_custom_profile_field( # of default external account types, but not any others. # # TODO: Make the name/hint/field_data parameters optional, and - # just require that None was passed for all of them for this case. + # explicitly require that the client passes None for all of them for this case. + # Right now, for name/hint/field_data we allow the client to send the existing + # values for the respective fields. After this TODO is done, we will only allow + # the client to pass None values if the field is unchanged. and is_default_external_field(field.field_type, orjson.loads(field.field_data)) - and not update_only_display_in_profile_summary(field_data, field, name, hint) + and not update_only_display_in_profile_summary(field, field_data, name, hint) ): raise JsonableError(_("Default custom field cannot be updated.")) - validate_custom_profile_field_update(field, field_data, display_in_profile_summary, name, hint) + validate_custom_profile_field_update(field, display_in_profile_summary, field_data, name, hint) try: try_update_realm_custom_profile_field( realm=realm,