From 6f773d1ecadebb6cf8af0879c0a1af4a5175342f Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Thu, 13 May 2021 14:06:11 -0400 Subject: [PATCH] feat: AA-802: Update the BlockCompletionTransformer We discovered a subsection that contained a unit without any content inside, but because of our logic requiring children, it would never be marked complete (meaning the subsection, section, and course could thus never be marked complete). This fixes that by removing the children check from setting completion, but first gating that code path on the xblock being an aggregator (to prevent leaves from marking as true simply because there are no children). Test fixes include adding a test for the empty aggregator case as well as some changes to not have an entire course marked complete because they are all empty aggregators. --- lms/djangoapps/course_api/blocks/api.py | 7 ++ .../blocks/transformers/block_completion.py | 18 ++-- .../tests/test_block_completion.py | 88 ++++++++++++------- .../tests/views/test_course_outline.py | 38 ++++---- 4 files changed, 94 insertions(+), 57 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index a52ca4c1e9..dd46cb846a 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -88,6 +88,13 @@ def get_blocks( HiddenContentTransformer() ] + # Note: A change to the BlockCompletionTransformer (https://github.com/edx/edx-platform/pull/27622/) + # will be introducing a bug if hide_access_denials is True. I'm accepting this risk because in + # the AccessDeniedMessageFilterTransformer, there is note about deleting it and I believe it is + # technically deprecated functionality. The only use case where hide_access_denials is True + # (outside of explicitly setting the temporary waffle flag) is in lms/djangoapps/course_api/blocks/urls.py + # for a v1 api that I also believe should have been deprecated and removed. When this code is removed, + # please also remove this comment. Thanks! if hide_access_denials: transformers += [AccessDeniedMessageFilterTransformer()] diff --git a/lms/djangoapps/course_api/blocks/transformers/block_completion.py b/lms/djangoapps/course_api/blocks/transformers/block_completion.py index 65bd3738e5..472555c4c7 100644 --- a/lms/djangoapps/course_api/blocks/transformers/block_completion.py +++ b/lms/djangoapps/course_api/blocks/transformers/block_completion.py @@ -71,17 +71,17 @@ class BlockCompletionTransformer(BlockStructureTransformer): block_structure.override_xblock_field(block_key, self.COMPLETE, True) if str(block_key) == str(latest_complete_block_key): block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True) + elif block_structure.get_xblock_field(block_key, 'completion_mode') == CompletionMode.AGGREGATOR: + children = block_structure.get_children(block_key) + all_children_complete = all(block_structure.get_xblock_field(child_key, self.COMPLETE) + for child_key in children + if not self._is_block_excluded(block_structure, child_key)) - children = block_structure.get_children(block_key) - all_children_complete = all(block_structure.get_xblock_field(child_key, self.COMPLETE) - for child_key in children - if not self._is_block_excluded(block_structure, child_key)) + if all_children_complete: + block_structure.override_xblock_field(block_key, self.COMPLETE, True) - if children and all_children_complete: - block_structure.override_xblock_field(block_key, self.COMPLETE, True) - - if any(block_structure.get_xblock_field(child_key, self.RESUME_BLOCK) for child_key in children): - block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True) + if any(block_structure.get_xblock_field(child_key, self.RESUME_BLOCK) for child_key in children): + block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True) def transform(self, usage_info, block_structure): """ diff --git a/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py b/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py index b5d872510a..8c57739c44 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py @@ -21,6 +21,7 @@ class StubAggregatorXBlock(XBlock): when transforming aggregator XBlock. """ completion_mode = XBlockCompletionMode.AGGREGATOR + has_children = True class StubExcludedXBlock(XBlock): @@ -36,7 +37,6 @@ class StubCompletableXBlock(XBlock, CompletableXBlockMixin): XBlock to test behaviour of BlockCompletionTransformer when transforming completable XBlock. """ - pass # lint-amnesty, pylint: disable=unnecessary-pass class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, CompletionWaffleTestMixin, ModuleStoreTestCase): @@ -44,7 +44,9 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio Tests behaviour of BlockCompletionTransformer """ TRANSFORMER_CLASS_TO_TEST = BlockCompletionTransformer - COMPLETION_TEST_VALUE = 0.4 + # Has to be 1.0 for 'complete' to function properly. The Completion api only uses 0.0 and 1.0 right now + # so this better reflects reality anyway. Should be updated if Completion api ever supports more. + COMPLETION_TEST_VALUE = 1.0 def setUp(self): super().setUp() @@ -53,31 +55,54 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio self.override_waffle_switch(True) @XBlock.register_temp_plugin(StubAggregatorXBlock, identifier='aggregator') - def test_transform_gives_none_for_aggregator(self): + @XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp') + def test_transform_aggregators(self): + """ + If there is an aggregator (such as a unit) that contains no children for some reason + (likely a mistake by the course team), it should be marked 'complete'. 'completion' should + still be None. + We mark 'complete' so learners are still able to have their subsections/sections/course + marked as complete and are not blocked by this one empty aggregator. + """ course = CourseFactory.create() - block = ItemFactory.create(category='aggregator', parent=course) - block_structure = get_course_blocks( - self.user, course.location, self.transformers + # Have to have at least one complete block to trigger entering the marking 'complete' flow + filled_aggregator = ItemFactory.create(category='aggregator', parent=course) + block = ItemFactory.create(category='comp', parent=filled_aggregator) + BlockCompletion.objects.submit_completion( + user=self.user, + block_key=block.location, + completion=self.COMPLETION_TEST_VALUE, ) + empty_aggregator = ItemFactory.create(category='aggregator', parent=course) + block_structure = get_course_blocks(self.user, course.location, self.transformers) - self._assert_block_has_proper_completion_value( - block_structure, block.location, None + self._assert_block_has_proper_completion_values( + block_structure, block.location, self.COMPLETION_TEST_VALUE, True + ) + self._assert_block_has_proper_completion_values( + block_structure, filled_aggregator.location, None, True + ) + self._assert_block_has_proper_completion_values( + block_structure, empty_aggregator.location, None, True ) @XBlock.register_temp_plugin(StubExcludedXBlock, identifier='excluded') def test_transform_gives_none_for_excluded(self): + """ + Excluded blocks always receive None for 'completion' and False for 'complete' + """ course = CourseFactory.create() block = ItemFactory.create(category='excluded', parent=course) - block_structure = get_course_blocks( - self.user, course.location, self.transformers - ) + block_structure = get_course_blocks(self.user, course.location, self.transformers) - self._assert_block_has_proper_completion_value( - block_structure, block.location, None - ) + self._assert_block_has_proper_completion_values(block_structure, block.location, None, False) @XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp') def test_transform_gives_value_for_completable(self): + """ + If a block is actually complete, make sure that value shows up in the transformed fields. + 'completion' should have the value and 'complete' should be True in these cases. + """ course = CourseFactory.create() block = ItemFactory.create(category='comp', parent=course) BlockCompletion.objects.submit_completion( @@ -85,34 +110,33 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio block_key=block.location, completion=self.COMPLETION_TEST_VALUE, ) - block_structure = get_course_blocks( - self.user, course.location, self.transformers - ) + block_structure = get_course_blocks(self.user, course.location, self.transformers) - self._assert_block_has_proper_completion_value( - block_structure, block.location, self.COMPLETION_TEST_VALUE + self._assert_block_has_proper_completion_values( + block_structure, block.location, self.COMPLETION_TEST_VALUE, True ) def test_transform_gives_zero_for_ordinary_block(self): + """ + I _think_ this is testing 'completion' is 0.0 before an ordinary block is viewed. + 'html' blocks end up receiving a 'completion' of 1.0 after being viewed. + """ course = CourseFactory.create() block = ItemFactory.create(category='html', parent=course) - block_structure = get_course_blocks( - self.user, course.location, self.transformers - ) + block_structure = get_course_blocks(self.user, course.location, self.transformers) - self._assert_block_has_proper_completion_value( - block_structure, block.location, 0.0 - ) + self._assert_block_has_proper_completion_values(block_structure, block.location, 0.0, False) - def _assert_block_has_proper_completion_value( - self, block_structure, block_key, expected_value + def _assert_block_has_proper_completion_values( + self, block_structure, block_key, expected_completion, expected_complete ): """ - Checks whether block's completion has expected value. + Checks whether block's completion and complete have expected values. """ - block_data = block_structure.get_transformer_block_data( - block_key, self.TRANSFORMER_CLASS_TO_TEST - ) + block_data = block_structure.get_transformer_block_data(block_key, self.TRANSFORMER_CLASS_TO_TEST) completion_value = block_data.fields['completion'] + # complete isn't saved as a transformer field, but just a regular xblock field /shrug + complete_value = block_structure.get_xblock_field(block_key, 'complete', False) - assert completion_value == expected_value + assert completion_value == expected_completion + assert complete_value == expected_complete diff --git a/openedx/features/course_experience/tests/views/test_course_outline.py b/openedx/features/course_experience/tests/views/test_course_outline.py index 20fa2b3949..2f3ae07507 100644 --- a/openedx/features/course_experience/tests/views/test_course_outline.py +++ b/openedx/features/course_experience/tests/views/test_course_outline.py @@ -462,6 +462,9 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT vertical2 = ItemFactory.create(category='vertical', parent_location=sequential2.location) vertical3 = ItemFactory.create(category='vertical', parent_location=sequential3.location) vertical4 = ItemFactory.create(category='vertical', parent_location=sequential4.location) + problem = ItemFactory.create(category='problem', parent_location=vertical.location) + problem2 = ItemFactory.create(category='problem', parent_location=vertical2.location) + problem3 = ItemFactory.create(category='problem', parent_location=vertical3.location) course.children = [chapter, chapter2] chapter.children = [sequential, sequential2] chapter2.children = [sequential3, sequential4] @@ -469,6 +472,9 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT sequential2.children = [vertical2] sequential3.children = [vertical3] sequential4.children = [vertical4] + vertical.children = [problem] + vertical2.children = [problem2] + vertical3.children = [problem3] if hasattr(cls, 'user'): CourseEnrollment.enroll(cls.user, course.id) return course @@ -545,8 +551,8 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT response = self.client.get(course_home_url(course)) content = pq(response.content) - # Subsection should be checked - assert len(content('.fa-check')) == 1 + # Subsection should be checked. Subsection 4 is also checked because it contains a vertical with no content + assert len(content('.fa-check')) == 2 def test_start_course(self): """ @@ -561,8 +567,8 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT response = self.visit_course_home(course, start_count=1, resume_count=0) content = pq(response.content) - vertical = course.children[0].children[0].children[0] - assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical.url_name) + problem = course.children[0].children[0].children[0].children[0] + assert content('.action-resume-course').attr('href').endswith('/problem/' + problem.url_name) @override_settings(LMS_BASE='test_url:9999') def test_resume_course_with_completion_api(self): @@ -573,33 +579,33 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT # Course tree course = self.course - vertical1 = course.children[0].children[0].children[0] - vertical2 = course.children[0].children[1].children[0] + problem1 = course.children[0].children[0].children[0].children[0] + problem2 = course.children[0].children[1].children[0].children[0] - self.complete_sequential(self.course, vertical1) + self.complete_sequential(self.course, problem1) # Test for 'resume' link response = self.visit_course_home(course, resume_count=1) - # Test for 'resume' link URL - should be vertical 1 + # Test for 'resume' link URL - should be problem 1 content = pq(response.content) - assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical1.url_name) + assert content('.action-resume-course').attr('href').endswith('/problem/' + problem1.url_name) - self.complete_sequential(self.course, vertical2) + self.complete_sequential(self.course, problem2) # Test for 'resume' link response = self.visit_course_home(course, resume_count=1) - # Test for 'resume' link URL - should be vertical 2 + # Test for 'resume' link URL - should be problem 2 content = pq(response.content) - assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical2.url_name) + assert content('.action-resume-course').attr('href').endswith('/problem/' + problem2.url_name) # visit sequential 1, make sure 'Resume Course' URL is robust against 'Last Visited' # (even though I visited seq1/vert1, 'Resume Course' still points to seq2/vert2) self.visit_sequential(course, course.children[0], course.children[0].children[0]) - # Test for 'resume' link URL - should be vertical 2 (last completed block, NOT last visited) + # Test for 'resume' link URL - should be problem 2 (last completed block, NOT last visited) response = self.visit_course_home(course, resume_count=1) content = pq(response.content) - assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical2.url_name) + assert content('.action-resume-course').attr('href').endswith('/problem/' + problem2.url_name) def test_resume_course_deleted_sequential(self): """ @@ -662,8 +668,8 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT CourseEnrollment.get_enrollment(self.user, course.id).delete() response = self.visit_course_home(course, start_count=1, resume_count=0) content = pq(response.content) - vertical = course.children[0].children[0].children[0] - assert content('.action-resume-course').attr('href').endswith('/vertical/' + vertical.url_name) + problem = course.children[0].children[0].children[0].children[0] + assert content('.action-resume-course').attr('href').endswith('/problem/' + problem.url_name) @override_waffle_switch(ENABLE_COMPLETION_TRACKING_SWITCH, active=True) def test_course_outline_auto_open(self):