From 65367a2ded9522a9de849adb138fd9d375ebb779 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Sun, 19 Jul 2015 16:34:15 -0700 Subject: [PATCH] ECOM-1911: Fixes to credit requirements in Studio. * Exclude orphaned XBlocks from requirements because they have been deleted. * Sort ICRV requirements by start date, then by display name. --- openedx/core/djangoapps/credit/models.py | 3 +- openedx/core/djangoapps/credit/tasks.py | 85 +++++++++++++----- .../djangoapps/credit/tests/test_tasks.py | 90 ++++++++++++++++--- 3 files changed, 143 insertions(+), 35 deletions(-) diff --git a/openedx/core/djangoapps/credit/models.py b/openedx/core/djangoapps/credit/models.py index 11c82d6e13..c1c653ac2e 100644 --- a/openedx/core/djangoapps/credit/models.py +++ b/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) diff --git a/openedx/core/djangoapps/credit/tasks.py b/openedx/core/djangoapps/credit/tasks.py index 97f36a2b69..136dd8a8f0 100644 --- a/openedx/core/djangoapps/credit/tasks.py +++ b/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 diff --git a/openedx/core/djangoapps/credit/tests/test_tasks.py b/openedx/core/djangoapps/credit/tests/test_tasks.py index 216a410ca7..3959168b7b 100644 --- a/openedx/core/djangoapps/credit/tests/test_tasks.py +++ b/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(