From f5754f6d1e39c00b94d4d18b78f5276ac6d67fef Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 29 Aug 2012 13:30:55 -0400 Subject: [PATCH] Add some rudimentary error checking to forum views [Hopefully fix #34992235] --- .../django_comment_client/forum/views.py | 203 ++++++++++-------- lms/lib/comment_client/utils.py | 22 +- 2 files changed, 128 insertions(+), 97 deletions(-) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 62055f0ab7..ef6f70954e 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -1,6 +1,6 @@ from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_POST -from django.http import HttpResponse +from django.http import HttpResponse, Http404 from django.utils import simplejson from django.core.context_processors import csrf from django.core.urlresolvers import reverse @@ -30,7 +30,7 @@ def _general_discussion_id(course_id): def _should_perform_search(request): return bool(request.GET.get('text', False) or \ request.GET.get('tags', False)) - + def render_accordion(request, course, discussion_id): @@ -59,7 +59,7 @@ def render_discussion(request, course_id, threads, *args, **kwargs): }[discussion_type] base_url = { - 'inline': (lambda: reverse('django_comment_client.forum.views.inline_discussion', args=[course_id, discussion_id])), + 'inline': (lambda: reverse('django_comment_client.forum.views.inline_discussion', args=[course_id, discussion_id])), 'forum': (lambda: reverse('django_comment_client.forum.views.forum_form_discussion', args=[course_id])), 'user': (lambda: reverse('django_comment_client.forum.views.user_profile', args=[course_id, user_id])), }[discussion_type]() @@ -98,6 +98,10 @@ def render_user_discussion(*args, **kwargs): return render_discussion(discussion_type='user', *args, **kwargs) def get_threads(request, course_id, discussion_id=None): + """ + This may raise cc.utils.CommentClientError or + cc.utils.CommentClientUnknownError if something goes wrong. + """ default_query_params = { 'page': 1, @@ -122,10 +126,17 @@ def get_threads(request, course_id, discussion_id=None): # discussion per page is fixed for now def inline_discussion(request, course_id, discussion_id): - threads, query_params = get_threads(request, course_id, discussion_id) + try: + threads, query_params = get_threads(request, course_id, discussion_id) + except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: + # 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. + raise Http404 + html = render_inline_discussion(request, course_id, threads, discussion_id=discussion_id, \ query_params=query_params) - + return utils.JsonResponse({ 'html': html, 'discussion_data': map(utils.safe_content, threads), @@ -143,37 +154,41 @@ def render_search_bar(request, course_id, discussion_id=None, text=''): def forum_form_discussion(request, course_id): course = get_course_with_access(request.user, course_id, 'load') - threads, query_params = get_threads(request, course_id) - content = render_forum_discussion(request, course_id, threads, discussion_id=_general_discussion_id(course_id), query_params=query_params) + try: + threads, query_params = get_threads(request, course_id) + content = render_forum_discussion(request, course_id, threads, discussion_id=_general_discussion_id(course_id), query_params=query_params) - if request.is_ajax(): - return utils.JsonResponse({ - 'html': content, - 'discussion_data': map(utils.safe_content, threads), - }) - else: - recent_active_threads = cc.search_recent_active_threads( - course_id, - recursive=False, - query_params={'follower_id': request.user.id}, - ) + if request.is_ajax(): + return utils.JsonResponse({ + 'html': content, + 'discussion_data': map(utils.safe_content, threads), + }) + else: + recent_active_threads = cc.search_recent_active_threads( + course_id, + recursive=False, + query_params={'follower_id': request.user.id}, + ) + + trending_tags = cc.search_trending_tags( + course_id, + ) + context = { + 'csrf': csrf(request)['csrf_token'], + 'course': course, + 'content': content, + 'recent_active_threads': recent_active_threads, + 'trending_tags': trending_tags, + 'staff_access' : has_access(request.user, course, 'staff'), + } + # print "start rendering.." + return render_to_response('discussion/index.html', context) + except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: + raise Http404 - trending_tags = cc.search_trending_tags( - course_id, - ) - context = { - 'csrf': csrf(request)['csrf_token'], - 'course': course, - 'content': content, - 'recent_active_threads': recent_active_threads, - 'trending_tags': trending_tags, - 'staff_access' : has_access(request.user, course, 'staff'), - } - # print "start rendering.." - return render_to_response('discussion/index.html', context) def render_single_thread(request, discussion_id, course_id, thread_id): - + thread = cc.Thread.find(thread_id).retrieve(recursive=True).to_dict() user_info = cc.User.from_django_user(request.user).to_dict() @@ -192,75 +207,81 @@ def render_single_thread(request, discussion_id, course_id, thread_id): def single_thread(request, course_id, discussion_id, thread_id): - if request.is_ajax(): - - user_info = cc.User.from_django_user(request.user).to_dict() - thread = cc.Thread.find(thread_id).retrieve(recursive=True) - annotated_content_info = utils.get_annotated_content_infos(course_id, thread, request.user, user_info=user_info) - context = {'thread': thread.to_dict(), 'course_id': course_id} - html = render_to_string('discussion/_ajax_single_thread.html', context) + try: + if request.is_ajax(): - return utils.JsonResponse({ - 'html': html, - 'content': utils.safe_content(thread.to_dict()), - 'annotated_content_info': annotated_content_info, - }) + user_info = cc.User.from_django_user(request.user).to_dict() + thread = cc.Thread.find(thread_id).retrieve(recursive=True) + annotated_content_info = utils.get_annotated_content_infos(course_id, thread, request.user, user_info=user_info) + context = {'thread': thread.to_dict(), 'course_id': course_id} + html = render_to_string('discussion/_ajax_single_thread.html', context) - else: - course = get_course_with_access(request.user, course_id, 'load') + return utils.JsonResponse({ + 'html': html, + 'content': utils.safe_content(thread.to_dict()), + 'annotated_content_info': annotated_content_info, + }) - recent_active_threads = cc.search_recent_active_threads( - course_id, - recursive=False, - query_params={'follower_id': request.user.id}, - ) + else: + course = get_course_with_access(request.user, course_id, 'load') - trending_tags = cc.search_trending_tags( - course_id, - ) + recent_active_threads = cc.search_recent_active_threads( + course_id, + recursive=False, + query_params={'follower_id': request.user.id}, + ) - context = { - 'discussion_id': discussion_id, - 'csrf': csrf(request)['csrf_token'], - 'init': '', - 'content': render_single_thread(request, discussion_id, course_id, thread_id), - 'course': course, - 'recent_active_threads': recent_active_threads, - 'trending_tags': trending_tags, - 'course_id': course.id, - } + trending_tags = cc.search_trending_tags( + course_id, + ) - return render_to_response('discussion/single_thread.html', context) + context = { + 'discussion_id': discussion_id, + 'csrf': csrf(request)['csrf_token'], + 'init': '', + 'content': render_single_thread(request, discussion_id, course_id, thread_id), + 'course': course, + 'recent_active_threads': recent_active_threads, + 'trending_tags': trending_tags, + 'course_id': course.id, + } + + return render_to_response('discussion/single_thread.html', context) + except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: + raise Http404 def user_profile(request, course_id, user_id): course = get_course_with_access(request.user, course_id, 'load') - profiled_user = cc.User(id=user_id, course_id=course_id) + try: + profiled_user = cc.User(id=user_id, course_id=course_id) - query_params = { - 'page': request.GET.get('page', 1), - 'per_page': THREADS_PER_PAGE, # more than threads_per_page to show more activities - } - - threads, page, num_pages = profiled_user.active_threads(query_params) - - query_params['page'] = page - query_params['num_pages'] = num_pages - - content = render_user_discussion(request, course_id, threads, user_id=user_id, query_params=query_params) - - if request.is_ajax(): - return utils.JsonResponse({ - 'html': content, - 'discussion_data': map(utils.safe_content, threads), - }) - else: - context = { - 'course': course, - 'user': request.user, - 'django_user': User.objects.get(id=user_id), - 'profiled_user': profiled_user.to_dict(), - 'content': content, + query_params = { + 'page': request.GET.get('page', 1), + 'per_page': THREADS_PER_PAGE, # more than threads_per_page to show more activities } - return render_to_response('discussion/user_profile.html', context) + threads, page, num_pages = profiled_user.active_threads(query_params) + + query_params['page'] = page + query_params['num_pages'] = num_pages + + content = render_user_discussion(request, course_id, threads, user_id=user_id, query_params=query_params) + + if request.is_ajax(): + return utils.JsonResponse({ + 'html': content, + 'discussion_data': map(utils.safe_content, threads), + }) + else: + context = { + 'course': course, + 'user': request.user, + 'django_user': User.objects.get(id=user_id), + 'profiled_user': profiled_user.to_dict(), + 'content': content, + } + + return render_to_response('discussion/user_profile.html', context) + except (cc.utils.CommentClientError, cc.utils.CommentClientUnknownError) as err: + raise Http404 diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 77e411424a..417d4eda8c 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -1,7 +1,10 @@ -import requests import json +import logging +import requests import settings +log = logging.getLogger('mitx.' + __name__) + def strip_none(dic): return dict([(k, v) for k, v in dic.iteritems() if v is not None]) @@ -18,15 +21,22 @@ def extract(dic, keys): def merge_dict(dic1, dic2): return dict(dic1.items() + dic2.items()) - + def perform_request(method, url, data_or_params=None, *args, **kwargs): if data_or_params is None: data_or_params = {} data_or_params['api_key'] = settings.API_KEY - if method in ['post', 'put', 'patch']: - response = requests.request(method, url, data=data_or_params) - else: - response = requests.request(method, url, params=data_or_params) + try: + if method in ['post', 'put', 'patch']: + response = requests.request(method, url, data=data_or_params) + else: + response = requests.request(method, url, params=data_or_params) + 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 200 < response.status_code < 500: raise CommentClientError(response.text) elif response.status_code == 500: