From 89d916918f72eab3023efc5419319c6c3ab19e9c Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 17 Nov 2016 21:55:10 -0500 Subject: [PATCH] Handle DAGs with inaccessible Subsections on Problem Score update TNL-5886 --- lms/djangoapps/grades/tasks.py | 23 +++++++------ lms/djangoapps/grades/tests/test_new.py | 14 ++++---- lms/djangoapps/grades/tests/test_tasks.py | 34 ++++++++++++++++--- .../lib/block_structure/block_structure.py | 2 +- .../tests/test_block_structure.py | 1 + 5 files changed, 50 insertions(+), 24 deletions(-) diff --git a/lms/djangoapps/grades/tasks.py b/lms/djangoapps/grades/tasks.py index 4e05677a3d..bfe96bc286 100644 --- a/lms/djangoapps/grades/tasks.py +++ b/lms/djangoapps/grades/tasks.py @@ -110,17 +110,18 @@ def _update_subsection_grades( try: for subsection_usage_key in subsections_to_update: - subsection_grade = subsection_grade_factory.update( - course_structure[subsection_usage_key], - only_if_higher, - ) - SUBSECTION_SCORE_CHANGED.send( - sender=recalculate_subsection_grade, - course=course, - course_structure=course_structure, - user=student, - subsection_grade=subsection_grade, - ) + if subsection_usage_key in course_structure: + subsection_grade = subsection_grade_factory.update( + course_structure[subsection_usage_key], + only_if_higher, + ) + SUBSECTION_SCORE_CHANGED.send( + sender=recalculate_subsection_grade, + course=course, + course_structure=course_structure, + user=student, + subsection_grade=subsection_grade, + ) except DatabaseError as exc: raise _retry_recalculate_subsection_grade( diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index d0e306ac3e..63aa039197 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -434,12 +434,6 @@ class TestVariedMetadata(ProblemSubmissionTestMixin, ModuleStoreTestCase): self.request = get_mock_request(UserFactory()) self.client.login(username=self.request.user.username, password="test") CourseEnrollment.enroll(self.request.user, self.course.id) - course_structure = get_course_blocks(self.request.user, self.course.location) - self.subsection_factory = SubsectionGradeFactory( - self.request.user, - course_structure=course_structure, - course=self.course, - ) def _get_altered_metadata(self, alterations): """ @@ -471,7 +465,13 @@ class TestVariedMetadata(ProblemSubmissionTestMixin, ModuleStoreTestCase): """ self.submit_question_answer(u'problem', {u'2_1': u'Correct'}) - return self.subsection_factory.create(self.sequence) + course_structure = get_course_blocks(self.request.user, self.course.location) + subsection_factory = SubsectionGradeFactory( + self.request.user, + course_structure=course_structure, + course=self.course, + ) + return subsection_factory.create(self.sequence) @ddt.data( ({}, 1.25, 2.5), diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 943d241f44..df0f3c1a3c 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -49,8 +49,8 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): PersistentGradesEnabledFlag.objects.create(enabled=False) self.chapter = ItemFactory.create(parent=self.course, category="chapter", display_name="Chapter") - self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Open Sequential") - self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='problem') + self.sequential = ItemFactory.create(parent=self.chapter, category='sequential', display_name="Sequential1") + self.problem = ItemFactory.create(parent=self.sequential, category='problem', display_name='Problem') self.problem_score_changed_kwargs = OrderedDict([ ('points_earned', 1.0), @@ -109,13 +109,13 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): mock_task_apply.assert_called_once_with(args=expected_args) @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send') - def test_subsection_update_triggers_course_update(self, mock_course_signal): + def test_subsection_update_triggers_signal(self, mock_subsection_signal): """ - Ensures that the subsection update operation also updates the course grade. + Ensures that the subsection update operation triggers a signal. """ self.set_up_course() self._apply_recalculate_subsection_grade() - self.assertTrue(mock_course_signal.called) + self.assertTrue(mock_subsection_signal.called) @ddt.data( (ModuleStoreEnum.Type.mongo, 1), @@ -129,6 +129,30 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase): with check_mongo_calls(2) and self.assertNumQueries(20 + added_queries): self._apply_recalculate_subsection_grade() + @patch('lms.djangoapps.grades.signals.signals.SUBSECTION_SCORE_CHANGED.send') + def test_other_inaccessible_subsection(self, mock_subsection_signal): + self.set_up_course() + accessible_seq = ItemFactory.create(parent=self.chapter, category='sequential') + inaccessible_seq = ItemFactory.create(parent=self.chapter, category='sequential', visible_to_staff_only=True) + + # Update problem to have 2 additional sequential parents. + # So in total, 3 sequential parents, with one inaccessible. + for sequential in (accessible_seq, inaccessible_seq): + sequential.children = [self.problem.location] + modulestore().update_item(sequential, self.user.id) # pylint: disable=no-member + + # Make sure the signal is sent for only the 2 accessible sequentials. + self._apply_recalculate_subsection_grade() + self.assertEquals(mock_subsection_signal.call_count, 2) + sequentials_signalled = { + args[1]['subsection_grade'].location + for args in mock_subsection_signal.call_args_list + } + self.assertSetEqual( + sequentials_signalled, + {self.sequential.location, accessible_seq.location}, + ) + def test_single_call_to_create_block_structure(self): self.set_up_course() self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id)) diff --git a/openedx/core/lib/block_structure/block_structure.py b/openedx/core/lib/block_structure/block_structure.py index 81cd99a945..af2b5de0e7 100644 --- a/openedx/core/lib/block_structure/block_structure.py +++ b/openedx/core/lib/block_structure/block_structure.py @@ -445,7 +445,7 @@ class BlockStructureBlockData(BlockStructure): """ Returns the BlockData associated with the given key. """ - return self._block_data_map.get(usage_key) + return self._block_data_map[usage_key] def get_xblock_field(self, usage_key, field_name, default=None): """ diff --git a/openedx/core/lib/block_structure/tests/test_block_structure.py b/openedx/core/lib/block_structure/tests/test_block_structure.py index 77da8fad51..b7fb807d7d 100644 --- a/openedx/core/lib/block_structure/tests/test_block_structure.py +++ b/openedx/core/lib/block_structure/tests/test_block_structure.py @@ -131,6 +131,7 @@ class TestBlockStructureData(TestCase, ChildrenMapTestMixin): block_structure = BlockStructureModulestoreData(root_block_usage_key=0) for block in blocks: block_structure._add_xblock(block.location, block) + block_structure._get_or_create_block(block.location) # request fields fields = ["field1", "field2", "field3"]