[fix] Allow cascade deletions in ReadOnlyAdmin#596
[fix] Allow cascade deletions in ReadOnlyAdmin#596UltraBot05 wants to merge 5 commits intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReadOnlyAdmin.has_delete_permission now returns False only when the request's resolver_match.url_name equals the model-specific "delete", the model-specific "change", or the "changelist" view; in all other request contexts it delegates to super().has_delete_permission(request, obj). Tests were added covering changelist and change URLs (expect False), a non-admin "index" resolver (expect True), no resolver_match (expect True), and a cascade-delete-like fallback where super() denies deletion (expect False). Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![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
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This PR effectively resolves issue #256 by modifying Implementation ReviewThe fix correctly:
Test CoverageComprehensive tests were added covering all scenarios:
Files Reviewed (2 files)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_admin.py`:
- Around line 138-154: The tests only cover happy-path fallbacks and miss
verifying behavior when a staff user lacks the child-model delete permission;
add a subTest that creates a staff user without the
"test_project.delete_radiusaccounting" permission, sets request.resolver_match
to simulate a cascade delete (e.g., mock_resolver.url_name = "index"), and
assert that modeladmin.has_delete_permission(request) delegates to
super().has_delete_permission by returning False for that user; reference the
existing modeladmin.has_delete_permission check and the permission string
"test_project.delete_radiusaccounting" to locate where to add this negative-case
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 679d8b50-3b0b-447f-baed-4f1c9411beff
📒 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.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (1)
openwisp_utils/admin.py (1)
36-46: LGTM: direct admin deletes stay blocked while cascade checks fall back to Django permissions.The
resolver_matchguard closes the old crash path, and the targeted URL-name block keeps the model’s own delete/change/changelist views read-only without breaking parent-driven cascade deletes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_project/tests/test_admin.py (1)
156-173: 🧹 Nitpick | 🔵 TrivialUse a real staff user in the negative cascade test.
This mock only proves that some
request.user.has_perm(...)call returnedFalse; it does not lock in Django's actualModelAdmin.has_delete_permission(...)behavior for a staff user withouttest_project.delete_radiusaccounting.Proposed test hardening
with self.subTest("cascade delete without child permission returns False"): - request = self.client.get( - reverse("admin:test_project_radiusaccounting_changelist") - ).wsgi_request - + user = User.objects.create( + username="readonly-staff", + password="pass", + is_staff=True, + is_superuser=False, + ) + self.client.force_login(user) + request = self.client.get(reverse("admin:index")).wsgi_request + mock_resolver = MagicMock() mock_resolver.url_name = "index" request.resolver_match = mock_resolver - - # Mock a staff user who lacks the specific delete permission - request.user = MagicMock() - request.user.is_active = True - request.user.is_staff = True - request.user.is_superuser = False - request.user.has_perm.return_value = False # Explicitly deny perm - - # It should fall back to super().has_delete_permission and return False + self.assertFalse(modeladmin.has_delete_permission(request))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_project/tests/test_admin.py` around lines 156 - 173, The negative cascade test uses a MagicMock user which doesn't exercise Django's real permission checks; replace the mocked request.user with a real Django User instance (import User from django.contrib.auth.models), create/save a user with is_active=True, is_staff=True, is_superuser=False and do NOT assign the 'test_project.delete_radiusaccounting' permission, then attach that user to request (request.user = user or use self.client.force_login(user)) and assert modeladmin.has_delete_permission(request) is False so the test verifies ModelAdmin.has_delete_permission behavior for a real staff user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_admin.py`:
- Around line 125-154: Add a subtest that asserts
modeladmin.has_delete_permission returns False when the request is for the
direct admin delete route: create an object via _create_radius_accounting, build
a request using reverse("admin:test_project_radiusaccounting_delete",
args=[obj.pk]).wsgi_request, and
assertFalse(modeladmin.has_delete_permission(request)); this ensures the guarded
direct-admin URL name pattern for the "_delete" route is covered alongside the
existing "_change" and "_changelist" checks.
---
Duplicate comments:
In `@tests/test_project/tests/test_admin.py`:
- Around line 156-173: The negative cascade test uses a MagicMock user which
doesn't exercise Django's real permission checks; replace the mocked
request.user with a real Django User instance (import User from
django.contrib.auth.models), create/save a user with is_active=True,
is_staff=True, is_superuser=False and do NOT assign the
'test_project.delete_radiusaccounting' permission, then attach that user to
request (request.user = user or use self.client.force_login(user)) and assert
modeladmin.has_delete_permission(request) is False so the test verifies
ModelAdmin.has_delete_permission behavior for a real staff user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: abbed59d-9171-4cc3-b23e-631b1f672e66
📒 Files selected for processing (1)
tests/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.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
Multiple CI Failures DetectedHello @UltraBot05, 1. Code Style/QA Failures
2. Commit Message Failure
|
UltraBot05
force-pushed
the
fix/readonlyadmin-cascade-delete
branch
from
979e5b6 to
a6dd1fd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_project/tests/test_admin.py (1)
125-136: 🧹 Nitpick | 🔵 TrivialAdd test coverage for the direct
_deleteadmin route.The implementation blocks three URL names (
*_delete,*_change,*_changelist), but only*_changeand*_changelistare tested. If*_deleteis accidentally removed from the blocked set, this test suite would still pass.Suggested test addition
with self.subTest("change URL returns False"): obj = self._create_radius_accounting(username="test", session_id="1") request = self.client.get( reverse("admin:test_project_radiusaccounting_change", args=[obj.pk]) ).wsgi_request self.assertFalse(modeladmin.has_delete_permission(request)) + + with self.subTest("delete URL returns False"): + obj = self._create_radius_accounting(username="delete-test", session_id="2") + request = self.client.get( + reverse("admin:test_project_radiusaccounting_delete", args=[obj.pk]) + ).wsgi_request + self.assertFalse(modeladmin.has_delete_permission(request))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_project/tests/test_admin.py` around lines 125 - 136, Add a subTest that asserts the admin direct delete URL is blocked: create a RadiusAccounting instance via _create_radius_accounting, build a request using reverse("admin:test_project_radiusaccounting_delete", args=[obj.pk]) and take .wsgi_request, then assertFalse(modeladmin.has_delete_permission(request)); this mirrors the existing change and changelist checks and ensures the "*_delete" route is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_project/tests/test_admin.py`:
- Around line 125-136: Add a subTest that asserts the admin direct delete URL is
blocked: create a RadiusAccounting instance via _create_radius_accounting, build
a request using reverse("admin:test_project_radiusaccounting_delete",
args=[obj.pk]) and take .wsgi_request, then
assertFalse(modeladmin.has_delete_permission(request)); this mirrors the
existing change and changelist checks and ensures the "*_delete" route is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f316520-5c48-48fa-af9f-f8db7889f8d8
📒 Files selected for processing (1)
tests/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.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (5)
tests/test_project/tests/test_admin.py (5)
122-129: LGTM!Test setup and changelist URL subtest are correct. The assertion properly validates that direct access via the changelist view is blocked.
131-136: LGTM!Change URL subtest correctly validates that the
*_changeroute is blocked.
138-147: LGTM!Good simulation of cascade delete context by mocking
resolver_match.url_nameto a value outside the blocked set. The test correctly validates that such requests delegate tosuper().has_delete_permission().
149-154: LGTM!This subtest properly covers the edge case where
resolver_matchisNone, ensuring thegetattrguards in the implementation work correctly.
156-173: LGTM!This subtest validates the important case where the fallback to
super().has_delete_permission()correctly denies access for users without the delete permission. Good use of mocking to isolate the permission check behavior.
Black and Flake8 FailuresHello @UltraBot05, The CI pipeline failed due to code style violations detected by Black and Flake8. Failures & Remediation:
Specifically, the errors point to blank lines with whitespace in To fix these issues, please run the following command in your project's root directory: openwisp-qa-format |
UltraBot05
force-pushed
the
fix/readonlyadmin-cascade-delete
branch
from
a6dd1fd to
7005ab9
Compare
Added a subTest to verify that cascade deletes still respect child-model permissions. Fixes openwisp#256
UltraBot05
force-pushed
the
fix/readonlyadmin-cascade-delete
branch
from
7005ab9 to
de1376f
Compare
|
Hey @nemesifier , just a quick ping on this one as well. I just updated the companion PR #391 over in This PR fixes the generic |
Looks good, I will test asap. I am a bit flooded with PRs now but will do my best. |
3 participants
Checklist
Reference to Existing Issue
Closes #256
Description of Changes
ReadOnlyAdmin.has_delete_permission previously blocked all deletions, preventing parent models (like Organization) from performing cascade deletes.
Key changes:
URL Filtering: Uses
request.resolver_match.url_nameto block deletions only on the model's own delete, change, and changelist views.Safety: Added
getattrguards forresolver_matchto prevent crashes in contexts where URL resolution hasn't occurred.Fallback: Delegates to
super().has_delete_permissionfor all other contexts (like cascade deletes) to maintain standard Django permission checks.Tests: Added coverage for direct admin routes, simulated cascade deletes, None resolver cases, and negative cases for staff users without delete permissions.
Screenshot
Screenshot 1 Shows the deletion of org -> Failure page
Screenshot 2 Shows the working Fix