Merge pull request #6953 from edx/gprice/registration-endpoint-refactor

Refactor registration endpoint and return field-specific detail in errors

common/djangoapps/student/forms.py

@@ -2,16 +2,23 @@
 Utility functions for validating forms
 """
 from django import forms
+from django.core.exceptions import ValidationError
 from django.contrib.auth.models import User
 from django.contrib.auth.forms import PasswordResetForm
 from django.contrib.auth.hashers import UNUSABLE_PASSWORD
 from django.contrib.auth.tokens import default_token_generator
 
 from django.utils.http import int_to_base36
+from django.utils.translation import ugettext_lazy as _
 from django.template import loader
 
 from django.conf import settings
 from microsite_configuration import microsite
+from util.password_policy_validators import (
+    validate_password_length,
+    validate_password_complexity,
+    validate_password_dictionary,
+)
 
 
 class PasswordResetFormNoActive(PasswordResetForm):
@@ -70,3 +77,160 @@ class PasswordResetFormNoActive(PasswordResetForm):
             subject = subject.replace('\n', '')
             email = loader.render_to_string(email_template_name, context)
             send_mail(subject, email, from_email, [user.email])
+
+
+class TrueField(forms.BooleanField):
+    """
+    A boolean field that only accepts "true" (case-insensitive) as true
+    """
+    def to_python(self, value):
+        # CheckboxInput converts string to bool by case-insensitive match to "true" or "false"
+        if value is True:
+            return value
+        else:
+            return None
+
+
+_USERNAME_TOO_SHORT_MSG = _("Username must be minimum of two characters long")
+_EMAIL_INVALID_MSG = _("A properly formatted e-mail is required")
+_PASSWORD_INVALID_MSG = _("A valid password is required")
+_NAME_TOO_SHORT_MSG = _("Your legal name must be a minimum of two characters long")
+
+
+class AccountCreationForm(forms.Form):
+    """
+    A form to for account creation data. It is currently only used for
+    validation, not rendering.
+    """
+    # TODO: Resolve repetition
+    username = forms.SlugField(
+        min_length=2,
+        max_length=30,
+        error_messages={
+            "required": _USERNAME_TOO_SHORT_MSG,
+            "invalid": _("Username should only consist of A-Z and 0-9, with no spaces."),
+            "min_length": _USERNAME_TOO_SHORT_MSG,
+            "max_length": _("Username cannot be more than %(limit_value)s characters long"),
+        }
+    )
+    email = forms.EmailField(
+        max_length=75,  # Limit per RFCs is 254, but User's email field in django 1.4 only takes 75
+        error_messages={
+            "required": _EMAIL_INVALID_MSG,
+            "invalid": _EMAIL_INVALID_MSG,
+            "max_length": _("Email cannot be more than %(limit_value)s characters long"),
+        }
+    )
+    password = forms.CharField(
+        min_length=2,
+        error_messages={
+            "required": _PASSWORD_INVALID_MSG,
+            "min_length": _PASSWORD_INVALID_MSG,
+        }
+    )
+    name = forms.CharField(
+        min_length=2,
+        error_messages={
+            "required": _NAME_TOO_SHORT_MSG,
+            "min_length": _NAME_TOO_SHORT_MSG,
+        }
+    )
+
+    def __init__(
+            self,
+            data=None,
+            extra_fields=None,
+            extended_profile_fields=None,
+            enforce_username_neq_password=False,
+            enforce_password_policy=False,
+            tos_required=True
+    ):
+        super(AccountCreationForm, self).__init__(data)
+
+        extra_fields = extra_fields or {}
+        self.extended_profile_fields = extended_profile_fields or {}
+        self.enforce_username_neq_password = enforce_username_neq_password
+        self.enforce_password_policy = enforce_password_policy
+        if tos_required:
+            self.fields["terms_of_service"] = TrueField(
+                error_messages={"required": _("You must accept the terms of service.")}
+            )
+
+        # TODO: These messages don't say anything about minimum length
+        error_message_dict = {
+            "level_of_education": _("A level of education is required"),
+            "gender": _("Your gender is required"),
+            "year_of_birth": _("Your year of birth is required"),
+            "mailing_address": _("Your mailing address is required"),
+            "goals": _("A description of your goals is required"),
+            "city": _("A city is required"),
+            "country": _("A country is required")
+        }
+        for field_name, field_value in extra_fields.items():
+            if field_name not in self.fields:
+                if field_name == "honor_code":
+                    if field_value == "required":
+                        self.fields[field_name] = TrueField(
+                            error_messages={
+                                "required": _("To enroll, you must follow the honor code.")
+                            }
+                        )
+                else:
+                    required = field_value == "required"
+                    min_length = 1 if field_name in ("gender", "level_of_education") else 2
+                    error_message = error_message_dict.get(
+                        field_name,
+                        _("You are missing one or more required fields")
+                    )
+                    self.fields[field_name] = forms.CharField(
+                        required=required,
+                        min_length=min_length,
+                        error_messages={
+                            "required": error_message,
+                            "min_length": error_message,
+                        }
+                    )
+
+        for field in self.extended_profile_fields:
+            if field not in self.fields:
+                self.fields[field] = forms.CharField(required=False)
+
+    def clean_password(self):
+        """Enforce password policies (if applicable)"""
+        password = self.cleaned_data["password"]
+        if (
+                self.enforce_username_neq_password and
+                "username" in self.cleaned_data and
+                self.cleaned_data["username"] == password
+        ):
+            raise ValidationError(_("Username and password fields cannot match"))
+        if self.enforce_password_policy:
+            try:
+                validate_password_length(password)
+                validate_password_complexity(password)
+                validate_password_dictionary(password)
+            except ValidationError, err:
+                raise ValidationError(_("Password: ") + "; ".join(err.messages))
+        return password
+
+    def clean_year_of_birth(self):
+        """
+        Parse year_of_birth to an integer, but just use None instead of raising
+        an error if it is malformed
+        """
+        try:
+            year_str = self.cleaned_data["year_of_birth"]
+            return int(year_str) if year_str is not None else None
+        except ValueError:
+            return None
+
+    @property
+    def cleaned_extended_profile(self):
+        """
+        Return a dictionary containing the extended_profile_fields and values
+        """
+        return {
+            key: value
+            for key, value in self.cleaned_data.items()
+            if key in self.extended_profile_fields and value is not None
+        }

common/djangoapps/student/management/commands/create_random_users.py

@@ -8,26 +8,30 @@ from student.models import CourseEnrollment
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from student.forms import AccountCreationForm
 from student.views import _do_create_account
 
 
-def get_random_post_override():
+def make_random_form():
     """
     Generate unique user data for dummy users.
     """
     identification = uuid.uuid4().hex[:8]
-    return {
-        'username': 'user_{id}'.format(id=identification),
-        'email': 'email_{id}@example.com'.format(id=identification),
-        'password': '12345',
-        'name': 'User {id}'.format(id=identification),
-    }
+    return AccountCreationForm(
+        data={
+            'username': 'user_{id}'.format(id=identification),
+            'email': 'email_{id}@example.com'.format(id=identification),
+            'password': '12345',
+            'name': 'User {id}'.format(id=identification),
+        },
+        tos_required=False
+    )
 
 
 def create(num, course_key):
     """Create num users, enrolling them in course_key if it's not None"""
     for idx in range(num):
-        (user, user_profile, __) = _do_create_account(get_random_post_override())
+        (user, _, _) = _do_create_account(make_random_form())
         if course_key is not None:
             CourseEnrollment.enroll(user, course_key)
 

common/djangoapps/student/management/commands/create_user.py

@@ -8,6 +8,7 @@ from django.utils import translation
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from student.forms import AccountCreationForm
 from student.models import CourseEnrollment, Registration, create_comments_service_user
 from student.views import _do_create_account, AccountValidationError
 from track.management.tracked_command import TrackedCommand
@@ -80,21 +81,22 @@ class Command(TrackedCommand):
             except InvalidKeyError:
                 course = SlashSeparatedCourseKey.from_deprecated_string(options['course'])
 
-        post_data = {
-            'username': username,
-            'email': options['email'],
-            'password': options['password'],
-            'name': name,
-            'honor_code': u'true',
-            'terms_of_service': u'true',
-        }
+        form = AccountCreationForm(
+            data={
+                'username': username,
+                'email': options['email'],
+                'password': options['password'],
+                'name': name,
+            },
+            tos_required=False
+        )
         # django.utils.translation.get_language() will be used to set the new
         # user's preferred language.  This line ensures that the result will
         # match this installation's default locale.  Otherwise, inside a
         # management command, it will always return "en-us".
         translation.activate(settings.LANGUAGE_CODE)
         try:
-            user, profile, reg = _do_create_account(post_data)
+            user, _, reg = _do_create_account(form)
             if options['staff']:
                 user.is_staff = True
                 user.save()

common/test/acceptance/tests/discussion/test_cohort_management.py

@@ -51,9 +51,9 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin):
         ).visit().get_user_id()
         self.add_user_to_cohort(self.course_fixture, self.student_name, self.manual_cohort_id)
 
-        # create a user with unicode characters in their username
-        self.unicode_student_id = AutoAuthPage(
-            self.browser, username="Ωπ", email="unicode_student_user@example.com",
+        # create a second student user
+        self.other_student_id = AutoAuthPage(
+            self.browser, username="other_student_user", email="other_student_user@example.com",
             course_id=self.course_id, staff=False
         ).visit().get_user_id()
 
@@ -389,12 +389,12 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin):
             }).count(),
             1
         )
-        # unicode_student_user (previously unassigned) is added to manual cohort
+        # other_student_user (previously unassigned) is added to manual cohort
         self.assertEqual(
             self.event_collection.find({
                 "name": "edx.cohort.user_added",
                 "time": {"$gt": start_time},
-                "event.user_id": {"$in": [int(self.unicode_student_id)]},
+                "event.user_id": {"$in": [int(self.other_student_id)]},
                 "event.cohort_name": self.manual_cohort_name,
             }).count(),
             1

common/test/acceptance/tests/test_cohorted_courseware.py

@@ -47,7 +47,7 @@ class EndToEndCohortedCoursewareTest(ContainerBase):
         ).visit()
 
         # Create a student who will end up in the default cohort group
-        self.cohort_default_student_username = "cohort default student"
+        self.cohort_default_student_username = "cohort_default_student"
         self.cohort_default_student_email = "cohort_default_student@example.com"
         StudioAutoAuthPage(
             self.browser, username=self.cohort_default_student_username,

common/test/data/uploads/cohort_users_both_columns.csv

 username,email,ignored_column,cohort
 instructor_user,,June,ManualCohort1
 ,student_user@example.com,Spring,AutoCohort1
-Ωπ,,Fall,ManualCohort1
+other_student_user,,Fall,ManualCohort1

common/test/data/uploads/cohort_users_only_email.csv

 email,cohort
 instructor_user@example.com,ManualCohort1
 student_user@example.com,AutoCohort1
-unicode_student_user@example.com,ManualCohort1
+other_student_user@example.com,ManualCohort1
 

common/test/data/uploads/cohort_users_only_username.csv

 username,cohort
 instructor_user,ManualCohort1
 student_user,AutoCohort1
-Ωπ,ManualCohort1
+other_student_user,ManualCohort1
\ No newline at end of file

lms/static/js/spec/student_account/register_spec.js

@@ -261,12 +261,8 @@ define([
             submitForm( true );
 
             // Verify that the client sent the course ID for analytics
-            var expectedData = {};
-            $.extend(expectedData, USER_DATA, {
-                analytics: JSON.stringify({
-                    enroll_course_id: COURSE_ID
-                })
-            });
+            var expectedData = {course_id: COURSE_ID};
+            $.extend(expectedData, USER_DATA);
 
             AjaxHelpers.expectRequest(
                 requests, 'POST',

lms/static/js/student_account/models/RegisterModel.js

@@ -32,20 +32,17 @@ var edx = edx || {};
         sync: function(method, model) {
             var headers = { 'X-CSRFToken': $.cookie('csrftoken') },
                 data = {},
-                analytics,
                 courseId = $.url( '?course_id' );
 
             // If there is a course ID in the query string param,
             // send that to the server as well so it can be included
             // in analytics events.
             if ( courseId ) {
-                analytics = JSON.stringify({
-                    enroll_course_id: decodeURIComponent( courseId )
-                });
+                data.course_id = decodeURIComponent(courseId);
             }
 
             // Include all form fields and analytics info in the data sent to the server
-            $.extend( data, model.attributes, { analytics: analytics });
+            $.extend( data, model.attributes);
 
             $.ajax({
                 url: model.urlRoot,

lms/static/js/student_account/views/RegisterView.js

@@ -58,7 +58,22 @@ var edx = edx || {};
 
         saveSuccess: function() {
             this.trigger('auth-complete');
-        }
+        },
         
+        saveError: function( error ) {
+            this.errors = _.flatten(
+                _.map(
+                    JSON.parse(error.responseText),
+                    function(error_list) {
+                        return _.map(
+                            error_list,
+                            function(error) { return "<li>" + error.user_message + "</li>"; }
+                        );
+                    }
+                )
+            );
+            this.setErrors();
+            this.toggleDisableButton(false);
+        }
     });
 })(jQuery, _, gettext);

openedx/core/djangoapps/user_api/helpers.py

@@ -374,16 +374,6 @@ def shim_student_view(view_func, check_logged_in=False):
                     )
                 )
 
-        # Backwards compatibility: the student view expects both
-        # terms of service and honor code values.  Since we're combining
-        # these into a single checkbox, the only value we may get
-        # from the new view is "honor_code".
-        # Longer term, we will need to make this more flexible to support
-        # open source installations that may have separate checkboxes
-        # for TOS, privacy policy, etc.
-        if request.POST.get("honor_code") is not None and request.POST.get("terms_of_service") is None:
-            request.POST["terms_of_service"] = request.POST.get("honor_code")
-
         # Call the original view to generate a response.
         # We can safely modify the status code or content
         # of the response, but to be safe we won't mess

openedx/core/djangoapps/user_api/tests/test_views.py

@@ -1233,6 +1233,7 @@ class RegistrationViewTest(ApiTestCase):
             "honor_code": "true",
         })
         self.assertHttpOK(response)
+        self.assertIn(settings.EDXMKTG_COOKIE_NAME, self.client.cookies)
 
         # Verify that the user exists
         self.assertEqual(
@@ -1367,11 +1368,19 @@ class RegistrationViewTest(ApiTestCase):
             "honor_code": "true",
         })
         self.assertEqual(response.status_code, 409)
+        response_json = json.loads(response.content)
         self.assertEqual(
-            response.content,
-            "It looks like {} belongs to an existing account. Try again with a different email address.".format(
-                self.EMAIL
-            )
+            response_json,
+            {
+                "email": [{
+                    "user_message": (
+                        "It looks like {} belongs to an existing account. "
+                        "Try again with a different email address."
+                    ).format(
+                        self.EMAIL
+                    )
+                }]
+            }
         )
 
     def test_register_duplicate_username(self):
@@ -1394,11 +1403,19 @@ class RegistrationViewTest(ApiTestCase):
             "honor_code": "true",
         })
         self.assertEqual(response.status_code, 409)
+        response_json = json.loads(response.content)
         self.assertEqual(
-            response.content,
-            "It looks like {} belongs to an existing account. Try again with a different username.".format(
-                self.USERNAME
-            )
+            response_json,
+            {
+                "username": [{
+                    "user_message": (
+                        "It looks like {} belongs to an existing account. "
+                        "Try again with a different username."
+                    ).format(
+                        self.USERNAME
+                    )
+                }]
+            }
         )
 
     def test_register_duplicate_username_and_email(self):
@@ -1421,11 +1438,46 @@ class RegistrationViewTest(ApiTestCase):
             "honor_code": "true",
         })
         self.assertEqual(response.status_code, 409)
+        response_json = json.loads(response.content)
         self.assertEqual(
-            response.content,
-            "It looks like {} and {} belong to an existing account. Try again with a different email address and username.".format(
-                self.EMAIL, self.USERNAME
-            )
+            response_json,
+            {
+                "username": [{
+                    "user_message": (
+                        "It looks like {} belongs to an existing account. "
+                        "Try again with a different username."
+                    ).format(
+                        self.USERNAME
+                    )
+                }],
+                "email": [{
+                    "user_message": (
+                        "It looks like {} belongs to an existing account. "
+                        "Try again with a different email address."
+                    ).format(
+                        self.EMAIL
+                    )
+                }]
+            }
+        )
+
+    def test_missing_fields(self):
+        response = self.client.post(
+            self.url,
+            {
+                "email": self.EMAIL,
+                "name": self.NAME,
+                "honor_code": "true",
+            }
+        )
+        self.assertEqual(response.status_code, 400)
+        response_json = json.loads(response.content)
+        self.assertEqual(
+            response_json,
+            {
+                "username": [{"user_message": "Username must be minimum of two characters long"}],
+                "password": [{"user_message": "A valid password is required"}],
+            }
         )
 
     def _assert_reg_field(self, extra_fields_setting, expected_field):

openedx/core/djangoapps/user_api/views.py

@@ -8,10 +8,10 @@ from django.conf import settings
 from django.contrib.auth.models import User
 from django.http import HttpResponse
 from django.core.urlresolvers import reverse
-from django.core.exceptions import ImproperlyConfigured
+from django.core.exceptions import ImproperlyConfigured, NON_FIELD_ERRORS, ValidationError
 from django.utils.translation import ugettext as _
 from django.utils.decorators import method_decorator
-from django.views.decorators.csrf import ensure_csrf_cookie, csrf_protect
+from django.views.decorators.csrf import ensure_csrf_cookie, csrf_protect, csrf_exempt
 from opaque_keys.edx import locator
 from rest_framework import authentication
 from rest_framework import filters
@@ -25,7 +25,9 @@ from django_comment_common.models import Role
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from edxmako.shortcuts import marketing_link
 
+from student.views import create_account_with_params, set_marketing_cookie
 from util.authentication import SessionAuthenticationAllowInactiveUser
+from util.json_request import JsonResponse
 from .api import account as account_api, profile as profile_api
 from .helpers import FormDescription, shim_student_view, require_post_params
 from .models import UserPreference, UserProfile
@@ -225,17 +227,14 @@ class RegistrationView(APIView):
 
         return HttpResponse(form_desc.to_json(), content_type="application/json")
 
-    @method_decorator(require_post_params(DEFAULT_FIELDS))
-    @method_decorator(csrf_protect)
+    @method_decorator(csrf_exempt)
     def post(self, request):
         """Create the user's account.
 
         You must send all required form fields with the request.
 
-        You can optionally send an `analytics` param with a JSON-encoded
-        object with additional info to include in the registration analytics event.
-        Currently, the only supported field is "enroll_course_id" to indicate
-        that the user registered while enrolling in a particular course.
+        You can optionally send a "course_id" param to indicate in analytics
+        events that the user registered while enrolling in a particular course.
 
         Arguments:
             request (HTTPRequest)
@@ -243,45 +242,60 @@ class RegistrationView(APIView):
         Returns:
             HttpResponse: 200 on success
             HttpResponse: 400 if the request is not valid.
-            HttpResponse: 302 if redirecting to another page.
-
+            HttpResponse: 409 if an account with the given username or email
+                address already exists
         """
-        email = request.POST.get('email')
-        username = request.POST.get('username')
+        data = request.POST.copy()
+
+        email = data.get('email')
+        username = data.get('username')
 
         # Handle duplicate email/username
         conflicts = account_api.check_account_exists(email=email, username=username)
         if conflicts:
-            if all(conflict in conflicts for conflict in ['email', 'username']):
-                # Translators: This message is shown to users who attempt to create a new
-                # account using both an email address and a username associated with an
-                # existing account.
-                error_msg = _(
-                    u"It looks like {email_address} and {username} belong to an existing account. Try again with a different email address and username."
-                ).format(email_address=email, username=username)
-            elif 'email' in conflicts:
+            conflict_messages = {
                 # Translators: This message is shown to users who attempt to create a new
                 # account using an email address associated with an existing account.
-                error_msg = _(
+                "email": _(
                     u"It looks like {email_address} belongs to an existing account. Try again with a different email address."
-                ).format(email_address=email)
-            else:
+                ).format(email_address=email),
                 # Translators: This message is shown to users who attempt to create a new
                 # account using a username associated with an existing account.
-                error_msg = _(
+                "username": _(
                     u"It looks like {username} belongs to an existing account. Try again with a different username."
-                ).format(username=username)
+                ).format(username=username),
+            }
+            errors = {
+                field: [{"user_message": conflict_messages[field]}]
+                for field in conflicts
+            }
+            return JsonResponse(errors, status=409)
+
+        # Backwards compatibility: the student view expects both
+        # terms of service and honor code values.  Since we're combining
+        # these into a single checkbox, the only value we may get
+        # from the new view is "honor_code".
+        # Longer term, we will need to make this more flexible to support
+        # open source installations that may have separate checkboxes
+        # for TOS, privacy policy, etc.
+        if data.get("honor_code") and "terms_of_service" not in data:
+            data["terms_of_service"] = data["honor_code"]
 
-            return HttpResponse(
-                status=409,
-                content=error_msg,
-                content_type="text/plain"
-            )
+        try:
+            create_account_with_params(request, data)
+        except ValidationError as err:
+            # Should only get non-field errors from this function
+            assert NON_FIELD_ERRORS not in err.message_dict
+            # Only return first error for each field
+            errors = {
+                field: [{"user_message": error} for error in error_list]
+                for field, error_list in err.message_dict.items()
+            }
+            return JsonResponse(errors, status=400)
 
-        # For the initial implementation, shim the existing login view
-        # from the student Django app.
-        from student.views import create_account
-        return shim_student_view(create_account)(request)
+        response = JsonResponse({"success": True})
+        set_marketing_cookie(request, response)
+        return response
 
     def _add_email_field(self, form_desc, required=True):
         """Add an email field to a form description.