Merge pull request #1456 from MITx/fix/cdodge/dangling-discussion-modules

Fix/cdodge/dangling discussion modules

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
 
@@ -164,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'):
@@ -171,6 +174,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:
+            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
+
         if skip_module:
             continue
 
@@ -235,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):
@@ -390,11 +402,23 @@ 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)
-        url = reverse('courseware_position', kwargs={"course_id": course_id,
-                                                     "chapter": chapter,
-                                                     "section": section,
-                                                     "position": position})
+
+        # cdodge: did we pre-compute, if so, then let's use that rather than recomputing
+        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:
+            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,
+                                                     "section":section,
+                                                     "position":position})
+
         content_info = {"courseware_url": url, "courseware_title": title}
     return content_info