types: Fix declared type of custom profile field values.

None of the existing custom profile field types have the value as an
integer like declared in many places - nor is it a string like currently
decalred in types.py. The correct type is Union[str, List[int]]. Rather
than tracking this in so many places throughout the codebase, we add a
new ProfileDataElementValue type and insert it where appropriate.
This commit is contained in:
Mateusz Mandera 2021-09-21 16:52:15 +02:00 committed by Tim Abbott
parent 91ea21a3fc
commit cacff28578
8 changed files with 29 additions and 19 deletions

View File

@ -169,7 +169,7 @@ from zerver.lib.topic import (
update_messages_for_topic_edit, update_messages_for_topic_edit,
) )
from zerver.lib.topic_mutes import add_topic_mute, get_topic_mutes, remove_topic_mute from zerver.lib.topic_mutes import add_topic_mute, get_topic_mutes, remove_topic_mute
from zerver.lib.types import ProfileFieldData from zerver.lib.types import ProfileDataElementValue, ProfileFieldData
from zerver.lib.upload import ( from zerver.lib.upload import (
claim_attachment, claim_attachment,
delete_avatar_image, delete_avatar_image,
@ -7738,7 +7738,7 @@ def notify_user_update_custom_profile_data(
def do_update_user_custom_profile_data_if_changed( def do_update_user_custom_profile_data_if_changed(
user_profile: UserProfile, user_profile: UserProfile,
data: List[Dict[str, Union[int, str, List[int]]]], data: List[Dict[str, Union[int, ProfileDataElementValue]]],
) -> None: ) -> None:
with transaction.atomic(): with transaction.atomic():
for custom_profile_field in data: for custom_profile_field in data:

View File

@ -14,6 +14,9 @@ ExtendedValidator = Callable[[str, str, object], str]
RealmUserValidator = Callable[[int, object, bool], List[int]] RealmUserValidator = Callable[[int, object, bool], List[int]]
ProfileDataElementValue = Union[str, List[int]]
class ProfileDataElementBase(TypedDict): class ProfileDataElementBase(TypedDict):
id: int id: int
name: str name: str
@ -24,13 +27,13 @@ class ProfileDataElementBase(TypedDict):
class ProfileDataElement(ProfileDataElementBase): class ProfileDataElement(ProfileDataElementBase):
value: str value: ProfileDataElementValue
rendered_value: Optional[str] rendered_value: Optional[str]
ProfileData = List[ProfileDataElement] ProfileData = List[ProfileDataElement]
FieldElement = Tuple[int, Promise, Validator[Union[int, str, List[int]]], Callable[[Any], Any], str] FieldElement = Tuple[int, Promise, Validator[ProfileDataElementValue], Callable[[Any], Any], str]
ExtendedFieldElement = Tuple[int, Promise, ExtendedValidator, Callable[[Any], Any], str] ExtendedFieldElement = Tuple[int, Promise, ExtendedValidator, Callable[[Any], Any], str]
UserFieldElement = Tuple[int, Promise, RealmUserValidator, Callable[[Any], Any], str] UserFieldElement = Tuple[int, Promise, RealmUserValidator, Callable[[Any], Any], str]

View File

@ -1,7 +1,7 @@
import re import re
import unicodedata import unicodedata
from collections import defaultdict from collections import defaultdict
from typing import Any, Dict, List, Optional, Sequence, Union from typing import Any, Dict, List, Optional, Sequence, Union, cast
from django.conf import settings from django.conf import settings
from django.core.exceptions import ValidationError from django.core.exceptions import ValidationError
@ -20,6 +20,7 @@ from zerver.lib.cache import (
) )
from zerver.lib.exceptions import JsonableError, OrganizationAdministratorRequired from zerver.lib.exceptions import JsonableError, OrganizationAdministratorRequired
from zerver.lib.timezone import canonicalize_timezone from zerver.lib.timezone import canonicalize_timezone
from zerver.lib.types import ProfileDataElementValue
from zerver.models import ( from zerver.models import (
CustomProfileField, CustomProfileField,
CustomProfileFieldValue, CustomProfileFieldValue,
@ -310,8 +311,8 @@ def get_all_api_keys(user_profile: UserProfile) -> List[str]:
def validate_user_custom_profile_field( def validate_user_custom_profile_field(
realm_id: int, field: CustomProfileField, value: Union[int, str, List[int]] realm_id: int, field: CustomProfileField, value: ProfileDataElementValue
) -> Union[int, str, List[int]]: ) -> ProfileDataElementValue:
validators = CustomProfileField.FIELD_VALIDATORS validators = CustomProfileField.FIELD_VALIDATORS
field_type = field.field_type field_type = field.field_type
var_name = f"{field.name}" var_name = f"{field.name}"
@ -332,7 +333,7 @@ def validate_user_custom_profile_field(
def validate_user_custom_profile_data( def validate_user_custom_profile_data(
realm_id: int, profile_data: List[Dict[str, Union[int, str, List[int]]]] realm_id: int, profile_data: List[Dict[str, Union[int, ProfileDataElementValue]]]
) -> None: ) -> None:
# This function validate all custom field values according to their field type. # This function validate all custom field values according to their field type.
for item in profile_data: for item in profile_data:
@ -343,7 +344,9 @@ def validate_user_custom_profile_data(
raise JsonableError(_("Field id {id} not found.").format(id=field_id)) raise JsonableError(_("Field id {id} not found.").format(id=field_id))
try: try:
validate_user_custom_profile_field(realm_id, field, item["value"]) validate_user_custom_profile_field(
realm_id, field, cast(ProfileDataElementValue, item["value"])
)
except ValidationError as error: except ValidationError as error:
raise JsonableError(error.message) raise JsonableError(error.message)

View File

@ -76,6 +76,7 @@ from zerver.lib.types import (
LinkifierDict, LinkifierDict,
ProfileData, ProfileData,
ProfileDataElementBase, ProfileDataElementBase,
ProfileDataElementValue,
RealmUserValidator, RealmUserValidator,
UserFieldElement, UserFieldElement,
Validator, Validator,
@ -3904,7 +3905,7 @@ class CustomProfileField(models.Model):
ALL_FIELD_TYPES = [*FIELD_TYPE_DATA, *SELECT_FIELD_TYPE_DATA, *USER_FIELD_TYPE_DATA] ALL_FIELD_TYPES = [*FIELD_TYPE_DATA, *SELECT_FIELD_TYPE_DATA, *USER_FIELD_TYPE_DATA]
FIELD_VALIDATORS: Dict[int, Validator[Union[int, str, List[int]]]] = { FIELD_VALIDATORS: Dict[int, Validator[ProfileDataElementValue]] = {
item[0]: item[2] for item in FIELD_TYPE_DATA item[0]: item[2] for item in FIELD_TYPE_DATA
} }
FIELD_CONVERTERS: Dict[int, Callable[[Any], Any]] = { FIELD_CONVERTERS: Dict[int, Callable[[Any], Any]] = {

View File

@ -566,7 +566,7 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase):
self.assert_error_update_invalid_value( self.assert_error_update_invalid_value(
field_name, "1909-3-5", f"{field_name} is not a date" field_name, "1909-3-5", f"{field_name} is not a date"
) )
self.assert_error_update_invalid_value(field_name, 123, f"{field_name} is not a string") self.assert_error_update_invalid_value(field_name, [123], f"{field_name} is not a string")
def test_update_invalid_url(self) -> None: def test_update_invalid_url(self) -> None:
field_name = "Favorite website" field_name = "Favorite website"

View File

@ -20,7 +20,7 @@ from zerver.lib.exceptions import JsonableError
from zerver.lib.external_accounts import validate_external_account_field_data from zerver.lib.external_accounts import validate_external_account_field_data
from zerver.lib.request import REQ, has_request_variables from zerver.lib.request import REQ, has_request_variables
from zerver.lib.response import json_success from zerver.lib.response import json_success
from zerver.lib.types import ProfileFieldData from zerver.lib.types import ProfileDataElementValue, ProfileFieldData
from zerver.lib.users import validate_user_custom_profile_data from zerver.lib.users import validate_user_custom_profile_data
from zerver.lib.validator import ( from zerver.lib.validator import (
check_capped_string, check_capped_string,
@ -196,12 +196,12 @@ def remove_user_custom_profile_data(
def update_user_custom_profile_data( def update_user_custom_profile_data(
request: HttpRequest, request: HttpRequest,
user_profile: UserProfile, user_profile: UserProfile,
data: List[Dict[str, Union[int, str, List[int]]]] = REQ( data: List[Dict[str, Union[int, ProfileDataElementValue]]] = REQ(
json_validator=check_list( json_validator=check_list(
check_dict_only( check_dict_only(
[ [
("id", check_int), ("id", check_int),
("value", check_union([check_int, check_string, check_list(check_int)])), ("value", check_union([check_string, check_list(check_int)])),
] ]
), ),
) )

View File

@ -38,7 +38,7 @@ from zerver.lib.integrations import EMBEDDED_BOTS
from zerver.lib.request import REQ, has_request_variables from zerver.lib.request import REQ, has_request_variables
from zerver.lib.response import json_success from zerver.lib.response import json_success
from zerver.lib.streams import access_stream_by_id, access_stream_by_name, subscribed_to_stream from zerver.lib.streams import access_stream_by_id, access_stream_by_name, subscribed_to_stream
from zerver.lib.types import Validator from zerver.lib.types import ProfileDataElementValue, Validator
from zerver.lib.upload import upload_avatar_image from zerver.lib.upload import upload_avatar_image
from zerver.lib.url_encoding import add_query_arg_to_redirect_url from zerver.lib.url_encoding import add_query_arg_to_redirect_url
from zerver.lib.users import ( from zerver.lib.users import (
@ -141,14 +141,16 @@ def reactivate_user_backend(
return json_success() return json_success()
check_profile_data: Validator[List[Dict[str, Optional[Union[int, str, List[int]]]]]] = check_list( check_profile_data: Validator[
List[Dict[str, Optional[Union[int, ProfileDataElementValue]]]]
] = check_list(
check_dict_only( check_dict_only(
[ [
("id", check_int), ("id", check_int),
( (
"value", "value",
check_none_or( check_none_or(
check_union([check_int, check_string, check_list(check_int)]), check_union([check_string, check_list(check_int)]),
), ),
), ),
] ]
@ -168,7 +170,7 @@ def update_user_backend(
UserProfile.ROLE_TYPES, UserProfile.ROLE_TYPES,
), ),
), ),
profile_data: Optional[List[Dict[str, Optional[Union[int, str, List[int]]]]]] = REQ( profile_data: Optional[List[Dict[str, Optional[Union[int, ProfileDataElementValue]]]]] = REQ(
default=None, default=None,
json_validator=check_profile_data, json_validator=check_profile_data,
), ),

View File

@ -73,6 +73,7 @@ from zerver.lib.rate_limiter import RateLimitedObject
from zerver.lib.redis_utils import get_dict_from_redis, get_redis_client, put_dict_in_redis from zerver.lib.redis_utils import get_dict_from_redis, get_redis_client, put_dict_in_redis
from zerver.lib.request import RequestNotes from zerver.lib.request import RequestNotes
from zerver.lib.subdomains import get_subdomain from zerver.lib.subdomains import get_subdomain
from zerver.lib.types import ProfileDataElementValue
from zerver.lib.url_encoding import add_query_to_redirect_url from zerver.lib.url_encoding import add_query_to_redirect_url
from zerver.lib.users import check_full_name, validate_user_custom_profile_field from zerver.lib.users import check_full_name, validate_user_custom_profile_field
from zerver.models import ( from zerver.models import (
@ -1343,7 +1344,7 @@ def sync_user_profile_custom_fields(
var_name = "_".join(data["name"].lower().split(" ")) var_name = "_".join(data["name"].lower().split(" "))
existing_values[var_name] = data["value"] existing_values[var_name] = data["value"]
profile_data: List[Dict[str, Union[int, str, List[int]]]] = [] profile_data: List[Dict[str, Union[int, ProfileDataElementValue]]] = []
for var_name, value in custom_field_name_to_value.items(): for var_name, value in custom_field_name_to_value.items():
try: try:
field = fields_by_var_name[var_name] field = fields_by_var_name[var_name]