From 39ac333b5d3e435e069e3e7f9424f912d71a3ca1 Mon Sep 17 00:00:00 2001 From: Uman Shahzad Date: Tue, 20 Jun 2017 07:17:15 +0500 Subject: [PATCH 1/5] Add backend AJAX API endpoint for client-side form validation. In particular, implement a validation API for registration, where a client makes AJAX calls to the endpoints requesting validation decisions on each input. Responses are strings dependent on the type of validation error; if no error, then empty string to indicate OK. --- common/djangoapps/student/forms.py | 22 +- .../student/tests/test_create_account.py | 2 +- .../student_account/test/test_views.py | 19 -- .../djangoapps/user_api/accounts/__init__.py | 47 +++ .../core/djangoapps/user_api/accounts/api.py | 321 +++++++++++++----- .../user_api/accounts/tests/test_api.py | 63 ++-- .../user_api/accounts/tests/testutils.py | 74 ++++ openedx/core/djangoapps/user_api/errors.py | 20 ++ .../djangoapps/user_api/tests/test_views.py | 1 - openedx/core/djangoapps/user_api/urls.py | 24 +- .../user_api/validation/__init__.py | 0 .../user_api/validation/tests/__init__.py | 0 .../user_api/validation/tests/test_views.py | 171 ++++++++++ .../djangoapps/user_api/validation/views.py | 146 ++++++++ openedx/core/djangoapps/user_api/views.py | 25 +- openedx/core/lib/api/test_utils.py | 2 +- 16 files changed, 752 insertions(+), 185 deletions(-) create mode 100644 openedx/core/djangoapps/user_api/accounts/tests/testutils.py create mode 100644 openedx/core/djangoapps/user_api/validation/__init__.py create mode 100644 openedx/core/djangoapps/user_api/validation/tests/__init__.py create mode 100644 openedx/core/djangoapps/user_api/validation/tests/test_views.py create mode 100644 openedx/core/djangoapps/user_api/validation/views.py diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 0bc46f80cb..89f802c28d 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -23,18 +23,6 @@ from student.models import CourseEnrollmentAllowed from util.password_policy_validators import validate_password_strength -USERNAME_TOO_SHORT_MSG = _("Username must be minimum of two characters long") -USERNAME_TOO_LONG_MSG = _("Username cannot be more than %(limit_value)s characters long") - -# Translators: This message is shown when the Unicode usernames are NOT allowed -USERNAME_INVALID_CHARS_ASCII = _("Usernames can only contain Roman letters, western numerals (0-9), " - "underscores (_), and hyphens (-).") - -# Translators: This message is shown only when the Unicode usernames are allowed -USERNAME_INVALID_CHARS_UNICODE = _("Usernames can only contain letters, numerals, underscore (_), numbers " - "and @/./+/-/_ characters.") - - class PasswordResetFormNoActive(PasswordResetForm): error_messages = { 'unknown': _("That e-mail address doesn't have an associated " @@ -127,12 +115,12 @@ def validate_username(username): username_re = slug_re flags = None - message = USERNAME_INVALID_CHARS_ASCII + message = accounts_settings.USERNAME_INVALID_CHARS_ASCII if settings.FEATURES.get("ENABLE_UNICODE_USERNAME"): username_re = r"^{regex}$".format(regex=settings.USERNAME_REGEX_PARTIAL) flags = re.UNICODE - message = USERNAME_INVALID_CHARS_UNICODE + message = accounts_settings.USERNAME_INVALID_CHARS_UNICODE validator = RegexValidator( regex=username_re, @@ -156,9 +144,9 @@ class UsernameField(forms.CharField): min_length=accounts_settings.USERNAME_MIN_LENGTH, max_length=accounts_settings.USERNAME_MAX_LENGTH, error_messages={ - "required": USERNAME_TOO_SHORT_MSG, - "min_length": USERNAME_TOO_SHORT_MSG, - "max_length": USERNAME_TOO_LONG_MSG, + "required": accounts_settings.USERNAME_BAD_LENGTH_MSG, + "min_length": accounts_settings.USERNAME_BAD_LENGTH_MSG, + "max_length": accounts_settings.USERNAME_BAD_LENGTH_MSG, } ) diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index 7eb9b55772..2195970574 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/common/djangoapps/student/tests/test_create_account.py @@ -22,8 +22,8 @@ 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.preferences.api import get_user_preference -from student.forms import USERNAME_INVALID_CHARS_ASCII, USERNAME_INVALID_CHARS_UNICODE from student.models import UserAttribute from student.views import REGISTRATION_AFFILIATE_ID, REGISTRATION_UTM_CREATED_AT, REGISTRATION_UTM_PARAMETERS diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index e82d4b4ab0..20ae639da0 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -36,7 +36,6 @@ from openedx.core.djangoapps.oauth_dispatch.tests import factories as dot_factor from openedx.core.djangoapps.programs.tests.mixins import ProgramsApiConfigMixin from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme_context -from openedx.core.djangoapps.user_api.accounts import EMAIL_MAX_LENGTH from openedx.core.djangoapps.user_api.accounts.api import activate_account, create_account from openedx.core.djangolib.js_utils import dump_js_escaped_json from openedx.core.djangolib.testing.utils import CacheIsolationTestCase @@ -62,24 +61,6 @@ class StudentAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): NEW_EMAIL = u"walt@savewalterwhite.com" INVALID_ATTEMPTS = 100 - - INVALID_EMAILS = [ - None, - u"", - u"a", - "no_domain", - "no+domain", - "@", - "@domain.com", - "test@no_extension", - - # Long email -- subtract the length of the @domain - # except for one character (so we exceed the max length limit) - u"{user}@example.com".format( - user=(u'e' * (EMAIL_MAX_LENGTH - 11)) - ) - ] - INVALID_KEY = u"123abc" URLCONF_MODULES = ['student_accounts.urls'] diff --git a/openedx/core/djangoapps/user_api/accounts/__init__.py b/openedx/core/djangoapps/user_api/accounts/__init__.py index 5441d168ff..ec47c6a2b6 100644 --- a/openedx/core/djangoapps/user_api/accounts/__init__.py +++ b/openedx/core/djangoapps/user_api/accounts/__init__.py @@ -2,6 +2,9 @@ Account constants """ +from django.utils.translation import ugettext as _ + + # The minimum and maximum length for the name ("full name") account field NAME_MIN_LENGTH = 2 NAME_MAX_LENGTH = 255 @@ -25,3 +28,47 @@ ALL_USERS_VISIBILITY = 'all_users' # Indicates the user's preference that all their account information be private. PRIVATE_VISIBILITY = 'private' + +# Translators: This message is shown when the Unicode usernames are NOT allowed. +# It is shown to users who attempt to create a new account using invalid characters +# in the username. +USERNAME_INVALID_CHARS_ASCII = _( + u"Usernames can only contain letters (A-Z, a-z), numerals (0-9), underscores (_), and hyphens (-)." +) + +# Translators: This message is shown only when the Unicode usernames are allowed. +# It is shown to users who attempt to create a new account using invalid characters +# in the username. +USERNAME_INVALID_CHARS_UNICODE = _( + u"Usernames can only contain letters, numerals, and @/./+/-/_ characters." +) + +# Translators: This message is shown to users who attempt to create a new account using +# an invalid email format. +EMAIL_INVALID_MSG = _(u"Email '{email}' format is not valid") + +# Translators: This message is shown to users who attempt to create a new +# account using an username/email associated with an existing account. +EMAIL_CONFLICT_MSG = _( + u"It looks like {email_address} belongs to an existing account. " + u"Try again with a different email address." +) +USERNAME_CONFLICT_MSG = _( + u"It looks like {username} belongs to an existing account. " + u"Try again with a different username." +) + +# Translators: This message is shown to users who enter a username/email/password +# with an inappropriate length (too short or too long). +USERNAME_BAD_LENGTH_MSG = _(u"Username '{username}' must be between {min} and {max} characters long") +EMAIL_BAD_LENGTH_MSG = _(u"Email '{email}' must be between {min} and {max} characters long") +PASSWORD_BAD_LENGTH_MSG = _(u"Password must be between {min} and {max} characters long") + +# These strings are normally not user-facing. +USERNAME_BAD_TYPE_MSG = u"Username must be a string" +EMAIL_BAD_TYPE_MSG = u"Email must be a string" +PASSWORD_BAD_TYPE_MSG = u"Password must be a string" + +# Translators: This message is shown to users who enter a password matching +# the username they enter(ed). +PASSWORD_CANT_EQUAL_USERNAME_MSG = _(u"Password cannot be the same as the username") diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 8fc3fcad65..1d503dabb7 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """ Programmatic integration point for User API Accounts sub-application """ @@ -20,16 +21,23 @@ from util.model_utils import emit_setting_changed_event from openedx.core.lib.api.view_utils import add_serializer_errors from ..errors import ( - AccountUpdateError, AccountValidationError, AccountUsernameInvalid, AccountPasswordInvalid, - AccountEmailInvalid, AccountUserAlreadyExists, + AccountUpdateError, AccountValidationError, + AccountDataBadLength, AccountDataBadType, + AccountUsernameInvalid, AccountPasswordInvalid, AccountEmailInvalid, + AccountUserAlreadyExists, AccountUsernameAlreadyExists, AccountEmailAlreadyExists, UserAPIInternalError, UserAPIRequestError, UserNotFound, UserNotAuthorized ) from ..forms import PasswordResetFormNoActive from ..helpers import intercept_errors from . import ( - EMAIL_MIN_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MIN_LENGTH, PASSWORD_MAX_LENGTH, - USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH + EMAIL_BAD_LENGTH_MSG, PASSWORD_BAD_LENGTH_MSG, USERNAME_BAD_LENGTH_MSG, + EMAIL_BAD_TYPE_MSG, PASSWORD_BAD_TYPE_MSG, USERNAME_BAD_TYPE_MSG, + EMAIL_CONFLICT_MSG, USERNAME_CONFLICT_MSG, + EMAIL_INVALID_MSG, USERNAME_INVALID_MSG, + EMAIL_MIN_LENGTH, PASSWORD_MIN_LENGTH, USERNAME_MIN_LENGTH, + EMAIL_MAX_LENGTH, PASSWORD_MAX_LENGTH, USERNAME_MAX_LENGTH, + PASSWORD_CANT_EQUAL_USERNAME_MSG ) from .serializers import ( AccountLegacyProfileSerializer, AccountUserSerializer, @@ -70,6 +78,7 @@ def get_account_settings(request, usernames=None, configuration=None, view=None) UserNotFound: no user with username `username` exists (or `request.user.username` if `username` is not specified) UserAPIInternalError: the operation failed due to an unexpected error. + """ requesting_user = request.user usernames = usernames or [requesting_user.username] @@ -122,6 +131,7 @@ def update_account_settings(requesting_user, update, username=None): in particular, the user account (not including e-mail address) may have successfully been updated, but then the e-mail change request, which is processed last, may throw an error. UserAPIInternalError: the operation failed due to an unexpected error. + """ if username is None: username = requesting_user.username @@ -243,20 +253,6 @@ def update_account_settings(requesting_user, update, username=None): ) -def _get_user_and_profile(username): - """ - Helper method to return the legacy user and profile objects based on username. - """ - try: - existing_user = User.objects.get(username=username) - except ObjectDoesNotExist: - raise UserNotFound() - - existing_user_profile, _ = UserProfile.objects.get_or_create(user=existing_user) - - return existing_user, existing_user_profile - - @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @transaction.atomic def create_account(username, password, email): @@ -296,6 +292,7 @@ def create_account(username, password, email): AccountEmailInvalid AccountPasswordInvalid UserAPIInternalError: the operation failed due to an unexpected error. + """ # Check if ALLOW_PUBLIC_ACCOUNT_CREATION flag turned off to restrict user account creation if not configuration_helpers.get_value( @@ -350,10 +347,13 @@ def check_account_exists(username=None, email=None): """ conflicts = [] - if email is not None and User.objects.filter(email=email).exists(): + try: + _validate_email_doesnt_exist(email) + except AccountEmailAlreadyExists: conflicts.append("email") - - if username is not None and User.objects.filter(username=username).exists(): + try: + _validate_username_doesnt_exist(username) + except AccountUsernameAlreadyExists: conflicts.append("username") return conflicts @@ -372,6 +372,7 @@ def activate_account(activation_key): Raises: UserNotAuthorized UserAPIInternalError: the operation failed due to an unexpected error. + """ try: registration = Registration.objects.get(activation_key=activation_key) @@ -400,6 +401,7 @@ def request_password_change(email, orig_host, is_secure): UserNotFound AccountRequestError UserAPIInternalError: the operation failed due to an unexpected error. + """ # Binding data to a form requires that the data be passed as a dictionary # to the Form class constructor. @@ -419,6 +421,101 @@ def request_password_change(email, orig_host, is_secure): raise UserNotFound +def get_username_validation_error(username, default=''): + """Get the built-in validation error message for when + the username is invalid in some way. + + :param username: The proposed username (unicode). + :param default: The message to default to in case of no error. + :return: Validation error message. + + """ + try: + _validate_username(username) + except AccountUsernameInvalid as invalid_username_err: + return invalid_username_err.message + return default + + +def get_email_validation_error(email, default=''): + """Get the built-in validation error message for when + the email is invalid in some way. + + :param email: The proposed email (unicode). + :param default: The message to default to in case of no error. + :return: Validation error message. + + """ + try: + _validate_email(email) + except AccountEmailInvalid as invalid_email_err: + return invalid_email_err.message + return default + + +def get_password_validation_error(password, username=None, default=''): + """Get the built-in validation error message for when + the password is invalid in some way. + + :param password: The proposed password (unicode). + :param username: The username associated with the user's account (unicode). + :param default: The message to default to in case of no error. + :return: Validation error message. + + """ + try: + _validate_password(password, username) + except AccountPasswordInvalid as invalid_password_err: + return invalid_password_err.message + return default + + +def get_username_existence_validation_error(username, default=''): + """Get the built-in validation error message for when + the username has an existence conflict. + + :param username: The proposed username (unicode). + :param default: The message to default to in case of no error. + :return: Validation error message. + + """ + try: + _validate_username_doesnt_exist(username) + except AccountUsernameAlreadyExists as username_exists_err: + return username_exists_err.message + return default + + +def get_email_existence_validation_error(email, default=''): + """Get the built-in validation error message for when + the email has an existence conflict. + + :param email: The proposed email (unicode). + :param default: The message to default to in case of no error. + :return: Validation error message. + + """ + try: + _validate_email_doesnt_exist(email) + except AccountEmailAlreadyExists as email_exists_err: + return email_exists_err.message + return default + + +def _get_user_and_profile(username): + """ + Helper method to return the legacy user and profile objects based on username. + """ + try: + existing_user = User.objects.get(username=username) + except ObjectDoesNotExist: + raise UserNotFound() + + existing_user_profile, _ = UserProfile.objects.get_or_create(user=existing_user) + + return existing_user, existing_user_profile + + def _validate_username(username): """Validate the username. @@ -432,33 +529,54 @@ def _validate_username(username): AccountUsernameInvalid """ - if not isinstance(username, basestring): - raise AccountUsernameInvalid(u"Username must be a string") - - if len(username) < USERNAME_MIN_LENGTH: - raise AccountUsernameInvalid( - u"Username '{username}' must be at least {min} characters long".format( - username=username, - min=USERNAME_MIN_LENGTH - ) - ) - if len(username) > USERNAME_MAX_LENGTH: - raise AccountUsernameInvalid( - u"Username '{username}' must be at most {max} characters long".format( + try: + _validate_unicode(username) + _validate_type(username, basestring, USERNAME_BAD_TYPE_MSG) + _validate_length( + username, USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH, USERNAME_BAD_LENGTH_MSG.format( username=username, + min=USERNAME_MIN_LENGTH, max=USERNAME_MAX_LENGTH ) ) - try: with override_language('en'): # `validate_username` provides a proper localized message, however the API needs only the English # message by convention. student_forms.validate_username(username) - except ValidationError as error: - raise AccountUsernameInvalid(error.message) + except (UnicodeError, AccountDataBadType, AccountDataBadLength, ValidationError) as invalid_username_err: + raise AccountUsernameInvalid(invalid_username_err.message) -def _validate_password(password, username): +def _validate_email(email): + """Validate the format of the email address. + + Arguments: + email (unicode): The proposed email. + + Returns: + None + + Raises: + AccountEmailInvalid + + """ + try: + _validate_unicode(email) + _validate_type(email, basestring, EMAIL_BAD_TYPE_MSG) + _validate_length( + email, EMAIL_MIN_LENGTH, EMAIL_MAX_LENGTH, EMAIL_BAD_LENGTH_MSG.format( + email=email, + min=EMAIL_MIN_LENGTH, + max=EMAIL_MAX_LENGTH + ) + ) + validate_email.message = EMAIL_INVALID_MSG.format(email=email) + validate_email(email) + except (UnicodeError, AccountDataBadType, AccountDataBadLength, ValidationError) as invalid_email_err: + raise AccountEmailInvalid(invalid_email_err.message) + + +def _validate_password(password, username=None): """Validate the format of the user's password. Passwords cannot be the same as the username of the account, @@ -475,62 +593,99 @@ def _validate_password(password, username): AccountPasswordInvalid """ - if not isinstance(password, basestring): - raise AccountPasswordInvalid(u"Password must be a string") - - if len(password) < PASSWORD_MIN_LENGTH: - raise AccountPasswordInvalid( - u"Password must be at least {min} characters long".format( - min=PASSWORD_MIN_LENGTH - ) - ) - - if len(password) > PASSWORD_MAX_LENGTH: - raise AccountPasswordInvalid( - u"Password must be at most {max} characters long".format( + try: + _validate_type(password, basestring, PASSWORD_BAD_TYPE_MSG) + _validate_length( + password, PASSWORD_MIN_LENGTH, PASSWORD_MAX_LENGTH, PASSWORD_BAD_LENGTH_MSG.format( + min=PASSWORD_MIN_LENGTH, max=PASSWORD_MAX_LENGTH ) ) + _validate_password_works_with_username(password, username) + except (AccountDataBadType, AccountDataBadLength) as invalid_password_err: + raise AccountPasswordInvalid(invalid_password_err.message) + +def _validate_username_doesnt_exist(username): + """Validate that the username is not associated with an existing user. + + :param username: The proposed username (unicode). + :return: None + :raises: AccountUsernameAlreadyExists + """ + if username is not None and User.objects.filter(username=username).exists(): + raise AccountUsernameAlreadyExists(_(USERNAME_CONFLICT_MSG).format(username=username)) + + +def _validate_email_doesnt_exist(email): + """Validate that the email is not associated with an existing user. + + :param email: The proposed email (unicode). + :return: None + :raises: AccountEmailAlreadyExists + """ + if email is not None and User.objects.filter(email=email).exists(): + raise AccountEmailAlreadyExists(_(EMAIL_CONFLICT_MSG).format(email_address=email)) + + +def _validate_password_works_with_username(password, username=None): + """Run validation checks on whether the password and username + go well together. + + An example check is to see whether they are the same. + + :param password: The proposed password (unicode). + :param username: The username associated with the user's account (unicode). + :return: None + :raises: AccountPasswordInvalid + """ if password == username: - raise AccountPasswordInvalid(u"Password cannot be the same as the username") + raise AccountPasswordInvalid(PASSWORD_CANT_EQUAL_USERNAME_MSG) -def _validate_email(email): - """Validate the format of the email address. +def _validate_type(data, type, err): + """Checks whether the input data is of type. If not, + throws a generic error message. - Arguments: - email (unicode): The proposed email. - - Returns: - None - - Raises: - AccountEmailInvalid + :param data: The data to check. + :param type: The type to check against. + :param err: The error message to throw back if data is not of type. + :return: None + :raises: AccountDataBadType """ - if not isinstance(email, basestring): - raise AccountEmailInvalid(u"Email must be a string") + if not isinstance(data, type): + raise AccountDataBadType(err) - if len(email) < EMAIL_MIN_LENGTH: - raise AccountEmailInvalid( - u"Email '{email}' must be at least {min} characters long".format( - email=email, - min=EMAIL_MIN_LENGTH - ) - ) - if len(email) > EMAIL_MAX_LENGTH: - raise AccountEmailInvalid( - u"Email '{email}' must be at most {max} characters long".format( - email=email, - max=EMAIL_MAX_LENGTH - ) - ) +def _validate_length(data, min, max, err): + """Validate that the data's length is less than or equal to max, + and greater than or equal to min. + :param data: The data to do the test on. + :param min: The minimum allowed length. + :param max: The maximum allowed length. + :return: None + :raises: AccountDataBadLength + + """ + if len(data) < min or len(data) > max: + raise AccountDataBadLength(err) + + +def _validate_unicode(data, err=u"Input not valid unicode"): + """Checks whether the input data is valid unicode or not. + + :param data: The data to check for unicode validity. + :param err: The error message to throw back if unicode is invalid. + :return: None + :raises: UnicodeError + + """ try: - validate_email(email) - except ValidationError: - raise AccountEmailInvalid( - u"Email '{email}' format is not valid".format(email=email) - ) + if not isinstance(data, str) and not isinstance(data, unicode): + raise UnicodeError + # In some cases we pass the above, but it's still inappropriate utf-8. + str(data) + except UnicodeError: + raise UnicodeError(err) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 129cf9883e..2928807f59 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -3,6 +3,7 @@ Unit tests for behavior that is specific to the api methods (vs. the view methods). Most of the functionality is covered in test_views.py. """ + import re import ddt from dateutil.parser import parse as parse_datetime @@ -17,17 +18,29 @@ from django.conf import settings from django.contrib.auth.models import User from django.core import mail from django.test.client import RequestFactory +from openedx.core.djangoapps.user_api.accounts import ( + USERNAME_MAX_LENGTH, + PRIVATE_VISIBILITY +) +from openedx.core.djangoapps.user_api.accounts.api import ( + get_account_settings, + update_account_settings, + create_account, + activate_account, + request_password_change +) +from openedx.core.djangoapps.user_api.errors import ( + UserNotFound, UserNotAuthorized, + AccountUpdateError, AccountValidationError, AccountUserAlreadyExists, + AccountUsernameInvalid, AccountEmailInvalid, AccountPasswordInvalid, + AccountRequestError +) +from openedx.core.djangoapps.user_api.accounts.tests.testutils import ( + INVALID_EMAILS, INVALID_PASSWORDS, INVALID_USERNAMES +) from openedx.core.djangolib.testing.utils import skip_unless_lms from student.models import PendingEmailChange from student.tests.tests import UserSettingsEventTestMixin -from ...errors import ( - UserNotFound, UserNotAuthorized, AccountUpdateError, AccountValidationError, - AccountUserAlreadyExists, AccountUsernameInvalid, AccountEmailInvalid, AccountPasswordInvalid, AccountRequestError -) -from ..api import ( - get_account_settings, update_account_settings, create_account, activate_account, request_password_change -) -from .. import USERNAME_MAX_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MAX_LENGTH, PRIVATE_VISIBILITY def mock_render_to_string(template_name, context): @@ -310,40 +323,6 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): ORIG_HOST = 'example.com' IS_SECURE = False - INVALID_USERNAMES = [ - None, - u'', - u'a', - u'a' * (USERNAME_MAX_LENGTH + 1), - u'invalid_symbol_@', - u'invalid-unicode_fŕáńḱ', - ] - - INVALID_EMAILS = [ - None, - u'', - u'a', - 'no_domain', - 'no+domain', - '@', - '@domain.com', - 'test@no_extension', - u'fŕáńḱ@example.com', - - # Long email -- subtract the length of the @domain - # except for one character (so we exceed the max length limit) - u'{user}@example.com'.format( - user=(u'e' * (EMAIL_MAX_LENGTH - 11)) - ) - ] - - INVALID_PASSWORDS = [ - None, - u'', - u'a', - u'a' * (PASSWORD_MAX_LENGTH + 1) - ] - @skip_unless_lms def test_activate_account(self): # Create the account, which is initially inactive diff --git a/openedx/core/djangoapps/user_api/accounts/tests/testutils.py b/openedx/core/djangoapps/user_api/accounts/tests/testutils.py new file mode 100644 index 0000000000..b5ab4cb187 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/testutils.py @@ -0,0 +1,74 @@ +# -*- coding: utf-8 -*- +""" +Utility functions, constants, etc. for testing. +""" + +from openedx.core.djangoapps.user_api.accounts import ( + USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH, + EMAIL_MAX_LENGTH, + PASSWORD_MIN_LENGTH, PASSWORD_MAX_LENGTH +) + + +INVALID_USERNAMES_ASCII = [ + '$invalid-ascii$', + 'invalid-fŕáńḱ', + '@invalid-ascii@' +] + +INVALID_USERNAMES_UNICODE = [ + u'invalid-unicode_fŕáńḱ', +] + +INVALID_USERNAMES = [ + None, + u'', + u'a', + u'a' * (USERNAME_MAX_LENGTH + 1), +] + INVALID_USERNAMES_ASCII + INVALID_USERNAMES_UNICODE + +INVALID_EMAILS = [ + None, + u'', + u'a', + 'no_domain', + 'no+domain', + '@', + '@domain.com', + 'test@no_extension', + u'fŕáńḱ@example.com', + + # Long email -- subtract the length of the @domain + # except for one character (so we exceed the max length limit) + u'{user}@example.com'.format( + user=(u'e' * (EMAIL_MAX_LENGTH - 11)) + ) +] + +INVALID_PASSWORDS = [ + None, + u'', + u'a', + u'a' * (PASSWORD_MAX_LENGTH + 1) +] + +VALID_USERNAMES = [ + u'username', + u'a' * USERNAME_MIN_LENGTH, + u'a' * USERNAME_MAX_LENGTH, + u'-' * USERNAME_MIN_LENGTH, + u'-' * USERNAME_MAX_LENGTH, + u'_username_', + u'-username-', + u'-_username_-' +] + +VALID_EMAILS = [ + 'has@domain.com' +] + +VALID_PASSWORDS = [ + u'password', # :) + u'a' * PASSWORD_MIN_LENGTH, + u'a' * PASSWORD_MAX_LENGTH +] diff --git a/openedx/core/djangoapps/user_api/errors.py b/openedx/core/djangoapps/user_api/errors.py index 1930d31365..9bd6be4a00 100644 --- a/openedx/core/djangoapps/user_api/errors.py +++ b/openedx/core/djangoapps/user_api/errors.py @@ -33,6 +33,16 @@ class AccountUserAlreadyExists(AccountRequestError): pass +class AccountUsernameAlreadyExists(AccountRequestError): + """User with the same username already exists. """ + pass + + +class AccountEmailAlreadyExists(AccountRequestError): + """User with the same email already exists. """ + pass + + class AccountUsernameInvalid(AccountRequestError): """The requested username is not in a valid format. """ pass @@ -48,6 +58,16 @@ class AccountPasswordInvalid(AccountRequestError): pass +class AccountDataBadLength(AccountRequestError): + """The requested account data is either too short or too long. """ + pass + + +class AccountDataBadType(AccountRequestError): + """The requested account data is of the wrong type. """ + pass + + class AccountUpdateError(AccountRequestError): """ An update to the account failed. More detailed information is present in developer_message, diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 389407bb6b..a5d7ed3b36 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -31,7 +31,6 @@ from third_party_auth.tests.utils import ( from .test_helpers import TestCaseForm from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from ..helpers import FormDescription from ..accounts import ( NAME_MAX_LENGTH, EMAIL_MIN_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MIN_LENGTH, PASSWORD_MAX_LENGTH, USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH diff --git a/openedx/core/djangoapps/user_api/urls.py b/openedx/core/djangoapps/user_api/urls.py index 38891f488e..ad11c09998 100644 --- a/openedx/core/djangoapps/user_api/urls.py +++ b/openedx/core/djangoapps/user_api/urls.py @@ -9,6 +9,7 @@ from ..profile_images.views import ProfileImageView from .accounts.views import AccountDeactivationView, AccountViewSet from .preferences.views import PreferencesDetailView, PreferencesView from .verification_api.views import PhotoVerificationStatusView +from .validation.views import RegistrationValidationView ME = AccountViewSet.as_view({ 'get': 'get', @@ -25,9 +26,21 @@ ACCOUNT_DETAIL = AccountViewSet.as_view({ urlpatterns = patterns( '', - url(r'^v1/me$', ME, name='own_username_api'), - url(r'^v1/accounts/{}$'.format(settings.USERNAME_PATTERN), ACCOUNT_DETAIL, name='accounts_api'), - url(r'^v1/accounts$', ACCOUNT_LIST, name='accounts_detail_api'), + url( + r'^v1/me$', + ME, + name='own_username_api' + ), + url( + r'^v1/accounts$', + ACCOUNT_LIST, + name='accounts_detail_api' + ), + url( + r'^v1/accounts/{}$'.format(settings.USERNAME_PATTERN), + ACCOUNT_DETAIL, + name='accounts_api' + ), url( r'^v1/accounts/{}/image$'.format(settings.USERNAME_PATTERN), ProfileImageView.as_view(), @@ -43,6 +56,11 @@ urlpatterns = patterns( PhotoVerificationStatusView.as_view(), name='verification_status' ), + url( + r'^v1/validation/registration$', + RegistrationValidationView.as_view(), + name='registration_validation' + ), url( r'^v1/preferences/{}$'.format(settings.USERNAME_PATTERN), PreferencesView.as_view(), diff --git a/openedx/core/djangoapps/user_api/validation/__init__.py b/openedx/core/djangoapps/user_api/validation/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/validation/tests/__init__.py b/openedx/core/djangoapps/user_api/validation/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/validation/tests/test_views.py b/openedx/core/djangoapps/user_api/validation/tests/test_views.py new file mode 100644 index 0000000000..566c1b1356 --- /dev/null +++ b/openedx/core/djangoapps/user_api/validation/tests/test_views.py @@ -0,0 +1,171 @@ +# -*- coding: utf-8 -*- +""" +Tests for an API endpoint for client-side user data validation. +""" + +import unittest + +import ddt +from django.conf import settings +from django.contrib.auth.models import User +from django.core.urlresolvers import reverse + +from openedx.core.djangoapps.user_api.accounts import ( + EMAIL_BAD_LENGTH_MSG, EMAIL_INVALID_MSG, + EMAIL_CONFLICT_MSG, EMAIL_MAX_LENGTH, EMAIL_MIN_LENGTH, + PASSWORD_BAD_LENGTH_MSG, PASSWORD_CANT_EQUAL_USERNAME_MSG, + PASSWORD_MAX_LENGTH, PASSWORD_MIN_LENGTH, + USERNAME_BAD_LENGTH_MSG, USERNAME_INVALID_CHARS_ASCII, USERNAME_INVALID_CHARS_UNICODE, + USERNAME_CONFLICT_MSG, USERNAME_MAX_LENGTH, USERNAME_MIN_LENGTH +) +from openedx.core.djangoapps.user_api.accounts.tests.testutils import ( + VALID_EMAILS, VALID_PASSWORDS, VALID_USERNAMES, + INVALID_EMAILS, INVALID_PASSWORDS, INVALID_USERNAMES, + INVALID_USERNAMES_ASCII, INVALID_USERNAMES_UNICODE +) +from openedx.core.lib.api import test_utils + + +@ddt.ddt +class RegistrationValidationViewTests(test_utils.ApiTestCase): + """ + Tests for validity of user data in registration forms. + """ + + endpoint_name = 'registration_validation' + path = reverse(endpoint_name) + + def get_validation_decision(self, data): + response = self.client.post(self.path, data) + return response.data.get('validation_decisions', {}) + + def assertValidationDecision(self, data, decision): + self.assertEqual( + self.get_validation_decision(data), + decision + ) + + def test_no_decision_for_empty_request(self): + self.assertValidationDecision({}, {}) + + def test_no_decision_for_invalid_request(self): + self.assertValidationDecision({'invalid_field': 'random_user_data'}, {}) + + @ddt.data( + ['email', (email for email in VALID_EMAILS)], + ['password', (password for password in VALID_PASSWORDS)], + ['username', (username for username in VALID_USERNAMES)] + ) + @ddt.unpack + def test_positive_validation_decision(self, form_field_name, user_data): + """ + Test if {0} as any item in {1} gives a positive validation decision. + """ + self.assertValidationDecision( + {form_field_name: user_data}, + {form_field_name: ''} + ) + + @ddt.data( + # Skip None type for invalidity checks. + ['email', (email for email in INVALID_EMAILS[1:])], + ['password', (password for password in INVALID_PASSWORDS[1:])], + ['username', (username for username in INVALID_USERNAMES[1:])] + ) + @ddt.unpack + def test_negative_validation_decision(self, form_field_name, user_data): + """ + Test if {0} as any item in {1} gives a negative validation decision. + """ + self.assertNotEqual( + self.get_validation_decision({form_field_name: user_data}), + {form_field_name: ''} + ) + + @ddt.data( + ['username', 'username@email.com'], # No conflict + ['user', 'username@email.com'], # Username conflict + ['username', 'user@email.com'], # Email conflict + ['user', 'user@email.com'] # Both conflict + ) + @ddt.unpack + def test_existence_conflict(self, username, email): + """ + Test if username '{0}' and email '{1}' have conflicts with + username 'user' and email 'user@email.com'. + """ + user = User.objects.create_user(username='user', email='user@email.com') + self.assertValidationDecision( + { + 'username': username, + 'email': email + }, + { + "username": USERNAME_CONFLICT_MSG.format(username=user.username) if username == user.username else '', + "email": EMAIL_CONFLICT_MSG.format(email_address=user.email) if email == user.email else '' + } + ) + + @ddt.data('', ('e' * EMAIL_MAX_LENGTH) + '@email.com') + def test_email_less_than_min_length_validation_decision(self, email): + self.assertValidationDecision( + {'email': email}, + {'email': EMAIL_BAD_LENGTH_MSG.format(email=email, min=EMAIL_MIN_LENGTH, max=EMAIL_MAX_LENGTH)} + ) + + def test_email_generically_invalid_validation_decision(self): + email = 'email' + self.assertValidationDecision( + {'email': email}, + {'email': EMAIL_INVALID_MSG.format(email=email)} + ) + + @ddt.data( + 'u' * (USERNAME_MIN_LENGTH - 1), + 'u' * (USERNAME_MAX_LENGTH + 1) + ) + def test_username_less_than_min_length_validation_decision(self, username): + self.assertValidationDecision( + {'username': username}, + { + 'username': USERNAME_BAD_LENGTH_MSG.format( + username=username, + min=USERNAME_MIN_LENGTH, + max=USERNAME_MAX_LENGTH + ) + } + ) + + @unittest.skipUnless(settings.FEATURES.get("ENABLE_UNICODE_USERNAME"), "Unicode usernames disabled.") + @ddt.data(*INVALID_USERNAMES_UNICODE) + @ddt.unpack + def test_username_invalid_unicode_validation_decision(self, username): + self.assertValidationDecision( + {'username': username}, + {'username': USERNAME_INVALID_CHARS_UNICODE} + ) + + @unittest.skipIf(settings.FEATURES.get("ENABLE_UNICODE_USERNAME"), "Unicode usernames enabled.") + @ddt.data(*INVALID_USERNAMES_ASCII) + @ddt.unpack + def test_username_invalid_ascii_validation_decision(self, username): + self.assertValidationDecision( + {'username': username}, + {"username": USERNAME_INVALID_CHARS_ASCII} + ) + + @ddt.data( + 'p' * (PASSWORD_MIN_LENGTH - 1), + 'p' * (PASSWORD_MAX_LENGTH + 1) + ) + def test_password_less_than_min_length_validation_decision(self, password): + self.assertValidationDecision( + {'password': password}, + {"password": PASSWORD_BAD_LENGTH_MSG.format(min=PASSWORD_MIN_LENGTH, max=PASSWORD_MAX_LENGTH)} + ) + + def test_password_equals_username_validation_decision(self): + self.assertValidationDecision( + {"username": "somephrase", "password": "somephrase"}, + {"username": "", "password": PASSWORD_CANT_EQUAL_USERNAME_MSG} + ) diff --git a/openedx/core/djangoapps/user_api/validation/views.py b/openedx/core/djangoapps/user_api/validation/views.py new file mode 100644 index 0000000000..8d90fb1617 --- /dev/null +++ b/openedx/core/djangoapps/user_api/validation/views.py @@ -0,0 +1,146 @@ +# -*- coding: utf-8 -*- +""" +An API for client-side validation of (potential) user data. +""" + +from rest_framework.response import Response +from rest_framework.views import APIView + +from openedx.core.djangoapps.user_api.accounts.api import ( + get_email_validation_error, + get_email_existence_validation_error, + get_password_validation_error, + get_username_validation_error, + get_username_existence_validation_error +) + + +class RegistrationValidationView(APIView): + """ + **Use Cases** + + Get validation information about user data during registration. + Client-side may request validation for any number of form fields, + and the API will return a conclusion from its analysis for each + input (i.e. valid or not valid, or a custom, detailed message). + + **Example Requests and Responses** + + - Checks the validity of the username and email inputs separately. + POST /api/user/v1/validation/registration/ + >>> { + >>> "username": "hi_im_new", + >>> "email": "newguy101@edx.org" + >>> } + RESPONSE + >>> { + >>> "validation_decisions": { + >>> "username": "", + >>> "email": "" + >>> } + >>> } + Empty strings indicate that there was no problem with the input. + + - Checks the validity of the password field (its validity depends + upon both the username and password fields, so we need both). If + only password is input, we don't check for password/username + compatibility issues. + POST /api/user/v1/validation/registration/ + >>> { + >>> "username": "myname", + >>> "password": "myname" + >>> } + RESPONSE + >>> { + >>> "validation_decisions": { + >>> "username": "", + >>> "password": "Password cannot be the same as the username" + >>> } + >>> } + + - Checks the validity of the username, email, and password fields + separately, and also tells whether an account exists. The password + field's validity depends upon both the username and password, and + the account's existence depends upon both the username and email. + POST /api/user/v1/validation/registration/ + >>> { + >>> "username": "hi_im_new", + >>> "email": "cto@edx.org", + >>> "password": "p" + >>> } + RESPONSE + >>> { + >>> "validation_decisions": { + >>> "username": "", + >>> "email": "It looks like cto@edx.org belongs to an existing account. Try again with a different email address.", + >>> "password": "Password must be at least 2 characters long", + >>> } + >>> } + In this example, username is valid and (we assume) there is + a preexisting account with that email. The password also seems + to contain the username. + + Note that a validation decision is returned *for all* inputs, whether + positive or negative. + + **Available Handlers** + + "username": + A handler to check the validity of usernames. + "email": + A handler to check the validity of emails. + "password": + A handler to check the validity of passwords; a compatibility + decision with the username is made if it exists in the input. + """ + + def username_handler(self, request): + username = request.data.get('username') + invalid_username_error = get_username_validation_error(username) + username_exists_error = get_username_existence_validation_error(username) + # Existing usernames are already valid, so we prefer that error. + return username_exists_error or invalid_username_error + + def email_handler(self, request): + email = request.data.get('email') + invalid_email_error = get_email_validation_error(email) + email_exists_error = get_email_existence_validation_error(email) + # Existing emails are already valid, so we prefer that error. + return email_exists_error or invalid_email_error + + def password_handler(self, request): + username = request.data.get('username') or None + password = request.data.get('password') + return get_password_validation_error(password, username) + + validation_handlers = { + "username": username_handler, + "email": email_handler, + "password": password_handler, + } + + def post(self, request): + """ + POST /api/user/v1/validation/registration/ + + Expects request of the form + >>> { + >>> "username": "mslm", + >>> "email": "mslm@gmail.com", + >>> "password": "password123" + >>> } + where each key is the appropriate form field name and the value is + user input. One may enter individual inputs if needed. Some inputs + can get extra verification checks if entered along with others, + like when the password may not equal the username. + """ + validation_decisions = {} + for form_field_key in self.validation_handlers: + # For every field requiring validation from the client, + # request a decision for it from the appropriate handler. + if form_field_key in request.data: + handler = self.validation_handlers[form_field_key] + validation_decisions.update({ + form_field_key: handler(self, request) + }) + return Response({"validation_decisions": validation_decisions}) diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index 487a31477b..61735b417f 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -32,13 +32,12 @@ from student.views import create_account_with_params, AccountValidationError from util.json_request import JsonResponse from .accounts import ( - EMAIL_MAX_LENGTH, - EMAIL_MIN_LENGTH, + EMAIL_MAX_LENGTH, EMAIL_MIN_LENGTH, NAME_MAX_LENGTH, - PASSWORD_MAX_LENGTH, - PASSWORD_MIN_LENGTH, - USERNAME_MAX_LENGTH, - USERNAME_MIN_LENGTH + PASSWORD_MAX_LENGTH, PASSWORD_MIN_LENGTH, + USERNAME_MAX_LENGTH, USERNAME_MIN_LENGTH, + EMAIL_CONFLICT_MSG, + USERNAME_CONFLICT_MSG ) from .accounts.api import check_account_exists from .helpers import FormDescription, require_post_params, shim_student_view @@ -340,18 +339,8 @@ class RegistrationView(APIView): conflicts = check_account_exists(email=email, username=username) if conflicts: conflict_messages = { - "email": _( - # Translators: This message is shown to users who attempt to create a new - # account using an email address associated with an existing account. - u"It looks like {email_address} belongs to an existing account. " - u"Try again with a different email address." - ).format(email_address=email), - "username": _( - # Translators: This message is shown to users who attempt to create a new - # account using a username associated with an existing account. - u"It looks like {username} belongs to an existing account. " - u"Try again with a different username." - ).format(username=username), + "email": EMAIL_CONFLICT_MSG.format(email_address=email), + "username": USERNAME_CONFLICT_MSG.format(username=username), } errors = { field: [{"user_message": conflict_messages[field]}] diff --git a/openedx/core/lib/api/test_utils.py b/openedx/core/lib/api/test_utils.py index 181429add3..eb8c2a2aa3 100644 --- a/openedx/core/lib/api/test_utils.py +++ b/openedx/core/lib/api/test_utils.py @@ -28,7 +28,7 @@ class ApiTestCase(TestCase): return getattr(self.client, method)(*args, HTTP_X_EDX_API_KEY=TEST_API_KEY, **kwargs) def get_json(self, *args, **kwargs): - """Make a request with the given args and return the parsed JSON repsonse""" + """Make a request with the given args and return the parsed JSON response""" resp = self.request_with_auth("get", *args, **kwargs) self.assertHttpOK(resp) self.assertTrue(resp["Content-Type"].startswith("application/json")) From cb034d4f2faab29d145f1bc941b3d5aeddd47f50 Mon Sep 17 00:00:00 2001 From: Uman Shahzad Date: Tue, 27 Jun 2017 06:58:04 +0500 Subject: [PATCH 2/5] 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 $('