diff --git a/backend/layers/business/business.py b/backend/layers/business/business.py index fe3430edca1e1..9eca00dad6e84 100644 --- a/backend/layers/business/business.py +++ b/backend/layers/business/business.py @@ -206,7 +206,6 @@ def update_collection_version(self, version_id: CollectionVersionId, body: Colle it should also update its publisher metadata """ - # TODO: we could collect all the changes and issue a single update at the end # TODO: CollectionMetadataUpdate should probably be used for collection creation as well # TODO: link.type should DEFINITELY move to an enum. pylance will help with the refactor @@ -224,13 +223,17 @@ def update_collection_version(self, version_id: CollectionVersionId, body: Colle else: new_doi = next((link.uri for link in body.links if link.type == "DOI"), None) + # Determine if publisher metadata should be unset, ignored or set at the end of the method. + # Note: the update needs to be done at the end to ensure atomicity + unset_publisher_metadata = False + publisher_metadata_to_set = None + if old_doi and new_doi is None: # If the DOI was deleted, remove the publisher_metadata field - self.database_provider.save_collection_publisher_metadata(version_id, None) + unset_publisher_metadata = True elif (new_doi is not None) and new_doi != old_doi: # If the DOI has changed, fetch and update the metadata - publisher_metadata = self._get_publisher_metadata(new_doi, errors) - self.database_provider.save_collection_publisher_metadata(version_id, publisher_metadata) + publisher_metadata_to_set = self._get_publisher_metadata(new_doi, errors) if errors: raise CollectionUpdateException(errors) @@ -245,6 +248,11 @@ def update_collection_version(self, version_id: CollectionVersionId, body: Colle value.strip_fields() setattr(new_metadata, field, value) + # Issue all updates + if unset_publisher_metadata: + self.database_provider.save_collection_publisher_metadata(version_id, None) + elif publisher_metadata_to_set is not None: + self.database_provider.save_collection_publisher_metadata(version_id, publisher_metadata_to_set) self.database_provider.save_collection_metadata(version_id, new_metadata) def _assert_collection_version_unpublished( diff --git a/backend/layers/common/validation.py b/backend/layers/common/validation.py index b1917c751dfff..0e484c201e9a4 100644 --- a/backend/layers/common/validation.py +++ b/backend/layers/common/validation.py @@ -22,7 +22,11 @@ def _verify_collection_metadata_fields( def check(key): value = getattr(metadata, key) - if check_existence and not value: + if check_existence and value is None: + # if checks_existence is true, value cannot be None since it must be required + errors.append({"name": key, "reason": "Cannot be empty."}) + elif value is not None and not value: + # In any case, if a value is defined, it cannot be falsey (aka blank) errors.append({"name": key, "reason": "Cannot be blank."}) elif value is not None and key == "name" and control_char_re.search(value): errors.append({"name": key, "reason": "Invalid characters detected."}) diff --git a/backend/portal/api/curation/v1/curation/collections/collection_id/actions.py b/backend/portal/api/curation/v1/curation/collections/collection_id/actions.py index e0a5a06be6cc6..ee77f6a78a16b 100644 --- a/backend/portal/api/curation/v1/curation/collections/collection_id/actions.py +++ b/backend/portal/api/curation/v1/curation/collections/collection_id/actions.py @@ -41,31 +41,55 @@ def get(collection_id: str, token_info: dict) -> Response: def patch(collection_id: str, body: dict, token_info: dict) -> Response: user_info = UserInfo(token_info) + + if "links" in body: + if not body["links"]: + raise InvalidParametersHTTPException(detail="If provided, the 'links' array may not be empty") + collection_version = get_infered_collection_version_else_forbidden(collection_id) is_owner_or_allowed_else_forbidden(collection_version, user_info) - if collection_version.published_at: + if collection_version.published_at is not None: raise MethodNotAllowedException( detail="Directly editing a public Collection is not allowed; you must create a revision." ) - # Build CollectionMetadataUpdate object - body["links"] = [Link(link.get("link_name"), link["link_type"], link["link_url"]) for link in body.get("links", [])] - collection_metadata = CollectionMetadataUpdate(**body) - - # Update the collection errors = [] - if doi_url := body.get("doi"): + # Verify DOI + if doi_url := body.pop("doi", None): if doi_url := doi.curation_get_normalized_doi_url(doi_url, errors): links = body.get("links", []) links.append({"link_type": ProjectLinkType.DOI.name, "link_url": doi_url}) body["links"] = links + + # TODO: dedup + def _link_from_request(body: dict): + return Link( + body.get("link_name"), + body["link_type"], + body["link_url"], + ) + + if body.get("links") is not None: + update_links = [_link_from_request(node) for node in body["links"]] + else: + update_links = None + + # Build CollectionMetadataUpdate object + collection_metadata = CollectionMetadataUpdate( + body.get("name"), + body.get("description"), + body.get("contact_name"), + body.get("contact_email"), + update_links, + ) + + # Update the collection try: get_business_logic().update_collection_version(collection_version.version_id, collection_metadata) except CollectionUpdateException as ex: errors.extend(ex.errors) if errors: raise InvalidParametersHTTPException(ext=dict(invalid_parameters=errors)) - # Make Response updated_collection_version = get_business_logic().get_collection_version(collection_version.version_id) diff --git a/tests/unit/backend/layers/api/test_curation_api.py b/tests/unit/backend/layers/api/test_curation_api.py index 89c9b64e1f8b5..a892532b1f2ea 100644 --- a/tests/unit/backend/layers/api/test_curation_api.py +++ b/tests/unit/backend/layers/api/test_curation_api.py @@ -1,12 +1,11 @@ import copy import json from dataclasses import asdict -import unittest from unittest.mock import Mock, patch import uuid from backend.common.corpora_orm import CollectionVisibility -from backend.common.providers.crossref_provider import CrossrefDOINotFoundException +from backend.layers.thirdparty.crossref_provider import CrossrefDOINotFoundException from backend.common.utils.api_key import generate from backend.layers.common.entities import ( CollectionVersion, @@ -15,9 +14,11 @@ DatasetStatusKey, DatasetUploadStatus, DatasetValidationStatus, + Link, OntologyTermId, ) from backend.portal.api.curation.v1.curation.collections.common import EntityColumns +from tests.unit.backend.layers.api.test_portal_api import generate_mock_publisher_metadata from tests.unit.backend.layers.common.base_test import DatasetArtifactUpdate, DatasetStatusUpdate from tests.unit.backend.layers.common.base_api_test import BaseAPIPortalTest @@ -715,7 +716,6 @@ def test__get_collection_with_x_approximate_distribution_none__OK(self): self.assertIsNone(res.json["datasets"][0]["x_approximate_distribution"]) -@unittest.skip("Trent will work on this") class TestPatchCollectionID(BaseAPIPortalTest): def setUp(self): super().setUp() @@ -738,26 +738,11 @@ def test__update_collection__OK(self): self.assertEqual(200, response.status_code) def test__update_collection_partial_data__OK(self): - description = "a description" - contact_name = "first last" - contact_email = "name@server.domain" - links = [ - {"link_name": "name", "link_type": "RAW_DATA", "link_url": "http://test_link.place"}, - ] - publisher_metadata = { - "authors": [{"name": "First Last", "given": "First", "family": "Last"}], - "journal": "Journal of Anamolous Results", - "published_year": 2022, - "published_month": 1, - } - collection_id = self.generate_collection( - description=description, - contact_name=contact_name, - contact_email=contact_email, - links=links, - publisher_metadata=publisher_metadata, - visibility="PRIVATE", - ).collection_id + links = [Link("name", "RAW_DATA", "http://test_link.place")] + self.crossref_provider.fetch_metadata = Mock(return_value=generate_mock_publisher_metadata()) + collection = self.generate_unpublished_collection(links=links) + collection_id = collection.collection_id + new_name = "A new name, and only a new name" metadata = {"name": new_name} response = self.app.patch( @@ -766,13 +751,18 @@ def test__update_collection_partial_data__OK(self): headers=self.make_owner_header(), ) self.assertEqual(200, response.status_code) + response = self.app.get(f"curation/v1/collections/{collection_id}") + print(response.json) self.assertEqual(new_name, response.json["name"]) - self.assertEqual(description, response.json["description"]) - self.assertEqual(contact_name, response.json["contact_name"]) - self.assertEqual(contact_email, response.json["contact_email"]) - self.assertEqual(links, response.json["links"]) - self.assertEqual(publisher_metadata, response.json["publisher_metadata"]) + self.assertEqual(collection.metadata.description, response.json["description"]) + self.assertEqual(collection.metadata.contact_name, response.json["contact_name"]) + self.assertEqual(collection.metadata.contact_email, response.json["contact_email"]) + self.assertEqual( + [{"link_name": "name", "link_type": "RAW_DATA", "link_url": "http://test_link.place"}], + response.json["links"], + ) + self.assertEqual(collection.publisher_metadata, response.json["publisher_metadata"]) def test_update_collection_with_empty_required_fields(self): tests = [dict(description=""), dict(contact_name=""), dict(contact_email=""), dict(name="")] @@ -818,9 +808,9 @@ def test__update_collection__links__OK(self): self.assertEqual(expected_links, response.json["links"]) def test__update_collection__doi__OK(self): - initial_doi = "12.3456/doi_curie_reference" + initial_doi = "10.2020" links = [ - {"link_name": "new doi", "link_type": "DOI", "link_url": initial_doi}, + {"link_name": "new doi", "link_type": "DOI", "link_url": "http://doi.org/10.2020"}, ] new_doi = "10.1016" # a real DOI (CURIE reference) collection_id = self.generate_collection(links=links, visibility="PRIVATE").collection_id @@ -861,38 +851,32 @@ def test__update_collection__doi_does_not_exist__BAD_REQUEST(self): new_links = [ {"link_name": "new link", "link_type": "RAW_DATA", "link_url": "http://brand_new_link.place"}, ] - publisher_metadata = { - "authors": [{"name": "First Last", "given": "First", "family": "Last"}], - "journal": "Journal of Anamolous Results", - "published_year": 2022, - "published_month": 1, - } - with patch( - "backend.common.providers.crossref_provider.CrossrefProvider.fetch_metadata", - side_effect=CrossrefDOINotFoundException(), - ): - collection = self.generate_collection( - links=links, publisher_metadata=publisher_metadata, visibility="PRIVATE" - ) - collection_id = collection.collection_id - original_collection = self.app.get(f"curation/v1/collections/{collection_id}").json - - # Only compare to first item in links list because "DOI" type gets removed from Curator API response - self.assertEqual(links[:1], original_collection["links"]) - metadata = {"links": new_links, "doi": "10.1016/bad_doi"} - response = self.app.patch( - f"/curation/v1/collections/{collection_id}", - data=json.dumps(metadata), - headers=self.make_owner_header(), - ) - self.assertEqual(400, response.status_code) - original_collection_unchanged = self.app.get(f"curation/v1/collections/{collection_id}").json - # Only compare to first item in links list because "DOI" type gets removed from Curator API response - self.assertEqual(links[:1], original_collection_unchanged["links"]) - self.assertEqual(publisher_metadata, original_collection_unchanged["publisher_metadata"]) + mock_publisher_metadata = generate_mock_publisher_metadata() + self.crossref_provider.fetch_metadata = Mock(return_value=mock_publisher_metadata) + collection = self.generate_collection(links=links, visibility="PRIVATE") + self.assertIsNotNone(collection.publisher_metadata) + collection_id = collection.collection_id + original_collection = self.app.get(f"curation/v1/collections/{collection_id}").json + + self.crossref_provider.fetch_metadata = Mock(side_effect=CrossrefDOINotFoundException()) + + # Only compare to first item in links list because "DOI" type gets removed from Curator API response + self.assertEqual(links[:1], original_collection["links"]) + metadata = {"links": new_links, "doi": "10.1016/bad_doi"} + response = self.app.patch( + f"/curation/v1/collections/{collection_id}", + data=json.dumps(metadata), + headers=self.make_owner_header(), + ) + self.assertEqual(400, response.status_code) + original_collection_unchanged = self.app.get(f"curation/v1/collections/{collection_id}").json + + # Only compare to first item in links list because "DOI" type gets removed from Curator API response + self.assertEqual(links[:1], original_collection_unchanged["links"]) + self.assertEqual(mock_publisher_metadata, original_collection_unchanged["publisher_metadata"]) def test__update_collection__Not_Owner(self): - collection_id = self.generate_unpublished_collection(owner="someone else") + collection_id = self.generate_unpublished_collection(owner="someone else").collection_id response = self.app.patch( f"/curation/v1/collections/{collection_id}", data=json.dumps(self.test_collection), @@ -901,7 +885,7 @@ def test__update_collection__Not_Owner(self): self.assertEqual(403, response.status_code) def test__update_collection__Super_Curator(self): - collection_id = self.generate_unpublished_collection() + collection_id = self.generate_unpublished_collection().collection_id headers = self.make_super_curator_header() response = self.app.patch( f"/curation/v1/collections/{collection_id}", data=json.dumps(self.test_collection), headers=headers @@ -909,7 +893,7 @@ def test__update_collection__Super_Curator(self): self.assertEqual(200, response.status_code) def test__update_public_collection_owner__405(self): - collection_id = self.generate_published_collection() + collection_id = self.generate_published_collection().collection_id headers = self.make_super_curator_header() response = self.app.patch( f"/curation/v1/collections/{collection_id}", data=json.dumps(self.test_collection), headers=headers @@ -917,7 +901,7 @@ def test__update_public_collection_owner__405(self): self.assertEqual(405, response.status_code) self.assertEqual( "Directly editing a public Collection is not allowed; you must create a revision.", - json.loads(response.text)["detail"], + response.json["detail"], ) diff --git a/tests/unit/backend/layers/common/base_test.py b/tests/unit/backend/layers/common/base_test.py index 3220d368f54ec..fda7717c4136f 100644 --- a/tests/unit/backend/layers/common/base_test.py +++ b/tests/unit/backend/layers/common/base_test.py @@ -138,8 +138,8 @@ def generate_unpublished_collection( links: List[Link] = [], add_datasets: int = 0, metadata=None, - ) -> CollectionVersionWithDatasets: - links = links if links else [] + ) -> CollectionVersion: + links = links or [] if not metadata: metadata = copy.deepcopy(self.sample_collection_metadata) metadata.links = links