From ef81556cc5db28d95937eeb6212fc68c4d35272e Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 27 Jun 2013 12:21:42 -0400 Subject: [PATCH] Use JsonResponse when it makes sense --- .../contentstore/tests/test_checklists.py | 2 +- cms/djangoapps/contentstore/views/assets.py | 2 +- .../contentstore/views/checklist.py | 7 +-- .../contentstore/views/component.py | 12 ++--- cms/djangoapps/contentstore/views/course.py | 53 ++++++++----------- common/djangoapps/util/json_request.py | 10 ++-- .../util/tests/test_json_request.py | 22 +++++++- 7 files changed, 59 insertions(+), 49 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_checklists.py b/cms/djangoapps/contentstore/tests/test_checklists.py index 92cc628a57..99ffb8678d 100644 --- a/cms/djangoapps/contentstore/tests/test_checklists.py +++ b/cms/djangoapps/contentstore/tests/test_checklists.py @@ -117,4 +117,4 @@ class ChecklistTestCase(CourseTestCase): 'name': self.course.location.name, 'checklist_index': 100}) response = self.client.delete(update_url) - self.assertContains(response, 'Unsupported request', status_code=400) + self.assertEqual(response.status_code, 405) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 902d5f3a07..664532c037 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -188,7 +188,7 @@ def upload_asset(request, org, course, coursename): 'msg': 'Upload completed' } - response = HttpResponse(json.dumps(response_payload), mimetype="application/json") + response = JsonResponse(response_payload) response['asset_url'] = StaticContent.get_url_path_from_location(content.location) return response diff --git a/cms/djangoapps/contentstore/views/checklist.py b/cms/djangoapps/contentstore/views/checklist.py index 99547a523b..5a7658542a 100644 --- a/cms/djangoapps/contentstore/views/checklist.py +++ b/cms/djangoapps/contentstore/views/checklist.py @@ -1,6 +1,7 @@ import json -from django.http import HttpResponse, HttpResponseBadRequest +from util.json_request import JsonResponse +from django.http import HttpResponseBadRequest from django.contrib.auth.decorators import login_required from django_future.csrf import ensure_csrf_cookie from mitxmako.shortcuts import render_to_response @@ -71,7 +72,7 @@ def update_checklist(request, org, course, name, checklist_index=None): course_module.checklists = course_module.checklists checklists, _ = expand_checklist_action_urls(course_module) modulestore.update_metadata(location, own_metadata(course_module)) - return HttpResponse(json.dumps(checklists[index]), mimetype="application/json") + return JsonResponse(checklists[index]) else: return HttpResponseBadRequest( "Could not save checklist state because the checklist index was out of range or unspecified.", @@ -81,7 +82,7 @@ def update_checklist(request, org, course, name, checklist_index=None): checklists, modified = expand_checklist_action_urls(course_module) if modified: modulestore.update_metadata(location, own_metadata(course_module)) - return HttpResponse(json.dumps(checklists), mimetype="application/json") + return JsonResponse(checklists) else: return HttpResponseBadRequest("Unsupported request.", content_type="text/plain") diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 6f5938f64f..c4fb927704 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -15,7 +15,7 @@ from xmodule.modulestore.django import modulestore from xmodule.util.date_utils import get_default_time_display from xblock.core import Scope -from util.json_request import expect_json +from util.json_request import expect_json, JsonResponse from contentstore.module_info_model import get_module_info, set_module_info from contentstore.utils import get_modulestore, get_lms_link_for_item, \ @@ -236,11 +236,9 @@ def assignment_type_update(request, org, course, category, name): raise HttpResponseForbidden() if request.method == 'GET': - return HttpResponse(json.dumps(CourseGradingModel.get_section_grader_type(location)), - mimetype="application/json") + return JsonResponse(CourseGradingModel.get_section_grader_type(location)) elif request.method == 'POST': # post or put, doesn't matter. - return HttpResponse(json.dumps(CourseGradingModel.update_section_grader_type(location, request.POST)), - mimetype="application/json") + return JsonResponse(CourseGradingModel.update_section_grader_type(location, request.POST)) @login_required @@ -309,8 +307,8 @@ def module_info(request, module_location): raise PermissionDenied() if real_method == 'GET': - return HttpResponse(json.dumps(get_module_info(get_modulestore(location), location, rewrite_static_links=rewrite_static_links)), mimetype="application/json") + return JsonResponse(get_module_info(get_modulestore(location), location, rewrite_static_links=rewrite_static_links)) elif real_method == 'POST' or real_method == 'PUT': - return HttpResponse(json.dumps(set_module_info(get_modulestore(location), location, request.POST)), mimetype="application/json") + return JsonResponse(set_module_info(get_modulestore(location), location, request.POST)) else: return HttpResponseBadRequest() diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index e6b1732c95..2b762cd4ef 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -11,7 +11,7 @@ from django.conf import settings from django.views.decorators.http import require_http_methods, require_POST from django.core.exceptions import PermissionDenied from django.core.urlresolvers import reverse -from django.http import HttpResponse, HttpResponseBadRequest +from django.http import HttpResponseBadRequest from util.json_request import JsonResponse from mitxmako.shortcuts import render_to_response @@ -109,8 +109,9 @@ def create_new_course(request): try: dest_location = Location('i4x', org, number, 'course', Location.clean(display_name)) except InvalidLocationError as error: - return HttpResponse(json.dumps({'ErrMsg': "Unable to create course '" + - display_name + "'.\n\n" + error.message})) + return JsonResponse({ + "ErrMsg": "Unable to create course '{name}'.\n\n{err}".format( + name=display_name, err=error.message)}) # see if the course already exists existing_course = None @@ -120,13 +121,13 @@ def create_new_course(request): pass if existing_course is not None: - return HttpResponse(json.dumps({'ErrMsg': 'There is already a course defined with this name.'})) + return JsonResponse({'ErrMsg': 'There is already a course defined with this name.'}) course_search_location = ['i4x', dest_location.org, dest_location.course, 'course', None] courses = modulestore().get_items(course_search_location) if len(courses) > 0: - return HttpResponse(json.dumps({'ErrMsg': 'There is already a course defined with the same organization and course number.'})) + return JsonResponse({'ErrMsg': 'There is already a course defined with the same organization and course number.'}) new_course = modulestore('direct').clone_item(template, dest_location) @@ -149,7 +150,7 @@ def create_new_course(request): # seed the forums seed_permissions_roles(new_course.location.course_id) - return HttpResponse(json.dumps({'id': new_course.location.url()})) + return JsonResponse({'id': new_course.location.url()}) @login_required @@ -201,19 +202,16 @@ def course_info_updates(request, org, course, provided_id=None): real_method = get_request_method(request) if request.method == 'GET': - return HttpResponse(json.dumps(get_course_updates(location)), - mimetype="application/json") + return JsonResponse(get_course_updates(location)) elif real_method == 'DELETE': try: - return HttpResponse(json.dumps(delete_course_update(location, - request.POST, provided_id)), mimetype="application/json") + return JsonResponse(delete_course_update(location, request.POST, provided_id)) except: return HttpResponseBadRequest("Failed to delete", content_type="text/plain") elif request.method == 'POST': try: - return HttpResponse(json.dumps(update_course_updates(location, - request.POST, provided_id)), mimetype="application/json") + return JsonResponse(update_course_updates(location, request.POST, provided_id)) except: return HttpResponseBadRequest("Failed to save", content_type="text/plain") @@ -304,11 +302,9 @@ def course_settings_updates(request, org, course, name, section): if request.method == 'GET': # Cannot just do a get w/o knowing the course name :-( - return HttpResponse(json.dumps(manager.fetch(Location(['i4x', org, course, 'course', name])), cls=CourseSettingsEncoder), - mimetype="application/json") + return JsonResponse(manager.fetch(Location(['i4x', org, course, 'course', name])), encoder=CourseSettingsEncoder) elif request.method == 'POST': # post or put, doesn't matter. - return HttpResponse(json.dumps(manager.update_from_json(request.POST), cls=CourseSettingsEncoder), - mimetype="application/json") + return JsonResponse(manager.update_from_json(request.POST), encoder=CourseSettingsEncoder) @expect_json @@ -328,15 +324,13 @@ def course_grader_updates(request, org, course, name, grader_index=None): if real_method == 'GET': # Cannot just do a get w/o knowing the course name :-( - return HttpResponse(json.dumps(CourseGradingModel.fetch_grader(Location(location), grader_index)), - mimetype="application/json") + return JsonResponse(CourseGradingModel.fetch_grader(Location(location), grader_index)) elif real_method == "DELETE": # ??? Should this return anything? Perhaps success fail? CourseGradingModel.delete_grader(Location(location), grader_index) - return HttpResponse() + return JsonResponse() elif request.method == 'POST': # post or put, doesn't matter. - return HttpResponse(json.dumps(CourseGradingModel.update_grader_from_json(Location(location), request.POST)), - mimetype="application/json") + return JsonResponse(CourseGradingModel.update_grader_from_json(Location(location), request.POST)) # # NB: expect_json failed on ["key", "key2"] and json payload @@ -354,12 +348,9 @@ def course_advanced_updates(request, org, course, name): real_method = get_request_method(request) if real_method == 'GET': - return HttpResponse(json.dumps(CourseMetadata.fetch(location)), - mimetype="application/json") + return JsonResponse(CourseMetadata.fetch(location)) elif real_method == 'DELETE': - return HttpResponse(json.dumps(CourseMetadata.delete_key(location, - json.loads(request.body))), - mimetype="application/json") + return JsonResponse(CourseMetadata.delete_key(location, json.loads(request.body))) elif real_method == 'POST' or real_method == 'PUT': # NOTE: request.POST is messed up because expect_json # cloned_request.POST.copy() is creating a defective entry w/ the whole payload as the key @@ -412,14 +403,12 @@ def course_advanced_updates(request, org, course, name): # Indicate that tabs should *not* be filtered out of the metadata filter_tabs = False try: - response_json = json.dumps(CourseMetadata.update_from_json(location, - request_body, - filter_tabs=filter_tabs)) - except (TypeError, ValueError), e: + return JsonResponse(CourseMetadata.update_from_json(location, + request_body, + filter_tabs=filter_tabs)) + except (TypeError, ValueError) as e: return HttpResponseBadRequest("Incorrect setting format. " + str(e), content_type="text/plain") - return HttpResponse(response_json, mimetype="application/json") - class TextbookValidationError(Exception): pass diff --git a/common/djangoapps/util/json_request.py b/common/djangoapps/util/json_request.py index e1d31a5f6a..32aa4ac9be 100644 --- a/common/djangoapps/util/json_request.py +++ b/common/djangoapps/util/json_request.py @@ -31,14 +31,16 @@ class JsonResponse(HttpResponse): """ Django HttpResponse subclass that has sensible defaults for outputting JSON. """ - def __init__(self, object=None, *args, **kwargs): + def __init__(self, object=None, status=None, encoder=DjangoJSONEncoder, + *args, **kwargs): if object in (None, ""): content = "" - kwargs.setdefault("status", 204) + status = status or 204 elif isinstance(object, QuerySet): content = serialize('json', object) else: - content = json.dumps(object, indent=2, cls=DjangoJSONEncoder, - ensure_ascii=False) + content = json.dumps(object, cls=encoder, indent=2, ensure_ascii=False) kwargs.setdefault("content_type", "application/json") + if status: + kwargs["status"] = status super(JsonResponse, self).__init__(content, *args, **kwargs) diff --git a/common/djangoapps/util/tests/test_json_request.py b/common/djangoapps/util/tests/test_json_request.py index 13751baad8..28d6a44286 100644 --- a/common/djangoapps/util/tests/test_json_request.py +++ b/common/djangoapps/util/tests/test_json_request.py @@ -2,6 +2,7 @@ from django.http import HttpResponse from util.json_request import JsonResponse import json import unittest +import mock class JsonResponseTestCase(unittest.TestCase): @@ -33,10 +34,29 @@ class JsonResponseTestCase(unittest.TestCase): self.assertEqual(resp.status_code, 200) self.assertEqual(resp["content-type"], "application/json") - def test_set_status(self): + def test_set_status_kwarg(self): obj = {"error": "resource not found"} resp = JsonResponse(obj, status=404) compare = json.loads(resp.content) self.assertEqual(obj, compare) self.assertEqual(resp.status_code, 404) self.assertEqual(resp["content-type"], "application/json") + + def test_set_status_arg(self): + obj = {"error": "resource not found"} + resp = JsonResponse(obj, 404) + compare = json.loads(resp.content) + self.assertEqual(obj, compare) + self.assertEqual(resp.status_code, 404) + self.assertEqual(resp["content-type"], "application/json") + + def test_encoder(self): + obj = [1, 2, 3] + encoder = object() + with mock.patch.object(json, "dumps", return_value="[1,2,3]") as dumps: + resp = JsonResponse(obj, encoder=encoder) + self.assertEqual(resp.status_code, 200) + compare = json.loads(resp.content) + self.assertEqual(obj, compare) + kwargs = dumps.call_args[1] + self.assertIs(kwargs["cls"], encoder)