Skip to content

Commit

Permalink
add variant and broken code
Browse files Browse the repository at this point in the history
  • Loading branch information
jedie committed May 10, 2012
1 parent c27fa91 commit 9c8bd85
Showing 1 changed file with 40 additions and 2 deletions.
42 changes: 40 additions & 2 deletions reversion_compare/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@
from django.utils.text import capfirst
from django.utils.translation import ugettext as _

import reversion
from reversion.admin import VersionAdmin
from reversion.models import Version

from reversion_compare.forms import SelectDiffForm
from reversion_compare.helpers import html_diff, compare_queryset
from django.conf import settings
from django.contrib.contenttypes.models import ContentType



class CompareObject(object):
Expand Down Expand Up @@ -94,6 +97,31 @@ def get_many_to_many(self):
returns a queryset with all many2many objects
"""
if self.field.get_internal_type() == "ManyToManyField": # FIXME!

"""
# FIXME:
print "-"*79
# get instance of Revision(): A group of related object versions.
old_revision = self.version.revision
assert isinstance(self.version, reversion.models.Version)
assert isinstance(old_revision, reversion.models.Revision)
# Get the related model of the current field:
related_model = self.field.rel.to
# XXX: That contains not all objects:
print "all():", old_revision.version_set.all()

This comment has been minimized.

Copy link
@etianen

etianen May 10, 2012

Contributor

If this isn't containing all the objects, then the registration code is probably wrong. Hard to tell without concrete example.

This comment has been minimized.

Copy link
@jedie

This comment has been minimized.

Copy link
@etianen

etianen May 11, 2012

Contributor

Aha! So, with that registration code, you're not set up to follow the m2m relation, which means that the related IDs, but not the content of the related model, is versioned.

If reversion is set up to follow the m2m with an explicit call to reversion.register, then you can provide a diff of the content and identity of the related models.

If reversion is not set up to follow the m2m (the default), then you'll have to load up the related models from the live database using the versioned IDs and just do a diff over which related models are referenced.

This comment has been minimized.

Copy link
@jedie

jedie May 13, 2012

Author Owner

Ah. Thanks for the info.

Question: Can i simple change the registration code? Or is this not a good idea, if reversion data already exists?

This comment has been minimized.

Copy link
@etianen

etianen via email May 14, 2012

Contributor

This comment has been minimized.

Copy link
@jedie

jedie May 14, 2012

Author Owner

I would like to support the two different data and i would like to add a info if m2m relations are "missing" (I don't know how to implement this, yet... But i will see)

queryset = old_revision.version_set.filter(
content_type=ContentType.objects.get_for_model(related_model),
)
# XXX: Contains not all objects:
print "filtered():", queryset
return queryset
"""

ids = self.value # is: version.field_dict[field.name]
related_model = self.field.rel.to
queryset = related_model.objects.all().filter(pk__in=ids)
Expand Down Expand Up @@ -379,8 +407,18 @@ def compare_view(self, request, object_id, extra_context=None):

object_id = unquote(object_id) # Underscores in primary key get quoted to "_5F"
obj = get_object_or_404(self.model, pk=object_id)
version1 = get_object_or_404(Version, pk=version_id1, object_id=unicode(obj.pk))
version2 = get_object_or_404(Version, pk=version_id2, object_id=unicode(obj.pk))

#----------------------------------------------------------------------
# XXX: Better/less better:
# variant 1:
# version1 = get_object_or_404(Version, pk=version_id1, object_id=unicode(obj.pk))
# version2 = get_object_or_404(Version, pk=version_id2, object_id=unicode(obj.pk))

# variant 2:
queryset = reversion.get_for_object(obj)
version1 = get_object_or_404(queryset, pk=version_id1)
version2 = get_object_or_404(queryset, pk=version_id2)
#----------------------------------------------------------------------

This comment has been minimized.

Copy link
@etianen

etianen May 10, 2012

Contributor

I don't think this will make any difference. Any sane database will do the lookup on the version PK, so it'll be a lookup on a unique index either way.

This comment has been minimized.

Copy link
@jedie

jedie May 15, 2012

Author Owner

I find out that there is an difference!

With variant 2 i will only get versions from the same object. In variante 1 i can change the "version id" and get no 404!

IMHO reversion.get_for_object() will filter by obj.__class__ and "variant 1" would only filter by obj.pk

Don't know if this is relevant for django-reversion, too?

With 19d7983 i use "variant 2"

This comment has been minimized.

Copy link
@etianen

etianen May 16, 2012

Contributor

Varient 2 is more strict, it's true. But in normal use case I can't see it making a difference. Unless admin users start messing with URLs to try and break things, in which case they're probably get a 500 instead of a 404.


if version_id1 > version_id2:
# Compare always the newest one with the older one
Expand Down

0 comments on commit 9c8bd85

Please sign in to comment.