From 8fd71937f4c151ed0c67a3060fbe195ade91b07c Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Thu, 30 Apr 2026 08:03:53 -0400 Subject: [PATCH] fix: Handle Taxonomy CASCADE deletes in _is_explicit_tag_delete When a Taxonomy is deleted, Django fires pre_delete for each related Tag with origin set to the Taxonomy instance. The previous code raised a TypeError for any non-Tag, non-QuerySet origin, breaking any consumer that deletes a Taxonomy. For a non-Tag-queryset origin (e.g. taxonomy.delete() or Taxonomy.objects.filter(...).delete()), we now emit only for root-level tags; their handler covers the whole subtree via lineage__startswith. Bumps to v1.0.1. Co-authored-by: Claude Sonnet 4.6 Co-authored-by: Claude Opus 4.7 (1M context) --- src/openedx_core/__init__.py | 2 +- src/openedx_tagging/signal_handlers.py | 19 ++++++++------ tests/openedx_tagging/test_models.py | 35 +++++++++++++++++++++----- 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/openedx_core/__init__.py b/src/openedx_core/__init__.py index e9bef3c71..1e2c806f4 100644 --- a/src/openedx_core/__init__.py +++ b/src/openedx_core/__init__.py @@ -6,4 +6,4 @@ """ # The version for the entire repository -__version__ = "1.0.0" +__version__ = "1.0.1" diff --git a/src/openedx_tagging/signal_handlers.py b/src/openedx_tagging/signal_handlers.py index c4950562f..75217f502 100644 --- a/src/openedx_tagging/signal_handlers.py +++ b/src/openedx_tagging/signal_handlers.py @@ -16,30 +16,33 @@ def _is_explicit_tag_delete( instance: Tag, - origin: Tag | QuerySet[Tag] | None, + origin: object, using: str | None, ) -> bool: """ Return True only for tags explicitly targeted by the delete operation. Descendants deleted via CASCADE are skipped here because the explicit root - tag's handler emits updates for the whole subtree. + tag's handler emits updates for the whole subtree via lineage__startswith. Args: instance: The Tag being deleted. - origin: The source of the delete operation - either a Tag instance (for instance.delete()) - or a QuerySet[Tag] (for queryset.delete()), or None for other origins. + origin: The source of the delete operation — a Tag instance (instance.delete()), + a QuerySet[Tag] (queryset.delete()), or any other value when the delete + was triggered by CASCADE from a parent model (e.g. taxonomy.delete()). using: The database alias to use for queries, passed from the Django signal. """ if isinstance(origin, Tag): return origin.pk == instance.pk - # Fail fast if origin has an unexpected type so callsites don't silently - # skip event emission logic. if not isinstance(origin, QuerySet): - raise TypeError(f"Expected origin to be Tag, QuerySet[Tag], or None; got {type(origin).__name__}") + # CASCADE from a non-queryset origin (e.g., taxonomy.delete(), or None for unknown callers). + # Only emit for root-level tags; the root handler covers the whole subtree via lineage__startswith. + return len(instance.get_lineage()) == 1 if origin.model is not Tag: - raise TypeError(f"Expected origin queryset model Tag; got {origin.model.__name__}") + # CASCADE from a queryset of a non-Tag model (e.g., Taxonomy.objects.filter(...).delete()). + # Only emit for root-level tags; the root handler covers the whole subtree via lineage__startswith. + return len(instance.get_lineage()) == 1 # Check if this instance is in the set of explicitly-targeted tags. If not, it's being deleted # as a CASCADE side-effect, so it's not explicit. diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 177704639..0590513b0 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -676,12 +676,16 @@ def test_object_tag_export_id(self): self.object_tag.refresh_from_db() assert self.object_tag.export_id == "another-taxonomy" - def test_is_explicit_tag_delete_raises_for_unexpected_origin_type(self): - with pytest.raises( - TypeError, - match=r"Expected origin to be Tag, QuerySet\[Tag\], or None; got Taxonomy", - ): - _is_explicit_tag_delete(instance=self.tag, origin=cast(Any, self.taxonomy), using="default") + def test_is_explicit_tag_delete_taxonomy_cascade(self): + # Root-level tag is treated as explicit when cascaded from a Taxonomy deletion, + # so its handler covers the whole subtree via lineage__startswith. + assert _is_explicit_tag_delete( + instance=self.tag, origin=cast(Any, self.taxonomy), using="default" + ) + # Non-root tag returns False — its ancestor's handler already covers it. + assert not _is_explicit_tag_delete( + instance=self.eubacteria, origin=cast(Any, self.taxonomy), using="default" + ) def test_object_tag_value(self): # ObjectTag's value defaults to its tag's value @@ -1185,6 +1189,25 @@ def test_delete_with_descendants_updates_search_index(self, mock_task_delay) -> delta_object_id, } + @patch("openedx_tagging.signal_handlers.emit_content_object_associations_changed_for_object_ids_task.delay") + def test_taxonomy_delete_updates_search_index(self, mock_task_delay) -> None: + """ + Deleting a Taxonomy should enqueue updates for any tagged objects, + including ones tagged with deep descendants of root tags. + """ + object_id = "content-v1:org+course+run+type@unit+block@128" + api.tag_object( + object_id=object_id, + taxonomy=self.foxtrot.taxonomy, + tags=[self.foxtrot.value], + ) + + with self.captureOnCommitCallbacks(execute=True): + self.foxtrot.taxonomy.delete() + + assert mock_task_delay.call_count == 1 + assert mock_task_delay.call_args.kwargs["object_ids"] == [object_id] + @patch("openedx_tagging.tasks.CONTENT_OBJECT_ASSOCIATIONS_CHANGED", new_callable=MagicMock) def test_emit_content_object_associations_changed_for_object_ids_task(self, mock_signal) -> None: """Task emits one CONTENT_OBJECT_ASSOCIATIONS_CHANGED event per distinct object."""