Refactor registration views to avoid shim

This also changes the registration API contract to use the course_id
parameter for analytics instead of an extra analytics parameter.

common/djangoapps/student/views.py

@@ -1404,15 +1404,23 @@ def _do_create_account(form):
     return (user, profile, registration)
 
 
-@csrf_exempt
-def create_account(request, post_override=None):  # pylint: disable-msg=too-many-statements
+def create_account_with_params(request, params):
     """
-    JSON call to create new edX account.
-    Used by form in signup_modal.html, which is included into navigation.html
-    """
-    js = {'success': False}  # pylint: disable-msg=invalid-name
+    Given a request and a dict of parameters (which may or may not have come
+    from the request), create an account for the requesting user, including
+    creating a comments service user object and sending an activation email.
+    This also takes external/third-party auth into account, updates that as
+    necessary, and authenticates the user for the request's session.
+
+    Does not return anything.
 
-    post_vars = post_override if post_override else request.POST
+    Raises AccountValidationError if an account with the username or email
+    specified by params already exists, or ValidationError if any of the given
+    parameters is invalid for any other reason.
+    """
+    # Copy params so we can modify it; we can't just do dict(params) because if
+    # params is request.POST, that results in a dict containing lists of values
+    params = dict(params.items())
 
     # allow for microsites to define their own set of required/optional/hidden fields
     extra_fields = microsite.get_value(
@@ -1421,33 +1429,25 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
     )
 
     if third_party_auth.is_enabled() and pipeline.running(request):
-        post_vars = dict(post_vars.items())
-        post_vars.update({'password': pipeline.make_random_password()})
+        params["password"] = pipeline.make_random_password()
 
     # if doing signup for an external authorization, then get email, password, name from the eamap
     # don't use the ones from the form, since the user could have hacked those
     # unless originally we didn't get a valid email or name from the external auth
+    # TODO: We do not check whether these values meet all necessary criteria, such as email length
     do_external_auth = 'ExternalAuthMap' in request.session
     if do_external_auth:
         eamap = request.session['ExternalAuthMap']
         try:
             validate_email(eamap.external_email)
-            email = eamap.external_email
+            params["email"] = eamap.external_email
         except ValidationError:
-            email = post_vars.get('email', '')
-        if eamap.external_name.strip() == '':
-            name = post_vars.get('name', '')
-        else:
-            name = eamap.external_name
-        password = eamap.internal_password
-        post_vars = dict(post_vars.items())
-        post_vars.update(dict(email=email, name=name, password=password))
-        log.debug(u'In create_account with external_auth: user = %s, email=%s', name, email)
+            pass
+        if eamap.external_name.strip() != '':
+            params["name"] = eamap.external_name
+        params["password"] = eamap.internal_password
+        log.debug(u'In create_account with external_auth: user = %s, email=%s', params["name"], params["email"])
 
-    extra_fields = microsite.get_value(
-        'REGISTRATION_EXTRA_FIELDS',
-        getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {})
-    )
     extended_profile_fields = microsite.get_value('extended_profile_fields', [])
     enforce_password_policy = (
         settings.FEATURES.get("ENFORCE_PASSWORD_POLICY", False) and
@@ -1464,7 +1464,7 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
     )
 
     form = AccountCreationForm(
-        data=post_vars,
+        data=params,
         extra_fields=extra_fields,
         extended_profile_fields=extended_profile_fields,
         enforce_username_neq_password=True,
@@ -1472,35 +1472,19 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
         tos_required=tos_required
     )
 
-    if not form.is_valid():
-        field, error_list = next(form.errors.iteritems())
-        return JsonResponse(
-            {
-                "success": False,
-                "field": field,
-                "value": error_list[0],
-            },
-            status=400
-        )
-
-    try:
     with transaction.commit_on_success():
         ret = _do_create_account(form)
-    except AccountValidationError as exc:
-        return JsonResponse({'success': False, 'value': exc.message, 'field': exc.field}, status=400)
 
     (user, profile, registration) = ret
 
     dog_stats_api.increment("common.student.account_created")
 
-    email = post_vars['email']
-
     # Track the user's registration
     if settings.FEATURES.get('SEGMENT_IO_LMS') and hasattr(settings, 'SEGMENT_IO_LMS_KEY'):
         tracking_context = tracker.get_tracker().resolve_context()
         analytics.identify(user.id, {
-            'email': email,
-            'username': username,
+            'email': user.email,
+            'username': user.username,
         })
 
         # If the user is registering via 3rd party auth, track which provider they use
@@ -1515,7 +1499,7 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
             "edx.bi.user.account.registered",
             {
                 'category': 'conversion',
-                'label': request.POST.get('course_id'),
+                'label': params.get('course_id'),
                 'provider': provider_name
             },
             context={
@@ -1528,7 +1512,7 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
     create_comments_service_user(user)
 
     context = {
-        'name': post_vars['name'],
+        'name': profile.name,
         'key': registration.activation_key,
     }
 
@@ -1559,16 +1543,11 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
                 user.email_user(subject, message, from_address)
         except Exception:  # pylint: disable=broad-except
             log.error(u'Unable to send activation email to user from "%s"', from_address, exc_info=True)
-            js['value'] = _('Could not send activation e-mail.')
-            # What is the correct status code to use here? I think it's 500, because
-            # the problem is on the server's end -- but also, the account was created.
-            # Seems like the core part of the request was successful.
-            return JsonResponse(js, status=500)
 
     # Immediately after a user creates an account, we log them in. They are only
     # logged in until they close the browser. They can't log in again until they click
     # the activation link from the email.
-    new_user = authenticate(username=post_vars['username'], password=post_vars['password'])
+    new_user = authenticate(username=user.username, password=params['password'])
     login(request, new_user)
     request.session.set_expiry(0)
 
@@ -1581,8 +1560,8 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
         eamap.user = new_user
         eamap.dtsignup = datetime.datetime.now(UTC)
         eamap.save()
-        AUDIT_LOG.info(u"User registered with external_auth %s", post_vars['username'])
-        AUDIT_LOG.info(u'Updated ExternalAuthMap for %s to be %s', post_vars['username'], eamap)
+        AUDIT_LOG.info(u"User registered with external_auth %s", new_user.username)
+        AUDIT_LOG.info(u'Updated ExternalAuthMap for %s to be %s', new_user.username, eamap)
 
         if settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'):
             log.info('bypassing activation email')
@@ -1590,23 +1569,12 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
             new_user.save()
             AUDIT_LOG.info(u"Login activated on extauth account - {0} ({1})".format(new_user.username, new_user.email))
 
-    dog_stats_api.increment("common.student.account_created")
-    redirect_url = try_change_enrollment(request)
-
-    # Resume the third-party-auth pipeline if necessary.
-    if third_party_auth.is_enabled() and pipeline.running(request):
-        running_pipeline = pipeline.get(request)
-        redirect_url = pipeline.get_complete_url(running_pipeline['backend'])
-
-    response = JsonResponse({
-        'success': True,
-        'redirect_url': redirect_url,
-    })
-
-    # set the login cookie for the edx marketing site
-    # we want this cookie to be accessed via javascript
-    # so httponly is set to None
 
+def set_marketing_cookie(request, response):
+    """
+    Set the login cookie for the edx marketing site on the given response. Its
+    expiration will match that of the given request's session.
+    """
     if request.session.get_expire_at_browser_close():
         max_age = None
         expires = None
@@ -1615,12 +1583,53 @@ def create_account(request, post_override=None):  # pylint: disable-msg=too-many
         expires_time = time.time() + max_age
         expires = cookie_date(expires_time)
 
-    response.set_cookie(settings.EDXMKTG_COOKIE_NAME,
-                        'true', max_age=max_age,
-                        expires=expires, domain=settings.SESSION_COOKIE_DOMAIN,
+    # we want this cookie to be accessed via javascript
+    # so httponly is set to None
+    response.set_cookie(
+        settings.EDXMKTG_COOKIE_NAME,
+        'true',
+        max_age=max_age,
+        expires=expires,
+        domain=settings.SESSION_COOKIE_DOMAIN,
         path='/',
         secure=None,
-                        httponly=None)
+        httponly=None
+    )
+
+
+@csrf_exempt
+def create_account(request, post_override=None):
+    """
+    JSON call to create new edX account.
+    Used by form in signup_modal.html, which is included into navigation.html
+    """
+    try:
+        create_account_with_params(request, post_override or request.POST)
+    except AccountValidationError as exc:
+        return JsonResponse({'success': False, 'value': exc.message, 'field': exc.field}, status=400)
+    except ValidationError as exc:
+        field, error_list = next(exc.message_dict.iteritems())
+        return JsonResponse(
+            {
+                "success": False,
+                "field": field,
+                "value": error_list[0],
+            },
+            status=400
+        )
+
+    redirect_url = try_change_enrollment(request)
+
+    # Resume the third-party-auth pipeline if necessary.
+    if third_party_auth.is_enabled() and pipeline.running(request):
+        running_pipeline = pipeline.get(request)
+        redirect_url = pipeline.get_complete_url(running_pipeline['backend'])
+
+    response = JsonResponse({
+        'success': True,
+        'redirect_url': redirect_url,
+    })
+    set_marketing_cookie(request, response)
     return response
 
 

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,

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(

openedx/core/djangoapps/user_api/views.py

@@ -8,7 +8,7 @@ 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, 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
@@ -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
@@ -232,10 +234,8 @@ class RegistrationView(APIView):
 
         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,11 +243,13 @@ 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)
@@ -278,10 +280,29 @@ class RegistrationView(APIView):
                 content_type="text/plain"
             )
 
-        # 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)
+        # 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"]
+
+        try:
+            create_account_with_params(request, data)
+        except ValidationError as err:
+            error_list = next(err.message_dict.itervalues())
+            return HttpResponse(
+                status=400,
+                content=error_list[0],
+                content_type="text/plain"
+            )
+
+        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.