From 5dfeefee5f1fffc0fbc8d2915b153c3e0328f16f Mon Sep 17 00:00:00 2001 From: Jurrian Tromp Date: Mon, 25 Nov 2024 15:02:40 +0100 Subject: [PATCH] History view pagination (#1277) * Implement history_view pagination Borrowed from Django's object_history.html * Move pagination logic below the permission check This avoids performing any database queries before we know that the user should be able to view data. * Reworded the docs around changing the page size A developer is more likely to look for the term page size. --------- --- AUTHORS.rst | 1 + CHANGES.rst | 1 + docs/admin.rst | 24 +++- simple_history/admin.py | 17 ++- .../simple_history/object_history.html | 2 +- .../simple_history/object_history_list.html | 17 ++- simple_history/tests/tests/test_admin.py | 120 ++++++++++++++++++ 7 files changed, 176 insertions(+), 6 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 082a4fca1..49d1d90de 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -83,6 +83,7 @@ Authors - Jordon Wing (`jordonwii `_) - Josh Fyne - Josh Thomas (`joshuadavidthomas `_) +- Jurrian Tromp (`jurrian `_) - Keith Hackbarth - Kevin Foster - Kira (`kiraware `_) diff --git a/CHANGES.rst b/CHANGES.rst index 6a01c92e7..95da1b52c 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,6 +12,7 @@ Unreleased - Updated all djangoproject.com links to reference the stable version (gh-1420) - Dropped support for Python 3.8, which reached end-of-life on 2024-10-07 (gh-1421) - Added support for Django 5.1 (gh-1388) +- Added pagination to ``SimpleHistoryAdmin`` (gh-1277) 3.7.0 (2024-05-29) ------------------ diff --git a/docs/admin.rst b/docs/admin.rst index 63329d110..1bf220143 100644 --- a/docs/admin.rst +++ b/docs/admin.rst @@ -49,7 +49,7 @@ By default, the history log displays one line per change containing You can add other columns (for example the object's status to see how it evolved) by adding a ``history_list_display`` array of fields to the -admin class +admin class. .. code-block:: python @@ -62,6 +62,7 @@ admin class list_display = ["id", "name", "status"] history_list_display = ["status"] search_fields = ['name', 'user__username'] + history_list_per_page = 100 admin.site.register(Poll, PollHistoryAdmin) admin.site.register(Choice, SimpleHistoryAdmin) @@ -70,6 +71,27 @@ admin class .. image:: screens/5_history_list_display.png +Changing the page size in the admin history list view +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +By default, the history list view of ``SimpleHistoryAdmin`` shows the last 100 records. +You can change this by adding a `history_list_per_page` attribute to the admin class. + + +.. code-block:: python + + from django.contrib import admin + from simple_history.admin import SimpleHistoryAdmin + from .models import Poll + + + class PollHistoryAdmin(SimpleHistoryAdmin): + # history_list_per_page defaults to 100 + history_list_per_page = 200 + + admin.site.register(Poll, PollHistoryAdmin) + + Customizing the History Admin Templates ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/simple_history/admin.py b/simple_history/admin.py index 0c4b470c5..fdee136e8 100644 --- a/simple_history/admin.py +++ b/simple_history/admin.py @@ -7,8 +7,10 @@ from django.contrib import admin from django.contrib.admin import helpers from django.contrib.admin.utils import unquote +from django.contrib.admin.views.main import PAGE_VAR from django.contrib.auth import get_permission_codename, get_user_model from django.core.exceptions import PermissionDenied +from django.core.paginator import Paginator from django.db.models import QuerySet from django.shortcuts import get_object_or_404, render from django.urls import re_path, reverse @@ -31,6 +33,7 @@ class SimpleHistoryAdmin(admin.ModelAdmin): object_history_template = "simple_history/object_history.html" object_history_list_template = "simple_history/object_history_list.html" object_history_form_template = "simple_history/object_history_form.html" + history_list_per_page = 100 def get_urls(self): """Returns the additional urls used by the Reversion admin.""" @@ -72,14 +75,19 @@ def history_view(self, request, object_id, extra_context=None): if not self.has_view_history_or_change_history_permission(request, obj): raise PermissionDenied + # Use the same pagination as in Django admin, with history_list_per_page items + paginator = Paginator(historical_records, self.history_list_per_page) + page_obj = paginator.get_page(request.GET.get(PAGE_VAR)) + page_range = paginator.get_elided_page_range(page_obj.number) + # Set attribute on each historical record from admin methods for history_list_entry in history_list_display: value_for_entry = getattr(self, history_list_entry, None) if value_for_entry and callable(value_for_entry): - for record in historical_records: + for record in page_obj.object_list: setattr(record, history_list_entry, value_for_entry(record)) - self.set_history_delta_changes(request, historical_records) + self.set_history_delta_changes(request, page_obj) content_type = self.content_type_model_cls.objects.get_for_model( get_user_model() @@ -92,7 +100,10 @@ def history_view(self, request, object_id, extra_context=None): context = { "title": self.history_view_title(request, obj), "object_history_list_template": self.object_history_list_template, - "historical_records": historical_records, + "page_obj": page_obj, + "page_range": page_range, + "page_var": PAGE_VAR, + "pagination_required": paginator.count > self.history_list_per_page, "module_name": capfirst(force_str(opts.verbose_name_plural)), "object": obj, "root_path": getattr(self.admin_site, "root_path", None), diff --git a/simple_history/templates/simple_history/object_history.html b/simple_history/templates/simple_history/object_history.html index 58c3d4536..21eb7be56 100644 --- a/simple_history/templates/simple_history/object_history.html +++ b/simple_history/templates/simple_history/object_history.html @@ -7,7 +7,7 @@ {% if not revert_disabled %}

{% blocktrans %}Choose a date from the list below to revert to a previous version of this object.{% endblocktrans %}

{% endif %}
- {% if historical_records %} + {% if page_obj.object_list %} {% include object_history_list_template %} {% else %}

{% trans "This object doesn't have a change history." %}

diff --git a/simple_history/templates/simple_history/object_history_list.html b/simple_history/templates/simple_history/object_history_list.html index 9356ddc09..b1398be38 100644 --- a/simple_history/templates/simple_history/object_history_list.html +++ b/simple_history/templates/simple_history/object_history_list.html @@ -18,7 +18,7 @@ - {% for record in historical_records %} + {% for record in page_obj %} @@ -65,3 +65,18 @@ {% endfor %} + +

+ {% if pagination_required %} + {% for i in page_range %} + {% if i == page_obj.paginator.ELLIPSIS %} + {{ page_obj.paginator.ELLIPSIS }} + {% elif i == page_obj.number %} + {{ i }} + {% else %} + {{ i }} + {% endif %} + {% endfor %} + {% endif %} + {{ page_obj.paginator.count }} {% blocktranslate count counter=page_obj.paginator.count %}entry{% plural %}entries{% endblocktranslate %} +

diff --git a/simple_history/tests/tests/test_admin.py b/simple_history/tests/tests/test_admin.py index d85e51a54..016571a1f 100644 --- a/simple_history/tests/tests/test_admin.py +++ b/simple_history/tests/tests/test_admin.py @@ -4,6 +4,7 @@ import django from django.contrib.admin import AdminSite from django.contrib.admin.utils import quote +from django.contrib.admin.views.main import PAGE_VAR from django.contrib.auth import get_user_model from django.contrib.auth.models import Permission from django.contrib.messages.storage.fallback import FallbackStorage @@ -556,6 +557,125 @@ def test_response_change(self): self.assertEqual(response["Location"], "/awesome/url/") + def test_history_view_pagination(self): + """ + Ensure the history_view handles pagination correctly. + The default history_list_per_page is 100 so page 2 should have 1 record. + """ + # Create a Poll object and make more than 100 changes to ensure pagination + poll = Poll.objects.create(question="what?", pub_date=today) + for i in range(100): + poll.question = f"change_{i}" + poll.save() + + # Verify that there are 100+1 (initial creation) historical records + self.assertEqual(poll.history.count(), 101) + + admin_site = AdminSite() + admin = SimpleHistoryAdmin(Poll, admin_site) + + self.login(superuser=True) + + # Simulate a request to the second page + request = RequestFactory().get("/", {PAGE_VAR: "2"}) + request.user = self.user + + # Patch the render function + with patch("simple_history.admin.render") as mock_render: + admin.history_view(request, str(poll.id)) + + # Ensure the render function was called + self.assertTrue(mock_render.called) + + # Extract context passed to render function + action_list_count = len(mock_render.call_args[0][2]["page_obj"].object_list) + + # Check if only 1 (101 - 100 from the first page) + # objects are present in the context + self.assertEqual(action_list_count, 1) + + def test_history_view_pagination_no_pagination(self): + """ + When all records fit on one page because the history_list_per_page is + higher than the number of records, ensure that the pagination is not set. + But it should show the number of entries. + """ + # Create a Poll object and make more than 50 changes to ensure pagination + poll = Poll.objects.create(question="what?", pub_date=today) + for i in range(60): + poll.question = f"change_{i}" + poll.save() + + # Verify that there are 60+1 (initial creation) historical records + self.assertEqual(poll.history.count(), 61) + + # Create an admin with more per page than the number of records + class CustomSimpleHistoryAdmin(SimpleHistoryAdmin): + history_list_per_page = 200 + + admin_site = AdminSite() + admin = CustomSimpleHistoryAdmin(Poll, admin_site) + + self.login(superuser=True) + + # Simulate a request to the second page + request = RequestFactory().get("/", {PAGE_VAR: "2"}) + request.user = self.user + + response = admin.history_view(request, str(poll.id)) + + expected = '

61 entries

' + self.assertInHTML(expected, response.content.decode()) + + def test_history_view_pagination_last_page(self): + """ + With 31 records, the last page should have 1 record. Non-existing pages + also end up on the last page. + """ + # Create a Poll object and make more than 30 changes to ensure pagination + poll = Poll.objects.create(question="what?", pub_date=today) + for i in range(30): + poll.question = f"change_{i}" + poll.save() + + expected_entry_count = 31 + + # Verify that there are 30+1 (initial creation) historical records + self.assertEqual(poll.history.count(), expected_entry_count) + + # Create an admin with less per page than the number of records + class CustomSimpleHistoryAdmin(SimpleHistoryAdmin): + history_list_per_page = 10 + + admin_site = AdminSite() + admin = CustomSimpleHistoryAdmin(Poll, admin_site) + + self.login(superuser=True) + + # Simulate a request to the 4th and last page + request = RequestFactory().get("/", {PAGE_VAR: "4"}) + request.user = self.user + + response = admin.history_view(request, str(poll.id)) + + expected = ( + '

' + '1' + '2' + '3' + '4' + f"{expected_entry_count} entries" + "

" + ) + self.assertInHTML(expected, response.content.decode()) + + # Also a non-existent page should return the last page + request = RequestFactory().get("/", {PAGE_VAR: "5"}) + request.user = self.user + + response = admin.history_view(request, str(poll.id)) + self.assertInHTML(expected, response.content.decode()) + def test_response_change_change_history_setting_off(self): """ Test the response_change method that it works with a _change_history