-
-
Notifications
You must be signed in to change notification settings - Fork 483
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
Changes from 10 commits
f084956
373c851
45c5e9b
0d6d6f8
828d72a
78aa382
6834542
0db3385
d0904b3
9f09c29
eb25d2f
a6fe107
d66c5cc
63d5daf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 = [] | ||
|
@@ -315,6 +317,24 @@ 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, | ||
null=True, | ||
on_delete=models.SET_NULL, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually probably want to have models.DO_NOTHING here. If you set null, and then try to undelete an object, this pattern no longer works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted and changed. |
||
related_name=self.related_name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should probably add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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""" | ||
|
||
|
@@ -387,6 +407,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 | ||
|
@@ -432,6 +453,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 and history_type != "-": | ||
attrs["history_relation"] = instance | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -74,6 +76,7 @@ | |
Series, | ||
SeriesWork, | ||
State, | ||
Street, | ||
Temperature, | ||
UUIDDefaultModel, | ||
UUIDModel, | ||
|
@@ -1413,3 +1416,47 @@ 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="first@user.com", password="top_secret" | ||
) | ||
self.user_two = get_user_model().objects.create( | ||
username="username_two", email="second@user.com", 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
self.two.delete() | ||
|
||
self.assertEqual(Street.log.filter(history_relation__isnull=True).count(), 2) | ||
self.assertEqual(Street.log.count(), 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The should be
Unreleased
until we release.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed that! Last fix and then I'll approve lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done