fix: ComponentLinks were not deleted when parent container was deleted

cms/djangoapps/contentstore/signals/handlers.py

@@ -189,12 +189,21 @@ def listen_for_library_update(sender, library_key, **kwargs):  # pylint: disable
         update_library_index.delay(str(library_key), datetime.now(UTC).isoformat())
 
 
-@receiver(SignalHandler.item_deleted)
-def handle_item_deleted(**kwargs):
+@receiver(SignalHandler.pre_item_delete)
+def handle_item_deleted(**kwargs) -> None:
     """
-    Receives the item_deleted signal sent by Studio when an XBlock is removed from
-    the course structure and removes any gating milestone data associated with it or
-    its descendants.
+    Receives the pre_item_delete signal sent by Studio when an XBlock is removed
+    from the course structure and removes any gating milestone and upstream link
+    data associated with it or its descendants.
+
+    We use the "pre" signal because once the actual "item_deleted" signal is
+    sent, it's impossible to fetch the descendants of the item.
+
+    NOTE: This partially overlaps with ``delete_upstream_downstream_link_handler``
+    (below), which also removes ComponentLink / ContainerLink rows on delete but
+    only for the single deleted block, not its descendants. This handler is the
+    one responsible for cascading the link deletion to the deleted block's
+    children. Keep the two in sync if you change either.
 
     Arguments:
         kwargs (dict): Contains the content usage key of the item deleted
@@ -211,14 +220,16 @@ def handle_item_deleted(**kwargs):
         try:
             deleted_block = modulestore().get_item(usage_key)
         except ItemNotFoundError:
+            log.warning("Unable to load XBlock %s to handle its pre_item_delete signal", str(usage_key), exc_info=True)
+            # There may be dangling ComponentLink / milestone data.
             return
-        id_list = {deleted_block.location}
+        id_list = {deleted_block.usage_key}
         for block in yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')):
             # Remove prerequisite milestone data
-            gating_api.remove_prerequisite(block.location)
+            gating_api.remove_prerequisite(block.usage_key)
             # Remove any 'requires' course content milestone relationships
-            gating_api.set_required_content(course_key, block.location, None, None, None)
-            id_list.add(block.location)
+            gating_api.set_required_content(course_key, block.usage_key, None, None, None)
+            id_list.add(block.usage_key)
 
         ComponentLink.objects.filter(downstream_usage_key__in=id_list).delete()
         ContainerLink.objects.filter(downstream_usage_key__in=id_list).delete()
@@ -275,6 +286,12 @@ def update_upstream_downstream_link_handler(**kwargs):
 def delete_upstream_downstream_link_handler(**kwargs):
     """
     Delete upstream->downstream link from database on xblock delete.
+
+    NOTE: This only removes the link for the single deleted block. Cascading the
+    deletion to the block's descendants (e.g. the child components of a deleted
+    container) is handled separately by ``handle_item_deleted`` (above), which
+    listens to the modulestore ``pre_item_delete`` signal. These two handlers
+    partially overlap; keep them in sync if you change either.
     """
     xblock_info = kwargs.get("xblock_info", None)
     if not xblock_info or not isinstance(xblock_info, XBlockData):

cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py

@@ -59,6 +59,38 @@ def _create_unit(self, num: int):
             upstream_version=num,
         )
 
+    def _create_container_with_linked_children(self, num_children: int = 3):
+        """
+        Create a unit (vertical) container that has an upstream link (ContainerLink)
+        and that contains child blocks which each have their own upstream link
+        (ComponentLink).
+
+        Returns the container block and the list of child blocks.
+        """
+        container_upstream = LibraryContainerLocator.from_string(
+            f"lct:OpenedX:CSPROB2:unit:{uuid4()}"
+        )
+        container = BlockFactory.create(
+            parent=self.sequence,
+            category='vertical',
+            display_name="A container with linked children",
+            upstream=str(container_upstream),
+            upstream_version=1,
+        )
+        children = []
+        for i in range(num_children):
+            child_upstream = LibraryUsageLocatorV2.from_string(
+                f"lb:OpenedX:CSPROB2:html:{uuid4()}"
+            )
+            children.append(BlockFactory.create(
+                parent=container,
+                category="html",
+                display_name=f"Child html block - {i}",
+                upstream=str(child_upstream),
+                upstream_version=i + 1,
+            ))
+        return container, children
+
     def _create_unit_and_expected_container_link(self, course_key: str | CourseKey, num_blocks: int = 3):
         """
         Create unit xblock with random upstream key and version number.
@@ -275,7 +307,11 @@ class TestUpstreamLinksEvents(
     Test signals related to managing upstream->downstream links.
     """
 
-    ENABLED_SIGNALS = ['course_deleted', 'course_published']
+    # 'pre_item_delete' is required so that handle_item_deleted runs and cascades
+    # link deletion to a deleted block's descendants. The xblock.deleted.v1 event
+    # only deletes the link for the single deleted block (see
+    # delete_upstream_downstream_link_handler).
+    ENABLED_SIGNALS = ['course_deleted', 'course_published', 'pre_item_delete']
     ENABLED_OPENEDX_EVENTS = [
         "org.openedx.content_authoring.xblock.created.v1",
         "org.openedx.content_authoring.xblock.updated.v1",
@@ -331,3 +367,24 @@ def test_delete_handler(self):
         assert ContainerLink.objects.filter(downstream_usage_key=usage_key).exists()
         self.store.delete_item(usage_key, self.user.id)
         assert not ContainerLink.objects.filter(downstream_usage_key=usage_key).exists()
+
+    def test_delete_container_deletes_child_component_links(self):
+        """
+        Deleting a container must delete its own ContainerLink *and* the
+        ComponentLinks of all of its descendant blocks.
+        """
+        with self.store.bulk_operations(self.course_key_1):
+            container, children = self._create_container_with_linked_children()
+
+        container_key = container.usage_key
+        child_keys = [child.usage_key for child in children]
+
+        # Sanity check: the container link and all child component links exist.
+        assert ContainerLink.objects.filter(downstream_usage_key=container_key).exists()
+        assert ComponentLink.objects.filter(downstream_usage_key__in=child_keys).count() == len(child_keys)
+
+        self.store.delete_item(container_key, self.user.id)
+
+        # The container's link and every child's component link must be gone.
+        assert not ContainerLink.objects.filter(downstream_usage_key=container_key).exists()
+        assert not ComponentLink.objects.filter(downstream_usage_key__in=child_keys).exists()

cms/djangoapps/contentstore/views/tests/test_gating.py

@@ -22,7 +22,7 @@ class TestSubsectionGating(CourseTestCase):
     Tests for the subsection gating feature
     """
     MODULESTORE = TEST_DATA_SPLIT_MODULESTORE
-    ENABLED_SIGNALS = ['item_deleted']
+    ENABLED_SIGNALS = ['pre_item_delete']
 
     def setUp(self):
         """