Fix M2M rollback: prevent bad rewriting of history_m2m

4 files changed, 90 insertions, 37 deletions

diff --git a/archaeological_finds/models_finds.py b/archaeological_finds/models_finds.py
index 803f885b4..9f81a5ecf 100644
--- a/archaeological_finds/models_finds.py
+++ b/archaeological_finds/models_finds.py
@@ -21,7 +21,6 @@ import datetime
 
 from django.conf import settings
 from django.contrib.gis.db import models
-from django.contrib.postgres.fields import JSONField
 from django.core.urlresolvers import reverse
 from django.db import connection
 from django.db.models import Max, Q, F
@@ -995,8 +994,6 @@ class Find(BulkUpdatedItem, ValueGetter, BaseHistorizedItem, OwnPerms,
             EXTRA_REQUEST_KEYS[unicode(v[0])] = v[1]
             deactivate()
 
-    EXTRA_REQUEST_FUNC = {""}
-
     PARENT_SEARCH_VECTORS = ['base_finds']
     BASE_SEARCH_VECTORS = [
         "cached_label", "label", "description", "container__location__name",
diff --git a/archaeological_finds/tests.py b/archaeological_finds/tests.py
index 2f485f36e..a86890839 100644
--- a/archaeological_finds/tests.py
+++ b/archaeological_finds/tests.py
@@ -17,6 +17,7 @@
 
 # See the file COPYING for details.
 
+import datetime
 import json
 
 from django.conf import settings
@@ -875,6 +876,7 @@ class FindHistoryTest(FindInit, TestCase):
         find.material_types.add(glass)
         d1_txt, d2_txt = self._add_datings(find)
 
+        find = models.Find.objects.get(pk=find.pk)
         self.assertIsNotNone(find.history_m2m)
         self.assertIn('material_types', find.history_m2m)
         self.assertIn(
@@ -886,6 +888,7 @@ class FindHistoryTest(FindInit, TestCase):
             find.history_m2m['datings'],
             [[d1_txt, d2_txt],   # order do not
              [d2_txt, d1_txt]])  # matter
+        self.assertEqual(find.history.count(), nb_hist + 1)
 
         historical_material_types = find.history_m2m['material_types']
 
@@ -908,10 +911,7 @@ class FindHistoryTest(FindInit, TestCase):
         self.assertEqual(find.history.all()[0].history_m2m['material_types'],
                          ["glass"])
 
-    def test_m2m_history_display(self):
-        c = Client()
-        user = self.get_default_user()
-        find = self.finds[0]
+    def _init_m2m(self, find, user):
         ceram = models.MaterialType.objects.get(txt_idx='ceramic').pk
         glass = models.MaterialType.objects.get(txt_idx='glass').pk
 
@@ -924,7 +924,7 @@ class FindHistoryTest(FindInit, TestCase):
         find.save()
         find.material_types.add(ceram)
         find.material_types.add(glass)
-        self._add_datings(find)
+        self.d1_dct, self.d2_dct = self._add_datings(find)
 
         find = models.Find.objects.get(pk=find.pk)
         find.history_modifier = user
@@ -936,6 +936,12 @@ class FindHistoryTest(FindInit, TestCase):
         find.datings.clear()
         find.material_types.remove(ceram)
 
+    def test_m2m_history_display(self):
+        c = Client()
+        user = self.get_default_user()
+        find = self.finds[0]
+        self._init_m2m(find, user)
+
         find = models.Find.objects.get(pk=find.pk)
         history_date = find.history.order_by('-history_date').all()[
             1].history_date.strftime('%Y-%m-%dT%H:%M:%S.%f')
@@ -960,14 +966,52 @@ class FindHistoryTest(FindInit, TestCase):
         self.assertIn('class="card sheet"', response.content)
         content = response.content.decode('utf-8')
         self.assertIn(
-            models.MaterialType.objects.get(txt_idx='ceramic').label, content)
+            models.MaterialType.objects.get(txt_idx='ceramic').label, content,
+            msg=u"ceramic not found in historical sheet")
         self.assertIn("5001", content, msg=u"5001 not found in historical "
                                            u"sheet")
         self.assertIn(
             Period.objects.get(txt_idx='neolithic').label, content)
 
     def test_m2m_history_restore(self):
-        pass
+        user = self.get_default_user()
+        find = self.finds[0]
+        self._init_m2m(find, user)
+
+        find = models.Find.objects.get(pk=find.pk)
+        ceram = models.MaterialType.objects.get(txt_idx='ceramic')
+        glass = models.MaterialType.objects.get(txt_idx='glass')
+
+        materials = list(find.material_types.all())
+        self.assertNotIn(ceram, materials)
+        self.assertIn(glass, materials)
+        self.assertEqual(list(find.datings.all()), [])
+
+        history_date = find.history.order_by('-history_date').all()[
+            1].history_date
+        find.rollback(history_date)
+
+        find = models.Find.objects.get(pk=find.pk)
+        materials = list(find.material_types.all())
+        self.assertIn(ceram, materials)
+        self.assertIn(glass, materials)
+
+        current_datings = []
+        for dating in find.datings.all():
+            dating_dct = {}
+            for k in self.d1_dct.keys():
+                if not getattr(dating, k):
+                    dating_dct[k] = ''
+                    continue
+                dating_dct[k] = getattr(dating, k)
+                if hasattr(dating_dct[k], 'txt_idx'):
+                    dating_dct[k] = dating_dct[k].txt_idx
+                dating_dct[k] = unicode(dating_dct[k])
+
+            current_datings.append(dating_dct)
+
+        self.assertIn(self.d1_dct, current_datings)
+        self.assertIn(self.d2_dct, current_datings)
 
 
 class TreatmentTest(FindInit, TestCase):
diff --git a/ishtar_common/models.py b/ishtar_common/models.py
index ef7dc8a4e..528ba6512 100644
--- a/ishtar_common/models.py
+++ b/ishtar_common/models.py
@@ -23,9 +23,11 @@ Models description
 import copy
 import datetime
 import inspect
+from importlib import import_module
 from jinja2 import TemplateSyntaxError
 import logging
 import os
+import pytz
 import re
 import shutil
 import tempfile
@@ -33,6 +35,7 @@ import time
 from cStringIO import StringIO
 from subprocess import Popen, PIPE
 
+
 from PIL import Image
 from django.conf import settings
 from django.contrib.auth.models import User, Group
@@ -53,6 +56,7 @@ from django.db.utils import DatabaseError
 from django.template.defaultfilters import slugify
 from django.utils.functional import lazy
 from django.utils.safestring import SafeUnicode, mark_safe
+#from django.utils.timezone import now, timedelta
 from django.utils.translation import ugettext_lazy as _, ugettext, \
     pgettext_lazy, activate, deactivate
 from secretary import Renderer as SecretaryRenderer
@@ -152,11 +156,15 @@ class HistoryModel(models.Model):
     class Meta:
         abstract = True
 
-    def m2m_listing(self, key):
+    def m2m_listing(self, key, create=False):
         if not self.history_m2m or key not in self.history_m2m:
             return
-        related_model = getattr(self, key).model
-        return related_model.history_decompress(self.history_m2m[key])
+        models = import_module(self.__class__.__module__ + ".models")
+        model = getattr(
+            models, self.__class__.__name__[len('Historical'):])
+        related_model = getattr(model, key).rel.model
+        return related_model.history_decompress(self.history_m2m[key],
+                                                create=create)
 
 
 class HistoricalRecords(BaseHistoricalRecords):
@@ -167,6 +175,7 @@ class HistoricalRecords(BaseHistoricalRecords):
         except (User.DoesNotExist, AssertionError):
             # on batch removing of users, user could have disappeared
             return
+        force = getattr(instance, "_force_history", False)
         manager = getattr(instance, self.manager_name)
         attrs = {}
         for field in instance._meta.fields:
@@ -174,7 +183,10 @@ class HistoricalRecords(BaseHistoricalRecords):
         q_history = instance.history \
             .filter(history_modifier_id=history_modifier.pk) \
             .order_by('-history_date', '-history_id')
+        # instance.skip_history_when_saving = True
         if not q_history.count():
+            if force:
+                delattr(instance, '_force_history')
             manager.create(history_type=type,
                            history_date=datetime.datetime.now(), **attrs)
             return
@@ -186,7 +198,6 @@ class HistoricalRecords(BaseHistoricalRecords):
         q = q_history.filter(history_date__isnull=False,
                              history_date__gt=min_history_date) \
             .order_by('-history_date', '-history_id')
-        force = getattr(instance, "_force_history", False)
         if not force and q.count():
             return
 
@@ -1590,29 +1601,26 @@ class BaseHistorizedItem(DocumentItem, FullSearch, Imported, JsonData,
                         k = k + "_id"
                     setattr(self, k, getattr(new_item, k))
 
-                # M2M history
-                if k.startwith('historical_') \
-                        and k[len('historical_'):] in field_keys:
-                    value = getattr(new_item, k)
-                    if not value:
-                        continue
-                    base_k = k[len('historical_'):]
-                    base_field = self._meta.get_field(base_k)
-                    base_field.clear()
-                    new_values = base_field.related_model.history_decompress(
-                        value, create=True
-                    )
-                    for val in new_values:
-                        base_field.add(val)
             try:
                 self.history_modifier = User.objects.get(
                     pk=new_item.history_modifier_id)
             except User.ObjectDoesNotExist:
                 pass
+            self.save()
+            saved_m2m = new_item.history_m2m.copy()
+            for hist_key in self.HISTORICAL_M2M:
+                # after each association m2m is rewrite - force the original
+                # to be reset
+                new_item.history_m2m = saved_m2m
+                values = new_item.m2m_listing(hist_key, create=True) or []
+                hist_field = getattr(self, hist_key)
+                hist_field.clear()
+                for val in values:
+                    hist_field.add(val)
             # force label regeneration
             self._cached_label_checked = False
             self.save()
-        except:
+        except ObjectDoesNotExist:
             raise HistoryError(u"The rollback has failed.")
         # clean the obsolete history
         for historized_item in to_del:
diff --git a/ishtar_common/utils.py b/ishtar_common/utils.py
index e293562ff..ba77a07e4 100644
--- a/ishtar_common/utils.py
+++ b/ishtar_common/utils.py
@@ -943,13 +943,17 @@ def m2m_historization_changed(sender, **kwargs):
                 continue
             values.append(value.history_compress())
         hist_values[attr] = values
-    # force resave of last history record
-    if hasattr(obj, 'skip_history_when_saving'):
-        delattr(obj, 'skip_history_when_saving')
     obj.history_m2m = hist_values
-    obj._force_history = True
-    q = obj.history.order_by("-history_date")
-    if q.count():
-        last = q.all()[0]
-        last.delete()
+    if getattr(obj, 'skip_history_when_saving', False):
+        # assume the last modifier is good...
+        q = obj.history.filter(
+            history_modifier_id=obj.history_modifier_id,
+        ).order_by('-history_date', '-history_id')
+        hist = q.all()[0]
+        hist.history_m2m = hist_values
+        hist.history_date = hist.last_modified = datetime.datetime.now()
+        hist.save()
+        obj.skip_history_when_saving = True
+    elif not obj.history_modifier:
+        obj.skip_history_when_saving = True
     obj.save()