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

nick-traeger · 2019-03-20T14:35:23Z

Description

i have created an optional parameter "related_name" which can be used to create another field "history_relation". This will then create a relation to the original entry.

Related Issue

No related issue

Motivation and Context

This should make filtering a little easier. For example, I can get all entries where a certain user has made changes.

How Has This Been Tested?

Added some unit tests (RelatedNameTest)

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.

nick-traeger · 2019-03-29T16:32:43Z

Is everything okay with the source code and merge request? And is there a chance that the feature will be implemented?

rossmechanic · 2019-03-29T16:44:48Z

Hey @nick-traeger. Really interesting PR. I've got a backlog of these (5 currently open) that I'm going to try to handle this weekend, but I'll take a look soon and try to get this into the next release.

Initial thoughts:
• Definitely want to have some sort of warning if you have a name clash (i.e. someone does `history = historical_records(related_name=history))
• Add docs

Overall, looks good tho. @ThePumpingLemma @kseever

codecov-io · 2019-04-02T08:58:34Z

Codecov Report

Merging #536 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
+ Coverage   97.71%   97.75%   +0.04%     
==========================================
  Files          17       17              
  Lines         830      847      +17     
  Branches      114      117       +3     
==========================================
+ Hits          811      828      +17     
  Misses          8        8              
  Partials       11       11

Impacted Files	Coverage Δ
simple_history/models.py	`98.99% <100%> (+0.03%)`	⬆️
simple_history/exceptions.py	`100% <100%> (ø)`	⬆️
...ory/management/commands/clean_duplicate_history.py	`100% <0%> (ø)`	⬆️
simple_history/utils.py	`100% <0%> (ø)`	⬆️
simple_history/admin.py	`98.07% <0%> (+0.03%)`	⬆️

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 2e7d9d3...63d5daf. Read the comment docs.

nick-traeger · 2019-04-02T09:03:19Z

I have implemented a check that prevents the related_name from being equal to the history manager.

nick-traeger · 2019-04-04T09:36:05Z

I have now extended the documentation. Is there anything else I should change?

rossmechanic · 2019-04-04T14:29:25Z

simple_history/models.py

@@ -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


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 bug which stored the id of the old instance. But now he stores None, which is no problem at all

rossmechanic · 2019-04-04T14:30:39Z

simple_history/models.py

+                    model,
+                    null=True,
+                    on_delete=models.SET_NULL,
+                    related_name=self.related_name,


should probably add db_constraint=False

…deleting

rossmechanic · 2019-04-04T20:10:39Z

simple_history/models.py

+                "history_relation": models.ForeignKey(
+                    model,
+                    null=True,
+                    on_delete=models.SET_NULL,


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.

Accepted and changed.

rossmechanic · 2019-04-05T14:27:01Z

simple_history/tests/tests/test_models.py

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


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

rossmechanic · 2019-04-05T14:58:09Z

simple_history/tests/tests/test_models.py

+        self.assertEqual(Street.log.count(), 4)
+
+    def test_revert(self):
+        id = self.one.pk


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())

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

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

rossmechanic · 2019-04-05T15:53:40Z

CHANGES.rst

@@ -3,6 +3,10 @@ Changes

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

+2.7.1 (2019-03-20)


The should be Unreleased until we release.

Sorry I missed that! Last fix and then I'll approve lol

rossmechanic

LGTM. Thanks @nick-traeger

nick-traeger added 4 commits March 20, 2019 14:36

added the possibility to create a relation to the original model

f084956

added some tests for related_name

373c851

reformatted using black

45c5e9b

added myself to AUTHORS.rst

0d6d6f8

Preventing the related name from being the same as the history manager

828d72a

nick-traeger added 4 commits April 2, 2019 14:26

Added test to check related name for equality to manager

78aa382

Rearranged myself in alphabetical order

6834542

Added changes to CHANGES.rst

0db3385

Added some documentation

d0904b3

rossmechanic reviewed Apr 4, 2019

View reviewed changes

db_constraint added and do not set instance of history_relation when …

9f09c29

…deleting

barm added the accepted Issue accepted for completion label Apr 4, 2019

rossmechanic reviewed Apr 4, 2019

View reviewed changes

Keeping relation after delete

eb25d2f

rossmechanic reviewed Apr 5, 2019

View reviewed changes

Test if the relation works again after deleting and restoring the object

a6fe107

rossmechanic reviewed Apr 5, 2019

View reviewed changes

Revert test extended

d66c5cc

rossmechanic reviewed Apr 5, 2019

View reviewed changes

Updated version in CHANGES to unreleased

63d5daf

rossmechanic approved these changes Apr 5, 2019

View reviewed changes

rossmechanic merged commit 6b113e3 into jazzband:master Apr 5, 2019

rossmechanic mentioned this pull request Apr 15, 2019

Fix non-backward compatible issue from 2.7.0 from GH-507 #547

Merged

11 tasks

rossmechanic mentioned this pull request May 28, 2019

Django asks for default value for history_relation field during migration after upgrade from 2.7.0 to 2.7.2 #564

Open

sreynen mentioned this pull request Aug 26, 2020

Use original model's primary key as foreign key on history model? #708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nick-traeger commented Mar 20, 2019 •

edited

Loading

nick-traeger commented Mar 29, 2019

rossmechanic commented Mar 29, 2019

codecov-io commented Apr 2, 2019 •

edited

Loading

nick-traeger commented Apr 2, 2019

nick-traeger commented Apr 4, 2019

rossmechanic Apr 4, 2019

nick-traeger Apr 4, 2019

rossmechanic Apr 4, 2019

nick-traeger Apr 4, 2019

rossmechanic Apr 4, 2019

nick-traeger Apr 5, 2019

rossmechanic Apr 5, 2019

nick-traeger Apr 5, 2019

rossmechanic Apr 5, 2019

rossmechanic Apr 5, 2019

nick-traeger Apr 5, 2019

rossmechanic Apr 5, 2019

rossmechanic Apr 5, 2019

nick-traeger Apr 5, 2019

rossmechanic left a comment

		@@ -3,6 +3,10 @@ Changes

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

		2.7.1 (2019-03-20)

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

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

Conversation

nick-traeger commented Mar 20, 2019 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

nick-traeger commented Mar 29, 2019

rossmechanic commented Mar 29, 2019

codecov-io commented Apr 2, 2019 • edited Loading

Codecov Report

nick-traeger commented Apr 2, 2019

nick-traeger commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossmechanic left a comment

Choose a reason for hiding this comment

nick-traeger commented Mar 20, 2019 •

edited

Loading

codecov-io commented Apr 2, 2019 •

edited

Loading