diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1318a13155..a4b5d31fea 100644 --- a/CHANGELOG.rst +++ b/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) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 7281e779d5..f9cb2b2713 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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') diff --git a/lms/djangoapps/django_comment_client/middleware.py b/lms/djangoapps/django_comment_client/middleware.py index 7876631d9e..3a4031f871 100644 --- a/lms/djangoapps/django_comment_client/middleware.py +++ b/lms/djangoapps/django_comment_client/middleware.py @@ -1,4 +1,4 @@ -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 diff --git a/lms/djangoapps/django_comment_client/tests/test_middleware.py b/lms/djangoapps/django_comment_client/tests/test_middleware.py index 54fcbd3c58..6b21fe056d 100644 --- a/lms/djangoapps/django_comment_client/tests/test_middleware.py +++ b/lms/djangoapps/django_comment_client/tests/test_middleware.py @@ -1,32 +1,38 @@ +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)) diff --git a/lms/lib/comment_client/__init__.py b/lms/lib/comment_client/__init__.py index 6228c32024..bd8187ecfc 100644 --- a/lms/lib/comment_client/__init__.py +++ b/lms/lib/comment_client/__init__.py @@ -1,2 +1,5 @@ from .comment_client import * -from .utils import CommentClientError, CommentClientUnknownError +from .utils import ( + CommentClientError, CommentClientRequestError, + CommentClient500Error, CommentClientMaintenanceError +) diff --git a/lms/lib/comment_client/comment.py b/lms/lib/comment_client/comment.py index fd68d5cdeb..2b0d77b15f 100644 --- a/lms/lib/comment_client/comment.py +++ b/lms/lib/comment_client/comment.py @@ -1,4 +1,4 @@ -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: diff --git a/lms/lib/comment_client/models.py b/lms/lib/comment_client/models.py index 97734c36e8..7296151c80 100644 --- a/lms/lib/comment_client/models.py +++ b/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() diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 00d5f01814..5cc14aa18f 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -1,5 +1,5 @@ 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: diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 1af64cc18c..014fe0a810 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -1,4 +1,4 @@ -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) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index aef4476406..c24dac8121 100644 --- a/lms/lib/comment_client/utils.py +++ b/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 CommentClientRequestError(CommentClientError): + def __init__(self, msg, status_code=400): + super(CommentClientRequestError, self).__init__(msg) + self.status_code = status_code + + +class CommentClient500Error(CommentClientError): + pass + + class CommentClientMaintenanceError(CommentClientError): pass - - -class CommentClientUnknownError(CommentClientError): - pass