From 1dfd3310f754a4b0b409e7626b9b747752c445d8 Mon Sep 17 00:00:00 2001
From: David Baumgold <david@davidbaumgold.com>
Date: Fri, 7 Feb 2014 16:16:46 -0500
Subject: [PATCH] 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 | 7 +-
common/djangoapps/student/tests/tests.py | 34 +++-
common/djangoapps/student/views.py | 188 +++++++++++-------
3 files changed, 142 insertions(+), 87 deletions(-)
diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py
index 9be1e7de04b..871e662fcd0 100644
--- a/common/djangoapps/student/tests/test_login.py
+++ b/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):
diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py
index 8df41609d69..cc25c1eb23a 100644
--- a/common/djangoapps/student/tests/tests.py
+++ b/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)
diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py
index f1feb1f1d53..ac047cd6da4 100644
--- a/common/djangoapps/student/views.py
+++ b/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
--
GitLab