Merge pull request #1038 from ddabble/bug/clean-duplicate-history-exception

Fix `KeyError` when cleaning duplicate history

Author: valberg

AUTHORS.rst

@@ -38,6 +38,7 @@ Authors
 - David Grochowski (`ThePumpingLemma <https://github.com/ThePumpingLemma>`_)
 - David Hite
 - David Smith
+- `ddabble <https://github.com/ddabble>`_
 - Dmytro Shyshov (`xahgmah <https://github.com/xahgmah>`_)
 - Edouard Richard (`vied12 <https://github.com/vied12>` _)
 - Eduardo Cuducos

CHANGES.rst

@@ -9,6 +9,8 @@ Unreleased
 - Started using ``exists`` query instead of ``count`` in ``populate_history`` command (gh-982)
 - Add basic support for many-to-many fields (gh-399)
 - Added support for Django 4.1 (gh-1021)
+- Added ``tracked_fields`` attribute to historical models (gh-1038)
+- Fixed ``KeyError`` when running ``clean_duplicate_history`` on models with ``excluded_fields`` (gh-1038)
 
 3.1.1 (2022-04-23)
 ------------------

simple_history/manager.py

@@ -155,11 +155,8 @@ def most_recent(self):
                 )
             )
         tmp = []
-        excluded_fields = getattr(self.model, "_history_excluded_fields", [])
 
-        for field in self.instance._meta.fields:
-            if field.name in excluded_fields:
-                continue
+        for field in self.model.tracked_fields:
             if isinstance(field, models.ForeignKey):
                 tmp.append(field.name + "_id")
             else:
@@ -263,8 +260,7 @@ def bulk_history_create(
                 history_type=history_type,
                 **{
                     field.attname: getattr(instance, field.attname)
-                    for field in instance._meta.fields
-                    if field.name not in self.model._history_excluded_fields
+                    for field in self.model.tracked_fields
                 },
             )
             if hasattr(self.model, "history_relation"):

simple_history/models.py

@@ -71,6 +71,8 @@ def _history_user_setter(historical_instance, user):
 
 
 class HistoricalRecords:
+    DEFAULT_MODEL_NAME_PREFIX = "Historical"
+
     thread = context = LocalContext()  # retain thread for backwards compatibility
     m2m_models = {}
 
@@ -230,7 +232,7 @@ def finalize(self, sender, **kwargs):
 
     def get_history_model_name(self, model):
         if not self.custom_model_name:
-            return f"Historical{model._meta.object_name}"
+            return f"{self.DEFAULT_MODEL_NAME_PREFIX}{model._meta.object_name}"
         # Must be trying to use a custom history model name
         if callable(self.custom_model_name):
             name = self.custom_model_name(model._meta.object_name)
@@ -274,6 +276,7 @@ def create_history_model(self, model, inherited):
             "__module__": self.module,
             "_history_excluded_fields": self.excluded_fields,
             "_history_m2m_fields": self.get_m2m_fields_from_model(model),
+            "tracked_fields": self.fields_included(model),
         }
 
         app_module = "%s.models" % model._meta.app_label
@@ -925,9 +928,7 @@ def diff_against(self, old_history, excluded_fields=None, included_fields=None):
 
         included_m2m_fields = {field.name for field in old_history._history_m2m_fields}
         if included_fields is None:
-            included_fields = {
-                f.name for f in old_history.instance_type._meta.fields if f.editable
-            }
+            included_fields = {f.name for f in old_history.tracked_fields if f.editable}
         else:
             included_m2m_fields = included_m2m_fields.intersection(included_fields)
 

simple_history/tests/tests/test_commands.py

@@ -410,6 +410,25 @@ def test_auto_cleanup_with_excluded_fields(self):
         )
         self.assertEqual(Poll.history.all().count(), 1)
 
+    def test_auto_cleanup_for_model_with_excluded_fields(self):
+        p = PollWithExcludeFields.objects.create(
+            question="Will this be deleted?", pub_date=datetime.now()
+        )
+        self.assertEqual(PollWithExcludeFields.history.all().count(), 1)
+        p.pub_date = p.pub_date + timedelta(days=1)
+        p.save()
+        self.assertEqual(PollWithExcludeFields.history.all().count(), 2)
+        out = StringIO()
+        management.call_command(
+            self.command_name, auto=True, stdout=out, stderr=StringIO()
+        )
+        self.assertEqual(
+            out.getvalue(),
+            "Removed 1 historical records for "
+            "<class 'simple_history.tests.models.PollWithExcludeFields'>\n",
+        )
+        self.assertEqual(PollWithExcludeFields.history.all().count(), 1)
+
 
 class TestCleanOldHistory(TestCase):
     command_name = "clean_old_history"

simple_history/tests/tests/test_models.py

@@ -77,6 +77,7 @@
     InheritedRestaurant,
     Library,
     ManyToManyModelOther,
+    ModelWithCustomAttrOneToOneField,
     ModelWithExcludedManyToMany,
     ModelWithFkToModelWithHistoryUsingBaseModelDb,
     ModelWithHistoryInDifferentDb,
@@ -93,6 +94,7 @@
     PollChildBookWithManyToMany,
     PollChildRestaurantWithManyToMany,
     PollInfo,
+    PollWithAlternativeManager,
     PollWithExcludedFieldsWithDefaults,
     PollWithExcludedFKField,
     PollWithExcludeFields,
@@ -894,33 +896,83 @@ def test_get_next_record_with_excluded_field(self):
 
 
 class CreateHistoryModelTests(unittest.TestCase):
+    @staticmethod
+    def create_history_model(model, inherited):
+        custom_model_name_prefix = f"Mock{HistoricalRecords.DEFAULT_MODEL_NAME_PREFIX}"
+        records = HistoricalRecords(
+            # Provide a custom history model name, to prevent name collisions
+            # with existing historical models
+            custom_model_name=lambda name: f"{custom_model_name_prefix}{name}",
+        )
+        records.module = model.__module__
+        return records.create_history_model(model, inherited)
+
+    def test_create_history_model_has_expected_tracked_files_attr(self):
+        def assert_tracked_fields_equal(model, expected_field_names):
+            from .. import models
+
+            history_model = getattr(
+                models, f"{HistoricalRecords.DEFAULT_MODEL_NAME_PREFIX}{model.__name__}"
+            )
+            self.assertListEqual(
+                [field.name for field in history_model.tracked_fields],
+                expected_field_names,
+            )
+
+        assert_tracked_fields_equal(
+            Poll,
+            ["id", "question", "pub_date"],
+        )
+        assert_tracked_fields_equal(
+            PollWithNonEditableField,
+            ["id", "question", "pub_date", "modified"],
+        )
+        assert_tracked_fields_equal(
+            PollWithExcludeFields,
+            ["id", "question", "place"],
+        )
+        assert_tracked_fields_equal(
+            PollWithExcludedFieldsWithDefaults,
+            ["id", "question"],
+        )
+        assert_tracked_fields_equal(
+            PollWithExcludedFKField,
+            ["id", "question", "pub_date"],
+        )
+        assert_tracked_fields_equal(
+            PollWithAlternativeManager,
+            ["id", "question", "pub_date"],
+        )
+        assert_tracked_fields_equal(
+            PollWithHistoricalIPAddress,
+            ["id", "question", "pub_date"],
+        )
+        assert_tracked_fields_equal(
+            ModelWithCustomAttrOneToOneField,
+            ["id", "poll"],
+        )
+
     def test_create_history_model_with_one_to_one_field_to_integer_field(self):
-        records = HistoricalRecords()
-        records.module = AdminProfile.__module__
         try:
-            records.create_history_model(AdminProfile, False)
+            self.create_history_model(AdminProfile, False)
         except Exception:
             self.fail(
                 "SimpleHistory should handle foreign keys to one to one"
                 "fields to integer fields without throwing an exception"
             )
 
     def test_create_history_model_with_one_to_one_field_to_char_field(self):
-        records = HistoricalRecords()
-        records.module = Bookcase.__module__
         try:
-            records.create_history_model(Bookcase, False)
+            self.create_history_model(Bookcase, False)
         except Exception:
             self.fail(
                 "SimpleHistory should handle foreign keys to one to one"
                 "fields to char fields without throwing an exception."
             )
 
     def test_create_history_model_with_multiple_one_to_ones(self):
-        records = HistoricalRecords()
-        records.module = MultiOneToOne.__module__
         try:
-            records.create_history_model(MultiOneToOne, False)
+            self.create_history_model(MultiOneToOne, False)
         except Exception:
             self.fail(
                 "SimpleHistory should handle foreign keys to one to one"