From 40b3975e7d3e1464a733c69171ad7d38f8814280 Mon Sep 17 00:00:00 2001 From: Akash Kumar Sen Date: Mon, 12 Jun 2023 08:50:55 +0530 Subject: [PATCH] Fixed #34462 -- Made admin log actions in bulk. This also deprecates ModelAdmin.log_deletion() and LogEntryManager.log_action(). --- django/contrib/admin/actions.py | 4 +- django/contrib/admin/models.py | 56 ++++++++++ django/contrib/admin/options.py | 48 +++++++-- docs/internals/deprecation.txt | 3 +- docs/releases/5.1.txt | 5 +- tests/admin_changelist/tests.py | 6 +- tests/admin_utils/models.py | 24 +++++ tests/admin_utils/test_logentry.py | 143 ++++++++++++++++++++----- tests/admin_views/test_actions.py | 19 +++- tests/admin_views/test_history_view.py | 9 +- tests/admin_views/tests.py | 24 ++--- tests/modeladmin/tests.py | 103 +++++++++++++++++- 12 files changed, 374 insertions(+), 70 deletions(-) diff --git a/django/contrib/admin/actions.py b/django/contrib/admin/actions.py index bf43c5d460..eefb63837e 100644 --- a/django/contrib/admin/actions.py +++ b/django/contrib/admin/actions.py @@ -45,9 +45,7 @@ def delete_selected(modeladmin, request, queryset): raise PermissionDenied n = len(queryset) if n: - for obj in queryset: - obj_display = str(obj) - modeladmin.log_deletion(request, obj, obj_display) + modeladmin.log_deletions(request, queryset) modeladmin.delete_queryset(request, queryset) modeladmin.message_user( request, diff --git a/django/contrib/admin/models.py b/django/contrib/admin/models.py index 021984160a..bb81be8297 100644 --- a/django/contrib/admin/models.py +++ b/django/contrib/admin/models.py @@ -1,4 +1,5 @@ import json +import warnings from django.conf import settings from django.contrib.admin.utils import quote @@ -6,6 +7,7 @@ from django.contrib.contenttypes.models import ContentType from django.db import models from django.urls import NoReverseMatch, reverse from django.utils import timezone +from django.utils.deprecation import RemovedInDjango60Warning from django.utils.text import get_text_list from django.utils.translation import gettext from django.utils.translation import gettext_lazy as _ @@ -33,6 +35,11 @@ class LogEntryManager(models.Manager): action_flag, change_message="", ): + warnings.warn( + "LogEntryManager.log_action() is deprecated. Use log_actions() instead.", + RemovedInDjango60Warning, + stacklevel=2, + ) if isinstance(change_message, list): change_message = json.dumps(change_message) return self.model.objects.create( @@ -44,6 +51,55 @@ class LogEntryManager(models.Manager): change_message=change_message, ) + def log_actions( + self, user_id, queryset, action_flag, change_message="", *, single_object=False + ): + # RemovedInDjango60Warning. + if type(self).log_action != LogEntryManager.log_action: + warnings.warn( + "The usage of log_action() is deprecated. Implement log_actions() " + "instead.", + RemovedInDjango60Warning, + stacklevel=2, + ) + return [ + self.log_action( + user_id=user_id, + content_type_id=ContentType.objects.get_for_model( + obj, for_concrete_model=False + ).id, + object_id=obj.pk, + object_repr=str(obj), + action_flag=action_flag, + change_message=change_message, + ) + for obj in queryset + ] + + if isinstance(change_message, list): + change_message = json.dumps(change_message) + + log_entry_list = [ + self.model( + user_id=user_id, + content_type_id=ContentType.objects.get_for_model( + obj, for_concrete_model=False + ).id, + object_id=obj.pk, + object_repr=str(obj)[:200], + action_flag=action_flag, + change_message=change_message, + ) + for obj in queryset + ] + + if single_object and log_entry_list: + instance = log_entry_list[0] + instance.save() + return instance + + return self.model.objects.bulk_create(log_entry_list) + class LogEntry(models.Model): action_time = models.DateTimeField( diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 1d072114c8..b045b2df02 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -2,6 +2,7 @@ import copy import enum import json import re +import warnings from functools import partial, update_wrapper from urllib.parse import parse_qsl from urllib.parse import quote as urlquote @@ -54,6 +55,7 @@ from django.http.response import HttpResponseBase from django.template.response import SimpleTemplateResponse, TemplateResponse from django.urls import reverse from django.utils.decorators import method_decorator +from django.utils.deprecation import RemovedInDjango60Warning from django.utils.html import format_html from django.utils.http import urlencode from django.utils.safestring import mark_safe @@ -945,13 +947,12 @@ class ModelAdmin(BaseModelAdmin): """ from django.contrib.admin.models import ADDITION, LogEntry - return LogEntry.objects.log_action( + return LogEntry.objects.log_actions( user_id=request.user.pk, - content_type_id=get_content_type_for_model(obj).pk, - object_id=obj.pk, - object_repr=str(obj), + queryset=[obj], action_flag=ADDITION, change_message=message, + single_object=True, ) def log_change(self, request, obj, message): @@ -962,13 +963,12 @@ class ModelAdmin(BaseModelAdmin): """ from django.contrib.admin.models import CHANGE, LogEntry - return LogEntry.objects.log_action( + return LogEntry.objects.log_actions( user_id=request.user.pk, - content_type_id=get_content_type_for_model(obj).pk, - object_id=obj.pk, - object_repr=str(obj), + queryset=[obj], action_flag=CHANGE, change_message=message, + single_object=True, ) def log_deletion(self, request, obj, object_repr): @@ -978,6 +978,11 @@ class ModelAdmin(BaseModelAdmin): The default implementation creates an admin LogEntry object. """ + warnings.warn( + "ModelAdmin.log_deletion() is deprecated. Use log_deletions() instead.", + RemovedInDjango60Warning, + stacklevel=2, + ) from django.contrib.admin.models import DELETION, LogEntry return LogEntry.objects.log_action( @@ -988,6 +993,31 @@ class ModelAdmin(BaseModelAdmin): action_flag=DELETION, ) + def log_deletions(self, request, queryset): + """ + Log that objects will be deleted. Note that this method must be called + before the deletion. + + The default implementation creates admin LogEntry objects. + """ + from django.contrib.admin.models import DELETION, LogEntry + + # RemovedInDjango60Warning. + if type(self).log_deletion != ModelAdmin.log_deletion: + warnings.warn( + "The usage of log_deletion() is deprecated. Implement log_deletions() " + "instead.", + RemovedInDjango60Warning, + stacklevel=2, + ) + return [self.log_deletion(request, obj, str(obj)) for obj in queryset] + + return LogEntry.objects.log_actions( + user_id=request.user.pk, + queryset=queryset, + action_flag=DELETION, + ) + def action_checkbox(self, obj): """ A list_display column containing a checkbox widget. @@ -2174,7 +2204,7 @@ class ModelAdmin(BaseModelAdmin): obj_display = str(obj) attr = str(to_field) if to_field else self.opts.pk.attname obj_id = obj.serializable_value(attr) - self.log_deletion(request, obj, obj_display) + self.log_deletions(request, [obj]) self.delete_model(request, obj) return self.response_delete(request, obj_display, obj_id) diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index e69772dce3..a2968ab625 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -56,7 +56,8 @@ details on these changes. See the :ref:`Django 5.1 release notes ` for more details on these changes. -* ... +* The ``ModelAdmin.log_deletion()`` and ``LogEntryManager.log_action()`` + methods will be removed. .. _deprecation-removed-in-5.1: diff --git a/docs/releases/5.1.txt b/docs/releases/5.1.txt index 73343b7499..d6c07d9c22 100644 --- a/docs/releases/5.1.txt +++ b/docs/releases/5.1.txt @@ -285,7 +285,10 @@ Features deprecated in 5.1 Miscellaneous ------------- -* ... +* The ``ModelAdmin.log_deletion()`` and ``LogEntryManager.log_action()`` + methods are deprecated. Subclasses should implement + ``ModelAdmin.log_deletions()`` and ``LogEntryManager.log_actions()`` + instead. Features removed in 5.1 ======================= diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 4caefdb9e4..4870d9bbe9 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -16,7 +16,6 @@ from django.contrib.admin.views.main import ( TO_FIELD_VAR, ) from django.contrib.auth.models import User -from django.contrib.contenttypes.models import ContentType from django.contrib.messages.storage.cookie import CookieStorage from django.db import DatabaseError, connection, models from django.db.models import F, Field, IntegerField @@ -1636,8 +1635,7 @@ class GetAdminLogTests(TestCase): """{% get_admin_log %} works without specifying a user.""" user = User(username="jondoe", password="secret", email="super@example.com") user.save() - ct = ContentType.objects.get_for_model(User) - LogEntry.objects.log_action(user.pk, ct.pk, user.pk, repr(user), 1) + LogEntry.objects.log_actions(user.pk, [user], 1, single_object=True) context = Context({"log_entries": LogEntry.objects.all()}) t = Template( "{% load log %}" @@ -1646,7 +1644,7 @@ class GetAdminLogTests(TestCase): "{{ entry|safe }}" "{% endfor %}" ) - self.assertEqual(t.render(context), "Added “”.") + self.assertEqual(t.render(context), "Added “jondoe”.") def test_missing_args(self): msg = "'get_admin_log' statements require two arguments" diff --git a/tests/admin_utils/models.py b/tests/admin_utils/models.py index 243f314b03..8e812e27eb 100644 --- a/tests/admin_utils/models.py +++ b/tests/admin_utils/models.py @@ -1,4 +1,5 @@ from django.contrib import admin +from django.contrib.admin.models import LogEntry, LogEntryManager from django.db import models from django.utils.translation import gettext_lazy as _ @@ -86,3 +87,26 @@ class VehicleMixin(Vehicle): class Car(VehicleMixin): pass + + +class InheritedLogEntryManager(LogEntryManager): + model = LogEntry + + def log_action( + self, + user_id, + content_type_id, + object_id, + object_repr, + action_flag, + change_message="", + ): + return LogEntry.objects.create( + user_id=user_id, + content_type_id=content_type_id, + object_id=str(object_id), + # Changing actual repr to test repr + object_repr="Test Repr", + action_flag=action_flag, + change_message=change_message, + ) diff --git a/tests/admin_utils/test_logentry.py b/tests/admin_utils/test_logentry.py index b700fe54b9..20bbcccb1c 100644 --- a/tests/admin_utils/test_logentry.py +++ b/tests/admin_utils/test_logentry.py @@ -8,9 +8,10 @@ from django.contrib.contenttypes.models import ContentType from django.test import TestCase, override_settings from django.urls import reverse from django.utils import translation +from django.utils.deprecation import RemovedInDjango60Warning from django.utils.html import escape -from .models import Article, ArticleProxy, Car, Site +from .models import Article, ArticleProxy, Car, InheritedLogEntryManager, Site @override_settings(ROOT_URLCONF="admin_utils.urls") @@ -26,14 +27,22 @@ class LogEntryTests(TestCase): title="Title", created=datetime(2008, 3, 12, 11, 54), ) - content_type_pk = ContentType.objects.get_for_model(Article).pk - LogEntry.objects.log_action( + cls.a2 = Article.objects.create( + site=cls.site, + title="Title 2", + created=datetime(2009, 3, 12, 11, 54), + ) + cls.a3 = Article.objects.create( + site=cls.site, + title="Title 3", + created=datetime(2010, 3, 12, 11, 54), + ) + LogEntry.objects.log_actions( cls.user.pk, - content_type_pk, - cls.a1.pk, - repr(cls.a1), + [cls.a1], CHANGE, change_message="Changed something", + single_object=True, ) def setUp(self): @@ -227,18 +236,95 @@ class LogEntryTests(TestCase): logentry = LogEntry.objects.first() self.assertEqual(repr(logentry), str(logentry.action_time)) + # RemovedInDjango60Warning. def test_log_action(self): - content_type_pk = ContentType.objects.get_for_model(Article).pk - log_entry = LogEntry.objects.log_action( - self.user.pk, - content_type_pk, - self.a1.pk, - repr(self.a1), - CHANGE, - change_message="Changed something else", - ) + msg = "LogEntryManager.log_action() is deprecated. Use log_actions() instead." + content_type_val = ContentType.objects.get_for_model(Article).pk + with self.assertWarnsMessage(RemovedInDjango60Warning, msg): + log_entry = LogEntry.objects.log_action( + self.user.pk, + content_type_val, + self.a1.pk, + repr(self.a1), + CHANGE, + change_message="Changed something else", + ) self.assertEqual(log_entry, LogEntry.objects.latest("id")) + def test_log_actions(self): + queryset = Article.objects.all().order_by("-id") + msg = "Deleted Something" + content_type = ContentType.objects.get_for_model(self.a1) + self.assertEqual(len(queryset), 3) + with self.assertNumQueries(1): + LogEntry.objects.log_actions( + self.user.pk, + queryset, + DELETION, + change_message=msg, + ) + logs = ( + LogEntry.objects.filter(action_flag=DELETION) + .order_by("id") + .values_list( + "user", + "content_type", + "object_id", + "object_repr", + "action_flag", + "change_message", + ) + ) + expected_log_values = [ + ( + self.user.pk, + content_type.id, + str(obj.pk), + str(obj), + DELETION, + msg, + ) + for obj in queryset + ] + self.assertSequenceEqual(logs, expected_log_values) + + # RemovedInDjango60Warning. + def test_log_action_fallback(self): + LogEntry.objects2 = InheritedLogEntryManager() + queryset = Article.objects.all().order_by("-id") + content_type = ContentType.objects.get_for_model(self.a1) + self.assertEqual(len(queryset), 3) + msg = ( + "The usage of log_action() is deprecated. Implement log_actions() instead." + ) + with self.assertNumQueries(3): + with self.assertWarnsMessage(RemovedInDjango60Warning, msg): + LogEntry.objects2.log_actions(self.user.pk, queryset, DELETION) + log_values = ( + LogEntry.objects.filter(action_flag=DELETION) + .order_by("id") + .values_list( + "user", + "content_type", + "object_id", + "object_repr", + "action_flag", + "change_message", + ) + ) + expected_log_values = [ + ( + self.user.pk, + content_type.id, + str(obj.pk), + "Test Repr", + DELETION, + "", + ) + for obj in queryset + ] + self.assertSequenceEqual(log_values, expected_log_values) + def test_recentactions_without_content_type(self): """ If a LogEntry is missing content_type it will not display it in span @@ -248,7 +334,7 @@ class LogEntryTests(TestCase): link = reverse("admin:admin_utils_article_change", args=(quote(self.a1.pk),)) should_contain = """%s""" % ( escape(link), - escape(repr(self.a1)), + escape(str(self.a1)), ) self.assertContains(response, should_contain) should_contain = "Article" @@ -320,28 +406,29 @@ class LogEntryTests(TestCase): self.assertEqual(log.get_action_flag_display(), display_name) def test_hook_get_log_entries(self): - LogEntry.objects.log_action( + LogEntry.objects.log_actions( self.user.pk, - ContentType.objects.get_for_model(Article).pk, - self.a1.pk, - "Article changed", + [self.a1], CHANGE, change_message="Article changed message", + single_object=True, ) c1 = Car.objects.create() - LogEntry.objects.log_action( + LogEntry.objects.log_actions( self.user.pk, - ContentType.objects.get_for_model(Car).pk, - c1.pk, - "Car created", + [c1], ADDITION, change_message="Car created message", + single_object=True, ) + exp_str_article = escape(str(self.a1)) + exp_str_car = escape(str(c1)) + response = self.client.get(reverse("admin:index")) - self.assertContains(response, "Article changed") - self.assertContains(response, "Car created") + self.assertContains(response, exp_str_article) + self.assertContains(response, exp_str_car) # site "custom_admin" only renders log entries of registered models response = self.client.get(reverse("custom_admin:index")) - self.assertContains(response, "Article changed") - self.assertNotContains(response, "Car created") + self.assertContains(response, exp_str_article) + self.assertNotContains(response, exp_str_car) diff --git a/tests/admin_views/test_actions.py b/tests/admin_views/test_actions.py index 35e4583d95..8e1fc144e4 100644 --- a/tests/admin_views/test_actions.py +++ b/tests/admin_views/test_actions.py @@ -4,9 +4,11 @@ from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME from django.contrib.admin.views.main import IS_POPUP_VAR from django.contrib.auth.models import Permission, User from django.core import mail +from django.db import connection from django.template.loader import render_to_string from django.template.response import TemplateResponse from django.test import TestCase, override_settings +from django.test.utils import CaptureQueriesContext from django.urls import reverse from .admin import SubscriberAdmin @@ -74,8 +76,21 @@ class AdminActionsTest(TestCase): self.assertContains(confirmation, "
  • Subscribers: 2
    • ") self.assertContains(confirmation, "
  • External subscribers: 1
    • ") self.assertContains(confirmation, ACTION_CHECKBOX_NAME, count=2) - self.client.post( - reverse("admin:admin_views_subscriber_changelist"), delete_confirmation_data + with CaptureQueriesContext(connection) as ctx: + self.client.post( + reverse("admin:admin_views_subscriber_changelist"), + delete_confirmation_data, + ) + # Log entries are inserted in bulk. + self.assertEqual( + len( + [ + q["sql"] + for q in ctx.captured_queries + if q["sql"].startswith("INSERT") + ] + ), + 1, ) self.assertEqual(Subscriber.objects.count(), 0) diff --git a/tests/admin_views/test_history_view.py b/tests/admin_views/test_history_view.py index ddfdd56ca6..dfac3530bf 100644 --- a/tests/admin_views/test_history_view.py +++ b/tests/admin_views/test_history_view.py @@ -1,7 +1,6 @@ from django.contrib.admin.models import CHANGE, LogEntry from django.contrib.admin.tests import AdminSeleniumTestCase from django.contrib.auth.models import User -from django.contrib.contenttypes.models import ContentType from django.core.paginator import Paginator from django.test import TestCase, override_settings from django.urls import reverse @@ -61,15 +60,13 @@ class SeleniumTests(AdminSeleniumTestCase): password="secret", email="super@example.com", ) - content_type_pk = ContentType.objects.get_for_model(User).pk for i in range(1, 1101): - LogEntry.objects.log_action( + LogEntry.objects.log_actions( self.superuser.pk, - content_type_pk, - self.superuser.pk, - repr(self.superuser), + [self.superuser], CHANGE, change_message=f"Changed something {i}", + single_object=True, ) self.admin_login( username="super", diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 053270db40..ecb177e47f 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -3808,33 +3808,27 @@ class AdminViewStringPrimaryKeyTest(TestCase): r"""-_.!~*'() ;/?:@&=+$, <>#%" {}|\^[]`""" ) cls.m1 = ModelWithStringPrimaryKey.objects.create(string_pk=cls.pk) - content_type_pk = ContentType.objects.get_for_model( - ModelWithStringPrimaryKey - ).pk user_pk = cls.superuser.pk - LogEntry.objects.log_action( + LogEntry.objects.log_actions( user_pk, - content_type_pk, - cls.pk, - cls.pk, + [cls.m1], 2, change_message="Changed something", + single_object=True, ) - LogEntry.objects.log_action( + LogEntry.objects.log_actions( user_pk, - content_type_pk, - cls.pk, - cls.pk, + [cls.m1], 1, change_message="Added something", + single_object=True, ) - LogEntry.objects.log_action( + LogEntry.objects.log_actions( user_pk, - content_type_pk, - cls.pk, - cls.pk, + [cls.m1], 3, change_message="Deleted something", + single_object=True, ) def setUp(self): diff --git a/tests/modeladmin/tests.py b/tests/modeladmin/tests.py index e4369dbe2f..ec46b04c2e 100644 --- a/tests/modeladmin/tests.py +++ b/tests/modeladmin/tests.py @@ -833,12 +833,59 @@ class ModelAdminTests(TestCase): self.assertEqual(fetched.change_message, str(message)) self.assertEqual(fetched.object_repr, str(self.band)) + def test_log_deletions(self): + ma = ModelAdmin(Band, self.site) + mock_request = MockRequest() + mock_request.user = User.objects.create(username="akash") + content_type = get_content_type_for_model(self.band) + Band.objects.create( + name="The Beatles", + bio="A legendary rock band from Liverpool.", + sign_date=date(1962, 1, 1), + ) + Band.objects.create( + name="Mohiner Ghoraguli", + bio="A progressive rock band from Calcutta.", + sign_date=date(1975, 1, 1), + ) + queryset = Band.objects.all().order_by("-id")[:3] + self.assertEqual(len(queryset), 3) + with self.assertNumQueries(1): + ma.log_deletions(mock_request, queryset) + logs = ( + LogEntry.objects.filter(action_flag=DELETION) + .order_by("id") + .values_list( + "user_id", + "content_type", + "object_id", + "object_repr", + "action_flag", + "change_message", + ) + ) + expected_log_values = [ + ( + mock_request.user.id, + content_type.id, + str(obj.pk), + str(obj), + DELETION, + "", + ) + for obj in queryset + ] + self.assertSequenceEqual(logs, expected_log_values) + + # RemovedInDjango60Warning. def test_log_deletion(self): ma = ModelAdmin(Band, self.site) mock_request = MockRequest() mock_request.user = User.objects.create(username="bill") content_type = get_content_type_for_model(self.band) - created = ma.log_deletion(mock_request, self.band, str(self.band)) + msg = "ModelAdmin.log_deletion() is deprecated. Use log_deletions() instead." + with self.assertWarnsMessage(RemovedInDjango60Warning, msg): + created = ma.log_deletion(mock_request, self.band, str(self.band)) fetched = LogEntry.objects.filter(action_flag=DELETION).latest("id") self.assertEqual(created, fetched) self.assertEqual(fetched.action_flag, DELETION) @@ -848,6 +895,60 @@ class ModelAdminTests(TestCase): self.assertEqual(fetched.change_message, "") self.assertEqual(fetched.object_repr, str(self.band)) + # RemovedInDjango60Warning. + def test_log_deletion_fallback(self): + class InheritedModelAdmin(ModelAdmin): + def log_deletion(self, request, obj, object_repr): + return super().log_deletion(request, obj, object_repr) + + ima = InheritedModelAdmin(Band, self.site) + mock_request = MockRequest() + mock_request.user = User.objects.create(username="akash") + content_type = get_content_type_for_model(self.band) + Band.objects.create( + name="The Beatles", + bio="A legendary rock band from Liverpool.", + sign_date=date(1962, 1, 1), + ) + Band.objects.create( + name="Mohiner Ghoraguli", + bio="A progressive rock band from Calcutta.", + sign_date=date(1975, 1, 1), + ) + queryset = Band.objects.all().order_by("-id")[:3] + self.assertEqual(len(queryset), 3) + msg = ( + "The usage of log_deletion() is deprecated. Implement log_deletions() " + "instead." + ) + with self.assertNumQueries(3): + with self.assertWarnsMessage(RemovedInDjango60Warning, msg): + ima.log_deletions(mock_request, queryset) + logs = ( + LogEntry.objects.filter(action_flag=DELETION) + .order_by("id") + .values_list( + "user_id", + "content_type", + "object_id", + "object_repr", + "action_flag", + "change_message", + ) + ) + expected_log_values = [ + ( + mock_request.user.id, + content_type.id, + str(obj.pk), + str(obj), + DELETION, + "", + ) + for obj in queryset + ] + self.assertSequenceEqual(logs, expected_log_values) + def test_get_autocomplete_fields(self): class NameAdmin(ModelAdmin): search_fields = ["name"]