Allow cascade deletion of auditlog entries

[aleh-rymasheuski]

Resolves #493.

This pull request allows cascade deletion of log entries, but still disallows direct deletion through the admin.

[codecov]

Codecov Report

Merging #556 (12d113a) into master (699d838) will increase coverage by 0.03%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   94.93%   94.96%   +0.03%     
==========================================
  Files          31       31              
  Lines         988      994       +6     
==========================================
+ Hits          938      944       +6     
  Misses         50       50              

Files Changed	Coverage Δ
auditlog/admin.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[aleh-rymasheuski]

@hramezani, we don't have a structure for testing this case with automated tests, and I don't have an elegant way to test this. If you can think of a good way of testing this, please add a test. Otherwise I'd suggest merging this change without a test.

[hramezani]

@aleh-rymasheuski Could you please explain more about what is happening here?

Also, I think you can move the [pattern.name for pattern in self.urls if pattern.name] out of method and define a class attr to don't repeat the loop in each call.

[aleh-rymasheuski]

request.resolver_match is the information about the matched URL handler:

(Pdb) request.resolver_match
ResolverMatch(func=django.contrib.admin.options.changelist_view, args=(), kwargs={}, url_name=auditlog_logentry_changelist, app_names=['admin'], namespaces=['admin'], route=admin/auditlog/logentry/)

Its url_name is the symbolic name of the route, e.g. 'auditlog_logentry_changelist'.

self.urls is the relative routes defined in the current admin class:

(Pdb) pp self.urls
[<URLPattern '' [name='auditlog_logentry_changelist']>,
 <URLPattern 'add/' [name='auditlog_logentry_add']>,
 <URLPattern '<path:object_id>/history/' [name='auditlog_logentry_history']>,
 <URLPattern '<path:object_id>/delete/' [name='auditlog_logentry_delete']>,
 <URLPattern '<path:object_id>/change/' [name='auditlog_logentry_change']>,
 <URLPattern '<path:object_id>/'>]

This check basically denies the delete permission if the request is handled by LogEntryAdmin, and allows it otherwise.

[hramezani]

@hramezani, we don't have a structure for testing this case with automated tests, and I don't have an elegant way to test this. If you can think of a good way of testing this, please add a test. Otherwise I'd suggest merging this change without a test.

Thanks @aleh-rymasheuski for this patch 🙏

I think we can test it like:

diff --git a/auditlog_tests/tests.py b/auditlog_tests/tests.py
index 12a4725..8824780 100644
--- a/auditlog_tests/tests.py
+++ b/auditlog_tests/tests.py
@@ -19,7 +19,7 @@ from django.core import management
 from django.db import models
 from django.db.models.signals import pre_save
 from django.test import RequestFactory, TestCase, override_settings
-from django.urls import reverse
+from django.urls import resolve, reverse
 from django.utils import dateformat, formats
 from django.utils import timezone as django_timezone
 from django.utils.encoding import smart_str
@@ -1555,6 +1555,9 @@ class DiffMsgTest(TestCase):
         super().setUp()
         self.site = AdminSite()
         self.admin = LogEntryAdmin(LogEntry, self.site)
+        self.user = User.objects.create_user(
+            username="test", email="test@example.com", password="top_secret"
+        )
 
     def _create_log_entry(self, action, changes):
         return LogEntry.objects.log_create(
@@ -1711,6 +1714,13 @@ class DiffMsgTest(TestCase):
         ):
             self.assertEqual(self.admin.field_verbose_name(log_entry, "actor"), "actor")
 
+    def test_has_delete_permission(self):
+        request = RequestFactory().get("/simplemodel/1/")
+        request.resolver_match = resolve("/simplemodel/1/")
+        request.user = self.user
+
+        self.assertFalse(self.admin.has_delete_permission(request))
+
 
 class NoDeleteHistoryTest(TestCase):
     def test_delete_related(self):

[aleh-rymasheuski]

@hramezani, I added a test based on your suggestion.

[hramezani]

can't we move the admin urls patterns list creation out of this method to not calculate it every time?

[aleh-rymasheuski]

I'm not sure when in the lifecycle of the admin self.urls is available, so I'll just move this expression into a cached property.

[aleh-rymasheuski]

Done.