diff --git a/core/common/constants.py b/core/common/constants.py index e38972344..111e276a1 100644 --- a/core/common/constants.py +++ b/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.' diff --git a/core/common/mixins.py b/core/common/mixins.py index f2ce95bc1..1d3ed1c69 100644 --- a/core/common/mixins.py +++ b/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) diff --git a/core/common/tests.py b/core/common/tests.py index ba563f0fd..ad5f68808 100644 --- a/core/common/tests.py +++ b/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): diff --git a/core/concepts/models.py b/core/concepts/models.py index 2aad0c868..fa0142f27 100644 --- a/core/concepts/models.py +++ b/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: diff --git a/core/concepts/tests/factories.py b/core/concepts/tests/factories.py index 10d6e8d2c..7fbc0f37b 100644 --- a/core/concepts/tests/factories.py +++ b/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): diff --git a/core/concepts/tests/tests.py b/core/concepts/tests/tests.py index 0ab6c614f..03ebb3da9 100644 --- a/core/concepts/tests/tests.py +++ b/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) diff --git a/core/concepts/views.py b/core/concepts/views.py index 50b173450..ed245dc50 100644 --- a/core/concepts/views.py +++ b/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) diff --git a/core/fixtures/toggles.json b/core/fixtures/toggles.json index 3b93c0ed1..2db0cf4a4 100644 --- a/core/fixtures/toggles.json +++ b/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 + } } ] \ No newline at end of file diff --git a/core/importers/tests.py b/core/importers/tests.py index 4fb36d6c5..520324075 100644 --- a/core/importers/tests.py +++ b/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) diff --git a/core/integration_tests/tests_concepts.py b/core/integration_tests/tests_concepts.py index fb94b2ada..47081d666 100644 --- a/core/integration_tests/tests_concepts.py +++ b/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) diff --git a/core/mappings/models.py b/core/mappings/models.py index f1c98fe2b..ad34a9ec8 100644 --- a/core/mappings/models.py +++ b/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: diff --git a/core/mappings/tests/tests.py b/core/mappings/tests/tests.py index ef32f2d4d..ff493236c 100644 --- a/core/mappings/tests/tests.py +++ b/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( diff --git a/core/samples/sample_ocldev.json b/core/samples/sample_ocldev.json index 578d7e3c3..71f804f63 100644 --- a/core/samples/sample_ocldev.json +++ b/core/samples/sample_ocldev.json @@ -50,7 +50,7 @@ {"type": "Organization", "id": "DemoOrg", "website": "", "name": "OCL Demo Organization", "company": "", "public_access": "View"} {"type": "Source", "id": "DemoSource", "short_code": "DemoSource", "name": "OCL Demo Source", "full_name": "OCL Demo Source", "owner_type": "Organization", "owner": "DemoOrg", "description": "Source used for demo purposes", "default_locale": "en", "source_type": "Dictionary", "public_access": "View", "supported_locales": "en", "custom_validation_schema": "None"} {"type": "Source Version", "id": "initial", "source": "DemoSource", "description": "Initial empty repository version", "released": true, "owner": "DemoOrg", "owner_type": "Organization"} -{"type": "Concept", "id": "Food", "concept_class": "Root", "names": [{"name": "Food", "locale": "en", "locale_preferred": "True", "name_type": "Fully Specified"}], "datatype": "None", "source": "DemoSource", "owner": "DemoOrg", "owner_type": "Organization", "descriptions": []} +{"type": "Concept", "id": "Food", "concept_class": "Root", "names": [{"name": "Food", "locale": "en", "locale_preferred": "True", "name_type": "Fully Specified"}], "datatype": "NA", "source": "DemoSource", "owner": "DemoOrg", "owner_type": "Organization", "descriptions": []} {"type": "Concept", "id": "Fruit", "concept_class": "Category", "names": [{"name": "Fruit", "locale": "en", "locale_preferred": "True", "name_type": "Fully Specified"}], "datatype": "None", "source": "DemoSource", "owner": "DemoOrg", "owner_type": "Organization", "descriptions": []} {"type": "Concept", "id": "Vegetable", "concept_class": "Category", "names": [{"name": "Vegetable", "locale": "en", "locale_preferred": "True", "name_type": "Fully Specified"}], "datatype": "None", "source": "DemoSource", "owner": "DemoOrg", "owner_type": "Organization", "descriptions": []} {"type": "Concept", "id": "Apple", "concept_class": "Product", "names": [{"name": "Apple", "locale": "en", "locale_preferred": "True", "name_type": "Fully Specified"}], "datatype": "None", "source": "DemoSource", "owner": "DemoOrg", "owner_type": "Organization", "descriptions": []}