From 47e556c7f5f7a76f48d569fc6f9f1290c8649e60 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 15 Sep 2016 09:07:22 -0400 Subject: [PATCH] Use explicit 'graded' value so grades are immune to subsection's value TNL-5560 --- lms/djangoapps/grades/new/subsection_grade.py | 19 +++++- .../grades/tests/test_transformer.py | 29 +++++++++- lms/djangoapps/grades/transformer.py | 58 ++++++++++++++++--- 3 files changed, 96 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index b9c6e140aa..1488c29b5c 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -11,6 +11,7 @@ from courseware.model_data import ScoresClient from lms.djangoapps.grades.scores import get_score, possibly_scored from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag +from lms.djangoapps.grades.transformer import GradesTransformer from student.models import anonymous_id_for_user, User from submissions import api as submissions_api from traceback import format_exc @@ -173,7 +174,7 @@ class SubsectionGrade(object): # # Cannot grade a problem with a denominator of 0. # TODO: None > 0 is not python 3 compatible. - block_graded = block.graded if possible > 0 else False + block_graded = self._get_explicit_graded(block, course_structure) if possible > 0 else False self.locations_to_weighted_scores[block.location] = ( Score( @@ -186,6 +187,22 @@ class SubsectionGrade(object): weight, ) + def _get_explicit_graded(self, block, course_structure): + """ + Returns the explicit graded field value for the given block + """ + field_value = course_structure.get_transformer_block_field( + block.location, + GradesTransformer, + GradesTransformer.EXPLICIT_GRADED_FIELD_NAME + ) + + # Set to True if grading is not explicitly disabled for + # this block. This allows us to include the block's score + # in the aggregated self.graded_total, regardless of the + # inherited graded value from the subsection. (TNL-5560) + return True if field_value is None else field_value + def _persisted_model_params(self, student): """ Returns the parameters for creating/updating the diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index d84f04336e..4ed2976ea6 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -6,6 +6,8 @@ import datetime import pytz import random +import ddt + from student.tests.factories import UserFactory from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -17,6 +19,7 @@ from openedx.core.djangoapps.content.block_structure.api import get_cache from ..transformer import GradesTransformer +@ddt.ddt class GradesTransformerTestCase(CourseStructureTestCase): """ Verify behavior of the GradesTransformer @@ -194,7 +197,7 @@ class GradesTransformerTestCase(CourseStructureTestCase): ) self.assertEqual(actual_subsections, {blocks[sub].location for sub in expected_subsections}) - def test_ungraded_block_collection(self): + def test_unscored_block_collection(self): blocks = self.build_course_with_problems() block_structure = get_course_blocks(self.student, blocks[u'course'].location, self.transformers) self.assert_collected_xblock_fields( @@ -211,6 +214,7 @@ class GradesTransformerTestCase(CourseStructureTestCase): blocks[u'course'].location, self.TRANSFORMER_CLASS_TO_TEST, max_score=None, + explicit_graded=None, ) def test_grades_collected_basic(self): @@ -227,6 +231,29 @@ class GradesTransformerTestCase(CourseStructureTestCase): due=self.problem_metadata[u'due'], format=None, ) + self.assert_collected_transformer_block_fields( + block_structure, + blocks[u'problem'].location, + self.TRANSFORMER_CLASS_TO_TEST, + max_score=0, + explicit_graded=True, + ) + + @ddt.data(True, False, None) + def test_graded_at_problem(self, graded): + problem_metadata = { + u'has_score': True, + } + if graded is not None: + problem_metadata[u'graded'] = graded + blocks = self.build_course_with_problems(metadata=problem_metadata) + block_structure = get_course_blocks(self.student, blocks[u'course'].location, self.transformers) + self.assert_collected_transformer_block_fields( + block_structure, + blocks[u'problem'].location, + self.TRANSFORMER_CLASS_TO_TEST, + explicit_graded=graded, + ) def test_collecting_staff_only_problem(self): # Demonstrate that the problem data can by collected by the SystemUser diff --git a/lms/djangoapps/grades/transformer.py b/lms/djangoapps/grades/transformer.py index 952bdc6dc7..df2e0a4b37 100644 --- a/lms/djangoapps/grades/transformer.py +++ b/lms/djangoapps/grades/transformer.py @@ -2,10 +2,11 @@ Grades Transformer """ from django.test.client import RequestFactory +from functools import reduce as functools_reduce from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor -from lms.djangoapps.course_blocks.transformers.utils import collect_unioned_set_field +from lms.djangoapps.course_blocks.transformers.utils import collect_unioned_set_field, get_field_on_block from openedx.core.lib.block_structure.transformer import BlockStructureTransformer from openedx.core.djangoapps.util.user_utils import SystemUser @@ -17,8 +18,8 @@ class GradesTransformer(BlockStructureTransformer): No runtime transformations are performed. - The following values are stored as xblock_fields on their respective blocks in the - block structure: + The following values are stored as xblock_fields on their respective blocks + in the block structure: due: (datetime) when the problem is due. format: (string) what type of problem it is @@ -26,14 +27,16 @@ class GradesTransformer(BlockStructureTransformer): has_score: (boolean) weight: (numeric) - Additionally, the following value is calculated and stored as a transformer_block_field - for each block: + Additionally, the following value is calculated and stored as a + transformer_block_field for each block: max_score: (numeric) """ - VERSION = 3 + VERSION = 4 FIELDS_TO_COLLECT = [u'due', u'format', u'graded', u'has_score', u'weight', u'course_version', u'subtree_edited_on'] + EXPLICIT_GRADED_FIELD_NAME = 'explicit_graded' + @classmethod def name(cls): """ @@ -56,6 +59,7 @@ class GradesTransformer(BlockStructureTransformer): merged_field_name='subsections', filter_by=lambda block_key: block_key.block_type == 'sequential', ) + cls._collect_explicit_graded(block_structure) def transform(self, block_structure, usage_context): """ @@ -63,6 +67,44 @@ class GradesTransformer(BlockStructureTransformer): """ pass + @classmethod + def _collect_explicit_graded(cls, block_structure): + """ + Collect the 'explicit_graded' field for every block. + """ + def _set_field(block_key, field_value): + """ + Sets the explicit graded field to the given value for the + given block. + """ + block_structure.set_transformer_block_field(block_key, cls, cls.EXPLICIT_GRADED_FIELD_NAME, field_value) + + def _get_field(block_key): + """ + Gets the explicit graded field to the given value for the + given block. + """ + return block_structure.get_transformer_block_field(block_key, cls, cls.EXPLICIT_GRADED_FIELD_NAME) + + block_types_to_ignore = {'course', 'chapter', 'sequential'} + + for block_key in block_structure.topological_traversal(): + if block_key.block_type in block_types_to_ignore: + _set_field(block_key, None) + else: + explicit_field_on_block = get_field_on_block(block_structure.get_xblock(block_key), 'graded') + if explicit_field_on_block is not None: + _set_field(block_key, explicit_field_on_block) + else: + values_from_parents = [ + _get_field(parent) + for parent in block_structure.get_parents(block_key) + if parent.block_type not in block_types_to_ignore + ] + non_null_values_from_parents = [value for value in values_from_parents if not None] + explicit_from_parents = functools_reduce(lambda x, y: x or y, non_null_values_from_parents, None) + _set_field(block_key, explicit_from_parents) + @classmethod def _collect_max_scores(cls, block_structure): """ @@ -83,8 +125,8 @@ class GradesTransformer(BlockStructureTransformer): @staticmethod def _iter_scorable_xmodules(block_structure): """ - Loop through all the blocks locators in the block structure, and retrieve - the module (XModule or XBlock) associated with that locator. + Loop through all the blocks locators in the block structure, and + retrieve the module (XModule or XBlock) associated with that locator. For implementation reasons, we need to pull the max_score from the XModule, even though the data is not user specific. Here we bind the