Merge branch 'release' into rc/2015-01-07

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 import ModuleStoreEnum
-from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
+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,
             )

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):
+@use_bulk_ops
-    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+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):
+@use_bulk_ops
-    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+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):
+@use_bulk_ops
-    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+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)