OpenConceptLab/ocl_issues#1746 | concept/mapping new version creation…

… to compare checksum and fail if same

core/common/constants.py

@@ -124,3 +124,4 @@
 FACET_SIZE = 20
 ALL = '*'
 CANONICAL_URL_REQUEST_PARAM = 'canonicalUrl'
+SAME_STANDARD_CHECKSUM_ERROR = 'No changes detected. Standard checksum is same as last version.'

core/common/mixins.py

@@ -19,13 +19,14 @@
 from core.common.constants import HEAD, ACCESS_TYPE_NONE, INCLUDE_FACETS, \
     LIST_DEFAULT_LIMIT, HTTP_COMPRESS_HEADER, CSV_DEFAULT_LIMIT, FACETS_ONLY, INCLUDE_RETIRED_PARAM, \
     SEARCH_STATS_ONLY, INCLUDE_SEARCH_STATS, UPDATED_BY_USERNAME_PARAM, CHECKSUM_STANDARD_HEADER, \
-    CHECKSUM_SMART_HEADER, SEARCH_LATEST_REPO_VERSION
+    CHECKSUM_SMART_HEADER, SEARCH_LATEST_REPO_VERSION, SAME_STANDARD_CHECKSUM_ERROR
 from core.common.permissions import HasPrivateAccess, HasOwnership, CanViewConceptDictionary, \
     CanViewConceptDictionaryVersion
 from .checksums import ChecksumModel, Checksum
 from .utils import write_csv_to_s3, get_csv_from_s3, get_query_params_from_url_string, compact_dict_by_values, \
     to_owner_uri, parse_updated_since_param, get_export_service, to_int, get_truthy_values, generate_temp_version
 from ..concepts.constants import PERSIST_CLONE_ERROR
+from ..toggles.models import Toggle
 
 logger = logging.getLogger('oclapi')
 TRUTHY = get_truthy_values()
@@ -748,7 +749,7 @@ def mark_latest_version(self, index=True, parent=None):
     def validate_locales_limit(names, descriptions):
         pass
 
-    def _process_prev_latest_version_children(self, prev_latest, add_prev_version_children):
+    def _process_prev_latest_version_hierarchy(self, prev_latest, add_prev_version_children):
         pass
 
     def _process_latest_version_hierarchy(self, prev_latest, parent_concept_uris=None, create_parent_version=True):
@@ -763,17 +764,19 @@ def _index_on_new_version_creation(self, prev_latest):
     def remove_locales(self):
         pass
 
-    def save_as_new_version(self, user, **kwargs):
+    def save_as_new_version(self, user, **kwargs):  # pylint: disable=too-many-branches,too-many-statements
         cls = self.__class__
         create_parent_version = kwargs.pop('create_parent_version', True)
         parent_concept_uris = kwargs.pop('parent_concept_uris', None)
         add_prev_version_children = kwargs.pop('add_prev_version_children', True)
+        _hierarchy_processing = kwargs.pop('_hierarchy_processing', False)
         errors = {}
         self.created_by = self.updated_by = user
         self.version = self.version or generate_temp_version()
         parent = self.parent
         persisted = False
         prev_latest = self.versions.exclude(id=self.id).filter(is_latest_version=True).first()
+        is_concept = self.__class__.__name__ == 'Concept'
         try:
             with transaction.atomic():
                 self.validate_locales_limit(get(self, 'cloned_names'), get(self, 'cloned_descriptions'))
@@ -782,15 +785,35 @@ def save_as_new_version(self, user, **kwargs):
                 self.save(**kwargs)
 
                 if self.id:
-                    self.post_version_create(parent)
+                    self.post_version_create(parent, parent_concept_uris)
+                    if not prev_latest or _hierarchy_processing:
+                        self.set_checksums()
+                    should_process_hierarchy = bool(parent_concept_uris)
                     if prev_latest:
+                        if not _hierarchy_processing:
+                            if is_concept:
+                                self._unsaved_child_concept_uris = prev_latest.child_concept_urls
+                            self.set_checksums()
+                        if Toggle.get(
+                                'PREVENT_DUPLICATE_VERSION_TOGGLE'
+                        ) and Toggle.get(
+                            'CHECKSUMS_TOGGLE'
+                        ) and not _hierarchy_processing and self.checksums.get(
+                            'standard'
+                        ) == prev_latest.get_checksums(recalculate=True).get('standard'):
+                            raise ValidationError({'__all__': [SAME_STANDARD_CHECKSUM_ERROR]})
                         if not self._index:
                             self.prev_latest_version_id = prev_latest.id
                         prev_latest.unmark_latest_version(self._index, parent)
-                        self._process_prev_latest_version_children(prev_latest, add_prev_version_children)
+                        should_process_hierarchy = should_process_hierarchy or bool(
+                            is_concept and prev_latest.parent_concept_urls)
+                        if should_process_hierarchy:
+                            self._process_prev_latest_version_hierarchy(prev_latest, add_prev_version_children)
+                    if should_process_hierarchy:
+                        self._process_latest_version_hierarchy(prev_latest, parent_concept_uris, create_parent_version)
                     persisted = True
                     cls.resume_indexing()
-                    self._process_latest_version_hierarchy(prev_latest, parent_concept_uris, create_parent_version)
+
                 transaction.on_commit(lambda: self._index_on_new_version_creation(prev_latest))
         except ValidationError as err:
             errors.update(err.message_dict)

core/common/tests.py

@@ -1205,7 +1205,15 @@ def test_post_200_concept(self, checksum_generate_mock):
 
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data, 'checksum')
-        checksum_generate_mock.assert_called_once_with({'concept_class': 'foobar', 'names': [], 'descriptions': []})
+        checksum_generate_mock.assert_called_once_with(
+            {
+                'concept_class': 'foobar',
+                'names': [],
+                'descriptions': [],
+                'child_concept_urls': [],
+                'parent_concept_urls': []
+            }
+        )
 
     @patch('core.common.checksums.Checksum.generate')
     def test_post_200_mapping_standard(self, checksum_generate_mock):

core/concepts/models.py

@@ -289,8 +289,8 @@ def get_standard_checksum_fields_for_resource(data):
             'concept_class': get(data, 'concept_class'),
             'datatype': get(data, 'datatype'),
             'retired': get(data, 'retired'),
-            'external_id': get(data, 'external_id'),
-            'extras': get(data, 'extras'),
+            'external_id': get(data, 'external_id') or None,
+            'extras': get(data, 'extras') or None,
             'names': Concept._locales_for_checksums(
                 data,
                 'names',
@@ -303,6 +303,12 @@ def get_standard_checksum_fields_for_resource(data):
                 ConceptDescription.CHECKSUM_INCLUSIONS,
                 lambda _: True
             ),
+            'parent_concept_urls': get(
+                data, '_unsaved_parent_concept_uris', []
+            ) or get(data, 'parent_concept_urls', []),
+            'child_concept_urls': get(
+                data, '_unsaved_child_concept_uris', []
+            ) or get(data, 'child_concept_urls', []),
         }
 
     @staticmethod
@@ -569,6 +575,7 @@ def create_initial_version(cls, concept, **kwargs):
     @classmethod
     def create_new_version_for(
             cls, instance, data, user, create_parent_version=True, add_prev_version_children=True,
+            _hierarchy_processing=False
     ):  # pylint: disable=too-many-arguments
         instance.id = None  # Clear id so it is persisted as a new object
         instance.version = data.get('version', None)
@@ -594,11 +601,12 @@ def create_new_version_for(
             user=user,
             create_parent_version=create_parent_version,
             parent_concept_uris=parent_concept_uris,
-            add_prev_version_children=add_prev_version_children
+            add_prev_version_children=add_prev_version_children,
+            _hierarchy_processing=_hierarchy_processing
         )
 
     def set_parent_concepts_from_uris(self, create_parent_version=True):
-        parent_concepts = get(self, '_parent_concepts', None)
+        parent_concepts = get(self, '_parent_concepts', [])
         if create_parent_version:
             for parent in parent_concepts:
                 current_latest_version = parent.get_latest_version()
@@ -611,7 +619,8 @@ def set_parent_concepts_from_uris(self, create_parent_version=True):
                         'parent_concept_urls': parent.parent_concept_urls
                     },
                     self.created_by,
-                    create_parent_version=False
+                    create_parent_version=False,
+                    _hierarchy_processing=True
                 )
                 new_latest_version = parent.get_latest_version()
                 for uri in current_latest_version.child_concept_urls:
@@ -638,7 +647,8 @@ def create_new_versions_for_removed_parents(self, uris):
                     },
                     concept.created_by,
                     create_parent_version=False,
-                    add_prev_version_children=False
+                    add_prev_version_children=False,
+                    _hierarchy_processing=True
                 )
                 new_latest_version = concept.get_latest_version()
                 for uri in current_latest_version.child_concept_urls:
@@ -817,7 +827,7 @@ def update_versioned_object(self):
         concept.save()
         concept.set_checksums()
 
-    def post_version_create(self, parent):
+    def post_version_create(self, parent, parent_concept_uris):
         if self.id:
             self.version = str(self.id)
             self.save()
@@ -828,7 +838,7 @@ def post_version_create(self, parent):
             self.clean()  # clean here to validate locales that can only be saved after obj is saved
             self.update_versioned_object()
             self.sources.set([parent])
-            self.set_checksums()
+            self._unsaved_parent_concept_uris = parent_concept_uris
 
     def _process_prev_latest_version_hierarchy(self, prev_latest, add_prev_version_children=True):
         if add_prev_version_children:

core/concepts/tests/factories.py

@@ -79,6 +79,7 @@ def descriptions(self, create, extracted, **kwargs):
                 for locale in extracted:
                     locale.concept = self
                     locale.save()
+            sync_latest_version(self)
 
 
 class ConceptNameFactory(factory.django.DjangoModelFactory):

core/concepts/tests/tests.py

@@ -821,6 +821,7 @@ def test_save_as_new_version(self):
         )
         source_version0.concepts.add(concept)
         cloned_concept = Concept.version_for_concept(concept, 'v1', source_head)
+        cloned_concept.datatype = 'foobar'
 
         self.assertEqual(cloned_concept.save_as_new_version(concept.created_by), {})
 
@@ -846,7 +847,9 @@ def test_retire(self):
             **factory.build(dict, FACTORY_CLASS=ConceptFactory), 'mnemonic': 'c1', 'parent': source,
             'names': [ConceptNameFactory.build(locale='en', name='English', locale_preferred=True)]
         })
-        concept.clone().save_as_new_version(concept.created_by)
+        concept_v1 = concept.clone()
+        concept_v1.datatype = 'foobar'
+        concept_v1.save_as_new_version(concept.created_by)
         concept_v1 = Concept.objects.order_by('-created_at').first()
         concept.refresh_from_db()
 
@@ -878,7 +881,9 @@ def test_unretire(self):
             **factory.build(dict, FACTORY_CLASS=ConceptFactory), 'mnemonic': 'c1', 'parent': source, 'retired': True,
             'names': [ConceptNameFactory.build(locale='en', name='English', locale_preferred=True)]
         })
-        concept.clone().save_as_new_version(concept.created_by)
+        concept_v1 = concept.clone()
+        concept_v1.datatype = 'foobar'
+        concept_v1.save_as_new_version(concept.created_by)
         concept_v1 = Concept.objects.order_by('-created_at').first()
         concept.refresh_from_db()
 
@@ -1186,6 +1191,7 @@ def test_from_uri_queryset_for_source_and_source_version(self):
             version='v2', mnemonic=source.mnemonic, organization=source.organization)
 
         cloned_concept = Concept.version_for_concept(concept, 'v1', source)
+        cloned_concept.datatype = 'foobar'
         cloned_concept.save_as_new_version(concept.created_by)
 
         self.assertEqual(concept.versions.count(), 2)
@@ -1226,6 +1232,7 @@ def test_from_uri_queryset_for_collection_and_collection_version(self):
         OrganizationSourceFactory(
             version='v2', mnemonic=source.mnemonic, organization=source.organization)
         cloned_concept = Concept.version_for_concept(concept, 'v1', source)
+        cloned_concept.datatype = 'foobar'
         cloned_concept.save_as_new_version(concept.created_by)
         self.assertEqual(concept.versions.count(), 2)
 

core/concepts/views.py

@@ -596,24 +596,25 @@ def get_queryset(self):
         return getattr(instance, self.parent_list_attribute).all()
 
     def create(self, request, **_):  # pylint: disable=arguments-differ
-        name = request.data.get('name', None) or request.data.get('description', None)
+        name = request.data.get('name', None)
+        description = request.data.get('description', None)
+        locale = name or description
         serializer = self.get_serializer(data=request.data.copy())
-        if name and serializer.is_valid():
-            new_version = self.get_object().clone()
-            new_version.comment = f'Added to {self.parent_list_attribute}: {name}.'
-            errors = new_version.save_as_new_version(request.user)
-            if new_version.id:
-                serializer = self.get_serializer(data={**request.data, 'concept_id': new_version.id})
-                if serializer.is_valid():
-                    serializer.save()
-                    locale = serializer.instance
-                    if locale.id:
-                        versioned_object_locale = locale.clone()
-                        versioned_object_locale.concept_id = new_version.versioned_object_id
-                        versioned_object_locale.save()
-
-            if errors:
-                return Response(errors, status=status.HTTP_400_BAD_REQUEST)
+        if locale and serializer.is_valid():
+            serializer = self.get_serializer(data=request.data)
+            if serializer.is_valid():
+                new_version = self.get_object().clone()
+                new_version.comment = f'Added to {self.parent_list_attribute}: {locale}.'
+                if name:
+                    new_version.cloned_names = [*new_version.cloned_names, request.data]
+                elif description:
+                    new_version.cloned_descriptions = [*new_version.cloned_descriptions, request.data]
+                errors = new_version.save_as_new_version(request.user)
+                if errors:
+                    return Response(errors, status=status.HTTP_400_BAD_REQUEST)
+                locales = new_version.names if name else new_version.descriptions
+                instance = locales.order_by('-id').first()
+                serializer = self.get_serializer(instance)
             headers = self.get_success_headers(serializer.data)
             return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers)
         return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

core/fixtures/toggles.json

@@ -30,5 +30,15 @@
       "dev": true,
       "qa": true
     }
+  },
+  {
+    "pk": 4,
+    "model": "toggles.toggle",
+    "fields": {
+      "is_active": true,
+      "name": "PREVENT_DUPLICATE_VERSION_TOGGLE",
+      "dev": true,
+      "qa": true
+    }
   }
 ]

core/importers/tests.py

@@ -541,8 +541,8 @@ def test_reference_import(self, batch_index_resources_mock):
         self.assertEqual(importer.processed, 9)
         self.assertEqual(len(importer.created), 2)
         self.assertEqual(len(importer.exists), 3)
-        self.assertEqual(len(importer.updated), 4)
-        self.assertEqual(len(importer.failed), 0)
+        self.assertEqual(len(importer.updated), 0)
+        self.assertEqual(len(importer.failed), 4)  # due to same concept checksum
         self.assertEqual(len(importer.invalid), 0)
         self.assertEqual(len(importer.others), 0)
         self.assertEqual(len(importer.permission_denied), 0)
@@ -568,9 +568,9 @@ def test_sample_import(self, batch_index_resources_mock):
         self.assertEqual(importer.processed, 64)
         self.assertEqual(len(importer.created), 49)
         self.assertEqual(len(importer.exists), 3)
-        self.assertEqual(len(importer.updated), 12)
+        self.assertEqual(len(importer.updated), 1)  # last 11 rows are duplicate rows
+        self.assertEqual(len(importer.failed), 11)
         self.assertEqual(len(importer.deleted), 0)
-        self.assertEqual(len(importer.failed), 0)
         self.assertEqual(len(importer.invalid), 0)
         self.assertEqual(len(importer.others), 0)
         self.assertEqual(len(importer.permission_denied), 0)

core/integration_tests/tests_concepts.py

@@ -233,6 +233,21 @@ def test_put_200(self):
         self.assertTrue(concept.is_versioned_object)
         self.assertEqual(concept.datatype, "None")
 
+        response = self.client.put(
+            concepts_url,
+            {**self.concept_payload, 'datatype': 'None', 'update_comment': 'Updated Nothing'},
+            HTTP_AUTHORIZATION='Token ' + self.token,
+            format='json'
+        )
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            response.data,
+            {
+                'non_field_errors': ['An error occurred while saving new version.'],
+                '__all__': ['No changes detected. Standard checksum is same as last version.']
+            }
+        )
+
     def test_put_200_openmrs_schema(self):  # pylint: disable=too-many-statements
         self.create_lookup_concept_classes()
         source = OrganizationSourceFactory(custom_validation_schema=OPENMRS_VALIDATION_SCHEMA)

core/mappings/models.py

@@ -155,8 +155,8 @@ def get_smart_checksum_fields(self):
     def get_standard_checksum_fields_for_resource(data):
         return {
             **Mapping.get_smart_checksum_fields_for_resource(data),
-            'extras': get(data, 'extras'),
-            'external_id': get(data, 'external_id'),
+            'extras': get(data, 'extras') or None,
+            'external_id': get(data, 'external_id') or None,
         }
 
     @staticmethod
@@ -545,13 +545,12 @@ def update_versioned_object(self):
         mapping.save()
         mapping.set_checksums()
 
-    def post_version_create(self, parent):
+    def post_version_create(self, parent, _):
         if self.id:
             self.version = str(self.id)
             self.save()
             self.update_versioned_object()
             self.sources.set([parent])
-            self.set_checksums()
 
     def _index_on_new_version_creation(self, prev_latest):
         if self._index:

core/mappings/tests/tests.py

@@ -288,7 +288,7 @@ def test_save_as_new_version(self):
 
         mapping = MappingFactory(parent=source_head)
         cloned_mapping = mapping.clone(mapping.created_by)
-
+        cloned_mapping.extras = {'foo': 'bar'}
         self.assertEqual(cloned_mapping.save_as_new_version(mapping.created_by), {})
 
         persisted_mapping = Mapping.objects.filter(