create historical instance with respect to using() for multidb enviro…

[erikvw]

Modify so historical record works in a multi-database setup by respecting Django's queryset chained method using() and the using keyword argument of save() and delete().

Description

In a multi-database environment, historical instances should be created in the same DB as the parent model instance. Currently this is not the case.

For example, if settings defines multiple databases:

DATABASES={
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
    },
    'other': {
        'ENGINE': 'django.db.backends.sqlite3',
    }
},

... the following code will create historical records in database default regardless of the value passed to using():

# book1 instance is created in `default`;
# book1 historical instance is created in `default`.
book1 = Book.objects.create(isbn='1-84356-028-1')

# book2 instance is created in `other`;
# book2 historical instance is created in `default` -- which is incorrect.
book2 = Book.objects.using('other').create(isbn='1-84356-028-2')

... the same incorrect behavior is seen with save() and delete():

# book2 instance is created in `other`;
# book2 historical instance is created in `default` -- which is incorrect.
book2 = Book(isbn='1-84356-028-2')
book2.save(using='other')

Related Issue

Motivation and Context

We use this module in a multi-database environment and detected this problem.

How Has This Been Tested?

Tests have been added to test_models.py

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have run the make format command to format my code
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have added my name and/or github handle to AUTHORS.rst
• I have added my change to CHANGES.rst
• All new and existing tests passed.

[codecov-io]

Codecov Report

Merging #507 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #507   +/-   ##
=======================================
  Coverage   97.74%   97.74%           
=======================================
  Files          17       17           
  Lines         799      799           
  Branches      111      111           
=======================================
  Hits          781      781           
  Misses          8        8           
  Partials       10       10

Impacted Files	Coverage Δ
simple_history/signals.py	100% <ø> (ø)	⬆️
simple_history/models.py	99.22% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65785a0...476985c. Read the comment docs.

[rossmechanic]

Looks good to me. @ThePumpingLemma @barm @kseever want to give it another look through? Thanks for this @erikvw !

[rossmechanic]

@erikvw waiting on another review here and then I'll merge. Out of curiosity, does your AUTH_USER_MODEL live in multiple databases? This will fail when using specifies a database that doesn't have the AUTH_USER_MODEL, so I was thinking of adding the option of holding a custom field for history_user, where you can manually retrieve the user

[rossmechanic]

@erikvw looking at this in the context of #539 and I think we made a mistake. It seems that using is always passed to history, regardless of whether the use specifies using in the queryset. Which means that this PR links the history to always use the same database as the base model. I'd love to find a way to make the historical model to follow whatever the router says, unless it's specifically stated somewhere that the base model's db should be the history model's db.