Merge pull request #8837 from edx/benmcmorran/discussion-caching-3

TNL-2389 Use discussion id map cache for thread creation and update

lms/djangoapps/django_comment_client/base/tests.py

 import logging
 import json
+import ddt
 
+from django.conf import settings
+from django.core.cache import get_cache
 from django.test.client import Client, RequestFactory
 from django.contrib.auth.models import User
 from django.core.management import call_command
 from django.core.urlresolvers import reverse
+from request_cache.middleware import RequestCache
 from mock import patch, ANY, Mock
 from nose.tools import assert_true, assert_equal  # pylint: disable=no-name-in-module
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -18,8 +22,11 @@ from django_comment_common.models import Role
 from django_comment_common.utils import seed_permissions_roles
 from student.tests.factories import CourseEnrollmentFactory, UserFactory, CourseAccessRoleFactory
 from util.testing import UrlResetMixin
-from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import check_mongo_calls
+from xmodule.modulestore.django import modulestore
+from xmodule.modulestore import ModuleStoreEnum
 
 
 log = logging.getLogger(__name__)
@@ -161,16 +168,19 @@ class ThreadActionGroupIdTestCase(
         )
 
 
-@patch('lms.lib.comment_client.utils.requests.request')
-class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
-
-    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
-    def setUp(self):
+class ViewsTestCaseMixin(object):
+    """
+    This class is used by both ViewsQueryCountTestCase and ViewsTestCase. By
+    breaking out set_up_course into its own method, ViewsQueryCountTestCase
+    can build a course in a particular modulestore, while ViewsTestCase can
+    just run it in setUp for all tests.
+    """
 
-        # Patching the ENABLE_DISCUSSION_SERVICE value affects the contents of urls.py,
-        # so we need to call super.setUp() which reloads urls.py (because
-        # of the UrlResetMixin)
-        super(ViewsTestCase, self).setUp(create_user=False)
+    def set_up_course(self, module_count=0):
+        """
+        Creates a course, optionally with module_count discussion modules, and
+        a user with appropriate permissions.
+        """
 
         # create a course
         self.course = CourseFactory.create(
@@ -179,6 +189,17 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
             display_name='Robot Super Course',
         )
         self.course_id = self.course.id
+
+        # add some discussion modules
+        for i in range(module_count):
+            ItemFactory.create(
+                parent_location=self.course.location,
+                category='discussion',
+                discussion_id='id_module_{}'.format(i),
+                discussion_category='Category {}'.format(i),
+                discussion_target='Discussion {}'.format(i)
+            )
+
         # seed the forums permissions and roles
         call_command('seed_permissions_roles', self.course_id.to_deprecated_string())
 
@@ -201,7 +222,24 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
             self.client = Client()
             assert_true(self.client.login(username='student', password='test'))
 
-    def test_create_thread(self, mock_request):
+    def _setup_mock_request(self, mock_request, include_depth=False):
+        """
+        Ensure that mock_request returns the data necessary to make views
+        function correctly
+        """
+        mock_request.return_value.status_code = 200
+        data = {
+            "user_id": str(self.student.id),
+            "closed": False,
+        }
+        if include_depth:
+            data["depth"] = 0
+        self._set_mock_request_data(mock_request, data)
+
+    def create_thread_helper(self, mock_request):
+        """
+        Issues a request to create a thread and verifies the result.
+        """
         mock_request.return_value.status_code = 200
         self._set_mock_request_data(mock_request, {
             "thread_type": "discussion",
@@ -255,7 +293,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
                 'title': u'Hello',
                 'commentable_id': u'i4x-MITx-999-course-Robot_Super_Course',
                 'anonymous': False,
-                'course_id': u'MITx/999/Robot_Super_Course',
+                'course_id': unicode(self.course_id),
             },
             params={'request_id': ANY},
             headers=ANY,
@@ -263,6 +301,83 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
         )
         assert_equal(response.status_code, 200)
 
+    def update_thread_helper(self, mock_request):
+        """
+        Issues a request to update a thread and verifies the result.
+        """
+        self._setup_mock_request(mock_request)
+        response = self.client.post(
+            reverse("update_thread", kwargs={"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}),
+            data={"body": "foo", "title": "foo", "commentable_id": "some_topic"}
+        )
+        self.assertEqual(response.status_code, 200)
+
+
+@ddt.ddt
+@patch('lms.lib.comment_client.utils.requests.request')
+class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin):
+
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
+    def setUp(self):
+        super(ViewsQueryCountTestCase, self).setUp(create_user=False)
+
+    def clear_caches(self):
+        """Clears caches so that query count numbers are accurate."""
+        for cache in settings.CACHES:
+            get_cache(cache).clear()
+        RequestCache.clear_request_cache()
+
+    def count_queries(func):  # pylint: disable=no-self-argument
+        """
+        Decorates test methods to count mongo and SQL calls for a
+        particular modulestore.
+        """
+        def inner(self, default_store, module_count, mongo_calls, sql_queries, *args, **kwargs):
+            with modulestore().default_store(default_store):
+                self.set_up_course(module_count=module_count)
+                self.clear_caches()
+                with self.assertNumQueries(sql_queries):
+                    with check_mongo_calls(mongo_calls):
+                        func(self, *args, **kwargs)
+        return inner
+
+    @ddt.data(
+        (ModuleStoreEnum.Type.mongo, 3, 4, 21),
+        (ModuleStoreEnum.Type.mongo, 20, 4, 21),
+        (ModuleStoreEnum.Type.split, 3, 13, 21),
+        (ModuleStoreEnum.Type.split, 20, 13, 21),
+    )
+    @ddt.unpack
+    @count_queries
+    def test_create_thread(self, mock_request):
+        self.create_thread_helper(mock_request)
+
+    @ddt.data(
+        (ModuleStoreEnum.Type.mongo, 3, 3, 16),
+        (ModuleStoreEnum.Type.mongo, 20, 3, 16),
+        (ModuleStoreEnum.Type.split, 3, 10, 16),
+        (ModuleStoreEnum.Type.split, 20, 10, 16),
+    )
+    @ddt.unpack
+    @count_queries
+    def test_update_thread(self, mock_request):
+        self.update_thread_helper(mock_request)
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin):
+
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
+    def setUp(self):
+        # Patching the ENABLE_DISCUSSION_SERVICE value affects the contents of urls.py,
+        # so we need to call super.setUp() which reloads urls.py (because
+        # of the UrlResetMixin)
+        super(ViewsTestCase, self).setUp(create_user=False)
+        self.set_up_course()
+
+    def test_create_thread(self, mock_request):
+        self.create_thread_helper(mock_request)
+
     def test_delete_comment(self, mock_request):
         self._set_mock_request_data(mock_request, {
             "user_id": str(self.student.id),
@@ -280,20 +395,6 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
         self.assertEqual(args[0], "delete")
         self.assertTrue(args[1].endswith("/{}".format(test_comment_id)))
 
-    def _setup_mock_request(self, mock_request, include_depth=False):
-        """
-        Ensure that mock_request returns the data necessary to make views
-        function correctly
-        """
-        mock_request.return_value.status_code = 200
-        data = {
-            "user_id": str(self.student.id),
-            "closed": False,
-        }
-        if include_depth:
-            data["depth"] = 0
-        self._set_mock_request_data(mock_request, data)
-
     def _test_request_error(self, view_name, view_kwargs, data, mock_request):
         """
         Submit a request against the given view with the given data and ensure
@@ -372,14 +473,9 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
         )
 
     def test_update_thread_course_topic(self, mock_request):
-        self._setup_mock_request(mock_request)
-        response = self.client.post(
-            reverse("update_thread", kwargs={"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}),
-            data={"body": "foo", "title": "foo", "commentable_id": "some_topic"}
-        )
-        self.assertEqual(response.status_code, 200)
+        self.update_thread_helper(mock_request)
 
-    @patch('django_comment_client.base.views.get_discussion_categories_ids', return_value=["test_commentable"])
+    @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"])
     def test_update_thread_wrong_commentable_id(self, mock_get_discussion_id_map, mock_request):
         self._test_request_error(
             "update_thread",
@@ -876,7 +972,7 @@ class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq
         self.student = UserFactory.create()
         CourseEnrollmentFactory(user=self.student, course_id=self.course.id)
 
-    @patch('django_comment_client.base.views.get_discussion_categories_ids', return_value=["test_commentable"])
+    @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"])
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request, mock_get_discussion_id_map):
         self._set_mock_request_data(mock_request, {

lms/djangoapps/django_comment_client/base/views.py

@@ -27,8 +27,7 @@ from django_comment_client.utils import (
     JsonResponse,
     prepare_content,
     get_group_id_for_comments_service,
-    get_discussion_categories_ids,
-    get_discussion_id_map,
+    discussion_category_id_access,
     get_cached_discussion_id_map,
 )
 from django_comment_client.permissions import check_permissions_by_view, has_permission
@@ -82,7 +81,7 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None):
     commentable_id = data['commentable_id']
 
     if id_map is None:
-        id_map = get_cached_discussion_id_map(course, commentable_id, user)
+        id_map = get_cached_discussion_id_map(course, [commentable_id], user)
 
     if commentable_id in id_map:
         data['category_name'] = id_map[commentable_id]["title"]
@@ -209,14 +208,9 @@ def create_thread(request, course_id, commentable_id):
     event_data = get_thread_created_event_data(thread, follow)
     data = thread.to_dict()
 
-    # Calls to id map are expensive, but we need this more than once.
-    # Prefetch it.
-    id_map = get_discussion_id_map(course, request.user)
+    add_courseware_context([data], course, request.user)
 
-    add_courseware_context([data], course, request.user, id_map=id_map)
-
-    track_forum_event(request, THREAD_CREATED_EVENT_NAME,
-                      course, thread, event_data, id_map=id_map)
+    track_forum_event(request, THREAD_CREATED_EVENT_NAME, course, thread, event_data)
 
     if request.is_ajax():
         return ajax_content_response(request, course_key, data)
@@ -246,8 +240,7 @@ def update_thread(request, course_id, thread_id):
         thread.thread_type = request.POST["thread_type"]
     if "commentable_id" in request.POST:
         course = get_course_with_access(request.user, 'load', course_key)
-        commentable_ids = get_discussion_categories_ids(course, request.user)
-        if request.POST.get("commentable_id") in commentable_ids:
+        if discussion_category_id_access(course, request.user, request.POST.get("commentable_id")):
             thread.commentable_id = request.POST["commentable_id"]
         else:
             return JsonError(_("Topic doesn't exist"))

lms/djangoapps/django_comment_client/forum/tests.py

@@ -316,11 +316,11 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
 
     @ddt.data(
         # old mongo with cache
-        (ModuleStoreEnum.Type.mongo, 1, 7, 5, 12, 7),
-        (ModuleStoreEnum.Type.mongo, 50, 7, 5, 12, 7),
+        (ModuleStoreEnum.Type.mongo, 1, 7, 5, 13, 8),
+        (ModuleStoreEnum.Type.mongo, 50, 7, 5, 13, 8),
         # split mongo: 3 queries, regardless of thread response size.
-        (ModuleStoreEnum.Type.split, 1, 3, 3, 12, 7),
-        (ModuleStoreEnum.Type.split, 50, 3, 3, 12, 7),
+        (ModuleStoreEnum.Type.split, 1, 3, 3, 13, 8),
+        (ModuleStoreEnum.Type.split, 50, 3, 3, 13, 8),
     )
     @ddt.unpack
     def test_number_of_mongo_queries(

lms/djangoapps/django_comment_client/tests/test_utils.py

@@ -170,6 +170,13 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase):
             discussion_category='Chapter',
             discussion_target='Discussion 1'
         )
+        self.discussion2 = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='test_discussion_id_2',
+            discussion_category='Chapter 2',
+            discussion_target='Discussion 2'
+        )
         self.private_discussion = ItemFactory.create(
             parent_location=self.course.location,
             category='discussion',
@@ -212,11 +219,18 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase):
         self.assertFalse(utils.has_required_keys(self.bad_discussion))
 
     def verify_discussion_metadata(self):
-        """Retrieves the metadata for self.discussion and verifies that it is correct"""
-        metadata = utils.get_cached_discussion_id_map(self.course, 'test_discussion_id', self.user)
-        metadata = metadata[self.discussion.discussion_id]
-        self.assertEqual(metadata['location'], self.discussion.location)
-        self.assertEqual(metadata['title'], 'Chapter / Discussion 1')
+        """Retrieves the metadata for self.discussion and self.discussion2 and verifies that it is correct"""
+        metadata = utils.get_cached_discussion_id_map(
+            self.course,
+            ['test_discussion_id', 'test_discussion_id_2'],
+            self.user
+        )
+        discussion1 = metadata[self.discussion.discussion_id]
+        discussion2 = metadata[self.discussion2.discussion_id]
+        self.assertEqual(discussion1['location'], self.discussion.location)
+        self.assertEqual(discussion1['title'], 'Chapter / Discussion 1')
+        self.assertEqual(discussion2['location'], self.discussion2.location)
+        self.assertEqual(discussion2['title'], 'Chapter 2 / Discussion 2')
 
     def test_get_discussion_id_map_from_cache(self):
         self.verify_discussion_metadata()
@@ -226,22 +240,36 @@ class CachedDiscussionIdMapTestCase(ModuleStoreTestCase):
         self.verify_discussion_metadata()
 
     def test_get_missing_discussion_id_map_from_cache(self):
-        metadata = utils.get_cached_discussion_id_map(self.course, 'bogus_id', self.user)
+        metadata = utils.get_cached_discussion_id_map(self.course, ['bogus_id'], self.user)
         self.assertEqual(metadata, {})
 
     def test_get_discussion_id_map_from_cache_without_access(self):
         user = UserFactory.create()
 
-        metadata = utils.get_cached_discussion_id_map(self.course, 'private_discussion_id', self.user)
+        metadata = utils.get_cached_discussion_id_map(self.course, ['private_discussion_id'], self.user)
         self.assertEqual(metadata['private_discussion_id']['title'], 'Chapter 3 / Beta Testing')
 
-        metadata = utils.get_cached_discussion_id_map(self.course, 'private_discussion_id', user)
+        metadata = utils.get_cached_discussion_id_map(self.course, ['private_discussion_id'], user)
         self.assertEqual(metadata, {})
 
     def test_get_bad_discussion_id(self):
-        metadata = utils.get_cached_discussion_id_map(self.course, 'bad_discussion_id', self.user)
+        metadata = utils.get_cached_discussion_id_map(self.course, ['bad_discussion_id'], self.user)
         self.assertEqual(metadata, {})
 
+    def test_discussion_id_accessible(self):
+        self.assertTrue(utils.discussion_category_id_access(self.course, self.user, 'test_discussion_id'))
+
+    def test_bad_discussion_id_not_accessible(self):
+        self.assertFalse(utils.discussion_category_id_access(self.course, self.user, 'bad_discussion_id'))
+
+    def test_missing_discussion_id_not_accessible(self):
+        self.assertFalse(utils.discussion_category_id_access(self.course, self.user, 'bogus_id'))
+
+    def test_discussion_id_not_accessible_without_access(self):
+        user = UserFactory.create()
+        self.assertTrue(utils.discussion_category_id_access(self.course, self.user, 'private_discussion_id'))
+        self.assertFalse(utils.discussion_category_id_access(self.course, user, 'private_discussion_id'))
+
 
 class CategoryMapTestMixin(object):
     """

lms/djangoapps/django_comment_client/utils.py

@@ -118,19 +118,22 @@ def get_cached_discussion_key(course, discussion_id):
         raise DiscussionIdMapIsNotCached()
 
 
-def get_cached_discussion_id_map(course, discussion_id, user):
+def get_cached_discussion_id_map(course, discussion_ids, user):
     """
-    Returns a dict mapping discussion_id to discussion module metadata if it is cached and visible to the user.
-    If not, returns the result of get_discussion_id_map
+    Returns a dict mapping discussion_ids to respective discussion module metadata if it is cached and visible to the
+    user. If not, returns the result of get_discussion_id_map
     """
     try:
-        key = get_cached_discussion_key(course, discussion_id)
-        if not key:
-            return {}
-        module = modulestore().get_item(key)
-        if not (has_required_keys(module) and has_access(user, 'load', module, course.id)):
-            return {}
-        return dict([get_discussion_id_map_entry(module)])
+        entries = []
+        for discussion_id in discussion_ids:
+            key = get_cached_discussion_key(course, discussion_id)
+            if not key:
+                continue
+            module = modulestore().get_item(key)
+            if not (has_required_keys(module) and has_access(user, 'load', module, course.id)):
+                continue
+            entries.append(get_discussion_id_map_entry(module))
+        return dict(entries)
     except DiscussionIdMapIsNotCached:
         return get_discussion_id_map(course, user)
 
@@ -324,6 +327,24 @@ def get_discussion_category_map(course, user, cohorted_if_in_list=False, exclude
     return _filter_unstarted_categories(category_map) if exclude_unstarted else category_map
 
 
+def discussion_category_id_access(course, user, discussion_id):
+    """
+    Returns True iff the given discussion_id is accessible for user in course.
+    Uses the discussion id cache if available, falling back to
+    get_discussion_categories_ids if there is no cache.
+    """
+    if discussion_id in course.top_level_discussion_topic_ids:
+        return True
+    try:
+        key = get_cached_discussion_key(course, discussion_id)
+        if not key:
+            return False
+        module = modulestore().get_item(key)
+        return has_required_keys(module) and has_access(user, 'load', module, course.id)
+    except DiscussionIdMapIsNotCached:
+        return discussion_id in get_discussion_categories_ids(course, user)
+
+
 def get_discussion_categories_ids(course, user, include_all=False):
     """
     Returns a list of available ids of categories for the course that
@@ -494,10 +515,14 @@ def extend_content(content):
 
 def add_courseware_context(content_list, course, user, id_map=None):
     """
-    Decorates `content_list` with courseware metadata.
+    Decorates `content_list` with courseware metadata using the discussion id map cache if available.
     """
     if id_map is None:
-        id_map = get_discussion_id_map(course, user)
+        id_map = get_cached_discussion_id_map(
+            course,
+            [content['commentable_id'] for content in content_list],
+            user
+        )
 
     for content in content_list:
         commentable_id = content['commentable_id']