Merge pull request #8984 from edx/will/ecom-1911

ECOM-1911: Fixes to credit requirements in Studio.

openedx/core/djangoapps/credit/models.py

@@ -286,6 +286,7 @@ class CreditRequirement(TimeStampedModel):
         Model metadata.
         """
         unique_together = ('namespace', 'name', 'course')
+        ordering = ["order"]
 
     @classmethod
     def add_or_update_course_requirement(cls, credit_course, requirement, order):
@@ -337,7 +338,7 @@ class CreditRequirement(TimeStampedModel):
 
         """
         # order credit requirements according to their appearance in courseware
-        requirements = CreditRequirement.objects.filter(course__course_key=course_key, active=True).order_by("-order")
+        requirements = CreditRequirement.objects.filter(course__course_key=course_key, active=True)
 
         if namespace is not None:
             requirements = requirements.filter(namespace=namespace)

openedx/core/djangoapps/credit/tasks.py

@@ -2,6 +2,9 @@
 This file contains celery tasks for credit course views.
 """
 
+import datetime
+from pytz import UTC
+
 from django.conf import settings
 
 from celery import task
@@ -13,17 +16,15 @@ from .api import set_credit_requirements
 from openedx.core.djangoapps.credit.exceptions import InvalidCreditRequirements
 from openedx.core.djangoapps.credit.models import CreditCourse
 from xmodule.modulestore.django import modulestore
+from xmodule.modulestore import ModuleStoreEnum
 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"
-    }
+CREDIT_REQUIREMENT_XBLOCK_CATEGORIES = [
+    "edx-reverification-block",
 ]
 
 
@@ -123,24 +124,62 @@ def _get_credit_course_requirement_xblocks(course_key):  # pylint: disable=inval
     # 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:
+    for category in CREDIT_REQUIREMENT_XBLOCK_CATEGORIES:
         requirements.extend([
             {
-                "namespace": desc["requirement-namespace"],
+                "namespace": block.get_credit_requirement_namespace(),
                 "name": block.get_credit_requirement_name(),
                 "display_name": block.get_credit_requirement_display_name(),
                 "criteria": {},
             }
-            for block in modulestore().get_items(
-                course_key,
-                qualifiers={"category": desc["category"]}
-            )
+            for block in _get_xblocks(course_key, category)
             if _is_credit_requirement(block)
         ])
 
     return requirements
 
 
+def _is_in_course_tree(block):
+    """
+    Check that the XBlock is in the course tree.
+
+    It's possible that the XBlock is not in the course tree
+    if its parent has been deleted and is now an orphan.
+    """
+    ancestor = block.get_parent()
+    while ancestor is not None and ancestor.location.category != "course":
+        ancestor = ancestor.get_parent()
+
+    return ancestor is not None
+
+
+def _get_xblocks(course_key, category):
+    """
+    Retrieve all XBlocks in the course for a particular category.
+
+    Returns only XBlocks that are published and haven't been deleted.
+    """
+    xblocks = [
+        block for block in modulestore().get_items(
+            course_key,
+            qualifiers={"category": category},
+            revision=ModuleStoreEnum.RevisionOption.published_only,
+        )
+        if _is_in_course_tree(block)
+    ]
+
+    # Secondary sort on credit requirement name
+    xblocks = sorted(xblocks, key=lambda block: block.get_credit_requirement_display_name())
+
+    # Primary sort on start date
+    xblocks = sorted(xblocks, key=lambda block: (
+        block.start if block.start is not None
+        else datetime.datetime(datetime.MINYEAR, 1, 1).replace(tzinfo=UTC)
+    ))
+
+    return xblocks
+
+
 def _is_credit_requirement(xblock):
     """
     Check if the given XBlock is a credit requirement.
@@ -152,19 +191,19 @@ def _is_credit_requirement(xblock):
         True if XBlock is a credit requirement else False
 
     """
-    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
+    required_methods = [
+        "get_credit_requirement_namespace",
+        "get_credit_requirement_name",
+        "get_credit_requirement_display_name"
+    ]
 
-    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
+    for method_name in required_methods:
+        if not callable(getattr(xblock, method_name, None)):
+            LOGGER.error(
+                "XBlock %s is marked as a credit requirement but does not "
+                "implement %s", unicode(xblock), method_name
+            )
+            return False
 
     return True
 

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

@@ -3,14 +3,16 @@ Tests for credit course tasks.
 """
 
 import mock
-from datetime import datetime
+from datetime import datetime, timedelta
 
+from pytz import UTC
 from openedx.core.djangoapps.credit.api import get_credit_requirements
 from openedx.core.djangoapps.credit.exceptions import InvalidCreditRequirements
 from openedx.core.djangoapps.credit.models import CreditCourse
 from openedx.core.djangoapps.credit.signals import on_course_publish
+from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls_range
 
 from edx_proctoring.api import create_exam
 
@@ -30,22 +32,32 @@ class TestTaskExecution(ModuleStoreTestCase):
         """
         raise InvalidCreditRequirements
 
-    def add_icrv_xblock(self):
+    def add_icrv_xblock(self, related_assessment_name=None, start_date=None):
         """ Create the 'edx-reverification-block' in course tree """
-
-        section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section')
-        subsection = ItemFactory.create(parent=section, category='sequential', display_name='Test Subsection')
-        vertical = ItemFactory.create(parent=subsection, category='vertical', display_name='Test Unit')
-        ItemFactory.create(
-            parent=vertical,
+        block = ItemFactory.create(
+            parent=self.vertical,
             category='edx-reverification-block',
-            display_name='Test Verification Block'
         )
 
+        if related_assessment_name is not None:
+            block.related_assessment = related_assessment_name
+
+        block.start = start_date
+
+        self.store.update_item(block, ModuleStoreEnum.UserID.test)
+
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.course.id):
+            self.store.publish(block.location, ModuleStoreEnum.UserID.test)
+
+        return block
+
     def setUp(self):
         super(TestTaskExecution, self).setUp()
 
         self.course = CourseFactory.create(start=datetime(2015, 3, 1))
+        self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section')
+        self.subsection = ItemFactory.create(parent=self.section, category='sequential', display_name='Test Subsection')
+        self.vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='Test Unit')
 
     def test_task_adding_requirements_invalid_course(self):
         """
@@ -172,9 +184,65 @@ class TestTaskExecution(ModuleStoreTestCase):
         self.add_credit_course(self.course.id)
         self.add_icrv_xblock()
 
-        with check_mongo_calls(3):
+        with check_mongo_calls_range(max_finds=7):
             on_course_publish(self.course.id)
 
+    def test_remove_icrv_requirement(self):
+        self.add_credit_course(self.course.id)
+        self.add_icrv_xblock()
+        on_course_publish(self.course.id)
+
+        # There should be one ICRV requirement
+        requirements = get_credit_requirements(self.course.id, namespace="reverification")
+        self.assertEqual(len(requirements), 1)
+
+        # Delete the parent section containing the ICRV block
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.course.id):
+            self.store.delete_item(self.subsection.location, ModuleStoreEnum.UserID.test)
+
+        # Check that the ICRV block is no longer visible in the requirements
+        on_course_publish(self.course.id)
+        requirements = get_credit_requirements(self.course.id, namespace="reverification")
+        self.assertEqual(len(requirements), 0)
+
+    def test_icrv_requirement_ordering(self):
+        self.add_credit_course(self.course.id)
+
+        # Create multiple ICRV blocks
+        start = datetime.now(UTC)
+        self.add_icrv_xblock(related_assessment_name="Midterm A", start_date=start)
+
+        start = start - timedelta(days=1)
+        self.add_icrv_xblock(related_assessment_name="Midterm B", start_date=start)
+
+        # Primary sort is based on start date
+        on_course_publish(self.course.id)
+        requirements = get_credit_requirements(self.course.id, namespace="reverification")
+        self.assertEqual(len(requirements), 2)
+        self.assertEqual(requirements[0]["display_name"], "Midterm B")
+        self.assertEqual(requirements[1]["display_name"], "Midterm A")
+
+        # Add two additional ICRV blocks that have no start date
+        # and the same name.
+        start = datetime.now(UTC)
+        first_block = self.add_icrv_xblock(related_assessment_name="Midterm Start Date")
+
+        start = start + timedelta(days=1)
+        second_block = self.add_icrv_xblock(related_assessment_name="Midterm Start Date")
+
+        on_course_publish(self.course.id)
+        requirements = get_credit_requirements(self.course.id, namespace="reverification")
+        self.assertEqual(len(requirements), 4)
+        self.assertEqual(requirements[0]["display_name"], "Midterm Start Date")
+        self.assertEqual(requirements[1]["display_name"], "Midterm Start Date")
+        self.assertEqual(requirements[2]["display_name"], "Midterm B")
+        self.assertEqual(requirements[3]["display_name"], "Midterm A")
+
+        # Since the first two requirements have the same display name,
+        # we need to also check that their internal names (locations) are the same.
+        self.assertEqual(requirements[0]["name"], first_block.get_credit_requirement_name())
+        self.assertEqual(requirements[1]["name"], second_block.get_credit_requirement_name())
+
     @mock.patch(
         'openedx.core.djangoapps.credit.tasks.set_credit_requirements',
         mock.Mock(