-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: Ensure problem response reports include all descendant problems regardless of nesting or randomization #36677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 6 commits
44ef9e0
dde4437
a3b5263
fc1572d
2357237
26612e5
8f0799b
9eb6cb1
cf5c003
5cc4bbc
4f1a643
4f9a318
45947e4
3a460af
071011c
0888917
176235b
16c1390
1b6c227
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| from django.contrib.auth import get_user_model | ||
| from lazy import lazy | ||
| from opaque_keys.edx.keys import UsageKey | ||
| from opaque_keys import InvalidKeyError | ||
| from pytz import UTC | ||
| from six.moves import zip_longest | ||
|
|
||
|
|
@@ -44,6 +45,7 @@ | |
| from openedx.core.lib.cache_utils import get_cache | ||
| from openedx.core.lib.courses import get_course_by_id | ||
| from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order | ||
| from xmodule.modulestore.exceptions import ItemNotFoundError | ||
| from xmodule.partitions.partitions_service import PartitionService # lint-amnesty, pylint: disable=wrong-import-order | ||
| from xmodule.split_test_block import get_split_user_partitions # lint-amnesty, pylint: disable=wrong-import-order | ||
|
|
||
|
|
@@ -823,6 +825,31 @@ def _build_block_base_path(block): | |
| path.append(block.display_name) | ||
| return list(reversed(path)) | ||
|
|
||
| @staticmethod | ||
| def resolve_block_descendants(course_key, usage_key): | ||
| """ | ||
| Return every usage_key of type 'problem' under any block in the course tree. | ||
| Recursively traverses the course structure to find all descendant problem blocks. | ||
|
|
||
| Args: | ||
| course_key: The course identifier | ||
| usage_key: The starting block to search from | ||
|
|
||
| Returns: | ||
| List[UsageKey]: All problem block usage keys found under the root block | ||
| """ | ||
| store = modulestore() | ||
| problem_keys = [] | ||
| stack = [usage_key] | ||
| while stack: | ||
| current_key = stack.pop() | ||
| block = store.get_item(current_key) | ||
| if getattr(block, 'category', '') == 'problem': | ||
| problem_keys.append(current_key) | ||
| elif hasattr(block, 'children'): | ||
| stack.extend(getattr(block, 'children', [])) | ||
| return problem_keys | ||
|
||
|
|
||
| @classmethod | ||
| def _build_problem_list(cls, course_blocks, root, path=None): | ||
| """ | ||
|
|
@@ -831,13 +858,22 @@ def _build_problem_list(cls, course_blocks, root, path=None): | |
| Arguments: | ||
| course_blocks (BlockStructureBlockData): Block structure for a course. | ||
| root (UsageKey): This block and its children will be used to generate | ||
| the problem list | ||
| the problem list. | ||
| path (List[str]): The list of display names for the parent of root block | ||
| Yields: | ||
| Tuple[str, List[str], UsageKey]: tuple of a block's display name, path, and | ||
| usage key | ||
| """ | ||
| name = course_blocks.get_xblock_field(root, 'display_name') or root.block_type | ||
| usage key. | ||
| """ | ||
| name = course_blocks.get_xblock_field(root, 'display_name') | ||
| if not name or name == 'problem': | ||
| # Fallback: CourseBlocks may not have display_name cached for all blocks, | ||
| # especially for dynamically generated content or library_content blocks. | ||
| # Loading the full block is necessary to get meaningful names for CSV reports | ||
| try: | ||
| block = modulestore().get_item(root) | ||
| name = getattr(block, 'display_name', None) or root.block_type | ||
| except ItemNotFoundError: | ||
| name = root.block_type | ||
| if path is None: | ||
| path = [name] | ||
|
|
||
|
|
@@ -871,6 +907,7 @@ def _build_student_data( | |
| UsageKey.from_string(usage_key_str).map_into_course(course_key) | ||
| for usage_key_str in usage_key_str_list | ||
| ] | ||
|
|
||
| user = get_user_model().objects.get(pk=user_id) | ||
|
|
||
| student_data = [] | ||
|
|
@@ -978,11 +1015,23 @@ def generate(cls, _xblock_instance_args, _entry_id, course_id, task_input, actio | |
| if problem_types_filter: | ||
| filter_types = problem_types_filter.split(',') | ||
|
|
||
| # Expand problem locations to include all descendant problems here | ||
| expanded_usage_keys = [] | ||
| for problem_location_str in problem_locations: | ||
| try: | ||
| usage_key = UsageKey.from_string(problem_location_str).map_into_course(course_id) | ||
| expanded_usage_keys.extend(cls.resolve_block_descendants(course_id, usage_key)) | ||
| except InvalidKeyError: | ||
| continue | ||
|
|
||
| # Convert back to strings for consistency with the existing interface | ||
| expanded_usage_key_strs = [str(key) for key in expanded_usage_keys] | ||
|
|
||
| # Compute result table and format it | ||
| student_data, student_data_keys = cls._build_student_data( | ||
| user_id=task_input.get('user_id'), | ||
| course_key=course_id, | ||
| usage_key_str_list=problem_locations, | ||
| usage_key_str_list=expanded_usage_key_strs, | ||
| filter_types=filter_types, | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you do not need to load the block to check its type; you can look at the key:
if current_key.block_type == "problem": ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @kdmccormick , I applied the change! 8f0799b