-
-
Notifications
You must be signed in to change notification settings - Fork 478
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
Allow historical tracking to be set on abstract bases #112
Conversation
The solution looks good to me. We should probably add some more tests for other inheritance structures. For example a test that non-abstract inheritance shouldn't track history in the child. Maybe even a test for a weird case like this:
|
I would actually expect all children of Also, @paulcollinsiii mentioned in the original issue that tracking through inheritance will allow attempted multiple tracking models for the same model. We probably want a meaningful error raised when that happens so the issue and resolution are clear. Do we want models registered through Maybe we should change this feature to be opt-in? Something like |
Really looking forward to this feature. Anything I can do to help get this PR finished and ready to merge? |
+1 i would really love this feature too, let me know how i can help. |
+1, Would like to see this as well. I can provide a hand too. thx |
@naegelyd / @rocktavious / @ccarlos: Thanks for your interest! Do any of your have some specifics of how you expect the tracking to propegate? Would you expect only direct concrete subclasses to be tracked, as @treyhunner mentions above? If we looked at inheritance, I think Django makes no restriction on the model parent combinations, so we need to consider all of these:
Probably the easiest to add and support is just the first, a concrete model with a single abstract parent that defines a history manager. |
My current use case is single abstract parent that defines a history manager so, if we started with support for that, it would solve my issue but might cause more unexpected behavior. Being new to this project, I would personally assume that all five of these cases would include history tracking on my model.
I would expect the child model to include history tracking on all these cases. While I know it is the more complicated route, it seems the one that makes most sense to me. All that said, I'm not going to argue much if only the single abstract parent with child model is the only case supported originally. |
dd37d55
to
baaf672
Compare
Are there any plans to merge this in? I agree with @naegelyd that if you're inheriting, regardless of whether it's an abstract parent or not, you get the history records. I see no reason why it would be anything but that. Either way, it's been over a year since this was requested and this would really help clean my code up. |
@jgr3go: I know the changes above look simple, but Django model inheritance is surprisingly flexible. Without tests for each case we can't get it merged in. I will probably return to work on this when I have time, but if you could look at it as well feel free to branch off of it and either make a PR back to this branch or to master. |
baaf672
to
f7c1c66
Compare
Alright, to try it out... Install this branch and when using class TrackedParent(models.Model):
history = HistoricalRecords(inherit=True)
class TrackedChild(models.Model):
pass I think we can easily support history tracking on any model that is registered once (through inheritance, the In the cases where behavior is ambiguous (the same model registered multiple times to have history tracking) I'm currently opting to raise an exception, this will cause this only breaking change for current behavior. Previously calling I did not touch the handling of inherited fields, so models inheriting from concrete parents will track the reference to the parent, but none of the fields on the parent model. The exact same behavior you'll see currently if a child of a concrete model is registered directly. I'll work on getting additions to the documentation before merging this in... If anyone interested in this feature could try it out, it'd be appreciated. |
@macro1 I'd be really interested in using this feature. Is there an ETA for a merge? |
@mikeengland, no hard timeline. I was kind of hoping a few people would try it out before it gets merged in, but maybe it'll be fine. |
Allow historical tracking to be set on abstract bases
Released as 1.8.0. Thanks everyone for your interest and input! |
This may or may not be useful, but we implemented a Model abstract class that dynamically adds HistoricalRecord to subclasses with
|
Addresses #63.
@treyhunner, thoughts? It doesn't seem as bad as I was expecting.