From b60f5f807d2001e6fc734ff0fe21a72252d715bf Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 10 Oct 2013 10:46:39 -0400 Subject: [PATCH] Make forums endpoints return better status codes Previously, AJAX calls would return 400, and page views and attempts to load inline discussions would return 404 if communication with the comments service failed. Now such problems cause a 500 status code. --- .../django_comment_client/forum/tests.py | 2 +- .../django_comment_client/forum/views.py | 15 ++++++--------- .../django_comment_client/middleware.py | 4 ++-- .../tests/test_middleware.py | 10 ++++++++-- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 0fc5a14c70..aac03d54ce 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -65,7 +65,7 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(self.response.status_code, 404) @patch('student.models.cc.User.from_django_user') - @patch('student.models.cc.User.active_threads') + @patch('student.models.cc.User.subscribed_threads') def test_user_followed_threads_exception(self, mock_threads, mock_from_django_user): # Mock the code that makes the HTTP requests to the cs_comment_service app diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index f18ecb24e8..23b0e8e986 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -115,11 +115,8 @@ def inline_discussion(request, course_id, discussion_id): cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError): - # TODO (vshnayder): since none of this code seems to be aware of the fact that - # sometimes things go wrong, I suspect that the js client is also not - # checking for errors on request. Check and fix as needed. log.error("Error loading inline discussion threads.") - raise Http404 + raise annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) @@ -180,7 +177,7 @@ def forum_form_discussion(request, course_id): return render_to_response('discussion/maintenance.html', {}) except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: log.error("Error loading forum discussion threads: %s", str(err)) - raise Http404 + raise user = cc.User.from_django_user(request.user) user_info = user.to_dict() @@ -247,7 +244,7 @@ def single_thread(request, course_id, discussion_id, thread_id): thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id) except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError): log.error("Error loading single thread.") - raise Http404 + raise if request.is_ajax(): courseware_context = get_courseware_context(thread, course) @@ -272,7 +269,7 @@ def single_thread(request, course_id, discussion_id, thread_id): threads.append(thread.to_dict()) except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError): log.error("Error loading single thread.") - raise Http404 + raise course = get_course_with_access(request.user, course_id, 'load_forum') @@ -370,7 +367,7 @@ def user_profile(request, course_id, user_id): } return render_to_response('discussion/user_profile.html', context) - except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError, User.DoesNotExist): + except User.DoesNotExist: raise Http404 @@ -413,5 +410,5 @@ def followed_threads(request, course_id, user_id): } return render_to_response('discussion/user_profile.html', context) - except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError, User.DoesNotExist): + except User.DoesNotExist: raise Http404 diff --git a/lms/djangoapps/django_comment_client/middleware.py b/lms/djangoapps/django_comment_client/middleware.py index b9efc1589e..7876631d9e 100644 --- a/lms/djangoapps/django_comment_client/middleware.py +++ b/lms/djangoapps/django_comment_client/middleware.py @@ -18,7 +18,7 @@ class AjaxExceptionMiddleware(object): """ if isinstance(exception, CommentClientError) and request.is_ajax(): try: - return JsonError(json.loads(exception.message)) + return JsonError(json.loads(exception.message), 500) except ValueError: - return JsonError(exception.message) + return JsonError(exception.message, 500) return None diff --git a/lms/djangoapps/django_comment_client/tests/test_middleware.py b/lms/djangoapps/django_comment_client/tests/test_middleware.py index ab9517c160..54fcbd3c58 100644 --- a/lms/djangoapps/django_comment_client/tests/test_middleware.py +++ b/lms/djangoapps/django_comment_client/tests/test_middleware.py @@ -20,8 +20,14 @@ class AjaxExceptionTestCase(TestCase): self.request0.META['HTTP_X_REQUESTED_WITH'] = "SHADOWFAX" def test_process_exception(self): - self.assertIsInstance(self.a.process_exception(self.request1, self.exception1), middleware.JsonError) - self.assertIsInstance(self.a.process_exception(self.request1, self.exception2), middleware.JsonError) + response1 = self.a.process_exception(self.request1, self.exception1) + self.assertIsInstance(response1, middleware.JsonError) + self.assertEqual(500, response1.status_code) + + response2 = self.a.process_exception(self.request1, self.exception2) + self.assertIsInstance(response2, middleware.JsonError) + self.assertEqual(500, response2.status_code) + self.assertIsNone(self.a.process_exception(self.request1, self.exception0)) self.assertIsNone(self.a.process_exception(self.request0, self.exception1)) self.assertIsNone(self.a.process_exception(self.request0, self.exception0))