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

Add Many-to-Many support #16

Closed
treyhunner opened this issue Apr 20, 2013 · 19 comments
Closed

Add Many-to-Many support #16

treyhunner opened this issue Apr 20, 2013 · 19 comments

Comments

@treyhunner
Copy link
Member

As implemented in this fork: https://bitbucket.org/joaofrancese/django-simple-history/src/0d1630812d2eed3718eabf350d27425af0521a51/simple_history/models.py?at=default

@clintonb
Copy link

Any plans to resolve this issue?

@treyhunner
Copy link
Member Author

I have no need for this feature. I only created the issue to note that it's been brought up before.

I'm not opposed to many-to-many support but I think we might want to allow it to be enabled/disabled for those that don't need it if it could cause unnecessary database accesses.

@clintonb
Copy link

@treyhunner I do have a need for this functionality, and am willing to help integrate it. Please share your thoughts on how one might enable/disable the feature.

@treyhunner
Copy link
Member Author

@clintonb it probably shouldn't be a universal feature since you might want many-to-many support on some models but not others. Maybe a keyword attribute to HistoricalRecords to enable many-to-many support. Maybe named similarly to Django forms' save_m2m method.

@yscumc
Copy link

yscumc commented Sep 18, 2014

Hi, I came across your post and thought django-simple-history seems the best suited for my use case but unfortunately I also need to store the history for m2m relationships.

Just wondering whether there are any plans to move this feature forward?

Thanks!

@treyhunner
Copy link
Member Author

@yscumc I have no interest in this for my own uses currently.

A pull request would be welcome.

James-Osgood pushed a commit to arrai-innovations/django-simple-history that referenced this issue Sep 23, 2014
@totycro
Copy link

totycro commented Mar 19, 2015

Hi, I'm also very much interested in this feature. Could you merge the pull request?

@totycro
Copy link

totycro commented Mar 19, 2015

Btw, I think that there is a typo-bug in the code mentioned at the first post.

In line 179 (https://bitbucket.org/joaofrancese/django-simple-history/src/0d1630812d2eed3718eabf350d27425af0521a51/simple_history/models.py?at=default#cl-179), it says __id__in, where it probably should say _id__in (else Django complains about nested lookups). The patch seems to work fine here with this change with version 1.5.4 (after fixing the merge conflict in the constructor due to the new parameters).

kennethzfeng pushed a commit to yext/django-simple-history that referenced this issue Apr 28, 2016
@jlachowski
Copy link

Will you add this into main? The solution seems to be mature and it would be greatly appreciated.

@macro1
Copy link
Collaborator

macro1 commented Jul 9, 2016

@jlachowski: If someone wants to use the example linked above as a starting point, and write some tests around it, it would probably be merged in.

It's not a feature I have needed yet, and it would require quite a bit of work, on testing alone. I still need to work through some bugs that were introduced in the last couple features (there is one open for abstract inheritance). I'm going to prioritize the bugs in existing features when I have time and energy to contribute.

@ghost
Copy link

ghost commented Jan 24, 2017

@macro1 I need this feature urgently and I am even willing to take the time to write tests. What should be tested in your opinion to be convincing enough to add this to main (if all tests work)?

@macro1
Copy link
Collaborator

macro1 commented Jan 24, 2017

@BenedictKuchenmeister: If you have an urgent need for it, it might be simpler to use a 'through' model for your many-to-many relationship, giving you control over things like history tracking:
https://docs.djangoproject.com/en/1.10/ref/models/fields/#django.db.models.ManyToManyField.through

Even if we were to add many-to-many support, personally I would prefer a custom 'through' model. Many-to-many relationships are really just syntactic sugar for a relationship between two models through a third one, with unique constraints on the two foreign keys. One-to-one relationships in Django are similar in this way.

@treyhunner
Copy link
Member Author

@macro1 maybe we should add an issue about updating the documentation to include your suggested solution so folks looking for this feature know how to do it. Thoughts?

@lives
Copy link

lives commented Mar 10, 2017

Does this mean, that the suggested solution "custom through model", consists in creating a custom "through model" as explained here for instance https://docs.djangoproject.com/en/1.10/topics/db/models/#intermediary-manytomany and configure the historical records both on containing model (the model that declares the ManyToMany) and the custom through model ?

from django.db import models
from simple_history.models import HistoricalRecords

class Person(models.Model):
    name = models.CharField(max_length=128)

    def __str__(self):              # __unicode__ on Python 2
        return self.name

class Group(models.Model):
    name = models.CharField(max_length=128)
    members = models.ManyToManyField(Person, through='Membership')
    history = HistoricalRecords()

    def __str__(self):              # __unicode__ on Python 2
        return self.name

class Membership(models.Model):
    person = models.ForeignKey(Person, on_delete=models.CASCADE)
    group = models.ForeignKey(Group, on_delete=models.CASCADE)
    date_joined = models.DateField()
    invite_reason = models.CharField(max_length=64)
    history = HistoricalRecords()

How would be the manner to query the Group history and retreive the members ?

@ghost
Copy link

ghost commented Mar 10, 2017

We have done that in our project and the problem with this solution was, that the history_date attributes in the model thats history you want to query (here Group) and of the "Through-Model" (MemberShip) have little time differences. We wrote a routine that gives our "Though-Model" some extra data: a history_reference_id that is the history-instance's history_id (here of Group).
We don't really like this but it was the easiest solution without applying big migrations to the db of our relatively big and complex project.

@wimfeijen
Copy link

@treyhunner an update to the documentation would be welcome!

I searched for ManyToManyField in the docs and couldn't find any reference. It would be good at least to note that ManyToManyFields are not stored in history.

@wimfeijen
Copy link

@BenedictKuchenmeister would it be possible for you to share your code example? Thanks!

@macro1 for me the proposal which specifies m2m_fields, is simpler and more elegant to implement than changing ManyToManyFields to use through models, both because of brevity of code and because it does not need complex data migrations to add through methods (which is not out-of-the-box supported by django migrations).

@hgylfason
Copy link

@BenedictKuchenmeister The time discrepancy between Group and Membership is it always so that the timestamp on Membership is later than on the related Group?

Cause if that's the case I would assume that you could get the related historical membership like this...

historical_group = group.history.get(...) # doesn't matter how we fetch the group
# get related historical membership instance
historical_membership = Membership.history.filter(history_date__gte=historical_group.history_date).earliest()

Note, this strategy isn't as performant.

@rossmechanic
Copy link
Collaborator

Closing this as dupe of #399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants