diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index 74ed55ec95ed..d603c0e583db 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/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): diff --git a/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py b/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py index f649299d560b..1f33cf00e74d 100644 --- a/cms/djangoapps/contentstore/tests/test_upstream_downstream_links.py +++ b/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() diff --git a/cms/djangoapps/contentstore/views/tests/test_gating.py b/cms/djangoapps/contentstore/views/tests/test_gating.py index 67275e0e688b..5f5473f2dc10 100644 --- a/cms/djangoapps/contentstore/views/tests/test_gating.py +++ b/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): """