From 6ed2737c36b4405169569c0eaedbf510508dbafe Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 6 Feb 2013 13:29:48 -0500 Subject: [PATCH 1/3] make LMS forum subsystem more robust in case of orphaned discussion modules. Given our draft/non-draft duality, we don't currently have a means to always do proper housekeeping at this point in time. However, we have to stop the LMS Forums from blowing up when encoutering one of these. --- lms/djangoapps/django_comment_client/utils.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index b58e3b30e6..40fd106b40 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -11,6 +11,8 @@ from django.http import HttpResponse from django.utils import simplejson from django_comment_client.models import Role from django_comment_client.permissions import check_permissions_by_view +from xmodule.modulestore.exceptions import NoPathToItem + from mitxmako import middleware import pystache_custom as pystache @@ -158,6 +160,14 @@ def initialize_discussion_info(course): log.warning("Required key '%s' not in discussion %s, leaving out of category map" % (key, module.location)) skip_module = True + # cdodge: pre-compute the path_to_location. Note this can throw an exception for any + # dangling discussion modules + try: + _DISCUSSIONINFO[course.id]['path_to_location'] = path_to_location(modulestore(), course.id, module.location) + except NoPathToItem: + log.warning("Could not compute path_to_location for {0}. Perhaps this is an orphaned discussion module?!? Skipping...".format(module.location)) + skip_module = True + if skip_module: continue @@ -360,7 +370,13 @@ def get_courseware_context(content, course): if id in id_map: location = id_map[id]["location"].url() title = id_map[id]["title"] - (course_id, chapter, section, position) = path_to_location(modulestore(), course.id, location) + + # cdodge: did we pre-compute, if so, then let's use that rather than recomputing + if 'path_to_location' in _DISCUSSIONINFO[course.id]: + (course_id, chapter, section, position) = _DISCUSSIONINFO[course.id]['path_to_location'] + else: + (course_id, chapter, section, position) = path_to_location(modulestore(), course.id, location) + url = reverse('courseware_position', kwargs={"course_id":course_id, "chapter":chapter, "section":section, From 114d800c6a6a6650c49c4cddee40d7866c5e442e Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 6 Feb 2013 15:17:47 -0500 Subject: [PATCH 2/3] need to make path_to_location a dictionary since it needs to be keyed by the location --- lms/djangoapps/django_comment_client/utils.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 0940d065f4..151bde3dd5 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -166,6 +166,7 @@ def initialize_discussion_info(course): # get all discussion models within this course_id all_modules = modulestore().get_items(['i4x', course.location.org, course.location.course, 'discussion', None], course_id=course_id) + path_to_locations = {} for module in all_modules: skip_module = False for key in ('id', 'discussion_category', 'for'): @@ -176,7 +177,7 @@ def initialize_discussion_info(course): # cdodge: pre-compute the path_to_location. Note this can throw an exception for any # dangling discussion modules try: - _DISCUSSIONINFO[course.id]['path_to_location'] = path_to_location(modulestore(), course.id, module.location) + path_to_locations[module.location] = path_to_location(modulestore(), course.id, module.location) except NoPathToItem: log.warning("Could not compute path_to_location for {0}. Perhaps this is an orphaned discussion module?!? Skipping...".format(module.location)) skip_module = True @@ -245,6 +246,7 @@ def initialize_discussion_info(course): _DISCUSSIONINFO[course.id]['id_map'] = discussion_id_map _DISCUSSIONINFO[course.id]['category_map'] = category_map _DISCUSSIONINFO[course.id]['timestamp'] = datetime.now() + _DISCUSSIONINFO[course.id]['path_to_location'] = path_to_locations class JsonResponse(HttpResponse): @@ -402,8 +404,8 @@ def get_courseware_context(content, course): title = id_map[id]["title"] # cdodge: did we pre-compute, if so, then let's use that rather than recomputing - if 'path_to_location' in _DISCUSSIONINFO[course.id]: - (course_id, chapter, section, position) = _DISCUSSIONINFO[course.id]['path_to_location'] + if 'path_to_location' in _DISCUSSIONINFO[course.id] and location in _DISCUSSIONINFO[course.id]['path_to_location']: + (course_id, chapter, section, position) = _DISCUSSIONINFO[course.id]['path_to_location'][location] else: (course_id, chapter, section, position) = path_to_location(modulestore(), course.id, location) From faf7c64ea50d7f96fe2645d09abacbd5115fd040 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 7 Feb 2013 10:32:13 -0500 Subject: [PATCH 3/3] add try/catch and fallback to returning a path to the root of the course --- lms/djangoapps/django_comment_client/utils.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 151bde3dd5..877c730539 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -407,7 +407,12 @@ def get_courseware_context(content, course): if 'path_to_location' in _DISCUSSIONINFO[course.id] and location in _DISCUSSIONINFO[course.id]['path_to_location']: (course_id, chapter, section, position) = _DISCUSSIONINFO[course.id]['path_to_location'][location] else: - (course_id, chapter, section, position) = path_to_location(modulestore(), course.id, location) + try: + (course_id, chapter, section, position) = path_to_location(modulestore(), course.id, location) + except NoPathToItem: + # Object is not in the graph any longer, let's just get path to the base of the course + # so that we can at least return something to the caller + (course_id, chapter, section, position) = path_to_location(modulestore(), course.id, course.location) url = reverse('courseware_position', kwargs={"course_id":course_id, "chapter":chapter,