Skip to content

E
edx-platform
Commit bced90d2 by Robert Raposa
Options

Performance enhancements for Discussions API. 

- Default to with_responses=False, except when required.
- Clean up search call unused params.
parent 500bac6f
Showing with 19 additions and 33 deletions
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'):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment