diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index fc720b91f2..c1bb20a096 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/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, } ) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 7f26ed51bd..63ad61840c 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/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"], } ) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 2e46ad7c8e..a04274d072 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/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"], } ) diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 2a624842e0..b50a3dd2bf 100644 --- a/lms/lib/comment_client/thread.py +++ b/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'):