Merge pull request #59 from openedx/bmtcril/fix_xblock_serialization_…

…order

fix: Fix course-overviews sink not ordering blocks correctly

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
 ******************
 

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

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,52 +102,66 @@ 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
         unit_idx = 0
 
         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,

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

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)

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.