Merge pull request #8767 from edx/will/optimize-credit-reqs-update

Credit requirement optimizations

openedx/core/djangoapps/credit/signals.py

@@ -2,6 +2,8 @@
 This file contains receivers of course publication signals.
 """
 
+import logging
+
 from django.dispatch import receiver
 from django.utils import timezone
 from opaque_keys.edx.keys import CourseKey
@@ -10,6 +12,9 @@ from xmodule.modulestore.django import SignalHandler
 from openedx.core.djangoapps.signals.signals import GRADES_UPDATED
 
 
+log = logging.getLogger(__name__)
+
+
 @receiver(SignalHandler.course_published)
 def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=unused-argument
     """Receive 'course_published' signal and kick off a celery task to update
@@ -18,9 +23,11 @@ def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=
 
     # Import here, because signal is registered at startup, but items in tasks
     # are not yet able to be loaded
-    from .tasks import update_credit_course_requirements
+    from openedx.core.djangoapps.credit import api, tasks
 
-    update_credit_course_requirements.delay(unicode(course_key))
+    if api.is_credit_course(course_key):
+        tasks.update_credit_course_requirements.delay(unicode(course_key))
+        log.info(u'Added task to update credit requirements for course "%s" to the task queue', course_key)
 
 
 @receiver(GRADES_UPDATED)

openedx/core/djangoapps/credit/tasks.py

@@ -18,10 +18,20 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 LOGGER = get_task_logger(__name__)
 
 
+# XBlocks that can be added as credit requirements
+CREDIT_REQUIREMENT_XBLOCKS = [
+    {
+        "category": "edx-reverification-block",
+        "requirement-namespace": "reverification"
+    }
+]
+
+
 # pylint: disable=not-callable
 @task(default_retry_delay=settings.CREDIT_TASK_DEFAULT_RETRY_DELAY, max_retries=settings.CREDIT_TASK_MAX_RETRIES)
 def update_credit_course_requirements(course_id):   # pylint: disable=invalid-name
-    """Updates course requirements table for a course.
+    """
+    Updates course requirements table for a course.
 
      Args:
         course_id(str): A string representation of course identifier
@@ -34,8 +44,7 @@ def update_credit_course_requirements(course_id):   # pylint: disable=invalid-na
         course_key = CourseKey.from_string(course_id)
         is_credit_course = CreditCourse.is_credit_course(course_key)
         if is_credit_course:
-            course = modulestore().get_course(course_key)
-            requirements = _get_course_credit_requirements(course)
+            requirements = _get_course_credit_requirements(course_key)
             set_credit_requirements(course_key, requirements)
     except (InvalidKeyError, ItemNotFoundError, InvalidCreditRequirements) as exc:
         LOGGER.error('Error on adding the requirements for course %s - %s', course_id, unicode(exc))
@@ -44,41 +53,40 @@ def update_credit_course_requirements(course_id):   # pylint: disable=invalid-na
         LOGGER.info('Requirements added for course %s', course_id)
 
 
-def _get_course_credit_requirements(course):
-    """Returns the list of credit requirements for the given course.
+def _get_course_credit_requirements(course_key):
+    """
+    Returns the list of credit requirements for the given course.
 
     It returns the minimum_grade_credit and also the ICRV checkpoints
     if any were added in the course
 
     Args:
-        course(Course): The course object
+        course_key (CourseKey): Identifier for the course.
 
     Returns:
-        List of minimum_grade_credit and ICRV requirements
+        List of credit requirements (dictionaries)
 
     """
-    credit_xblock_requirements = _get_credit_course_requirement_xblocks(course)
-    min_grade_requirement = _get_min_grade_requirement(course)
+    credit_xblock_requirements = _get_credit_course_requirement_xblocks(course_key)
+    min_grade_requirement = _get_min_grade_requirement(course_key)
     credit_requirements = min_grade_requirement + credit_xblock_requirements
     return credit_requirements
 
 
-def _get_min_grade_requirement(course):
-    """Get list of 'minimum_grade_credit' requirement for the given course.
+def _get_min_grade_requirement(course_key):
+    """
+    Get list of 'minimum_grade_credit' requirement for the given course.
 
     Args:
-        course(Course): The course object
-
-    Raises:
-        AttributeError if the course has not 'minimum_grade_credit' attribute
+        course_key (CourseKey): Identifier for the course.
 
     Returns:
         The list of minimum_grade_credit requirements
 
     """
-    requirement = []
+    course = modulestore().get_course(course_key, depth=0)
     try:
-        requirement = [
+        return [
             {
                 "namespace": "grade",
                 "name": "grade",
@@ -90,41 +98,46 @@ def _get_min_grade_requirement(course):
         ]
     except AttributeError:
         LOGGER.error("The course %s does not has minimum_grade_credit attribute", unicode(course.id))
-    return requirement
+    else:
+        return []
 
 
-def _get_credit_course_requirement_xblocks(course):  # pylint: disable=invalid-name
+def _get_credit_course_requirement_xblocks(course_key):  # pylint: disable=invalid-name
     """Generate a course structure dictionary for the specified course.
 
     Args:
-        course(Course): The course object
+        course_key (CourseKey): Identifier for the course.
 
     Returns:
         The list of credit requirements xblocks dicts
 
     """
-    blocks_stack = [course]
-    requirements_blocks = []
-    while blocks_stack:
-        curr_block = blocks_stack.pop()
-        children = curr_block.get_children() if curr_block.has_children else []
-        if _is_credit_requirement(curr_block):
-            block = {
-                "namespace": curr_block.get_credit_requirement_namespace(),
-                "name": curr_block.get_credit_requirement_name(),
-                "display_name": curr_block.get_credit_requirement_display_name(),
-                "criteria": "",
+    requirements = []
+
+    # Retrieve all XBlocks from the course that we know to be credit requirements.
+    # For performance reasons, we look these up by their "category" to avoid
+    # loading and searching the entire course tree.
+    for desc in CREDIT_REQUIREMENT_XBLOCKS:
+        requirements.extend([
+            {
+                "namespace": desc["requirement-namespace"],
+                "name": block.get_credit_requirement_name(),
+                "display_name": block.get_credit_requirement_display_name(),
+                "criteria": {},
             }
-            requirements_blocks.append(block)
+            for block in modulestore().get_items(
+                course_key,
+                qualifiers={"category": desc["category"]}
+            )
+            if _is_credit_requirement(block)
+        ])
 
-        # Add the children of current block to the stack so that we can
-        # traverse them as well.
-        blocks_stack.extend(children)
-    return requirements_blocks
+    return requirements
 
 
 def _is_credit_requirement(xblock):
-    """Check if the given XBlock is a credit requirement.
+    """
+    Check if the given XBlock is a credit requirement.
 
     Args:
         xblock(XBlock): The given XBlock object
@@ -133,21 +146,18 @@ def _is_credit_requirement(xblock):
         True if XBlock is a credit requirement else False
 
     """
-    is_credit_requirement = False
-
-    if callable(getattr(xblock, "is_course_credit_requirement", None)):
-        is_credit_requirement = xblock.is_course_credit_requirement()
-        if is_credit_requirement:
-            if not callable(getattr(xblock, "get_credit_requirement_namespace", None)):
-                is_credit_requirement = False
-                LOGGER.error(
-                    "XBlock %s is marked as a credit requirement but does not "
-                    "implement get_credit_requirement_namespace()", unicode(xblock)
-                )
-            if not callable(getattr(xblock, "get_credit_requirement_name", None)):
-                is_credit_requirement = False
-                LOGGER.error(
-                    "XBlock %s is marked as a credit requirement but does not "
-                    "implement get_credit_requirement_name()", unicode(xblock)
-                )
-    return is_credit_requirement
+    if not callable(getattr(xblock, "get_credit_requirement_namespace", None)):
+        LOGGER.error(
+            "XBlock %s is marked as a credit requirement but does not "
+            "implement get_credit_requirement_namespace()", unicode(xblock)
+        )
+        return False
+
+    if not callable(getattr(xblock, "get_credit_requirement_name", None)):
+        LOGGER.error(
+            "XBlock %s is marked as a credit requirement but does not "
+            "implement get_credit_requirement_name()", unicode(xblock)
+        )
+        return False
+
+    return True

openedx/core/djangoapps/credit/tests/test_tasks.py

@@ -11,7 +11,7 @@ from openedx.core.djangoapps.credit.models import CreditCourse
 from openedx.core.djangoapps.credit.signals import listen_for_course_publish
 from xmodule.modulestore.django import SignalHandler
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls
 
 
 class TestTaskExecution(ModuleStoreTestCase):
@@ -85,6 +85,13 @@ class TestTaskExecution(ModuleStoreTestCase):
         requirements = get_credit_requirements(self.course.id)
         self.assertEqual(len(requirements), 2)
 
+    def test_query_counts(self):
+        self.add_credit_course(self.course.id)
+        self.add_icrv_xblock()
+
+        with check_mongo_calls(3):
+            listen_for_course_publish(self, self.course.id)
+
     @mock.patch(
         'openedx.core.djangoapps.credit.tasks.set_credit_requirements',
         mock.Mock(