Skip to content

Merge pull request #7812 from specify/issue-7649#8090

Closed
CarolineDenis wants to merge 1 commit into
temp-pre-7649from
retro-review-7649-base
Closed

Merge pull request #7812 from specify/issue-7649#8090
CarolineDenis wants to merge 1 commit into
temp-pre-7649from
retro-review-7649-base

Conversation

@CarolineDenis
Copy link
Copy Markdown
Contributor

@CarolineDenis CarolineDenis commented May 18, 2026

Allow new discipline deletion when it has no users and no collections

Fixes #

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add relevant issue to release milestone
  • Add pr to documentation list
  • Add automated tests
  • Add a reverse migration if a migration is present in the PR
  • Add migration function to
    def fix_schema_config(stdout: WriteToStdOut | None = None):

Testing instructions

Summary by CodeRabbit

  • New Features

    • Added ability to delete disciplines with built-in safeguards
    • Restricted to administrator users only
    • Prevents deletion if discipline has associated collections or users
    • Automatically manages cleanup of related tree definitions
  • Tests

    • New test coverage for discipline deletion protection scenarios

Review Change Stack

Allow new discipline deletion when it has no users and no collections
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 377ed3ab-8062-4b55-b54d-95d2ee402e76

📥 Commits

Reviewing files that changed from the base of the PR and between 0906e1e and adb24be.

📒 Files selected for processing (5)
  • specifyweb/backend/delete_blockers/views.py
  • specifyweb/backend/trees/utils.py
  • specifyweb/specify/api/crud.py
  • specifyweb/specify/api/dispatch.py
  • specifyweb/specify/tests/test_delete_blockers.py

📝 Walkthrough

Walkthrough

This PR implements discipline deletion with blocker guards and authorization checks. It adds tree model constants and CRUD helpers to detect disciplines, compute deletion blockers from collections and users, and perform cleanup before deletion. Dispatch and backend views add authorization and blocker collection logic. Tests validate blocker behavior.

Changes

Discipline Deletion with Guards

Layer / File(s) Summary
Tree models and CRUD discipline guards
specifyweb/backend/trees/utils.py, specifyweb/specify/api/crud.py
Defines DISCIPLINE_TREE_MODELS constant and adds helpers: is_discipline() to detect discipline objects, get_discipline_delete_guard_blockers() to compute blockers from associated collections and users, and prepare_discipline_for_delete() to detach owned trees and bulk-delete discipline-scoped setup rows.
CRUD delete_resource integration
specifyweb/specify/api/crud.py
delete_resource now branches on discipline objects: checks blocker conditions and raises BusinessRuleException if deletion is blocked; otherwise passes the discipline preparation callback to the underlying delete operation.
Dispatch authorization
specifyweb/specify/api/dispatch.py
Reformats django.http imports and adds authorization check that prevents non-admin users from deleting discipline resources by returning HttpResponseForbidden.
Backend delete_blockers view
specifyweb/backend/delete_blockers/views.py
Refactors delete_blockers to handle discipline-specific path: checks admin permission, optionally collects early blockers, runs preparation inside atomic transaction with explicit rollback, and factors blocker collection into _collect_delete_blockers() helper using Django Collector.
Test coverage
specifyweb/specify/tests/test_delete_blockers.py
Adds test helper _create_discipline_with_owned_trees() and three test cases: discipline blocked when collections exist, blocked when users exist (with BusinessRuleException), and allowed to delete when neither users nor collections exist.
🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title is a generic merge commit message template that does not convey the actual change; it references an issue number but provides no meaningful information about what the PR accomplishes. Use a descriptive title that summarizes the main change, such as: 'Allow deletion of empty disciplines' or 'Add discipline deletion support with blocker validation.'
Testing Instructions ⚠️ Warning The PR description provides no explicit testing instructions. The PR template requires a "Testing instructions" section that is empty. Add testing steps for: discipline creation/deletion without users/collections; deletion blocking with collections; deletion blocking with users. Include command: manage.py test specifyweb.specify.tests.test_delete_blockers
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Automatic Tests ✅ Passed Three new test methods validate discipline deletion with appropriate coverage of blocking conditions and successful deletion. Test fixtures and integration testing with delete_resource API included.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch retro-review-7649-base

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@CarolineDenis
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-project-automation github-project-automation Bot moved this from 📋Back Log to ✅Done in General Tester Board May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅Done

Development

Successfully merging this pull request may close these issues.

2 participants