[fix] Allow cascade deletions in ReadOnlyAdmin#596
Open
UltraBot05 wants to merge 1 commit intoopenwisp:masterfrom
Open
[fix] Allow cascade deletions in ReadOnlyAdmin#596UltraBot05 wants to merge 1 commit intoopenwisp:masterfrom
UltraBot05 wants to merge 1 commit intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThe changes modify ReadOnlyAdmin.has_delete_permission to deny deletion only for requests whose resolver URL name matches the model-specific delete view or the changelist view; for other request contexts it delegates to super().has_delete_permission(request, obj). A new test verifies that a direct delete URL returns False while a simulated cascade-delete context (resolver_match.url_name set to "index") results in True. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_utils/admin.py`:
- Around line 36-44: has_delete_permission currently blocks delete for delete
and changelist URLs but not the change view, so the delete button can still
appear; update the check in has_delete_permission to also include
f"{opts.app_label}_{opts.model_name}_change" in the tuple of blocked url_name
values (use the existing local variables opts and url_name to locate the logic)
so the method returns False for the change view as well and otherwise falls back
to super().has_delete_permission(request, obj).
In `@tests/test_project/tests/test_admin.py`:
- Around line 122-142: has_delete_permission currently dereferences
request.resolver_match.url_name and can raise AttributeError for requests where
resolver_match is None; update ReadOnlyAdmin.has_delete_permission to guard
using url_name = getattr(request.resolver_match, "url_name", None) and then
check the two URL names (f"{opts.app_label}_{opts.model_name}_delete" and
f"{opts.app_label}_{opts.model_name}_changelist") before returning False,
otherwise return super().has_delete_permission(request, obj). Add a unit test in
tests/test_admin.py that sets request.resolver_match = None (or uses a
RequestFactory request without URL resolution) and asserts the call to
modeladmin.has_delete_permission(request) does not raise and returns the same
value as super().has_delete_permission for that request.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
openwisp_utils/admin.pytests/test_project/tests/test_admin.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_project/tests/test_admin.py (2)
openwisp_utils/admin.py (2)
ReadOnlyAdmin(15-59)has_delete_permission(36-44)tests/test_project/models.py (1)
RadiusAccounting(81-101)
🔇 Additional comments (1)
openwisp_utils/admin.py (1)
36-44: Unnecessary defensive programming:resolver_matchis guaranteed in Django admin context.In Django admin,
request.resolver_matchis always set by the URL dispatcher beforehas_delete_permission()is called. The method is only invoked during normal admin operations where the request has been properly routed. Direct attribute access is the standard pattern used throughout Django admin. A guard is not needed.
UltraBot05
force-pushed
the
fix/readonlyadmin-cascade-delete
branch
from
c567a9a to
da00fb6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openwisp_utils/admin.py (2)
36-44:⚠️ Potential issue | 🟡 MinorBlock the change view URL to keep ReadOnlyAdmin truly read-only.
has_delete_permission()doesn’t block the model’s_changeURL, so the delete UI can still appear on the change form even though deletion is later denied. If the goal is to keep direct delete actions hidden, include_changein the blocked list.✅ Suggested fix
if url_name in ( f"{opts.app_label}_{opts.model_name}_delete", f"{opts.app_label}_{opts.model_name}_changelist", + f"{opts.app_label}_{opts.model_name}_change", ): return FalseDjango admin url_name for change view and delete button visibility behavior🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_utils/admin.py` around lines 36 - 44, The has_delete_permission method currently only blocks the delete and changelist URLs, but not the model change view so the delete button still appears; inside has_delete_permission (use opts = self.model._meta and url_name variable) add the change-view URL pattern f"{opts.app_label}_{opts.model_name}_change" to the blocked tuple so the method returns False for that URL and the delete UI is hidden.
36-44:⚠️ Potential issue | 🟠 MajorGuard
resolver_matchbefore dereferencingurl_name.
request.resolver_matchcan beNonein some request contexts (eg. certain tests or non-resolved requests), which will raiseAttributeErrorhere. Safer to guard withgetattrand treat a missingurl_nameas “not blocked.”🐛 Suggested fix
- url_name = request.resolver_match.url_name + url_name = getattr(request.resolver_match, "url_name", None)Django HttpRequest resolver_match None behavior and when it's populated in admin views🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_utils/admin.py` around lines 36 - 44, The has_delete_permission method dereferences request.resolver_match.url_name without guarding for resolver_match possibly being None; change it to first retrieve resolver_match = getattr(request, "resolver_match", None) and then url_name = getattr(resolver_match, "url_name", None) (or equivalent) and only perform the blocked-url_name check when url_name is truthy; otherwise return super().has_delete_permission(request, obj). This preserves the existing checks against opts.app_label/opts.model_name while avoiding AttributeError in tests or unresolved requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_utils/admin.py`:
- Around line 36-44: The has_delete_permission method currently only blocks the
delete and changelist URLs, but not the model change view so the delete button
still appears; inside has_delete_permission (use opts = self.model._meta and
url_name variable) add the change-view URL pattern
f"{opts.app_label}_{opts.model_name}_change" to the blocked tuple so the method
returns False for that URL and the delete UI is hidden.
- Around line 36-44: The has_delete_permission method dereferences
request.resolver_match.url_name without guarding for resolver_match possibly
being None; change it to first retrieve resolver_match = getattr(request,
"resolver_match", None) and then url_name = getattr(resolver_match, "url_name",
None) (or equivalent) and only perform the blocked-url_name check when url_name
is truthy; otherwise return super().has_delete_permission(request, obj). This
preserves the existing checks against opts.app_label/opts.model_name while
avoiding AttributeError in tests or unresolved requests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
openwisp_utils/admin.pytests/test_project/tests/test_admin.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (1)
tests/test_project/tests/test_admin.py (1)
122-141: Good coverage for direct delete vs cascade-delete context.The subtests clearly distinguish the blocked delete URL from a cascade-like request context.
UltraBot05
changed the title
UltraBot05
force-pushed
the
fix/readonlyadmin-cascade-delete
branch
2 times, most recently
from
2c77953 to
c9402ce
Compare
ReadOnlyAdmin.has_delete_permission previously returned False unconditionally, which blocked cascade deletions from parent models (like Organization). Updated the method to check request.resolver_match.url_name. It now correctly blocks direct deletes on the model's own views while delegating to the standard Django permission check for cascade deletes.
UltraBot05
force-pushed
the
fix/readonlyadmin-cascade-delete
branch
from
c9402ce to
c640a98
Compare
nemesifier
reviewed
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Reference to Existing Issue
Closes #256
Description of Changes
ReadOnlyAdmin.has_delete_permission previously returned False unconditionally, which blocked cascade deletions from parent models (like Organization).
Updated the method to check request.resolver_match.url_name. It now correctly blocks direct deletes on the model's own views while delegating to the standard Django permission check for cascade deletes.
Screenshot
Screenshot 1 Shows the deletion of org -> Failure page
Screenshot 2 Shows the working Fix