From cb034d4f2faab29d145f1bc941b3d5aeddd47f50 Mon Sep 17 00:00:00 2001 From: Uman Shahzad Date: Tue, 27 Jun 2017 06:58:04 +0500 Subject: [PATCH] Implement client-side registration form validation. Input forms that need validation will have AJAX requests performed to get validation decisions live. All but a few important and common form fields perform generic validation; these will need a back-end handler in the future in order to have them validated through AJAX requests. Information is conveyed on focus and blur for both errors and successes. --- .../student/tests/test_create_account.py | 10 +- .../student/tests/test_long_username_email.py | 4 +- .../common/js/utils/edx.utils.validate.js | 16 +- common/test/acceptance/tests/lms/test_lms.py | 5 +- conf/locale/en/LC_MESSAGES/django.po | 129 ++++---- conf/locale/en/LC_MESSAGES/djangojs.po | 4 + .../js/spec/student_account/register_spec.js | 177 +++++++++-- lms/static/js/student_account/tos_modal.js | 1 + .../js/student_account/views/FormView.js | 116 +++++-- .../js/student_account/views/RegisterView.js | 288 +++++++++++++++-- lms/static/sass/views/_login-register.scss | 31 +- .../student_account/form_field.underscore | 113 ++++--- .../djangoapps/user_api/accounts/__init__.py | 32 +- .../core/djangoapps/user_api/accounts/api.py | 292 ++++++++++-------- .../user_api/accounts/tests/test_api.py | 11 +- .../user_api/accounts/tests/testutils.py | 30 ++ openedx/core/djangoapps/user_api/errors.py | 5 + .../djangoapps/user_api/tests/test_views.py | 23 +- .../user_api/validation/tests/test_views.py | 129 ++++---- .../djangoapps/user_api/validation/views.py | 46 ++- openedx/core/djangoapps/user_api/views.py | 74 +++-- 21 files changed, 1079 insertions(+), 457 deletions(-) diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index 2195970574..da11e8e1a6 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/common/djangoapps/student/tests/test_create_account.py @@ -22,7 +22,9 @@ from notification_prefs import NOTIFICATION_PREF_KEY from openedx.core.djangoapps.external_auth.models import ExternalAuthMap from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin -from openedx.core.djangoapps.user_api.accounts import USERNAME_INVALID_CHARS_ASCII, USERNAME_INVALID_CHARS_UNICODE +from openedx.core.djangoapps.user_api.accounts import ( + USERNAME_BAD_LENGTH_MSG, USERNAME_INVALID_CHARS_ASCII, USERNAME_INVALID_CHARS_UNICODE +) from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from student.models import UserAttribute from student.views import REGISTRATION_AFFILIATE_ID, REGISTRATION_UTM_CREATED_AT, REGISTRATION_UTM_PARAMETERS @@ -476,16 +478,16 @@ class TestCreateAccountValidation(TestCase): # Missing del params["username"] - assert_username_error("Username must be minimum of two characters long") + assert_username_error(USERNAME_BAD_LENGTH_MSG) # Empty, too short for username in ["", "a"]: params["username"] = username - assert_username_error("Username must be minimum of two characters long") + assert_username_error(USERNAME_BAD_LENGTH_MSG) # Too long params["username"] = "this_username_has_31_characters" - assert_username_error("Username cannot be more than 30 characters long") + assert_username_error(USERNAME_BAD_LENGTH_MSG) # Invalid params["username"] = "invalid username" diff --git a/common/djangoapps/student/tests/test_long_username_email.py b/common/djangoapps/student/tests/test_long_username_email.py index b1e15adbbf..f949440ba2 100644 --- a/common/djangoapps/student/tests/test_long_username_email.py +++ b/common/djangoapps/student/tests/test_long_username_email.py @@ -5,6 +5,8 @@ import json from django.core.urlresolvers import reverse from django.test import TestCase +from openedx.core.djangoapps.user_api.accounts import USERNAME_BAD_LENGTH_MSG + class TestLongUsernameEmail(TestCase): @@ -34,7 +36,7 @@ class TestLongUsernameEmail(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Username cannot be more than 30 characters long", + USERNAME_BAD_LENGTH_MSG, ) def test_long_email(self): diff --git a/common/static/common/js/utils/edx.utils.validate.js b/common/static/common/js/utils/edx.utils.validate.js index a6af7aedff..57e7f84134 100644 --- a/common/static/common/js/utils/edx.utils.validate.js +++ b/common/static/common/js/utils/edx.utils.validate.js @@ -21,7 +21,7 @@ var _fn = { validate: { - template: _.template('
  • <%= content %>
    • '), + template: _.template('
  • <%- content %>
    • '), msg: { email: gettext("The email address you've provided isn't formatted correctly."), @@ -32,6 +32,7 @@ field: function(el) { var $el = $(el), + id = $el.attr('id'), required = true, min = true, max = true, @@ -66,6 +67,8 @@ }); } + response.id = id; + return response; }, @@ -107,7 +110,7 @@ regex: new RegExp( [ '(^[-!#$%&\'*+/=?^_`{}|~0-9A-Z]+(\\.[-!#$%&\'*+/=?^_`{}|~0-9A-Z]+)*', - '|^"([\\001-\\010\\013\\014\\016-\\037!#-\\[\\]-\\177]|\\\\[\\001-\\011\\013\\014\\016-\\177])*"', + '|^"([\\001-\\010\\013\\014\\016-\\037!#-\\[\\]-\\177]|\\\\[\\001-\\011\\013\\014\\016-\\177])*"', // eslint-disable-line max-len ')@((?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\\.)+[A-Z]{2,6}\\.?$)', '|\\[(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}\\]$' ].join(''), 'i' @@ -124,7 +127,7 @@ getLabel: function(id) { // Extract the field label, remove the asterisk (if it appears) and any extra whitespace - return $('label[for=' + id + ']').text().split('*')[0].trim(); + return $('label[for=' + id + '] > span.label-text').text().split('*')[0].trim(); }, getMessage: function($el, tests) { @@ -154,7 +157,10 @@ content = _.sprintf(_fn.validate.msg[key], context); } - txt.push(_fn.validate.template({content: content})); + txt.push(_fn.validate.template({ + content: content, + id: $el.attr('id') + })); } }); @@ -173,7 +179,7 @@ return { validate: _fn.validate.field }; - })(); + }()); return utils; }); diff --git a/common/test/acceptance/tests/lms/test_lms.py b/common/test/acceptance/tests/lms/test_lms.py index 116068681a..b0ef31309d 100644 --- a/common/test/acceptance/tests/lms/test_lms.py +++ b/common/test/acceptance/tests/lms/test_lms.py @@ -344,10 +344,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest): # Verify that the expected errors are displayed. errors = self.register_page.wait_for_errors() self.assertIn(u'Please enter your Public Username.', errors) - self.assertIn( - u'You must agree to the édX Terms of Service and Honor Code', - errors - ) + self.assertIn(u'You must agree to the édX Terms of Service and Honor Code', errors) self.assertIn(u'Please select your Country.', errors) self.assertIn(u'Please tell us your favorite movie.', errors) diff --git a/conf/locale/en/LC_MESSAGES/django.po b/conf/locale/en/LC_MESSAGES/django.po index 5ec35013b9..6243d2c92c 100644 --- a/conf/locale/en/LC_MESSAGES/django.po +++ b/conf/locale/en/LC_MESSAGES/django.po @@ -330,31 +330,6 @@ msgstr "" msgid "User profile" msgstr "" -#: common/djangoapps/student/forms.py -msgid "Username must be minimum of two characters long" -msgstr "" - -#: common/djangoapps/student/forms.py -#, python-format -msgid "Username cannot be more than %(limit_value)s characters long" -msgstr "" - -#. Translators: This message is shown when the Unicode usernames are NOT -#. allowed -#: common/djangoapps/student/forms.py -msgid "" -"Usernames can only contain Roman letters, western numerals (0-9), " -"underscores (_), and hyphens (-)." -msgstr "" - -#. Translators: This message is shown only when the Unicode usernames are -#. allowed -#: common/djangoapps/student/forms.py -msgid "" -"Usernames can only contain letters, numerals, underscore (_), numbers and " -"@/./+/-/_ characters." -msgstr "" - #: common/djangoapps/student/forms.py msgid "" "That e-mail address doesn't have an associated user account. Are you sure " @@ -9288,23 +9263,83 @@ msgstr "" msgid "Enable course home page improvements." msgstr "" -#: openedx/core/djangoapps/theming/views.py +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "" +"Usernames can only contain letters (A-Z, a-z), numerals (0-9), underscores " +"(_), and hyphens (-)." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "" +"Usernames can only contain letters, numerals, and @/./+/-/_ characters." +msgstr "" + +#. Translators: This message is shown to users who attempt to create a new +#. account using +#. an invalid email format. +#: openedx/core/djangoapps/user_api/accounts/__init__.py #, python-brace-format -msgid "Site theme changed to {site_theme}" +msgid "\"{email}\" is not a valid email address." msgstr "" -#: openedx/core/djangoapps/theming/views.py +#: openedx/core/djangoapps/user_api/accounts/__init__.py #, python-brace-format -msgid "Theme {site_theme} does not exist" +msgid "" +"It looks like {email_address} belongs to an existing account. Try again with" +" a different email address." msgstr "" -#: openedx/core/djangoapps/theming/views.py -msgid "Site theme reverted to the default" +#: openedx/core/djangoapps/user_api/accounts/__init__.py +#, python-brace-format +msgid "" +"It looks like {username} belongs to an existing account. Try again with a " +"different username." msgstr "" -#: openedx/core/djangoapps/theming/views.py -#: openedx/core/djangoapps/theming/templates/theming/theming-admin-fragment.html -msgid "Theming Administration" +#. Translators: This message is shown to users who enter a +#. username/email/password +#. with an inappropriate length (too short or too long). +#: openedx/core/djangoapps/user_api/accounts/__init__.py +#, python-brace-format +msgid "Username must be between {min} and {max} characters long." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +#, python-brace-format +msgid "Enter a valid email address that contains at least {min} characters." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "Please enter a password." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "Password is not long enough." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +#, python-brace-format +msgid "Password cannot be longer than {max} character." +msgstr "" + +#. Translators: This message is shown to users who enter a password matching +#. the username they enter(ed). +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "Password cannot be the same as the username." +msgstr "" + +#. Translators: These messages are shown to users who do not enter information +#. into the required field or enter it incorrectly. +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "Please enter your Full Name." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "The email addresses do not match." +msgstr "" + +#: openedx/core/djangoapps/user_api/accounts/__init__.py +msgid "Please select your Country." msgstr "" #: openedx/core/djangoapps/user_api/accounts/api.py @@ -9392,24 +9427,6 @@ msgstr "" msgid "Remember me" msgstr "" -#. Translators: This message is shown to users who attempt to create a new -#. account using an email address associated with an existing account. -#: openedx/core/djangoapps/user_api/views.py -#, python-brace-format -msgid "" -"It looks like {email_address} belongs to an existing account. Try again with" -" a different email address." -msgstr "" - -#. Translators: This message is shown to users who attempt to create a new -#. account using a username associated with an existing account. -#: openedx/core/djangoapps/user_api/views.py -#, python-brace-format -msgid "" -"It looks like {username} belongs to an existing account. Try again with a " -"different username." -msgstr "" - #. Translators: These instructions appear on the registration form, #. immediately #. below a field meant to hold the user's email address. @@ -9423,10 +9440,6 @@ msgstr "" msgid "Confirm Email" msgstr "" -#: openedx/core/djangoapps/user_api/views.py -msgid "The email addresses do not match." -msgstr "" - #. Translators: This example name is used as a placeholder in #. a field on the registration form meant to hold the user's name. #: openedx/core/djangoapps/user_api/views.py @@ -9505,10 +9518,6 @@ msgstr "" msgid "Company" msgstr "" -#: openedx/core/djangoapps/user_api/views.py -msgid "Please select your Country." -msgstr "" - #: openedx/core/djangoapps/user_api/views.py msgid "Review the Honor Code" msgstr "" diff --git a/conf/locale/en/LC_MESSAGES/djangojs.po b/conf/locale/en/LC_MESSAGES/djangojs.po index 6e72b476c9..65d38c71aa 100644 --- a/conf/locale/en/LC_MESSAGES/djangojs.po +++ b/conf/locale/en/LC_MESSAGES/djangojs.po @@ -4218,6 +4218,10 @@ msgstr "" msgid "We couldn't create your account." msgstr "" +#: lms/static/js/student_account/views/RegisterView.js +msgid "(required)" +msgstr "" + #: lms/static/js/student_account/views/RegisterView.js msgid "You've successfully signed into %(currentProvider)s." msgstr "" diff --git a/lms/static/js/spec/student_account/register_spec.js b/lms/static/js/spec/student_account/register_spec.js index d035155ac7..b50e82c8f8 100644 --- a/lms/static/js/spec/student_account/register_spec.js +++ b/lms/static/js/spec/student_account/register_spec.js @@ -30,6 +30,17 @@ confirm_email: 'xsy@edx.org', honor_code: true }, + $email = null, + $name = null, + $username = null, + $password = null, + $levelOfEducation = null, + $gender = null, + $yearOfBirth = null, + $mailingAddress = null, + $goals = null, + $confirmEmail = null, + $honorCode = null, THIRD_PARTY_AUTH = { currentProvider: null, providers: [ @@ -49,9 +60,26 @@ } ] }, + VALIDATION_DECISIONS_POSITIVE = { + validation_decisions: { + email: '', + username: '', + password: '', + confirm_email: '' + } + }, + VALIDATION_DECISIONS_NEGATIVE = { + validation_decisions: { + email: 'Error.', + username: 'Error.', + password: 'Error.', + confirm_email: 'Error' + } + }, FORM_DESCRIPTION = { method: 'post', submit_url: '/user_api/v1/account/registration/', + validation_url: '/api/user/v1/validation/registration', fields: [ { placeholder: 'username@domain.com', @@ -110,10 +138,10 @@ defaultValue: '', type: 'select', options: [ - {value: '', name: '--'}, - {value: 'p', name: 'Doctorate'}, - {value: 'm', name: "Master's or professional degree"}, - {value: 'b', name: "Bachelor's degree"} + {value: '', name: '--'}, + {value: 'p', name: 'Doctorate'}, + {value: 'm', name: "Master's or professional degree"}, + {value: 'b', name: "Bachelor's degree"} ], required: false, instructions: 'Select your education level.', @@ -126,10 +154,10 @@ defaultValue: '', type: 'select', options: [ - {value: '', name: '--'}, - {value: 'm', name: 'Male'}, - {value: 'f', name: 'Female'}, - {value: 'o', name: 'Other'} + {value: '', name: '--'}, + {value: 'm', name: 'Male'}, + {value: 'f', name: 'Female'}, + {value: 'o', name: 'Other'} ], required: false, instructions: 'Select your gender.', @@ -142,10 +170,10 @@ defaultValue: '', type: 'select', options: [ - {value: '', name: '--'}, - {value: 1900, name: '1900'}, - {value: 1950, name: '1950'}, - {value: 2014, name: '2014'} + {value: '', name: '--'}, + {value: 1900, name: '1900'}, + {value: 1950, name: '1950'}, + {value: 2014, name: '2014'} ], required: false, instructions: 'Select your year of birth.', @@ -185,7 +213,6 @@ } ] }; - var createRegisterView = function(that) { // Initialize the register model model = new RegisterModel({}, { @@ -209,6 +236,43 @@ view.on('auth-complete', function() { authComplete = true; }); + + // Target each form field. + $email = $('#register-email'); + $confirmEmail = $('#register-confirm_email'); + $name = $('#register-name'); + $username = $('#register-username'); + $password = $('#register-password'); + $levelOfEducation = $('#register-level_of_education'); + $gender = $('#register-gender'); + $yearOfBirth = $('#register-year_of_birth'); + $mailingAddress = $('#register-mailing_address'); + $goals = $('#register-goals'); + $honorCode = $('#register-honor_code'); + }; + + var fillData = function() { + $email.val(USER_DATA.email); + $confirmEmail.val(USER_DATA.email); + $name.val(USER_DATA.name); + $username.val(USER_DATA.username); + $password.val(USER_DATA.password); + $levelOfEducation.val(USER_DATA.level_of_education); + $gender.val(USER_DATA.gender); + $yearOfBirth.val(USER_DATA.year_of_birth); + $mailingAddress.val(USER_DATA.mailing_address); + $goals.val(USER_DATA.goals); + // Check the honor code checkbox + $honorCode.prop('checked', USER_DATA.honor_code); + }; + + var liveValidate = function($el, validationSuccess) { + $el.focus(); + if (!_.isUndefined(validationSuccess) && !validationSuccess) { + model.trigger('validation', $el, VALIDATION_DECISIONS_NEGATIVE); + } else { + model.trigger('validation', $el, VALIDATION_DECISIONS_POSITIVE); + } }; var submitForm = function(validationSuccess) { @@ -216,19 +280,7 @@ var clickEvent = $.Event('click'); // Simulate manual entry of registration form data - $('#register-email').val(USER_DATA.email); - $('#register-confirm_email').val(USER_DATA.email); - $('#register-name').val(USER_DATA.name); - $('#register-username').val(USER_DATA.username); - $('#register-password').val(USER_DATA.password); - $('#register-level_of_education').val(USER_DATA.level_of_education); - $('#register-gender').val(USER_DATA.gender); - $('#register-year_of_birth').val(USER_DATA.year_of_birth); - $('#register-mailing_address').val(USER_DATA.mailing_address); - $('#register-goals').val(USER_DATA.goals); - - // Check the honor code checkbox - $('#register-honor_code').prop('checked', USER_DATA.honor_code); + fillData(); // If validationSuccess isn't passed, we avoid // spying on `view.validate` twice @@ -238,6 +290,10 @@ isValid: validationSuccess, message: 'Submission was validated.' }); + // Successful validation means there's no need to use AJAX calls from liveValidate, + if (validationSuccess) { + spyOn(view, 'liveValidate').and.callFake(function() {}); + } } // Submit the email address @@ -284,6 +340,7 @@ if (param === '?course_id') { return encodeURIComponent(COURSE_ID); } + return null; }); // Attempt to register @@ -308,17 +365,17 @@ expect($('.button-oa2-facebook')).toBeVisible(); }); - it('validates registration form fields', function() { + it('validates registration form fields on form submission', function() { createRegisterView(this); // Submit the form, with successful validation submitForm(true); // Verify that validation of form fields occurred - expect(view.validate).toHaveBeenCalledWith($('#register-email')[0]); - expect(view.validate).toHaveBeenCalledWith($('#register-name')[0]); - expect(view.validate).toHaveBeenCalledWith($('#register-username')[0]); - expect(view.validate).toHaveBeenCalledWith($('#register-password')[0]); + expect(view.validate).toHaveBeenCalledWith($email[0]); + expect(view.validate).toHaveBeenCalledWith($name[0]); + expect(view.validate).toHaveBeenCalledWith($username[0]); + expect(view.validate).toHaveBeenCalledWith($password[0]); // Verify that no submission errors are visible expect(view.$formFeedback.find('.' + view.formErrorsJsHook).length).toEqual(0); @@ -327,7 +384,34 @@ expect(view.$submitButton).toHaveAttr('disabled'); }); - it('displays registration form validation errors', function() { + it('live validates registration form fields', function() { + var requiredValidationFields = [$email, $confirmEmail, $username, $password], + i, + $el; + createRegisterView(this); + + for (i = 0; i < requiredValidationFields.length; ++i) { + $el = requiredValidationFields[i]; + + // Perform successful live validations. + liveValidate($el); + + // Confirm success. + expect($el).toHaveClass('success'); + + // Confirm that since we've blurred from each input, required text doesn't show. + expect(view.getRequiredTextLabel($el)).toHaveClass('hidden'); + + // Confirm fa-check shows. + expect(view.getIcon($el)).toHaveClass('fa-check'); + expect(view.getIcon($el)).toBeVisible(); + + // Confirm the error tip is empty. + expect(view.getErrorTip($el).val().length).toBe(0); + } + }); + + it('displays registration form validation errors on form submission', function() { createRegisterView(this); // Submit the form, with failed validation @@ -343,7 +427,34 @@ expect(view.$submitButton).not.toHaveAttr('disabled'); }); - it('displays an error if the server returns an error while registering', function() { + it('displays live registration form validation errors', function() { + var requiredValidationFields = [$email, $confirmEmail, $username, $password], + i, + $el; + createRegisterView(this); + + for (i = 0; i < requiredValidationFields.length; ++i) { + $el = requiredValidationFields[i]; + + // Perform invalid live validations. + liveValidate($el, false); + + // Confirm error. + expect($el).toHaveClass('error'); + + // Confirm that since we've blurred from each input, required text still shows for errors. + expect(view.getRequiredTextLabel($el)).not.toHaveClass('hidden'); + + // Confirm fa-times shows. + expect(view.getIcon($el)).toHaveClass('fa-exclamation'); + expect(view.getIcon($el)).toBeVisible(); + + // Confirm the error tip shows an error message. + expect(view.getErrorTip($el).val()).not.toBeEmpty(); + } + }); + + it('displays an error on form submission if the server returns an error', function() { createRegisterView(this); // Submit the form, with successful validation diff --git a/lms/static/js/student_account/tos_modal.js b/lms/static/js/student_account/tos_modal.js index d4982833c5..b1c9ad0571 100644 --- a/lms/static/js/student_account/tos_modal.js +++ b/lms/static/js/student_account/tos_modal.js @@ -79,6 +79,7 @@ var buildIframe = function(link, modalSelector, contentSelector, tosLinkSelector) { // Create an iframe with contents from the link and set its height to match the content area return $('