Merge pull request jazzband#1128 from raunaq-sailo/feat/adding-histor…

…y-diff

Add history diff column to admin change history table

.pre-commit-config.yaml

@@ -4,8 +4,7 @@ repos:
     rev: 1.7.8
     hooks:
       - id: bandit
-        args:
-          - "-x *test*.py"
+        exclude: /.*tests/
 
   - repo: https://github.com/psf/black-pre-commit-mirror
     rev: 24.3.0

CHANGES.rst

@@ -5,6 +5,26 @@ Unreleased
 ----------
 
 - Support custom History ``Manager`` and ``QuerySet`` classes (gh-1280)
+- Renamed the (previously internal) admin template
+  ``simple_history/_object_history_list.html`` to
+  ``simple_history/object_history_list.html``, and added the field
+  ``SimpleHistoryAdmin.object_history_list_template`` for overriding it (gh-1128)
+- Deprecated the undocumented template tag ``simple_history_admin_list.display_list()``;
+  it will be removed in version 3.8 (gh-1128)
+- Added ``SimpleHistoryAdmin.get_history_queryset()`` for overriding which ``QuerySet``
+  is used to list the historical records (gh-1128)
+- Added ``SimpleHistoryAdmin.get_history_list_display()`` which returns
+  ``history_list_display`` by default, and made the latter into an actual field (gh-1128)
+- ``ModelDelta`` and ``ModelChange`` (in ``simple_history.models``) are now immutable
+  dataclasses; their signatures remain unchanged (gh-1128)
+- ``ModelDelta``'s ``changes`` and ``changed_fields`` are now sorted alphabetically by
+  field name. Also, if ``ModelChange`` is for an M2M field, its ``old`` and ``new``
+  lists are sorted by the related object. This should help prevent flaky tests. (gh-1128)
+- ``diff_against()`` has a new keyword argument, ``foreign_keys_are_objs``;
+  see usage in the docs under "History Diffing" (gh-1128)
+- Added a "Changes" column to ``SimpleHistoryAdmin``'s object history table, listing
+  the changes between each historical record of the object; see the docs under
+  "Customizing the History Admin Templates" for overriding its template context (gh-1128)
 
 3.5.0 (2024-02-19)
 ------------------

docs/admin.rst

@@ -69,6 +69,43 @@ admin class
 
 .. image:: screens/5_history_list_display.png
 
+
+Customizing the History Admin Templates
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you'd like to customize the HTML of ``SimpleHistoryAdmin``'s object history pages,
+you can override the following attributes with the names of your own templates:
+
+- ``object_history_template``: The main object history page, which includes (inserts)
+  ``object_history_list_template``.
+- ``object_history_list_template``: The table listing an object's historical records and
+  the changes made between them.
+- ``object_history_form_template``: The form pre-filled with the details of an object's
+  historical record, which also allows you to revert the object to a previous version.
+
+If you'd like to only customize certain parts of the mentioned templates, look for
+``block`` template tags in the source code that you can override - like the
+``history_delta_changes`` block in ``simple_history/object_history_list.html``,
+which lists the changes made between each historical record.
+
+Customizing Context
+^^^^^^^^^^^^^^^^^^^
+
+You can also customize the template context by overriding the following methods:
+
+- ``render_history_view()``: Called by both ``history_view()`` and
+  ``history_form_view()`` before the templates are rendered. Customize the context by
+  changing the ``context`` parameter.
+- ``history_view()``: Returns a rendered ``object_history_template``.
+  Inject context by calling the super method with the ``extra_context`` argument.
+- ``get_historical_record_context_helper()``: Returns an instance of
+  ``simple_history.template_utils.HistoricalRecordContextHelper`` that's used to format
+  some template context for each historical record displayed through ``history_view()``.
+  Customize the context by extending the mentioned class and overriding its methods.
+- ``history_form_view()``: Returns a rendered ``object_history_form_template``.
+  Inject context by calling the super method with the ``extra_context`` argument.
+
+
 Disabling the option to revert an object
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

docs/historical_model.rst

@@ -554,7 +554,6 @@ You will see the many to many changes when diffing between two historical record
     informal = Category.objects.create(name="informal questions")
     official = Category.objects.create(name="official questions")
     p = Poll.objects.create(question="what's up?")
-    p.save()
     p.categories.add(informal, official)
     p.categories.remove(informal)
 

docs/history_diffing.rst

@@ -1,24 +1,110 @@
 History Diffing
 ===============
 
-When you have two instances of the same ``HistoricalRecord`` (such as the ``HistoricalPoll`` example above),
-you can perform diffs to see what changed. This will result in a ``ModelDelta`` containing the following properties:
+When you have two instances of the same historical model
+(such as the ``HistoricalPoll`` example above),
+you can perform a diff using the ``diff_against()`` method to see what changed.
+This will return a ``ModelDelta`` object with the following attributes:
 
-1. A list with each field changed between the two historical records
-2. A list with the names of all fields that incurred changes from one record to the other
-3. the old and new records.
+- ``old_record`` and ``new_record``: The old and new history records
+- ``changed_fields``: A list of the names of all fields that were changed between
+  ``old_record`` and ``new_record``, in alphabetical order
+- ``changes``: A list of ``ModelChange`` objects - one for each field in
+  ``changed_fields``, in the same order.
+  These objects have the following attributes:
 
-This may be useful when you want to construct timelines and need to get only the model modifications.
+  - ``field``: The name of the changed field
+    (this name is equal to the corresponding field in ``changed_fields``)
+  - ``old`` and ``new``: The old and new values of the changed field
+
+    - For many-to-many fields, these values will be lists of dicts from the through
+      model field names to the primary keys of the through model's related objects.
+      The lists are sorted by the value of the many-to-many related object.
+
+This may be useful when you want to construct timelines and need to get only
+the model modifications.
 
 .. code-block:: python
 
-    p = Poll.objects.create(question="what's up?")
-    p.question = "what's up, man?"
-    p.save()
+    poll = Poll.objects.create(question="what's up?")
+    poll.question = "what's up, man?"
+    poll.save()
 
-    new_record, old_record = p.history.all()
+    new_record, old_record = poll.history.all()
     delta = new_record.diff_against(old_record)
     for change in delta.changes:
-        print("{} changed from {} to {}".format(change.field, change.old, change.new))
+        print(f"'{change.field}' changed from '{change.old}' to '{change.new}'")
+
+    # Output:
+    # 'question' changed from 'what's up?' to 'what's up, man?'
+
+``diff_against()`` also accepts the following additional arguments:
+
+- ``excluded_fields`` and ``included_fields``: These can be used to either explicitly
+  exclude or include fields from being diffed, respectively.
+- ``foreign_keys_are_objs``:
+
+  - If ``False`` (default): The diff will only contain the raw primary keys of any
+    ``ForeignKey`` fields.
+  - If ``True``: The diff will contain the actual related model objects instead of just
+    the primary keys.
+    Deleted related objects (both foreign key objects and many-to-many objects)
+    will be instances of ``DeletedObject``, which only contain a ``model`` field with a
+    reference to the deleted object's model, as well as a ``pk`` field with the value of
+    the deleted object's primary key.
+
+    Note that this will add extra database queries for each related field that's been
+    changed - as long as the related objects have not been prefetched
+    (using e.g. ``select_related()``).
+
+  A couple examples showing the difference:
+
+  .. code-block:: python
+
+      # --- Effect on foreign key fields ---
+
+      whats_up = Poll.objects.create(pk=15, name="what's up?")
+      still_around = Poll.objects.create(pk=31, name="still around?")
+
+      choice = Choice.objects.create(poll=whats_up)
+      choice.poll = still_around
+      choice.save()
+
+      new, old = choice.history.all()
+
+      default_delta = new.diff_against(old)
+      # Printing the changes of `default_delta` will output:
+      # 'poll' changed from '15' to '31'
+
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will output:
+      # 'poll' changed from 'what's up?' to 'still around?'
+
+      # Deleting all the polls:
+      Poll.objects.all().delete()
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will now output:
+      # 'poll' changed from 'Deleted poll (pk=15)' to 'Deleted poll (pk=31)'
+
+
+      # --- Effect on many-to-many fields ---
+
+      informal = Category.objects.create(pk=63, name="informal questions")
+      whats_up.categories.add(informal)
+
+      new = whats_up.history.latest()
+      old = new.prev_record
+
+      default_delta = new.diff_against(old)
+      # Printing the changes of `default_delta` will output:
+      # 'categories' changed from [] to [{'poll': 15, 'category': 63}]
+
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will output:
+      # 'categories' changed from [] to [{'poll': <Poll: what's up?>, 'category': <Category: informal questions>}]
 
-``diff_against`` also accepts 2 arguments ``excluded_fields`` and ``included_fields`` to either explicitly include or exclude fields from being diffed.
+      # Deleting all the categories:
+      Category.objects.all().delete()
+      delta_with_objs = new.diff_against(old, foreign_keys_are_objs=True)
+      # Printing the changes of `delta_with_objs` will now output:
+      # 'categories' changed from [] to [{'poll': <Poll: what's up?>, 'category': DeletedObject(model=<class 'models.Category'>, pk=63)}]

simple_history/admin.py

@@ -1,3 +1,5 @@
+from typing import Any, Sequence
+
 from django import http
 from django.apps import apps as django_apps
 from django.conf import settings
@@ -6,20 +8,27 @@
 from django.contrib.admin.utils import unquote
 from django.contrib.auth import get_permission_codename, get_user_model
 from django.core.exceptions import PermissionDenied
+from django.db.models import QuerySet
 from django.shortcuts import get_object_or_404, render
 from django.urls import re_path, reverse
 from django.utils.encoding import force_str
 from django.utils.html import mark_safe
 from django.utils.text import capfirst
 from django.utils.translation import gettext as _
 
+from .manager import HistoricalQuerySet, HistoryManager
+from .models import HistoricalChanges
+from .template_utils import HistoricalRecordContextHelper
 from .utils import get_history_manager_for_model, get_history_model_for_model
 
 SIMPLE_HISTORY_EDIT = getattr(settings, "SIMPLE_HISTORY_EDIT", False)
 
 
 class SimpleHistoryAdmin(admin.ModelAdmin):
+    history_list_display = []
+
     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"
 
     def get_urls(self):
@@ -46,41 +55,43 @@ def history_view(self, request, object_id, extra_context=None):
         pk_name = opts.pk.attname
         history = getattr(model, model._meta.simple_history_manager_attribute)
         object_id = unquote(object_id)
-        action_list = history.filter(**{pk_name: object_id})
-        if not isinstance(history.model.history_user, property):
-            # Only select_related when history_user is a ForeignKey (not a property)
-            action_list = action_list.select_related("history_user")
-        history_list_display = getattr(self, "history_list_display", [])
+        historical_records = self.get_history_queryset(
+            request, history, pk_name, object_id
+        )
+        history_list_display = self.get_history_list_display(request)
         # If no history was found, see whether this object even exists.
         try:
             obj = self.get_queryset(request).get(**{pk_name: object_id})
         except model.DoesNotExist:
             try:
-                obj = action_list.latest("history_date").instance
-            except action_list.model.DoesNotExist:
+                obj = historical_records.latest("history_date").instance
+            except historical_records.model.DoesNotExist:
                 raise http.Http404
 
         if not self.has_view_history_or_change_history_permission(request, obj):
             raise PermissionDenied
 
-        # Set attribute on each action_list entry from admin methods
+        # 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 list_entry in action_list:
-                    setattr(list_entry, history_list_entry, value_for_entry(list_entry))
+                for record in historical_records:
+                    setattr(record, history_list_entry, value_for_entry(record))
+
+        self.set_history_delta_changes(request, historical_records)
 
         content_type = self.content_type_model_cls.objects.get_for_model(
             get_user_model()
         )
-
         admin_user_view = "admin:{}_{}_change".format(
             content_type.app_label,
             content_type.model,
         )
+
         context = {
             "title": self.history_view_title(request, obj),
-            "action_list": action_list,
+            "object_history_list_template": self.object_history_list_template,
+            "historical_records": historical_records,
             "module_name": capfirst(force_str(opts.verbose_name_plural)),
             "object": obj,
             "root_path": getattr(self.admin_site, "root_path", None),
@@ -97,6 +108,73 @@ def history_view(self, request, object_id, extra_context=None):
             request, self.object_history_template, context, **extra_kwargs
         )
 
+    def get_history_queryset(
+        self, request, history_manager: HistoryManager, pk_name: str, object_id: Any
+    ) -> QuerySet:
+        """
+        Return a ``QuerySet`` of all historical records that should be listed in the
+        ``object_history_list_template`` template.
+        This is used by ``history_view()``.
+
+        :param request:
+        :param history_manager:
+        :param pk_name: The name of the original model's primary key field.
+        :param object_id: The primary key of the object whose history is listed.
+        """
+        qs: HistoricalQuerySet = history_manager.filter(**{pk_name: object_id})
+        if not isinstance(history_manager.model.history_user, property):
+            # Only select_related when history_user is a ForeignKey (not a property)
+            qs = qs.select_related("history_user")
+        # Prefetch related objects to reduce the number of DB queries when diffing
+        qs = qs._select_related_history_tracked_objs()
+        return qs
+
+    def get_history_list_display(self, request) -> Sequence[str]:
+        """
+        Return a sequence containing the names of additional fields to be displayed on
+        the object history page. These can either be fields or properties on the model
+        or the history model, or methods on the admin class.
+        """
+        return self.history_list_display
+
+    def get_historical_record_context_helper(
+        self, request, historical_record: HistoricalChanges
+    ) -> HistoricalRecordContextHelper:
+        """
+        Return an instance of ``HistoricalRecordContextHelper`` for formatting
+        the template context for ``historical_record``.
+        """
+        return HistoricalRecordContextHelper(self.model, historical_record)
+
+    def set_history_delta_changes(
+        self,
+        request,
+        historical_records: Sequence[HistoricalChanges],
+        foreign_keys_are_objs=True,
+    ):
+        """
+        Add a ``history_delta_changes`` attribute to all historical records
+        except the first (oldest) one.
+
+        :param request:
+        :param historical_records:
+        :param foreign_keys_are_objs: Passed to ``diff_against()`` when calculating
+               the deltas; see its docstring for details.
+        """
+        previous = None
+        for current in historical_records:
+            if previous is None:
+                previous = current
+                continue
+            # Related objects should have been prefetched in `get_history_queryset()`
+            delta = previous.diff_against(
+                current, foreign_keys_are_objs=foreign_keys_are_objs
+            )
+            helper = self.get_historical_record_context_helper(request, previous)
+            previous.history_delta_changes = helper.context_for_delta_changes(delta)
+
+            previous = current
+
     def history_view_title(self, request, obj):
         if self.revert_disabled(request, obj) and not SIMPLE_HISTORY_EDIT:
             return _("View history: %s") % force_str(obj)