Generic manner of managing external id

7 files changed, 76 insertions, 58 deletions

diff --git a/archaeological_context_records/models.py b/archaeological_context_records/models.py
index 8dd082e7b..2f02ed9df 100644
--- a/archaeological_context_records/models.py
+++ b/archaeological_context_records/models.py
@@ -32,8 +32,8 @@ from ishtar_common.utils import cached_label_changed
 
 from ishtar_common.models import GeneralType, BaseHistorizedItem, \
     HistoricalRecords, OwnPerms, ShortMenuItem, Source, GeneralRelationType,\
-    GeneralRecordRelations, post_delete_record_relation, get_external_id, \
-    ImageModel, post_save_cache, ValueGetter, BulkUpdatedItem, ExternalIdManager
+    GeneralRecordRelations, post_delete_record_relation, \
+    ImageModel, post_save_cache, ValueGetter, BulkUpdatedItem
 from archaeological_operations.models import Operation, Period, Parcel
 
 
@@ -189,7 +189,7 @@ class CRBulkView(object):
     """
 
 
-class ContextRecord(ExternalIdManager, BulkUpdatedItem, BaseHistorizedItem,
+class ContextRecord(BulkUpdatedItem, BaseHistorizedItem,
                     ImageModel, OwnPerms, ValueGetter, ShortMenuItem):
     SHOW_URL = 'show-contextrecord'
     SLUG = 'contextrecord'
diff --git a/archaeological_finds/models_finds.py b/archaeological_finds/models_finds.py
index be563f34e..a335eb6ec 100644
--- a/archaeological_finds/models_finds.py
+++ b/archaeological_finds/models_finds.py
@@ -34,7 +34,7 @@ from ishtar_common.utils import cached_label_changed, post_save_point
 from ishtar_common.models import GeneralType, ImageModel, BaseHistorizedItem, \
     ShortMenuItem, LightHistorizedItem, HistoricalRecords, OwnPerms, Source, \
     Person, Basket, post_save_cache, ValueGetter, \
-    get_current_profile, ExternalIdManager
+    get_current_profile
 
 from archaeological_operations.models import AdministrativeAct
 from archaeological_context_records.models import ContextRecord, Dating
@@ -150,8 +150,7 @@ class BFBulkView(object):
     """
 
 
-class BaseFind(ExternalIdManager, BulkUpdatedItem, BaseHistorizedItem,
-               OwnPerms):
+class BaseFind(BulkUpdatedItem, BaseHistorizedItem, OwnPerms):
     EXTERNAL_ID_KEY = 'base_find_external_id'
     EXTERNAL_ID_DEPENDENCIES = ['find']
     label = models.TextField(_(u"Free ID"))
@@ -494,8 +493,8 @@ class FBulkView(object):
     """
 
 
-class Find(ExternalIdManager, BulkUpdatedItem, ValueGetter,
-           BaseHistorizedItem, ImageModel, OwnPerms, ShortMenuItem):
+class Find(BulkUpdatedItem, ValueGetter, BaseHistorizedItem, ImageModel,
+           OwnPerms, ShortMenuItem):
     EXTERNAL_ID_KEY = 'find_external_id'
     CHECK_DICT = dict(CHECK_CHOICES)
     SHOW_URL = 'show-find'
@@ -746,10 +745,9 @@ class Find(ExternalIdManager, BulkUpdatedItem, ValueGetter,
         return lbl
 
     def get_first_base_find(self):
-        q = self.base_finds
-        if not q.count():
+        if not self.base_finds.count():
             return
-        return q.order_by('-pk').all()[0]
+        return self.base_finds.order_by('-pk').all()[0]
 
     @property
     def reference(self):
diff --git a/archaeological_finds/tests.py b/archaeological_finds/tests.py
index 05eef2c96..7ae81a1bb 100644
--- a/archaeological_finds/tests.py
+++ b/archaeological_finds/tests.py
@@ -366,6 +366,7 @@ class FindTest(FindInit, TestCase):
                 "New label"))
         cr = ContextRecord.objects.get(pk=base_find.context_record.pk)
         cr.label = "new-label-too"
+        cr.skip_history_when_saving = True
         cr.save()
         base_find = models.BaseFind.objects.get(pk=base_find.pk)
         find = models.Find.objects.get(pk=find.pk)
@@ -374,6 +375,7 @@ class FindTest(FindInit, TestCase):
         self.assertIn("new-label-too", base_find.external_id)
 
         cr.operation.code_patriarche = "PAT"
+        cr.operation.skip_history_when_saving = True
         cr.operation.save()
         base_find = models.BaseFind.objects.get(pk=base_find.pk)
         find = models.Find.objects.get(pk=find.pk)
diff --git a/archaeological_operations/forms.py b/archaeological_operations/forms.py
index 24b0a70be..651cd740f 100644
--- a/archaeological_operations/forms.py
+++ b/archaeological_operations/forms.py
@@ -260,8 +260,8 @@ class ParcelFormSet(FormSet):
                 if not value:
                     continue
                 try:
-                    parcel = models.Parcel.objects.get(pk=value)
-                except models.Parcel.DoesNotExist:
+                    parcel = models.Parcel.objects.get(pk=int(value))
+                except (models.Parcel.DoesNotExist, ValueError):
                     continue
                 ordering_keys[number] = [
                     parcel.public_domain, parcel.town, parcel.year,
diff --git a/archaeological_operations/models.py b/archaeological_operations/models.py
index 44f7a529e..54ed96cec 100644
--- a/archaeological_operations/models.py
+++ b/archaeological_operations/models.py
@@ -24,6 +24,7 @@ from django.conf import settings
 from django.contrib.gis.db import models
 from django.core.cache import cache
 from django.core.urlresolvers import reverse
+from django.db import IntegrityError, transaction
 from django.db.models import Q, Count, Sum, Max, Avg
 from django.db.models.signals import post_save, m2m_changed, post_delete
 from django.forms import ValidationError
@@ -37,7 +38,7 @@ from ishtar_common.models import GeneralType, BaseHistorizedItem, \
     SourceType, Person, Organization, Town, Dashboard, IshtarUser, ValueGetter,\
     DocumentTemplate, ShortMenuItem, DashboardFormItem, GeneralRelationType,\
     GeneralRecordRelations, post_delete_record_relation, OperationType, \
-    get_external_id, ImageModel, post_save_cache, ExternalIdManager
+    ImageModel, post_save_cache
 
 
 class RemainType(GeneralType):
@@ -818,10 +819,10 @@ def operation_post_save(sender, **kwargs):
 
     # external id update
     for parcel in operation.parcels.all():
-        parcel.update_external_id()
+        parcel.update_external_id(save=True)
 
     for cr in operation.context_record.all():
-        cr.update_external_id()
+        cr.update_external_id(save=True)
 
 
 post_save.connect(operation_post_save, sender=Operation)
@@ -1255,7 +1256,7 @@ def strip_zero(value):
     return value
 
 
-class Parcel(ExternalIdManager, LightHistorizedItem):
+class Parcel(LightHistorizedItem):
     EXTERNAL_ID_KEY = 'parcel_external_id'
 
     associated_file = models.ForeignKey(
@@ -1452,12 +1453,22 @@ def parcel_post_save(sender, **kwargs):
                 parcel_id=parcel.id)
 
     if parcel.operation and parcel.operation.pk and \
-       parcel.town not in list(parcel.operation.towns.all()):
-        parcel.operation.towns.add(parcel.town)
+            parcel.town not in list(parcel.operation.towns.all()):
+        try:
+            # multiple save can cause multiple add
+            with transaction.atomic():
+                parcel.operation.towns.add(parcel.town)
+        except IntegrityError:
+            pass
     if parcel.associated_file and \
-       parcel.associated_file.pk and \
-       parcel.town not in list(parcel.associated_file.towns.all()):
-        parcel.associated_file.towns.add(parcel.town)
+            parcel.associated_file.pk and \
+            parcel.town not in list(parcel.associated_file.towns.all()):
+        try:
+            # multiple save can cause multiple add
+            with transaction.atomic():
+                parcel.associated_file.towns.add(parcel.town)
+        except IntegrityError:
+            pass
     if parcel.operation and parcel.associated_file:
         # parcels are copied between files and operations
         parcel.copy_to_operation()
diff --git a/ishtar_common/models.py b/ishtar_common/models.py
index 55795c91e..b0b050c6c 100644
--- a/ishtar_common/models.py
+++ b/ishtar_common/models.py
@@ -175,35 +175,6 @@ class ValueGetter(object):
         return values
 
 
-class ExternalIdManager(object):
-    EXTERNAL_ID_KEY = ''
-    EXTERNAL_ID_DEPENDENCIES = []
-
-    def update_external_id(self, save=True):
-        if not self.EXTERNAL_ID_KEY:
-            raise NotImplementedError(
-                "{} should have an EXTERNAL_ID_KEY".format(self.__class__))
-        if self.external_id and not self.auto_external_id:
-            return
-        external_id = get_external_id(self.EXTERNAL_ID_KEY, self)
-        if external_id == self.external_id:
-            return
-        self.auto_external_id = True
-        self.external_id = external_id
-        if save:
-            self.skip_history_when_saving = True
-            self.save()
-        for dep in self.EXTERNAL_ID_DEPENDENCIES:
-            for obj in getattr(self, dep).all():
-                obj.update_external_id()
-        return external_id
-
-    def save(self, *args, **kwargs):
-        returned = super(ExternalIdManager, self).save(*args, **kwargs)
-        self.update_external_id()
-        return returned
-
-
 class HistoricalRecords(BaseHistoricalRecords):
     def create_historical_record(self, instance, type):
         try:
@@ -931,7 +902,12 @@ class Imported(models.Model):
 
 
 class BaseHistorizedItem(Imported):
+    """
+    Historized item with external ID management
+    """
     IS_BASKET = False
+    EXTERNAL_ID_KEY = ''
+    EXTERNAL_ID_DEPENDENCIES = []
     history_modifier = models.ForeignKey(
         User, related_name='+', on_delete=models.SET_NULL,
         verbose_name=_(u"Last editor"), blank=True, null=True)
@@ -942,12 +918,20 @@ class BaseHistorizedItem(Imported):
     class Meta:
         abstract = True
 
-    def save(self, *args, **kwargs):
-        assert hasattr(self, 'history_modifier')
-        if not self.id:
-            self.history_creator = self.history_modifier
-        super(BaseHistorizedItem, self).save(*args, **kwargs)
-        return True
+    def update_external_id(self, save=False):
+        if not self.EXTERNAL_ID_KEY or (
+                self.external_id and not self.auto_external_id):
+            return
+        external_id = get_external_id(self.EXTERNAL_ID_KEY, self)
+        if external_id == self.external_id:
+            return
+        self.auto_external_id = True
+        self.external_id = external_id
+        self._cached_label_checked = False
+        if save:
+            self.skip_history_when_saving = True
+            self.save()
+        return external_id
 
     def get_previous(self, step=None, date=None, strict=True):
         """
@@ -1077,6 +1061,28 @@ class BaseHistorizedItem(Imported):
             items.append('00000000')
         return u"-".join([unicode(item) for item in items])
 
+    def save(self, *args, **kwargs):
+        created = not self.pk
+        if not getattr(self, 'skip_history_when_saving', False):
+            assert hasattr(self, 'history_modifier')
+            if created:
+                self.history_creator = self.history_modifier
+        # external ID can have related item not available before save
+        external_id_updated = kwargs.pop('external_id_updated') \
+            if 'external_id_updated' in kwargs else False
+        if not created and not external_id_updated:
+            self.update_external_id()
+        super(BaseHistorizedItem, self).save(*args, **kwargs)
+        if created and self.update_external_id():
+            # force resave for external ID creation
+            self.skip_history_when_saving = True
+            self._updated_id = True
+            return self.save(external_id_updated=True)
+        for dep in self.EXTERNAL_ID_DEPENDENCIES:
+            for obj in getattr(self, dep).all():
+                obj.update_external_id(save=True)
+        return True
+
 
 class GeneralRelationType(GeneralType):
     order = models.IntegerField(_(u"Order"), default=1)
diff --git a/ishtar_common/tests.py b/ishtar_common/tests.py
index 4afc74c9b..ad83b4a87 100644
--- a/ishtar_common/tests.py
+++ b/ishtar_common/tests.py
@@ -95,7 +95,8 @@ class CommandsTestCase(TestCase):
         """
         from archaeological_operations.models import Parcel
         p = Parcel.objects.create(
-            town=models.Town.objects.create(name='test', numero_insee='25000'))
+            town=models.Town.objects.create(name='test', numero_insee='25000'),
+        )
         parcel_nb = Parcel.objects.count()
         out = StringIO()
         call_command('clean_ishtar', stdout=out)