diff --git a/static/js/settings_account.js b/static/js/settings_account.js index d5b111dbd6..9fec76ff63 100644 --- a/static/js/settings_account.js +++ b/static/js/settings_account.js @@ -64,6 +64,8 @@ function add_custom_profile_fields_to_settings() { // 1 & 2 type represent textual data. if (field.type === 1 || field.type === 2) { type = "text"; + } else if (field.type === 3) { + type = "choice"; } else { blueslip.error("Undefined field type."); } diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 11526a208e..676631ea10 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -124,6 +124,7 @@ from zerver.lib.upload import attachment_url_re, attachment_url_to_path_id, \ claim_attachment, delete_message_image, upload_emoji_image from zerver.lib.str_utils import NonBinaryStr, force_str from zerver.tornado.event_queue import request_event_queue, send_event +from zerver.lib.types import ProfileFieldData from analytics.models import StreamCount @@ -4528,9 +4529,13 @@ def notify_realm_custom_profile_fields(realm: Realm, operation: str) -> None: send_event(event, active_user_ids(realm.id)) def try_add_realm_custom_profile_field(realm: Realm, name: Text, field_type: int, - hint: Text='') -> CustomProfileField: + hint: Text='', + field_data: ProfileFieldData=None) -> CustomProfileField: field = CustomProfileField(realm=realm, name=name, field_type=field_type) field.hint = hint + if field.field_type == CustomProfileField.CHOICE: + field.field_data = ujson.dumps(field_data or {}) + field.save() notify_realm_custom_profile_fields(realm, 'add') return field @@ -4544,10 +4549,13 @@ def do_remove_realm_custom_profile_field(realm: Realm, field: CustomProfileField notify_realm_custom_profile_fields(realm, 'delete') def try_update_realm_custom_profile_field(realm: Realm, field: CustomProfileField, - name: Text, hint: Text='') -> None: + name: Text, hint: Text='', + field_data: ProfileFieldData=None) -> None: field.name = name field.hint = hint - field.save(update_fields=['name', 'hint']) + if field.field_type == CustomProfileField.CHOICE: + field.field_data = ujson.dumps(field_data or {}) + field.save() notify_realm_custom_profile_fields(realm, 'update') def do_update_user_custom_profile_data(user_profile: UserProfile, diff --git a/zerver/lib/types.py b/zerver/lib/types.py index ea4dbef6af..9bfcc111f6 100644 --- a/zerver/lib/types.py +++ b/zerver/lib/types.py @@ -1,4 +1,4 @@ -from typing import TypeVar, Callable, Optional, List, Dict, Union +from typing import TypeVar, Callable, Optional, List, Dict, Union, Tuple, Any from django.http import HttpResponse ViewFuncT = TypeVar('ViewFuncT', bound=Callable[..., HttpResponse]) @@ -6,6 +6,14 @@ ViewFuncT = TypeVar('ViewFuncT', bound=Callable[..., HttpResponse]) # See zerver/lib/validator.py for more details of Validators, # including many examples Validator = Callable[[str, object], Optional[str]] +ExtendedValidator = Callable[[str, str, object], Optional[str]] ProfileDataElement = Dict[str, Union[int, float, Optional[str]]] ProfileData = List[ProfileDataElement] + +FieldElement = Tuple[int, str, Validator, Callable[[Any], Any]] +ExtendedFieldElement = Tuple[int, str, ExtendedValidator, Callable[[Any], Any]] + +FieldTypeData = List[Union[FieldElement, ExtendedFieldElement]] + +ProfileFieldData = Dict[str, Dict[str, str]] diff --git a/zerver/lib/validator.py b/zerver/lib/validator.py index d6993eae7a..67bd8c52d1 100644 --- a/zerver/lib/validator.py +++ b/zerver/lib/validator.py @@ -25,19 +25,32 @@ A simple example of composition is this: To extend this concept, it's simply a matter of writing your own validator for any particular type of object. ''' +import ujson from django.utils.translation import ugettext as _ from django.core.exceptions import ValidationError from django.core.validators import validate_email, URLValidator -from typing import Callable, Iterable, Optional, Tuple, TypeVar, Text +from typing import Callable, Iterable, Optional, Tuple, TypeVar, Text, cast, \ + Dict from zerver.lib.request import JsonableError -from zerver.lib.types import Validator +from zerver.lib.types import Validator, ProfileFieldData def check_string(var_name: str, val: object) -> Optional[str]: if not isinstance(val, str): return _('%s is not a string') % (var_name,) return None +def check_required_string(var_name: str, val: object) -> Optional[str]: + error = check_string(var_name, val) + if error: + return error + + val = cast(str, val) + if not val.strip(): + return _("{item} cannot be blank.").format(item=var_name) + + return None + def check_short_string(var_name: str, val: object) -> Optional[str]: return check_capped_string(50)(var_name, val) @@ -174,3 +187,33 @@ def check_url(var_name: str, val: object) -> Optional[str]: return None except ValidationError as err: return _('%s is not a URL') % (var_name,) + +def validate_field_data(field_data: ProfileFieldData) -> Optional[str]: + """ + This function is used to validate the data sent to the server while + creating/editing choices of the choice field in Organization settings. + """ + validator = check_dict_only([ + ('text', check_required_string), + ('order', check_required_string), + ]) + + for key, value in field_data.items(): + if not key.strip(): + return _("'{item}' cannot be blank.").format(item='value') + + error = validator('field_data', value) + if error: + return error + + return None + +def validate_choice_field(var_name: str, field_data: str, value: object) -> None: + """ + This function is used to validate the value selected by the user against a + choice field. This is not used to validate admin data. + """ + field_data_dict = ujson.loads(field_data) + if value not in field_data_dict: + msg = _("'{value}' is not a valid choice for '{field_name}'.") + return msg.format(value=value, field_name=var_name) diff --git a/zerver/migrations/0160_add_choice_field.py b/zerver/migrations/0160_add_choice_field.py new file mode 100644 index 0000000000..f789dda870 --- /dev/null +++ b/zerver/migrations/0160_add_choice_field.py @@ -0,0 +1,25 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.11 on 2018-04-10 04:57 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0159_realm_google_hangouts_domain'), + ] + + operations = [ + migrations.AddField( + model_name='customprofilefield', + name='field_data', + field=models.TextField(default='', null=True), + ), + migrations.AlterField( + model_name='customprofilefield', + name='field_type', + field=models.PositiveSmallIntegerField(choices=[(1, 'Short Text'), (2, 'Long Text'), (3, 'Choice')], default=1), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index 06d4a4b1fd..e39d85c025 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -32,9 +32,10 @@ from django.db.models.signals import pre_save, post_save, post_delete from django.utils.translation import ugettext_lazy as _ from zerver.lib import cache from zerver.lib.validator import check_int, check_float, \ - check_short_string, check_long_string + check_short_string, check_long_string, validate_choice_field from zerver.lib.name_restrictions import is_disposable_domain -from zerver.lib.types import Validator, ProfileDataElement, ProfileData +from zerver.lib.types import Validator, ExtendedValidator, \ + ProfileDataElement, ProfileData, FieldTypeData from django.utils.encoding import force_text @@ -1891,19 +1892,35 @@ class CustomProfileField(models.Model): realm = models.ForeignKey(Realm, on_delete=CASCADE) # type: Realm name = models.CharField(max_length=100) # type: Text hint = models.CharField(max_length=HINT_MAX_LENGTH, default='', null=True) # type: Optional[Text] + # There is no performance overhead of using TextField in PostGreSQL. + # See https://www.postgresql.org/docs/9.0/static/datatype-character.html + field_data = models.TextField(default='', null=True) # type: Optional[Text] SHORT_TEXT = 1 LONG_TEXT = 2 + CHOICE = 3 + + # These are the fields whose validators require field_data + # argument as well. + EXTENDED_FIELD_TYPE_DATA = [ + (CHOICE, 'Choice', validate_choice_field, str), + ] # type: FieldTypeData + + EXTENDED_FIELD_VALIDATORS = { + item[0]: item[2] for item in EXTENDED_FIELD_TYPE_DATA + } # type: Dict[int, ExtendedValidator] FIELD_TYPE_DATA = [ # Type, Name, Validator, Converter (SHORT_TEXT, u'Short Text', check_short_string, str), (LONG_TEXT, u'Long Text', check_long_string, str), - ] # type: List[Tuple[int, Text, Validator, Callable[[Any], Any]]] + ] # type: FieldTypeData + + ALL_FIELD_TYPES = FIELD_TYPE_DATA + EXTENDED_FIELD_TYPE_DATA FIELD_VALIDATORS = {item[0]: item[2] for item in FIELD_TYPE_DATA} # type: Dict[int, Validator] - FIELD_CONVERTERS = {item[0]: item[3] for item in FIELD_TYPE_DATA} # type: Dict[int, Callable[[Any], Any]] - FIELD_TYPE_CHOICES = [(item[0], item[1]) for item in FIELD_TYPE_DATA] # type: List[Tuple[int, Text]] + FIELD_CONVERTERS = {item[0]: item[3] for item in ALL_FIELD_TYPES} # type: Dict[int, Callable[[Any], Any]] + FIELD_TYPE_CHOICES = [(item[0], item[1]) for item in ALL_FIELD_TYPES] # type: List[Tuple[int, Text]] field_type = models.PositiveSmallIntegerField(choices=FIELD_TYPE_CHOICES, default=SHORT_TEXT) # type: int @@ -1917,6 +1934,7 @@ class CustomProfileField(models.Model): 'name': self.name, 'type': self.field_type, 'hint': self.hint, + 'field_data': self.field_data, } def __str__(self) -> str: diff --git a/zerver/tests/test_custom_profile_data.py b/zerver/tests/test_custom_profile_data.py index 5645e4690b..191e061d01 100644 --- a/zerver/tests/test_custom_profile_data.py +++ b/zerver/tests/test_custom_profile_data.py @@ -19,7 +19,7 @@ class CustomProfileFieldTest(ZulipTestCase): self.assert_json_success(result) self.assertEqual(200, result.status_code) content = result.json() - self.assertEqual(len(content["custom_fields"]), 3) + self.assertEqual(len(content["custom_fields"]), 4) def test_create(self) -> None: self.login(self.example_email("iago")) @@ -53,6 +53,59 @@ class CustomProfileFieldTest(ZulipTestCase): self.assert_json_error(result, u'A field with that name already exists.') + def test_create_choice_field(self) -> None: + self.login(self.example_email("iago")) + data = {} # type: Dict[str, Union[str, int]] + data["name"] = "Favorite programming language" + data["field_type"] = CustomProfileField.CHOICE + + data['field_data'] = 'invalid' + result = self.client_post("/json/realm/profile_fields", info=data) + error_msg = "Bad value for 'field_data': invalid" + self.assert_json_error(result, error_msg) + + data["field_data"] = ujson.dumps({ + 'python': ['1'], + 'java': ['2'], + }) + result = self.client_post("/json/realm/profile_fields", info=data) + self.assert_json_error(result, 'field_data is not a dict') + + data["field_data"] = ujson.dumps({ + 'python': {'text': 'Python'}, + 'java': {'text': 'Java'}, + }) + result = self.client_post("/json/realm/profile_fields", info=data) + self.assert_json_error(result, "order key is missing from field_data") + + data["field_data"] = ujson.dumps({ + 'python': {'text': 'Python', 'order': ''}, + 'java': {'text': 'Java', 'order': '2'}, + }) + result = self.client_post("/json/realm/profile_fields", info=data) + self.assert_json_error(result, 'field_data["order"] cannot be blank.') + + data["field_data"] = ujson.dumps({ + '': {'text': 'Python', 'order': '1'}, + 'java': {'text': 'Java', 'order': '2'}, + }) + result = self.client_post("/json/realm/profile_fields", info=data) + self.assert_json_error(result, "'value' cannot be blank.") + + data["field_data"] = ujson.dumps({ + 'python': {'text': 'Python', 'order': 1}, + 'java': {'text': 'Java', 'order': '2'}, + }) + result = self.client_post("/json/realm/profile_fields", info=data) + self.assert_json_error(result, 'field_data["order"] is not a string') + + data["field_data"] = ujson.dumps({ + 'python': {'text': 'Python', 'order': '1'}, + 'java': {'text': 'Java', 'order': '2'}, + }) + result = self.client_post("/json/realm/profile_fields", info=data) + self.assert_json_success(result) + def test_not_realm_admin(self) -> None: self.login(self.example_email("hamlet")) result = self.client_post("/json/realm/profile_fields") @@ -67,11 +120,11 @@ class CustomProfileFieldTest(ZulipTestCase): result = self.client_delete("/json/realm/profile_fields/100") self.assert_json_error(result, 'Field id 100 not found.') - self.assertEqual(CustomProfileField.objects.count(), 3) + self.assertEqual(CustomProfileField.objects.count(), 4) result = self.client_delete( "/json/realm/profile_fields/{}".format(field.id)) self.assert_json_success(result) - self.assertEqual(CustomProfileField.objects.count(), 2) + self.assertEqual(CustomProfileField.objects.count(), 3) def test_update(self) -> None: self.login(self.example_email("iago")) @@ -92,14 +145,14 @@ class CustomProfileFieldTest(ZulipTestCase): field = CustomProfileField.objects.get(name="Phone number", realm=realm) - self.assertEqual(CustomProfileField.objects.count(), 3) + self.assertEqual(CustomProfileField.objects.count(), 4) result = self.client_patch( "/json/realm/profile_fields/{}".format(field.id), info={'name': 'New phone number', 'field_type': CustomProfileField.SHORT_TEXT}) self.assert_json_success(result) field = CustomProfileField.objects.get(id=field.id, realm=realm) - self.assertEqual(CustomProfileField.objects.count(), 3) + self.assertEqual(CustomProfileField.objects.count(), 4) self.assertEqual(field.name, 'New phone number') self.assertIs(field.hint, '') self.assertEqual(field.field_type, CustomProfileField.SHORT_TEXT) @@ -120,11 +173,39 @@ class CustomProfileFieldTest(ZulipTestCase): self.assert_json_success(result) field = CustomProfileField.objects.get(id=field.id, realm=realm) - self.assertEqual(CustomProfileField.objects.count(), 3) + self.assertEqual(CustomProfileField.objects.count(), 4) self.assertEqual(field.name, 'New phone number') self.assertEqual(field.hint, 'New contact number') self.assertEqual(field.field_type, CustomProfileField.SHORT_TEXT) + field = CustomProfileField.objects.get(name="Favorite editor", realm=realm) + result = self.client_patch( + "/json/realm/profile_fields/{}".format(field.id), + info={'name': 'Favorite editor', + 'field_data': 'invalid'}) + self.assert_json_error(result, "Bad value for 'field_data': invalid") + + field_data = ujson.dumps({ + 'vim': 'Vim', + 'emacs': {'order': '2', 'text': 'Emacs'}, + }) + result = self.client_patch( + "/json/realm/profile_fields/{}".format(field.id), + info={'name': 'Favorite editor', + 'field_data': field_data}) + self.assert_json_error(result, "field_data is not a dict") + + field_data = ujson.dumps({ + 'vim': {'order': '1', 'text': 'Vim'}, + 'emacs': {'order': '2', 'text': 'Emacs'}, + 'notepad': {'order': '3', 'text': 'Notepad'}, + }) + result = self.client_patch( + "/json/realm/profile_fields/{}".format(field.id), + info={'name': 'Favorite editor', + 'field_data': field_data}) + self.assert_json_success(result) + def test_update_is_aware_of_uniqueness(self) -> None: self.login(self.example_email("iago")) realm = get_realm('zulip') @@ -137,7 +218,7 @@ class CustomProfileFieldTest(ZulipTestCase): CustomProfileField.SHORT_TEXT ) - self.assertEqual(CustomProfileField.objects.count(), 5) + self.assertEqual(CustomProfileField.objects.count(), 6) result = self.client_patch( "/json/realm/profile_fields/{}".format(field.id), info={'name': 'Phone', 'field_type': CustomProfileField.SHORT_TEXT}) @@ -179,6 +260,7 @@ class CustomProfileDataTest(ZulipTestCase): ('Phone number', 'short text data'), ('Biography', 'long text data'), ('Favorite food', 'short text data'), + ('Favorite editor', 'vim'), ] data = [] @@ -200,10 +282,10 @@ class CustomProfileDataTest(ZulipTestCase): for field_dict in iago.profile_data: self.assertEqual(field_dict['value'], expected_value[field_dict['id']]) - for k in ['id', 'type', 'name']: + for k in ['id', 'type', 'name', 'field_data']: self.assertIn(k, field_dict) - self.assertEqual(len(iago.profile_data), 3) + self.assertEqual(len(iago.profile_data), 4) # Update value of one field. field = CustomProfileField.objects.get(name='Biography', realm=realm) @@ -219,6 +301,30 @@ class CustomProfileDataTest(ZulipTestCase): if f['id'] == field.id: self.assertEqual(f['value'], 'foobar') + def test_update_choice_field(self) -> None: + self.login(self.example_email("iago")) + realm = get_realm('zulip') + field = CustomProfileField.objects.get(name='Favorite editor', + realm=realm) + data = [{ + 'id': field.id, + 'value': 'foobar', + }] + + result = self.client_patch("/json/users/me/profile_data", + {'data': ujson.dumps(data)}) + self.assert_json_error(result, + "'foobar' is not a valid choice for 'value[4]'.") + + data = [{ + 'id': field.id, + 'value': 'emacs', + }] + + result = self.client_patch("/json/users/me/profile_data", + {'data': ujson.dumps(data)}) + self.assert_json_success(result) + def test_delete(self) -> None: user_profile = self.example_user('iago') realm = user_profile.realm @@ -226,10 +332,10 @@ class CustomProfileDataTest(ZulipTestCase): data = [{'id': field.id, 'value': u'123456'}] # type: List[Dict[str, Union[int, Text]]] do_update_user_custom_profile_data(user_profile, data) - self.assertEqual(len(custom_profile_fields_for_realm(realm.id)), 3) - self.assertEqual(user_profile.customprofilefieldvalue_set.count(), 3) + self.assertEqual(len(custom_profile_fields_for_realm(realm.id)), 4) + self.assertEqual(user_profile.customprofilefieldvalue_set.count(), 4) do_remove_realm_custom_profile_field(realm, field) - self.assertEqual(len(custom_profile_fields_for_realm(realm.id)), 2) - self.assertEqual(user_profile.customprofilefieldvalue_set.count(), 2) + self.assertEqual(len(custom_profile_fields_for_realm(realm.id)), 3) + self.assertEqual(user_profile.customprofilefieldvalue_set.count(), 3) diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 4be2cb14b7..45bb4dadd7 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -917,6 +917,7 @@ class EventsRegisterTest(ZulipTestCase): ('type', check_int), ('name', check_string), ('hint', check_string), + ('field_data', check_string), ]))), ]) diff --git a/zerver/views/custom_profile_fields.py b/zerver/views/custom_profile_fields.py index 69c9de78e5..7e7fa4b108 100644 --- a/zerver/views/custom_profile_fields.py +++ b/zerver/views/custom_profile_fields.py @@ -1,6 +1,7 @@ from typing import Union, List, Dict, Optional import logging +import ujson from django.core.exceptions import ValidationError from django.db import IntegrityError, connection @@ -14,7 +15,9 @@ from zerver.lib.actions import (try_add_realm_custom_profile_field, try_update_realm_custom_profile_field, do_update_user_custom_profile_data) from zerver.lib.response import json_success, json_error -from zerver.lib.validator import check_dict, check_list, check_int, check_capped_string +from zerver.lib.types import ProfileFieldData +from zerver.lib.validator import (check_dict, check_list, check_int, + validate_field_data, check_capped_string) from zerver.models import (custom_profile_fields_for_realm, UserProfile, CustomProfileField, custom_profile_fields_for_realm) @@ -30,6 +33,8 @@ hint_validator = check_capped_string(CustomProfileField.HINT_MAX_LENGTH) def create_realm_custom_profile_field(request: HttpRequest, user_profile: UserProfile, name: str=REQ(), hint: str=REQ(default=''), + field_data: ProfileFieldData=REQ(default={}, + converter=ujson.loads), field_type: int=REQ(validator=check_int)) -> HttpResponse: if not name.strip(): return json_error(_("Name cannot be blank.")) @@ -42,10 +47,15 @@ def create_realm_custom_profile_field(request: HttpRequest, if field_type not in field_types: return json_error(_("Invalid field type.")) + error = validate_field_data(field_data) + if error: + return json_error(error) + try: field = try_add_realm_custom_profile_field( realm=user_profile.realm, name=name, + field_data=field_data, field_type=field_type, hint=hint, ) @@ -69,7 +79,9 @@ def delete_realm_custom_profile_field(request: HttpRequest, user_profile: UserPr @has_request_variables def update_realm_custom_profile_field(request: HttpRequest, user_profile: UserProfile, field_id: int, name: str=REQ(), - hint: str=REQ(default='') + hint: str=REQ(default=''), + field_data: ProfileFieldData=REQ(default={}, + converter=ujson.loads), ) -> HttpResponse: if not name.strip(): return json_error(_("Name cannot be blank.")) @@ -78,6 +90,10 @@ def update_realm_custom_profile_field(request: HttpRequest, user_profile: UserPr if error: return json_error(error, data={'field': 'hint'}) + error = validate_field_data(field_data) + if error: + return json_error(error) + realm = user_profile.realm try: field = CustomProfileField.objects.get(realm=realm, id=field_id) @@ -85,7 +101,8 @@ def update_realm_custom_profile_field(request: HttpRequest, user_profile: UserPr return json_error(_('Field id {id} not found.').format(id=field_id)) try: - try_update_realm_custom_profile_field(realm, field, name, hint=hint) + try_update_realm_custom_profile_field(realm, field, name, hint=hint, + field_data=field_data) except IntegrityError: return json_error(_('A field with that name already exists.')) return json_success() @@ -104,8 +121,21 @@ def update_user_custom_profile_data( except CustomProfileField.DoesNotExist: return json_error(_('Field id {id} not found.').format(id=field_id)) - validator = CustomProfileField.FIELD_VALIDATORS[field.field_type] - result = validator('value[{}]'.format(field_id), item['value']) + validators = CustomProfileField.FIELD_VALIDATORS + extended_validators = CustomProfileField.EXTENDED_FIELD_VALIDATORS + field_type = field.field_type + value = item['value'] + if field_type in validators: + validator = validators[field_type] + var_name = 'value[{}]'.format(field_id) + result = validator(var_name, value) + else: + # Check extended validators. + extended_validator = extended_validators[field_type] + field_data = field.field_data + var_name = 'value[{}]'.format(field_id) + result = extended_validator(var_name, field_data, value) + if result is not None: return json_error(result) diff --git a/zilencer/management/commands/populate_db.py b/zilencer/management/commands/populate_db.py index 057785e97d..b2a7ac7645 100644 --- a/zilencer/management/commands/populate_db.py +++ b/zilencer/management/commands/populate_db.py @@ -266,6 +266,14 @@ class Command(BaseCommand): favorite_food = try_add_realm_custom_profile_field(zulip_realm, "Favorite food", CustomProfileField.SHORT_TEXT, hint="Or drink, if you'd prefer") + field_data = { + 'vim': {'text': 'Vim', 'order': '1'}, + 'emacs': {'text': 'Emacs', 'order': '2'}, + } + favorite_editor = try_add_realm_custom_profile_field(zulip_realm, + "Favorite editor", + CustomProfileField.CHOICE, + field_data=field_data) # Fill in values for Iago and Hamlet hamlet = get_user("hamlet@zulip.com", zulip_realm) @@ -273,11 +281,13 @@ class Command(BaseCommand): {"id": phone_number.id, "value": "+1-234-567-8901"}, {"id": biography.id, "value": "Betrayer of Othello."}, {"id": favorite_food.id, "value": "Apples"}, + {"id": favorite_editor.id, "value": "emacs"}, ]) do_update_user_custom_profile_data(hamlet, [ {"id": phone_number.id, "value": "+0-11-23-456-7890"}, {"id": biography.id, "value": "Prince of Denmark, and other things!"}, {"id": favorite_food.id, "value": "Dark chocolate"}, + {"id": favorite_editor.id, "value": "vim"}, ]) else: zulip_realm = get_realm("zulip")