diff --git a/common/djangoapps/embargo/tests/test_api.py b/common/djangoapps/embargo/tests/test_api.py index 4d96ddc029..551f910536 100644 --- a/common/djangoapps/embargo/tests/test_api.py +++ b/common/djangoapps/embargo/tests/test_api.py @@ -188,6 +188,7 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase): mock_ip.return_value = country_code yield + @ddt.ddt @override_settings(MODULESTORE=MODULESTORE_CONFIG) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') diff --git a/lms/djangoapps/course_structure_api/v0/tests.py b/lms/djangoapps/course_structure_api/v0/tests.py index d5f2825080..9426126217 100644 --- a/lms/djangoapps/course_structure_api/v0/tests.py +++ b/lms/djangoapps/course_structure_api/v0/tests.py @@ -18,7 +18,8 @@ from xmodule.modulestore.xml import CourseLocationManager from xmodule.tests import get_test_system from courseware.tests.factories import GlobalStaffFactory, StaffFactory -from openedx.core.djangoapps.content.course_structures.models import CourseStructure, update_course_structure +from openedx.core.djangoapps.content.course_structures.models import CourseStructure +from openedx.core.djangoapps.content.course_structures.tasks import update_course_structure TEST_SERVER_HOST = 'http://testserver' diff --git a/lms/djangoapps/course_structure_api/v0/views.py b/lms/djangoapps/course_structure_api/v0/views.py index a50133a2d9..ef358daa7a 100644 --- a/lms/djangoapps/course_structure_api/v0/views.py +++ b/lms/djangoapps/course_structure_api/v0/views.py @@ -14,7 +14,7 @@ from opaque_keys.edx.keys import CourseKey from course_structure_api.v0 import serializers from courseware import courses from courseware.access import has_access -from openedx.core.djangoapps.content.course_structures import models +from openedx.core.djangoapps.content.course_structures import models, tasks from openedx.core.lib.api.permissions import IsAuthenticatedOrDebug from openedx.core.lib.api.serializers import PaginationSerializer from student.roles import CourseInstructorRole, CourseStaffRole @@ -191,7 +191,7 @@ class CourseStructure(CourseViewMixin, RetrieveAPIView): return super(CourseStructure, self).retrieve(request, *args, **kwargs) except models.CourseStructure.DoesNotExist: # If we don't have data stored, generate it and return a 503. - models.update_course_structure.delay(unicode(self.course.id)) + tasks.update_course_structure.delay(unicode(self.course.id)) return Response(status=503, headers={'Retry-After': '120'}) def get_object(self, queryset=None): diff --git a/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py b/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py index b8b0eb4dc5..1ef34ae2a9 100644 --- a/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py +++ b/openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py @@ -2,14 +2,13 @@ import logging from optparse import make_option from django.core.management.base import BaseCommand - from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore -from openedx.core.djangoapps.content.course_structures.models import update_course_structure +from openedx.core.djangoapps.content.course_structures.tasks import update_course_structure -logger = logging.getLogger(__name__) +log = logging.getLogger(__name__) class Command(BaseCommand): @@ -31,16 +30,21 @@ class Command(BaseCommand): course_keys = [CourseKey.from_string(arg) for arg in args] if not course_keys: - logger.fatal('No courses specified.') + log.fatal('No courses specified.') return - logger.info('Generating course structures for %d courses.', len(course_keys)) - logging.debug('Generating course structure(s) for the following courses: %s', course_keys) + log.info('Generating course structures for %d courses.', len(course_keys)) + log.debug('Generating course structure(s) for the following courses: %s', course_keys) for course_key in course_keys: try: - update_course_structure(unicode(course_key)) - except Exception as e: - logger.error('An error occurred while generating course structure for %s: %s', unicode(course_key), e) + # Run the update task synchronously so that we know when all course structures have been updated. + # TODO Future improvement: Use .delay(), add return value to ResultSet, and wait for execution of + # all tasks using ResultSet.join(). I (clintonb) am opting not to make this improvement right now + # as I do not have time to test it fully. + update_course_structure.apply(unicode(course_key)) + except Exception as ex: + log.exception('An error occurred while generating course structure for %s: %s', + unicode(course_key), ex.message) - logger.info('Finished generating course structures.') + log.info('Finished generating course structures.') diff --git a/openedx/core/djangoapps/content/course_structures/models.py b/openedx/core/djangoapps/content/course_structures/models.py index ebbd320c4f..2dcbccbc1a 100644 --- a/openedx/core/djangoapps/content/course_structures/models.py +++ b/openedx/core/djangoapps/content/course_structures/models.py @@ -1,11 +1,7 @@ import json import logging -from celery.task import task -from django.dispatch import receiver from model_utils.models import TimeStampedModel -from opaque_keys.edx.keys import CourseKey -from xmodule.modulestore.django import modulestore, SignalHandler from util.models import CompressedTextField from xmodule_django.models import CourseKeyField @@ -30,66 +26,6 @@ class CourseStructure(TimeStampedModel): return json.loads(self.structure_json) return None - -def generate_course_structure(course_key): - """ - Generates a course structure dictionary for the specified course. - """ - course = modulestore().get_course(course_key, depth=None) - blocks_stack = [course] - blocks_dict = {} - while blocks_stack: - curr_block = blocks_stack.pop() - children = curr_block.get_children() if curr_block.has_children else [] - blocks_dict[unicode(curr_block.scope_ids.usage_id)] = { - "usage_key": unicode(curr_block.scope_ids.usage_id), - "block_type": curr_block.category, - "display_name": curr_block.display_name, - "graded": curr_block.graded, - "format": curr_block.format, - "children": [unicode(child.scope_ids.usage_id) for child in children] - } - blocks_stack.extend(children) - return { - "root": unicode(course.scope_ids.usage_id), - "blocks": blocks_dict - } - - -@receiver(SignalHandler.course_published) -def listen_for_course_publish(sender, course_key, **kwargs): - # 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.delay(unicode(course_key), countdown=0) - - -@task(name=u'openedx.core.djangoapps.content.course_structures.models.update_course_structure') -def update_course_structure(course_key): - """ - Regenerates and updates the course structure (in the database) for the specified course. - """ - # Ideally we'd like to accept a CourseLocator; however, CourseLocator is not JSON-serializable (by default) so - # Celery's delayed tasks fail to start. For this reason, callers should pass the course key as a Unicode string. - if not isinstance(course_key, basestring): - raise ValueError('course_key must be a string. {} is not acceptable.'.format(type(course_key))) - - course_key = CourseKey.from_string(course_key) - - try: - structure = generate_course_structure(course_key) - except Exception as e: - logger.error('An error occurred while generating course structure: %s', e) - raise - - structure_json = json.dumps(structure) - - cs, created = CourseStructure.objects.get_or_create( - course_id=course_key, - defaults={'structure_json': structure_json} - ) - - if not created: - cs.structure_json = structure_json - cs.save() - - return cs +# Signals must be imported in a file that is automatically loaded at app startup (e.g. models.py). We import them +# at the end of this file to avoid circular dependencies. +import signals # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/content/course_structures/signals.py b/openedx/core/djangoapps/content/course_structures/signals.py new file mode 100644 index 0000000000..966dcd05e8 --- /dev/null +++ b/openedx/core/djangoapps/content/course_structures/signals.py @@ -0,0 +1,13 @@ +from django.dispatch.dispatcher import receiver + +from xmodule.modulestore.django import SignalHandler + + +@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 + + # 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.delay(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 new file mode 100644 index 0000000000..f621cbe25f --- /dev/null +++ b/openedx/core/djangoapps/content/course_structures/tasks.py @@ -0,0 +1,79 @@ +import json +import logging + +from celery.task import task +from opaque_keys.edx.keys import CourseKey +from xmodule.modulestore.django import modulestore + + +log = logging.getLogger('edx.celery.task') + + +def _generate_course_structure(course_key): + """ + Generates a course structure dictionary for the specified course. + """ + course = modulestore().get_course(course_key, depth=None) + blocks_stack = [course] + blocks_dict = {} + while blocks_stack: + curr_block = blocks_stack.pop() + children = curr_block.get_children() if curr_block.has_children else [] + key = unicode(curr_block.scope_ids.usage_id) + block = { + "usage_key": key, + "block_type": curr_block.category, + "display_name": curr_block.display_name, + "children": [unicode(child.scope_ids.usage_id) for child in children] + } + + # Retrieve these attributes separately so that we can fail gracefully if the block doesn't have the attribute. + attrs = (('graded', False), ('format', None)) + for attr, default in attrs: + if hasattr(curr_block, attr): + block[attr] = getattr(curr_block, attr, default) + else: + log.warning('Failed to retrieve %s attribute of block %s. Defaulting to %s.', attr, key, default) + block[attr] = default + + blocks_dict[key] = block + + # 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 + } + + +@task(name=u'openedx.core.djangoapps.content.course_structures.tasks.update_course_structure') +def update_course_structure(course_key): + """ + Regenerates and updates the course structure (in the database) for the specified course. + """ + # Import here to avoid circular import. + from .models import CourseStructure + + # Ideally we'd like to accept a CourseLocator; however, CourseLocator is not JSON-serializable (by default) so + # Celery's delayed tasks fail to start. For this reason, callers should pass the course key as a Unicode string. + if not isinstance(course_key, basestring): + raise ValueError('course_key must be a string. {} is not acceptable.'.format(type(course_key))) + + course_key = CourseKey.from_string(course_key) + + try: + structure = _generate_course_structure(course_key) + except Exception as ex: + log.exception('An error occurred while generating course structure: %s', ex.message) + raise + + structure_json = json.dumps(structure) + + cs, created = CourseStructure.objects.get_or_create( + course_id=course_key, + defaults={'structure_json': structure_json} + ) + + if not created: + cs.structure_json = structure_json + cs.save() diff --git a/openedx/core/djangoapps/content/course_structures/tests.py b/openedx/core/djangoapps/content/course_structures/tests.py index 4c279a4114..93fc780d11 100644 --- a/openedx/core/djangoapps/content/course_structures/tests.py +++ b/openedx/core/djangoapps/content/course_structures/tests.py @@ -1,13 +1,15 @@ import json + from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from openedx.core.djangoapps.content.course_structures.models import generate_course_structure, CourseStructure +from openedx.core.djangoapps.content.course_structures.models import CourseStructure +from openedx.core.djangoapps.content.course_structures.tasks import _generate_course_structure, update_course_structure -class CourseStructureTests(ModuleStoreTestCase): +class CourseStructureTaskTests(ModuleStoreTestCase): def setUp(self, **kwargs): - super(CourseStructureTests, self).setUp() + super(CourseStructureTaskTests, self).setUp() self.course = CourseFactory.create() self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section') CourseStructure.objects.all().delete() @@ -38,7 +40,7 @@ class CourseStructureTests(ModuleStoreTestCase): } self.maxDiff = None - actual = generate_course_structure(self.course.id) + actual = _generate_course_structure(self.course.id) self.assertDictEqual(actual, expected) def test_structure_json(self): @@ -77,3 +79,41 @@ class CourseStructureTests(ModuleStoreTestCase): structure_json = json.dumps(structure) cs = CourseStructure.objects.create(course_id=self.course.id, structure_json=structure_json) self.assertDictEqual(cs.structure, structure) + + def test_block_with_missing_fields(self): + """ + The generator should continue to operate on blocks/XModule that do not have graded or format fields. + """ + # TODO In the future, test logging using testfixtures.LogCapture + # (https://pythonhosted.org/testfixtures/logging.html). Talk to TestEng before adding that library. + category = 'peergrading' + display_name = 'Testing Module' + module = ItemFactory.create(parent=self.section, category=category, display_name=display_name) + structure = _generate_course_structure(self.course.id) + + usage_key = unicode(module.location) + actual = structure['blocks'][usage_key] + expected = { + "usage_key": usage_key, + "block_type": category, + "display_name": display_name, + "graded": False, + "format": None, + "children": [] + } + self.assertEqual(actual, expected) + + def test_update_course_structure(self): + """ + Test the actual task that orchestrates data generation and updating the database. + """ + # Method requires string input + course_id = self.course.id + self.assertRaises(ValueError, update_course_structure, course_id) + + # Ensure a CourseStructure object is created + 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)