Merge pull request #7281 from edx/clintonb/task-fix

Reorganized course_structures app

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')

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'

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):

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)
-
-        logger.info('Finished generating course structures.')
+                # 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)
+
+        log.info('Finished generating course structures.')

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

 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

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

+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)

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

+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()

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

 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)