diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 1647fb85c4..79eecaa2b6 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -1,6 +1,7 @@ import json import logging +import ddt from django.core.urlresolvers import reverse from django.http import Http404 from django.test.client import Client, RequestFactory @@ -17,8 +18,14 @@ from django_comment_client.tests.utils import CohortedContentTestCase from django_comment_client.utils import strip_none from student.tests.factories import UserFactory, CourseEnrollmentFactory from util.testing import UrlResetMixin -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_MOCK_MODULESTORE -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + TEST_DATA_MOCK_MODULESTORE, + TEST_DATA_MONGO_MODULESTORE +) +from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, ItemFactory from courseware.courses import UserNotEnrolled from nose.tools import assert_true # pylint: disable=E0611 @@ -98,7 +105,7 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(self.response.status_code, 404) -def make_mock_thread_data(text, thread_id, include_children, group_id=None, group_name=None, commentable_id=None): +def make_mock_thread_data(text, thread_id, num_children, group_id=None, group_name=None, commentable_id=None): thread_data = { "id": thread_id, "type": "thread", @@ -112,25 +119,31 @@ def make_mock_thread_data(text, thread_id, include_children, group_id=None, grou } if group_id is not None: thread_data['group_name'] = group_name - if include_children: + if num_children is not None: thread_data["children"] = [{ - "id": "dummy_comment_id", + "id": "dummy_comment_id_{}".format(i), "type": "comment", "body": text, - }] + } for i in range(num_children)] return thread_data -def make_mock_request_impl(text, thread_id="dummy_thread_id", group_id=None, commentable_id=None): +def make_mock_request_impl( + text, + thread_id="dummy_thread_id", + group_id=None, + commentable_id=None, + num_thread_responses=1, +): def mock_request_impl(*args, **kwargs): url = args[1] data = None if url.endswith("threads") or url.endswith("user_profile"): data = { - "collection": [make_mock_thread_data(text, thread_id, False, group_id=group_id, commentable_id=commentable_id)] + "collection": [make_mock_thread_data(text, thread_id, None, group_id=group_id, commentable_id=commentable_id)] } elif thread_id and url.endswith(thread_id): - data = make_mock_thread_data(text, thread_id, True, group_id=group_id) + data = make_mock_thread_data(text, thread_id, num_thread_responses, group_id=group_id) elif "/users/" in url: data = { "default_sort_key": "date", @@ -200,7 +213,7 @@ class SingleThreadTestCase(ModuleStoreTestCase): # django view performs prior to writing thread data to the response self.assertEquals( response_data["content"], - strip_none(make_mock_thread_data(text, thread_id, True)) + strip_none(make_mock_thread_data(text, thread_id, 1)) ) mock_request.assert_called_with( "get", @@ -236,7 +249,7 @@ class SingleThreadTestCase(ModuleStoreTestCase): # django view performs prior to writing thread data to the response self.assertEquals( response_data["content"], - strip_none(make_mock_thread_data(text, thread_id, True)) + strip_none(make_mock_thread_data(text, thread_id, 1)) ) mock_request.assert_called_with( "get", @@ -278,6 +291,54 @@ class SingleThreadTestCase(ModuleStoreTestCase): ) +@ddt.ddt +@patch('requests.request') +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class SingleThreadQueryCountTestCase(ModuleStoreTestCase): + """ + Ensures the number of modulestore queries is deterministic based on the + number of responses retrieved for a given discussion thread. + """ + + @ddt.data( + # old mongo: number of responses plus 16. TODO: O(n)! + (ModuleStoreEnum.Type.mongo, 1, 17), + (ModuleStoreEnum.Type.mongo, 50, 66), + # split mongo: 3 queries, regardless of thread response size. + (ModuleStoreEnum.Type.split, 1, 3), + (ModuleStoreEnum.Type.split, 50, 3), + ) + @ddt.unpack + def test_number_of_mongo_queries(self, default_store, num_thread_responses, num_mongo_calls, mock_request): + + with modulestore().default_store(default_store): + course = CourseFactory.create() + + student = UserFactory.create() + CourseEnrollmentFactory.create(user=student, course_id=course.id) + + test_thread_id = "test_thread_id" + mock_request.side_effect = make_mock_request_impl( + "dummy content", + test_thread_id, + num_thread_responses=num_thread_responses, + ) + request = RequestFactory().get( + "dummy_url", + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + request.user = student + with check_mongo_calls(num_mongo_calls): + response = views.single_thread( + request, + course.id.to_deprecated_string(), + "dummy_discussion_id", + test_thread_id + ) + self.assertEquals(response.status_code, 200) + self.assertEquals(len(json.loads(response.content)["content"]["children"]), num_thread_responses) + + @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) @patch('requests.request') class SingleCohortedThreadTestCase(CohortedContentTestCase): @@ -309,7 +370,7 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase): self.assertEquals( response_data["content"], make_mock_thread_data( - self.mock_text, self.mock_thread_id, True, + self.mock_text, self.mock_thread_id, 1, group_id=self.student_cohort.id, group_name=self.student_cohort.name, ) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index c2e08a3b61..d1cb4e83da 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -1,3 +1,8 @@ +""" +Views handling read (GET) requests for the Discussion tab and inline discussions. +""" + +from functools import wraps import json import logging import xml.sax.saxutils as saxutils @@ -18,6 +23,7 @@ from openedx.core.djangoapps.course_groups.cohorts import ( is_commentable_cohorted ) from courseware.access import has_access +from xmodule.modulestore.django import modulestore from django_comment_client.permissions import cached_has_permission from django_comment_client.utils import ( @@ -30,7 +36,7 @@ from django_comment_client.utils import ( import django_comment_client.utils as utils import lms.lib.comment_client as cc -from opaque_keys.edx.locations import SlashSeparatedCourseKey +from opaque_keys.edx.keys import CourseKey THREADS_PER_PAGE = 20 INLINE_THREADS_PER_PAGE = 20 @@ -130,13 +136,27 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA return threads, query_params +def use_bulk_ops(view_func): + """ + Wraps internal request handling inside a modulestore bulk op, significantly + reducing redundant database calls. Also converts the course_id parsed from + the request uri to a CourseKey before passing to the view. + """ + @wraps(view_func) + def wrapped_view(request, course_id, *args, **kwargs): # pylint: disable=missing-docstring + course_key = CourseKey.from_string(course_id) + with modulestore().bulk_operations(course_key): + return view_func(request, course_key, *args, **kwargs) + return wrapped_view + + @login_required -def inline_discussion(request, course_id, discussion_id): +@use_bulk_ops +def inline_discussion(request, course_key, discussion_id): """ Renders JSON for DiscussionModules """ nr_transaction = newrelic.agent.current_transaction() - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) course = get_course_with_access(request.user, 'load_forum', course_key) cc_user = cc.User.from_django_user(request.user) @@ -166,11 +186,11 @@ def inline_discussion(request, course_id, discussion_id): @login_required -def forum_form_discussion(request, course_id): +@use_bulk_ops +def forum_form_discussion(request, course_key): """ Renders the main Discussion page, potentially filtered by a search query """ - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) nr_transaction = newrelic.agent.current_transaction() course = get_course_with_access(request.user, 'load_forum', course_key, check_if_enrolled=True) @@ -233,8 +253,12 @@ def forum_form_discussion(request, course_id): @require_GET @login_required -def single_thread(request, course_id, discussion_id, thread_id): - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) +@use_bulk_ops +def single_thread(request, course_key, discussion_id, thread_id): + """ + Renders a response to display a single discussion thread. + """ + nr_transaction = newrelic.agent.current_transaction() course = get_course_with_access(request.user, 'load_forum', course_key) @@ -326,8 +350,13 @@ def single_thread(request, course_id, discussion_id, thread_id): @require_GET @login_required -def user_profile(request, course_id, user_id): - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) +@use_bulk_ops +def user_profile(request, course_key, user_id): + """ + Renders a response to display the user profile page (shown after clicking + on a post author's username). + """ + nr_transaction = newrelic.agent.current_transaction() #TODO: Allow sorting? @@ -384,8 +413,12 @@ def user_profile(request, course_id, user_id): @login_required -def followed_threads(request, course_id, user_id): - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) +@use_bulk_ops +def followed_threads(request, course_key, user_id): + """ + Ajax-only endpoint retrieving the threads followed by a specific user. + """ + nr_transaction = newrelic.agent.current_transaction() course = get_course_with_access(request.user, 'load_forum', course_key)