Ensure custom arguments for fields are included in historical models' fields

CHANGES.rst

@@ -7,6 +7,7 @@ Unreleased
 - Fix header on history pages when custom site_header is used (gh-448)
 - Modify `pre_create_historircal_record` to pass `history_instance` for ease of customization (gh-421)
 - Raise warning if HistoricalRecords(inherit=False) is in an abstract model (gh-341)
+- Ensure custom arguments for fields are included in historical models' fields (gh-431)
 
 2.5.1 (2018-10-19)
 ------------------

simple_history/models.py

@@ -184,29 +184,22 @@ def copy_fields(self, model):
                 field.__class__ = models.IntegerField
             if isinstance(field, models.ForeignKey):
                 old_field = field
-                field_arguments = {'db_constraint': False}
+                old_swappable = old_field.swappable
+                old_field.swappable = False
+                try:
+                    _name, _path, args, field_args = old_field.deconstruct()
+                finally:
+                    old_field.swappable = old_swappable
                 if getattr(old_field, 'one_to_one', False) \
                    or isinstance(old_field, models.OneToOneField):
                     FieldType = models.ForeignKey
                 else:
                     FieldType = type(old_field)
-                if getattr(old_field, 'to_fields', []):
-                    field_arguments['to_field'] = old_field.to_fields[0]
-                if getattr(old_field, 'db_column', None):
-                    field_arguments['db_column'] = old_field.db_column
-
-                # If old_field.remote_field.model is 'self' then we have a
-                # case where object has a foreign key to itself. In this case
-                # we need to set the `model` value of the field to a model. We
-                # can use the old_field.model value.
-                if isinstance(old_field.remote_field.model, str) and \
-                   old_field.remote_field.model == 'self':
-                    object_to = old_field.model
-                else:
-                    object_to = old_field.remote_field.model
 
-                field = FieldType(
-                    object_to,
+                # Override certain arguments passed when creating the field
+                # so that they work for the historical field.
+                field_args.update(
+                    db_constraint=False,
                     related_name='+',
                     null=True,
                     blank=True,
@@ -215,7 +208,10 @@ def copy_fields(self, model):
                     serialize=True,
                     unique=False,
                     on_delete=models.DO_NOTHING,
-                    **field_arguments
+                )
+                field = FieldType(
+                    *args,
+                    **field_args
                 )
                 field.name = old_field.name
             else:

simple_history/registry_tests/migration_test_app/migrations/0002_historicalmodelwithcustomattrforeignkey_modelwithcustomattrforeignkey.py

@@ -0,0 +1,42 @@
+# Generated by Django 2.1 on 2018-10-19 21:53
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+import simple_history.models
+from .. import models as my_models
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('migration_test_app', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='HistoricalModelWithCustomAttrForeignKey',
+            fields=[
+                ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')),
+                ('history_id', models.AutoField(primary_key=True, serialize=False)),
+                ('history_change_reason', models.CharField(max_length=100, null=True)),
+                ('history_date', models.DateTimeField()),
+                ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)),
+                ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)),
+                ('what_i_mean', my_models.CustomAttrNameForeignKey(attr_name='custom_attr_name', blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='migration_test_app.WhatIMean')),
+            ],
+            options={
+                'verbose_name': 'historical model with custom attr foreign key',
+                'ordering': ('-history_date', '-history_id'),
+                'get_latest_by': 'history_date',
+            },
+            bases=(simple_history.models.HistoricalChanges, models.Model),
+        ),
+        migrations.CreateModel(
+            name='ModelWithCustomAttrForeignKey',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('what_i_mean', my_models.CustomAttrNameForeignKey(attr_name='custom_attr_name', on_delete=django.db.models.deletion.CASCADE, to='migration_test_app.WhatIMean')),
+            ],
+        ),
+    ]

simple_history/registry_tests/migration_test_app/models.py

@@ -13,3 +13,31 @@ class WhatIMean(DoYouKnow):
 class Yar(models.Model):
     what = models.ForeignKey(WhatIMean, on_delete=models.CASCADE)
     history = HistoricalRecords()
+
+
+class CustomAttrNameForeignKey(models.ForeignKey):
+    def __init__(self, *args, **kwargs):
+        self.attr_name = kwargs.pop("attr_name", None)
+        super(CustomAttrNameForeignKey, self).__init__(*args, **kwargs)
+
+    def get_attname(self):
+        return self.attr_name or super(
+            CustomAttrNameForeignKey, self
+        ).get_attname()
+
+    def deconstruct(self):
+        name, path, args, kwargs = super(
+            CustomAttrNameForeignKey, self
+        ).deconstruct()
+        if self.attr_name:
+            kwargs["attr_name"] = self.attr_name
+        return name, path, args, kwargs
+
+
+class ModelWithCustomAttrForeignKey(models.Model):
+    what_i_mean = CustomAttrNameForeignKey(
+        WhatIMean,
+        models.CASCADE,
+        attr_name='custom_attr_name',
+    )
+    history = HistoricalRecords()

simple_history/registry_tests/tests.py

@@ -16,7 +16,8 @@
                             Restaurant, TrackedAbstractBaseA,
                             TrackedAbstractBaseB, TrackedWithAbstractBase,
                             TrackedWithConcreteBase, UserAccessorDefault,
-                            UserAccessorOverride, UUIDRegisterModel, Voter)
+                            UserAccessorOverride, UUIDRegisterModel, Voter,
+                            ModelWithCustomAttrForeignKey)
 
 get_model = apps.get_model
 User = get_user_model()
@@ -109,15 +110,15 @@ def test_tracked_abstract_base(self):
 
     def test_tracked_concrete_base(self):
         self.assertEqual(
-            [
+            sorted(
                 f.attname
                 for f in TrackedWithConcreteBase.history.model._meta.fields
-            ],
-            [
+            ),
+            sorted([
                 'id', 'trackedconcretebase_ptr_id', 'history_id',
                 'history_change_reason', 'history_date', 'history_user_id',
                 'history_type',
-            ],
+            ]),
         )
 
     def test_multiple_tracked_bases(self):
@@ -128,39 +129,55 @@ class TrackedWithMultipleAbstractBases(
 
     def test_tracked_abstract_and_untracked_concrete_base(self):
         self.assertEqual(
-            [f.attname for f in InheritTracking1.history.model._meta.fields],
-            [
+            sorted(
+                f.attname for f in InheritTracking1.history.model._meta.fields
+            ),
+            sorted([
                 'id', 'untrackedconcretebase_ptr_id', 'history_id',
                 'history_change_reason', 'history_date',
                 'history_user_id', 'history_type',
-            ],
+            ]),
         )
 
     def test_indirect_tracked_abstract_base(self):
         self.assertEqual(
-            [f.attname for f in InheritTracking2.history.model._meta.fields],
-            [
+            sorted(
+                f.attname for f in InheritTracking2.history.model._meta.fields
+            ),
+            sorted([
                 'id', 'baseinherittracking2_ptr_id', 'history_id',
                 'history_change_reason', 'history_date',
                 'history_user_id', 'history_type',
-            ],
+            ]),
         )
 
     def test_indirect_tracked_concrete_base(self):
         self.assertEqual(
-            [f.attname for f in InheritTracking3.history.model._meta.fields],
-            [
+            sorted(
+                f.attname for f in InheritTracking3.history.model._meta.fields
+            ),
+            sorted([
                 'id', 'baseinherittracking3_ptr_id', 'history_id',
                 'history_change_reason', 'history_date',
                 'history_user_id', 'history_type',
-            ],
+            ]),
         )
 
     def test_registering_with_tracked_abstract_base(self):
         with self.assertRaises(exceptions.MultipleRegistrationsError):
             register(InheritTracking4)
 
 
+class TestCustomAttrForeignKey(TestCase):
+    """ https://github.com/treyhunner/django-simple-history/issues/431 """
+
+    def test_custom_attr(self):
+        field = ModelWithCustomAttrForeignKey.history.model._meta.get_field(
+            'poll',
+        )
+        self.assertEqual(field.attr_name, 'custom_poll')
+
+
 class TestMigrate(TestCase):
 
     def test_makemigration_command(self):

simple_history/tests/models.py

@@ -59,6 +59,34 @@ def get_absolute_url(self):
         return reverse('poll-detail', kwargs={'pk': self.pk})
 
 
+class CustomAttrNameForeignKey(models.ForeignKey):
+    def __init__(self, *args, **kwargs):
+        self.attr_name = kwargs.pop("attr_name", None)
+        super(CustomAttrNameForeignKey, self).__init__(*args, **kwargs)
+
+    def get_attname(self):
+        return self.attr_name or super(
+            CustomAttrNameForeignKey, self
+        ).get_attname()
+
+    def deconstruct(self):
+        name, path, args, kwargs = super(
+            CustomAttrNameForeignKey, self
+        ).deconstruct()
+        if self.attr_name:
+            kwargs["attr_name"] = self.attr_name
+        return name, path, args, kwargs
+
+
+class ModelWithCustomAttrForeignKey(models.Model):
+    poll = CustomAttrNameForeignKey(
+        Poll,
+        models.CASCADE,
+        attr_name='custom_poll',
+    )
+    history = HistoricalRecords()
+
+
 class Temperature(models.Model):
     location = models.CharField(max_length=200)
     temperature = models.IntegerField()