diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 006b9fe..159805d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,15 @@ Change Log Unreleased ********** +0.9.6 - 2024-06-07 +****************** + +Fixes +===== + +* The CourseOutline sink was not always ordering blocks correctly, leading to issues with blocks appearing in the wrong sections/subsection. + + 0.9.5 - 2024-05-24 ****************** diff --git a/platform_plugin_aspects/__init__.py b/platform_plugin_aspects/__init__.py index 956270b..6c0a073 100644 --- a/platform_plugin_aspects/__init__.py +++ b/platform_plugin_aspects/__init__.py @@ -5,6 +5,6 @@ import os from pathlib import Path -__version__ = "0.9.5" +__version__ = "0.9.6" ROOT_DIRECTORY = Path(os.path.dirname(os.path.abspath(__file__))) diff --git a/platform_plugin_aspects/sinks/course_overview_sink.py b/platform_plugin_aspects/sinks/course_overview_sink.py index ad4e78e..826d0db 100644 --- a/platform_plugin_aspects/sinks/course_overview_sink.py +++ b/platform_plugin_aspects/sinks/course_overview_sink.py @@ -29,6 +29,8 @@ "input_format_allow_errors_ratio": 0.1, } +MODULESTORE_PUBLISHED_ONLY_FLAG = "rev-opt-published-only" + class XBlockSink(ModelBaseSink): """ @@ -49,6 +51,48 @@ def dump_related(self, serialized_item, dump_id, time_last_dumped): initial={"dump_id": dump_id, "time_last_dumped": time_last_dumped}, ) + def get_xblocks_recursive(self, parent_block, detached_xblock_types, initial): + """ + Serialize the course tree recursively, return a flattened list of XBlocks. + + Note that this list will not include detached blocks, those are handled + in get_detached_xblocks. This method preserves the course ordering for + non-detached blocks. + """ + items = [ + self.serialize_xblock( + parent_block, + detached_xblock_types, + initial["dump_id"], + initial["time_last_dumped"], + ) + ] + + for child in parent_block.get_children(): + items.extend( + self.get_xblocks_recursive(child, detached_xblock_types, initial) + ) + + return items + + def get_detached_xblocks(self, course_blocks, detached_xblock_types, initial): + """ + Spin through the flat list of all blocks in a course and return only + the detached blocks. Ordering of non-detached blocks is already + guaranteed in get_xblocks_recursive. Order of detached blocks + is not guaranteed. + """ + return [ + self.serialize_xblock( + block, + detached_xblock_types, + initial["dump_id"], + initial["time_last_dumped"], + ) + for block in course_blocks + if block.scope_ids.block_type in detached_xblock_types + ] + def serialize_item(self, item, many=False, initial=None): """ Serialize an XBlock into a dict @@ -58,10 +102,26 @@ def serialize_item(self, item, many=False, initial=None): detached_xblock_types = get_detached_xblock_types() location_to_node = {} - items = modulestore.get_items(course_key) - # Serialize the XBlocks to dicts and map them with their location as keys the - # whole map needs to be completed before we can define relationships + # This call gets the entire course tree in order, because the + # get_items call does not guarantee ordering. It does not return + # detached blocks, so we gather them separately below. + course_block = modulestore.get_course( + course_key, revision=MODULESTORE_PUBLISHED_ONLY_FLAG + ) + + items = self.get_xblocks_recursive(course_block, detached_xblock_types, initial) + + # Here we fetch the detached blocks and add them to the list. + detached = self.get_detached_xblocks( + modulestore.get_items(course_key, revision=MODULESTORE_PUBLISHED_ONLY_FLAG), + detached_xblock_types, + initial, + ) + + items.extend(detached) + + # Add location and tag data to the dict mappings of the blocks index = 0 section_idx = 0 subsection_idx = 0 @@ -69,41 +129,39 @@ def serialize_item(self, item, many=False, initial=None): for block in items: index += 1 - fields = self.serialize_xblock( - block, - index, - detached_xblock_types, - initial["dump_id"], - initial["time_last_dumped"], - ) - if fields["xblock_data_json"]["block_type"] == "chapter": - section_idx += 1 - subsection_idx = 0 - unit_idx = 0 - elif fields["xblock_data_json"]["block_type"] == "sequential": - subsection_idx += 1 - unit_idx = 0 - elif fields["xblock_data_json"]["block_type"] == "vertical": - unit_idx += 1 - - fields["xblock_data_json"]["section"] = section_idx - fields["xblock_data_json"]["subsection"] = subsection_idx - fields["xblock_data_json"]["unit"] = unit_idx - fields["xblock_data_json"]["tags"] = get_tags_for_block( - fields["location"], + block["order"] = index + + # Ensure that detached types aren't part of the tree + if block["xblock_data_json"]["detached"]: + block["xblock_data_json"]["section"] = 0 + block["xblock_data_json"]["subsection"] = 0 + block["xblock_data_json"]["unit"] = 0 + else: + if block["xblock_data_json"]["block_type"] == "chapter": + section_idx += 1 + subsection_idx = 0 + unit_idx = 0 + elif block["xblock_data_json"]["block_type"] == "sequential": + subsection_idx += 1 + unit_idx = 0 + elif block["xblock_data_json"]["block_type"] == "vertical": + unit_idx += 1 + + block["xblock_data_json"]["section"] = section_idx + block["xblock_data_json"]["subsection"] = subsection_idx + block["xblock_data_json"]["unit"] = unit_idx + + block["xblock_data_json"]["tags"] = get_tags_for_block( + block["location"], ) - fields["xblock_data_json"] = json.dumps(fields["xblock_data_json"]) - location_to_node[XBlockSink.strip_branch_and_version(block.location)] = ( - fields - ) + block["xblock_data_json"] = json.dumps(block["xblock_data_json"]) + location_to_node[block["location"]] = block return list(location_to_node.values()) - def serialize_xblock( - self, item, index, detached_xblock_types, dump_id, time_last_dumped - ): + def serialize_xblock(self, item, detached_xblock_types, dump_id, time_last_dumped): """Serialize an XBlock instance into a dict""" course_key = item.scope_ids.usage_id.course_key block_type = item.scope_ids.block_type @@ -123,10 +181,9 @@ def serialize_xblock( serialized_block = { "org": course_key.org, "course_key": str(course_key), - "location": str(item.location), + "location": str(XBlockSink.strip_branch_and_version(item.location)), "display_name": item.display_name_with_default.replace("'", "'"), "xblock_data_json": json_data, - "order": index, "edited_on": str(getattr(item, "edited_on", "")), "dump_id": dump_id, "time_last_dumped": time_last_dumped, diff --git a/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py b/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py index 1811938..f7ad9c1 100644 --- a/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py +++ b/platform_plugin_aspects/sinks/tests/test_course_overview_sink.py @@ -21,8 +21,10 @@ check_overview_csv_matcher, course_factory, course_str_factory, + detached_xblock_factory, fake_course_overview_factory, fake_serialize_fake_course_overview, + get_all_course_blocks_list, get_clickhouse_http_params, mock_detached_xblock_types, ) @@ -51,8 +53,10 @@ def test_course_publish_success( """ # Create a fake course structure with a few fake XBlocks course = course_factory() + detached_blocks = detached_xblock_factory() course_overview = fake_course_overview_factory(modified=datetime.now()) - mock_modulestore.return_value.get_items.return_value = course + mock_modulestore.return_value.get_course.return_value = course + mock_modulestore.return_value.get_items.return_value = detached_blocks mock_serialize_item.return_value = fake_serialize_fake_course_overview( course_overview @@ -83,18 +87,24 @@ def test_course_publish_success( "https://foo.bar/", match=[ matchers.query_param_matcher(blocks_params), - check_block_csv_matcher(course), + check_block_csv_matcher( + get_all_course_blocks_list(course, detached_blocks) + ), ], ) - course = course_str_factory() - dump_course_to_clickhouse(course) + course_key = course_str_factory() + dump_course_to_clickhouse(course_key) # Just to make sure we're not calling things more than we need to assert mock_modulestore.call_count == 1 + assert mock_modulestore.return_value.get_course.call_count == 1 + assert mock_modulestore.return_value.get_items.call_count == 1 assert mock_detached.call_count == 1 mock_get_ccx_courses.assert_called_once_with(course_overview.id) - mock_get_tags.call_count == len(course) + assert mock_get_tags.call_count == len( + get_all_course_blocks_list(course, detached_blocks) + ) @responses.activate( # pylint: disable=unexpected-keyword-arg,no-value-for-parameter @@ -288,7 +298,7 @@ def test_xblock_tree_structure(mock_modulestore, mock_detached, mock_get_tags): # Create a fake course structure with a few fake XBlocks course = course_factory() course_overview = fake_course_overview_factory(modified=datetime.now()) - mock_modulestore.return_value.get_items.return_value = course + mock_modulestore.return_value.get_course.return_value = course # Fake the "detached types" list since we can't import it here mock_detached.return_value = mock_detached_xblock_types() @@ -352,7 +362,7 @@ def test_xblock_graded_completable_mode(mock_modulestore, mock_detached, mock_ge # Create a fake course structure with a few fake XBlocks course = course_factory() course_overview = fake_course_overview_factory(modified=datetime.now()) - mock_modulestore.return_value.get_items.return_value = course + mock_modulestore.return_value.get_course.return_value = course # Fake the "detached types" list since we can't import it here mock_detached.return_value = mock_detached_xblock_types() @@ -387,11 +397,11 @@ def _check_item_serialized_location( raise # These tree indexes are the only ones which should have gradable set - _check_item_serialized_location(results[31], 1) - _check_item_serialized_location(results[32], 1) - _check_item_serialized_location(results[33], 1) + _check_item_serialized_location(results[28], 1) + _check_item_serialized_location(results[29], 1) + _check_item_serialized_location(results[30], 1) # These tree indexes are the only ones which should have non-"unknown" completion_modes. - _check_item_serialized_location(results[34], 0, "completable") - _check_item_serialized_location(results[35], 0, "aggregator") - _check_item_serialized_location(results[36], 0, "excluded") + _check_item_serialized_location(results[31], 0, "completable") + _check_item_serialized_location(results[32], 0, "aggregator") + _check_item_serialized_location(results[33], 0, "excluded") diff --git a/platform_plugin_aspects/tests/test_utils.py b/platform_plugin_aspects/tests/test_utils.py index f192b3c..ee43729 100644 --- a/platform_plugin_aspects/tests/test_utils.py +++ b/platform_plugin_aspects/tests/test_utils.py @@ -262,9 +262,9 @@ def mock_tag(taxonomy, value, parent=None): mock_tag22 = mock_tag(mock_taxonomy2, "tag2.2") mock_get_object_tags.return_value = [mock_tag13, mock_tag21, mock_tag22] - course_tags = get_tags_for_block(course[0].location) + course_tags = get_tags_for_block(course.location) assert course_tags == { "Taxonomy One": ["tag1.3", "tag1.2", "tag1.1"], "Taxonomy Two": ["tag2.1", "tag2.2"], } - mock_get_object_tags.assert_called_once_with(course[0].location) + mock_get_object_tags.assert_called_once_with(course.location) diff --git a/test_utils/helpers.py b/test_utils/helpers.py index 8e09fa1..febb9ad 100644 --- a/test_utils/helpers.py +++ b/test_utils/helpers.py @@ -76,6 +76,9 @@ def get_children(self): """ return self.children + def __repr__(self): + return f"FakeXBlock({self.location}): {self.display_name_with_default}" + def course_str_factory(course_id=None): """ @@ -207,46 +210,67 @@ def course_factory(): Return a fake course structure that exercises most of the serialization features. """ # Create a base block - top_block = FakeXBlock("top", block_type="course") - course = [ - top_block, - ] + course = FakeXBlock("top", block_type="course") + + # This gets used below to add some blocks to a vertical + unit_block = None # Create a few sections for i in range(3): - block = FakeXBlock(f"Section {i}", block_type="chapter") - course.append(block) - top_block.children.append(block) + section_block = FakeXBlock(f"Section {i}", block_type="chapter") + course.children.append(section_block) # Create some subsections if i > 0: for ii in range(3): - sub_block = FakeXBlock(f"Subsection {ii}", block_type="sequential") - course.append(sub_block) - block.children.append(sub_block) + subsection_block = FakeXBlock( + f"Subsection {ii}", block_type="sequential" + ) + section_block.children.append(subsection_block) for iii in range(3): # Create some units unit_block = FakeXBlock(f"Unit {iii}", block_type="vertical") - course.append(unit_block) - sub_block.children.append(unit_block) - - # Create some detached blocks at the top level - for i in range(3): - course.append(FakeXBlock(f"Detached {i}", block_type="course_info")) + subsection_block.children.append(unit_block) - # Create some graded blocks at the top level + # Create some graded blocks for i in range(3): - course.append(FakeXBlock(f"Graded {i}", graded=True)) + unit_block.children.append(FakeXBlock(f"Graded {i}", graded=True)) - # Create some completable blocks at the top level - course.append(FakeXBlock("Completable", completion_mode="completable")) - course.append(FakeXBlock("Aggregator", completion_mode="aggregator")) - course.append(FakeXBlock("Excluded", completion_mode="excluded")) + # Create some completable blocks + unit_block.children.append(FakeXBlock("Completable", completion_mode="completable")) + unit_block.children.append(FakeXBlock("Aggregator", completion_mode="aggregator")) + unit_block.children.append(FakeXBlock("Excluded", completion_mode="excluded")) return course +def detached_xblock_factory(): + """ + Create some detached xblocks for a course + """ + return [FakeXBlock(f"Detached {i}", block_type="course_info") for i in range(3)] + + +def get_all_course_blocks_list(course_block, detached_blocks): + """ + Return a flattened list of all blocks in the course. + """ + + def _get_blocks_recursive(parent_block): + blocks = [parent_block] + + for child in parent_block.get_children(): + blocks.extend(_get_blocks_recursive(child)) + + return blocks + + course_blocks = _get_blocks_recursive(course_block) + course_blocks.extend(detached_blocks) + + return course_blocks + + def check_overview_csv_matcher(course_overview): """ Match the course overview CSV against the test course.