From 8807d9fe51c14d035827c71542f4fe691789b4df Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 5 Aug 2013 11:25:02 -0400 Subject: [PATCH 1/4] Integrate JsonResponse into lms/djangoapps/instructor/views/api.py --- lms/djangoapps/instructor/views/api.py | 90 +++++++++----------------- 1 file changed, 31 insertions(+), 59 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 34698225c5..ecfb7f46c6 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -7,12 +7,13 @@ Many of these GETs may become PUTs in the future. """ import re -import json import logging from django_future.csrf import ensure_csrf_cookie from django.views.decorators.cache import cache_control from django.core.urlresolvers import reverse -from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden +from django.utils.translation import ugettext as _ +from django.http import HttpResponseBadRequest, HttpResponseForbidden +from util.json_request import JsonResponse from courseware.access import has_access from courseware.courses import get_course_with_access, get_course_by_id @@ -41,13 +42,23 @@ def common_exceptions_400(func): Catches common exceptions and renders matching 400 errors. (decorator without arguments) """ - def wrapped(*args, **kwargs): # pylint: disable=C0111 + def wrapped(request, *args, **kwargs): # pylint: disable=C0111 + use_json = (request.is_ajax() or + request.META.get("HTTP_ACCEPT", "").startswith("application/json")) try: - return func(*args, **kwargs) + return func(request, *args, **kwargs) except User.DoesNotExist: - return HttpResponseBadRequest("User does not exist.") + message = "User does not exist." + if use_json: + return JsonResponse({"error": _(message)}, 400) + else: + return HttpResponseBadRequest(_(message)) except AlreadyRunningError: - return HttpResponseBadRequest("Task already running.") + message = "Task is already running." + if use_json: + return JsonResponse({"error": _(message)}, 400) + else: + return HttpResponseBadRequest(_(message)) return wrapped @@ -82,10 +93,7 @@ def require_query_params(*args, **kwargs): error_response_data['info'][param] = extra if len(error_response_data['parameters']) > 0: - return HttpResponseBadRequest( - json.dumps(error_response_data), - mimetype="application/json", - ) + return JsonResponse(error_response_data, status=400) else: return func(*args, **kwargs) return wrapped @@ -194,10 +202,7 @@ def students_update_enrollment(request, course_id): 'results': results, 'auto_enroll': auto_enroll, } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -255,10 +260,7 @@ def modify_access(request, course_id): 'action': action, 'success': 'yes', } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -308,10 +310,7 @@ def list_course_role_members(request, course_id): course, rolename )), } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -330,10 +329,7 @@ def get_grading_config(request, course_id): 'course_id': course_id, 'grading_config_summary': grading_config_summary, } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -362,10 +358,7 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=W06 'queried_features': query_features, 'available_features': available_features, } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) else: header, datarows = analytics.csvs.format_dictlist(student_data, query_features) return analytics.csvs.create_csv_response("enrolled_profiles.csv", header, datarows) @@ -417,10 +410,7 @@ def get_distribution(request, course_id): if p_dist.type == 'EASY_CHOICE': response_payload['feature_results']['choices_display_names'] = p_dist.choices_display_names - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -449,10 +439,7 @@ def get_student_progress_url(request, course_id): 'course_id': course_id, 'progress_url': progress_url, } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -521,10 +508,7 @@ def reset_student_attempts(request, course_id): else: return HttpResponseBadRequest() - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -572,10 +556,7 @@ def rescore_problem(request, course_id): else: return HttpResponseBadRequest() - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -618,10 +599,7 @@ def list_instructor_tasks(request, course_id): response_payload = { 'tasks': map(extract_task_features, tasks), } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -680,10 +658,7 @@ def list_forum_members(request, course_id): 'course_id': course_id, rolename: map(extract_user_info, users), } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) @ensure_csrf_cookie @@ -747,10 +722,7 @@ def update_forum_role_membership(request, course_id): 'course_id': course_id, 'action': action, } - response = HttpResponse( - json.dumps(response_payload), content_type="application/json" - ) - return response + return JsonResponse(response_payload) def _split_input_list(str_list): @@ -782,6 +754,6 @@ def _msk_from_problem_urlname(course_id, urlname): urlname = "problem/" + urlname - (org, course_name, _) = course_id.split("/") + (org, course_name, __) = course_id.split("/") module_state_key = "i4x://" + org + "/" + course_name + "/" + urlname return module_state_key From 7b01147c1a92b34503bebc40a82e6669859fad48 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 5 Aug 2013 13:51:47 -0400 Subject: [PATCH 2/4] Translate messages early --- lms/djangoapps/instructor/views/api.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index ecfb7f46c6..64d70232f8 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -48,17 +48,17 @@ def common_exceptions_400(func): try: return func(request, *args, **kwargs) except User.DoesNotExist: - message = "User does not exist." + message = _("User does not exist.") if use_json: - return JsonResponse({"error": _(message)}, 400) + return JsonResponse({"error": message}, 400) else: - return HttpResponseBadRequest(_(message)) + return HttpResponseBadRequest(message) except AlreadyRunningError: - message = "Task is already running." + message = _("Task is already running.") if use_json: - return JsonResponse({"error": _(message)}, 400) + return JsonResponse({"error": message}, 400) else: - return HttpResponseBadRequest(_(message)) + return HttpResponseBadRequest(message) return wrapped From ef4220ee431de87e04f5c08753f8fcd2f8ae9a3b Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 5 Aug 2013 16:54:56 -0400 Subject: [PATCH 3/4] Add unit tests to exercise common_exceptions_400 decorator --- lms/djangoapps/instructor/tests/test_api.py | 61 ++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index d8bea4e4c7..d546805a5e 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1,7 +1,7 @@ """ Unit tests for instructor.api methods. """ - +import unittest import json from urllib import quote from django.test import TestCase @@ -9,6 +9,7 @@ from nose.tools import raises from mock import Mock from django.test.utils import override_settings from django.core.urlresolvers import reverse +from django.http import HttpRequest, HttpResponse from django.contrib.auth.models import User from courseware.tests.modulestore_config import TEST_DATA_MONGO_MODULESTORE @@ -21,7 +22,63 @@ from student.models import CourseEnrollment from courseware.models import StudentModule from instructor.access import allow_access -from instructor.views.api import _split_input_list, _msk_from_problem_urlname +from instructor.views.api import ( + _split_input_list, _msk_from_problem_urlname, common_exceptions_400) +from instructor_task.api_helper import AlreadyRunningError + + +@common_exceptions_400 +def view_success(request): + "A dummy view for testing that returns a simple HTTP response" + return HttpResponse('success') + + +@common_exceptions_400 +def view_user_doesnotexist(request): + "A dummy view that raises a User.DoesNotExist exception" + raise User.DoesNotExist() + + +@common_exceptions_400 +def view_alreadyrunningerror(request): + "A dummy view that raises an AlreadyRunningError exception" + raise AlreadyRunningError() + + +class TestCommonExceptions400(unittest.TestCase): + def setUp(self): + self.request = Mock(spec=HttpRequest) + self.request.META = {} + + def test_happy_path(self): + resp = view_success(self.request) + self.assertEqual(resp.status_code, 200) + + def test_user_doesnotexist(self): + self.request.is_ajax.return_value = False + resp = view_user_doesnotexist(self.request) + self.assertEqual(resp.status_code, 400) + self.assertIn("User does not exist", resp.content) + + def test_user_doesnotexist_ajax(self): + self.request.is_ajax.return_value = True + resp = view_user_doesnotexist(self.request) + self.assertEqual(resp.status_code, 400) + result = json.loads(resp.content) + self.assertIn("User does not exist", result["error"]) + + def test_alreadyrunningerror(self): + self.request.is_ajax.return_value = False + resp = view_alreadyrunningerror(self.request) + self.assertEqual(resp.status_code, 400) + self.assertIn("Task is already running", resp.content) + + def test_alreadyrunningerror_ajax(self): + self.request.is_ajax.return_value = True + resp = view_alreadyrunningerror(self.request) + self.assertEqual(resp.status_code, 400) + result = json.loads(resp.content) + self.assertIn("Task is already running", result["error"]) @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) From 714d3b96626018fa4ce56e70e36500f1583bebd7 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 6 Aug 2013 14:36:13 -0400 Subject: [PATCH 4/4] pylint fixes --- lms/djangoapps/instructor/tests/test_api.py | 16 +++++++--------- lms/djangoapps/instructor/views/api.py | 10 +++++----- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index d546805a5e..cc2e23e8fe 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1,6 +1,7 @@ """ Unit tests for instructor.api methods. """ +# pylint: disable=E1111 import unittest import json from urllib import quote @@ -28,24 +29,27 @@ from instructor_task.api_helper import AlreadyRunningError @common_exceptions_400 -def view_success(request): +def view_success(request): # pylint: disable=W0613 "A dummy view for testing that returns a simple HTTP response" return HttpResponse('success') @common_exceptions_400 -def view_user_doesnotexist(request): +def view_user_doesnotexist(request): # pylint: disable=W0613 "A dummy view that raises a User.DoesNotExist exception" raise User.DoesNotExist() @common_exceptions_400 -def view_alreadyrunningerror(request): +def view_alreadyrunningerror(request): # pylint: disable=W0613 "A dummy view that raises an AlreadyRunningError exception" raise AlreadyRunningError() class TestCommonExceptions400(unittest.TestCase): + """ + Testing the common_exceptions_400 decorator. + """ def setUp(self): self.request = Mock(spec=HttpRequest) self.request.META = {} @@ -749,12 +753,6 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertEqual(json.loads(response.content), expected_res) - - # class TestInstructorAPILevelsForums - # # list_forum_members - # # update_forum_role_membership - - class TestInstructorAPIHelpers(TestCase): """ Test helpers for instructor.api """ def test_split_input_list(self): diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 64d70232f8..7655fd5b13 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -189,7 +189,7 @@ def students_update_enrollment(request, course_id): }) # catch and log any exceptions # so that one error doesn't cause a 500. - except Exception as exc: + except Exception as exc: # pylint: disable=W0703 log.exception("Error while #{}ing student") log.exception(exc) results.append({ @@ -401,10 +401,10 @@ def get_distribution(request, course_id): if not feature is None: p_dist = analytics.distributions.profile_distribution(course_id, feature) response_payload['feature_results'] = { - 'feature': p_dist.feature, - 'feature_display_name': p_dist.feature_display_name, - 'data': p_dist.data, - 'type': p_dist.type, + 'feature': p_dist.feature, + 'feature_display_name': p_dist.feature_display_name, + 'data': p_dist.data, + 'type': p_dist.type, } if p_dist.type == 'EASY_CHOICE':