Merge pull request #355 from rossmechanic/inheritedmodule

Fixes issue with tracking an inherited model

Author: Ross Mechanic

AUTHORS.rst

@@ -44,6 +44,7 @@ Authors
 - Jesse Shapiro
 - Kevin Foster
 - Shane Engelman
+- Ray Logel
 
 Background
 ==========

CHANGES.rst

@@ -6,6 +6,8 @@ Unreleased
 - Use get_queryset rather than model.objects in history_view. (gh-303)
 - Change ugettext calls in models.py to ugettext_lazy
 - Resolve issue where model references itself (gh-278)
+- Fix issue with tracking an inherited model (abstract class) (gh-269)
+- Resolve issue where model references itself (gh-278)
 
 1.9.0 (2017-06-11)
 ------------------

simple_history/models.py

@@ -73,13 +73,15 @@ def save_without_historical_record(self, *args, **kwargs):
                 save_without_historical_record)
 
     def finalize(self, sender, **kwargs):
+        inherited = False
         try:
             hint_class = self.cls
         except AttributeError:  # called via `register`
             pass
         else:
             if hint_class is not sender:  # set in concrete
-                if not (self.inherit and issubclass(sender, hint_class)):
+                inherited = (self.inherit and issubclass(sender, hint_class))
+                if not inherited:
                     return  # set in abstract
         if hasattr(sender._meta, 'simple_history_manager_attribute'):
             raise exceptions.MultipleRegistrationsError(
@@ -88,8 +90,12 @@ def finalize(self, sender, **kwargs):
                     sender._meta.object_name,
                 )
             )
-        history_model = self.create_history_model(sender)
-        module = importlib.import_module(self.module)
+        history_model = self.create_history_model(sender, inherited)
+        if inherited:
+            # Make sure history model is in same module as concrete model
+            module = importlib.import_module(history_model.__module__)
+        else:
+            module = importlib.import_module(self.module)
         setattr(module, history_model.__name__, history_model)
 
         # The HistoricalRecords object will be discarded,
@@ -103,14 +109,18 @@ def finalize(self, sender, **kwargs):
         setattr(sender, self.manager_name, descriptor)
         sender._meta.simple_history_manager_attribute = self.manager_name
 
-    def create_history_model(self, model):
+    def create_history_model(self, model, inherited):
         """
         Creates a historical model to associate with the model provided.
         """
         attrs = {'__module__': self.module}
 
         app_module = '%s.models' % model._meta.app_label
-        if model.__module__ != self.module:
+
+        if inherited:
+            # inherited use models module
+            attrs['__module__'] = model.__module__
+        elif model.__module__ != self.module:
             # registered under different app
             attrs['__module__'] = self.module
         elif app_module != self.module:

simple_history/registry_tests/tests.py

@@ -71,6 +71,21 @@ def test_accessor_override(self):
         assert hasattr(User, 'my_history_model_accessor')
 
 
+class TestInheritedModule(TestCase):
+
+    def test_using_app_label(self):
+        try:
+            from ..tests.models import HistoricalConcreteExternal
+        except ImportError:
+            self.fail("HistoricalConcreteExternal is in wrong module")
+
+    def test_default(self):
+        try:
+            from ..tests.models import HistoricalConcreteExternal2
+        except ImportError:
+            self.fail("HistoricalConcreteExternal2 is in wrong module")
+
+
 class TestTrackingInheritance(TestCase):
 
     def test_tracked_abstract_base(self):

simple_history/tests/models.py

@@ -355,6 +355,13 @@ class Meta:
         app_label = 'tests'
 
 
+class ConcreteExternal2(AbstractExternal):
+    name = models.CharField(max_length=50)
+
+    class Meta:
+        pass    # Don't set app_label to test inherited module path
+
+
 class TrackedWithAbstractBase(TrackedAbstractBaseA):
     pass
 

simple_history/tests/tests/test_models.py

@@ -4,26 +4,25 @@
 import warnings
 from datetime import datetime, timedelta
 
-import django
+from django.apps import apps
 from django.contrib.auth import get_user_model
 from django.core.files.base import ContentFile
 from django.db import models
 from django.db.models.fields.proxy import OrderWrt
 from django.test import TestCase
+
 from simple_history.models import HistoricalRecords, convert_auto_field
 from simple_history.utils import update_change_reason
-
 from ..external.models import ExternalModel2, ExternalModel4
 from ..models import (AbstractBase, AdminProfile, Book, Bookcase, Choice, City,
-                      ConcreteAttr, ConcreteUtil, Contact, ContactRegister,
+                      ConcreteAttr, ConcreteExternal, ConcreteUtil, Contact, ContactRegister,
                       Country, Document, Employee, ExternalModel1,
                       ExternalModel3, FileModel, HistoricalChoice,
                       HistoricalCustomFKError, HistoricalPoll, HistoricalState,
                       Library, MultiOneToOne, Person, Poll, PollInfo,
                       PollWithExcludeFields, Province, Restaurant, SelfFK,
                       Series, SeriesWork, State, Temperature,
                       UnicodeVerboseName, WaterLevel)
-from django.apps import apps
 
 get_model = apps.get_model
 User = get_user_model()
@@ -345,7 +344,7 @@ 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)
+            records.create_history_model(AdminProfile, False)
         except:
             self.fail("SimpleHistory should handle foreign keys to one to one"
                       "fields to integer fields without throwing an exception")
@@ -354,7 +353,7 @@ 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)
+            records.create_history_model(Bookcase, False)
         except:
             self.fail("SimpleHistory should handle foreign keys to one to one"
                       "fields to char fields without throwing an exception.")
@@ -363,7 +362,7 @@ def test_create_history_model_with_multiple_one_to_ones(self):
         records = HistoricalRecords()
         records.module = MultiOneToOne.__module__
         try:
-            records.create_history_model(MultiOneToOne)
+            records.create_history_model(MultiOneToOne, False)
         except:
             self.fail("SimpleHistory should handle foreign keys to one to one"
                       "fields to one to one fields without throwing an "
@@ -396,6 +395,10 @@ def test_register_app_label(self):
                          'external_externalmodel4')
         self.assertEqual(self.get_table_name(ExternalModel4.histories),
                          'tests_historicalexternalmodel4')
+        self.assertEqual(self.get_table_name(ConcreteExternal.objects),
+                         'tests_concreteexternal')
+        self.assertEqual(self.get_table_name(ConcreteExternal.history),
+                         'tests_historicalconcreteexternal')
 
     def test_get_model(self):
         self.assertEqual(get_model('external', 'ExternalModel1'),
@@ -418,6 +421,13 @@ def test_get_model(self):
         self.assertEqual(get_model('tests', 'HistoricalExternalModel4'),
                          ExternalModel4.histories.model)
 
+        # Test that historical model is defined within app of concrete
+        # model rather than abstract base model
+        self.assertEqual(get_model('tests', 'ConcreteExternal'),
+                         ConcreteExternal)
+        self.assertEqual(get_model('tests', 'HistoricalConcreteExternal'),
+                         ConcreteExternal.history.model)
+
 
 class HistoryManagerTest(TestCase):
     def test_most_recent(self):