Don't populate excluded fields in populate history (#410)

* Adds tests for two excluded-fields fixes:
- tests the management command on an historical record which has excluded fields
- tests the excluded fields are retrieved from the current model instance

* Adds test for foreign-key excluded field.

* Reduces number of queries made by `History-Model.get_instance`.
Previously, it was pre-loading directly related objects.

* Don't populate excluded fields while populating existing model history

* Adjustments

* Updated AUTHORS and log CHANGES

* Renames

excluded_fields -> _history_excluded_fields
update_record -> initial_history_record

* Fixed accidental deletions to CHANGES.rst and AUTHORS.rst

Author: Ross Mechanic

AUTHORS.rst

@@ -62,6 +62,8 @@ Authors
 - Mike Spainhower
 - Alexander Anikeev
 - Kyle Seever
+- Adnan Umer (@uadnan)
+- Jonathan Zvesper (@zvesp)
 
 Background
 ==========

CHANGES.rst

@@ -4,6 +4,7 @@ Changes
 Unreleased
 ----------
 - Fixed out-of-memory exception when running populate_history management command (gh-408)
+- Fix TypeError on populate_history if excluded_fields are specified (gh-410)
 
 2.1.0 (2018-06-04)
 ------------------

simple_history/manager.py

@@ -100,6 +100,7 @@ def bulk_history_create(self, objs, batch_size=None):
                 **{
                     field.attname: getattr(instance, field.attname)
                     for field in instance._meta.fields
+                    if field.name not in self.model._history_excluded_fields
                 }
             ) for instance in objs]
 

simple_history/models.py

@@ -113,7 +113,10 @@ def create_history_model(self, model, inherited):
         """
         Creates a historical model to associate with the model provided.
         """
-        attrs = {'__module__': self.module}
+        attrs = {
+            '__module__': self.module,
+            '_history_excluded_fields': self.excluded_fields
+        }
 
         app_module = '%s.models' % model._meta.app_label
 
@@ -220,10 +223,20 @@ def revert_url(self):
             )
 
         def get_instance(self):
-            return model(**{
+            attrs = {
                 field.attname: getattr(self, field.attname)
                 for field in fields.values()
-            })
+            }
+            if self._history_excluded_fields:
+                excluded_attnames = [
+                    model._meta.get_field(field).attname
+                    for field in self._history_excluded_fields
+                ]
+                values = model.objects.filter(
+                    pk=getattr(self, model._meta.pk.attname)
+                ).values(*excluded_attnames).get()
+                attrs.update(values)
+            return model(**attrs)
 
         def get_next_record(self):
             """

simple_history/tests/models.py

@@ -32,6 +32,14 @@ class PollWithExcludeFields(models.Model):
     history = HistoricalRecords(excluded_fields=['pub_date'])
 
 
+class PollWithExcludedFKField(models.Model):
+    question = models.CharField(max_length=200)
+    pub_date = models.DateTimeField('date published')
+    place = models.ForeignKey('Place', on_delete=models.CASCADE)
+
+    history = HistoricalRecords(excluded_fields=['place'])
+
+
 class Temperature(models.Model):
     location = models.CharField(max_length=200)
     temperature = models.IntegerField()

simple_history/tests/tests/test_commands.py

@@ -7,7 +7,7 @@
 
 from simple_history import models as sh_models
 from simple_history.management.commands import populate_history
-from ..models import Book, Poll, Restaurant
+from ..models import Book, Poll, PollWithExcludeFields, Restaurant
 
 
 @contextmanager
@@ -142,3 +142,13 @@ def test_stdout_printed_when_verbosity_is_not_specified(self):
                                 stdout=out, stderr=StringIO())
 
         self.assertNotEqual(out.getvalue(), '')
+
+    def test_excluded_fields(self):
+        poll = PollWithExcludeFields.objects.create(
+            question="Will this work?", pub_date=datetime.now())
+        PollWithExcludeFields.history.all().delete()
+        management.call_command(self.command_name,
+                                'tests.pollwithexcludefields', auto=True,
+                                stdout=StringIO(), stderr=StringIO())
+        initial_history_record = PollWithExcludeFields.history.all()[0]
+        self.assertEqual(initial_history_record.question, poll.question)

simple_history/tests/tests/test_models.py

@@ -40,9 +40,11 @@
     Library,
     MultiOneToOne,
     Person,
+    Place,
     Poll,
     PollInfo,
     PollWithExcludeFields,
+    PollWithExcludedFKField,
     Province,
     Restaurant,
     SelfFK,
@@ -946,3 +948,51 @@ def test_custom_table_name_from_register(self):
             self.get_table_name(ContactRegister.history),
             'contacts_register_history',
         )
+
+
+class ExcludeFieldsTest(TestCase):
+    def test_restore_pollwithexclude(self):
+        poll = PollWithExcludeFields.objects.create(question="what's up?",
+                                                    pub_date=today)
+        historical = poll.history.order_by('pk')[0]
+        with self.assertRaises(AttributeError):
+            historical.pub_date
+        original = historical.instance
+        self.assertEqual(original.pub_date, poll.pub_date)
+
+
+class ExcludeForeignKeyTest(TestCase):
+    def setUp(self):
+        self.poll = PollWithExcludedFKField.objects.create(
+            question="Is it?", pub_date=today,
+            place=Place.objects.create(name="Somewhere")
+        )
+
+    def get_first_historical(self):
+        """
+        Retrieve the idx'th HistoricalPoll, ordered by time.
+        """
+        return self.poll.history.order_by('history_date')[0]
+
+    def test_instance_fk_value(self):
+        historical = self.get_first_historical()
+        original = historical.instance
+        self.assertEqual(original.place, self.poll.place)
+
+    def test_history_lacks_fk(self):
+        historical = self.get_first_historical()
+        with self.assertRaises(AttributeError):
+            historical.place
+
+    def test_nb_queries(self):
+        with self.assertNumQueries(2):
+            historical = self.get_first_historical()
+            historical.instance
+
+    def test_changed_value_lost(self):
+        new_place = Place.objects.create(name="More precise")
+        self.poll.place = new_place
+        self.poll.save()
+        historical = self.get_first_historical()
+        instance = historical.instance
+        self.assertEqual(instance.place, new_place)