Merge pull request #1989 from dandi/unembargo-version-metadata

Re-validate version metadata during unembargo

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.')

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
 

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)

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