Merge pull request #1496 from edx/gprice/comment-client-error-sanity

Improve forum error handling

CHANGELOG.rst

@@ -5,6 +5,10 @@ These are notable changes in edx-platform.  This is a rolling list of changes,
 in roughly chronological order, most recent first.  Add your entries at or near
 the top.  Include a label indicating the component affected.
 
+LMS: Improve forum error handling so that errors in the logs are
+clearer and HTTP status codes from the comments service indicating
+client error are correctly passed through to the client.
+
 LMS: The wiki markup cheatsheet dialog is now accessible to people with
 disabilites.  (LMS-1303)
 

lms/djangoapps/django_comment_client/forum/views.py

@@ -29,8 +29,8 @@ log = logging.getLogger("edx.discussions")
 @newrelic.agent.function_trace()
 def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAGE):
     """
-    This may raise cc.utils.CommentClientError or
-    cc.utils.CommentClientUnknownError if something goes wrong.
+    This may raise an appropriate subclass of cc.utils.CommentClientError
+    if something goes wrong.
     """
     default_query_params = {
         'page': 1,
@@ -113,13 +113,9 @@ def inline_discussion(request, course_id, discussion_id):
 
     course = get_course_with_access(request.user, course_id, 'load_forum')
 
-    try:
-        threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
-        cc_user = cc.User.from_django_user(request.user)
-        user_info = cc_user.to_dict()
-    except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError):
-        log.error("Error loading inline discussion threads.")
-        raise
+    threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
+    cc_user = cc.User.from_django_user(request.user)
+    user_info = cc_user.to_dict()
 
     with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"):
         annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info)
@@ -181,9 +177,6 @@ def forum_form_discussion(request, course_id):
     except cc.utils.CommentClientMaintenanceError:
         log.warning("Forum is in maintenance mode")
         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
 
     user = cc.User.from_django_user(request.user)
     user_info = user.to_dict()
@@ -252,11 +245,7 @@ def single_thread(request, course_id, discussion_id, thread_id):
     cc_user = cc.User.from_django_user(request.user)
     user_info = cc_user.to_dict()
 
-    try:
-        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
+    thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id)
 
     if request.is_ajax():
         with newrelic.agent.FunctionTrace(nr_transaction, "get_courseware_context"):
@@ -279,12 +268,8 @@ def single_thread(request, course_id, discussion_id, thread_id):
         with newrelic.agent.FunctionTrace(nr_transaction, "get_discussion_category_map"):
             category_map = utils.get_discussion_category_map(course)
 
-        try:
-            threads, query_params = get_threads(request, course_id)
-            threads.append(thread.to_dict())
-        except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError):
-            log.error("Error loading single thread.")
-            raise
+        threads, query_params = get_threads(request, course_id)
+        threads.append(thread.to_dict())
 
         course = get_course_with_access(request.user, course_id, 'load_forum')
 

lms/djangoapps/django_comment_client/middleware.py

-from comment_client import CommentClientError
+from comment_client import CommentClientRequestError
 from django_comment_client.utils import JsonError
 import json
 import logging
@@ -8,17 +8,17 @@ log = logging.getLogger(__name__)
 
 class AjaxExceptionMiddleware(object):
     """
-    Middleware that captures CommentClientErrors during ajax requests
+    Middleware that captures CommentClientRequestErrors during ajax requests
     and tranforms them into json responses
     """
     def process_exception(self, request, exception):
         """
-        Processes CommentClientErrors in ajax requests. If the request is an ajax request,
+        Processes CommentClientRequestErrors in ajax requests. If the request is an ajax request,
         returns a http response that encodes the error as json
         """
-        if isinstance(exception, CommentClientError) and request.is_ajax():
+        if isinstance(exception, CommentClientRequestError) and request.is_ajax():
             try:
-                return JsonError(json.loads(exception.message), 500)
+                return JsonError(json.loads(exception.message), exception.status_code)
             except ValueError:
-                return JsonError(exception.message, 500)
+                return JsonError(exception.message, exception.status_code)
         return None

lms/djangoapps/django_comment_client/tests/test_middleware.py

+import django.http
 from django.test import TestCase
+import json
 
 import comment_client
-import django.http
 import django_comment_client.middleware as middleware
 
 
 class AjaxExceptionTestCase(TestCase):
-
-# TODO: check whether the correct error message is produced.
-# The error message should be the same as the argument to CommentClientError
     def setUp(self):
         self.a = middleware.AjaxExceptionMiddleware()
         self.request1 = django.http.HttpRequest()
         self.request0 = django.http.HttpRequest()
-        self.exception1 = comment_client.CommentClientError('{}')
-        self.exception2 = comment_client.CommentClientError('Foo!')
-        self.exception0 = ValueError()
+        self.exception1 = comment_client.CommentClientRequestError('{}', 401)
+        self.exception2 = comment_client.CommentClientRequestError('Foo!', 404)
+        self.exception0 = comment_client.CommentClient500Error("Holy crap the server broke!")
         self.request1.META['HTTP_X_REQUESTED_WITH'] = "XMLHttpRequest"
         self.request0.META['HTTP_X_REQUESTED_WITH'] = "SHADOWFAX"
 
     def test_process_exception(self):
         response1 = self.a.process_exception(self.request1, self.exception1)
         self.assertIsInstance(response1, middleware.JsonError)
-        self.assertEqual(500, response1.status_code)
+        self.assertEqual(self.exception1.status_code, response1.status_code)
+        self.assertEqual(
+            {"errors": json.loads(self.exception1.message)},
+            json.loads(response1.content)
+        )
 
         response2 = self.a.process_exception(self.request1, self.exception2)
         self.assertIsInstance(response2, middleware.JsonError)
-        self.assertEqual(500, response2.status_code)
+        self.assertEqual(self.exception2.status_code, response2.status_code)
+        self.assertEqual(
+            {"errors": [self.exception2.message]},
+            json.loads(response2.content)
+        )
 
         self.assertIsNone(self.a.process_exception(self.request1, self.exception0))
         self.assertIsNone(self.a.process_exception(self.request0, self.exception1))

lms/lib/comment_client/__init__.py

 from .comment_client import *
-from .utils import CommentClientError, CommentClientUnknownError
+from .utils import (
+    CommentClientError, CommentClientRequestError,
+    CommentClient500Error, CommentClientMaintenanceError
+)

lms/lib/comment_client/comment.py

-from .utils import CommentClientError, perform_request
+from .utils import CommentClientRequestError, perform_request
 
 from .thread import Thread, _url_for_flag_abuse_thread, _url_for_unflag_abuse_thread
 import models
@@ -48,7 +48,7 @@ class Comment(models.Model):
         elif voteable.type == 'comment':
             url = _url_for_flag_abuse_comment(voteable.id)
         else:
-            raise CommentClientError("Can only flag/unflag threads or comments")
+            raise CommentClientRequestError("Can only flag/unflag threads or comments")
         params = {'user_id': user.id}
         request = perform_request('put', url, params)
         voteable.update_attributes(request)
@@ -59,7 +59,7 @@ class Comment(models.Model):
         elif voteable.type == 'comment':
             url = _url_for_unflag_abuse_comment(voteable.id)
         else:
-            raise CommentClientError("Can flag/unflag for threads or comments")
+            raise CommentClientRequestError("Can flag/unflag for threads or comments")
         params = {'user_id': user.id}
 
         if removeAll:

lms/lib/comment_client/models.py

@@ -119,13 +119,13 @@ class Model(object):
     @classmethod
     def url(cls, action, params={}):
         if cls.base_url is None:
-            raise CommentClientError("Must provide base_url when using default url function")
+            raise CommentClientRequestError("Must provide base_url when using default url function")
         if action not in cls.DEFAULT_ACTIONS:
             raise ValueError("Invalid action {0}. The supported action must be in {1}".format(action, str(cls.DEFAULT_ACTIONS)))
         elif action in cls.DEFAULT_ACTIONS_WITH_ID:
             try:
                 return cls.url_with_id(params)
             except KeyError:
-                raise CommentClientError("Cannot perform action {0} without id".format(action))
+                raise CommentClientRequestError("Cannot perform action {0} without id".format(action))
         else:   # action must be in DEFAULT_ACTIONS_WITHOUT_ID now
             return cls.url_without_id()

lms/lib/comment_client/thread.py

 from .utils import merge_dict, strip_blank, strip_none, extract, perform_request
-from .utils import CommentClientError
+from .utils import CommentClientRequestError
 import models
 import settings
 
@@ -88,7 +88,7 @@ class Thread(models.Model):
         elif voteable.type == 'comment':
             url = _url_for_flag_comment(voteable.id)
         else:
-            raise CommentClientError("Can only flag/unflag threads or comments")
+            raise CommentClientRequestError("Can only flag/unflag threads or comments")
         params = {'user_id': user.id}
         request = perform_request('put', url, params)
         voteable.update_attributes(request)
@@ -99,7 +99,7 @@ class Thread(models.Model):
         elif voteable.type == 'comment':
             url = _url_for_unflag_comment(voteable.id)
         else:
-            raise CommentClientError("Can only flag/unflag for threads or comments")
+            raise CommentClientRequestError("Can only flag/unflag for threads or comments")
         params = {'user_id': user.id}
         #if you're an admin, when you unflag, remove ALL flags
         if removeAll:

lms/lib/comment_client/user.py

-from .utils import merge_dict, perform_request, CommentClientError
+from .utils import merge_dict, perform_request, CommentClientRequestError
 
 import models
 import settings
@@ -41,7 +41,7 @@ class User(models.Model):
         elif voteable.type == 'comment':
             url = _url_for_vote_comment(voteable.id)
         else:
-            raise CommentClientError("Can only vote / unvote for threads or comments")
+            raise CommentClientRequestError("Can only vote / unvote for threads or comments")
         params = {'user_id': self.id, 'value': value}
         request = perform_request('put', url, params)
         voteable.update_attributes(request)
@@ -52,14 +52,14 @@ class User(models.Model):
         elif voteable.type == 'comment':
             url = _url_for_vote_comment(voteable.id)
         else:
-            raise CommentClientError("Can only vote / unvote for threads or comments")
+            raise CommentClientRequestError("Can only vote / unvote for threads or comments")
         params = {'user_id': self.id}
         request = perform_request('delete', url, params)
         voteable.update_attributes(request)
 
     def active_threads(self, query_params={}):
         if not self.course_id:
-            raise CommentClientError("Must provide course_id when retrieving active threads for the user")
+            raise CommentClientRequestError("Must provide course_id when retrieving active threads for the user")
         url = _url_for_user_active_threads(self.id)
         params = {'course_id': self.course_id}
         params = merge_dict(params, query_params)
@@ -68,7 +68,7 @@ class User(models.Model):
 
     def subscribed_threads(self, query_params={}):
         if not self.course_id:
-            raise CommentClientError("Must provide course_id when retrieving subscribed threads for the user")
+            raise CommentClientRequestError("Must provide course_id when retrieving subscribed threads for the user")
         url = _url_for_user_subscribed_threads(self.id)
         params = {'course_id': self.course_id}
         params = merge_dict(params, query_params)

lms/lib/comment_client/utils.py

@@ -55,35 +55,30 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs):
     headers = {'X-Edx-Api-Key': settings.API_KEY}
     request_id = uuid4()
     request_id_dict = {'request_id': request_id}
-    try:
-        if method in ['post', 'put', 'patch']:
-            data = data_or_params
-            params = request_id_dict
-        else:
-            data = None
-            params = merge_dict(data_or_params, request_id_dict)
-        with request_timer(request_id, method, url):
-            response = requests.request(
-                method,
-                url,
-                data=data,
-                params=params,
-                headers=headers,
-                timeout=5
-            )
-    except Exception as err:
-        log.exception("Trying to call {method} on {url} with params {params}".format(
-            method=method, url=url, params=data_or_params))
-        # Reraise with a single exception type
-        raise CommentClientError(str(err))
+
+    if method in ['post', 'put', 'patch']:
+        data = data_or_params
+        params = request_id_dict
+    else:
+        data = None
+        params = merge_dict(data_or_params, request_id_dict)
+    with request_timer(request_id, method, url):
+        response = requests.request(
+            method,
+            url,
+            data=data,
+            params=params,
+            headers=headers,
+            timeout=5
+        )
 
     if 200 < response.status_code < 500:
-        raise CommentClientError(response.text)
+        raise CommentClientRequestError(response.text, response.status_code)
     # Heroku returns a 503 when an application is in maintenance mode
     elif response.status_code == 503:
         raise CommentClientMaintenanceError(response.text)
     elif response.status_code == 500:
-        raise CommentClientUnknownError(response.text)
+        raise CommentClient500Error(response.text)
     else:
         if kwargs.get("raw", False):
             return response.text
@@ -99,9 +94,15 @@ class CommentClientError(Exception):
         return repr(self.message)
 
 
-class CommentClientMaintenanceError(CommentClientError):
+class CommentClientRequestError(CommentClientError):
+    def __init__(self, msg, status_code=400):
+        super(CommentClientRequestError, self).__init__(msg)
+        self.status_code = status_code
+
+
+class CommentClient500Error(CommentClientError):
     pass
 
 
-class CommentClientUnknownError(CommentClientError):
+class CommentClientMaintenanceError(CommentClientError):
     pass