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):