From 9bc0f85982806617426072cc942d2914bd9fe0b6 Mon Sep 17 00:00:00 2001 From: Ivan Niedielnitsev <81557788+NiedielnitsevIvan@users.noreply.github.com> Date: Fri, 24 May 2024 19:59:09 +0300 Subject: [PATCH] feat: include units when calculating completion percentage (#34816) This is an enhancement to the API used for the courseware navigation sidebar. --- .../course_home_api/outline/serializers.py | 2 + .../outline/tests/test_view.py | 120 +++++++++++++++++- .../course_home_api/outline/views.py | 78 +++++++++--- 3 files changed, 178 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/course_home_api/outline/serializers.py b/lms/djangoapps/course_home_api/outline/serializers.py index c728efe0473a..83b8a9729eb0 100644 --- a/lms/djangoapps/course_home_api/outline/serializers.py +++ b/lms/djangoapps/course_home_api/outline/serializers.py @@ -66,6 +66,8 @@ def get_blocks(self, block): # pylint: disable=missing-function-docstring } if 'special_exam_info' in self.context.get('extra_fields', []) and block.get('special_exam_info'): serialized[block_key]['special_exam_info'] = block.get('special_exam_info').get('short_description') + if 'completion_stat' in self.context.get('extra_fields', []): + serialized[block_key]['completion_stat'] = block.get('completion_stat', {}) for child in children: serialized.update(self.get_blocks(child)) diff --git a/lms/djangoapps/course_home_api/outline/tests/test_view.py b/lms/djangoapps/course_home_api/outline/tests/test_view.py index f19efbd48cf9..4f40d9a80192 100644 --- a/lms/djangoapps/course_home_api/outline/tests/test_view.py +++ b/lms/djangoapps/course_home_api/outline/tests/test_view.py @@ -9,6 +9,7 @@ import ddt # lint-amnesty, pylint: disable=wrong-import-order import json # lint-amnesty, pylint: disable=wrong-import-order +from completion.models import BlockCompletion from django.conf import settings # lint-amnesty, pylint: disable=wrong-import-order from django.urls import reverse # lint-amnesty, pylint: disable=wrong-import-order from edx_toggles.toggles.testutils import override_waffle_flag # lint-amnesty, pylint: disable=wrong-import-order @@ -489,7 +490,7 @@ def add_blocks_to_course(self): parent_location=self.chapter.location ) self.vertical = BlockFactory.create( - category='problem', + category='vertical', graded=True, has_score=True, parent_location=self.sequential.location @@ -500,11 +501,20 @@ def add_blocks_to_course(self): parent_location=self.chapter.location ) self.ungraded_vertical = BlockFactory.create( - category='problem', + category='vertical', parent_location=self.ungraded_sequential.location ) update_outline_from_modulestore(self.course.id) + def create_completion(self, problem, completion): + return BlockCompletion.objects.create( + user=self.user, + context_key=problem.context_key, + block_type='problem', + block_key=problem.location, + completion=completion, + ) + @ddt.data(CourseMode.AUDIT, CourseMode.VERIFIED) def test_get_authenticated_enrolled_user(self, enrollment_mode): """ @@ -594,6 +604,18 @@ def test_proctored_exam(self, mock_summary): hide_after_due=False, is_onboarding_exam=False, ) + vertical = BlockFactory.create( + parent=sequence, + category='vertical', + graded=True, + has_score=True, + ) + BlockFactory.create( + parent=vertical, + category='problem', + graded=True, + has_score=True, + ) sequence.is_proctored_exam = True update_outline_from_modulestore(course.id) CourseEnrollment.enroll(self.user, course.id) @@ -608,7 +630,7 @@ def test_proctored_exam(self, mock_summary): exam_data = response.data['blocks'][str(sequence.location)] assert not exam_data['complete'] - assert exam_data['display_name'] == 'Test Proctored Exam' + assert exam_data['display_name'] == 'Test Proctored Exam (1 Question)' assert exam_data['special_exam_info'] == 'My Exam' assert exam_data['due'] is not None @@ -623,8 +645,8 @@ def test_assignment(self): assert response.status_code == 200 exam_data = response.data['blocks'][str(self.sequential.location)] - assert exam_data['display_name'] == 'Test (1 Question)' - assert exam_data['icon'] == 'fa-pencil-square-o' + assert exam_data['display_name'] == 'Test' + assert exam_data['icon'] is None assert str(self.vertical.location) in exam_data['children'] ungraded_data = response.data['blocks'][str(self.ungraded_sequential.location)] @@ -686,3 +708,91 @@ def test_hide_learning_sequences(self): replace_course_outline(new_learning_seq_outline) blocks = self.client.get(self.url).data['blocks'] assert seq_block_id not in blocks + + def test_empty_blocks_complete(self): + """ + Test that the API returns the correct complete state for empty blocks. + """ + self.add_blocks_to_course() + CourseEnrollment.enroll(self.user, self.course.id) + url = reverse('course-home:course-navigation', args=[self.course.id]) + response = self.client.get(url) + assert response.status_code == 200 + + sequence_data = response.data['blocks'][str(self.sequential.location)] + vertical_data = response.data['blocks'][str(self.vertical.location)] + assert sequence_data['complete'] + assert vertical_data['complete'] + + @ddt.data(True, False) + def test_blocks_complete_with_problem(self, problem_complete): + self.add_blocks_to_course() + problem = BlockFactory.create(parent=self.vertical, category='problem', graded=True, has_score=True) + CourseEnrollment.enroll(self.user, self.course.id) + self.create_completion(problem, int(problem_complete)) + + response = self.client.get(reverse('course-home:course-navigation', args=[self.course.id])) + + sequence_data = response.data['blocks'][str(self.sequential.location)] + vertical_data = response.data['blocks'][str(self.vertical.location)] + + assert sequence_data['complete'] == problem_complete + assert vertical_data['complete'] == problem_complete + + def test_blocks_completion_stat(self): + """ + Test that the API returns the correct completion statistics for the blocks. + """ + self.add_blocks_to_course() + completed_problem = BlockFactory.create(parent=self.vertical, category='problem', graded=True, has_score=True) + uncompleted_problem = BlockFactory.create(parent=self.vertical, category='problem', graded=True, has_score=True) + update_outline_from_modulestore(self.course.id) + CourseEnrollment.enroll(self.user, self.course.id) + self.create_completion(completed_problem, 1) + self.create_completion(uncompleted_problem, 0) + response = self.client.get(reverse('course-home:course-navigation', args=[self.course.id])) + + expected_sequence_completion_stat = { + 'completion': 0, + 'completable_children': 1, + } + expected_vertical_completion_stat = { + 'completion': 1, + 'completable_children': 2, + } + sequence_data = response.data['blocks'][str(self.sequential.location)] + vertical_data = response.data['blocks'][str(self.vertical.location)] + + assert not sequence_data['complete'] + assert not vertical_data['complete'] + assert sequence_data['completion_stat'] == expected_sequence_completion_stat + assert vertical_data['completion_stat'] == expected_vertical_completion_stat + + def test_blocks_completion_stat_all_problem_completed(self): + """ + Test that the API returns the correct completion statistics for the blocks when all problems are completed. + """ + self.add_blocks_to_course() + problem1 = BlockFactory.create(parent=self.vertical, category='problem', graded=True, has_score=True) + problem2 = BlockFactory.create(parent=self.vertical, category='problem', graded=True, has_score=True) + update_outline_from_modulestore(self.course.id) + CourseEnrollment.enroll(self.user, self.course.id) + self.create_completion(problem1, 1) + self.create_completion(problem2, 1) + response = self.client.get(reverse('course-home:course-navigation', args=[self.course.id])) + + expected_sequence_completion_stat = { + 'completion': 1, + 'completable_children': 1, + } + expected_vertical_completion_stat = { + 'completion': 2, + 'completable_children': 2, + } + sequence_data = response.data['blocks'][str(self.sequential.location)] + vertical_data = response.data['blocks'][str(self.vertical.location)] + + assert sequence_data['complete'] + assert vertical_data['complete'] + assert sequence_data['completion_stat'] == expected_sequence_completion_stat + assert vertical_data['completion_stat'] == expected_vertical_completion_stat diff --git a/lms/djangoapps/course_home_api/outline/views.py b/lms/djangoapps/course_home_api/outline/views.py index 6b701a2bbc84..e7ebb2eef28f 100644 --- a/lms/djangoapps/course_home_api/outline/views.py +++ b/lms/djangoapps/course_home_api/outline/views.py @@ -462,11 +462,9 @@ def get(self, request, *args, **kwargs): is_masquerading=user_is_masquerading, ) if navigation_sidebar_caching_is_disabled := courseware_disable_navigation_sidebar_blocks_caching(): - cached = False course_blocks = None else: course_blocks = cache.get(cache_key) - cached = course_blocks is not None if not course_blocks: if getattr(enrollment, 'is_active', False) or bool(staff_access): @@ -478,17 +476,12 @@ def get(self, request, *args, **kwargs): cache.set(cache_key, course_blocks, self.COURSE_BLOCKS_CACHE_TIMEOUT) course_blocks = self.filter_inaccessible_blocks(course_blocks, course_key) - - if cached: - # Note: The course_blocks received from get_course_outline_block_tree already has completion data, - # but since the course_blocks can be cached, and this status can change quite often, - # we need to update it every time if the data has not been cached. - course_blocks = self.mark_complete_recursive(course_blocks) + course_blocks = self.mark_complete_recursive(course_blocks) context = self.get_serializer_context() context.update({ 'include_vertical': True, - 'extra_fields': ['special_exam_info'], + 'extra_fields': ['special_exam_info', 'completion_stat'], 'enable_prerequisite_block_type': True, }) @@ -508,7 +501,7 @@ def filter_inaccessible_blocks(self, course_blocks, course_key): for section_data in course_sections: section_data['children'] = self.get_accessible_sequences( user_course_outline, - section_data.get('children', []) + section_data.get('children', ['completion']) ) accessible_sequence_ids = {str(usage_key) for usage_key in user_course_outline.accessible_sequences} for sequence_data in section_data['children']: @@ -520,15 +513,60 @@ def mark_complete_recursive(self, block): """ Mark blocks as complete or not complete based on the completions_dict. """ + if not block: + return block + if 'children' in block: - block['children'] = [self.mark_complete_recursive(child) for child in block['children']] - block['complete'] = all( - child['complete'] for child in block['children'] if child['type'] in self.completable_block_types - ) + block['children'] = [self.mark_complete_recursive(child) for child in block['children'] if child] + completable_children = self.get_completable_children(block) + block['complete'] = all(child['complete'] for child in completable_children) + block['completion_stat'] = self.get_block_completion_stat(block, completable_children) else: - block['complete'] = self.completions_dict.get(block['id'], False) + # If the block is a course, chapter, sequential, or vertical, without children, + # it should be completed by default. + completion = self.completions_dict.get(block['id'], 0) + block['complete'] = bool(completion) or block['type'] in ['course', 'chapter', 'sequential', 'vertical'] + block['completion_stat'] = self.get_block_completion_stat(block, completable_children=[]) + return block + def get_block_completion_stat(self, block, completable_children): + """ + Get the completion status of a block. + + Returns dictionary with the completion status and the number + of completable children of a block. + Completion is the value from 0 to 1 meaning the percentage of completion for lower-level blocks, + and sum of the completion status of the completable children. + """ + block_type = block['type'] + completable_children_num = len(completable_children) + + if block_type in ['course', 'sequential']: + completion = sum(child['complete'] for child in completable_children) + elif block_type == 'chapter': + # For sections, we have to count the status on the number of completed units + # and, accordingly, the number of units in it. + completion = sum(child['completion_stat']['completion'] for child in completable_children) + completable_children_num = sum( + child['completion_stat']['completable_children'] for child in completable_children + ) + elif block_type == 'vertical': + completion = sum(child['completion_stat']['completion'] for child in completable_children) + else: + completion = self.completions_dict.get(block['id'], 0) + + return { + 'completion': completion, + 'completable_children': completable_children_num, + } + + def get_completable_children(self, block): + """ + Get the completable children of a block. + """ + return [child for child in block.get('children', []) if child['type'] in self.completable_block_types] + @staticmethod def get_accessible_sections(user_course_outline, course_sections): """ @@ -573,11 +611,17 @@ def completions_dict(self): @cached_property def completable_block_types(self): """ - Return a set of block types that are completable. + Returns a set of block types that can be marked as completed. + + In addition to the lower-level x-blocks, it also includes blocks + that belong to XBlockCompletionMode.AGGREGATOR, because they can also be marked as complete. """ return { block_type for (block_type, block_cls) in XBlock.load_classes() - if XBlockCompletionMode.get_mode(block_cls) == XBlockCompletionMode.COMPLETABLE + if XBlockCompletionMode.get_mode(block_cls) in ( + XBlockCompletionMode.COMPLETABLE, + XBlockCompletionMode.AGGREGATOR + ) }