From b816ea2a0a4c6e1356e98b0f59db0a300c014ffc Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 4 Jun 2024 14:14:11 -0400 Subject: [PATCH 01/12] Automate dandiset unembargo --- dandiapi/api/services/embargo/__init__.py | 82 ++++++++++++++++----- dandiapi/api/services/embargo/exceptions.py | 6 ++ 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 773c301fd..dc2827e14 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -1,29 +1,34 @@ from __future__ import annotations from concurrent.futures import ThreadPoolExecutor +import logging from typing import TYPE_CHECKING from botocore.config import Config from django.conf import settings from django.db import transaction -from dandiapi.api.mail import send_dandisets_to_unembargo_message -from dandiapi.api.models import Asset, AssetBlob, Dandiset, Version +from dandiapi.api.mail import send_dandiset_unembargoed_message, send_dandisets_to_unembargo_message +from dandiapi.api.models import AssetBlob, Dandiset, Version from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError +from dandiapi.api.services.exceptions import DandiError from dandiapi.api.storage import get_boto_client from .exceptions import ( AssetBlobEmbargoedError, + AssetTagRemovalError, DandisetActiveUploadsError, DandisetNotEmbargoedError, ) if TYPE_CHECKING: from django.contrib.auth.models import User - from django.db.models import QuerySet from mypy_boto3_s3 import S3Client +logger = logging.getLogger(__name__) + + def _delete_asset_blob_tags(client: S3Client, blob: str): client.delete_object_tagging( Bucket=settings.DANDI_DANDISETS_BUCKET_NAME, @@ -33,28 +38,65 @@ def _delete_asset_blob_tags(client: S3Client, blob: str): # NOTE: In testing this took ~2 minutes for 100,000 files def _remove_dandiset_asset_blob_embargo_tags(dandiset: Dandiset): - # First we need to generate a CSV manifest containing all asset blobs that need to be untaged - embargoed_asset_blobs = AssetBlob.objects.filter( - embargoed=True, assets__versions__dandiset=dandiset - ).values_list('blob', flat=True) + embargoed_asset_blobs = list( + AssetBlob.objects.filter(embargoed=True, assets__versions__dandiset=dandiset).values_list( + 'blob', flat=True + ) + ) client = get_boto_client(config=Config(max_pool_connections=100)) with ThreadPoolExecutor(max_workers=100) as e: - for blob in embargoed_asset_blobs: + futures = [ e.submit(_delete_asset_blob_tags, client=client, blob=blob) + for blob in embargoed_asset_blobs + ] + + # Check if any failed and raise exception if so + failed = [ + blob for i, blob in enumerate(embargoed_asset_blobs) if futures[i].exception() is not None + ] + if failed: + raise AssetTagRemovalError('Some blobs failed to remove tags', blobs=failed) @transaction.atomic() -def _unembargo_dandiset(dandiset: Dandiset): - # NOTE: Before proceeding, all asset blobs must have their embargoed tags removed in s3 +def _unembargo_dandiset(ds: Dandiset): + logger.info('Unembargoing Dandiset %s', ds.identifier) + logger.info('\t%s assets', ds.draft_version.assets.count()) + + if ds.embargo_status != Dandiset.EmbargoStatus.UNEMBARGOING: + raise DandiError( + message=f'Expected dandiset state {Dandiset.EmbargoStatus.UNEMBARGOING}, found {ds.embargo_status}', # noqa: E501 + http_status_code=500, + ) + if ds.uploads.all().exists(): + raise DandisetActiveUploadsError(http_status_code=500) + + # Remove tags in S3 + logger.info('Removing tags...') + _remove_dandiset_asset_blob_embargo_tags(ds) + + # Update embargoed flag on asset blobs + updated = AssetBlob.objects.filter(embargoed=True, assets__versions__dandiset=ds).update( + embargoed=False + ) + logger.info('Updated %s asset blobs', updated) + + # Set status to OPEN + Dandiset.objects.filter(pk=ds.pk).update(embargo_status=Dandiset.EmbargoStatus.OPEN) + logger.info('Dandiset embargo status updated') - draft_version: Version = dandiset.draft_version - embargoed_assets: QuerySet[Asset] = draft_version.assets.filter(blob__embargoed=True) - AssetBlob.objects.filter(assets__in=embargoed_assets).update(embargoed=False) + # Fetch version to ensure changed embargo_status is included + # Save version to update metadata through populate_metadata + v = Version.objects.get(dandiset=ds, version='draft') + v.save() + logger.info('Version metadata updated') - # Set access on dandiset - dandiset.embargo_status = Dandiset.EmbargoStatus.OPEN - dandiset.save() + # Notify owners of completed unembargo + send_dandiset_unembargoed_message(ds) + logger.info('Dandiset owners notified.') + + logger.info('...Done') def remove_asset_blob_embargoed_tag(asset_blob: AssetBlob) -> None: @@ -76,10 +118,10 @@ def unembargo_dandiset(*, user: User, dandiset: Dandiset): if dandiset.uploads.count(): raise DandisetActiveUploadsError - # A scheduled task will pick up any new dandisets with this status and email the admins to - # initiate the unembargo process - dandiset.embargo_status = Dandiset.EmbargoStatus.UNEMBARGOING - dandiset.save() + # A scheduled task will pick up any new dandisets with this status + Dandiset.objects.filter(pk=dandiset.pk).update( + embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) # Send initial email to ensure it's seen in a timely manner send_dandisets_to_unembargo_message(dandisets=[dandiset]) diff --git a/dandiapi/api/services/embargo/exceptions.py b/dandiapi/api/services/embargo/exceptions.py index 13038bd59..1bdc0fb25 100644 --- a/dandiapi/api/services/embargo/exceptions.py +++ b/dandiapi/api/services/embargo/exceptions.py @@ -30,3 +30,9 @@ class UnauthorizedEmbargoAccessError(DandiError): message = ( 'Authentication credentials must be provided when attempting to access embargoed dandisets' ) + + +class AssetTagRemovalError(Exception): + def __init__(self, message: str, blobs: list[str]) -> None: + super().__init__(message) + self.blobs = blobs From c8801941ac8c0a2fcf2c3154d064a54327c271de Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 1 Jul 2024 14:59:47 -0400 Subject: [PATCH 02/12] Rename unembargo functions --- dandiapi/api/services/embargo/__init__.py | 7 ++++--- dandiapi/api/views/dandiset.py | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index dc2827e14..0a421d368 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -60,7 +60,8 @@ def _remove_dandiset_asset_blob_embargo_tags(dandiset: Dandiset): @transaction.atomic() -def _unembargo_dandiset(ds: Dandiset): +def unembargo_dandiset(ds: Dandiset): + """Unembargo a dandiset by copying all embargoed asset blobs to the public bucket.""" logger.info('Unembargoing Dandiset %s', ds.identifier) logger.info('\t%s assets', ds.draft_version.assets.count()) @@ -107,8 +108,8 @@ def remove_asset_blob_embargoed_tag(asset_blob: AssetBlob) -> None: _delete_asset_blob_tags(client=get_boto_client(), blob=asset_blob.blob.name) -def unembargo_dandiset(*, user: User, dandiset: Dandiset): - """Unembargo a dandiset by copying all embargoed asset blobs to the public bucket.""" +def kickoff_dandiset_unembargo(*, user: User, dandiset: Dandiset): + """Set dandiset status to kickoff unembargo.""" if dandiset.embargo_status != Dandiset.EmbargoStatus.EMBARGOED: raise DandisetNotEmbargoedError diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index d202cc11f..770e58366 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -25,7 +25,7 @@ from dandiapi.api.mail import send_ownership_change_emails from dandiapi.api.models import Dandiset, Version from dandiapi.api.services.dandiset import create_dandiset, delete_dandiset -from dandiapi.api.services.embargo import unembargo_dandiset +from dandiapi.api.services.embargo import kickoff_dandiset_unembargo from dandiapi.api.services.embargo.exceptions import ( DandisetUnembargoInProgressError, UnauthorizedEmbargoAccessError, @@ -349,7 +349,7 @@ def destroy(self, request, dandiset__pk): @method_decorator(permission_required_or_403('owner', (Dandiset, 'pk', 'dandiset__pk'))) def unembargo(self, request, dandiset__pk): dandiset: Dandiset = get_object_or_404(Dandiset, pk=dandiset__pk) - unembargo_dandiset(user=request.user, dandiset=dandiset) + kickoff_dandiset_unembargo(user=request.user, dandiset=dandiset) return Response(None, status=status.HTTP_200_OK) From 10e81b9130855b31ecb7cc8e4bfd090f535866b4 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 1 Jul 2024 16:52:33 -0400 Subject: [PATCH 03/12] Add unembargo tests --- dandiapi/api/tests/test_unembargo.py | 140 +++++++++++++++++++++++---- 1 file changed, 122 insertions(+), 18 deletions(-) diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index b4976d832..1d3bdacda 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -1,11 +1,23 @@ from __future__ import annotations +from typing import TYPE_CHECKING + +import dandischema from guardian.shortcuts import assign_perm import pytest from dandiapi.api.models.dandiset import Dandiset -from dandiapi.api.services.embargo import AssetBlobEmbargoedError, remove_asset_blob_embargoed_tag -from dandiapi.api.tasks.scheduled import send_dandisets_to_unembargo_email +from dandiapi.api.services.embargo import ( + AssetBlobEmbargoedError, + remove_asset_blob_embargoed_tag, + unembargo_dandiset, +) +from dandiapi.api.services.embargo.exceptions import DandisetActiveUploadsError +from dandiapi.api.services.exceptions import DandiError + +if TYPE_CHECKING: + from dandiapi.api.models.asset import AssetBlob + from dandiapi.api.models.version import Version @pytest.mark.django_db() @@ -18,39 +30,131 @@ def test_remove_asset_blob_embargoed_tag_fails_on_embargod(embargoed_asset_blob, @pytest.mark.django_db() -def test_unembargo_dandiset_sends_emails( - api_client, user, dandiset, draft_version_factory, mailoutbox +def test_kickoff_dandiset_unembargo_dandiset_not_embargoed( + api_client, user, dandiset_factory, draft_version_factory ): + dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.OPEN) draft_version_factory(dandiset=dandiset) - assign_perm('owner', user, dandiset) api_client.force_authenticate(user=user) - dandiset.embargo_status = Dandiset.EmbargoStatus.EMBARGOED - dandiset.save() - resp = api_client.post(f'/api/dandisets/{dandiset.identifier}/unembargo/') - assert resp.status_code == 200 + assert resp.status_code == 400 - # Simulate the scheduled task calling this function - send_dandisets_to_unembargo_email() - assert mailoutbox - assert 'unembargo' in mailoutbox[0].subject - assert dandiset.identifier in mailoutbox[0].message().get_payload() - assert user.username in mailoutbox[0].message().get_payload() +@pytest.mark.django_db() +def test_kickoff_dandiset_unembargo_not_owner( + api_client, user, dandiset_factory, draft_version_factory +): + dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) + draft_version_factory(dandiset=dandiset) + api_client.force_authenticate(user=user) + + resp = api_client.post(f'/api/dandisets/{dandiset.identifier}/unembargo/') + assert resp.status_code == 403 @pytest.mark.django_db() -def test_unembargo_dandiset_lingering_uploads( +def test_kickoff_dandiset_unembargo_active_uploads( api_client, user, dandiset_factory, draft_version_factory, upload_factory ): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.EMBARGOED) draft_version_factory(dandiset=dandiset) - upload_factory(dandiset=dandiset) - assign_perm('owner', user, dandiset) api_client.force_authenticate(user=user) + # Test that active uploads prevent unembargp + upload_factory(dandiset=dandiset) resp = api_client.post(f'/api/dandisets/{dandiset.identifier}/unembargo/') assert resp.status_code == 400 + + +@pytest.mark.django_db() +def test_kickoff_dandiset_unembargo(api_client, user, draft_version_factory, mailoutbox): + draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) + ds: Dandiset = draft_version.dandiset + + assign_perm('owner', user, ds) + api_client.force_authenticate(user=user) + + resp = api_client.post(f'/api/dandisets/{ds.identifier}/unembargo/') + assert resp.status_code == 200 + + ds.refresh_from_db() + assert ds.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING + assert mailoutbox + assert 'unembargo' in mailoutbox[0].subject + assert ds.identifier in mailoutbox[0].message().get_payload() + assert user.username in mailoutbox[0].message().get_payload() + + +@pytest.mark.django_db() +def test_unembargo_dandiset_not_unembargoing(draft_version_factory): + draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) + ds: Dandiset = draft_version.dandiset + + with pytest.raises(DandiError): + unembargo_dandiset(ds) + + +@pytest.mark.django_db() +def test_unembargo_dandiset_uploads_exist(draft_version_factory, upload_factory): + draft_version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + ds: Dandiset = draft_version.dandiset + + upload_factory(dandiset=ds) + with pytest.raises(DandisetActiveUploadsError): + unembargo_dandiset(ds) + + +@pytest.mark.django_db() +def test_unembargo_dandiset( + draft_version_factory, + asset_factory, + embargoed_asset_blob_factory, + mocker, + mailoutbox, + user_factory, +): + draft_version: Version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + ds: Dandiset = draft_version.dandiset + owners = [user_factory() for _ in range(5)] + for user in owners: + assign_perm('owner', user, ds) + + embargoed_blob: AssetBlob = embargoed_asset_blob_factory() + asset = asset_factory(blob=embargoed_blob) + draft_version.assets.add(asset) + assert embargoed_blob.embargoed + + # Patch this function to check if it's been called, since we can't test the tagging directly + patched = mocker.patch('dandiapi.api.services.embargo._delete_asset_blob_tags') + + unembargo_dandiset(ds) + patched.assert_called_once() + + embargoed_blob.refresh_from_db() + ds.refresh_from_db() + draft_version.refresh_from_db() + assert not embargoed_blob.embargoed + assert ds.embargo_status == Dandiset.EmbargoStatus.OPEN + assert ( + draft_version.metadata['access'][0]['status'] + == dandischema.models.AccessType.OpenAccess.value + ) + + # Check that a correct email exists + assert mailoutbox + assert 'has been unembargoed' in mailoutbox[0].subject + payload = mailoutbox[0].message().get_payload()[0].get_payload() + assert ds.identifier in payload + assert 'has been unembargoed' in payload + + # Check that the email was sent to all owners + owner_email_set = {user.email for user in owners} + mailoutbox_to_email_set = set(mailoutbox[0].to) + assert owner_email_set == mailoutbox_to_email_set From 3c3669de904047d8cab9ae5e69b11a86d2c1e8b0 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 2 Jul 2024 13:50:30 -0400 Subject: [PATCH 04/12] Connect unembargo endpoint and task --- dandiapi/api/mail.py | 25 ------------ dandiapi/api/models/dandiset.py | 4 ++ dandiapi/api/services/dandiset/__init__.py | 2 +- dandiapi/api/services/embargo/__init__.py | 15 ++++---- dandiapi/api/tasks/__init__.py | 9 +++++ dandiapi/api/tasks/scheduled.py | 14 +------ .../api/mail/dandisets_to_unembargo.txt | 10 ----- dandiapi/api/tests/test_asset.py | 6 +-- dandiapi/api/tests/test_dandiset.py | 38 +++++++++---------- dandiapi/api/tests/test_unembargo.py | 16 +++++--- dandiapi/api/tests/test_upload.py | 2 +- dandiapi/api/tests/test_version.py | 4 +- dandiapi/api/views/asset.py | 6 +-- dandiapi/api/views/dandiset.py | 2 +- dandiapi/api/views/upload.py | 2 +- dandiapi/api/views/version.py | 2 +- .../DandisetLandingView/DandisetActions.vue | 3 +- .../DandisetLandingView/DandisetUnembargo.vue | 10 ++--- web/src/views/FileBrowserView/FileBrowser.vue | 3 +- 19 files changed, 71 insertions(+), 102 deletions(-) delete mode 100644 dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt diff --git a/dandiapi/api/mail.py b/dandiapi/api/mail.py index d68395d69..4d94dfcfb 100644 --- a/dandiapi/api/mail.py +++ b/dandiapi/api/mail.py @@ -182,31 +182,6 @@ def send_pending_users_message(users: Iterable[User]): connection.send_messages(messages) -def build_dandisets_to_unembargo_message(dandisets: Iterable[Dandiset]): - dandiset_context = [ - { - 'identifier': ds.identifier, - 'owners': [user.username for user in ds.owners], - 'asset_count': ds.draft_version.asset_count, - 'size': ds.draft_version.size, - } - for ds in dandisets - ] - render_context = {**BASE_RENDER_CONTEXT, 'dandisets': dandiset_context} - return build_message( - subject='DANDI: New Dandisets to unembargo', - message=render_to_string('api/mail/dandisets_to_unembargo.txt', render_context), - to=[settings.DANDI_DEV_EMAIL], - ) - - -def send_dandisets_to_unembargo_message(dandisets: Iterable[Dandiset]): - logger.info('Sending dandisets to unembargo message to devs at %s', settings.DANDI_DEV_EMAIL) - messages = [build_dandisets_to_unembargo_message(dandisets)] - with mail.get_connection() as connection: - connection.send_messages(messages) - - def build_dandiset_unembargoed_message(dandiset: Dandiset): dandiset_context = { 'identifier': dandiset.identifier, diff --git a/dandiapi/api/models/dandiset.py b/dandiapi/api/models/dandiset.py index 595467dc7..4d95b6db4 100644 --- a/dandiapi/api/models/dandiset.py +++ b/dandiapi/api/models/dandiset.py @@ -62,6 +62,10 @@ def identifier(self) -> str: def embargoed(self) -> bool: return self.embargo_status == self.EmbargoStatus.EMBARGOED + @property + def unembargo_in_progress(self) -> bool: + return self.embargo_status == self.EmbargoStatus.UNEMBARGOING + @property def most_recent_published_version(self): return self.versions.exclude(version='draft').order_by('modified').last() diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 05450a94f..1b8d7f9e5 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -56,7 +56,7 @@ def delete_dandiset(*, user, dandiset: Dandiset) -> None: raise NotAllowedError('Cannot delete dandisets with published versions.') if dandiset.versions.filter(status=Version.Status.PUBLISHING).exists(): raise NotAllowedError('Cannot delete dandisets that are currently being published.') - if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError # Delete all versions first, so that AssetPath deletion is cascaded diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 0a421d368..2ad9a8aca 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -8,11 +8,12 @@ from django.conf import settings from django.db import transaction -from dandiapi.api.mail import send_dandiset_unembargoed_message, send_dandisets_to_unembargo_message +from dandiapi.api.mail import send_dandiset_unembargoed_message from dandiapi.api.models import AssetBlob, Dandiset, Version from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.services.exceptions import DandiError from dandiapi.api.storage import get_boto_client +from dandiapi.api.tasks import unembargo_dandiset_task from .exceptions import ( AssetBlobEmbargoedError, @@ -119,10 +120,8 @@ def kickoff_dandiset_unembargo(*, user: User, dandiset: Dandiset): if dandiset.uploads.count(): raise DandisetActiveUploadsError - # A scheduled task will pick up any new dandisets with this status - Dandiset.objects.filter(pk=dandiset.pk).update( - embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING - ) - - # Send initial email to ensure it's seen in a timely manner - send_dandisets_to_unembargo_message(dandisets=[dandiset]) + with transaction.atomic(): + Dandiset.objects.filter(pk=dandiset.pk).update( + embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + transaction.on_commit(lambda: unembargo_dandiset_task.delay(dandiset.pk)) diff --git a/dandiapi/api/tasks/__init__.py b/dandiapi/api/tasks/__init__.py index b06a87b2c..2a7fa9318 100644 --- a/dandiapi/api/tasks/__init__.py +++ b/dandiapi/api/tasks/__init__.py @@ -12,6 +12,7 @@ write_dandiset_yaml, ) from dandiapi.api.models import Asset, AssetBlob, Version +from dandiapi.api.models.dandiset import Dandiset logger = get_task_logger(__name__) @@ -74,3 +75,11 @@ def publish_dandiset_task(dandiset_id: int): from dandiapi.api.services.publish import _publish_dandiset _publish_dandiset(dandiset_id=dandiset_id) + + +@shared_task +def unembargo_dandiset_task(dandiset_id: int): + from dandiapi.api.services.embargo import unembargo_dandiset + + ds = Dandiset.objects.get(pk=dandiset_id) + unembargo_dandiset(ds) diff --git a/dandiapi/api/tasks/scheduled.py b/dandiapi/api/tasks/scheduled.py index 9fa8af71d..2b4a100e9 100644 --- a/dandiapi/api/tasks/scheduled.py +++ b/dandiapi/api/tasks/scheduled.py @@ -20,10 +20,9 @@ from django.db.models.query_utils import Q from dandiapi.analytics.tasks import collect_s3_log_records_task -from dandiapi.api.mail import send_dandisets_to_unembargo_message, send_pending_users_message +from dandiapi.api.mail import send_pending_users_message from dandiapi.api.models import UserMetadata, Version from dandiapi.api.models.asset import Asset -from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.services.metadata import version_aggregate_assets_summary from dandiapi.api.services.metadata.exceptions import VersionMetadataConcurrentlyModifiedError from dandiapi.api.tasks import ( @@ -112,14 +111,6 @@ def send_pending_users_email() -> None: send_pending_users_message(pending_users) -@shared_task(soft_time_limit=20) -def send_dandisets_to_unembargo_email() -> None: - """Send an email to admins listing dandisets that have requested unembargo.""" - dandisets = Dandiset.objects.filter(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) - if dandisets.exists(): - send_dandisets_to_unembargo_message(dandisets) - - @shared_task(soft_time_limit=60) def refresh_materialized_view_search() -> None: """ @@ -148,9 +139,6 @@ def register_scheduled_tasks(sender: Celery, **kwargs): # Send daily email to admins containing a list of users awaiting approval sender.add_periodic_task(crontab(hour=0, minute=0), send_pending_users_email.s()) - # Send daily email to admins containing a list of dandisets to unembargo - sender.add_periodic_task(crontab(hour=0, minute=0), send_dandisets_to_unembargo_email.s()) - # Refresh the materialized view used by asset search every 10 mins. sender.add_periodic_task(timedelta(minutes=10), refresh_materialized_view_search.s()) diff --git a/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt b/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt deleted file mode 100644 index c716839b1..000000000 --- a/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt +++ /dev/null @@ -1,10 +0,0 @@ -{% autoescape off %} -The following new dandisets are awaiting unembargo: - -{% for ds in dandisets %} -Dandiset ID: {{ ds.identifier }} -Owners: {{ ds.owners }} -Number of Assets: {{ ds.asset_count }} -Total data size: {{ ds.size }} -{% endfor %} -{% endautoescape %} diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 68dadca42..fc0e32f4a 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -637,7 +637,7 @@ def test_asset_create_embargo( @pytest.mark.django_db() -def test_asset_create_unembargoing( +def test_asset_create_unembargo_in_progress( api_client, user, draft_version_factory, dandiset_factory, embargoed_asset_blob ): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) @@ -1122,7 +1122,7 @@ def test_asset_rest_update_embargo(api_client, user, draft_version, asset, embar @pytest.mark.django_db() -def test_asset_rest_update_unembargoing( +def test_asset_rest_update_unembargo_in_progress( api_client, user, draft_version_factory, asset, embargoed_asset_blob ): draft_version = draft_version_factory( @@ -1325,7 +1325,7 @@ def test_asset_rest_delete(api_client, user, draft_version, asset): @pytest.mark.django_db() -def test_asset_rest_delete_unembargoing(api_client, user, draft_version_factory, asset): +def test_asset_rest_delete_unembargo_in_progress(api_client, user, draft_version_factory, asset): draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index 9efeed612..4ac6d78e1 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -735,30 +735,28 @@ def test_dandiset_rest_create_with_invalid_identifier(api_client, admin_user): @pytest.mark.django_db() -def test_dandiset_rest_delete(api_client, draft_version_factory, user): +@pytest.mark.parametrize( + ('embargo_status', 'success'), + [ + (Dandiset.EmbargoStatus.OPEN, True), + (Dandiset.EmbargoStatus.EMBARGOED, True), + (Dandiset.EmbargoStatus.UNEMBARGOING, False), + ], +) +def test_dandiset_rest_delete(api_client, draft_version_factory, user, embargo_status, success): api_client.force_authenticate(user=user) # Ensure that open or embargoed dandisets can be deleted - draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.OPEN) - assign_perm('owner', user, draft_version.dandiset) - response = api_client.delete(f'/api/dandisets/{draft_version.dandiset.identifier}/') - assert response.status_code == 204 - assert not Dandiset.objects.all() - - draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) + draft_version = draft_version_factory(dandiset__embargo_status=embargo_status) assign_perm('owner', user, draft_version.dandiset) response = api_client.delete(f'/api/dandisets/{draft_version.dandiset.identifier}/') - assert response.status_code == 204 - assert not Dandiset.objects.all() - # Ensure that currently unembargoing dandisets can't be deleted - draft_version = draft_version_factory( - dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING - ) - assign_perm('owner', user, draft_version.dandiset) - response = api_client.delete(f'/api/dandisets/{draft_version.dandiset.identifier}/') - assert response.status_code == 400 - assert Dandiset.objects.count() == 1 + if success: + assert response.status_code == 204 + assert not Dandiset.objects.all() + else: + assert response.status_code >= 400 + assert Dandiset.objects.count() == 1 @pytest.mark.django_db() @@ -885,13 +883,13 @@ def test_dandiset_rest_change_owner( @pytest.mark.django_db() -def test_dandiset_rest_change_owners_unembargoing( +def test_dandiset_rest_change_owners_unembargo_in_progress( api_client, draft_version_factory, user_factory, social_account_factory, ): - """Test that unembargoing a dandiset prevents user modification.""" + """Test that a dandiset undergoing unembargo prevents user modification.""" draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index 1d3bdacda..f59d9fc59 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -69,23 +69,27 @@ def test_kickoff_dandiset_unembargo_active_uploads( assert resp.status_code == 400 -@pytest.mark.django_db() -def test_kickoff_dandiset_unembargo(api_client, user, draft_version_factory, mailoutbox): +# transaction=True required due to how `kickoff_dandiset_unembargo` calls `unembargo_dandiset_task` +@pytest.mark.django_db(transaction=True) +def test_kickoff_dandiset_unembargo(api_client, user, draft_version_factory, mailoutbox, mocker): draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) ds: Dandiset = draft_version.dandiset assign_perm('owner', user, ds) api_client.force_authenticate(user=user) + # mock this task to check if called + patched_task = mocker.patch('dandiapi.api.services.embargo.unembargo_dandiset_task') + resp = api_client.post(f'/api/dandisets/{ds.identifier}/unembargo/') assert resp.status_code == 200 ds.refresh_from_db() assert ds.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING - assert mailoutbox - assert 'unembargo' in mailoutbox[0].subject - assert ds.identifier in mailoutbox[0].message().get_payload() - assert user.username in mailoutbox[0].message().get_payload() + + # Check that unembargo dandiset task was delayed + assert len(patched_task.mock_calls) == 1 + assert str(patched_task.mock_calls[0]) == f'call.delay({ds.pk})' @pytest.mark.django_db() diff --git a/dandiapi/api/tests/test_upload.py b/dandiapi/api/tests/test_upload.py index 133c2ff7a..84c1b7bb2 100644 --- a/dandiapi/api/tests/test_upload.py +++ b/dandiapi/api/tests/test_upload.py @@ -110,7 +110,7 @@ def test_upload_initialize(api_client, user, dandiset_factory, embargoed): @pytest.mark.django_db() -def test_upload_initialize_unembargoing(api_client, user, dandiset_factory): +def test_upload_initialize_unembargo_in_progress(api_client, user, dandiset_factory): dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) api_client.force_authenticate(user=user) assign_perm('owner', user, dandiset) diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index f25ae29cb..08b615ebc 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -606,7 +606,7 @@ def test_version_rest_update(api_client, user, draft_version): @pytest.mark.django_db() -def test_version_rest_update_unembargoing(api_client, user, draft_version_factory): +def test_version_rest_update_unembargo_in_progress(api_client, user, draft_version_factory): draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) @@ -802,7 +802,7 @@ def test_version_rest_publish_embargo(api_client: APIClient, user: User, draft_v @pytest.mark.django_db() -def test_version_rest_publish_unembargoing( +def test_version_rest_publish_unembargo_in_progress( api_client: APIClient, user: User, draft_version_factory ): draft_version = draft_version_factory( diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index d92fa0d0e..5a3502e53 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -324,7 +324,7 @@ def create(self, request, versions__dandiset__pk, versions__version): version=versions__version, ) - if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if version.dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError serializer = AssetRequestSerializer(data=self.request.data) @@ -361,7 +361,7 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): ) if version.version != 'draft': raise DraftDandisetNotModifiableError - if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if version.dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError serializer = AssetRequestSerializer(data=self.request.data) @@ -399,7 +399,7 @@ def destroy(self, request, versions__dandiset__pk, versions__version, **kwargs): dandiset__pk=versions__dandiset__pk, version=versions__version, ) - if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if version.dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError # Lock asset for delete diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 770e58366..984b80d2c 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -377,7 +377,7 @@ def unembargo(self, request, dandiset__pk): def users(self, request, dandiset__pk): # noqa: C901 dandiset: Dandiset = self.get_object() if request.method == 'PUT': - if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError # Verify that the user is currently an owner diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index acdf6a41e..433195716 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -137,7 +137,7 @@ def upload_initialize_view(request: Request) -> HttpResponseBase: return response # Ensure dandiset not in the process of unembargo - if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError logging.info( diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 0a052d9a7..d37adc931 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -95,7 +95,7 @@ def update(self, request, **kwargs): 'Only draft versions can be modified.', status=status.HTTP_405_METHOD_NOT_ALLOWED, ) - if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + if version.dandiset.unembargo_in_progress: raise DandisetUnembargoInProgressError serializer = VersionMetadataSerializer(data=request.data) diff --git a/web/src/views/DandisetLandingView/DandisetActions.vue b/web/src/views/DandisetLandingView/DandisetActions.vue index fc9ae4391..9249090ee 100644 --- a/web/src/views/DandisetLandingView/DandisetActions.vue +++ b/web/src/views/DandisetLandingView/DandisetActions.vue @@ -72,7 +72,7 @@ id="view-data" outlined block - :disabled="currentDandiset.dandiset.embargo_status === 'UNEMBARGOING'" + :disabled="unembargo_in_progress" :to="fileBrowserLink" exact > @@ -156,6 +156,7 @@ const store = useDandisetStore(); const currentDandiset = computed(() => store.dandiset); const currentVersion = computed(() => store.version); +const unembargo_in_progress = computed(() => currentDandiset.value && currentDandiset.value.dandiset.embargo_status === 'UNEMBARGOING') const fileBrowserLink: ComputedRef = computed(() => { if (!currentDandiset.value) { diff --git a/web/src/views/DandisetLandingView/DandisetUnembargo.vue b/web/src/views/DandisetLandingView/DandisetUnembargo.vue index c6c846a80..e1fbfc520 100644 --- a/web/src/views/DandisetLandingView/DandisetUnembargo.vue +++ b/web/src/views/DandisetLandingView/DandisetUnembargo.vue @@ -65,7 +65,7 @@ > - This dandiset is being unembargoed, please wait. + This dandiset is being unembargoed, please wait. @@ -126,7 +126,7 @@ function formatDate(date: string): string { const store = useDandisetStore(); const currentDandiset = computed(() => store.dandiset); -const unembargoing = computed(() => currentDandiset.value?.dandiset.embargo_status === 'UNEMBARGOING'); +const unembargo_in_progress = computed(() => currentDandiset.value?.dandiset.embargo_status === 'UNEMBARGOING'); const showWarningDialog = ref(false); const confirmationPhrase = ref(''); diff --git a/web/src/views/FileBrowserView/FileBrowser.vue b/web/src/views/FileBrowserView/FileBrowser.vue index 0a7c6a89e..55f19a143 100644 --- a/web/src/views/FileBrowserView/FileBrowser.vue +++ b/web/src/views/FileBrowserView/FileBrowser.vue @@ -1,5 +1,5 @@