settings: Use cleaner validators for display settings.

This simplifies the update_display_settings endpoint to use REQ for
validation, rather than custom if/else statements.

The test changes just take advantage of the now more consistent
syntax.

zerver/lib/validator.py

@@ -31,7 +31,8 @@ from django.utils.translation import ugettext as _
 from django.conf import settings
 from django.core.exceptions import ValidationError
 from django.core.validators import validate_email, URLValidator
-from typing import Any, Dict, Iterable, Optional, Tuple, cast, List, Callable, TypeVar
+from typing import Any, Dict, Iterable, Optional, Tuple, cast, List, Callable, TypeVar, \
+    Collection
 
 from datetime import datetime
 from zerver.lib.request import JsonableError
@@ -70,7 +71,7 @@ def check_required_string(var_name: str, val: object) -> Optional[str]:
 
     return None
 
-def check_string_in(possible_values: List[str]) -> Validator:
+def check_string_in(possible_values: Collection[str]) -> Validator:
     @set_type_structure("str")
     def validator(var_name: str, val: object) -> Optional[str]:
         not_str = check_string(var_name, val)

zerver/tests/test_settings.py

@@ -4,7 +4,7 @@ import ujson
 
 from django.http import HttpResponse
 from django.test import override_settings
-from typing import Any, Dict, Union
+from typing import Any, Dict
 
 from zerver.lib.initial_password import initial_password
 from zerver.lib.test_classes import ZulipTestCase
@@ -332,8 +332,8 @@ class ChangeSettingsTest(ZulipTestCase):
         if test_value is None:
             raise AssertionError('No test created for %s' % (setting_name,))
 
-        if setting_name == 'demote_inactive_streams':
+        if isinstance(test_value, int):
-            invalid_value = 4  # type: Union[int, str]
+            invalid_value = 100  # type: Any
         else:
             invalid_value = 'invalid_' + setting_name
         data = {setting_name: ujson.dumps(test_value)}
@@ -350,11 +350,7 @@ class ChangeSettingsTest(ZulipTestCase):
         result = self.client_patch("/json/settings/display", data)
         # the json error for multiple word setting names (ex: default_language)
         # displays as 'Invalid language'. Using setting_name.split('_') to format.
-        if setting_name == 'demote_inactive_streams':
+        self.assert_json_error(result, "Invalid %s" % (setting_name,))
-            self.assert_json_error(result, "Invalid setting value '%s'" % (invalid_value,))
-        else:
-            self.assert_json_error(result, "Invalid %s '%s'" % (setting_name.split('_')[-1],
-                                                                invalid_value))
 
         user_profile = self.example_user('hamlet')
         self.assertNotEqual(getattr(user_profile, setting_name), invalid_value)
@@ -378,7 +374,7 @@ class ChangeSettingsTest(ZulipTestCase):
 
         for emojiset in banned_emojisets:
             result = self.do_change_emojiset(emojiset)
-            self.assert_json_error(result, "Invalid emojiset '%s'" % (emojiset,))
+            self.assert_json_error(result, "Invalid emojiset")
 
         for emojiset in valid_emojisets:
             result = self.do_change_emojiset(emojiset)

zerver/views/user_settings.py

@@ -21,7 +21,8 @@ from zerver.lib.send_email import send_email, FromAddress
 from zerver.lib.i18n import get_available_language_codes
 from zerver.lib.response import json_success, json_error
 from zerver.lib.upload import upload_avatar_image
-from zerver.lib.validator import check_bool, check_string, check_int
+from zerver.lib.validator import check_bool, check_string, check_int, check_int_in, \
+    check_string_in
 from zerver.lib.rate_limiter import RateLimited, get_rate_limit_result_from_request
 from zerver.lib.request import JsonableError
 from zerver.lib.timezone import get_all_timezones
@@ -141,6 +142,10 @@ def json_change_settings(request: HttpRequest, user_profile: UserProfile,
 
     return json_success(result)
 
+language_codes = set(get_available_language_codes())
+all_timezones = set(get_all_timezones())
+emojiset_choices = set([emojiset['key'] for emojiset in UserProfile.emojiset_choices()])
+
 @human_users_only
 @has_request_variables
 def update_display_settings_backend(
@@ -152,28 +157,15 @@ def update_display_settings_backend(
         high_contrast_mode: Optional[bool]=REQ(validator=check_bool, default=None),
         night_mode: Optional[bool]=REQ(validator=check_bool, default=None),
         translate_emoticons: Optional[bool]=REQ(validator=check_bool, default=None),
-        default_language: Optional[bool]=REQ(validator=check_string, default=None),
+        default_language: Optional[bool]=REQ(validator=check_string_in(
+            language_codes), default=None),
         left_side_userlist: Optional[bool]=REQ(validator=check_bool, default=None),
-        emojiset: Optional[str]=REQ(validator=check_string, default=None),
+        emojiset: Optional[str]=REQ(validator=check_string_in(
-        demote_inactive_streams: Optional[int]=REQ(validator=check_int, default=None),
+            emojiset_choices), default=None),
-        timezone: Optional[str]=REQ(validator=check_string, default=None)) -> HttpResponse:
+        demote_inactive_streams: Optional[int]=REQ(validator=check_int_in(
-
+            UserProfile.DEMOTE_STREAMS_CHOICES), default=None),
-    if (default_language is not None and
+        timezone: Optional[str]=REQ(validator=check_string_in(all_timezones),
-            default_language not in get_available_language_codes()):
+                                    default=None)) -> HttpResponse:
-        raise JsonableError(_("Invalid language '%s'") % (default_language,))
-
-    if (timezone is not None and
-            timezone not in get_all_timezones()):
-        raise JsonableError(_("Invalid timezone '%s'") % (timezone,))
-
-    if (emojiset is not None and
-            emojiset not in [emojiset_choice['key'] for emojiset_choice in UserProfile.emojiset_choices()]):
-        raise JsonableError(_("Invalid emojiset '%s'") % (emojiset,))
-
-    if (demote_inactive_streams is not None and
-            demote_inactive_streams not in UserProfile.DEMOTE_STREAMS_CHOICES):
-        raise JsonableError(_("Invalid setting value '%s'") % (demote_inactive_streams,))
-
     request_settings = {k: v for k, v in list(locals().items()) if k in user_profile.property_types}
     result = {}  # type: Dict[str, Any]
     for k, v in list(request_settings.items()):