Merge pull request #13436 from edx/robrap/TNL-5457

TNL-5457: Performance enhancements for Discussions API.

lms/djangoapps/discussion_api/api.py

@@ -96,7 +96,8 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None):
     """
     retrieve_kwargs = retrieve_kwargs or {}
     try:
-        retrieve_kwargs["with_responses"] = True
+        if "with_responses" not in retrieve_kwargs:
+            retrieve_kwargs["with_responses"] = False
         if "mark_as_read" not in retrieve_kwargs:
             retrieve_kwargs["mark_as_read"] = False
         cc_thread = Thread(id=thread_id).retrieve(**retrieve_kwargs)
@@ -617,6 +618,7 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, requested_fi
         request,
         thread_id,
         retrieve_kwargs={
+            "with_responses": True,
             "recursive": False,
             "user_id": request.user.id,
             "response_skip": response_skip,
@@ -975,10 +977,15 @@ def get_thread(request, thread_id, requested_fields=None):
         requested_fields: Indicates which additional fields to return for
         thread. (i.e. ['profile_image'])
     """
+    # Possible candidate for optimization with caching:
+    #   Param with_responses=True required only to add "response_count" to response.
     cc_thread, context = _get_thread_and_context(
         request,
         thread_id,
-        retrieve_kwargs={"user_id": unicode(request.user.id)}
+        retrieve_kwargs={
+            "with_responses": True,
+            "user_id": unicode(request.user.id),
+        }
     )
     return _serialize_discussion_entities(request, context, [cc_thread], requested_fields, DiscussionEntity.thread)[0]
 
@@ -1012,6 +1019,7 @@ def get_response_comments(request, comment_id, page, page_size, requested_fields
             request,
             cc_comment["thread_id"],
             retrieve_kwargs={
+                "with_responses": True,
                 "recursive": True,
             }
         )

lms/djangoapps/discussion_api/tests/test_api.py

@@ -630,8 +630,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["1"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
             "commentable_ids": ["topic_x,topic_meow"]
         })
 
@@ -644,8 +642,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "sort_order": ["desc"],
             "page": ["6"],
             "per_page": ["14"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
         })
 
     def test_thread_content(self):
@@ -858,8 +854,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
             "text": ["test search string"],
         })
 
@@ -924,9 +918,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["11"],
-            "recursive": ["False"],
             query: ["true"],
-            "with_responses": ["True"],
         })
 
     @ddt.data(
@@ -968,8 +960,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["11"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
         })
 
     @ddt.data("asc", "desc")
@@ -999,8 +989,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "sort_order": [http_query],
             "page": ["1"],
             "per_page": ["11"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
         })
 
 
@@ -1182,12 +1170,12 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase):
         self.assert_query_params_equal(
             httpretty.httpretty.latest_requests[-2],
             {
-                "recursive": ["False"],
                 "user_id": [str(self.user.id)],
                 "mark_as_read": ["False"],
+                "recursive": ["False"],
                 "resp_skip": ["70"],
                 "resp_limit": ["14"],
-                "with_responses": ["True"]
+                "with_responses": ["True"],
             }
         )
 

lms/djangoapps/discussion_api/tests/test_views.py

@@ -431,8 +431,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
         })
 
     @ddt.data("unread", "unanswered")
@@ -452,8 +450,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
             "sort_order": ["desc"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
             "page": ["1"],
             "per_page": ["10"],
             query: ["true"],
@@ -478,8 +474,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "sort_order": ["desc"],
             "page": ["18"],
             "per_page": ["4"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
         })
 
     def test_text_search(self):
@@ -506,8 +500,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
-            "recursive": ["False"],
-            "with_responses": ["True"],
             "text": ["test search string"],
         })
 
@@ -598,11 +590,9 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_order": ["desc"],
-            "recursive": ["False"],
             "page": ["1"],
             "per_page": ["10"],
             "sort_key": [cc_query],
-            "with_responses": ["True"],
         })
 
     @ddt.data("asc", "desc")
@@ -621,11 +611,9 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "recursive": ["False"],
             "page": ["1"],
             "per_page": ["10"],
             "sort_order": [query],
-            "with_responses": ["True"],
         })
 
     def test_mutually_exclusive(self):
@@ -1131,11 +1119,11 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pr
         self.assert_query_params_equal(
             httpretty.httpretty.latest_requests[-2],
             {
-                "recursive": ["False"],
                 "resp_skip": ["0"],
                 "resp_limit": ["10"],
                 "user_id": [str(self.user.id)],
                 "mark_as_read": ["False"],
+                "recursive": ["False"],
                 "with_responses": ["True"],
             }
         )
@@ -1165,11 +1153,11 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pr
         self.assert_query_params_equal(
             httpretty.httpretty.latest_requests[-2],
             {
-                "recursive": ["False"],
                 "resp_skip": ["68"],
                 "resp_limit": ["4"],
                 "user_id": [str(self.user.id)],
                 "mark_as_read": ["False"],
+                "recursive": ["False"],
                 "with_responses": ["True"],
             }
         )

lms/lib/comment_client/thread.py

@@ -45,11 +45,13 @@ class Thread(models.Model):
     @classmethod
     def search(cls, query_params):
 
+        # NOTE: Params 'recursive' and 'with_responses' are currently not used by
+        # either the 'search' or 'get_all' actions below.  Both already use
+        # with_responses=False internally in the comment service, so no additional
+        # optimization is required.
         default_params = {'page': 1,
                           'per_page': 20,
-                          'course_id': query_params['course_id'],
-                          'recursive': False,
-                          'with_responses': True}
+                          'course_id': query_params['course_id']}
         params = merge_dict(default_params, strip_blank(strip_none(query_params)))
 
         if query_params.get('text'):