Refactor student views

Fix pylint/pep8 warnings, use JsonResponse instead of HttpResponse where useful,
put in TODOs to change HTTP status codes to be more accurate.

common/djangoapps/student/tests/test_login.py

@@ -23,6 +23,7 @@ from external_auth.models import ExternalAuthMap
 
 TEST_DATA_MIXED_MODULESTORE = mixed_store_config(settings.COMMON_TEST_DATA_ROOT, {})
 
+
 class LoginTest(TestCase):
     '''
     Test student.views.login_user() view
@@ -224,7 +225,11 @@ class ExternalAuthShibTest(ModuleStoreTestCase):
         """
         response = self.client.post(reverse('login'), {'email': self.user_w_map.email, 'password': ''})
         self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.content, json.dumps({'success': False, 'redirect': reverse('shib-login')}))
+        obj = json.loads(response.content)
+        self.assertEqual(obj, {
+            'success': False,
+            'redirect': reverse('shib-login'),
+        })
 
     @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set")
     def test__get_course_enrollment_domain(self):

common/djangoapps/student/tests/tests.py

@@ -68,8 +68,11 @@ class ResetPasswordTests(TestCase):
         bad_pwd_resp = password_reset(bad_pwd_req)
         # If they've got an unusable password, we return a successful response code
         self.assertEquals(bad_pwd_resp.status_code, 200)
-        self.assertEquals(bad_pwd_resp.content, json.dumps({'success': True,
-                                                            'value': "('registration/password_reset_done.html', [])"}))
+        obj = json.loads(bad_pwd_resp.content)
+        self.assertEquals(obj, {
+            'success': True,
+            'value': "('registration/password_reset_done.html', [])",
+        })
 
     @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True))
     def test_nonexist_email_password_reset(self):
@@ -80,12 +83,19 @@ class ResetPasswordTests(TestCase):
         # Note: even if the email is bad, we return a successful response code
         # This prevents someone potentially trying to "brute-force" find out which emails are and aren't registered with edX
         self.assertEquals(bad_email_resp.status_code, 200)
-        self.assertEquals(bad_email_resp.content, json.dumps({'success': True,
-                                                              'value': "('registration/password_reset_done.html', [])"}))
-
-    @unittest.skipIf(settings.FEATURES.get('DISABLE_RESET_EMAIL_TEST', False),
-                         dedent("""Skipping Test because CMS has not provided necessary templates for password reset.
-                                If LMS tests print this message, that needs to be fixed."""))
+        obj = json.loads(bad_email_resp.content)
+        self.assertEquals(obj, {
+            'success': True,
+            'value': "('registration/password_reset_done.html', [])",
+        })
+
+    @unittest.skipIf(
+        settings.FEATURES.get('DISABLE_RESET_EMAIL_TEST', False),
+        dedent("""
+            Skipping Test because CMS has not provided necessary templates for password reset.
+            If LMS tests print this message, that needs to be fixed.
+        """)
+    )
     @patch('django.core.mail.send_mail')
     @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True))
     def test_reset_password_email(self, send_email):
@@ -94,9 +104,11 @@ class ResetPasswordTests(TestCase):
         good_req = self.request_factory.post('/password_reset/', {'email': self.user.email})
         good_resp = password_reset(good_req)
         self.assertEquals(good_resp.status_code, 200)
-        self.assertEquals(good_resp.content,
-                          json.dumps({'success': True,
-                                      'value': "('registration/password_reset_done.html', [])"}))
+        obj = json.loads(good_resp.content)
+        self.assertEquals(obj, {
+            'success': True,
+            'value': "('registration/password_reset_done.html', [])",
+        })
 
         ((subject, msg, from_addr, to_addrs), sm_kwargs) = send_email.call_args
         self.assertIn("Password reset", subject)

common/djangoapps/student/views.py

@@ -4,9 +4,7 @@ Student Views
 import datetime
 import json
 import logging
-import random
 import re
-import string       # pylint: disable=W0402
 import urllib
 import uuid
 import time
@@ -18,13 +16,11 @@ from django.contrib.auth import logout, authenticate, login
 from django.contrib.auth.models import User, AnonymousUser
 from django.contrib.auth.decorators import login_required
 from django.contrib.auth.views import password_reset_confirm
-# from django.contrib.sessions.models import Session
 from django.core.cache import cache
 from django.core.context_processors import csrf
 from django.core.mail import send_mail
 from django.core.urlresolvers import reverse
 from django.core.validators import validate_email, validate_slug, ValidationError
-from django.core.exceptions import ObjectDoesNotExist
 from django.db import IntegrityError, transaction
 from django.http import (HttpResponse, HttpResponseBadRequest, HttpResponseForbidden,
                          Http404)
@@ -33,7 +29,6 @@ from django_future.csrf import ensure_csrf_cookie
 from django.utils.http import cookie_date, base36_to_int
 from django.utils.translation import ugettext as _
 from django.views.decorators.http import require_POST, require_GET
-from django.contrib.admin.views.decorators import staff_member_required
 
 from ratelimitbackend.exceptions import RateLimitException
 
@@ -46,6 +41,7 @@ from student.models import (
     CourseEnrollmentAllowed, UserStanding, LoginFailures
 )
 from student.forms import PasswordResetFormNoActive
+from student.firebase_token_generator import create_token
 
 from verify_student.models import SoftwareSecurePhotoVerification, MidcourseReverificationWindow
 from certificates.models import CertificateStatuses, certificate_status_for_student
@@ -69,7 +65,6 @@ import shoppingcart
 import track.views
 
 from dogapi import dog_stats_api
-from pytz import UTC
 
 from util.json_request import JsonResponse
 
@@ -265,7 +260,6 @@ def get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set):
                       .format(user.username, enrollment.course_id))
 
 
-
 def _cert_info(user, course, cert_status):
     """
     Implements the logic for cert_info -- split out for testing.
@@ -674,8 +668,10 @@ def accounts_login(request):
 def login_user(request, error=""):
     """AJAX request to log in the user."""
     if 'email' not in request.POST or 'password' not in request.POST:
-        return HttpResponse(json.dumps({'success': False,
-                                        'value': _('There was an error receiving your login information. Please email us.')}))  # TODO: User error message
+        return JsonResponse({
+            "success": False,
+            "value": _('There was an error receiving your login information. Please email us.'),  # TODO: User error message
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     email = request.POST['email']
     password = request.POST['password']
@@ -692,7 +688,10 @@ def login_user(request, error=""):
         try:
             eamap = ExternalAuthMap.objects.get(user=user)
             if eamap.external_domain.startswith(external_auth.views.SHIBBOLETH_DOMAIN_PREFIX):
-                return HttpResponse(json.dumps({'success': False, 'redirect': reverse('shib-login')}))
+                return JsonResponse({
+                    "success": False,
+                    "redirect": reverse('shib-login'),
+                })  # TODO: this should be status code 301  # pylint: disable=fixme
         except ExternalAuthMap.DoesNotExist:
             # This is actually the common case, logging in user without external linked login
             AUDIT_LOG.info("User %s w/o external auth attempting login", user)
@@ -701,12 +700,10 @@ def login_user(request, error=""):
     user_found_by_email_lookup = user
     if user_found_by_email_lookup and LoginFailures.is_feature_enabled():
         if LoginFailures.is_user_locked_out(user_found_by_email_lookup):
-            return HttpResponse(
-                json.dumps({
-                    'success': False,
-                    'value': _('This account has been temporarily locked due to excessive login failures. Try again later.')
-                })
-            )
+            return JsonResponse({
+                "success": False,
+                "value": _('This account has been temporarily locked due to excessive login failures. Try again later.'),
+            })  # TODO: this should be status code 429  # pylint: disable=fixme
 
     # if the user doesn't exist, we want to set the username to an invalid
     # username so that authentication is guaranteed to fail and we can take
@@ -716,8 +713,10 @@ def login_user(request, error=""):
         user = authenticate(username=username, password=password, request=request)
     # this occurs when there are too many attempts from the same IP address
     except RateLimitException:
-        return HttpResponse(json.dumps({'success': False,
-                                        'value': _('Too many failed login attempts. Try again later.')}))
+        return JsonResponse({
+            "success": False,
+            "value": _('Too many failed login attempts. Try again later.'),
+        })  # TODO: this should be status code 429  # pylint: disable=fixme
     if user is None:
         # tick the failed login counters if the user exists in the database
         if user_found_by_email_lookup and LoginFailures.is_feature_enabled():
@@ -727,8 +726,10 @@ def login_user(request, error=""):
         # doesn't exist, and doesn't have a corresponding password
         if username != "":
             AUDIT_LOG.warning(u"Login failed - password for {0} is invalid".format(email))
-        return HttpResponse(json.dumps({'success': False,
-                                        'value': _('Email or password is incorrect.')}))
+        return JsonResponse({
+            "success": False,
+            "value": _('Email or password is incorrect.'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     # successful login, clear failed login attempts counters, if applicable
     if LoginFailures.is_feature_enabled():
@@ -753,7 +754,10 @@ def login_user(request, error=""):
         redirect_url = try_change_enrollment(request)
 
         dog_stats_api.increment("common.student.successful_login")
-        response = HttpResponse(json.dumps({'success': True, 'redirect_url': redirect_url}))
+        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
@@ -767,12 +771,11 @@ def login_user(request, error=""):
             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,
-                            path='/',
-                            secure=None,
-                            httponly=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,
+        )
 
         return response
 
@@ -780,8 +783,10 @@ def login_user(request, error=""):
 
     reactivation_email_for_user(user)
     not_activated_msg = _("This account has not been activated. We have sent another activation message. Please check your e-mail for the activation instructions.")
-    return HttpResponse(json.dumps({'success': False,
-                                    'value': not_activated_msg}))
+    return JsonResponse({
+        "success": False,
+        "value": not_activated_msg,
+    })  # TODO: this should be status code 400  # pylint: disable=fixme
 
 
 @ensure_csrf_cookie
@@ -799,11 +804,13 @@ def logout_user(request):
     else:
         target = '/'
     response = redirect(target)
-    response.delete_cookie(settings.EDXMKTG_COOKIE_NAME,
-                           path='/',
-                           domain=settings.SESSION_COOKIE_DOMAIN)
+    response.delete_cookie(
+        settings.EDXMKTG_COOKIE_NAME,
+        path='/', domain=settings.SESSION_COOKIE_DOMAIN,
+    )
     return response
 
+
 @require_GET
 @login_required
 @ensure_csrf_cookie
@@ -890,8 +897,10 @@ def change_setting(request):
         up.location = request.POST['location']
     up.save()
 
-    return HttpResponse(json.dumps({'success': True,
-                                    'location': up.location, }))
+    return JsonResponse({
+        "success": True,
+        "location": up.location,
+    })
 
 
 def _do_create_account(post_vars):
@@ -1106,8 +1115,8 @@ def create_account(request, post_override=None):
                            '-' * 80 + '\n\n' + message)
                 send_mail(subject, message, from_address, [dest_addr], fail_silently=False)
             else:
-                _res = user.email_user(subject, message, from_address)
-        except:
+                user.email_user(subject, message, from_address)
+        except Exception:  # pylint: disable=broad-except
             log.warning('Unable to send activation email to user', 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
@@ -1298,8 +1307,10 @@ def password_reset(request):
                   from_email=settings.DEFAULT_FROM_EMAIL,
                   request=request,
                   domain_override=request.get_host())
-    return HttpResponse(json.dumps({'success': True,
-                                        'value': render_to_string('registration/password_reset_done.html', {})}))
+    return JsonResponse({
+        'success': True,
+        'value': render_to_string('registration/password_reset_done.html', {}),
+    })
 
 
 def password_reset_confirm_wrapper(
@@ -1330,23 +1341,30 @@ def reactivation_email_for_user(user):
     try:
         reg = Registration.objects.get(user=user)
     except Registration.DoesNotExist:
-        return HttpResponse(json.dumps({'success': False,
-                                        'error': _('No inactive user with this e-mail exists')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('No inactive user with this e-mail exists'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
-    d = {'name': user.profile.name,
-         'key': reg.activation_key}
+    context = {
+        'name': user.profile.name,
+        'key': reg.activation_key,
+    }
 
-    subject = render_to_string('emails/activation_email_subject.txt', d)
+    subject = render_to_string('emails/activation_email_subject.txt', context)
     subject = ''.join(subject.splitlines())
-    message = render_to_string('emails/activation_email.txt', d)
+    message = render_to_string('emails/activation_email.txt', context)
 
     try:
-        _res = user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
-    except:
+        user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
+    except Exception:  # pylint: disable=broad-except
         log.warning('Unable to send reactivation email', exc_info=True)
-        return HttpResponse(json.dumps({'success': False, 'error': _('Unable to send reactivation email')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Unable to send reactivation email')
+        })  # TODO: this should be status code 500  # pylint: disable=fixme
 
-    return HttpResponse(json.dumps({'success': True}))
+    return JsonResponse({"success": True})
 
 
 @ensure_csrf_cookie
@@ -1360,20 +1378,26 @@ def change_email_request(request):
     user = request.user
 
     if not user.check_password(request.POST['password']):
-        return HttpResponse(json.dumps({'success': False,
-                                        'error': _('Invalid password')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Invalid password'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     new_email = request.POST['new_email']
     try:
         validate_email(new_email)
     except ValidationError:
-        return HttpResponse(json.dumps({'success': False,
-                                        'error': _('Valid e-mail address required.')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Valid e-mail address required.'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     if User.objects.filter(email=new_email).count() != 0:
         ## CRITICAL TODO: Handle case sensitivity for e-mails
-        return HttpResponse(json.dumps({'success': False,
-                                        'error': _('An account with this e-mail already exists.')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('An account with this e-mail already exists.'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     pec_list = PendingEmailChange.objects.filter(user=request.user)
     if len(pec_list) == 0:
@@ -1388,8 +1412,10 @@ def change_email_request(request):
 
     if pec.new_email == user.email:
         pec.delete()
-        return HttpResponse(json.dumps({'success': False,
-                                        'error': _('Old email is the same as the new email.')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Old email is the same as the new email.'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     context = {
         'key': pec.activation_key,
@@ -1407,9 +1433,9 @@ def change_email_request(request):
         settings.DEFAULT_FROM_EMAIL
     )
 
-    _res = send_mail(subject, message, from_address, [pec.new_email])
+    send_mail(subject, message, from_address, [pec.new_email])
 
-    return HttpResponse(json.dumps({'success': True}))
+    return JsonResponse({"success": True})
 
 
 @ensure_csrf_cookie
@@ -1491,14 +1517,17 @@ def change_name_request(request):
     pnc.new_name = request.POST['new_name']
     pnc.rationale = request.POST['rationale']
     if len(pnc.new_name) < 2:
-        return HttpResponse(json.dumps({'success': False, 'error': _('Name required')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Name required'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
     pnc.save()
 
     # The following automatically accepts name change requests. Remove this to
     # go back to the old system where it gets queued up for admin approval.
     accept_name_change_by_id(pnc.id)
 
-    return HttpResponse(json.dumps({'success': True}))
+    return JsonResponse({"success": True})
 
 
 @ensure_csrf_cookie
@@ -1507,14 +1536,19 @@ def pending_name_changes(request):
     if not request.user.is_staff:
         raise Http404
 
-    changes = list(PendingNameChange.objects.all())
-    js = {'students': [{'new_name': c.new_name,
-                        'rationale': c.rationale,
-                        'old_name': UserProfile.objects.get(user=c.user).name,
-                        'email': c.user.email,
-                        'uid': c.user.id,
-                        'cid': c.id} for c in changes]}
-    return render_to_response('name_changes.html', js)
+    students = []
+    for change in PendingNameChange.objects.all():
+        profile = UserProfile.objects.get(user=change.user)
+        students.append({
+            "new_name": change.new_name,
+            "rationale": change.rationale,
+            "old_name": profile.name,
+            "email": change.user.email,
+            "uid": change.user.id,
+            "cid": change.id,
+        })
+
+    return render_to_response("name_changes.html", {"students": students})
 
 
 @ensure_csrf_cookie
@@ -1526,17 +1560,23 @@ def reject_name_change(request):
     try:
         pnc = PendingNameChange.objects.get(id=int(request.POST['id']))
     except PendingNameChange.DoesNotExist:
-        return HttpResponse(json.dumps({'success': False, 'error': _('Invalid ID')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Invalid ID'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     pnc.delete()
-    return HttpResponse(json.dumps({'success': True}))
+    return JsonResponse({"success": True})
 
 
 def accept_name_change_by_id(id):
     try:
         pnc = PendingNameChange.objects.get(id=id)
     except PendingNameChange.DoesNotExist:
-        return HttpResponse(json.dumps({'success': False, 'error': _('Invalid ID')}))
+        return JsonResponse({
+            "success": False,
+            "error": _('Invalid ID'),
+        })  # TODO: this should be status code 400  # pylint: disable=fixme
 
     u = pnc.user
     up = UserProfile.objects.get(user=u)
@@ -1552,7 +1592,7 @@ def accept_name_change_by_id(id):
     up.save()
     pnc.delete()
 
-    return HttpResponse(json.dumps({'success': True}))
+    return JsonResponse({"success": True})
 
 
 @ensure_csrf_cookie
@@ -1589,9 +1629,7 @@ def change_email_settings(request):
         log.info(u"User {0} ({1}) opted out of receiving emails from course {2}".format(user.username, user.email, course_id))
         track.views.server_track(request, "change-email-settings", {"receive_emails": "no", "course": course_id}, page='dashboard')
 
-    return HttpResponse(json.dumps({'success': True}))
-
-from student.firebase_token_generator import create_token
+    return JsonResponse({"success": True})
 
 
 @login_required