Refactored versioning.py (and associated templates) to improve readability and testability.

Takes inspiration from, but does not use, django-reversion-compare. We do a lot of RIGS-specific stuff that requires a lot of hacking to get working nicely with django-reversion-compare. The main example of this is event-item “many-to-one” fields. The performance difference of my code compared to django-reversion-compare was found to be negligible.

RIGS/templates/RIGS/activity_feed_data.html

@@ -33,13 +33,13 @@
                 {% endif %}
                       <p>
                       <small>
-                        {% if version.old == None %}
+                        {% if version.changes.old == None %}
                           Created
                         {% else %}
                           Changed {% include 'RIGS/version_changes.html' %} in
                         {% endif %}
                           
-                        {% include 'RIGS/object_button.html' with object=version.new %}
+                        {% include 'RIGS/object_button.html' with object=version.changes.new %}
                         {% if version.revision.comment %}
                           ({{ version.revision.comment }})
                         {% endif %}

RIGS/templates/RIGS/activity_table.html

@@ -67,16 +67,16 @@
                         
                             <tr>
                                 <td>{{ version.revision.date_created }}</td>
-                                <td><a href="{{ version.new.get_absolute_url }}">{{version.new|to_class_name}} {{ version.new.pk|stringformat:"05d" }}</a></td>
+                                <td><a href="{{ version.changes.new.get_absolute_url }}">{{version.changes.new|to_class_name}} {{ version.changes.new.pk|stringformat:"05d" }}</a></td>
-                           		<td>{{ version.version.pk }}|{{ version.revision.pk }}</td>
+                           		<td>{{ version.pk }}|{{ version.revision.pk }}</td>
                                 <td>{{ version.revision.user.name }}</td>
                                 <td>
-                                  {% if version.old == None %}
+                                  {% if version.changes.old == None %}
-                                    {{version.new|to_class_name}} Created
+                                    {{version.changes.new|to_class_name}} Created
                                   {% else %}
                                     {% include 'RIGS/version_changes.html' %}
                                   {% endif %}                        </td>
-                                  <td>{{ version.revision.comment }}</td>
+                                  <td>{{ version.changes.revision.comment }}</td>
                             </tr>
                         
         			{% endfor %}

RIGS/templates/RIGS/version_changes.html

@@ -1,21 +1,26 @@
-{% for change in version.field_changes %}
+{% if version.changes.item_changes or version.changes.field_changes or version.changes.old == None %}
 
-	<button title="Changes to {{ change.field.verbose_name }}" type="button" class="btn btn-default btn-xs" data-container="body" data-html="true" data-trigger='hover' data-toggle="popover" data-content='{% spaceless %}
+    {% for change in version.changes.field_changes %}
-        {% include "RIGS/version_changes_change.html" %}
-        {% endspaceless %}'>{{ change.field.verbose_name }}</button>
 
-{% endfor %}
+    	<button title="Changes to {{ change.field.verbose_name }}" type="button" class="btn btn-default btn-xs" data-container="body" data-html="true" data-trigger='hover' data-toggle="popover" data-content='{% spaceless %}
+            {% include "RIGS/version_changes_change.html" %}
+            {% endspaceless %}'>{{ change.field.verbose_name }}</button>
 
-{% for itemChange in version.item_changes %}
+    {% endfor %}
-    <button title="Changes to item '{% if itemChange.new %}{{ itemChange.new.name }}{% else %}{{ itemChange.old.name }}{% endif %}'" type="button" class="btn btn-default btn-xs" data-container="body" data-html="true" data-trigger='hover' data-toggle="popover" data-content='{% spaceless %}
-        <ul class="list-group">
-        {% for change in itemChange.changes %}
-              <li class="list-group-item">
-                <h4 class="list-group-item-heading">{{ change.field.verbose_name }}</h4>
-                {% include "RIGS/version_changes_change.html" %}
-              </li>
-        {% endfor %}
-        </ul>
 
-        {% endspaceless %}'>item '{% if itemChange.new %}{{ itemChange.new.name }}{% else %}{{ itemChange.old.name }}{% endif %}'</button>
+    {% for itemChange in version.changes.item_changes %}
-{% endfor %}
+        <button title="Changes to item '{% if itemChange.new %}{{ itemChange.new.name }}{% else %}{{ itemChange.old.name }}{% endif %}'" type="button" class="btn btn-default btn-xs" data-container="body" data-html="true" data-trigger='hover' data-toggle="popover" data-content='{% spaceless %}
+            <ul class="list-group">
+            {% for change in itemChange.field_changes %}
+                  <li class="list-group-item">
+                    <h4 class="list-group-item-heading">{{ change.field.verbose_name }}</h4>
+                    {% include "RIGS/version_changes_change.html" %}
+                  </li>
+            {% endfor %}
+            </ul>
+            
+            {% endspaceless %}'>item '{% if itemChange.new %}{{ itemChange.new.name }}{% else %}{{ itemChange.old.name }}{% endif %}'</button>
+    {% endfor %}
+{% else %}
+    nothing useful
+{% endif %}

RIGS/templates/RIGS/version_history.html

@@ -40,13 +40,13 @@
                 </thead>
                 <tbody>
                 	{% for version in object_list %}
-                        {% if version.item_changes or version.field_changes or version.old == None %}
+                        
                             <tr>
                            		<td>{{ version.revision.date_created }}</td>
-                                <td>{{ version.version.pk }}|{{ version.revision.pk }}</td>
+                                <td>{{ version.pk }}|{{ version.revision.pk }}</td>
                                 <td>{{ version.revision.user.name }}</td>
                                 <td>
-                                    {% if version.old == None %}
+                                    {% if version.changes.old is None %}
                                         {{object|to_class_name}} Created
                                     {% else %}
                                         {% include 'RIGS/version_changes.html' %}
@@ -56,7 +56,8 @@
                                     {{ version.revision.comment }}
                                 </td>
                             </tr>
-                        {% endif %}
+
+                        
         			{% endfor %}
 
                 </tbody>

RIGS/versioning.py

@@ -1,303 +1,259 @@
+from __future__ import unicode_literals
+
 import logging
+import datetime
 
 from django.core.exceptions import ObjectDoesNotExist
 from django.shortcuts import get_object_or_404
 from django.views import generic
-
+from django.utils.functional import cached_property
-# Versioning
-import reversion
-from reversion.models import Version
-from django.contrib.contenttypes.models import ContentType  # Used to lookup the content_type
 from django.db.models import IntegerField, EmailField, TextField
+from django.contrib.contenttypes.models import ContentType
+
+from reversion.models import Version, VersionQuerySet
 from diff_match_patch import diff_match_patch
 
 from RIGS import models
-import datetime
 
 logger = logging.getLogger('tec.pyrigs')
 
 
-def model_compare(oldObj, newObj, excluded_keys=[]):
+class FieldComparison(object):
-    # recieves two objects of the same model, and compares them. Returns an array of FieldCompare objects
+    def __init__(self, field=None, old=None, new=None):
-    try:
+        self.field = field
-        theFields = oldObj._meta.fields  # This becomes deprecated in Django 1.8!!!!!!!!!!!!! (but an alternative becomes available)
+        self._old = old
-    except AttributeError:
+        self._new = new
-        theFields = newObj._meta.fields
 
-    class FieldCompare(object):
+    def display_value(self, value):
-        def __init__(self, field=None, old=None, new=None):
+        if isinstance(self.field, IntegerField) and len(self.field.choices) > 0:
-            self.field = field
+            return [x[1] for x in self.field.choices if x[0] == value][0]
-            self._old = old
+        return value
-            self._new = new
 
-        def display_value(self, value):
+    @property
-            if isinstance(self.field, IntegerField) and len(self.field.choices) > 0:
+    def old(self):
-                return [x[1] for x in self.field.choices if x[0] == value][0]
+        return self.display_value(self._old)
-            return value
 
-        @property
+    @property
-        def old(self):
+    def new(self):
-            return self.display_value(self._old)
+        return self.display_value(self._new)
 
-        @property
+    @property
-        def new(self):
+    def long(self):
-            return self.display_value(self._new)
+        if isinstance(self.field, EmailField):
-
+            return True
-        @property
-        def long(self):
-            if isinstance(self.field, EmailField):
-                return True
-            return False
-
-        @property
-        def linebreaks(self):
-            if isinstance(self.field, TextField):
-                return True
-            return False
-
-        @property
-        def diff(self):
-            oldText = unicode(self.display_value(self._old)) or ""
-            newText = unicode(self.display_value(self._new)) or ""
-            dmp = diff_match_patch()
-            diffs = dmp.diff_main(oldText, newText)
-            dmp.diff_cleanupSemantic(diffs)
-
-            outputDiffs = []
-
-            for (op, data) in diffs:
-                if op == dmp.DIFF_INSERT:
-                    outputDiffs.append({'type': 'insert', 'text': data})
-                elif op == dmp.DIFF_DELETE:
-                    outputDiffs.append({'type': 'delete', 'text': data})
-                elif op == dmp.DIFF_EQUAL:
-                    outputDiffs.append({'type': 'equal', 'text': data})
-            return outputDiffs
-
-    changes = []
-
-    for thisField in theFields:
-        name = thisField.name
-
-        if name in excluded_keys:
-            continue  # if we're excluding this field, skip over it
-
-        try:
-            oldValue = getattr(oldObj, name, None)
-        except ObjectDoesNotExist:
-            oldValue = None
-
-        try:
-            newValue = getattr(newObj, name, None)
-        except ObjectDoesNotExist:
-            newValue = None
-
-        try:
-            bothBlank = (not oldValue) and (not newValue)
-            if oldValue != newValue and not bothBlank:
-                compare = FieldCompare(thisField, oldValue, newValue)
-                changes.append(compare)
-        except TypeError:  # logs issues with naive vs tz-aware datetimes
-            logger.error('TypeError when comparing models')
-
-    return changes
-
-
-def compare_event_items(old, new):
-    # Recieves two event version objects and compares their items, returns an array of ItemCompare objects
-
-    item_type = ContentType.objects.get_for_model(models.EventItem)
-    old_item_versions = old.revision.version_set.filter(content_type=item_type)
-    new_item_versions = new.revision.version_set.filter(content_type=item_type)
-
-    class ItemCompare(object):
-        def __init__(self, old=None, new=None, changes=None):
-            self.old = old
-            self.new = new
-            self.changes = changes
-
-    # Build some dicts of what we have
-    item_dict = {}  # build a list of items, key is the item_pk
-    for version in old_item_versions:  # put all the old versions in a list
-        if version.field_dict["event_id"] == int(old.object_id):
-            compare = ItemCompare(old=version._object_version.object)
-            item_dict[version.object_id] = compare
-
-    for version in new_item_versions:  # go through the new versions
-        if version.field_dict["event_id"] == int(new.object_id):
-            try:
-                compare = item_dict[version.object_id]  # see if there's a matching old version
-                compare.new = version._object_version.object  # then add the new version to the dictionary
-            except KeyError:  # there's no matching old version, so add this item to the dictionary by itself
-                compare = ItemCompare(new=version._object_version.object)
-
-            item_dict[version.object_id] = compare  # update the dictionary with the changes
-
-    changes = []
-    for (_, compare) in item_dict.items():
-        compare.changes = model_compare(compare.old, compare.new, ['id', 'event', 'order'])  # see what's changed
-        if len(compare.changes) >= 1:
-            changes.append(compare)  # transfer into a sequential array to make it easier to deal with later
-
-    return changes
-
-
-def get_versions_for_model(models):
-    content_types = []
-    for model in models:
-        content_types.append(ContentType.objects.get_for_model(model))
-
-    versions = reversion.models.Version.objects.filter(
-        content_type__in=content_types,
-    ).select_related("revision").order_by("-pk")
-
-    return versions
-
-
-def get_previous_version(version):
-    thisId = version.object_id
-    thisVersionId = version.pk
-
-    versions = Version.objects.get_for_object_reference(version.content_type.model_class(), thisId).all()
-
-    try:
-        previousVersions = versions.filter(revision_id__lt=version.revision_id).latest(
-            field_name='revision__date_created')
-    except ObjectDoesNotExist:
         return False
 
-    return previousVersions
+    @property
+    def linebreaks(self):
+        if isinstance(self.field, TextField):
+            return True
+        return False
+
+    @property
+    def diff(self):
+        oldText = unicode(self.display_value(self._old)) or ""
+        newText = unicode(self.display_value(self._new)) or ""
+        dmp = diff_match_patch()
+        diffs = dmp.diff_main(oldText, newText)
+        dmp.diff_cleanupSemantic(diffs)
+
+        outputDiffs = []
+
+        for (op, data) in diffs:
+            if op == dmp.DIFF_INSERT:
+                outputDiffs.append({'type': 'insert', 'text': data})
+            elif op == dmp.DIFF_DELETE:
+                outputDiffs.append({'type': 'delete', 'text': data})
+            elif op == dmp.DIFF_EQUAL:
+                outputDiffs.append({'type': 'equal', 'text': data})
+        return outputDiffs
 
 
-def get_changes_for_version(newVersion, oldVersion=None):
+class ModelComparison(object):
-    # Pass in a previous version if you already know it (for efficiancy)
-    # if not provided then it will be looked up in the database
 
-    if oldVersion == None:
+    def __init__(self, old=None, new=None, version=None, excluded_keys=[]):
-        oldVersion = get_previous_version(newVersion)
+        # recieves two objects of the same model, and compares them. Returns an array of FieldCompare objects
+        try:
+            self.fields = old._meta.get_fields()
+        except AttributeError:
+            self.fields = new._meta.get_fields()
 
-    modelClass = newVersion.content_type.model_class()
+        self.old = old
+        self.new = new
+        self.excluded_keys = excluded_keys
+        self.version = version
 
-    compare = {
+    @cached_property
-        'revision': newVersion.revision,
+    def revision(self):
-        'new': newVersion._object_version.object,
+        return self.version.revision
-        'current': modelClass.objects.filter(pk=newVersion.pk).first(),
-        'version': newVersion,
 
-        # Old things that may not be used
+    @cached_property
-        'old': None,
+    def field_changes(self):
-        'field_changes': None,
+        changes = []
-        'item_changes': None,
+        for field in self.fields:
-    }
+            field_name = field.name
 
-    if oldVersion:
+            if field_name in self.excluded_keys:
-        compare['old'] = oldVersion._object_version.object
+                continue  # if we're excluding this field, skip over it
-        compare['field_changes'] = model_compare(compare['old'], compare['new'])
-        compare['item_changes'] = compare_event_items(oldVersion, newVersion)
 
-    return compare
+            try:
+                oldValue = getattr(self.old, field_name, None)
+            except ObjectDoesNotExist:
+                oldValue = None
+
+            try:
+                newValue = getattr(self.new, field_name, None)
+            except ObjectDoesNotExist:
+                newValue = None
+
+            bothBlank = (not oldValue) and (not newValue)
+            if oldValue != newValue and not bothBlank:
+                comparison = FieldComparison(field, oldValue, newValue)
+                changes.append(comparison)
+
+        return changes
+
+    @cached_property
+    def fields_changed(self):
+        return len(self.field_changes) > 0
+
+    @cached_property
+    def item_changes(self):
+        # Recieves two event version objects and compares their items, returns an array of ItemCompare objects
+
+        item_type = ContentType.objects.get_for_model(models.EventItem)
+        old_item_versions = self.version.parent.revision.version_set.filter(content_type=item_type)
+        new_item_versions = self.version.revision.version_set.filter(content_type=item_type)
+
+        comparisonParams = {'excluded_keys': ['id', 'event', 'order']}
+
+        # Build some dicts of what we have
+        item_dict = {}  # build a list of items, key is the item_pk
+        for version in old_item_versions:  # put all the old versions in a list
+            if version.field_dict["event_id"] == int(self.new.pk):
+                compare = ModelComparison(old=version._object_version.object, **comparisonParams)
+                item_dict[version.object_id] = compare
+
+        for version in new_item_versions:  # go through the new versions
+            if version.field_dict["event_id"] == int(self.new.pk):
+                try:
+                    compare = item_dict[version.object_id]  # see if there's a matching old version
+                    compare.new = version._object_version.object  # then add the new version to the dictionary
+                except KeyError:  # there's no matching old version, so add this item to the dictionary by itself
+                    compare = ModelComparison(new=version._object_version.object, **comparisonParams)
+
+                item_dict[version.object_id] = compare  # update the dictionary with the changes
+
+        changes = []
+        for (_, compare) in item_dict.items():
+            if compare.fields_changed:
+                changes.append(compare)
+
+        return changes
+
+    @cached_property
+    def items_changed(self):
+        return len(self.item_changes) > 0
+
+    @cached_property
+    def anything_changed(self):
+        return self.fields_changed or self.items_changed
+
+
+class RIGSVersionManager(VersionQuerySet):
+    def get_for_multiple_models(self, model_array):
+        content_types = []
+        for model in model_array:
+            content_types.append(ContentType.objects.get_for_model(model))
+
+        return self.filter(content_type__in=content_types).select_related("revision").order_by("-pk")
+
+
+class RIGSVersion(Version):
+    class Meta:
+        proxy = True
+
+    objects = RIGSVersionManager.as_manager()
+
+    @cached_property
+    def parent(self):
+        thisId = self.object_id
+
+        versions = RIGSVersion.objects.get_for_object_reference(self.content_type.model_class(), thisId).select_related("revision", "revision__user").all()
+
+        try:
+            previousVersion = versions.filter(revision_id__lt=self.revision_id).latest(
+                field_name='revision__date_created')
+        except ObjectDoesNotExist:
+            return False
+
+        return previousVersion
+
+    @cached_property
+    def changes(self):
+        return ModelComparison(
+            version=self,
+            new=self._object_version.object,
+            old=self.parent._object_version.object if self.parent else None
+        )
 
 
 class VersionHistory(generic.ListView):
-    model = Version
+    model = RIGSVersion
     template_name = "RIGS/version_history.html"
     paginate_by = 25
 
     def get_queryset(self, **kwargs):
         thisModel = self.kwargs['model']
 
-        # thisObject = get_object_or_404(thisModel, pk=self.kwargs['pk'])
+        versions = RIGSVersion.objects.get_for_object_reference(thisModel, self.kwargs['pk']).select_related("revision", "revision__user").all()
-        versions = Version.objects.get_for_object_reference(thisModel, self.kwargs['pk']).all()
 
         return versions
 
     def get_context_data(self, **kwargs):
         thisModel = self.kwargs['model']
-
         context = super(VersionHistory, self).get_context_data(**kwargs)
-
-        versions = context['object_list']
         thisObject = get_object_or_404(thisModel, pk=self.kwargs['pk'])
-
-        items = []
-
-        for versionNo, thisVersion in enumerate(versions):
-            if versionNo >= len(versions) - 1:
-                thisItem = get_changes_for_version(thisVersion, None)
-            else:
-                thisItem = get_changes_for_version(thisVersion, versions[versionNo + 1])
-
-            items.append(thisItem)
-
-        context['object_list'] = items
         context['object'] = thisObject
 
         return context
 
 
 class ActivityTable(generic.ListView):
-    model = Version
+    model = RIGSVersion
     template_name = "RIGS/activity_table.html"
     paginate_by = 25
 
     def get_queryset(self):
-        versions = get_versions_for_model([models.Event, models.Venue, models.Person, models.Organisation, models.EventAuthorisation])
+        versions = RIGSVersion.objects.get_for_multiple_models([models.Event, models.Venue, models.Person, models.Organisation, models.EventAuthorisation])
         return versions
 
-    def get_context_data(self, **kwargs):
-        # Call the base implementation first to get a context
-        context = super(ActivityTable, self).get_context_data(**kwargs)
-
-        items = []
-
-        for thisVersion in context['object_list']:
-            thisItem = get_changes_for_version(thisVersion, None)
-            items.append(thisItem)
-
-        context['object_list'] = items
-
-        return context
-
 
 class ActivityFeed(generic.ListView):
-    model = Version
+    model = RIGSVersion
     template_name = "RIGS/activity_feed_data.html"
     paginate_by = 25
 
     def get_queryset(self):
-        versions = get_versions_for_model([models.Event, models.Venue, models.Person, models.Organisation, models.EventAuthorisation])
+        versions = RIGSVersion.objects.get_for_multiple_models([models.Event, models.Venue, models.Person, models.Organisation, models.EventAuthorisation])
         return versions
 
     def get_context_data(self, **kwargs):
-        maxTimeDelta = []
-
-        maxTimeDelta.append({'maxAge': datetime.timedelta(days=1), 'group': datetime.timedelta(hours=1)})
-        maxTimeDelta.append({'maxAge': None, 'group': datetime.timedelta(days=1)})
-
         # Call the base implementation first to get a context
         context = super(ActivityFeed, self).get_context_data(**kwargs)
 
+        maxTimeDelta = datetime.timedelta(hours=1)
+
         items = []
 
         for thisVersion in context['object_list']:
-            thisItem = get_changes_for_version(thisVersion, None)
+            thisVersion.withPrevious = False
-            if thisItem['item_changes'] or thisItem['field_changes'] or thisItem['old'] == None:
+            if len(items) >= 1:
-                thisItem['withPrevious'] = False
+                timeDiff = items[-1].revision.date_created - thisVersion.revision.date_created
-                if len(items) >= 1:
+                timeTogether = timeDiff < maxTimeDelta
-                    timeAgo = datetime.datetime.now(thisItem['revision'].date_created.tzinfo) - thisItem[
+                sameUser = thisVersion.revision.user_id == items[-1].revision.user_id
-                        'revision'].date_created
+                thisVersion.withPrevious = timeTogether & sameUser
-                    timeDiff = items[-1]['revision'].date_created - thisItem['revision'].date_created
-                    timeTogether = False
-                    for params in maxTimeDelta:
-                        if params['maxAge'] is None or timeAgo <= params['maxAge']:
-                            timeTogether = timeDiff < params['group']
-                            break
 
-                    sameUser = thisItem['revision'].user == items[-1]['revision'].user
+            items.append(thisVersion)
-                    thisItem['withPrevious'] = timeTogether & sameUser
-
-                items.append(thisItem)
-
-        context['object_list'] = items
 
         return context