From 4d4b93c6ed92e3be3d06ae68b1994751a3e9c6e6 Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Wed, 13 May 2020 10:22:50 -0700 Subject: [PATCH 1/2] AA-159: Improving performance of get_course_assignments We saw an increase in response time with recent changes to the logic behind get_course_assignments. This effort works to better access the information we need in order to improve performance. Namely, this is done by using the course_blocks_api --- .../blocks/transformers/block_completion.py | 47 ++++++++++++++++++- lms/djangoapps/course_blocks/api.py | 4 ++ lms/djangoapps/courseware/courses.py | 37 ++++++++------- 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/transformers/block_completion.py b/lms/djangoapps/course_api/blocks/transformers/block_completion.py index 6d02caff2d..57cf23c070 100644 --- a/lms/djangoapps/course_api/blocks/transformers/block_completion.py +++ b/lms/djangoapps/course_api/blocks/transformers/block_completion.py @@ -16,6 +16,8 @@ class BlockCompletionTransformer(BlockStructureTransformer): READ_VERSION = 1 WRITE_VERSION = 1 COMPLETION = 'completion' + COMPLETE = 'complete' + RESUME_BLOCK = 'resume_block' @classmethod def name(cls): @@ -43,9 +45,46 @@ class BlockCompletionTransformer(BlockStructureTransformer): def collect(cls, block_structure): block_structure.request_xblock_fields('completion_mode') + def mark_complete(self, course_block_completions, latest_completion_block_key, block_key, block_structure): + """ + Helper function to mark a block as 'complete' as dictated by + course_block_completions (for problems) or all of a block's children being complete. + This also sets the 'resume_block' field as that is connected to the latest completed block. + + :param course_block_completions: dict[course_completion_object] = completion_value + :param latest_completion_block_key: block key for the latest completed block. + :param block_key: A opaque_keys.edx.locator.BlockUsageLocator object + :param block_structure: A BlockStructureBlockData object + """ + if block_key in course_block_completions: + block_structure.override_xblock_field(block_key, self.COMPLETE, True) + if block_key == latest_completion_block_key: + block_structure.override_xblock_field(block_key, self.RESUME_BLOCK, True) + + children = block_structure.get_children(block_key) + non_discussion_children = (child_key for child_key in children + if block_structure.get_xblock_field(child_key, 'category') != 'discussion') + child_complete = (block_structure.get_xblock_field(child_key, self.COMPLETE) + for child_key in non_discussion_children) + if children and all(child_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) + def transform(self, usage_info, block_structure): """ - Mutates block_structure adding extra field which contains block's completion. + Mutates block_structure adding three extra fields which contains block's completion, + complete status, and if the block is a resume_block, indicating it is the most recently + completed block. + + IMPORTANT!: There is a subtle, but important difference between 'completion' and 'complete' + which are both set in this transformer: + 'completion': Returns a percentile (0.0 - 1.0) of correctness for a _problem_. This field will + be None for all other blocks that are not leaves and captured in BlockCompletion. + 'complete': Returns a boolean indicating whether the block is complete. For problems, this will + be taken from a BlockCompletion entry existing. For all other blocks, it will be marked True + if all of the children of the block are all marked complete (this is calculated recursively) """ def _is_block_an_aggregator_or_excluded(block_key): """ @@ -82,3 +121,9 @@ class BlockCompletionTransformer(BlockStructureTransformer): block_structure.set_transformer_block_field( block_key, self, self.COMPLETION, completion_value ) + + latest_completion = completions.latest() if completions.exists() else None + if latest_completion: + latest_completion_block_key = latest_completion[0] + for block_key in block_structure.post_order_traversal(): + self.mark_complete(completions_dict, latest_completion_block_key, block_key, block_structure) diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index 05a9614a4b..569eeb947e 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -7,6 +7,7 @@ get_course_blocks function. from django.conf import settings from edx_when import field_data +from lms.djangoapps.course_api.blocks.transformers.block_completion import BlockCompletionTransformer from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from openedx.features.content_type_gating.block_transformers import ContentTypeGateTransformer @@ -58,6 +59,7 @@ def get_course_blocks( transformers=None, collected_block_structure=None, allow_start_dates_in_future=False, + include_completion=False, ): """ A higher order function implemented on top of the @@ -91,6 +93,8 @@ def get_course_blocks( """ if not transformers: transformers = BlockStructureTransformers(get_course_block_access_transformers(user)) + if include_completion: + transformers += [BlockCompletionTransformer()] transformers.usage_info = CourseUsageInfo(starting_block_usage_key.course_key, user, allow_start_dates_in_future) return get_block_structure_manager(starting_block_usage_key.course_key).get_transformed( diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 0403f81ef3..40e8261c2d 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -52,13 +52,13 @@ from lms.djangoapps.courseware.access_utils import ( ) from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException from lms.djangoapps.courseware.exceptions import CourseAccessRedirect +from lms.djangoapps.course_blocks.api import get_course_blocks from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.enrollments.api import get_course_enrollment_details from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.lib.api.view_utils import LazySequence from openedx.features.course_duration_limits.access import AuditExpiredError from openedx.features.course_experience import RELATIVE_DATES_FLAG -from openedx.features.course_experience.utils import get_course_outline_block_tree from static_replace import replace_static_urls from student.models import CourseEnrollment from survey.utils import SurveyRequiredAccessError, check_survey_required_and_unanswered @@ -517,31 +517,34 @@ def get_course_assignments(course_key, user, request, include_access=False): Each returned object is a namedtuple with fields: title, url, date, contains_gated_content, complete, past_due """ - assignments = [] - # Ideally this function is always called with a request being passed in, but because it is also - # a subfunction of `get_course_date_blocks` which does not require a request, we are being defensive here. - if not request: - return assignments + store = modulestore() + course_usage_key = store.make_course_usage_key(course_key) + block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True) now = datetime.now(pytz.UTC) - course_root_block = get_course_outline_block_tree(request, str(course_key), user, allow_start_dates_in_future=True) - for section in course_root_block.get('children', []): - for subsection in section.get('children', []): - if not subsection.get('due') or not subsection.get('graded'): + assignments = [] + for section_key in block_data.get_children(course_usage_key): + for subsection_key in block_data.get_children(section_key): + due = block_data.get_xblock_field(subsection_key, 'due') + graded = block_data.get_xblock_field(subsection_key, 'graded', False) + if not due or not graded: continue - contains_gated_content = include_access and subsection.get('contains_gated_content', False) - title = subsection.get('display_name', _('Assignment')) + contains_gated_content = include_access and block_data.get_xblock_field( + subsection_key, 'contains_gated_content', False) + title = block_data.get_xblock_field(subsection_key, 'display_name', _('Assignment')) url = None - assignment_released = not subsection.get('start') or subsection.get('start') < now + start = block_data.get_xblock_field(subsection_key, 'start') + assignment_released = not start or start < now if assignment_released: - url = subsection.get('lms_web_url') + url = reverse('jump_to', args=[course_key, subsection_key]) + url = request and request.build_absolute_uri(url) - complete = subsection.get('complete') - past_due = not complete and subsection.get('due', now + timedelta(1)) < now + complete = block_data.get_xblock_field(subsection_key, 'complete', False) + past_due = not complete and due < now assignments.append(_Assignment( - subsection.get('id'), title, url, subsection.get('due'), contains_gated_content, complete, past_due + subsection_key, title, url, due, contains_gated_content, complete, past_due )) return assignments From 39d45eee4276857f0b7d5a617af1c6fff1c2896b Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Thu, 14 May 2020 13:12:43 -0700 Subject: [PATCH 2/2] Moving SUPPORTED_FIELDS for transformers into serializers It is only referenced inside of the serializers so we can just define it there --- .../course_api/blocks/serializers.py | 75 +++++++++++++++++- .../blocks/transformers/__init__.py | 79 ------------------- 2 files changed, 74 insertions(+), 80 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/serializers.py b/lms/djangoapps/course_api/blocks/serializers.py index f45f17381e..eee1d5d4ed 100644 --- a/lms/djangoapps/course_api/blocks/serializers.py +++ b/lms/djangoapps/course_api/blocks/serializers.py @@ -8,7 +8,80 @@ from django.conf import settings from rest_framework import serializers from rest_framework.reverse import reverse -from .transformers import SUPPORTED_FIELDS +from lms.djangoapps.course_blocks.transformers.visibility import VisibilityTransformer + +from .transformers.block_completion import BlockCompletionTransformer +from .transformers.block_counts import BlockCountsTransformer +from .transformers.milestones import MilestonesAndSpecialExamsTransformer +from .transformers.navigation import BlockNavigationTransformer +from .transformers.student_view import StudentViewTransformer + + +class SupportedFieldType(object): + """ + Metadata about fields supported by different transformers + """ + def __init__( + self, + block_field_name, + transformer=None, + requested_field_name=None, + serializer_field_name=None, + default_value=None + ): + self.transformer = transformer + self.block_field_name = block_field_name + self.requested_field_name = requested_field_name or block_field_name + self.serializer_field_name = serializer_field_name or self.requested_field_name + self.default_value = default_value + + +# A list of metadata for additional requested fields to be used by the +# BlockSerializer` class. Each entry provides information on how that field can +# be requested (`requested_field_name`), can be found (`transformer` and +# `block_field_name`), and should be serialized (`serializer_field_name` and +# `default_value`). + +SUPPORTED_FIELDS = [ + SupportedFieldType('category', requested_field_name='type'), + SupportedFieldType('display_name', default_value=''), + SupportedFieldType('graded'), + SupportedFieldType('format'), + SupportedFieldType('start'), + SupportedFieldType('due'), + SupportedFieldType('contains_gated_content'), + SupportedFieldType('has_score'), + SupportedFieldType('weight'), + SupportedFieldType('show_correctness'), + # 'student_view_data' + SupportedFieldType(StudentViewTransformer.STUDENT_VIEW_DATA, StudentViewTransformer), + # 'student_view_multi_device' + SupportedFieldType(StudentViewTransformer.STUDENT_VIEW_MULTI_DEVICE, StudentViewTransformer), + + SupportedFieldType('special_exam_info', MilestonesAndSpecialExamsTransformer), + + # set the block_field_name to None so the entire data for the transformer is serialized + SupportedFieldType(None, BlockCountsTransformer, BlockCountsTransformer.BLOCK_COUNTS), + + SupportedFieldType( + BlockNavigationTransformer.BLOCK_NAVIGATION, + BlockNavigationTransformer, + requested_field_name='nav_depth', + serializer_field_name='descendants', + ), + + # Provide the staff visibility info stored when VisibilityTransformer ran previously + SupportedFieldType( + 'merged_visible_to_staff_only', + VisibilityTransformer, + requested_field_name='visible_to_staff_only', + ), + SupportedFieldType( + BlockCompletionTransformer.COMPLETION, + BlockCompletionTransformer, + 'completion' + ) +] # This lists the names of all fields that are allowed # to be show to users who do not have access to a particular piece diff --git a/lms/djangoapps/course_api/blocks/transformers/__init__.py b/lms/djangoapps/course_api/blocks/transformers/__init__.py index 27c4ecd065..e69de29bb2 100644 --- a/lms/djangoapps/course_api/blocks/transformers/__init__.py +++ b/lms/djangoapps/course_api/blocks/transformers/__init__.py @@ -1,79 +0,0 @@ -""" -Course API Block Transformers -""" - - -from lms.djangoapps.course_blocks.transformers.visibility import VisibilityTransformer - -from .block_completion import BlockCompletionTransformer -from .block_counts import BlockCountsTransformer -from .milestones import MilestonesAndSpecialExamsTransformer -from .navigation import BlockNavigationTransformer -from .student_view import StudentViewTransformer - - -class SupportedFieldType(object): - """ - Metadata about fields supported by different transformers - """ - def __init__( - self, - block_field_name, - transformer=None, - requested_field_name=None, - serializer_field_name=None, - default_value=None - ): - self.transformer = transformer - self.block_field_name = block_field_name - self.requested_field_name = requested_field_name or block_field_name - self.serializer_field_name = serializer_field_name or self.requested_field_name - self.default_value = default_value - - -# A list of metadata for additional requested fields to be used by the -# BlockSerializer` class. Each entry provides information on how that field can -# be requested (`requested_field_name`), can be found (`transformer` and -# `block_field_name`), and should be serialized (`serializer_field_name` and -# `default_value`). - -SUPPORTED_FIELDS = [ - SupportedFieldType('category', requested_field_name='type'), - SupportedFieldType('display_name', default_value=''), - SupportedFieldType('graded'), - SupportedFieldType('format'), - SupportedFieldType('start'), - SupportedFieldType('due'), - SupportedFieldType('contains_gated_content'), - SupportedFieldType('has_score'), - SupportedFieldType('weight'), - SupportedFieldType('show_correctness'), - # 'student_view_data' - SupportedFieldType(StudentViewTransformer.STUDENT_VIEW_DATA, StudentViewTransformer), - # 'student_view_multi_device' - SupportedFieldType(StudentViewTransformer.STUDENT_VIEW_MULTI_DEVICE, StudentViewTransformer), - - SupportedFieldType('special_exam_info', MilestonesAndSpecialExamsTransformer), - - # set the block_field_name to None so the entire data for the transformer is serialized - SupportedFieldType(None, BlockCountsTransformer, BlockCountsTransformer.BLOCK_COUNTS), - - SupportedFieldType( - BlockNavigationTransformer.BLOCK_NAVIGATION, - BlockNavigationTransformer, - requested_field_name='nav_depth', - serializer_field_name='descendants', - ), - - # Provide the staff visibility info stored when VisibilityTransformer ran previously - SupportedFieldType( - 'merged_visible_to_staff_only', - VisibilityTransformer, - requested_field_name='visible_to_staff_only', - ), - SupportedFieldType( - BlockCompletionTransformer.COMPLETION, - BlockCompletionTransformer, - 'completion' - ) -]