Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Improve forum error handling

Browse Source 
CommentClientError now has sane subclasses that are meaningfully
distinct, and each subclass is handled appropriately. Errors raised by
the requests library are no longer handled by turning them into
CommentClientErrors, since there is no meaningful handling we can do,
and this way we will get more visibility into why errors are occurring.
Also, HTTP status codes from the comments service indicating client
error are correctly passed through to the client.
This commit is contained in:
Greg Price
2013-10-24 18:40:37 -04:00
parent d8444266d7
commit 7abaecd8b7
10 changed files with 78 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
CHANGELOG.rst
View File

@@ -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)

31
lms/djangoapps/django_comment_client/forum/views.py
View File

@@ -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')

12
lms/djangoapps/django_comment_client/middleware.py
View File

@@ -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

24
lms/djangoapps/django_comment_client/tests/test_middleware.py
View File

@@ -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))

5
lms/lib/comment_client/__init__.py
View File

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

6
lms/lib/comment_client/comment.py
View File

@@ -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:

4
lms/lib/comment_client/models.py
View File

@@ -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()

6
lms/lib/comment_client/thread.py
View File

@@ -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:

10
lms/lib/comment_client/user.py
View File

@@ -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)

55
lms/lib/comment_client/utils.py
View File

@@ -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