From cb5c03f0d1fb06ee9c03316271add3f76ea8f0b7 Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Mon, 10 Nov 2014 09:58:25 -0500 Subject: [PATCH] ECOM-626 Added required check for select dropdowns and validation --- common/djangoapps/user_api/helpers.py | 19 ++++++++++-- .../djangoapps/user_api/tests/test_views.py | 8 ++--- common/djangoapps/user_api/views.py | 18 +++++------ .../static/js/spec/edx.utils.validate_spec.js | 31 +++++++++++++++++-- common/static/js/utils/edx.utils.validate.js | 13 +++++++- lms/static/sass/views/_login-register.scss | 4 +++ .../student_account/form_field.underscore | 5 +-- 7 files changed, 75 insertions(+), 23 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index bfd7d79d50..f6cd34d3a6 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -125,7 +125,7 @@ class FormDescription(object): def add_field( self, name, label=u"", field_type=u"text", default=u"", placeholder=u"", instructions=u"", required=True, restrictions=None, - options=None, error_messages=None + options=None, include_default_option=False, error_messages=None ): """Add a field to the form description. @@ -158,6 +158,9 @@ class FormDescription(object): and `display_name` is the name to display to the user. If the field type is "select", you *must* provide this kwarg. + include_default_option (boolean): If True, include a "default" empty option + at the beginning of the options list. + error_messages (dict): Custom validation error messages. Currently, the only supported key is "required" indicating that the messages should be displayed if the user does @@ -188,10 +191,20 @@ class FormDescription(object): if field_type == "select": if options is not None: - field_dict["options"] = [ + field_dict["options"] = [] + + # Include an empty "default" option at the beginning of the list + if include_default_option: + field_dict["options"].append({ + "value": "", + "name": "--", + "default": True + }) + + field_dict["options"].extend([ {"value": option_value, "name": option_name} for option_value, option_name in options - ] + ]) else: raise InvalidFieldError("You must provide options for a select field.") diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 4b0c67a159..d6319c549b 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -955,7 +955,7 @@ class RegistrationViewTest(ApiTestCase): "required": False, "label": "Highest Level of Education Completed", "options": [ - {"value": "", "name": "--"}, + {"value": "", "name": "--", "default": True}, {"value": "p", "name": "Doctorate"}, {"value": "m", "name": "Master's or professional degree"}, {"value": "b", "name": "Bachelor's degree"}, @@ -978,7 +978,7 @@ class RegistrationViewTest(ApiTestCase): "required": False, "label": "Gender", "options": [ - {"value": "", "name": "--"}, + {"value": "", "name": "--", "default": True}, {"value": "m", "name": "Male"}, {"value": "f", "name": "Female"}, {"value": "o", "name": "Other"}, @@ -989,7 +989,7 @@ class RegistrationViewTest(ApiTestCase): def test_register_form_year_of_birth(self): this_year = datetime.datetime.now(UTC).year year_options = ( - [{"value": "", "name": "--"}] + [ + [{"value": "", "name": "--", "default": True}] + [ {"value": unicode(year), "name": unicode(year)} for year in range(this_year, this_year - 120, -1) ] @@ -1042,7 +1042,7 @@ class RegistrationViewTest(ApiTestCase): def test_registration_form_country(self): country_options = ( - [{"name": "--", "value": ""}] + + [{"name": "--", "value": "", "default": True}] + [ {"value": country_code, "name": unicode(country_name)} for country_code, country_name in SORTED_COUNTRIES diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 81b791f18f..bab9ed2ec9 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -379,7 +379,8 @@ class RegistrationView(APIView): "level_of_education", label=education_level_label, field_type="select", - options=self._options_with_default(UserProfile.LEVEL_OF_EDUCATION_CHOICES), + options=UserProfile.LEVEL_OF_EDUCATION_CHOICES, + include_default_option=True, required=required ) @@ -392,7 +393,8 @@ class RegistrationView(APIView): "gender", label=gender_label, field_type="select", - options=self._options_with_default(UserProfile.GENDER_CHOICES), + options=UserProfile.GENDER_CHOICES, + include_default_option=True, required=required ) @@ -406,7 +408,8 @@ class RegistrationView(APIView): "year_of_birth", label=yob_label, field_type="select", - options=self._options_with_default(options), + options=options, + include_default_option=True, required=required ) @@ -463,7 +466,8 @@ class RegistrationView(APIView): "country", label=country_label, field_type="select", - options=self._options_with_default(options), + options=options, + include_default_option=True, required=required ) @@ -534,12 +538,6 @@ class RegistrationView(APIView): } ) - def _options_with_default(self, options): - """Include a default option as the first option. """ - return ( - [("", "--")] + list(options) - ) - def _apply_third_party_auth_overrides(self, request, form_desc): """Modify the registration form if the user has authenticated with a third-party provider. diff --git a/common/static/js/spec/edx.utils.validate_spec.js b/common/static/js/spec/edx.utils.validate_spec.js index a755a16c5d..cb84f43f74 100644 --- a/common/static/js/spec/edx.utils.validate_spec.js +++ b/common/static/js/spec/edx.utils.validate_spec.js @@ -52,10 +52,10 @@ describe('edx.utils.validate', function () { createFixture('text', 'username', true, MIN_LENGTH, MAX_LENGTH, ''); expectInvalid(REQUIRED_ERROR_FRAGMENT); }); - + it('fails if a field is provided a value below its minimum character limit', function () { createFixture('text', 'username', false, MIN_LENGTH, MAX_LENGTH, SHORT_STRING); - + // Verify optional field behavior expectInvalid(MIN_ERROR_FRAGMENT); @@ -66,7 +66,7 @@ describe('edx.utils.validate', function () { it('succeeds if a field with no minimum character limit is provided a value below its maximum character limit', function () { createFixture('text', 'username', false, null, MAX_LENGTH, SHORT_STRING); - + // Verify optional field behavior expectValid(); @@ -154,6 +154,31 @@ describe('edx.utils.validate', function () { expectInvalid(REQUIRED_ERROR_FRAGMENT); }); + it('succeeds if a select is optional, or required and default is selected, but fails if a required select has the default option selected', function () { + var select = [ + '' + ].join(''); + + setFixtures(select); + + dropdown = $('#dropdown'); + + // Optional + expectValid(); + + // Required, default text selected + dropdown.attr('required', true); + expectInvalid(REQUIRED_ERROR_FRAGMENT); + + // Required, country selected + dropdown.val('BE'); + expectValid(); + }); + it('returns a custom error message if an invalid field has one attached', function () { // Create a blank required field createFixture('text', 'username', true, MIN_LENGTH, MAX_LENGTH, ''); diff --git a/common/static/js/utils/edx.utils.validate.js b/common/static/js/utils/edx.utils.validate.js index 45868a8554..b20a10016e 100644 --- a/common/static/js/utils/edx.utils.validate.js +++ b/common/static/js/utils/edx.utils.validate.js @@ -87,7 +87,18 @@ var edx = edx || {}; }, isBlank: function( $el ) { - return ( $el.attr('type') === 'checkbox' ) ? !$el.prop('checked') : !$el.val(); + var type = $el.attr('type'), + isBlank; + + if ( type === 'checkbox' ) { + isBlank = !$el.prop('checked'); + } else if ( type === 'select' ) { + isBlank = ( $el.data('isdefault') === true ); + } else { + isBlank = !$el.val(); + } + + return isBlank; }, email: { diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index c73938e9c4..f1c2cb92ee 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -208,6 +208,10 @@ select { width: 100%; + + &.error { + border-color: tint($red,50%); + } } /** FROM _accounts.scss - end **/ } diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index d65d030414..46b13dd24d 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -10,9 +10,10 @@ <% } else if ( type === 'textarea' ) { %>