Improve performance of forum views

Avoid recomputing course module information for every thread, which
should dramatically improve the performance of high-percentile latency
queries.

JIRA: FOR-250

lms/djangoapps/django_comment_client/base/views.py

@@ -23,7 +23,7 @@ from mitxmako.shortcuts import render_to_string
 from courseware.courses import get_course_with_access, get_course_by_id
 from course_groups.cohorts import get_cohort_id, is_commentable_cohorted
 
-from django_comment_client.utils import JsonResponse, JsonError, extract, get_courseware_context
+from django_comment_client.utils import JsonResponse, JsonError, extract, add_courseware_context
 
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
 from django_comment_common.models import Role
@@ -128,10 +128,8 @@ def create_thread(request, course_id, commentable_id):
     if post.get('auto_subscribe', 'false').lower() == 'true':
         user = cc.User.from_django_user(request.user)
         user.follow(thread)
-    courseware_context = get_courseware_context(thread, course)
     data = thread.to_dict()
-    if courseware_context:
-        data.update(courseware_context)
+    add_courseware_context([data], course)
     if request.is_ajax():
         return ajax_content_response(request, course_id, data, 'discussion/ajax_create_thread.html')
     else:

lms/djangoapps/django_comment_client/forum/views.py

@@ -15,7 +15,7 @@ from course_groups.cohorts import (is_course_cohorted, get_cohort_id, is_comment
 from courseware.access import has_access
 
 from django_comment_client.permissions import cached_has_permission
-from django_comment_client.utils import (merge_dict, extract, strip_none, get_courseware_context)
+from django_comment_client.utils import (merge_dict, extract, strip_none, add_courseware_context)
 import django_comment_client.utils as utils
 import comment_client as cc
 
@@ -184,11 +184,8 @@ def forum_form_discussion(request, course_id):
     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)
 
-    with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context_to_threads"):
-        for thread in threads:
-            courseware_context = get_courseware_context(thread, course)
-            if courseware_context:
-                thread.update(courseware_context)
+    with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
+        add_courseware_context(threads, course)
 
     if request.is_ajax():
         return utils.JsonResponse({
@@ -248,16 +245,14 @@ def single_thread(request, course_id, discussion_id, thread_id):
     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"):
-            courseware_context = get_courseware_context(thread, course)
         with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"):
             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}
         # TODO: Remove completely or switch back to server side rendering
         # html = render_to_string('discussion/_ajax_single_thread.html', context)
         content = utils.safe_content(thread.to_dict())
-        if courseware_context:
-            content.update(courseware_context)
+        with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
+            add_courseware_context([content], course)
         return utils.JsonResponse({
             #'html': html,
             'content': content,
@@ -273,17 +268,16 @@ def single_thread(request, course_id, discussion_id, thread_id):
 
         course = get_course_with_access(request.user, course_id, 'load_forum')
 
-        with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context_to_threads"):
-            for thread in threads:
-                courseware_context = get_courseware_context(thread, course)
-                if courseware_context:
-                    thread.update(courseware_context)
-                if thread.get('group_id') and not thread.get('group_name'):
-                    thread['group_name'] = get_cohort_by_id(course_id, thread.get('group_id')).name
+        with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
+            add_courseware_context(threads, course)
 
-                #patch for backward compatibility with comments service
-                if not "pinned" in thread:
-                    thread["pinned"] = False
+        for thread in threads:
+            if thread.get('group_id') and not thread.get('group_name'):
+                thread['group_name'] = get_cohort_by_id(course_id, thread.get('group_id')).name
+
+            #patch for backward compatibility with comments service
+            if not "pinned" in thread:
+                thread["pinned"] = False
 
         threads = [utils.safe_content(thread) for thread in threads]
 

lms/djangoapps/django_comment_client/tests/test_utils.py

@@ -83,17 +83,29 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
             discussion_target="Discussion 2"
         )
 
+    def test_empty(self):
+        utils.add_courseware_context([], self.course)
+
     def test_missing_commentable_id(self):
-        thread = {"commentable_id": "non-inline"}
-        self.assertEqual(utils.get_courseware_context(thread, self.course), None)
+        orig = {"commentable_id": "non-inline"}
+        modified = dict(orig)
+        utils.add_courseware_context([modified], self.course)
+        self.assertEqual(modified, orig)
 
     def test_basic(self):
-        def test_discussion(discussion, expected_title):
-            thread = {"commentable_id": discussion.discussion_id}
-            courseware_context = utils.get_courseware_context(thread, self.course)
-            self.assertEqual(set(courseware_context.keys()), set(["courseware_url", "courseware_title"]))
+        threads = [
+            {"commentable_id": self.discussion1.discussion_id},
+            {"commentable_id": self.discussion2.discussion_id}
+        ]
+        utils.add_courseware_context(threads, self.course)
+
+        def assertThreadCorrect(thread, discussion, expected_title):  #pylint: disable=C0103
             self.assertEqual(
-                courseware_context["courseware_url"],
+                set(thread.keys()),
+                set(["commentable_id", "courseware_url", "courseware_title"])
+            )
+            self.assertEqual(
+                thread.get("courseware_url"),
                 reverse(
                     "jump_to",
                     kwargs={
@@ -102,13 +114,10 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
                     }
                 )
             )
-            self.assertEqual(
-                courseware_context["courseware_title"],
-                expected_title
-            )
+            self.assertEqual(thread.get("courseware_title"), expected_title)
 
-        test_discussion(self.discussion1, "Chapter / Discussion 1")
-        test_discussion(self.discussion2, "Subsection / Discussion 2")
+        assertThreadCorrect(threads[0], self.discussion1, "Chapter / Discussion 1")
+        assertThreadCorrect(threads[1], self.discussion2, "Subsection / Discussion 2")
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)

lms/djangoapps/django_comment_client/utils.py

@@ -347,19 +347,19 @@ def extend_content(content):
     return merge_dict(content, content_info)
 
 
-def get_courseware_context(content, course):
+def add_courseware_context(content_list, course):
     id_map = _get_discussion_id_map(course)
-    id = content['commentable_id']
-    content_info = None
-    if id in id_map:
-        location = id_map[id]["location"].url()
-        title = id_map[id]["title"]
 
-        url = reverse('jump_to', kwargs={"course_id": course.location.course_id,
-                      "location": location})
+    for content in content_list:
+        commentable_id = content['commentable_id']
+        if commentable_id in id_map:
+            location = id_map[commentable_id]["location"].url()
+            title = id_map[commentable_id]["title"]
 
-        content_info = {"courseware_url": url, "courseware_title": title}
-    return content_info
+            url = reverse('jump_to', kwargs={"course_id": course.location.course_id,
+                          "location": location})
+
+            content.update({"courseware_url": url, "courseware_title": title})
 
 
 def safe_content(content):