diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index bfe0cc7715..b930908d29 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/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 diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index ca69ceb0be..cabcb83c4d 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/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 diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 7d8e2cf079..3bfce30039 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/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): diff --git a/openedx/core/djangoapps/content/course_structures/migrations/0002_auto__add_field_coursestructure_discussion_id_map_json.py b/openedx/core/djangoapps/content/course_structures/migrations/0002_auto__add_field_coursestructure_discussion_id_map_json.py new file mode 100644 index 0000000000..5d8cb4bb41 --- /dev/null +++ b/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'] \ No newline at end of file diff --git a/openedx/core/djangoapps/content/course_structures/models.py b/openedx/core/djangoapps/content/course_structures/models.py index 82acf61173..b28e416bb2 100644 --- a/openedx/core/djangoapps/content/course_structures/models.py +++ b/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 diff --git a/openedx/core/djangoapps/content/course_structures/signals.py b/openedx/core/djangoapps/content/course_structures/signals.py index 4070c5787e..18ebe3544c 100644 --- a/openedx/core/djangoapps/content/course_structures/signals.py +++ b/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) diff --git a/openedx/core/djangoapps/content/course_structures/tasks.py b/openedx/core/djangoapps/content/course_structures/tasks.py index 5ed989be33..d246adf17d 100644 --- a/openedx/core/djangoapps/content/course_structures/tasks.py +++ b/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() diff --git a/openedx/core/djangoapps/content/course_structures/tests.py b/openedx/core/djangoapps/content/course_structures/tests.py index c2f1b4f5cf..38e0237270 100644 --- a/openedx/core/djangoapps/content/course_structures/tests.py +++ b/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() + )