Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added the possibility to create a relation to the original model #536

Merged
merged 14 commits into from
Apr 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Authors
- Mike Spainhower
- Nathan Villagaray-Carski (`ncvc <https://github.com/ncvc>`_)
- Nianpeng Li
- Nick Träger
- Phillip Marshall
- Rajesh Pappula
- Ray Logel
Expand Down
4 changes: 4 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ Changes

- remove reference to vendored library ``django.utils.six`` in favor of ``six`` (gh-526)

Unreleased
----------
- Added the possibility to create a relation to the original model

2.7.0 (2019-01-16)
------------------
- Add support for ``using`` chained manager method and save/delete keyword argument (gh-507)
Expand Down
15 changes: 15 additions & 0 deletions docs/querying_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,18 @@ If you want to save a model without a historical record, you can use the followi

poll = Poll(question='something')
poll.save_without_historical_record()


Filtering data using a relationship to the model
------------------------------------------------

To filter changes to the data, a relationship to the history can be established. For example, all data records in which a particular user was involved.

.. code-block:: python

class Poll(models.Model):
question = models.CharField(max_length=200)
log = HistoricalRecords(related_name='history')


Poll.objects.filter(history__history_user=4)
6 changes: 6 additions & 0 deletions simple_history/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,9 @@ class NotHistoricalModelError(TypeError):
"""No related history model found."""

pass


class RelatedNameConflictError(Exception):
"""Related name conflicting with history manager"""

pass
24 changes: 24 additions & 0 deletions simple_history/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def __init__(
history_user_id_field=None,
history_user_getter=_history_user_getter,
history_user_setter=_history_user_setter,
related_name=None,
):
self.user_set_verbose_name = verbose_name
self.user_related_name = user_related_name
Expand All @@ -93,6 +94,7 @@ def __init__(
self.user_id_field = history_user_id_field
self.user_getter = history_user_getter
self.user_setter = history_user_setter
self.related_name = related_name

if excluded_fields is None:
excluded_fields = []
Expand Down Expand Up @@ -315,6 +317,23 @@ def _get_history_user_fields(self):

return history_user_fields

def _get_history_related_field(self, model):
if self.related_name:
if self.manager_name == self.related_name:
raise exceptions.RelatedNameConflictError(
"The related name must not be called like the history manager."
)
return {
"history_relation": models.ForeignKey(
model,
on_delete=models.DO_NOTHING,
related_name=self.related_name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably add db_constraint=False

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

db_constraint=False,
)
}
else:
return {}

def get_extra_fields(self, model, fields):
"""Return dict of extra fields added to the historical record model"""

Expand Down Expand Up @@ -387,6 +406,7 @@ def get_prev_record(self):
),
}

extra_fields.update(self._get_history_related_field(model))
extra_fields.update(self._get_history_user_fields())

return extra_fields
Expand Down Expand Up @@ -432,6 +452,10 @@ def create_historical_record(self, instance, history_type, using=None):
for field in self.fields_included(instance):
attrs[field.attname] = getattr(instance, field.attname)

relation_field = getattr(manager.model, "history_relation", None)
if relation_field is not None:
attrs["history_relation"] = instance
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here when the base object is deleted? I imagine something goes wrong, since you'll have a foreign key to a null object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug which stored the id of the old instance. But now he stores None, which is no problem at all


history_instance = manager.model(
history_date=history_date,
history_type=history_type,
Expand Down
5 changes: 5 additions & 0 deletions simple_history/tests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -567,3 +567,8 @@ class ForeignKeyToSelfModel(models.Model):
"self", null=True, related_name="+", on_delete=models.CASCADE
)
history = HistoricalRecords()


class Street(models.Model):
name = models.CharField(max_length=150)
log = HistoricalRecords(related_name="history")
65 changes: 65 additions & 0 deletions simple_history/tests/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
from django.test import TestCase, override_settings
from django.urls import reverse

from simple_history import register
from simple_history.exceptions import RelatedNameConflictError
from simple_history.models import HistoricalRecords, ModelChange
from simple_history.signals import pre_create_historical_record
from simple_history.tests.custom_user.models import CustomUser
Expand Down Expand Up @@ -74,6 +76,7 @@
Series,
SeriesWork,
State,
Street,
Temperature,
UUIDDefaultModel,
UUIDModel,
Expand Down Expand Up @@ -1413,3 +1416,65 @@ def test_history_user_does_not_exist(self):

self.assertEqual(user_id, instance.history.first().history_user_id)
self.assertIsNone(instance.history.first().history_user)


class RelatedNameTest(TestCase):
def setUp(self):
self.user_one = get_user_model().objects.create(
username="username_one", email="[email protected]", password="top_secret"
)
self.user_two = get_user_model().objects.create(
username="username_two", email="[email protected]", password="top_secret"
)

self.one = Street(name="Test Street")
self.one._history_user = self.user_one
self.one.save()

self.two = Street(name="Sesame Street")
self.two._history_user = self.user_two
self.two.save()

self.one.name = "ABC Street"
self.one._history_user = self.user_two
self.one.save()

def test_relation(self):
self.assertEqual(self.one.history.count(), 2)
self.assertEqual(self.two.history.count(), 1)

def test_filter(self):
self.assertEqual(
Street.objects.filter(history__history_user=self.user_one.pk).count(), 1
)
self.assertEqual(
Street.objects.filter(history__history_user=self.user_two.pk).count(), 2
)

def test_name_equals_manager(self):
with self.assertRaises(RelatedNameConflictError):
register(Place, manager_name="history", related_name="history")

def test_deletion(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add one more test that deletes the object and then reverts it, and see if the history queries still work? Will approve after that. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

self.two.delete()

self.assertEqual(Street.log.filter(history_relation=2).count(), 2)
self.assertEqual(Street.log.count(), 4)

def test_revert(self):
id = self.one.pk
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is a little confusing to me. Was hoping for something that would tell me that all of the previous historical records are still related to the reverted object, so:

id = self.one.id 
self.one.delete()

self.assertEqual(0, Street.objects.filter(pk=id).count())

old = Street.history.filter(id=id).first()
old.history_object.save()
self.assertEqual(4, old.history.count())

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially just want to make sure that the same key is reused and all the foreign keys still link to the same base object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now test whether the object behaves as before and whether the relations still exist


self.one.delete()
self.assertEqual(
Street.objects.filter(history__history_user=self.user_one.pk).count(), 0
)
self.assertEqual(Street.objects.filter(pk=id).count(), 0)

old = Street.log.filter(id=id).first()
old.history_object.save()
self.assertEqual(
Street.objects.filter(history__history_user=self.user_one.pk).count(), 1
)

self.one = Street.objects.get(pk=id)
self.assertEqual(self.one.history.count(), 4)