Fix external id generation for finds

7 files changed, 101 insertions, 56 deletions

diff --git a/archaeological_context_records/models.py b/archaeological_context_records/models.py
index 70858ea31..8dd082e7b 100644
--- a/archaeological_context_records/models.py
+++ b/archaeological_context_records/models.py
@@ -33,7 +33,7 @@ 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
+    ImageModel, post_save_cache, ValueGetter, BulkUpdatedItem, ExternalIdManager
 from archaeological_operations.models import Operation, Period, Parcel
 
 
@@ -189,10 +189,12 @@ class CRBulkView(object):
     """
 
 
-class ContextRecord(BulkUpdatedItem, BaseHistorizedItem, ImageModel, OwnPerms,
-                    ValueGetter, ShortMenuItem):
+class ContextRecord(ExternalIdManager, BulkUpdatedItem, BaseHistorizedItem,
+                    ImageModel, OwnPerms, ValueGetter, ShortMenuItem):
     SHOW_URL = 'show-contextrecord'
     SLUG = 'contextrecord'
+    EXTERNAL_ID_KEY = 'context_record_external_id'
+    EXTERNAL_ID_DEPENDENCIES = ['base_finds']
     TABLE_COLS = ['label', 'operation__common_name', 'parcel__town__name',
                   'parcel__label', 'unit']
     if settings.COUNTRY == 'fr':
@@ -518,20 +520,6 @@ class ContextRecord(BulkUpdatedItem, BaseHistorizedItem, ImageModel, OwnPerms,
         from archaeological_finds.models import FindSource
         return FindSource.objects.filter(find__base_finds__context_record=self)
 
-    def save(self, *args, **kwargs):
-        returned = super(ContextRecord, self).save(*args, **kwargs)
-        updated = False
-        if not self.external_id or self.auto_external_id:
-            external_id = get_external_id('context_record_external_id', self)
-            if external_id != self.external_id:
-                updated = True
-                self.auto_external_id = True
-                self.external_id = external_id
-        if updated:
-            self._cached_label_checked = False
-            self.save()
-        return returned
-
     def fix(self):
         """
         Fix redundant m2m dating association (usually after imports)
diff --git a/archaeological_context_records/tests.py b/archaeological_context_records/tests.py
index e29c37511..89b15fbbf 100644
--- a/archaeological_context_records/tests.py
+++ b/archaeological_context_records/tests.py
@@ -382,7 +382,6 @@ class ContextRecordTest(ContextRecordInit, TestCase):
         self.assertEqual(obj.datings.count(), 2)
 
 
-
 class ContextRecordSearchTest(ContextRecordInit, TestCase):
     fixtures = CONTEXT_RECORD_TOWNS_FIXTURES
 
diff --git a/archaeological_finds/models_finds.py b/archaeological_finds/models_finds.py
index 8a7dbaf08..be563f34e 100644
--- a/archaeological_finds/models_finds.py
+++ b/archaeological_finds/models_finds.py
@@ -33,8 +33,8 @@ 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, get_external_id, post_save_cache, ValueGetter, \
-    get_current_profile
+    Person, Basket, post_save_cache, ValueGetter, \
+    get_current_profile, ExternalIdManager
 
 from archaeological_operations.models import AdministrativeAct
 from archaeological_context_records.models import ContextRecord, Dating
@@ -150,7 +150,10 @@ class BFBulkView(object):
     """
 
 
-class BaseFind(BulkUpdatedItem, BaseHistorizedItem, OwnPerms):
+class BaseFind(ExternalIdManager, BulkUpdatedItem, BaseHistorizedItem,
+               OwnPerms):
+    EXTERNAL_ID_KEY = 'base_find_external_id'
+    EXTERNAL_ID_DEPENDENCIES = ['find']
     label = models.TextField(_(u"Free ID"))
     external_id = models.TextField(_(u"External ID"), blank=True, null=True)
     auto_external_id = models.BooleanField(
@@ -327,21 +330,6 @@ class BaseFind(BulkUpdatedItem, BaseHistorizedItem, OwnPerms):
     def name(self):
         return self.label
 
-    def save(self, *args, **kwargs):
-        returned = super(BaseFind, self).save(*args, **kwargs)
-
-        updated = False
-        if not self.external_id or self.auto_external_id:
-            external_id = get_external_id('base_find_external_id', self)
-            if external_id != self.external_id:
-                updated = True
-                self.auto_external_id = True
-                self.external_id = external_id
-        if updated:
-            self._cached_label_checked = False
-            self.save()
-        return returned
-
     @classmethod
     def cached_label_bulk_update(cls, operation_id=None, parcel_id=None,
                                  context_record_id=None, transaction_id=None):
@@ -506,8 +494,9 @@ class FBulkView(object):
     """
 
 
-class Find(BulkUpdatedItem, ValueGetter, BaseHistorizedItem, ImageModel,
-           OwnPerms, ShortMenuItem):
+class Find(ExternalIdManager, BulkUpdatedItem, ValueGetter,
+           BaseHistorizedItem, ImageModel, OwnPerms, ShortMenuItem):
+    EXTERNAL_ID_KEY = 'find_external_id'
     CHECK_DICT = dict(CHECK_CHOICES)
     SHOW_URL = 'show-find'
     SLUG = 'find'
@@ -1161,14 +1150,8 @@ class Find(BulkUpdatedItem, ValueGetter, BaseHistorizedItem, ImageModel,
     def save(self, *args, **kwargs):
         super(Find, self).save(*args, **kwargs)
 
-        updated = False
         self.skip_history_when_saving = True
-        if not self.external_id or self.auto_external_id:
-            external_id = get_external_id('find_external_id', self)
-            if external_id != self.external_id:
-                updated = True
-                self.auto_external_id = True
-                self.external_id = external_id
+        updated = self.update_external_id(save=False)
         if updated:
             self._cached_label_checked = False
             self.save()
diff --git a/archaeological_finds/tests.py b/archaeological_finds/tests.py
index 3e89273a0..05eef2c96 100644
--- a/archaeological_finds/tests.py
+++ b/archaeological_finds/tests.py
@@ -29,7 +29,7 @@ from ishtar_common.models import ImporterType, IshtarUser, ImporterColumn,\
     FormaterType, ImportTarget, IshtarSiteProfile
 
 from ishtar_common.models import Person, get_current_profile
-from archaeological_context_records.models import Period, Dating
+from archaeological_context_records.models import Period, Dating, ContextRecord
 from archaeological_finds import models, views
 from archaeological_warehouse.models import Warehouse, WarehouseType
 
@@ -356,6 +356,30 @@ class FindTest(FindInit, TestCase):
             u"{}-{}".format(
                 base_find.context_record.external_id,
                 base_find.label))
+        base_find.label = "New label"
+        base_find.save()
+        base_find = models.BaseFind.objects.get(pk=base_find.pk)
+        self.assertEqual(
+            base_find.external_id,
+            u"{}-{}".format(
+                base_find.context_record.external_id,
+                "New label"))
+        cr = ContextRecord.objects.get(pk=base_find.context_record.pk)
+        cr.label = "new-label-too"
+        cr.save()
+        base_find = models.BaseFind.objects.get(pk=base_find.pk)
+        find = models.Find.objects.get(pk=find.pk)
+        cr = ContextRecord.objects.get(pk=cr.pk)
+        self.assertIn("new-label-too", find.external_id)
+        self.assertIn("new-label-too", base_find.external_id)
+
+        cr.operation.code_patriarche = "PAT"
+        cr.operation.save()
+        base_find = models.BaseFind.objects.get(pk=base_find.pk)
+        find = models.Find.objects.get(pk=find.pk)
+        cr = ContextRecord.objects.get(pk=cr.pk)
+        self.assertIn("PAT", find.external_id)
+        self.assertIn("PAT", base_find.external_id)
 
     def testIndex(self):
         profile = get_current_profile()
diff --git a/archaeological_operations/models.py b/archaeological_operations/models.py
index 1f4108c28..44f7a529e 100644
--- a/archaeological_operations/models.py
+++ b/archaeological_operations/models.py
@@ -37,7 +37,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
+    get_external_id, ImageModel, post_save_cache, ExternalIdManager
 
 
 class RemainType(GeneralType):
@@ -815,6 +815,15 @@ def operation_post_save(sender, **kwargs):
         # manage parcel association
         for parcel in operation.parcels.all():
             parcel.copy_to_file()
+
+    # external id update
+    for parcel in operation.parcels.all():
+        parcel.update_external_id()
+
+    for cr in operation.context_record.all():
+        cr.update_external_id()
+
+
 post_save.connect(operation_post_save, sender=Operation)
 
 
@@ -839,6 +848,7 @@ class RecordRelations(GeneralRecordRelations, models.Model):
         verbose_name_plural = _(u"Operation record relations")
         ordering = ('left_record', 'relation_type')
 
+
 post_delete.connect(post_delete_record_relation, sender=RecordRelations)
 
 
@@ -1245,7 +1255,9 @@ def strip_zero(value):
     return value
 
 
-class Parcel(LightHistorizedItem):
+class Parcel(ExternalIdManager, LightHistorizedItem):
+    EXTERNAL_ID_KEY = 'parcel_external_id'
+
     associated_file = models.ForeignKey(
         'archaeological_files.File',
         related_name='parcels', verbose_name=_(u"File"),
@@ -1426,13 +1438,6 @@ def parcel_post_save(sender, **kwargs):
             parcel.delete()
             return
 
-    if not parcel.external_id or parcel.auto_external_id:
-        external_id = get_external_id('parcel_external_id', parcel)
-        if external_id != parcel.external_id:
-            updated = True
-            parcel._updated_id = True
-            parcel.auto_external_id = True
-            parcel.external_id = external_id
     if updated:
         parcel.save()
         return
diff --git a/archaeological_operations/tests.py b/archaeological_operations/tests.py
index 60078b7e2..0d6908374 100644
--- a/archaeological_operations/tests.py
+++ b/archaeological_operations/tests.py
@@ -1245,6 +1245,7 @@ class OperationWizardCreationTest(WizardTest, OperationInitTest, TestCase):
             form_datas={
                 'filechoice': {},
                 'general': {
+                    'code_patriarche': 'codeope1',
                     'operation_type': None,
                     'year': 2016},
                 'townsgeneral': [],
@@ -1259,6 +1260,7 @@ class OperationWizardCreationTest(WizardTest, OperationInitTest, TestCase):
             form_datas={
                 'filechoice': {},
                 'general': {
+                    'code_patriarche': 'codeope2',
                     'operation_type': None,
                     'year': 2016},
                 'townsgeneral': [],
@@ -1273,6 +1275,7 @@ class OperationWizardCreationTest(WizardTest, OperationInitTest, TestCase):
             form_datas={
                 'filechoice': {},
                 'general': {
+                    'code_patriarche': 'codeope3',
                     'operation_type': None,
                     'year': 2016},
                 'towns': [],
@@ -1332,8 +1335,17 @@ class OperationWizardCreationTest(WizardTest, OperationInitTest, TestCase):
     def post_wizard(self):
         self.assertEqual(models.Operation.objects.count(),
                          self.operation_number + 3)
+        operations = models.Operation.objects.order_by("-pk").all()[:3]
+
+        parcel_ids = []
+        for operation in operations:
+            for parcel in operation.parcels.all():
+                parcel_ids.append(parcel.external_id)
+        self.assertEqual(list(sorted(parcel_ids)),
+                         ['codeope1-12345-S42', 'codeope2-12345-S42',
+                          'codeope3-12345-G43'])
         self.assertEqual(models.Parcel.objects.count(),
-                         self.parcel_number + 2)
+                         self.parcel_number + 3)
 
 
 class OperationWizardModifTest(WizardTest, OperationInitTest, TestCase):
@@ -1369,6 +1381,7 @@ class OperationWizardModifTest(WizardTest, OperationInitTest, TestCase):
             form_datas={
                 'selec': {},
                 'general': {
+                    'code_patriarche': "codeope42",
                     'operation_type': 2,
                     'year': 2017},
                 'townsgeneral': [],
@@ -1470,6 +1483,10 @@ class OperationWizardModifTest(WizardTest, OperationInitTest, TestCase):
             # the init parcel is not detached from the operation
             test_object.assertEqual(operation.parcels.count(),
                                     test_object.parcel_number + 1)
+            # update teh external id on update
+            cr = ContextRecord.objects.get(pk=self.cr.pk)
+            test_object.assertEqual(cr.external_id,
+                                    "codeope42-12345-A1-Context record")
 
         def pre_third_wizard(test_object):
             parcel_nb = models.Parcel.objects.count()
diff --git a/ishtar_common/models.py b/ishtar_common/models.py
index 53d5c85ac..55795c91e 100644
--- a/ishtar_common/models.py
+++ b/ishtar_common/models.py
@@ -175,6 +175,35 @@ 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: