Merge pull request #8708 from edx/benmcmorran/discussion-caching-2

TNL-2458 Cache discussion id mapping on course publish

lms/djangoapps/django_comment_client/base/views.py

@@ -29,6 +29,7 @@ from django_comment_client.utils import (
     get_group_id_for_comments_service,
     get_discussion_categories_ids,
     get_discussion_id_map,
+    get_cached_discussion_id_map,
 )
 from django_comment_client.permissions import check_permissions_by_view, has_permission
 from eventtracking import tracker
@@ -78,10 +79,11 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None):
     """
     user = request.user
     data['id'] = obj.id
-    if id_map is None:
-        id_map = get_discussion_id_map(course, user)
-
     commentable_id = data['commentable_id']
+
+    if id_map is None:
+        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"]
         data['category_id'] = commentable_id

lms/djangoapps/django_comment_client/tests/test_utils.py

@@ -18,6 +18,7 @@ from courseware.tests.factories import InstructorFactory
 from courseware.tabs import get_course_tab_list
 from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings
 from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentFactory
+from openedx.core.djangoapps.content.course_structures.models import CourseStructure
 from openedx.core.djangoapps.util.testing import ContentGroupTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
@@ -154,6 +155,94 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
         assertThreadCorrect(threads[1], self.discussion2, "Subsection / Discussion 2")
 
 
+class CachedDiscussionIdMapTestCase(ModuleStoreTestCase):
+    """
+    Tests that using the cache of discussion id mappings has the same behavior as searching through the course.
+    """
+    def setUp(self):
+        super(CachedDiscussionIdMapTestCase, self).setUp(create_user=True)
+
+        self.course = CourseFactory.create(org='TestX', number='101', display_name='Test Course')
+        self.discussion = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='test_discussion_id',
+            discussion_category='Chapter',
+            discussion_target='Discussion 1'
+        )
+        self.private_discussion = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='private_discussion_id',
+            discussion_category='Chapter 3',
+            discussion_target='Beta Testing',
+            visible_to_staff_only=True
+        )
+        self.bad_discussion = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='bad_discussion_id',
+            discussion_category=None,
+            discussion_target=None
+        )
+
+    def test_cache_returns_correct_key(self):
+        usage_key = utils.get_cached_discussion_key(self.course, 'test_discussion_id')
+        self.assertEqual(usage_key, self.discussion.location)
+
+    def test_cache_returns_none_if_id_is_not_present(self):
+        usage_key = utils.get_cached_discussion_key(self.course, 'bogus_id')
+        self.assertIsNone(usage_key)
+
+    def test_cache_raises_exception_if_course_structure_not_cached(self):
+        CourseStructure.objects.all().delete()
+        with self.assertRaises(utils.DiscussionIdMapIsNotCached):
+            utils.get_cached_discussion_key(self.course, 'test_discussion_id')
+
+    def test_cache_raises_exception_if_discussion_id_not_cached(self):
+        cache = CourseStructure.objects.get(course_id=self.course.id)
+        cache.discussion_id_map_json = None
+        cache.save()
+
+        with self.assertRaises(utils.DiscussionIdMapIsNotCached):
+            utils.get_cached_discussion_key(self.course, 'test_discussion_id')
+
+    def test_module_does_not_have_required_keys(self):
+        self.assertTrue(utils.has_required_keys(self.discussion))
+        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')
+
+    def test_get_discussion_id_map_from_cache(self):
+        self.verify_discussion_metadata()
+
+    def test_get_discussion_id_map_without_cache(self):
+        CourseStructure.objects.all().delete()
+        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)
+        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)
+        self.assertEqual(metadata['private_discussion_id']['title'], 'Chapter 3 / Beta Testing')
+
+        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)
+        self.assertEqual(metadata, {})
+
+
 class CategoryMapTestMixin(object):
     """
     Provides functionality for classes that test

lms/djangoapps/django_comment_client/utils.py

@@ -20,6 +20,7 @@ from django_comment_client.settings import MAX_COMMENT_DEPTH
 from edxmako import lookup_template
 
 from courseware.access import has_access
+from openedx.core.djangoapps.content.course_structures.models import CourseStructure
 from openedx.core.djangoapps.course_groups.cohorts import (
     get_course_cohort_settings, get_cohort_by_id, get_cohort_id, is_commentable_cohorted, is_course_cohorted
 )
@@ -62,6 +63,15 @@ def has_forum_access(uname, course_id, rolename):
     return role.users.filter(username=uname).exists()
 
 
+def has_required_keys(module):
+    """Returns True iff module has the proper attributes for generating metadata with get_discussion_id_map_entry()"""
+    for key in ('discussion_id', 'discussion_category', 'discussion_target'):
+        if getattr(module, key, None) is None:
+            log.debug("Required key '%s' not in discussion %s, leaving out of category map", key, module.location)
+            return False
+    return True
+
+
 def get_accessible_discussion_modules(course, user, include_all=False):  # pylint: disable=invalid-name
     """
     Return a list of all valid discussion modules in this course that
@@ -69,31 +79,68 @@ def get_accessible_discussion_modules(course, user, include_all=False):  # pylin
     """
     all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'})
 
-    def has_required_keys(module):
-        for key in ('discussion_id', 'discussion_category', 'discussion_target'):
-            if getattr(module, key, None) is None:
-                log.warning("Required key '%s' not in discussion %s, leaving out of category map" % (key, module.location))
-                return False
-        return True
-
     return [
         module for module in all_modules
         if has_required_keys(module) and (include_all or has_access(user, 'load', module, course.id))
     ]
 
 
+def get_discussion_id_map_entry(module):
+    """
+    Returns a tuple of (discussion_id, metadata) suitable for inclusion in the results of get_discussion_id_map().
+    """
+    return (
+        module.discussion_id,
+        {
+            "location": module.location,
+            "title": module.discussion_category.split("/")[-1].strip() + " / " + module.discussion_target
+        }
+    )
+
+
+class DiscussionIdMapIsNotCached(Exception):
+    """Thrown when the discussion id map is not cached for this course, but an attempt was made to access it."""
+    pass
+
+
+def get_cached_discussion_key(course, discussion_id):
+    """
+    Returns the usage key of the discussion module associated with discussion_id if it is cached. If the discussion id
+    map is cached but does not contain discussion_id, returns None. If the discussion id map is not cached for course,
+    raises a DiscussionIdMapIsNotCached exception.
+    """
+    try:
+        cached_mapping = CourseStructure.objects.get(course_id=course.id).discussion_id_map
+        if not cached_mapping:
+            raise DiscussionIdMapIsNotCached()
+        return cached_mapping.get(discussion_id)
+    except CourseStructure.DoesNotExist:
+        raise DiscussionIdMapIsNotCached()
+
+
+def get_cached_discussion_id_map(course, discussion_id, 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
+    """
+    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)])
+    except DiscussionIdMapIsNotCached:
+        return get_discussion_id_map(course, user)
+
+
 def get_discussion_id_map(course, user):
     """
     Transform the list of this course's discussion modules (visible to a given user) into a dictionary of metadata keyed
     by discussion_id.
     """
-    def get_entry(module):  # pylint: disable=missing-docstring
-        discussion_id = module.discussion_id
-        title = module.discussion_target
-        last_category = module.discussion_category.split("/")[-1].strip()
-        return (discussion_id, {"location": module.location, "title": last_category + " / " + title})
-
-    return dict(map(get_entry, get_accessible_discussion_modules(course, user)))
+    return dict(map(get_discussion_id_map_entry, get_accessible_discussion_modules(course, user)))
 
 
 def _filter_unstarted_categories(category_map):

openedx/core/djangoapps/content/course_structures/migrations/0002_auto__add_field_coursestructure_discussion_id_map_json.py

@@ -0,0 +1,34 @@
+# -*- coding: utf-8 -*-
+from south.utils import datetime_utils as datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # Adding field 'CourseStructure.discussion_id_map_json'
+        db.add_column('course_structures_coursestructure', 'discussion_id_map_json',
+                      self.gf('django.db.models.fields.TextField')(null=True, blank=True),
+                      keep_default=False)
+
+
+    def backwards(self, orm):
+        # Deleting field 'CourseStructure.discussion_id_map_json'
+        db.delete_column('course_structures_coursestructure', 'discussion_id_map_json')
+
+
+    models = {
+        'course_structures.coursestructure': {
+            'Meta': {'object_name': 'CourseStructure'},
+            'course_id': ('xmodule_django.models.CourseKeyField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}),
+            'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}),
+            'discussion_id_map_json': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'modified': ('model_utils.fields.AutoLastModifiedField', [], {'default': 'datetime.datetime.now'}),
+            'structure_json': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['course_structures']

openedx/core/djangoapps/content/course_structures/models.py

@@ -5,7 +5,7 @@ from collections import OrderedDict
 from model_utils.models import TimeStampedModel
 
 from util.models import CompressedTextField
-from xmodule_django.models import CourseKeyField
+from xmodule_django.models import CourseKeyField, UsageKey
 
 
 logger = logging.getLogger(__name__)  # pylint: disable=invalid-name
@@ -21,6 +21,9 @@ class CourseStructure(TimeStampedModel):
     # we'd have to be careful about caching.
     structure_json = CompressedTextField(verbose_name='Structure JSON', blank=True, null=True)
 
+    # JSON mapping of discussion ids to usage keys for the corresponding discussion modules
+    discussion_id_map_json = CompressedTextField(verbose_name='Discussion ID Map JSON', blank=True, null=True)
+
     @property
     def structure(self):
         if self.structure_json:
@@ -37,6 +40,19 @@ class CourseStructure(TimeStampedModel):
             self._traverse_tree(self.structure['root'], self.structure['blocks'], ordered_blocks)
             return ordered_blocks
 
+    @property
+    def discussion_id_map(self):
+        """
+        Return a mapping of discussion ids to usage keys of the corresponding discussion modules.
+        """
+        if self.discussion_id_map_json:
+            result = json.loads(self.discussion_id_map_json)
+            for discussion_id in result:
+                # Usage key strings might not include the course run, so we add it back in with map_into_course
+                result[discussion_id] = UsageKey.from_string(result[discussion_id]).map_into_course(self.course_id)
+            return result
+        return None
+
     def _traverse_tree(self, block, unordered_structure, ordered_blocks, parent=None):
         """
         Traverses the tree and fills in the ordered_blocks OrderedDict with the blocks in

openedx/core/djangoapps/content/course_structures/signals.py

@@ -2,12 +2,22 @@ from django.dispatch.dispatcher import receiver
 
 from xmodule.modulestore.django import SignalHandler
 
+from .models import CourseStructure
+
 
 @receiver(SignalHandler.course_published)
 def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=unused-argument
     # Import tasks here to avoid a circular import.
     from .tasks import update_course_structure
 
+    # Delete the existing discussion id map cache to avoid inconsistencies
+    try:
+        structure = CourseStructure.objects.get(course_id=course_key)
+        structure.discussion_id_map_json = None
+        structure.save()
+    except CourseStructure.DoesNotExist:
+        pass
+
     # Note: The countdown=0 kwarg is set to to ensure the method below does not attempt to access the course
     # before the signal emitter has finished all operations. This is also necessary to ensure all tests pass.
     update_course_structure.apply_async([unicode(course_key)], countdown=0)

openedx/core/djangoapps/content/course_structures/tasks.py

@@ -17,6 +17,7 @@ def _generate_course_structure(course_key):
         course = modulestore().get_course(course_key, depth=None)
         blocks_stack = [course]
         blocks_dict = {}
+        discussions = {}
         while blocks_stack:
             curr_block = blocks_stack.pop()
             children = curr_block.get_children() if curr_block.has_children else []
@@ -28,6 +29,11 @@ def _generate_course_structure(course_key):
                 "children": [unicode(child.scope_ids.usage_id) for child in children]
             }
 
+            if (curr_block.category == 'discussion' and
+                    hasattr(curr_block, 'discussion_id') and
+                    curr_block.discussion_id):
+                discussions[curr_block.discussion_id] = unicode(curr_block.scope_ids.usage_id)
+
             # Retrieve these attributes separately so that we can fail gracefully
             # if the block doesn't have the attribute.
             attrs = (('graded', False), ('format', None))
@@ -43,8 +49,11 @@ def _generate_course_structure(course_key):
             # Add this blocks children to the stack so that we can traverse them as well.
             blocks_stack.extend(children)
         return {
-            "root": unicode(course.scope_ids.usage_id),
-            "blocks": blocks_dict
+            'structure': {
+                "root": unicode(course.scope_ids.usage_id),
+                "blocks": blocks_dict
+            },
+            'discussion_id_map': discussions
         }
 
 
@@ -69,13 +78,18 @@ def update_course_structure(course_key):
         log.exception('An error occurred while generating course structure: %s', ex.message)
         raise
 
-    structure_json = json.dumps(structure)
+    structure_json = json.dumps(structure['structure'])
+    discussion_id_map_json = json.dumps(structure['discussion_id_map'])
 
-    cs, created = CourseStructure.objects.get_or_create(
+    structure_model, created = CourseStructure.objects.get_or_create(
         course_id=course_key,
-        defaults={'structure_json': structure_json}
+        defaults={
+            'structure_json': structure_json,
+            'discussion_id_map_json': discussion_id_map_json
+        }
     )
 
     if not created:
-        cs.structure_json = structure_json
-        cs.save()
+        structure_model.structure_json = structure_json
+        structure_model.discussion_id_map_json = discussion_id_map_json
+        structure_model.save()

openedx/core/djangoapps/content/course_structures/tests.py

@@ -1,5 +1,6 @@
 import json
 
+from xmodule_django.models import UsageKey
 from xmodule.modulestore.django import SignalHandler
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
@@ -21,8 +22,18 @@ class SignalDisconnectTestMixin(object):
 class CourseStructureTaskTests(ModuleStoreTestCase):
     def setUp(self, **kwargs):
         super(CourseStructureTaskTests, self).setUp()
-        self.course = CourseFactory.create()
+        self.course = CourseFactory.create(org='TestX', course='TS101', run='T1')
         self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section')
+        self.discussion_module_1 = ItemFactory.create(
+            parent=self.course,
+            category='discussion',
+            discussion_id='test_discussion_id_1'
+        )
+        self.discussion_module_2 = ItemFactory.create(
+            parent=self.course,
+            category='discussion',
+            discussion_id='test_discussion_id_2'
+        )
         CourseStructure.objects.all().delete()
 
     def test_generate_course_structure(self):
@@ -52,7 +63,7 @@ class CourseStructureTaskTests(ModuleStoreTestCase):
 
         self.maxDiff = None
         actual = _generate_course_structure(self.course.id)
-        self.assertDictEqual(actual, expected)
+        self.assertDictEqual(actual['structure'], expected)
 
     def test_structure_json(self):
         """
@@ -138,7 +149,7 @@ class CourseStructureTaskTests(ModuleStoreTestCase):
         structure = _generate_course_structure(self.course.id)
 
         usage_key = unicode(module.location)
-        actual = structure['blocks'][usage_key]
+        actual = structure['structure']['blocks'][usage_key]
         expected = {
             "usage_key": usage_key,
             "block_type": category,
@@ -149,6 +160,53 @@ class CourseStructureTaskTests(ModuleStoreTestCase):
         }
         self.assertEqual(actual, expected)
 
+    def test_generate_discussion_id_map(self):
+        id_map = {}
+
+        def add_block(block):
+            """Adds the given block and all of its children to the expected discussion id map"""
+            children = block.get_children() if block.has_children else []
+
+            if block.category == 'discussion':
+                id_map[block.discussion_id] = unicode(block.location)
+
+            for child in children:
+                add_block(child)
+
+        add_block(self.course)
+
+        actual = _generate_course_structure(self.course.id)
+        self.assertEqual(actual['discussion_id_map'], id_map)
+
+    def test_discussion_id_map_json(self):
+        id_map = {
+            'discussion_id_1': 'module_location_1',
+            'discussion_id_2': 'module_location_2'
+        }
+        id_map_json = json.dumps(id_map)
+        structure = CourseStructure.objects.create(course_id=self.course.id, discussion_id_map_json=id_map_json)
+        self.assertEqual(structure.discussion_id_map_json, id_map_json)
+
+        structure = CourseStructure.objects.get(course_id=self.course.id)
+        self.assertEqual(structure.discussion_id_map_json, id_map_json)
+
+    def test_discussion_id_map(self):
+        id_map = {
+            'discussion_id_1': 'block-v1:TestX+TS101+T1+type@discussion+block@b141953dff414921a715da37eb14ecdc',
+            'discussion_id_2': 'i4x://TestX/TS101/discussion/466f474fa4d045a8b7bde1b911e095ca'
+        }
+        id_map_json = json.dumps(id_map)
+        structure = CourseStructure.objects.create(course_id=self.course.id, discussion_id_map_json=id_map_json)
+        expected_id_map = {
+            key: UsageKey.from_string(value).map_into_course(self.course.id)
+            for key, value in id_map.iteritems()
+        }
+        self.assertEqual(structure.discussion_id_map, expected_id_map)
+
+    def test_discussion_id_map_missing(self):
+        structure = CourseStructure.objects.create(course_id=self.course.id)
+        self.assertIsNone(structure.discussion_id_map)
+
     def test_update_course_structure(self):
         """
         Test the actual task that orchestrates data generation and updating the database.
@@ -158,8 +216,13 @@ class CourseStructureTaskTests(ModuleStoreTestCase):
         self.assertRaises(ValueError, update_course_structure, course_id)
 
         # Ensure a CourseStructure object is created
-        structure = _generate_course_structure(course_id)
+        expected_structure = _generate_course_structure(course_id)
         update_course_structure(unicode(course_id))
-        cs = CourseStructure.objects.get(course_id=course_id)
-        self.assertEqual(cs.course_id, course_id)
-        self.assertEqual(cs.structure, structure)
+        structure = CourseStructure.objects.get(course_id=course_id)
+        self.assertEqual(structure.course_id, course_id)
+        self.assertEqual(structure.structure, expected_structure['structure'])
+        self.assertEqual(structure.discussion_id_map.keys(), expected_structure['discussion_id_map'].keys())
+        self.assertEqual(
+            [unicode(value) for value in structure.discussion_id_map.values()],
+            expected_structure['discussion_id_map'].values()
+        )