Skip to content

fix: Handle Taxonomy CASCADE deletes in _is_explicit_tag_delete#575

Merged
kdmccormick merged 1 commit intomainfrom
kdmccormick/cascade-delete-tag-event
Apr 30, 2026
Merged

fix: Handle Taxonomy CASCADE deletes in _is_explicit_tag_delete#575
kdmccormick merged 1 commit intomainfrom
kdmccormick/cascade-delete-tag-event

Conversation

@kdmccormick
Copy link
Copy Markdown
Member

@kdmccormick kdmccormick commented Apr 30, 2026

Merge info

For Verawood, we need to merge either this PR, or just the straightforward revert: #577. Both are ready to go.

EDIT: #576 is also a potential fix.

Description

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.

Supporting information

The original code was added in #571

while upgrading to v1.0, openedx-platform unit tests caught this:
https://gist.github.com/kdmccormick/912e71d4a57bf8aed046e9be3ca297e7

AI

Generated by Claude Sonnet 4.6 with my prompt:

(We're working within the src/openedx_tagging directory, no need to look at other subdirs of src)
In 508be7d, test_is_explicit_tag_delete_raises_for_unexpected_origin_type checks that a Taxonomy cannot be passed as the origin to _is_explicit_tag_delete. But this same behavior is causing failures in external consumes of this package, because deleting a Taxonomy causes a CASCADE which delete tags, thus passing Taxonomy as the origin to _is_explicit_tag_delete. This seems like a bug in openedx_tagging, no? Can you discern why the commit author made the choice to disallow this scenario?

which it got right on the first try. I just had a couple follow-ups to tweak its coding style.

I then had Opus 4.7 audit the change, and it suggested the additional test_taxonomy_delete_updates_search_index test.

I reviewed all code myself.

@kdmccormick kdmccormick force-pushed the kdmccormick/cascade-delete-tag-event branch from 758d08c to bc2b795 Compare April 30, 2026 12:16
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 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ormsbee
ormsbee previously approved these changes Apr 30, 2026
Copy link
Copy Markdown
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, and I would prefer to fix forward.

@ormsbee ormsbee dismissed their stale review April 30, 2026 12:52

Need to check my assumptions about how taxonomy deletes are already handled.

@kdmccormick kdmccormick merged commit d3750e8 into main Apr 30, 2026
6 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/cascade-delete-tag-event branch April 30, 2026 12:53
@bradenmacdonald
Copy link
Copy Markdown
Contributor

Nice fix - thanks @kdmccormick !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants