From a52b6cb0c0beb7de3d875500bf64ea42a6274fd5 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 29 Jul 2024 15:35:52 -0400 Subject: [PATCH 1/2] Update log message in validate_version_metadata Also add test to check that versions validated with a status besides `PENDING` are skipped --- dandiapi/api/services/metadata/__init__.py | 2 +- dandiapi/api/tests/test_tasks.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/services/metadata/__init__.py b/dandiapi/api/services/metadata/__init__.py index 985c04ff8..0656916a1 100644 --- a/dandiapi/api/services/metadata/__init__.py +++ b/dandiapi/api/services/metadata/__init__.py @@ -172,7 +172,7 @@ def _get_version_validation_result( # If the version has since been modified, return early if current_version.status != Version.Status.PENDING: logger.info( - 'Skipping validation for version %s due to concurrent modification', version_id + 'Skipping validation for version with a status of %s', current_version.status ) return diff --git a/dandiapi/api/tests/test_tasks.py b/dandiapi/api/tests/test_tasks.py index 8e7e11d98..b83945e0e 100644 --- a/dandiapi/api/tests/test_tasks.py +++ b/dandiapi/api/tests/test_tasks.py @@ -210,6 +210,26 @@ def test_validate_version_metadata(draft_version: Version, asset: Asset): assert draft_version.modified > old_modified +@pytest.mark.django_db() +def test_validate_version_metadata_non_pending_version(draft_version: Version, asset: Asset): + # Bypass .save to manually set an older timestamp, set status to INVALID + old_modified = timezone.now() - datetime.timedelta(minutes=10) + updated = Version.objects.filter(id=draft_version.id).update( + modified=old_modified, status=Version.Status.INVALID, validation_errors=['foobar'] + ) + assert updated == 1 + + draft_version.refresh_from_db() + old_validation_errors = draft_version.validation_errors + tasks.validate_version_metadata_task(draft_version.id) + draft_version.refresh_from_db() + + # Assert fields not updated + assert draft_version.status == Version.Status.INVALID + assert draft_version.validation_errors == old_validation_errors + assert draft_version.modified == old_modified + + @pytest.mark.django_db() def test_validate_version_metadata_no_side_effects(draft_version: Version, asset: Asset): draft_version.assets.add(asset) From c9ffca173cac981f303ed4d1633a4c874ef96f53 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 29 Jul 2024 15:36:54 -0400 Subject: [PATCH 2/2] Re-validate version metadata during unembargo --- dandiapi/api/services/embargo/__init__.py | 7 ++++++ dandiapi/api/tests/test_unembargo.py | 29 ++++++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 565a518fe..e2f671998 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -14,6 +14,7 @@ from dandiapi.api.services import audit from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.services.exceptions import DandiError +from dandiapi.api.services.metadata import validate_version_metadata from dandiapi.api.storage import get_boto_client from dandiapi.api.tasks import unembargo_dandiset_task @@ -94,9 +95,15 @@ def unembargo_dandiset(ds: Dandiset, user: User): # Fetch version to ensure changed embargo_status is included # Save version to update metadata through populate_metadata v = Version.objects.select_for_update().get(dandiset=ds, version='draft') + v.status = Version.Status.PENDING v.save() logger.info('Version metadata updated') + # Pre-emptively validate version metadata, so that old validation + # errors don't show up once un-embargo is finished + validate_version_metadata(version=v) + logger.info('Version metadata validated') + # Notify owners of completed unembargo send_dandiset_unembargoed_message(ds) logger.info('Dandiset owners notified.') diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index 3fd7a878e..86c656dfd 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -7,6 +7,7 @@ import pytest from dandiapi.api.models.dandiset import Dandiset +from dandiapi.api.models.version import Version from dandiapi.api.services.embargo import ( AssetBlobEmbargoedError, _remove_dandiset_asset_blob_embargo_tags, @@ -22,7 +23,6 @@ if TYPE_CHECKING: from dandiapi.api.models.asset import AssetBlob - from dandiapi.api.models.version import Version @pytest.mark.django_db() @@ -223,6 +223,33 @@ def test_unembargo_dandiset( assert owner_email_set == mailoutbox_to_email_set +@pytest.mark.django_db() +def test_unembargo_dandiset_validate_version_metadata( + draft_version_factory, asset_factory, user, mocker +): + from dandiapi.api.services import embargo as embargo_service + + draft_version: Version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + ds: Dandiset = draft_version.dandiset + assign_perm('owner', user, ds) + + draft_version.validation_errors = ['error ajhh'] + draft_version.status = Version.Status.INVALID + draft_version.save() + draft_version.assets.add(asset_factory()) + + # Spy on the imported function in the embargo service + validate_version_spy = mocker.spy(embargo_service, 'validate_version_metadata') + + unembargo_dandiset(ds, user=user) + + assert validate_version_spy.call_count == 1 + draft_version.refresh_from_db() + assert not draft_version.validation_errors + + @pytest.mark.django_db() def test_unembargo_dandiset_task_failure(draft_version_factory, mailoutbox, user, api_client): # Intentionally set the status to embargoed so the task will fail