From 84ba4ec6358806077defdc636e4fe0499e04a5b3 Mon Sep 17 00:00:00 2001 From: Matthew Piatetsky Date: Tue, 19 Jan 2021 10:00:07 -0500 Subject: [PATCH] Only set due dates on subsections that contain graded assignments with a score and a nonzero weight Previously we were only checking the graded attribute which doesn't account for ungraded assignments https://openedx.atlassian.net/browse/AA-497 --- .../course_date_signals/handlers.py | 20 ++++- .../djangoapps/course_date_signals/tests.py | 75 +++++++++++++++++++ openedx/core/lib/graph_traversals.py | 20 +++++ 3 files changed, 114 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/course_date_signals/handlers.py b/openedx/core/djangoapps/course_date_signals/handlers.py index d656a9456a..c4aaade206 100644 --- a/openedx/core/djangoapps/course_date_signals/handlers.py +++ b/openedx/core/djangoapps/course_date_signals/handlers.py @@ -6,6 +6,9 @@ import logging from django.dispatch import receiver from edx_when.api import FIELDS_TO_EXTRACT, set_dates_for_course from six import text_type + +from common.lib.xmodule.xmodule.util.misc import is_xblock_an_assignment +from openedx.core.lib.graph_traversals import get_children, leaf_filter, traverse_pre_order from xblock.fields import Scope from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import SignalHandler, modulestore @@ -38,6 +41,17 @@ def _field_values(fields, xblock): return result +def _has_assignment_blocks(item): + """ + Check if a given block contains children that are assignments. + Assignments have graded, has_score and nonzero weight attributes. + """ + return any( + is_xblock_an_assignment(block) + for block in traverse_pre_order(item, get_children, leaf_filter) + ) + + def _gather_graded_items(root, due): items = [root] has_non_ora_scored_content = False @@ -45,8 +59,12 @@ def _gather_graded_items(root, due): while items: next_item = items.pop() if next_item.graded: + # Sequentials can be marked as graded, while only containing ungraded problems + # To handle this case, we can look at the leaf blocks within a sequential + # and check that they are a graded assignment + # (if they have graded/has_score attributes and nonzero weight) # TODO: Once studio can manually set relative dates, we would need to manually check for them here - collected_items.append((next_item.location, {'due': due})) + collected_items.append((next_item.location, {'due': due if _has_assignment_blocks(next_item) else None})) # TODO: This is pretty gross, and should maybe be configurable in the future, # especially if we find ourselves needing more exceptions. has_non_ora_scored_content = ( diff --git a/openedx/core/djangoapps/course_date_signals/tests.py b/openedx/core/djangoapps/course_date_signals/tests.py index 5001d11bd2..182504ce7a 100644 --- a/openedx/core/djangoapps/course_date_signals/tests.py +++ b/openedx/core/djangoapps/course_date_signals/tests.py @@ -2,6 +2,8 @@ from datetime import timedelta import ddt from unittest.mock import patch +from openedx.core.djangoapps.course_date_signals.handlers import _gather_graded_items, _has_assignment_blocks +from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from . import utils @@ -40,3 +42,76 @@ class SelfPacedDueDatesTests(ModuleStoreTestCase): actual = [(idx, section.display_name, offset) for (idx, section, offset) in utils.spaced_out_sections(self.course)] self.assertEqual(actual, expected_sections) + + def test_dates_for_ungraded_assignments(self): + """ + _has_assignment_blocks should return true if the argument block + children leaf nodes include an assignment that is graded and scored + """ + with modulestore().bulk_operations(self.course.id): + sequence = ItemFactory(parent=self.course, category="sequential") + vertical = ItemFactory(parent=sequence, category="vertical") + sequence = modulestore().get_item(sequence.location) + self.assertEqual(_has_assignment_blocks(sequence), False) + + # Ungraded problems do not count as assignment blocks + ItemFactory.create( + parent=vertical, + category='problem', + graded=True, + weight=0, + ) + sequence = modulestore().get_item(sequence.location) + self.assertEqual(_has_assignment_blocks(sequence), False) + ItemFactory.create( + parent=vertical, + category='problem', + graded=False, + weight=1, + ) + sequence = modulestore().get_item(sequence.location) + self.assertEqual(_has_assignment_blocks(sequence), False) + + # Method will return true after adding a graded, scored assignment block + ItemFactory.create( + parent=vertical, + category='problem', + graded=True, + weight=1, + ) + sequence = modulestore().get_item(sequence.location) + self.assertEqual(_has_assignment_blocks(sequence), True) + + def test_sequence_with_graded_and_ungraded_assignments(self): + """ + _gather_graded_items should set a due date of None on ungraded problem blocks + even if the block has graded siblings in the sequence + """ + with modulestore().bulk_operations(self.course.id): + sequence = ItemFactory(parent=self.course, category="sequential") + vertical = ItemFactory(parent=sequence, category="vertical") + sequence = modulestore().get_item(sequence.location) + ItemFactory.create( + parent=vertical, + category='problem', + graded=False, + weight=1, + ) + ungraded_problem_2 = ItemFactory.create( + parent=vertical, + category='problem', + graded=True, + weight=0, + ) + graded_problem_1 = ItemFactory.create( + parent=vertical, + category='problem', + graded=True, + weight=1, + ) + expected_graded_items = [ + (ungraded_problem_2.location, {'due': None}), + (graded_problem_1.location, {'due': 5}), + ] + sequence = modulestore().get_item(sequence.location) + self.assertCountEqual(_gather_graded_items(sequence, 5), expected_graded_items) diff --git a/openedx/core/lib/graph_traversals.py b/openedx/core/lib/graph_traversals.py index a19a6afa90..b700a8c18a 100644 --- a/openedx/core/lib/graph_traversals.py +++ b/openedx/core/lib/graph_traversals.py @@ -333,3 +333,23 @@ def _traverse_generic( # Keep track of whether or not the node was yielded so we # know whether or not to yield its children. yield_results[current_node] = should_yield_node + + +def leaf_filter(block): + """ + Filter for traversals to find leaf blocks + """ + return ( + block.location.block_type not in ('chapter', 'sequential', 'vertical') and + len(block.get_children()) == 0 + ) + + +def get_children(parent): + """ + Function for traversals to get the children of a block + """ + if parent.has_children: + return parent.get_children() + else: + return []