Merge pull request #5899 from edx/alasdair/logistration-select-validation

ECOM-626 Added required check for select dropdowns and validation

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.")
 

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

common/djangoapps/user_api/views.py

@@ -393,7 +393,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
         )
 
@@ -406,7 +407,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
         )
 
@@ -420,7 +422,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
         )
 
@@ -477,7 +480,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
         )
 
@@ -548,12 +552,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.
 

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 = [
+            '<select id="dropdown" name="country">',
+                '<option value="" data-isdefault="true">Please select a country</option>',
+                '<option value="BE">Belgium</option>',
+                '<option value="DE">Germany</option>',
+            '</select>'
+        ].join('');
+
+        setFixtures(select);
+
+        field = $('#dropdown');
+
+        // Optional
+        expectValid();
+
+        // Required, default text selected
+        field.attr('required', true);
+        expectInvalid(REQUIRED_ERROR_FRAGMENT);
+
+        // Required, country selected
+        field.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, '');

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: {

lms/static/sass/views/_login-register.scss

@@ -210,6 +210,10 @@
 
         select {
             width: 100%;
+
+            &.error {
+                border-color: tint($red,50%);
+            }
         }
         /** FROM _accounts.scss - end **/
     }

lms/templates/student_account/form_field.underscore

@@ -10,9 +10,10 @@
         <select id="<%= form %>-<%= name %>"
             name="<%= name %>"
             class="input-inline"
-            aria-describedby="<%= form %>-<%= name %>-desc">
+            aria-describedby="<%= form %>-<%= name %>-desc"
+            <% if ( required ) { %> required<% } %>>
         <% _.each(options, function(el) { %>
-            <option value="<%= el.value%>"><%= el.name %></option>
+            <option value="<%= el.value%>"<% if ( el.default ) { %> data-isdefault="true"<% } %>><%= el.name %></option>
         <% }); %>
         </select>
     <% } else if ( type === 'textarea' ) { %>