custom profile fields: Fix error handling for admin clearing values.

This commit fixes an error in the logic for allowing admins to edit any
user's CPF (custom profile field) values. The logic allowing users to
edit their own CPF values is however sound. What happens is that of all
the CPF types, for "choice fields" as well as "URL" and "date fields",
when the value is reset/deleted/cleared by the admin in the Admin UI
(organization settings), the frontend would send a null (empty string)
value to the backend for that custom profile field (as this is, after
all, the new value in this case). This would then triggers the backend
validators to return an error message.

We fix this by using the method check_remove_custom_profile_field_value,
that both code paths (user editing their own CPFs and admin editing a
user's CPF) can call.

zerver/tests/test_users.py

@@ -395,6 +395,68 @@ class PermissionTest(ZulipTestCase):
                                        {'profile_data': ujson.dumps(new_profile_data)})
             self.assert_json_error(result, error_msg)
 
+        # non-existant field and no data
+        invalid_profile_data = [{
+            'id': 9001,
+            'value': ''
+        }]
+        result = self.client_patch('/json/users/{}'.format(cordelia.id),
+                                   {'profile_data': ujson.dumps(invalid_profile_data)})
+        self.assert_json_error(result, 'Field id 9001 not found.')
+
+        # non-existant field and data
+        invalid_profile_data = [{
+            'id': 9001,
+            'value': 'some data'
+        }]
+        result = self.client_patch('/json/users/{}'.format(cordelia.id),
+                                   {'profile_data': ujson.dumps(invalid_profile_data)})
+        self.assert_json_error(result, 'Field id 9001 not found.')
+
+        # Test for clearing/resetting field values.
+        empty_profile_data = []
+        for field_name in fields:
+            field = CustomProfileField.objects.get(name=field_name, realm=realm)
+            value = ''  # type: Union[str, None, List[Any]]
+            if field.field_type == CustomProfileField.USER:
+                value = []
+            empty_profile_data.append({
+                'id': field.id,
+                'value': value,
+            })
+        result = self.client_patch('/json/users/{}'.format(cordelia.id),
+                                   {'profile_data': ujson.dumps(empty_profile_data)})
+        self.assert_json_success(result)
+        for field_dict in cordelia.profile_data:
+            self.assertEqual(field_dict['value'], None)
+
+        # Test adding some of the field values after removing all.
+        hamlet = self.example_user("hamlet")
+        new_fields = {
+            'Phone number': None,
+            'Biography': 'A test user',
+            'Favorite food': None,
+            'Favorite editor': None,
+            'Birthday': None,
+            'GitHub profile': 'https://github.com/DEF',
+            'Mentor': [hamlet.id]
+        }
+        new_profile_data = []
+        for field_name in fields:
+            field = CustomProfileField.objects.get(name=field_name, realm=realm)
+            value = None
+            if new_fields[field_name]:
+                value = new_fields[field_name]
+            new_profile_data.append({
+                'id': field.id,
+                'value': value,
+            })
+        result = self.client_patch('/json/users/{}'.format(cordelia.id),
+                                   {'profile_data': ujson.dumps(new_profile_data)})
+        self.assert_json_success(result)
+        for field_dict in cordelia.profile_data:
+            self.assertEqual(field_dict['value'], new_fields[str(field_dict['name'])])
+
     def test_non_admin_user_cannot_change_profile_data(self) -> None:
         self.login(self.example_email("cordelia"))
         hamlet = self.example_user("hamlet")

zerver/views/users.py

@@ -19,7 +19,7 @@ from zerver.lib.actions import do_change_avatar_fields, do_change_bot_owner, \
     do_create_user, do_deactivate_user, do_reactivate_user, do_regenerate_api_key, \
     check_change_full_name, notify_created_bot, do_update_outgoing_webhook_service, \
     do_update_bot_config_data, check_change_bot_full_name, do_change_is_guest, \
-    do_update_user_custom_profile_data
+    do_update_user_custom_profile_data, check_remove_custom_profile_field_value
 from zerver.lib.avatar import avatar_url, get_gravatar_url, get_avatar_field
 from zerver.lib.bot_config import set_bot_config
 from zerver.lib.exceptions import JsonableError, CannotDeactivateLastUserError
@@ -113,8 +113,15 @@ def update_user_backend(request: HttpRequest, user_profile: UserProfile, user_id
         check_change_full_name(target, full_name, user_profile)
 
     if profile_data is not None:
-        validate_user_custom_profile_data(target.realm.id, profile_data)
-        do_update_user_custom_profile_data(target, profile_data)
+        clean_profile_data = []
+        for entry in profile_data:
+            if not entry["value"]:
+                field_id = entry["id"]
+                check_remove_custom_profile_field_value(target, field_id)
+            else:
+                clean_profile_data.append(entry)
+        validate_user_custom_profile_data(target.realm.id, clean_profile_data)
+        do_update_user_custom_profile_data(target, clean_profile_data)
 
     return json_success()