/create_account: use proper HTTP status codes

Use status code 400 when there is a validation error in creating an account.

cms/djangoapps/contentstore/tests/tests.py

@@ -111,7 +111,7 @@ class AuthTestCase(ContentStoreTestCase):
     def test_create_account_errors(self):
         # No post data -- should fail
         resp = self.client.post('/create_account', {})
-        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(resp.status_code, 400)
         data = parse_json(resp)
         self.assertEqual(data['success'], False)
 

cms/templates/signup.html

@@ -107,31 +107,25 @@ require(["jquery", "jquery.cookie"], function($) {
     $("label").removeClass("is-focused");
   });
 
-  // form validation
-  function postJSON(url, data, callback) {
-    $.ajax({type:'POST',
-      url: url,
-      dataType: 'json',
-      data: data,
-      success: callback,
-      headers : {'X-CSRFToken': $.cookie('csrftoken')}
-    });
-  }
-
   $('form#register_form').submit(function(e) {
     e.preventDefault();
     var submit_data = $('#register_form').serialize();
 
-    postJSON('/create_account',
-      submit_data,
-      function(json) {
-        if(json.success) {
-          location.href = "${'/course'}";
-        } else {
+    $.ajax({
+      url: '/create_account',
+      type: 'POST',
+      dataType: 'json',
+      data: submit_data,
+      headers: {'X-CSRFToken': $.cookie('csrftoken')},
+      success: function(json) {
+        location.href = "/course";
+      },
+      error: function(jqXHR, textStatus, errorThrown) {
+        json = $.parseJSON(jqXHR.responseText);
         $('#register_error').html(json.value).stop().addClass('is-shown');
-        }
-      }
-    );
+      },
+      notifyOnError: false
+    });
   });
 });
   </script>

common/djangoapps/student/views.py

@@ -897,13 +897,13 @@ def create_account(request, post_override=None):
         if a not in post_vars:
             js['value'] = _("Error (401 {field}). E-mail us.").format(field=a)
             js['field'] = a
-            return HttpResponse(json.dumps(js))
+            return JsonResponse(js, status=400)
 
     if extra_fields.get('honor_code', 'required') == 'required' and \
             post_vars.get('honor_code', 'false') != u'true':
         js['value'] = _("To enroll, you must follow the honor code.").format(field=a)
         js['field'] = 'honor_code'
-        return HttpResponse(json.dumps(js))
+        return JsonResponse(js, status=400)
 
     # Can't have terms of service for certain SHIB users, like at Stanford
     tos_required = (
@@ -919,7 +919,7 @@ def create_account(request, post_override=None):
         if post_vars.get('terms_of_service', 'false') != u'true':
             js['value'] = _("You must accept the terms of service.").format(field=a)
             js['field'] = 'terms_of_service'
-            return HttpResponse(json.dumps(js))
+            return JsonResponse(js, status=400)
 
     # Confirm appropriate fields are there.
     # TODO: Check e-mail format is correct.
@@ -957,21 +957,21 @@ def create_account(request, post_override=None):
             }
             js['value'] = error_str[field_name]
             js['field'] = field_name
-            return HttpResponse(json.dumps(js))
+            return JsonResponse(js, status=400)
 
     try:
         validate_email(post_vars['email'])
     except ValidationError:
         js['value'] = _("Valid e-mail is required.").format(field=a)
         js['field'] = 'email'
-        return HttpResponse(json.dumps(js))
+        return JsonResponse(js, status=400)
 
     try:
         validate_slug(post_vars['username'])
     except ValidationError:
         js['value'] = _("Username should only consist of A-Z and 0-9, with no spaces.").format(field=a)
         js['field'] = 'username'
-        return HttpResponse(json.dumps(js))
+        return JsonResponse(js, status=400)
 
     # Ok, looks like everything is legit.  Create the account.
     ret = _do_create_account(post_vars)
@@ -1007,7 +1007,10 @@ def create_account(request, post_override=None):
         except:
             log.warning('Unable to send activation email to user', exc_info=True)
             js['value'] = _('Could not send activation e-mail.')
-            return HttpResponse(json.dumps(js))
+            # 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
@@ -1034,14 +1037,12 @@ def create_account(request, post_override=None):
             login_user.save()
             AUDIT_LOG.info(u"Login activated on extauth account - {0} ({1})".format(login_user.username, login_user.email))
 
-    redirect_url = try_change_enrollment(request)
-
     dog_stats_api.increment("common.student.account_created")
 
-    response_params = {'success': True,
-                       'redirect_url': redirect_url}
-
-    response = HttpResponse(json.dumps(response_params))
+    response = JsonResponse({
+        'success': True,
+        'redirect_url': try_change_enrollment(request),
+    })
 
     # set the login cookie for the edx marketing site
     # we want this cookie to be accessed via javascript

lms/djangoapps/courseware/tests/test_registration_extra_vars.py

@@ -6,49 +6,51 @@ import json
 import uuid
 
 from django.conf import settings
+from django.test import TestCase
 from django.core.urlresolvers import reverse
 from mock import patch
 
-from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_post_code
 
-
-class TestExtraRegistrationVariables(LoginEnrollmentTestCase):
+class TestExtraRegistrationVariables(TestCase):
     """
     Test that extra registration variables are properly checked according to settings
     """
-
-    def _do_register_attempt(self, **extra_fields_values):
-        """
-        Helper method to make the call to the do registration
-        """
+    def setUp(self):
+        super(TestExtraRegistrationVariables, self).setUp()
+        self.url = reverse('create_account')
         username = 'foo_bar' + uuid.uuid4().hex
-        fields_values = {
+        self.url_params = {
             'username': username,
+            'name': username,
             'email': 'foo' + uuid.uuid4().hex + '@bar.com',
             'password': 'password',
-            'name': username,
             'terms_of_service': 'true',
+            'honor_code': 'true',
         }
-        fields_values = dict(fields_values.items() + extra_fields_values.items())
-        resp = check_for_post_code(self, 200, reverse('create_account'), fields_values)
-        data = json.loads(resp.content)
-        return data
 
     def test_default_missing_honor(self):
         """
         By default, the honor code must be required
         """
-        data = self._do_register_attempt(honor_code='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'To enroll, you must follow the honor code.')
+        self.url_params['honor_code'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'To enroll, you must follow the honor code.',
+        )
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'honor_code': 'optional'})
     def test_optional_honor(self):
         """
         With the honor code is made optional, should pass without extra vars
         """
-        data = self._do_register_attempt(honor_code='')
-        self.assertEqual(data['success'], True)
+        self.url_params['honor_code'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertEqual(obj['success'], True)
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {
         'level_of_education': 'hidden',
@@ -63,89 +65,164 @@ class TestExtraRegistrationVariables(LoginEnrollmentTestCase):
         """
         When the fields are all hidden, should pass without extra vars
         """
-        data = self._do_register_attempt()
-        self.assertEqual(data['success'], True)
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'city': 'required'})
     def test_required_city_missing(self):
         """
         Should require the city if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', city='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'A city is required')
+        self.url_params['city'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'A city is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', city='New York')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'city': 'required'})
+    def test_required_city(self):
+        """
+        Should require the city if configured as 'required' but missing
+        """
+        self.url_params['city'] = 'New York'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'})
     def test_required_country_missing(self):
         """
         Should require the country if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', country='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'A country is required')
+        self.url_params['country'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'A country is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', country='New York')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'})
+    def test_required_country(self):
+        self.url_params['country'] = 'New York'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'level_of_education': 'required'})
     def test_required_level_of_education_missing(self):
         """
         Should require the level_of_education if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', level_of_education='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'A level of education is required.')
+        self.url_params['level_of_education'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'A level of education is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', level_of_education='p')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'level_of_education': 'required'})
+    def test_required_level_of_education(self):
+        self.url_params['level_of_education'] = 'p'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'gender': 'required'})
     def test_required_gender_missing(self):
         """
         Should require the gender if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', gender='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'Your gender is required')
+        self.url_params['gender'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'Your gender is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', gender='m')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'gender': 'required'})
+    def test_required_gender(self):
+        self.url_params['gender'] = 'm'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'year_of_birth': 'required'})
     def test_required_year_of_birth_missing(self):
         """
         Should require the year_of_birth if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', year_of_birth='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'Your year of birth is required')
+        self.url_params['year_of_birth'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'Your year of birth is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', year_of_birth='1982')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'year_of_birth': 'required'})
+    def test_required_year_of_birth(self):
+        self.url_params['year_of_birth'] = '1982'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'mailing_address': 'required'})
     def test_required_mailing_address_missing(self):
         """
         Should require the mailing_address if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', mailing_address='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'Your mailing address is required')
+        self.url_params['mailing_address'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'Your mailing address is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', mailing_address='my address')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'mailing_address': 'required'})
+    def test_required_mailing_address(self):
+        self.url_params['mailing_address'] = 'my address'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])
 
     @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'goals': 'required'})
     def test_required_goals_missing(self):
         """
         Should require the goals if configured as 'required' but missing
         """
-        data = self._do_register_attempt(honor_code='true', goals='')
-        self.assertEqual(data['success'], False)
-        self.assertEqual(data['value'], u'A description of your goals is required')
+        self.url_params['goals'] = ''
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 400)
+        obj = json.loads(response.content)
+        self.assertEqual(
+            obj['value'],
+            u'A description of your goals is required',
+        )
 
-        data = self._do_register_attempt(honor_code='true', goals='my goals')
-        self.assertEqual(data['success'], True)
+    @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'goals': 'required'})
+    def test_required_goals(self):
+        self.url_params['goals'] = 'my goals'
+        response = self.client.post(self.url, self.url_params)
+        self.assertEqual(response.status_code, 200)
+        obj = json.loads(response.content)
+        self.assertTrue(obj['success'])

lms/templates/register-shib.html

@@ -51,15 +51,17 @@
       });
 
       $('#register-form').on('ajax:success', function(event, json, xhr) {
-        if(json.success) {
-          location.href="${reverse('dashboard')}";
-       } else {
+        var url = json.redirect_url || "${reverse('dashboard')}";
+        location.href = url;
+      });
+
+      $('#register-form').on('ajax:error', function(event, jqXHR, textStatus) {
         toggleSubmitButton(true);
+        json = $.parseJSON(jqXHR.responseText);
         $('.status.message.submission-error').addClass('is-shown').focus();
         $('.status.message.submission-error .message-copy').html(json.value).stop().css("display", "block");
         $(".field-error").removeClass('field-error');
         $("[data-field='"+json.field+"']").addClass('field-error')
-       }
       });
     })(this);
 

lms/templates/register.html

@@ -53,20 +53,17 @@
       });
 
       $('#register-form').on('ajax:success', function(event, json, xhr) {
-        if(json.success) {
-          if(json.redirect_url){
-            location.href=json.redirect_url;
-          }
-          else {
-            location.href="${reverse('dashboard')}";
-          }
-       } else {
+        var url = json.redirect_url || "${reverse('dashboard')}";
+        location.href = url;
+      });
+
+      $('#register-form').on('ajax:error', function(event, jqXHR, textStatus) {
         toggleSubmitButton(true);
+        json = $.parseJSON(jqXHR.responseText);
         $('.status.message.submission-error').addClass('is-shown').focus();
         $('.status.message.submission-error .message-copy').html(json.value).stop().css("display", "block");
         $(".field-error").removeClass('field-error');
         $("[data-field='"+json.field+"']").addClass('field-error')
-       }
       });
     })(this);
 

lms/templates/signup_modal.html

@@ -152,13 +152,13 @@
 <script type="text/javascript">
   (function() {
    $(document).delegate('#register_form', 'ajax:success', function(data, json, xhr) {
-     if(json.success) {
      location.href="${reverse('dashboard')}";
-     } else {
+    });
+   $(document).delegate('#register_form', 'ajax:error', function(event, jqXHR, textStatus) {
+     json = $.parseJSON(jqXHR.responseText);
      $(".field-error").removeClass('field-error');
      $('#register_error').html(json.value).stop().css("display", "block");
      $("[data-field='"+json.field+"']").addClass('field-error')
-     }
     });
 
    // removing close link's default behavior