feat: add admin action to delete inactive users - #60
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Implemented in the django admin panel and not the "verktyg" that admins use. But it's a niche function so it fits better there. |
it-amanuens
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rodatraden/tests.py (1)
57-59: ⚡ Quick winAssert
message_usercalls to lock admin feedback behaviorYou already mock
message_user; adding call assertions would protect the warning/info/success messaging paths from regressions.Also applies to: 82-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rodatraden/tests.py` around lines 57 - 59, The tests mock self.user_admin.message_user but never assert it was called; update the relevant test cases to assert the admin messaging behavior by adding assertions after the action under test that check self.user_admin.message_user was invoked (e.g., self.user_admin.message_user.assert_called() or assert_called_with(...) with the expected message and level), and mirror this change for the other test that mocks message_user (the second occurrence) to protect warning/info/success message paths from regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rodatraden/admin.py`:
- Line 39: The message string currently claims "0 or not set" which is incorrect
because the code uses getattr(settings, 'INACTIVE_USER_AUTODELETE_YEARS', 5) and
thus treats "not set" as enabled; update the message text where it's defined
(the tuple/list entry containing _("Inactive user auto-deletion is disabled
(INACTIVE_USER_AUTODELETE_YEARS is 0 or not set).")) to accurately reflect the
real disabled condition (<= 0) — e.g., change it to _("Inactive user
auto-deletion is disabled (INACTIVE_USER_AUTODELETE_YEARS is <= 0).") so the
message matches the behavior driven by settings.INACTIVE_USER_AUTODELETE_YEARS.
- Around line 33-53: The admin action delete_inactive_users currently builds
inactive_users from User.objects rather than the passed queryset, so it may
delete users not selected in the UI; change the logic to start from the provided
queryset (use the queryset argument) and then apply the same filters (exclude
is_staff/is_superuser and apply the last_login/date_joined Q conditions using
threshold_date) so only the selected records are considered, and keep the
existing message_user and threshold_date logic intact.
- Around line 76-80: The admin action delete_inactive_users currently performs
hard deletes but lacks permission restrictions; add an explicit
allowed_permissions attribute to the action (e.g., set
delete_inactive_users.allowed_permissions = ('delete',)) so only users with the
User model delete permission can execute it, and keep the
CustomUserAdmin.actions = [delete_inactive_users] + list(UserAdmin.actions)
registration intact.
In `@rodatraden/tests.py`:
- Line 1: Import override_settings from django.test and use it to wrap the two
test methods that currently modify settings.INACTIVE_USER_AUTODELETE_YEARS
(remove the manual assignment/restore). Specifically, add "from django.test
import override_settings" to the imports and annotate each test method inside
the TestCase subclass that mutates INACTIVE_USER_AUTODELETE_YEARS with
`@override_settings`(INACTIVE_USER_AUTODELETE_YEARS=<desired_value>) so the
setting is automatically restored; remove the lines that set and reset
settings.INACTIVE_USER_AUTODELETE_YEARS within those test methods.
---
Nitpick comments:
In `@rodatraden/tests.py`:
- Around line 57-59: The tests mock self.user_admin.message_user but never
assert it was called; update the relevant test cases to assert the admin
messaging behavior by adding assertions after the action under test that check
self.user_admin.message_user was invoked (e.g.,
self.user_admin.message_user.assert_called() or assert_called_with(...) with the
expected message and level), and mirror this change for the other test that
mocks message_user (the second occurrence) to protect warning/info/success
message paths from regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21d652cb-3d9f-467d-a987-bb405e7043f2
📒 Files selected for processing (3)
rodatraden/admin.pyrodatraden/tests.pytf/settings-template.py
| if years <= 0: | ||
| modeladmin.message_user( | ||
| request, | ||
| _("Inactive user auto-deletion is disabled (INACTIVE_USER_AUTODELETE_YEARS is 0 or not set)."), |
There was a problem hiding this comment.
Fix inaccurate disabled-state message
The text says “0 or not set”, but getattr(settings, 'INACTIVE_USER_AUTODELETE_YEARS', 5) treats “not set” as enabled (5). Update the message to match the real condition (<= 0).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rodatraden/admin.py` at line 39, The message string currently claims "0 or
not set" which is incorrect because the code uses getattr(settings,
'INACTIVE_USER_AUTODELETE_YEARS', 5) and thus treats "not set" as enabled;
update the message text where it's defined (the tuple/list entry containing
_("Inactive user auto-deletion is disabled (INACTIVE_USER_AUTODELETE_YEARS is 0
or not set).")) to accurately reflect the real disabled condition (<= 0) — e.g.,
change it to _("Inactive user auto-deletion is disabled
(INACTIVE_USER_AUTODELETE_YEARS is <= 0).") so the message matches the behavior
driven by settings.INACTIVE_USER_AUTODELETE_YEARS.
| delete_inactive_users.short_description = _("Delete inactive users") | ||
|
|
||
|
|
||
| class CustomUserAdmin(UserAdmin): | ||
| actions = [delete_inactive_users] + list(UserAdmin.actions) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rodatraden/admin.py | head -100Repository: it-amanuens/rodatraden
Length of output: 3019
🏁 Script executed:
rg "allowed_permissions" --type py -iRepository: it-amanuens/rodatraden
Length of output: 48
🏁 Script executed:
fd -e txt -e txt -e cfg -e ini | grep -i reqRepository: it-amanuens/rodatraden
Length of output: 83
🏁 Script executed:
cat requirements.txtRepository: it-amanuens/rodatraden
Length of output: 817
🌐 Web query:
Django admin action allowed_permissions attribute Django 6.0
💡 Result:
In Django 6.0, the admin action function attribute you should use is action_func.allowed_permissions (not ModelAdmin.allowed_permissions). You set it via the @admin.action decorator (preferred) or by assigning allowed_permissions on the action function directly. Example (Django 6.0 docs): @admin.action(permissions=["publish"], description="Mark selected stories as published") def make_published(self, request, queryset): queryset.update(status="p") The docs also state this is equivalent to: def make_published(self, request, queryset): queryset.update(status="p") make_published.allowed_permissions = ["publish"] make_published.short_description = "Mark selected stories as published" [1]. How permissions are checked: - Django documents that passing permissions to the action decorator limits the action’s availability based on permission checks on the ModelAdmin, and that the make_published action is only shown to users who pass the relevant ModelAdmin.has_*permission check [1]. - The available permissions values map to these methods: 'add' -> has_add_permission, 'change' -> has_change_permission, 'delete' -> has_delete_permission, 'view' -> has_view_permission [1]. - The docs also note you can use other values if you implement a corresponding has(self, request) method on ModelAdmin [1]. Note: There is no separate/alternate “Django admin action allowed_permissions attribute” for Django 6.0 beyond the allowed_permissions attribute on the action function described above; it is part of the action decorator/action-function API documented for 6.0 [1].
Citations:
Require delete permission for this destructive action
The delete_inactive_users action performs hard deletes (line 64) but does not declare required permissions. Without allowed_permissions, any user with change access to the User model can execute this destructive action. Add explicit permission requirements to restrict access.
Suggested fix
delete_inactive_users.short_description = _("Delete inactive users")
+delete_inactive_users.allowed_permissions = ("delete",)
class CustomUserAdmin(UserAdmin):
actions = [delete_inactive_users] + list(UserAdmin.actions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete_inactive_users.short_description = _("Delete inactive users") | |
| class CustomUserAdmin(UserAdmin): | |
| actions = [delete_inactive_users] + list(UserAdmin.actions) | |
| delete_inactive_users.short_description = _("Delete inactive users") | |
| delete_inactive_users.allowed_permissions = ("delete",) | |
| class CustomUserAdmin(UserAdmin): | |
| actions = [delete_inactive_users] + list(UserAdmin.actions) |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 80-80: Consider [delete_inactive_users, *list(UserAdmin.actions)] instead of concatenation
Replace with [delete_inactive_users, *list(UserAdmin.actions)]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rodatraden/admin.py` around lines 76 - 80, The admin action
delete_inactive_users currently performs hard deletes but lacks permission
restrictions; add an explicit allowed_permissions attribute to the action (e.g.,
set delete_inactive_users.allowed_permissions = ('delete',)) so only users with
the User model delete permission can execute it, and keep the
CustomUserAdmin.actions = [delete_inactive_users] + list(UserAdmin.actions)
registration intact.
| @@ -1,3 +1,95 @@ | |||
| from django.test import TestCase | |||
| from django.test import TestCase, RequestFactory | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files rodatraden/tests.pyRepository: it-amanuens/rodatraden
Length of output: 86
🏁 Script executed:
fd tests.pyRepository: it-amanuens/rodatraden
Length of output: 86
🏁 Script executed:
cat -n rodatraden/tests.py | head -100Repository: it-amanuens/rodatraden
Length of output: 4355
Use @override_settings decorator to avoid test isolation issues
Tests directly mutate settings.INACTIVE_USER_AUTODELETE_YEARS and restore it at method end (lines 71-72, 94-95). If an assertion fails before restoration, the setting leaks into subsequent tests. Use Django's @override_settings decorator instead for guaranteed cleanup.
Import override_settings from django.test and apply it to both test methods:
Suggested fix
-from django.test import TestCase, RequestFactory
+from django.test import TestCase, RequestFactory, override_settings
...
- def test_delete_inactive_users(self):
- # Set the threshold to 5 years
- original_setting = getattr(settings, 'INACTIVE_USER_AUTODELETE_YEARS', 5)
- settings.INACTIVE_USER_AUTODELETE_YEARS = 5
+ `@override_settings`(INACTIVE_USER_AUTODELETE_YEARS=5)
+ def test_delete_inactive_users(self):
...
- # Restore setting
- settings.INACTIVE_USER_AUTODELETE_YEARS = original_setting
- def test_delete_inactive_users_disabled(self):
- # Set to disabled
- original_setting = getattr(settings, 'INACTIVE_USER_AUTODELETE_YEARS', 5)
- settings.INACTIVE_USER_AUTODELETE_YEARS = 0
+ `@override_settings`(INACTIVE_USER_AUTODELETE_YEARS=0)
+ def test_delete_inactive_users_disabled(self):
...
- # Restore setting
- settings.INACTIVE_USER_AUTODELETE_YEARS = original_setting🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rodatraden/tests.py` at line 1, Import override_settings from django.test and
use it to wrap the two test methods that currently modify
settings.INACTIVE_USER_AUTODELETE_YEARS (remove the manual assignment/restore).
Specifically, add "from django.test import override_settings" to the imports and
annotate each test method inside the TestCase subclass that mutates
INACTIVE_USER_AUTODELETE_YEARS with
`@override_settings`(INACTIVE_USER_AUTODELETE_YEARS=<desired_value>) so the
setting is automatically restored; remove the lines that set and reset
settings.INACTIVE_USER_AUTODELETE_YEARS within those test methods.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
add admin action to delete inactive users after specified number of years
Warning this code is AI generated and has not been read through by human
Summary by CodeRabbit
New Features
Tests