From 5b92f2c374627da2ea6d05a4837e887969ffb7b1 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 10:57:42 -0500 Subject: [PATCH 001/131] Add AuditRecord model --- dandiapi/api/models/__init__.py | 2 ++ dandiapi/api/models/audit.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 dandiapi/api/models/audit.py diff --git a/dandiapi/api/models/__init__.py b/dandiapi/api/models/__init__.py index fdc3bd2b5..ff0c21c61 100644 --- a/dandiapi/api/models/__init__.py +++ b/dandiapi/api/models/__init__.py @@ -2,6 +2,7 @@ from .asset import Asset, AssetBlob, EmbargoedAssetBlob from .asset_paths import AssetPath, AssetPathRelation +from .audit import AuditRecord from .dandiset import Dandiset from .oauth import StagingApplication from .upload import EmbargoedUpload, Upload @@ -13,6 +14,7 @@ 'AssetBlob', 'AssetPath', 'AssetPathRelation', + 'AuditRecord', 'Dandiset', 'EmbargoedAssetBlob', 'EmbargoedUpload', diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py new file mode 100644 index 000000000..27bff46da --- /dev/null +++ b/dandiapi/api/models/audit.py @@ -0,0 +1,16 @@ +from __future__ import annotations + +from django.db import models + + +class AuditRecord(models.Model): + timestamp = models.DateTimeField(auto_now_add=True) + dandiset_id = models.IntegerField() + username = models.CharField(max_length=39) + user_email = models.CharField(max_length=254) + user_fullname = models.CharField(max_length=301) + record_type = models.CharField(max_length=32, choices=AUDIT_RECORD_CHOICES) + details = models.JSONField(blank=True) + + def __str__(self): + return f'{self.record_type}/{self.dandiset_id:06}' From 5ad7856924509e87b24726cb8761563e75883604 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 10:58:05 -0500 Subject: [PATCH 002/131] Add admin model for AuditRecord --- dandiapi/api/admin.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/dandiapi/api/admin.py b/dandiapi/api/admin.py index 06abb2e85..b0dc23a7f 100644 --- a/dandiapi/api/admin.py +++ b/dandiapi/api/admin.py @@ -19,6 +19,7 @@ from dandiapi.api.models import ( Asset, AssetBlob, + AuditRecord, Dandiset, EmbargoedAssetBlob, Upload, @@ -240,3 +241,39 @@ class AssetAdmin(admin.ModelAdmin): class UploadAdmin(admin.ModelAdmin): list_display = ['id', 'upload_id', 'blob', 'etag', 'upload_id', 'size', 'created'] list_display_links = ['id', 'upload_id'] + + +@admin.register(AuditRecord) +class AuditRecordAdmin(admin.ModelAdmin): + actions = None + date_hierarchy = 'timestamp' + search_fields = [ + 'dandiset_id', + 'username', + 'record_type', + ] + list_display = [ + 'id', + 'timestamp', + 'dandiset', + 'record_type', + 'details', + 'username', + ] + + @admin.display(description='Dandiset', ordering='dandiset_id') + def dandiset(self, obj): + return format_html( + '{}', + reverse('admin:api_dandiset_change', args=(obj.dandiset_id,)), + f'{obj.dandiset_id:06}', + ) + + def has_add_permission(self, request): + return False + + def has_change_permission(self, request, obj=None): + return False + + def has_delete_permission(self, request, obj=None): + return False From 31472ee2836c4c40566f69d956090c18a7b4f6b6 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:00:12 -0500 Subject: [PATCH 003/131] Generate create_dandiset audit record --- dandiapi/api/models/audit.py | 38 ++++++++++++++++++++++ dandiapi/api/services/dandiset/__init__.py | 6 ++++ 2 files changed, 44 insertions(+) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 27bff46da..039911fa2 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -1,7 +1,22 @@ from __future__ import annotations +from typing import TYPE_CHECKING, Literal, get_args + from django.db import models +if TYPE_CHECKING: + from django.contrib.auth.models import User + + from dandiapi.zarr.models import ZarrArchive + + from .asset import Asset + from .dandiset import Dandiset + +AuditRecordType = Literal[ + 'create_dandiset', +] +AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] + class AuditRecord(models.Model): timestamp = models.DateTimeField(auto_now_add=True) @@ -14,3 +29,26 @@ class AuditRecord(models.Model): def __str__(self): return f'{self.record_type}/{self.dandiset_id:06}' + + @staticmethod + def make_audit_record( + *, dandiset: Dandiset, user: User, record_type: AuditRecordType, details: dict + ) -> AuditRecord: + return AuditRecord( + dandiset_id=dandiset.id, + username=user.username, + user_email=user.email, + user_fullname=f'{user.first_name} {user.last_name}', + record_type=record_type, + details=details, + ) + + @staticmethod + def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed: bool): + details = { + 'metadata': metadata, + 'embargoed': embargoed, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='create_dandiset', details=details + ) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 0cf80d82c..9cc15d35d 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -2,6 +2,7 @@ from django.db import transaction +from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError @@ -45,6 +46,11 @@ def create_dandiset( draft_version.full_clean(validate_constraints=False) draft_version.save() + audit_record = AuditRecord.create_dandiset( + dandiset=dandiset, user=user, metadata=version_metadata, embargoed=embargo + ) + audit_record.save() + return dandiset, draft_version From eccedd9ee410d10bbf2b4017e0605ee4f0882e89 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:02:09 -0500 Subject: [PATCH 004/131] Generate change_owners audit record --- dandiapi/api/models/audit.py | 20 ++++++++++++++++++++ dandiapi/api/views/dandiset.py | 15 +++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 039911fa2..a790d280a 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -14,6 +14,7 @@ AuditRecordType = Literal[ 'create_dandiset', + 'change_owners', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -52,3 +53,22 @@ def create_dandiset(*, dandiset: Dandiset, user: User, metadata: dict, embargoed return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='create_dandiset', details=details ) + + @staticmethod + def change_owners( + *, dandiset: Dandiset, user: User, removed_owners: list[User], added_owners: list[User] + ): + def glean_user_info(user: User): + return { + 'username': user.username, + 'email': user.email, + 'name': f'{user.first_name} {user.last_name}', + } + + details = { + 'removed_owners': [glean_user_info(u) for u in removed_owners], + 'added_owners': [glean_user_info(u) for u in added_owners], + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='change_owners', details=details + ) diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 040395aef..1789ee7ad 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -4,6 +4,7 @@ from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User +from django.db import transaction from django.db.models import Count, Max, OuterRef, Subquery, Sum from django.db.models.functions import Coalesce from django.db.models.query_utils import Q @@ -23,7 +24,7 @@ from dandiapi.api.asset_paths import get_root_paths_many from dandiapi.api.mail import send_ownership_change_emails -from dandiapi.api.models import Dandiset, Version +from dandiapi.api.models import AuditRecord, Dandiset, Version from dandiapi.api.services.dandiset import create_dandiset, delete_dandiset from dandiapi.api.services.dandiset.exceptions import UnauthorizedEmbargoAccessError from dandiapi.api.services.embargo import unembargo_dandiset @@ -408,7 +409,17 @@ def users(self, request, dandiset__pk): # All owners found owners = user_owners + [acc.user for acc in socialaccount_owners] removed_owners, added_owners = dandiset.set_owners(owners) - dandiset.save() + with transaction.atomic(): + dandiset.save() + + if removed_owners or added_owners: + audit_record = AuditRecord.change_owners( + dandiset=dandiset, + user=request.user, + removed_owners=removed_owners, + added_owners=added_owners, + ) + audit_record.save() send_ownership_change_emails(dandiset, removed_owners, added_owners) From c50290bbd2dfb4cbc6bc845a8fec067facfaa70c Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:03:56 -0500 Subject: [PATCH 005/131] Generate update_metadata audit record --- dandiapi/api/models/audit.py | 8 ++++++++ dandiapi/api/views/version.py | 7 ++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index a790d280a..9fddf8cc5 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -15,6 +15,7 @@ AuditRecordType = Literal[ 'create_dandiset', 'change_owners', + 'update_metadata', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -72,3 +73,10 @@ def glean_user_info(user: User): return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='change_owners', details=details ) + + @staticmethod + def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditRecord: + details = {'metadata': metadata} + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='update_metadata', details=details + ) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index b94592414..65678e747 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -13,7 +13,7 @@ from rest_framework.viewsets import ReadOnlyModelViewSet from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin -from dandiapi.api.models import Dandiset, Version +from dandiapi.api.models import AuditRecord, Dandiset, Version from dandiapi.api.services.publish import publish_dandiset from dandiapi.api.tasks import delete_doi_task from dandiapi.api.views.common import DANDISET_PK_PARAM, VERSION_PARAM, DandiPagination @@ -116,6 +116,11 @@ def update(self, request, **kwargs): locked_version.status = Version.Status.PENDING locked_version.save() + audit_record = AuditRecord.update_metadata( + dandiset=locked_version.dandiset, user=request.user, metadata=new_metadata + ) + audit_record.save() + serializer = VersionDetailSerializer(instance=locked_version) return Response(serializer.data, status=status.HTTP_200_OK) From fdb49a5931e6c1f8c29029999f6a86cdabd5bac7 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:06:52 -0500 Subject: [PATCH 006/131] Generate [add|update|remove]_asset audit records --- dandiapi/api/models/audit.py | 45 +++++++++++++++++++++++++ dandiapi/api/services/asset/__init__.py | 22 ++++++++++-- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 9fddf8cc5..798255b51 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -16,6 +16,9 @@ 'create_dandiset', 'change_owners', 'update_metadata', + 'add_asset', + 'update_asset', + 'remove_asset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -80,3 +83,45 @@ def update_metadata(*, dandiset: Dandiset, user: User, metadata: dict) -> AuditR return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='update_metadata', details=details ) + + @staticmethod + def _asset_details(asset: Asset) -> dict: + checksum = ( + (asset.blob and asset.blob.etag) + or (asset.embargoed_blob and asset.embargoed_blob.etag) + or (asset.zarr and asset.zarr.checksum) + ) + + return { + 'path': asset.path, + 'asset_blob_id': asset.blob and asset.blob.id, + 'embargoed_asset_blob_id': asset.embargoed_blob and asset.embargoed_blob.id, + 'zarr_archive_id': asset.zarr and asset.zarr.id, + 'asset_id': asset.id, + 'checksum': checksum, + 'metadata': asset.metadata, + } + + @staticmethod + def add_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = AuditRecord._asset_details(asset) + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='add_asset', details=details + ) + + @staticmethod + def update_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = AuditRecord._asset_details(asset) + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='update_asset', details=details + ) + + @staticmethod + def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord: + details = { + 'path': asset.path, + 'asset_id': asset.id, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='remove_asset', details=details + ) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index 8cedcd739..23beadfd9 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -6,6 +6,7 @@ from dandiapi.api.asset_paths import add_asset_paths, delete_asset_paths, get_conflicting_paths from dandiapi.api.models.asset import Asset, AssetBlob, EmbargoedAssetBlob +from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.version import Version from dandiapi.api.services.asset.exceptions import ( AssetAlreadyExistsError, @@ -84,7 +85,7 @@ def change_asset( # noqa: PLR0913 raise AssetAlreadyExistsError with transaction.atomic(): - remove_asset_from_version(user=user, asset=asset, version=version) + remove_asset_from_version(user=user, asset=asset, version=version, audit=False) new_asset = add_asset_to_version( user=user, @@ -92,11 +93,15 @@ def change_asset( # noqa: PLR0913 asset_blob=new_asset_blob, zarr_archive=new_zarr_archive, metadata=new_metadata, + audit=False, ) # Set previous asset and save new_asset.previous = asset new_asset.save() + audit_record = AuditRecord.update_asset(dandiset=version.dandiset, user=user, asset=asset) + audit_record.save() + return new_asset, True @@ -107,6 +112,7 @@ def add_asset_to_version( asset_blob: AssetBlob | EmbargoedAssetBlob | None = None, zarr_archive: ZarrArchive | None = None, metadata: dict, + audit: bool = True, ) -> Asset: """Create an asset, adding it to a version.""" if not asset_blob and not zarr_archive: @@ -158,10 +164,16 @@ def add_asset_to_version( # Save the version so that the modified field is updated version.save() + if audit: + audit_record = AuditRecord.add_asset(dandiset=version.dandiset, user=user, asset=asset) + audit_record.save() + return asset -def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Version: +def remove_asset_from_version( + *, user, asset: Asset, version: Version, audit: bool = True +) -> Version: if not user.has_perm('owner', version.dandiset): raise DandisetOwnerRequiredError if version.version != 'draft': @@ -177,4 +189,10 @@ def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Versio # Save the version so that the modified field is updated version.save() + if audit: + audit_record = AuditRecord.remove_asset( + dandiset=version.dandiset, user=user, asset=asset + ) + audit_record.save() + return version From 1ad599d6a9a9103e69672923cacb89cd4673a393 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:09:52 -0500 Subject: [PATCH 007/131] Generate [create|upload|finalize|delete]_zarr audit records --- dandiapi/api/models/audit.py | 47 +++++++++++++++++++++++++++++++++ dandiapi/zarr/views/__init__.py | 30 ++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 798255b51..6ffcfd30f 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -19,6 +19,10 @@ 'add_asset', 'update_asset', 'remove_asset', + 'create_zarr', + 'upload_zarr', + 'delete_zarr_chunks', + 'finalize_zarr', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -125,3 +129,46 @@ def remove_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='remove_asset', details=details ) + + @staticmethod + def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'name': zarr_archive.name, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='create_zarr', details=details + ) + + @staticmethod + def upload_zarr( + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, urls: list[str] + ) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'num_urls': len(urls), + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='upload_zarr', details=details + ) + + @staticmethod + def delete_zarr_chunks( + *, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive, paths: list[str] + ) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + 'num_chunks': len(paths), + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='delete_zarr_chunks', details=details + ) + + @staticmethod + def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> AuditRecord: + details = { + 'zarr_id': zarr_archive.id, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='finalize_zarr', details=details + ) diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index 61a1057c6..19e34f58c 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -14,6 +14,7 @@ from rest_framework.utils.urls import replace_query_param from rest_framework.viewsets import ReadOnlyModelViewSet +from dandiapi.api.models.audit import AuditRecord from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.storage import get_boto_client from dandiapi.api.views.common import DandiPagination @@ -139,7 +140,13 @@ def create(self, request): raise ValidationError('Cannot add zarr to embargoed dandiset') zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset) try: - zarr_archive.save() + with transaction.atomic(): + zarr_archive.save() + + audit_record = AuditRecord.create_zarr( + dandiset=dandiset, user=request.user, zarr_archive=zarr_archive + ) + audit_record.save() except IntegrityError as e: raise ValidationError('Zarr already exists') from e @@ -175,6 +182,11 @@ def finalize(self, request, zarr_id): zarr_archive.status = ZarrArchiveStatus.UPLOADED zarr_archive.save() + audit_record = AuditRecord.finalize_zarr( + dandiset=zarr_archive.dandiset, user=request.user, zarr_archive=zarr_archive + ) + audit_record.save() + # Dispatch task ingest_zarr_archive.delay(zarr_id=zarr_archive.zarr_id) return Response(None, status=status.HTTP_204_NO_CONTENT) @@ -289,6 +301,14 @@ def create_files(self, request, zarr_id): zarr_archive.mark_pending() zarr_archive.save() + audit_record = AuditRecord.upload_zarr( + dandiset=zarr_archive.dandiset, + user=request.user, + zarr_archive=zarr_archive, + urls=urls, + ) + audit_record.save() + # Return presigned urls logger.info( 'Presigned %d URLs to upload to zarr archive %s', len(urls), zarr_archive.zarr_id @@ -320,4 +340,12 @@ def delete_files(self, request, zarr_id): paths = [file['path'] for file in serializer.validated_data] zarr_archive.delete_files(paths) + audit_record = AuditRecord.delete_zarr_chunks( + dandiset=zarr_archive.dandiset, + user=request.user, + zarr_archive=zarr_archive, + paths=paths, + ) + audit_record.save() + return Response(None, status=status.HTTP_204_NO_CONTENT) From 7f0fc968c8fe3a44dc447275d169a4d63c4fda36 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:12:19 -0500 Subject: [PATCH 008/131] Generate unembargo_dandiset audit record --- dandiapi/api/models/audit.py | 7 +++++++ dandiapi/api/services/embargo/__init__.py | 9 ++++++--- dandiapi/api/tasks/__init__.py | 6 ++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 6ffcfd30f..0d2694d29 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -23,6 +23,7 @@ 'upload_zarr', 'delete_zarr_chunks', 'finalize_zarr', + 'unembargo_dandiset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -172,3 +173,9 @@ def finalize_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='finalize_zarr', details=details ) + + @staticmethod + def unembargo_dandiset(*, dandiset: Dandiset, user: User) -> AuditRecord: + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='unembargo_dandiset', details={} + ) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 9b3e4425b..0f3bb854f 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -5,7 +5,7 @@ from django.conf import settings from dandiapi.api.copy import copy_object_multipart -from dandiapi.api.models import Asset, AssetBlob, Dandiset, Upload, Version +from dandiapi.api.models import Asset, AssetBlob, AuditRecord, Dandiset, Upload, Version from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.tasks import unembargo_dandiset_task @@ -56,7 +56,7 @@ def _unembargo_asset(asset: Asset): asset.save() -def _unembargo_dandiset(dandiset: Dandiset): +def _unembargo_dandiset(dandiset: Dandiset, user: User): draft_version: Version = dandiset.draft_version embargoed_assets: QuerySet[Asset] = draft_version.assets.filter(embargoed_blob__isnull=False) @@ -74,6 +74,9 @@ def _unembargo_dandiset(dandiset: Dandiset): dandiset.embargo_status = Dandiset.EmbargoStatus.OPEN dandiset.save() + audit_record = AuditRecord.unembargo_dandiset(dandiset=dandiset, user=user) + audit_record.save() + def unembargo_dandiset(*, user: User, dandiset: Dandiset): """Unembargo a dandiset by copying all embargoed asset blobs to the public bucket.""" @@ -83,4 +86,4 @@ def unembargo_dandiset(*, user: User, dandiset: Dandiset): if not user.has_perm('owner', dandiset): raise DandisetOwnerRequiredError - unembargo_dandiset_task.delay(dandiset.id) + unembargo_dandiset_task.delay(dandiset.id, user.id) diff --git a/dandiapi/api/tasks/__init__.py b/dandiapi/api/tasks/__init__.py index 80f8ef87c..817f73e99 100644 --- a/dandiapi/api/tasks/__init__.py +++ b/dandiapi/api/tasks/__init__.py @@ -2,6 +2,7 @@ from celery import shared_task from celery.utils.log import get_task_logger +from django.contrib.auth.models import User from dandiapi.api.doi import delete_doi from dandiapi.api.manifests import ( @@ -68,11 +69,12 @@ def delete_doi_task(doi: str) -> None: @shared_task -def unembargo_dandiset_task(dandiset_id: int): +def unembargo_dandiset_task(dandiset_id: int, user_id: int): from dandiapi.api.services.embargo import _unembargo_dandiset dandiset = Dandiset.objects.get(id=dandiset_id) - _unembargo_dandiset(dandiset) + user = User.objects.get(id=user_id) + _unembargo_dandiset(dandiset, user) @shared_task From 2c9c6b38eddd4f470f0089d5b84b7a0b211d268c Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:17:23 -0500 Subject: [PATCH 009/131] Generate publish_dandiset audit record --- dandiapi/api/models/audit.py | 10 ++++++++++ dandiapi/api/services/publish/__init__.py | 14 ++++++++++---- dandiapi/api/tasks/__init__.py | 4 ++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 0d2694d29..848cbfa0b 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -24,6 +24,7 @@ 'delete_zarr_chunks', 'finalize_zarr', 'unembargo_dandiset', + 'publish_dandiset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -179,3 +180,12 @@ def unembargo_dandiset(*, dandiset: Dandiset, user: User) -> AuditRecord: return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='unembargo_dandiset', details={} ) + + @staticmethod + def publish_dandiset(*, dandiset: Dandiset, user: User, version: str) -> AuditRecord: + details = { + 'version': version, + } + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='publish_dandiset', details=details + ) diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index a41ead487..9b4938a45 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -4,12 +4,13 @@ from typing import TYPE_CHECKING from dandischema.metadata import aggregate_assets_summary, validate +from django.contrib.auth.models import User from django.db import transaction from more_itertools import ichunked from dandiapi.api import doi from dandiapi.api.asset_paths import add_version_asset_paths -from dandiapi.api.models import Asset, Dandiset, Version +from dandiapi.api.models import Asset, AuditRecord, Dandiset, Version from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.publish.exceptions import ( DandisetAlreadyPublishedError, @@ -22,7 +23,6 @@ from dandiapi.api.tasks import write_manifest_files if TYPE_CHECKING: - from django.contrib.auth.models import User from django.db.models import QuerySet @@ -103,7 +103,7 @@ def _build_publishable_version_from_draft(draft_version: Version) -> Version: return publishable_version -def _publish_dandiset(dandiset_id: int) -> None: +def _publish_dandiset(dandiset_id: int, user_id: int) -> None: """ Publish a dandiset. @@ -189,10 +189,16 @@ def _create_doi(version_id: int): transaction.on_commit(lambda: _create_doi(new_version.id)) + user = User.objects.get(id=user_id) + audit_record = AuditRecord.publish_dandiset( + dandiset=new_version.dandiset, user=user, version=new_version.version + ) + audit_record.save() + def publish_dandiset(*, user: User, dandiset: Dandiset) -> None: from dandiapi.api.tasks import publish_dandiset_task with transaction.atomic(): _lock_dandiset_for_publishing(user=user, dandiset=dandiset) - transaction.on_commit(lambda: publish_dandiset_task.delay(dandiset.id)) + transaction.on_commit(lambda: publish_dandiset_task.delay(dandiset.id, user.id)) diff --git a/dandiapi/api/tasks/__init__.py b/dandiapi/api/tasks/__init__.py index 817f73e99..31cecc310 100644 --- a/dandiapi/api/tasks/__init__.py +++ b/dandiapi/api/tasks/__init__.py @@ -78,7 +78,7 @@ def unembargo_dandiset_task(dandiset_id: int, user_id: int): @shared_task -def publish_dandiset_task(dandiset_id: int): +def publish_dandiset_task(dandiset_id: int, user_id: int): from dandiapi.api.services.publish import _publish_dandiset - _publish_dandiset(dandiset_id=dandiset_id) + _publish_dandiset(dandiset_id=dandiset_id, user_id=user_id) From 01f1b85efe13f3b8418359addf3c067ed5e96710 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:19:55 -0500 Subject: [PATCH 010/131] Generate delete_dandiset audit record --- dandiapi/api/models/audit.py | 7 +++++++ dandiapi/api/services/dandiset/__init__.py | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/dandiapi/api/models/audit.py b/dandiapi/api/models/audit.py index 848cbfa0b..03f6d479b 100644 --- a/dandiapi/api/models/audit.py +++ b/dandiapi/api/models/audit.py @@ -25,6 +25,7 @@ 'finalize_zarr', 'unembargo_dandiset', 'publish_dandiset', + 'delete_dandiset', ] AUDIT_RECORD_CHOICES = [(t, t) for t in get_args(AuditRecordType)] @@ -189,3 +190,9 @@ def publish_dandiset(*, dandiset: Dandiset, user: User, version: str) -> AuditRe return AuditRecord.make_audit_record( dandiset=dandiset, user=user, record_type='publish_dandiset', details=details ) + + @staticmethod + def delete_dandiset(*, dandiset: Dandiset, user: User): + return AuditRecord.make_audit_record( + dandiset=dandiset, user=user, record_type='delete_dandiset', details={} + ) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 9cc15d35d..3b28172ac 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -65,5 +65,10 @@ def delete_dandiset(*, user, dandiset: Dandiset) -> None: # Delete all versions first, so that AssetPath deletion is cascaded # through versions, rather than through zarrs directly with transaction.atomic(): + # Record the audit event first so that the AuditRecord instance has a + # chance to grab the Dandiset information before it is destroyed. + audit_record = AuditRecord.delete_dandiset(dandiset=dandiset, user=user) + audit_record.save() + dandiset.versions.all().delete() dandiset.delete() From 52ff47ebcb752f3c605deec06c5179fe6838b8b2 Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 11:20:10 -0500 Subject: [PATCH 011/131] Add migration for AuditRecord --- dandiapi/api/migrations/0006_auditrecord.py | 51 +++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 dandiapi/api/migrations/0006_auditrecord.py diff --git a/dandiapi/api/migrations/0006_auditrecord.py b/dandiapi/api/migrations/0006_auditrecord.py new file mode 100644 index 000000000..4d64a0bff --- /dev/null +++ b/dandiapi/api/migrations/0006_auditrecord.py @@ -0,0 +1,51 @@ +# Generated by Django 4.1.13 on 2024-03-06 15:39 +from __future__ import annotations + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('api', '0005_null_charfield'), + ] + + operations = [ + migrations.CreateModel( + name='AuditRecord', + fields=[ + ( + 'id', + models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID' + ), + ), + ('timestamp', models.DateTimeField(auto_now_add=True)), + ('dandiset_id', models.IntegerField()), + ('username', models.CharField(max_length=39)), + ('user_email', models.CharField(max_length=254)), + ('user_fullname', models.CharField(max_length=301)), + ( + 'record_type', + models.CharField( + choices=[ + ('create_dandiset', 'create_dandiset'), + ('change_owners', 'change_owners'), + ('update_metadata', 'update_metadata'), + ('add_asset', 'add_asset'), + ('update_asset', 'update_asset'), + ('remove_asset', 'remove_asset'), + ('create_zarr', 'create_zarr'), + ('upload_zarr', 'upload_zarr'), + ('delete_zarr_chunks', 'delete_zarr_chunks'), + ('finalize_zarr', 'finalize_zarr'), + ('unembargo_dandiset', 'unembargo_dandiset'), + ('publish_dandiset', 'publish_dandiset'), + ('delete_dandiset', 'delete_dandiset'), + ], + max_length=32, + ), + ), + ('details', models.JSONField(blank=True)), + ], + ), + ] From 08b26c61b20810446e8067bdbed8fc14edad27bb Mon Sep 17 00:00:00 2001 From: Roni Choudhury Date: Wed, 6 Mar 2024 14:27:13 -0500 Subject: [PATCH 012/131] Fix tests broken by signature changes --- dandiapi/api/tests/test_asset_paths.py | 6 ++++-- dandiapi/api/tests/test_tasks.py | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dandiapi/api/tests/test_asset_paths.py b/dandiapi/api/tests/test_asset_paths.py index 3d8f5b1ff..92c5430e5 100644 --- a/dandiapi/api/tests/test_asset_paths.py +++ b/dandiapi/api/tests/test_asset_paths.py @@ -295,7 +295,7 @@ def test_asset_path_search_asset_paths(draft_version_factory, asset_factory): @pytest.mark.django_db() -def test_asset_path_publish_version(draft_version_factory, asset_factory): +def test_asset_path_publish_version(draft_version_factory, asset_factory, user_factory): version: Version = draft_version_factory() asset = asset_factory(path='foo/bar.txt', status=Asset.Status.VALID) version.assets.add(asset) @@ -308,8 +308,10 @@ def test_asset_path_publish_version(draft_version_factory, asset_factory): version.status = Version.Status.PUBLISHING version.save() + user = user_factory() + # Publish - publish_dandiset_task(version.dandiset.id) + publish_dandiset_task(version.dandiset.id, user.id) # Get published version published_version = ( diff --git a/dandiapi/api/tests/test_tasks.py b/dandiapi/api/tests/test_tasks.py index bfffb13f0..e299486d7 100644 --- a/dandiapi/api/tests/test_tasks.py +++ b/dandiapi/api/tests/test_tasks.py @@ -305,6 +305,7 @@ def test_publish_task( draft_asset_factory, published_asset_factory, draft_version_factory, + user_factory, django_capture_on_commit_callbacks, ): # Create a draft_version in PUBLISHING state @@ -323,8 +324,10 @@ def test_publish_task( # Ensure that the number of versions increases by 1 after publishing starting_version_count = draft_version.dandiset.versions.count() + user = user_factory() + with django_capture_on_commit_callbacks(execute=True): - tasks.publish_dandiset_task(draft_version.dandiset.id) + tasks.publish_dandiset_task(draft_version.dandiset.id, user.id) assert draft_version.dandiset.versions.count() == starting_version_count + 1 From 2f563f9ac28eac9d52038e6138bdee46ae1e8a87 Mon Sep 17 00:00:00 2001 From: Ben Dichter Date: Fri, 24 May 2024 14:09:09 -0400 Subject: [PATCH 013/131] add handbook to welcome email --- dandiapi/api/templates/api/mail/registered_message.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/dandiapi/api/templates/api/mail/registered_message.txt b/dandiapi/api/templates/api/mail/registered_message.txt index aa994d55e..b35668b89 100644 --- a/dandiapi/api/templates/api/mail/registered_message.txt +++ b/dandiapi/api/templates/api/mail/registered_message.txt @@ -14,6 +14,7 @@ registered with our Slack workspace. Please use the following links to post any questions or issues. +DANDI Handbook: https://www.dandiarchive.org/handbook Discussions: https://github.com/dandi/helpdesk/discussions Issues: https://github.com/dandi/helpdesk/issues/new/choose YouTube: https://www.youtube.com/@dandiarchive From d25837b6de1ba0c399ccabee5501ff0f6cda0551 Mon Sep 17 00:00:00 2001 From: Ben Dichter Date: Fri, 24 May 2024 14:10:12 -0400 Subject: [PATCH 014/131] Update approved_user_message.txt --- dandiapi/api/templates/api/mail/approved_user_message.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/dandiapi/api/templates/api/mail/approved_user_message.txt b/dandiapi/api/templates/api/mail/approved_user_message.txt index 7b85297ee..4c1f73de4 100644 --- a/dandiapi/api/templates/api/mail/approved_user_message.txt +++ b/dandiapi/api/templates/api/mail/approved_user_message.txt @@ -6,6 +6,7 @@ to start creating dandisets and uploading data. Please use the following links to post any questions or issues. +DANDI Handbook: https://www.dandiarchive.org/handbook Discussions: https://github.com/dandi/helpdesk/discussions Issues: https://github.com/dandi/helpdesk/issues/new/choose YouTube: https://www.youtube.com/@dandiarchive From 0b346957702df2aa6cef1d96db00cf82baa93c76 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Fri, 24 May 2024 14:47:01 -0400 Subject: [PATCH 015/131] In 1.14.3 it became client_config and .config was announced deprecated, caused it to become None and I believe that caused our tests to start failing etc --- dandiapi/api/storage.py | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/storage.py b/dandiapi/api/storage.py index dcaa517ea..0fa1e1a34 100644 --- a/dandiapi/api/storage.py +++ b/dandiapi/api/storage.py @@ -155,7 +155,7 @@ class TimeoutS3Storage(S3Storage): def __init__(self, **settings): super().__init__(**settings) - self.config = self.config.merge( + self.client_config = self.client_config.merge( Config(connect_timeout=5, read_timeout=5, retries={'max_attempts': 2}) ) diff --git a/setup.py b/setup.py index eeab69fe0..15738bcab 100644 --- a/setup.py +++ b/setup.py @@ -67,7 +67,7 @@ # Production-only 'django-composed-configuration[prod]>=0.23.0', 'django-s3-file-field[s3]>=1.0.0', - 'django-storages[s3]>=1.14.2', + 'django-storages[s3]>=1.14.3', 'gunicorn', # Development-only, but required 'django-minio-storage', From 68904eebba631e9e2a0046c8975b15ec07a4d960 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 28 May 2024 14:58:23 -0400 Subject: [PATCH 016/131] Use normal PageNumber pagination for listing endpoints This retains the behavior of using a custom pagination class for the asset list endpoint --- dandiapi/api/tests/test_pagination.py | 86 --------------------------- dandiapi/api/views/asset.py | 9 ++- dandiapi/api/views/pagination.py | 31 ++++++---- 3 files changed, 24 insertions(+), 102 deletions(-) diff --git a/dandiapi/api/tests/test_pagination.py b/dandiapi/api/tests/test_pagination.py index bc890be81..2081fc03b 100644 --- a/dandiapi/api/tests/test_pagination.py +++ b/dandiapi/api/tests/test_pagination.py @@ -3,65 +3,6 @@ import pytest -@pytest.mark.django_db() -def test_dandiset_pagination(api_client, dandiset_factory): - endpoint = '/api/dandisets/' - for _ in range(10): - dandiset_factory() - - # First page - resp = api_client.get(endpoint, {'order': 'id', 'page_size': 5}).json() - assert resp['count'] == 10 - assert resp['next'] is not None - page_one = resp['results'] - assert len(page_one) == 5 - - # Second page - resp = api_client.get(endpoint, {'order': 'id', 'page_size': 5, 'page': 2}).json() - assert resp['count'] is None - assert resp['next'] is None - page_two = resp['results'] - assert len(page_two) == 5 - - # Full page - resp = api_client.get(endpoint, {'order': 'id', 'page_size': 100}).json() - assert resp['count'] == 10 - assert resp['next'] is None - full_page = resp['results'] - assert len(full_page) == 10 - - # Assert full list is ordered the same as both paginated lists - assert full_page == page_one + page_two - - -@pytest.mark.django_db() -def test_version_pagination(api_client, dandiset, published_version_factory): - endpoint = f'/api/dandisets/{dandiset.identifier}/versions/' - - for _ in range(10): - published_version_factory(dandiset=dandiset) - - resp = api_client.get(endpoint, {'order': 'created', 'page_size': 5}).json() - - assert resp['count'] == 10 - page_one = resp['results'] - assert len(page_one) == 5 - - resp = api_client.get(endpoint, {'order': 'created', 'page_size': 5, 'page': 2}).json() - assert resp['count'] is None - assert resp['next'] is None - page_two = resp['results'] - assert len(page_two) == 5 - - resp = api_client.get(endpoint, {'order': 'created', 'page_size': 100}).json() - assert resp['count'] == 10 - assert resp['next'] is None - full_page = resp['results'] - assert len(full_page) == 10 - - assert full_page == page_one + page_two - - @pytest.mark.django_db() def test_asset_pagination(api_client, version, asset_factory): endpoint = f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/assets/' @@ -92,30 +33,3 @@ def test_asset_pagination(api_client, version, asset_factory): # Assert full list is ordered the same as both paginated lists assert full_page == page_one + page_two - - -@pytest.mark.django_db() -def test_zarr_pagination(api_client, zarr_archive_factory): - endpoint = '/api/zarr/' - - for _ in range(10): - zarr_archive_factory() - - resp = api_client.get(endpoint, {'page_size': 5}).json() - assert resp['count'] == 10 - page_one = resp['results'] - assert len(page_one) == 5 - - resp = api_client.get(endpoint, {'page_size': 5, 'page': 2}).json() - assert resp['count'] is None - assert resp['next'] is None - page_two = resp['results'] - assert len(page_two) == 5 - - resp = api_client.get(endpoint, {'page_size': 100}).json() - assert resp['count'] == 10 - assert resp['next'] is None - full_page = resp['results'] - assert len(full_page) == 10 - - assert full_page == page_one + page_two diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 4595b9ebd..4e86cf273 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -46,7 +46,7 @@ VERSIONS_DANDISET_PK_PARAM, VERSIONS_VERSION_PARAM, ) -from dandiapi.api.views.pagination import DandiPagination +from dandiapi.api.views.pagination import DandiPagination, LazyPagination from dandiapi.api.views.serializers import ( AssetDetailSerializer, AssetDownloadQueryParameterSerializer, @@ -431,7 +431,10 @@ def list(self, request, *args, **kwargs): asset_queryset = asset_queryset.filter(path__iregex=glob_pattern.replace('\\*', '.*')) # Retrieve just the first N asset IDs, and use them for pagination - page_of_asset_ids = self.paginate_queryset(asset_queryset.values_list('id', flat=True)) + # Use custom pagination class to reduce unnecessary counts of assets + paginator = LazyPagination() + qs = asset_queryset.values_list('id', flat=True) + page_of_asset_ids = paginator.paginate_queryset(qs, request=self.request, view=self) # Not sure when the page is ever None, but this condition is checked for compatibility with # the original implementation: https://github.com/encode/django-rest-framework/blob/f4194c4684420ac86485d9610adf760064db381f/rest_framework/mixins.py#L37-L46 @@ -453,7 +456,7 @@ def list(self, request, *args, **kwargs): # Paginate and return serializer = self.get_serializer(queryset, many=True, metadata=include_metadata) - return self.get_paginated_response(serializer.data) + return paginator.get_paginated_response(serializer.data) @swagger_auto_schema( query_serializer=AssetPathsQueryParameterSerializer, diff --git a/dandiapi/api/views/pagination.py b/dandiapi/api/views/pagination.py index c48b76f72..d2efaffbe 100644 --- a/dandiapi/api/views/pagination.py +++ b/dandiapi/api/views/pagination.py @@ -1,12 +1,3 @@ -""" -Implement an optimized pagination scheme. - -This module provides a custom pagination implementation, as the existing `PageNumberPagination` -class returns a `count` field for every page returned. This can be very inefficient on large tables, -and in reality, the count is only necessary on the first page of results. This module implements -such a pagination scheme, only returning 'count' on the first page of results. -""" - from __future__ import annotations from collections import OrderedDict @@ -17,6 +8,24 @@ class returns a `count` field for every page returned. This can be very ineffici from rest_framework.response import Response +class DandiPagination(PageNumberPagination): + page_size = 100 + max_page_size = 1000 + page_size_query_param = 'page_size' + + @cached_property + def page_size_query_description(self): + return f'{super().page_size_query_description[:-1]} (maximum {self.max_page_size}).' + + +""" +The below code provides a custom pagination implementation, as the existing `PageNumberPagination` +class returns a `count` field for every page returned. This can be very inefficient on large tables, +and in reality, the count is only necessary on the first page of results. This module implements +such a pagination scheme, only returning 'count' on the first page of results. +""" + + class LazyPage(Page): """ A page class that doesn't call .count() on the queryset it's paging from. @@ -96,7 +105,3 @@ def get_paginated_response(self, data) -> Response: ) return Response(page_dict) - - -# Alias -DandiPagination = LazyPagination From d862e6dd2a5f9909756395e19fa2c7348eda0ebd Mon Sep 17 00:00:00 2001 From: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com> Date: Thu, 9 May 2024 12:56:27 -0400 Subject: [PATCH 017/131] Fix race condition in sha256 calculation task --- dandiapi/api/tasks/__init__.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dandiapi/api/tasks/__init__.py b/dandiapi/api/tasks/__init__.py index eb3a6ef99..b06a87b2c 100644 --- a/dandiapi/api/tasks/__init__.py +++ b/dandiapi/api/tasks/__init__.py @@ -32,8 +32,7 @@ def calculate_sha256(blob_id: str) -> None: # TODO: Run dandi-cli validation - asset_blob.sha256 = sha256 - asset_blob.save() + AssetBlob.objects.filter(blob_id=blob_id).update(sha256=sha256) @shared_task(soft_time_limit=180) From 268eabac01da5ad48c5e9c9895e882b41057bb8e Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 3 Jun 2024 17:17:54 -0400 Subject: [PATCH 018/131] Empty commit to cut a release From 94185a031aa96c2f00f8e471c8759a360767c7be Mon Sep 17 00:00:00 2001 From: dandibot Date: Mon, 3 Jun 2024 21:21:29 +0000 Subject: [PATCH 019/131] auto shipit - CHANGELOG.md etc --- CHANGELOG.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c18591607..413e6f55a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,31 @@ +# v0.3.85 (Mon Jun 03 2024) + +:tada: This release contains work from a new contributor! :tada: + +Thank you, Ben Dichter ([@bendichter](https://github.com/bendichter)), for all your work! + +#### 🐛 Bug Fix + +- Empty PR to trigger a release [#1951](https://github.com/dandi/dandi-archive/pull/1951) ([@jjnesbitt](https://github.com/jjnesbitt)) +- Only use custom pagination class for asset list endpoint [#1947](https://github.com/dandi/dandi-archive/pull/1947) ([@jjnesbitt](https://github.com/jjnesbitt)) +- In 1.14.3 it became client_config and .config was announced deprecated [#1946](https://github.com/dandi/dandi-archive/pull/1946) ([@yarikoptic](https://github.com/yarikoptic)) +- neurosift external service for .nwb.lindi.json [#1922](https://github.com/dandi/dandi-archive/pull/1922) ([@magland](https://github.com/magland)) +- Fix documentation for server downtime message var [#1927](https://github.com/dandi/dandi-archive/pull/1927) ([@jjnesbitt](https://github.com/jjnesbitt)) +- Revert "Add `workflow_dispatch` trigger to staging deploy workflow" [#1930](https://github.com/dandi/dandi-archive/pull/1930) ([@jjnesbitt](https://github.com/jjnesbitt)) + +#### 📝 Documentation + +- add handbook to welcome email [#1945](https://github.com/dandi/dandi-archive/pull/1945) ([@bendichter](https://github.com/bendichter)) + +#### Authors: 4 + +- Ben Dichter ([@bendichter](https://github.com/bendichter)) +- Jacob Nesbitt ([@jjnesbitt](https://github.com/jjnesbitt)) +- Jeremy Magland ([@magland](https://github.com/magland)) +- Yaroslav Halchenko ([@yarikoptic](https://github.com/yarikoptic)) + +--- + # v0.3.84 (Mon Apr 29 2024) #### 🐛 Bug Fix From c832cd799debd7bf2cfc26ff0fa9bec72d08af63 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 6 Jun 2024 16:24:29 -0400 Subject: [PATCH 020/131] Validate updates to access metadata --- dandiapi/api/models/dandiset.py | 4 + dandiapi/api/models/version.py | 24 ++++++ dandiapi/api/services/embargo/__init__.py | 6 -- dandiapi/api/services/version/metadata.py | 7 -- dandiapi/api/tests/factories.py | 9 +++ dandiapi/api/tests/test_version.py | 92 +++++++++++++++++++++++ 6 files changed, 129 insertions(+), 13 deletions(-) diff --git a/dandiapi/api/models/dandiset.py b/dandiapi/api/models/dandiset.py index 91267427b..595467dc7 100644 --- a/dandiapi/api/models/dandiset.py +++ b/dandiapi/api/models/dandiset.py @@ -58,6 +58,10 @@ def identifier(self) -> str: # Compare against None, to allow id 0 return f'{self.id:06}' if self.id is not None else '' + @property + def embargoed(self) -> bool: + return self.embargo_status == self.EmbargoStatus.EMBARGOED + @property def most_recent_published_version(self): return self.versions.exclude(version='draft').order_by('modified').last() diff --git a/dandiapi/api/models/version.py b/dandiapi/api/models/version.py index d52cba3be..6007e732a 100644 --- a/dandiapi/api/models/version.py +++ b/dandiapi/api/models/version.py @@ -3,6 +3,7 @@ import datetime import logging +from dandischema.models import AccessType from django.conf import settings from django.contrib.postgres.indexes import HashIndex from django.core.validators import RegexValidator @@ -167,6 +168,28 @@ def strip_metadata(cls, metadata): ] return {key: metadata[key] for key in metadata if key not in computed_fields} + def _populate_access_metadata(self): + default_access = [{}] + access = self.metadata.get('access', default_access) + + # Ensure access is a non-empty list + if not (isinstance(access, list) and access): + access = default_access + + # Ensure that every item in access is a dict + access = [x for x in access if isinstance(x, dict)] or default_access + + # Set first access item + access[0] = { + **access[0], + 'schemaKey': 'AccessRequirements', + 'status': AccessType.EmbargoedAccess.value + if self.dandiset.embargoed + else AccessType.OpenAccess.value, + } + + return access + def _populate_metadata(self): from dandiapi.api.manifests import manifest_location @@ -187,6 +210,7 @@ def _populate_metadata(self): f'{self.dandiset.identifier}/{self.version}' ), 'dateCreated': self.dandiset.created.isoformat(), + 'access': self._populate_access_metadata(), } if 'assetsSummary' not in metadata: diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index dd07e04da..bd022b762 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -48,12 +48,6 @@ def _unembargo_dandiset(dandiset: Dandiset): embargoed_assets: QuerySet[Asset] = draft_version.assets.filter(blob__embargoed=True) AssetBlob.objects.filter(assets__in=embargoed_assets).update(embargoed=False) - # Update draft version metadata - draft_version.metadata['access'] = [ - {'schemaKey': 'AccessRequirements', 'status': 'dandi:OpenAccess'} - ] - draft_version.save() - # Set access on dandiset dandiset.embargo_status = Dandiset.EmbargoStatus.OPEN dandiset.save() diff --git a/dandiapi/api/services/version/metadata.py b/dandiapi/api/services/version/metadata.py index bc04c8114..bd82bcc40 100644 --- a/dandiapi/api/services/version/metadata.py +++ b/dandiapi/api/services/version/metadata.py @@ -32,13 +32,6 @@ def _normalize_version_metadata( 'includeInCitation': True, }, ], - # TODO: move this into dandischema - 'access': [ - { - 'schemaKey': 'AccessRequirements', - 'status': 'dandi:EmbargoedAccess' if embargo else 'dandi:OpenAccess', - } - ], **version_metadata, } # Run the version_metadata through the pydantic model to automatically include any boilerplate diff --git a/dandiapi/api/tests/factories.py b/dandiapi/api/tests/factories.py index a32573e6e..ac0998590 100644 --- a/dandiapi/api/tests/factories.py +++ b/dandiapi/api/tests/factories.py @@ -5,6 +5,7 @@ from allauth.socialaccount.models import SocialAccount from dandischema.digests.dandietag import DandiETag +from dandischema.models import AccessType from django.conf import settings from django.contrib.auth.models import User from django.core import files as django_files @@ -95,6 +96,14 @@ def metadata(self): 'schemaVersion': settings.DANDI_SCHEMA_VERSION, 'schemaKey': 'Dandiset', 'description': faker.Faker().sentence(), + 'access': [ + { + 'schemaKey': 'AccessRequirements', + 'status': AccessType.EmbargoedAccess.value + if self.dandiset.embargoed + else AccessType.OpenAccess.value, + } + ], 'contributor': [ { 'name': f'{faker.Faker().last_name()}, {faker.Faker().first_name()}', diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index 9c530999d..2879de7c2 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -4,6 +4,7 @@ from time import sleep from typing import TYPE_CHECKING +from dandischema.models import AccessType from django.conf import settings from freezegun import freeze_time from guardian.shortcuts import assign_perm @@ -84,6 +85,7 @@ def test_draft_version_metadata_computed(draft_version: Version): ), 'repository': settings.DANDI_WEB_APP_URL, 'dateCreated': draft_version.dandiset.created.isoformat(), + 'access': [{'schemaKey': 'AccessRequirements', 'status': AccessType.OpenAccess.value}], '@context': f'https://raw.githubusercontent.com/dandi/schema/master/releases/{settings.DANDI_SCHEMA_VERSION}/context.json', 'assetsSummary': { 'numberOfBytes': 0, @@ -127,6 +129,7 @@ def test_published_version_metadata_computed(published_version: Version): ), 'repository': settings.DANDI_WEB_APP_URL, 'dateCreated': published_version.dandiset.created.isoformat(), + 'access': [{'schemaKey': 'AccessRequirements', 'status': AccessType.OpenAccess.value}], '@context': f'https://raw.githubusercontent.com/dandi/schema/master/releases/{settings.DANDI_SCHEMA_VERSION}/context.json', 'assetsSummary': { 'numberOfBytes': 0, @@ -556,6 +559,7 @@ def test_version_rest_update(api_client, user, draft_version): 'url': url, 'repository': settings.DANDI_WEB_APP_URL, 'dateCreated': UTC_ISO_TIMESTAMP_RE, + 'access': [{'schemaKey': 'AccessRequirements', 'status': AccessType.OpenAccess.value}], 'citation': f'{new_name} ({year}). (Version draft) [Data set]. DANDI archive. {url}', 'assetsSummary': { 'numberOfBytes': 0, @@ -635,6 +639,94 @@ def test_version_rest_update_not_an_owner(api_client, user, version): ) +@pytest.mark.parametrize( + ('access'), + [ + 'some value', + 123, + None, + [], + ['a', 'b'], + ['a', 'b', {}], + [{'schemaKey': 'AccessRequirements', 'status': 'foobar'}], + ], +) +@pytest.mark.django_db() +def test_version_rest_update_access_values(api_client, user, draft_version, access): + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + new_metadata = {**draft_version.metadata, 'access': access} + resp = api_client.put( + f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/', + {'metadata': new_metadata, 'name': draft_version.name}, + format='json', + ) + assert resp.status_code == 200 + draft_version.refresh_from_db() + + access = draft_version.metadata['access'] + assert access != new_metadata['access'] + assert access[0]['schemaKey'] == 'AccessRequirements' + assert ( + access[0]['status'] == AccessType.EmbargoedAccess.value + if draft_version.dandiset.embargoed + else AccessType.OpenAccess.value + ) + + +@pytest.mark.django_db() +def test_version_rest_update_access_missing(api_client, user, draft_version): + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + # Check that the field missing entirely is also okay + new_metadata = {**draft_version.metadata} + new_metadata.pop('access', None) + + resp = api_client.put( + f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/', + {'metadata': new_metadata, 'name': draft_version.name}, + format='json', + ) + assert resp.status_code == 200 + draft_version.refresh_from_db() + assert 'access' in draft_version.metadata + access = draft_version.metadata['access'] + assert access[0]['schemaKey'] == 'AccessRequirements' + assert ( + access[0]['status'] == AccessType.EmbargoedAccess.value + if draft_version.dandiset.embargoed + else AccessType.OpenAccess.value + ) + + +@pytest.mark.django_db() +def test_version_rest_update_access_valid(api_client, user, draft_version): + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + # Check that extra fields persist + new_metadata = {**draft_version.metadata, 'access': [{'extra': 'field'}]} + resp = api_client.put( + f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/', + {'metadata': new_metadata, 'name': draft_version.name}, + format='json', + ) + assert resp.status_code == 200 + draft_version.refresh_from_db() + access = draft_version.metadata['access'] + assert access != new_metadata['access'] + assert access[0]['schemaKey'] == 'AccessRequirements' + assert ( + access[0]['status'] == AccessType.EmbargoedAccess.value + if draft_version.dandiset.embargoed + else AccessType.OpenAccess.value + ) + + assert access[0]['extra'] == 'field' + + @pytest.mark.django_db() def test_version_rest_publish( api_client: APIClient, From bb0c841881afc804e77a988d1af02569d7d3e7ca Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 13:45:25 -0400 Subject: [PATCH 021/131] Add DandisetUnEmbargoInProgressError --- dandiapi/api/services/dandiset/exceptions.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dandiapi/api/services/dandiset/exceptions.py b/dandiapi/api/services/dandiset/exceptions.py index a71088c1b..00e2d9828 100644 --- a/dandiapi/api/services/dandiset/exceptions.py +++ b/dandiapi/api/services/dandiset/exceptions.py @@ -14,3 +14,8 @@ class UnauthorizedEmbargoAccessError(DandiError): message = ( 'Authentication credentials must be provided when attempting to access embargoed dandisets' ) + + +class DandisetUnEmbargoInProgressError(DandiError): + http_status_code = status.HTTP_400_BAD_REQUEST + message = 'Dandiset modification not allowed during un-embargo' From b4f94cac2a4ba45450f7caf81e0ee730a8198e85 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 13:47:27 -0400 Subject: [PATCH 022/131] Prohibit dandiset modification during un-embargo --- dandiapi/api/services/dandiset/__init__.py | 7 ++- dandiapi/api/tests/test_dandiset.py | 65 +++++++++++++++++++--- dandiapi/api/views/dandiset.py | 8 ++- 3 files changed, 69 insertions(+), 11 deletions(-) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 0cf80d82c..050428375 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -4,7 +4,10 @@ from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version -from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError +from dandiapi.api.services.dandiset.exceptions import ( + DandisetAlreadyExistsError, + DandisetUnEmbargoInProgressError, +) from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError from dandiapi.api.services.version.metadata import _normalize_version_metadata @@ -55,6 +58,8 @@ 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: + raise DandisetUnEmbargoInProgressError # Delete all versions first, so that AssetPath deletion is cascaded # through versions, rather than through zarrs directly diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index 34e656d2d..18cc1adb1 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -734,22 +734,32 @@ def test_dandiset_rest_create_with_invalid_identifier(api_client, admin_user): assert response.data == f'Invalid Identifier {identifier}' -@pytest.mark.parametrize( - ('embargo_status'), - [choice[0] for choice in Dandiset.EmbargoStatus.choices], - ids=[choice[1] for choice in Dandiset.EmbargoStatus.choices], -) @pytest.mark.django_db() -def test_dandiset_rest_delete(api_client, draft_version_factory, user, embargo_status): - draft_version = draft_version_factory(dandiset__embargo_status=embargo_status) +def test_dandiset_rest_delete(api_client, draft_version_factory, user): api_client.force_authenticate(user=user) - assign_perm('owner', user, draft_version.dandiset) + # 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) + 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 un-embargoing 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 + @pytest.mark.django_db() def test_dandiset_rest_delete_with_zarrs( @@ -831,14 +841,20 @@ def test_dandiset_rest_get_owners_no_social_account(api_client, dandiset, user): assert resp.data == [{'username': user.username, 'name': f'{user.first_name} {user.last_name}'}] +@pytest.mark.parametrize( + 'embargo_status', + [Dandiset.EmbargoStatus.OPEN, Dandiset.EmbargoStatus.EMBARGOED], +) @pytest.mark.django_db() def test_dandiset_rest_change_owner( api_client, - draft_version, + draft_version_factory, user_factory, social_account_factory, mailoutbox, + embargo_status, ): + draft_version = draft_version_factory(dandiset__embargo_status=embargo_status) dandiset = draft_version.dandiset user1 = user_factory() user2 = user_factory() @@ -868,6 +884,37 @@ def test_dandiset_rest_change_owner( assert mailoutbox[1].to == [user2.email] +@pytest.mark.django_db() +def test_dandiset_rest_change_owners_unembargoing( + api_client, + draft_version_factory, + user_factory, + social_account_factory, +): + """Test that un-embargoing a dandiset prevents user modification.""" + draft_version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + dandiset = draft_version.dandiset + user1 = user_factory() + user2 = user_factory() + social_account1 = social_account_factory(user=user1) + social_account2 = social_account_factory(user=user2) + assign_perm('owner', user1, dandiset) + api_client.force_authenticate(user=user1) + + resp = api_client.put( + f'/api/dandisets/{dandiset.identifier}/users/', + [ + {'username': social_account1.extra_data['login']}, + {'username': social_account2.extra_data['login']}, + ], + format='json', + ) + + assert resp.status_code == 400 + + @pytest.mark.django_db() def test_dandiset_rest_add_owner( api_client, diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 93bcef604..5cce647d5 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -25,7 +25,10 @@ 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.dandiset.exceptions import UnauthorizedEmbargoAccessError +from dandiapi.api.services.dandiset.exceptions import ( + DandisetUnEmbargoInProgressError, + UnauthorizedEmbargoAccessError, +) from dandiapi.api.services.embargo import unembargo_dandiset from dandiapi.api.views.common import DANDISET_PK_PARAM from dandiapi.api.views.pagination import DandiPagination @@ -374,6 +377,9 @@ def unembargo(self, request, dandiset__pk): def users(self, request, dandiset__pk): dandiset: Dandiset = self.get_object() if request.method == 'PUT': + if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + raise DandisetUnEmbargoInProgressError + # Verify that the user is currently an owner response = get_40x_or_None(request, ['owner'], dandiset, return_403=True) if response: From bc41f887f59ad4e25bd951df056902445792454c Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 14:03:42 -0400 Subject: [PATCH 023/131] Prohibit asset modification during un-embargo --- dandiapi/api/tests/test_asset.py | 75 ++++++++++++++++++++++++++++++++ dandiapi/api/views/asset.py | 14 ++++-- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index ea70ad969..1da6a32c3 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -636,6 +636,35 @@ def test_asset_create_embargo( assert draft_version.status == Version.Status.PENDING +@pytest.mark.django_db() +def test_asset_create_unembargoing( + api_client, user, draft_version_factory, dandiset_factory, embargoed_asset_blob +): + dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) + draft_version = draft_version_factory(dandiset=dandiset) + + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + path = 'test/create/asset.txt' + metadata = { + 'encodingFormat': 'application/x-nwb', + 'path': path, + 'meta': 'data', + 'foo': ['bar', 'baz'], + '1': 2, + } + + resp = api_client.post( + f'/api/dandisets/{draft_version.dandiset.identifier}' + f'/versions/{draft_version.version}/assets/', + {'metadata': metadata, 'blob_id': embargoed_asset_blob.blob_id}, + format='json', + ) + + assert resp.status_code == 400 + + @pytest.mark.django_db() def test_asset_create_embargoed_asset_blob_open_dandiset( api_client, user, draft_version, embargoed_asset_blob, mocker @@ -1092,6 +1121,35 @@ def test_asset_rest_update_embargo(api_client, user, draft_version, asset, embar assert draft_version.status == Version.Status.PENDING +@pytest.mark.django_db() +def test_asset_rest_update_unembargoing( + api_client, user, draft_version_factory, asset, embargoed_asset_blob +): + draft_version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + draft_version.assets.add(asset) + + new_path = 'test/asset/rest/update.txt' + new_metadata = { + 'encodingFormat': 'application/x-nwb', + 'path': new_path, + 'foo': 'bar', + 'num': 123, + 'list': ['a', 'b', 'c'], + } + + resp = api_client.put( + f'/api/dandisets/{draft_version.dandiset.identifier}/' + f'versions/{draft_version.version}/assets/{asset.asset_id}/', + {'metadata': new_metadata, 'blob_id': embargoed_asset_blob.blob_id}, + format='json', + ) + assert resp.status_code == 400 + + @pytest.mark.django_db() def test_asset_rest_update_zarr( api_client, @@ -1266,6 +1324,23 @@ def test_asset_rest_delete(api_client, user, draft_version, asset): assert draft_version.status == Version.Status.PENDING +@pytest.mark.django_db() +def test_asset_rest_delete_unembargoing(api_client, user, draft_version_factory, asset): + draft_version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + assign_perm('owner', user, draft_version.dandiset) + draft_version.assets.add(asset) + + # Make request + api_client.force_authenticate(user=user) + response = api_client.delete( + f'/api/dandisets/{draft_version.dandiset.identifier}/' + f'versions/{draft_version.version}/assets/{asset.asset_id}/' + ) + assert response.status_code == 400 + + @pytest.mark.django_db() def test_asset_rest_delete_zarr( api_client, diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 4e86cf273..23d91acf5 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -9,6 +9,7 @@ remove_asset_from_version, ) from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError +from dandiapi.api.services.dandiset.exceptions import DandisetUnEmbargoInProgressError from dandiapi.zarr.models import ZarrArchive try: @@ -318,11 +319,14 @@ def validation(self, request, **kwargs): ) def create(self, request, versions__dandiset__pk, versions__version): version: Version = get_object_or_404( - Version, + Version.objects.select_related('dandiset'), dandiset=versions__dandiset__pk, version=versions__version, ) + if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + raise DandisetUnEmbargoInProgressError + serializer = AssetRequestSerializer(data=self.request.data) serializer.is_valid(raise_exception=True) @@ -351,12 +355,14 @@ def create(self, request, versions__dandiset__pk, versions__version): def update(self, request, versions__dandiset__pk, versions__version, **kwargs): """Update the metadata of an asset.""" version: Version = get_object_or_404( - Version, + Version.objects.select_related('dandiset'), dandiset__pk=versions__dandiset__pk, version=versions__version, ) if version.version != 'draft': raise DraftDandisetNotModifiableError + if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + raise DandisetUnEmbargoInProgressError serializer = AssetRequestSerializer(data=self.request.data) serializer.is_valid(raise_exception=True) @@ -389,10 +395,12 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): ) def destroy(self, request, versions__dandiset__pk, versions__version, **kwargs): version = get_object_or_404( - Version, + Version.objects.select_related('dandiset'), dandiset__pk=versions__dandiset__pk, version=versions__version, ) + if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + raise DandisetUnEmbargoInProgressError # Lock asset for delete with transaction.atomic(): From acc1373a4927b5233821adfe4481a83920f25788 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 14:38:52 -0400 Subject: [PATCH 024/131] Handle embargo status check explicitly during publish --- dandiapi/api/services/publish/__init__.py | 9 ++++--- dandiapi/api/tests/test_version.py | 31 +++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index a41ead487..10ea61f41 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -48,11 +48,12 @@ def _lock_dandiset_for_publishing(*, user: User, dandiset: Dandiset) -> None: This function MUST be called before _publish_dandiset is called. """ - if ( - not user.has_perm('owner', dandiset) - or dandiset.embargo_status != Dandiset.EmbargoStatus.OPEN - ): + if not user.has_perm('owner', dandiset): raise NotAllowedError + + if dandiset.embargo_status != Dandiset.EmbargoStatus.OPEN: + raise NotAllowedError('Operation only allowed on OPEN dandisets', 400) + if dandiset.zarr_archives.exists() or dandiset.embargoed_zarr_archives.exists(): raise NotAllowedError('Cannot publish dandisets which contain zarrs', 400) diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index 9c530999d..144869aae 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -9,6 +9,7 @@ from guardian.shortcuts import assign_perm import pytest +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 @@ -669,6 +670,36 @@ def test_version_rest_publish( assert draft_version.status == Version.Status.PUBLISHING +@pytest.mark.django_db() +def test_version_rest_publish_embargo(api_client: APIClient, user: User, draft_version_factory): + draft_version = draft_version_factory(dandiset__embargo_status=Dandiset.EmbargoStatus.EMBARGOED) + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + resp = api_client.post( + f'/api/dandisets/{draft_version.dandiset.identifier}' + f'/versions/{draft_version.version}/publish/' + ) + assert resp.status_code == 400 + + +@pytest.mark.django_db() +def test_version_rest_publish_unembargoing( + api_client: APIClient, user: User, draft_version_factory +): + draft_version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + resp = api_client.post( + f'/api/dandisets/{draft_version.dandiset.identifier}' + f'/versions/{draft_version.version}/publish/' + ) + assert resp.status_code == 400 + + @pytest.mark.django_db() def test_version_rest_publish_zarr( api_client, From 637830dab3ef4f7a8ae2d40d5b1fd48c4230e085 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 14:39:40 -0400 Subject: [PATCH 025/131] Prohibit version update during un-embargo --- dandiapi/api/tests/test_version.py | 26 ++++++++++++++++++++++++++ dandiapi/api/views/version.py | 3 +++ 2 files changed, 29 insertions(+) diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index 144869aae..74e170e99 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -601,6 +601,32 @@ def test_version_rest_update(api_client, user, draft_version): assert draft_version.status == Version.Status.PENDING +@pytest.mark.django_db() +def test_version_rest_update_unembargoing(api_client, user, draft_version_factory): + draft_version = draft_version_factory( + dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING + ) + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + new_name = 'A unique and special name!' + new_metadata = { + '@context': ( + 'https://raw.githubusercontent.com/dandi/schema/master/releases/' + f'{settings.DANDI_SCHEMA_VERSION}/context.json' + ), + 'schemaVersion': settings.DANDI_SCHEMA_VERSION, + 'num': 123, + } + + resp = api_client.put( + f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/', + {'metadata': new_metadata, 'name': new_name}, + format='json', + ) + assert resp.status_code == 400 + + @pytest.mark.django_db() def test_version_rest_update_published_version(api_client, user, published_version): assign_perm('owner', user, published_version.dandiset) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 2cfc3335a..17dd3a9b6 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -14,6 +14,7 @@ from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin from dandiapi.api.models import Dandiset, Version +from dandiapi.api.services.dandiset.exceptions import DandisetUnEmbargoInProgressError from dandiapi.api.services.publish import publish_dandiset from dandiapi.api.tasks import delete_doi_task from dandiapi.api.views.common import DANDISET_PK_PARAM, VERSION_PARAM @@ -94,6 +95,8 @@ 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: + raise DandisetUnEmbargoInProgressError serializer = VersionMetadataSerializer(data=request.data) serializer.is_valid(raise_exception=True) From c5083ff8f42654b48c8e3798647beff4b334c188 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 16:25:00 -0400 Subject: [PATCH 026/131] Add linter ignores --- dandiapi/api/services/publish/__init__.py | 2 +- dandiapi/api/views/dandiset.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index 10ea61f41..406ab01f6 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -42,7 +42,7 @@ def publish_asset(*, asset: Asset) -> None: locked_asset.save() -def _lock_dandiset_for_publishing(*, user: User, dandiset: Dandiset) -> None: +def _lock_dandiset_for_publishing(*, user: User, dandiset: Dandiset) -> None: # noqa: C901 """ Prepare a dandiset to be published by locking it and setting its status to PUBLISHING. diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 5cce647d5..46bec9572 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -374,7 +374,7 @@ def unembargo(self, request, dandiset__pk): ) # TODO: move these into a viewset @action(methods=['GET', 'PUT'], detail=True) - def users(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: From ef7c5308a40576261ec6f3319b0e15dd248b2c24 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 17 Jun 2024 11:28:37 -0400 Subject: [PATCH 027/131] Prohibit uploads to dandiset during un-embargo --- dandiapi/api/tests/test_upload.py | 19 +++++++++++++++++++ dandiapi/api/views/upload.py | 5 +++++ 2 files changed, 24 insertions(+) diff --git a/dandiapi/api/tests/test_upload.py b/dandiapi/api/tests/test_upload.py index e8a7493f8..133c2ff7a 100644 --- a/dandiapi/api/tests/test_upload.py +++ b/dandiapi/api/tests/test_upload.py @@ -109,6 +109,25 @@ def test_upload_initialize(api_client, user, dandiset_factory, embargoed): assert upload.blob.name == f'test-prefix/blobs/{upload_id[:3]}/{upload_id[3:6]}/{upload_id}' +@pytest.mark.django_db() +def test_upload_initialize_unembargoing(api_client, user, dandiset_factory): + dandiset = dandiset_factory(embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING) + api_client.force_authenticate(user=user) + assign_perm('owner', user, dandiset) + + content_size = 123 + resp = api_client.post( + '/api/uploads/initialize/', + { + 'contentSize': content_size, + 'digest': {'algorithm': 'dandi:dandi-etag', 'value': 'f' * 32 + '-1'}, + 'dandiset': dandiset.identifier, + }, + format='json', + ) + assert resp.status_code == 400 + + @pytest.mark.django_db() def test_upload_initialize_existing_asset_blob(api_client, user, dandiset, asset_blob): api_client.force_authenticate(user=user) diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index 178d9ea16..c9efdac68 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -17,6 +17,7 @@ from dandiapi.api.models import AssetBlob, Dandiset, Upload from dandiapi.api.permissions import IsApproved +from dandiapi.api.services.dandiset.exceptions import DandisetUnEmbargoInProgressError from dandiapi.api.tasks import calculate_sha256 from dandiapi.api.views.serializers import AssetBlobSerializer @@ -135,6 +136,10 @@ def upload_initialize_view(request: Request) -> HttpResponseBase: if response: return response + # Ensure dandiset not in the process of un-embargo + if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: + raise DandisetUnEmbargoInProgressError + logging.info( 'Starting upload initialization of size %s, ETag %s to dandiset %s', content_size, From b7896457e117d1b31604c431721a04ed45db16b2 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 17 Jun 2024 11:39:01 -0400 Subject: [PATCH 028/131] Prohibit un-embargo if dandiset has active uploads --- dandiapi/api/services/embargo/__init__.py | 9 ++++++++- dandiapi/api/services/embargo/exceptions.py | 5 +++++ dandiapi/api/tests/test_unembargo.py | 15 +++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index dd07e04da..954f96098 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -12,7 +12,11 @@ from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.storage import get_boto_client -from .exceptions import AssetBlobEmbargoedError, DandisetNotEmbargoedError +from .exceptions import ( + AssetBlobEmbargoedError, + DandisetActiveUploadsError, + DandisetNotEmbargoedError, +) if TYPE_CHECKING: from django.contrib.auth.models import User @@ -75,6 +79,9 @@ def unembargo_dandiset(*, user: User, dandiset: Dandiset): if not user.has_perm('owner', dandiset): raise DandisetOwnerRequiredError + 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 un-embargo process dandiset.embargo_status = Dandiset.EmbargoStatus.UNEMBARGOING diff --git a/dandiapi/api/services/embargo/exceptions.py b/dandiapi/api/services/embargo/exceptions.py index b9d58aa2f..c4db7ad72 100644 --- a/dandiapi/api/services/embargo/exceptions.py +++ b/dandiapi/api/services/embargo/exceptions.py @@ -13,3 +13,8 @@ class AssetBlobEmbargoedError(DandiError): class DandisetNotEmbargoedError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'Dandiset not embargoed' + + +class DandisetActiveUploadsError(DandiError): + http_status_code = status.HTTP_400_BAD_REQUEST + message = 'Dandiset un-embargo not allowed with active uploads' diff --git a/dandiapi/api/tests/test_unembargo.py b/dandiapi/api/tests/test_unembargo.py index ac41a6c41..dae007551 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -39,3 +39,18 @@ def test_unembargo_dandiset_sends_emails( assert 'un-embargo' 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_unembargo_dandiset_lingering_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) + + resp = api_client.post(f'/api/dandisets/{dandiset.identifier}/unembargo/') + assert resp.status_code == 400 From 6ae94fcd64f8ee7cc7a984bcebd39a8f16682ac8 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 17 Jun 2024 11:42:49 -0400 Subject: [PATCH 029/131] Rearrange embargo exceptions --- dandiapi/api/services/dandiset/__init__.py | 6 ++---- dandiapi/api/services/dandiset/exceptions.py | 12 ------------ dandiapi/api/services/embargo/exceptions.py | 12 ++++++++++++ dandiapi/api/views/asset.py | 2 +- dandiapi/api/views/dandiset.py | 4 ++-- dandiapi/api/views/upload.py | 2 +- dandiapi/api/views/version.py | 2 +- 7 files changed, 19 insertions(+), 21 deletions(-) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 050428375..2159c9dbd 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -4,10 +4,8 @@ from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version -from dandiapi.api.services.dandiset.exceptions import ( - DandisetAlreadyExistsError, - DandisetUnEmbargoInProgressError, -) +from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError +from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError from dandiapi.api.services.version.metadata import _normalize_version_metadata diff --git a/dandiapi/api/services/dandiset/exceptions.py b/dandiapi/api/services/dandiset/exceptions.py index 00e2d9828..361d2e08b 100644 --- a/dandiapi/api/services/dandiset/exceptions.py +++ b/dandiapi/api/services/dandiset/exceptions.py @@ -7,15 +7,3 @@ class DandisetAlreadyExistsError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST - - -class UnauthorizedEmbargoAccessError(DandiError): - http_status_code = status.HTTP_401_UNAUTHORIZED - message = ( - 'Authentication credentials must be provided when attempting to access embargoed dandisets' - ) - - -class DandisetUnEmbargoInProgressError(DandiError): - http_status_code = status.HTTP_400_BAD_REQUEST - message = 'Dandiset modification not allowed during un-embargo' diff --git a/dandiapi/api/services/embargo/exceptions.py b/dandiapi/api/services/embargo/exceptions.py index c4db7ad72..351522269 100644 --- a/dandiapi/api/services/embargo/exceptions.py +++ b/dandiapi/api/services/embargo/exceptions.py @@ -18,3 +18,15 @@ class DandisetNotEmbargoedError(DandiError): class DandisetActiveUploadsError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'Dandiset un-embargo not allowed with active uploads' + + +class DandisetUnEmbargoInProgressError(DandiError): + http_status_code = status.HTTP_400_BAD_REQUEST + message = 'Dandiset modification not allowed during un-embargo' + + +class UnauthorizedEmbargoAccessError(DandiError): + http_status_code = status.HTTP_401_UNAUTHORIZED + message = ( + 'Authentication credentials must be provided when attempting to access embargoed dandisets' + ) diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 23d91acf5..549ce113a 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -9,7 +9,7 @@ remove_asset_from_version, ) from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError -from dandiapi.api.services.dandiset.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError from dandiapi.zarr.models import ZarrArchive try: diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 46bec9572..50d693bfc 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -25,11 +25,11 @@ 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.dandiset.exceptions import ( +from dandiapi.api.services.embargo import unembargo_dandiset +from dandiapi.api.services.embargo.exceptions import ( DandisetUnEmbargoInProgressError, UnauthorizedEmbargoAccessError, ) -from dandiapi.api.services.embargo import unembargo_dandiset from dandiapi.api.views.common import DANDISET_PK_PARAM from dandiapi.api.views.pagination import DandiPagination from dandiapi.api.views.serializers import ( diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index c9efdac68..7ef0aa0bd 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -17,7 +17,7 @@ from dandiapi.api.models import AssetBlob, Dandiset, Upload from dandiapi.api.permissions import IsApproved -from dandiapi.api.services.dandiset.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError from dandiapi.api.tasks import calculate_sha256 from dandiapi.api.views.serializers import AssetBlobSerializer diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 17dd3a9b6..7ef74d9ed 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -14,7 +14,7 @@ from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin from dandiapi.api.models import Dandiset, Version -from dandiapi.api.services.dandiset.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError from dandiapi.api.services.publish import publish_dandiset from dandiapi.api.tasks import delete_doi_task from dandiapi.api.views.common import DANDISET_PK_PARAM, VERSION_PARAM From 070d42de1fe9f8b36b7644ead65730f38ce300bf Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Fri, 14 Jun 2024 13:38:37 -0400 Subject: [PATCH 030/131] Strip certain access information in Version.strip_metadata --- dandiapi/api/models/version.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/models/version.py b/dandiapi/api/models/version.py index 6007e732a..a5767b7ec 100644 --- a/dandiapi/api/models/version.py +++ b/dandiapi/api/models/version.py @@ -166,7 +166,19 @@ def strip_metadata(cls, metadata): 'publishedBy', 'manifestLocation', ] - return {key: metadata[key] for key in metadata if key not in computed_fields} + stripped = {key: metadata[key] for key in metadata if key not in computed_fields} + + # Strip the status and schemaKey fields, as modifying them is not supported + if ( + 'access' in stripped + and isinstance(stripped['access'], list) + and len(stripped['access']) + and isinstance(stripped['access'][0], dict) + ): + stripped['access'][0].pop('schemaKey', None) + stripped['access'][0].pop('status', None) + + return stripped def _populate_access_metadata(self): default_access = [{}] From d224ea8ba538174eb8fb320ca4f6e2955f7db44f Mon Sep 17 00:00:00 2001 From: dandibot Date: Tue, 18 Jun 2024 21:36:55 +0000 Subject: [PATCH 031/131] auto shipit - CHANGELOG.md etc --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 413e6f55a..32c107057 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +# v0.3.86 (Tue Jun 18 2024) + +#### 🐛 Bug Fix + +- Restrict updates to metadata `access` field [#1954](https://github.com/dandi/dandi-archive/pull/1954) ([@jjnesbitt](https://github.com/jjnesbitt)) +- Fix race condition in sha256 calculation task [#1936](https://github.com/dandi/dandi-archive/pull/1936) ([@mvandenburgh](https://github.com/mvandenburgh)) + +#### Authors: 2 + +- Jacob Nesbitt ([@jjnesbitt](https://github.com/jjnesbitt)) +- Mike VanDenburgh ([@mvandenburgh](https://github.com/mvandenburgh)) + +--- + # v0.3.85 (Mon Jun 03 2024) :tada: This release contains work from a new contributor! :tada: From 492b48c311ac4b8faac9f2e84a6e9d249f59d2b6 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 18 Jun 2024 16:42:46 -0400 Subject: [PATCH 032/131] Fix spelling of "unembargo" --- dandiapi/api/mail.py | 10 ++++------ dandiapi/api/migrations/0008_migrate_embargoed_data.py | 2 +- dandiapi/api/services/asset/__init__.py | 4 ++-- dandiapi/api/services/dandiset/__init__.py | 4 ++-- dandiapi/api/services/embargo/__init__.py | 2 +- dandiapi/api/services/embargo/exceptions.py | 6 +++--- dandiapi/api/tasks/scheduled.py | 4 ++-- .../api/templates/api/mail/dandiset_unembargoed.html | 2 +- .../api/templates/api/mail/dandisets_to_unembargo.txt | 2 +- dandiapi/api/tests/test_asset.py | 2 +- dandiapi/api/tests/test_dandiset.py | 4 ++-- dandiapi/api/tests/test_unembargo.py | 2 +- dandiapi/api/views/asset.py | 8 ++++---- dandiapi/api/views/dandiset.py | 4 ++-- dandiapi/api/views/upload.py | 6 +++--- dandiapi/api/views/version.py | 4 ++-- doc/design/embargo-redesign.md | 10 +++++----- .../views/DandisetLandingView/DandisetUnembargo.vue | 4 ++-- 18 files changed, 39 insertions(+), 41 deletions(-) diff --git a/dandiapi/api/mail.py b/dandiapi/api/mail.py index 9f05acdb5..d68395d69 100644 --- a/dandiapi/api/mail.py +++ b/dandiapi/api/mail.py @@ -194,14 +194,14 @@ def build_dandisets_to_unembargo_message(dandisets: Iterable[Dandiset]): ] render_context = {**BASE_RENDER_CONTEXT, 'dandisets': dandiset_context} return build_message( - subject='DANDI: New Dandisets to un-embargo', + 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 un-embargo message to devs at %s', settings.DANDI_DEV_EMAIL) + 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) @@ -219,7 +219,7 @@ def build_dandiset_unembargoed_message(dandiset: Dandiset): } html_message = render_to_string('api/mail/dandiset_unembargoed.html', render_context) return build_message( - subject='Your Dandiset has been un-embargoed!', + subject='Your Dandiset has been unembargoed!', message=strip_tags(html_message), html_message=html_message, to=[owner.email for owner in dandiset.owners], @@ -227,9 +227,7 @@ def build_dandiset_unembargoed_message(dandiset: Dandiset): def send_dandiset_unembargoed_message(dandiset: Dandiset): - logger.info( - 'Sending dandisets un-embargoed message to dandiset %s owners.', dandiset.identifier - ) + logger.info('Sending dandisets unembargoed message to dandiset %s owners.', dandiset.identifier) messages = [build_dandiset_unembargoed_message(dandiset)] with mail.get_connection() as connection: connection.send_messages(messages) diff --git a/dandiapi/api/migrations/0008_migrate_embargoed_data.py b/dandiapi/api/migrations/0008_migrate_embargoed_data.py index 13943aeaf..d454fe3fb 100644 --- a/dandiapi/api/migrations/0008_migrate_embargoed_data.py +++ b/dandiapi/api/migrations/0008_migrate_embargoed_data.py @@ -21,7 +21,7 @@ def migrate_embargoed_blob(embargoed_blob): # as the above case, but between an embargoed and open dandiset, instead of two # embargoed dandisets. # - # In case #3, the asset will effectively be un-embargoed. + # In case #3, the asset will effectively be unembargoed. existing_blob = AssetBlob.objects.filter( etag=embargoed_blob.etag, size=embargoed_blob.size ).first() diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index 6c0ee60ed..4e75048b3 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -136,7 +136,7 @@ def add_asset_to_version( raise ZarrArchiveBelongsToDifferentDandisetError # Creating an asset in an OPEN dandiset that points to an embargoed blob results in that - # blob being un-embargoed + # blob being unembargoed unembargo_asset_blob = ( asset_blob is not None and asset_blob.embargoed @@ -159,7 +159,7 @@ def add_asset_to_version( version.save() # Perform this after the above transaction has finished, to ensure we only operate on - # un-embargoed asset blobs + # unembargoed asset blobs if asset_blob and unembargo_asset_blob: remove_asset_blob_embargoed_tag_task.delay(blob_id=asset_blob.blob_id) diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 2159c9dbd..05450a94f 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -5,7 +5,7 @@ from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError -from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError from dandiapi.api.services.version.metadata import _normalize_version_metadata @@ -57,7 +57,7 @@ def delete_dandiset(*, user, dandiset: Dandiset) -> None: 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: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError # Delete all versions first, so that AssetPath deletion is cascaded # through versions, rather than through zarrs directly diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index 954f96098..c4bcf89a3 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -83,7 +83,7 @@ def unembargo_dandiset(*, user: User, dandiset: Dandiset): raise DandisetActiveUploadsError # A scheduled task will pick up any new dandisets with this status and email the admins to - # initiate the un-embargo process + # initiate the unembargo process dandiset.embargo_status = Dandiset.EmbargoStatus.UNEMBARGOING dandiset.save() diff --git a/dandiapi/api/services/embargo/exceptions.py b/dandiapi/api/services/embargo/exceptions.py index 351522269..13038bd59 100644 --- a/dandiapi/api/services/embargo/exceptions.py +++ b/dandiapi/api/services/embargo/exceptions.py @@ -17,12 +17,12 @@ class DandisetNotEmbargoedError(DandiError): class DandisetActiveUploadsError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST - message = 'Dandiset un-embargo not allowed with active uploads' + message = 'Dandiset unembargo not allowed with active uploads' -class DandisetUnEmbargoInProgressError(DandiError): +class DandisetUnembargoInProgressError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST - message = 'Dandiset modification not allowed during un-embargo' + message = 'Dandiset modification not allowed during unembargo' class UnauthorizedEmbargoAccessError(DandiError): diff --git a/dandiapi/api/tasks/scheduled.py b/dandiapi/api/tasks/scheduled.py index bba10340d..9fa8af71d 100644 --- a/dandiapi/api/tasks/scheduled.py +++ b/dandiapi/api/tasks/scheduled.py @@ -114,7 +114,7 @@ def send_pending_users_email() -> None: @shared_task(soft_time_limit=20) def send_dandisets_to_unembargo_email() -> None: - """Send an email to admins listing dandisets that have requested un-embargo.""" + """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) @@ -148,7 +148,7 @@ 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 un-embargo + # 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. diff --git a/dandiapi/api/templates/api/mail/dandiset_unembargoed.html b/dandiapi/api/templates/api/mail/dandiset_unembargoed.html index 978dea42f..236a5281b 100644 --- a/dandiapi/api/templates/api/mail/dandiset_unembargoed.html +++ b/dandiapi/api/templates/api/mail/dandiset_unembargoed.html @@ -1,4 +1,4 @@ -Dandiset {{ dandiset.identifier }} has been un-embargoed, and is now open for +Dandiset {{ dandiset.identifier }} has been unembargoed, and is now open for public access.

You are receiving this email because you are an owner of this dandiset. diff --git a/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt b/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt index 3676f7721..c716839b1 100644 --- a/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt +++ b/dandiapi/api/templates/api/mail/dandisets_to_unembargo.txt @@ -1,5 +1,5 @@ {% autoescape off %} -The following new dandisets are awaiting un-embargo: +The following new dandisets are awaiting unembargo: {% for ds in dandisets %} Dandiset ID: {{ ds.identifier }} diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 1da6a32c3..68dadca42 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -670,7 +670,7 @@ def test_asset_create_embargoed_asset_blob_open_dandiset( api_client, user, draft_version, embargoed_asset_blob, mocker ): # Ensure that creating an asset in an open dandiset that points to an embargoed asset blob - # results in that asset blob being un-embargoed + # results in that asset blob being unembargoed assert draft_version.dandiset.embargo_status == Dandiset.EmbargoStatus.OPEN assert embargoed_asset_blob.embargoed diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index 18cc1adb1..9efeed612 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -751,7 +751,7 @@ def test_dandiset_rest_delete(api_client, draft_version_factory, user): assert response.status_code == 204 assert not Dandiset.objects.all() - # Ensure that currently un-embargoing dandisets can't be deleted + # Ensure that currently unembargoing dandisets can't be deleted draft_version = draft_version_factory( dandiset__embargo_status=Dandiset.EmbargoStatus.UNEMBARGOING ) @@ -891,7 +891,7 @@ def test_dandiset_rest_change_owners_unembargoing( user_factory, social_account_factory, ): - """Test that un-embargoing a dandiset prevents user modification.""" + """Test that unembargoing a dandiset 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 dae007551..b4976d832 100644 --- a/dandiapi/api/tests/test_unembargo.py +++ b/dandiapi/api/tests/test_unembargo.py @@ -36,7 +36,7 @@ def test_unembargo_dandiset_sends_emails( send_dandisets_to_unembargo_email() assert mailoutbox - assert 'un-embargo' in mailoutbox[0].subject + assert 'unembargo' in mailoutbox[0].subject assert dandiset.identifier in mailoutbox[0].message().get_payload() assert user.username in mailoutbox[0].message().get_payload() diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 549ce113a..d92fa0d0e 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -9,7 +9,7 @@ remove_asset_from_version, ) from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError -from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.zarr.models import ZarrArchive try: @@ -325,7 +325,7 @@ def create(self, request, versions__dandiset__pk, versions__version): ) if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError serializer = AssetRequestSerializer(data=self.request.data) serializer.is_valid(raise_exception=True) @@ -362,7 +362,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: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError serializer = AssetRequestSerializer(data=self.request.data) serializer.is_valid(raise_exception=True) @@ -400,7 +400,7 @@ def destroy(self, request, versions__dandiset__pk, versions__version, **kwargs): version=versions__version, ) if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError # Lock asset for delete with transaction.atomic(): diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 50d693bfc..d202cc11f 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -27,7 +27,7 @@ from dandiapi.api.services.dandiset import create_dandiset, delete_dandiset from dandiapi.api.services.embargo import unembargo_dandiset from dandiapi.api.services.embargo.exceptions import ( - DandisetUnEmbargoInProgressError, + DandisetUnembargoInProgressError, UnauthorizedEmbargoAccessError, ) from dandiapi.api.views.common import DANDISET_PK_PARAM @@ -378,7 +378,7 @@ def users(self, request, dandiset__pk): # noqa: C901 dandiset: Dandiset = self.get_object() if request.method == 'PUT': if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError # Verify that the user is currently an owner response = get_40x_or_None(request, ['owner'], dandiset, return_403=True) diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index 7ef0aa0bd..acdf6a41e 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -17,7 +17,7 @@ from dandiapi.api.models import AssetBlob, Dandiset, Upload from dandiapi.api.permissions import IsApproved -from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.tasks import calculate_sha256 from dandiapi.api.views.serializers import AssetBlobSerializer @@ -136,9 +136,9 @@ def upload_initialize_view(request: Request) -> HttpResponseBase: if response: return response - # Ensure dandiset not in the process of un-embargo + # Ensure dandiset not in the process of unembargo if dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError logging.info( 'Starting upload initialization of size %s, ETag %s to dandiset %s', diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index 7ef74d9ed..0a052d9a7 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -14,7 +14,7 @@ from rest_framework_extensions.mixins import DetailSerializerMixin, NestedViewSetMixin from dandiapi.api.models import Dandiset, Version -from dandiapi.api.services.embargo.exceptions import DandisetUnEmbargoInProgressError +from dandiapi.api.services.embargo.exceptions import DandisetUnembargoInProgressError from dandiapi.api.services.publish import publish_dandiset from dandiapi.api.tasks import delete_doi_task from dandiapi.api.views.common import DANDISET_PK_PARAM, VERSION_PARAM @@ -96,7 +96,7 @@ def update(self, request, **kwargs): status=status.HTTP_405_METHOD_NOT_ALLOWED, ) if version.dandiset.embargo_status == Dandiset.EmbargoStatus.UNEMBARGOING: - raise DandisetUnEmbargoInProgressError + raise DandisetUnembargoInProgressError serializer = VersionMetadataSerializer(data=request.data) serializer.is_valid(raise_exception=True) diff --git a/doc/design/embargo-redesign.md b/doc/design/embargo-redesign.md index 754395afc..d3e783c2b 100644 --- a/doc/design/embargo-redesign.md +++ b/doc/design/embargo-redesign.md @@ -71,9 +71,9 @@ sequenceDiagram end ``` -## Change to Un-Embargo Procedure +## Change to Unembargo Procedure -Once the time comes to *********un-embargo********* those files, all that is required is to remove the `embargoed` tag from all of the objects. This can be achieved by an [S3 Batch Operations Job](https://docs.aws.amazon.com/AmazonS3/latest/userguide/batch-ops-create-job.html), in which the list of files is specified (all files belonging to the dandiset), and the desired action is specified (delete/replace tags). +Once the time comes to *********unembargo********* those files, all that is required is to remove the `embargoed` tag from all of the objects. This can be achieved by an [S3 Batch Operations Job](https://docs.aws.amazon.com/AmazonS3/latest/userguide/batch-ops-create-job.html), in which the list of files is specified (all files belonging to the dandiset), and the desired action is specified (delete/replace tags). The benefit of this approach is that once the files are uploaded, no further movement is required to change the embargo state, eliminating the storage, egress, and time costs associated with unembargoing from a second bucket. Using S3 Batch Operations to perform the untagging also means we can rely on AWS’s own error reporting mechanisms, while retrying any failed operations requires only minimal engineering effort within the Archive codebase. @@ -85,7 +85,7 @@ The benefit of this approach is that once the files are uploaded, no further mov 4. If there are no failures, the Job ID is set to null in the DB model, and the embargo status, metadata, etc. is updated to reflect that the dandiset is now `OPEN`. 5. Otherwise, an exception is raised and attended to by the developers. -A diagram of the un-embargo procedure (pertaining to just the objects) is shown below +A diagram of the unembargo procedure (pertaining to just the objects) is shown below ```mermaid sequenceDiagram @@ -95,8 +95,8 @@ sequenceDiagram participant Worker participant S3 - Client ->> Server: Un-embargo dandiset - Server ->> Worker: Dispatch un-embargo task + Client ->> Server: Unembargo dandiset + Server ->> Worker: Dispatch unembargo task Worker ->> S3: List of all dandiset objects are aggregated into a manifest Worker ->> S3: S3 Batch Operation job created S3 ->> Worker: Job ID is returned diff --git a/web/src/views/DandisetLandingView/DandisetUnembargo.vue b/web/src/views/DandisetLandingView/DandisetUnembargo.vue index 516f3cf77..c6c846a80 100644 --- a/web/src/views/DandisetLandingView/DandisetUnembargo.vue +++ b/web/src/views/DandisetLandingView/DandisetUnembargo.vue @@ -17,11 +17,11 @@ - This action will un-embargo this dandiset. Note that this is a + This action will unembargo this dandiset. Note that this is a permanent - action and cannot be undone. Once a dandiset has been un-embargoed, + action and cannot be undone. Once a dandiset has been unembargoed, it cannot be re-embargoed.

Note: this may take several days to complete. From 1a294062997a79c26e3a91bb4e4a903224f7cc56 Mon Sep 17 00:00:00 2001 From: dandibot Date: Fri, 21 Jun 2024 00:46:10 +0000 Subject: [PATCH 033/131] auto shipit - CHANGELOG.md etc --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32c107057..75119ee38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,15 @@ +# v0.3.87 (Fri Jun 21 2024) + +#### 🐛 Bug Fix + +- Lock dandisets during un-embargo [#1957](https://github.com/dandi/dandi-archive/pull/1957) ([@jjnesbitt](https://github.com/jjnesbitt)) + +#### Authors: 1 + +- Jacob Nesbitt ([@jjnesbitt](https://github.com/jjnesbitt)) + +--- + # v0.3.86 (Tue Jun 18 2024) #### 🐛 Bug Fix From 6d81e0f4eb77d7e6821f7c1860ec72c02d56048d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 24 Jun 2024 22:00:16 +0000 Subject: [PATCH 034/131] [gh-actions](deps): Bump actions/add-to-project from 0.6.0 to 1.0.2 Bumps [actions/add-to-project](https://github.com/actions/add-to-project) from 0.6.0 to 1.0.2. - [Release notes](https://github.com/actions/add-to-project/releases) - [Commits](https://github.com/actions/add-to-project/compare/v0.6.0...v1.0.2) --- updated-dependencies: - dependency-name: actions/add-to-project dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/auto-add-issues.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-add-issues.yml b/.github/workflows/auto-add-issues.yml index 79cbb207d..6eca60f09 100644 --- a/.github/workflows/auto-add-issues.yml +++ b/.github/workflows/auto-add-issues.yml @@ -11,7 +11,7 @@ jobs: name: Add issue to project runs-on: ubuntu-latest steps: - - uses: actions/add-to-project@v0.6.0 + - uses: actions/add-to-project@v1.0.2 with: project-url: https://github.com/orgs/dandi/projects/16 github-token: ${{ secrets.AUTO_ADD_ISSUES }} From 3b5163827d0bdb150e2b9cb7f79725992a6ec4a9 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 10 Jul 2024 08:30:30 -1000 Subject: [PATCH 035/131] Remove File to avoid confusion (Closes #1971) --- web/src/views/DandisetLandingView/DandisetMain.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/views/DandisetLandingView/DandisetMain.vue b/web/src/views/DandisetLandingView/DandisetMain.vue index 0fd94c030..32828e1ee 100644 --- a/web/src/views/DandisetLandingView/DandisetMain.vue +++ b/web/src/views/DandisetLandingView/DandisetMain.vue @@ -118,7 +118,7 @@ mdi-server - File Size {{ transformFilesize(stats.size) }} + Size {{ transformFilesize(stats.size) }} From 84c019cb86f28183b946ef5a93edbf818891e97a Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Wed, 10 Jul 2024 16:42:08 -0400 Subject: [PATCH 036/131] Suppress lint error (SIM103) --- dandiapi/api/models/asset.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandiapi/api/models/asset.py b/dandiapi/api/models/asset.py index cc2bed6ad..7fc106ae4 100644 --- a/dandiapi/api/models/asset.py +++ b/dandiapi/api/models/asset.py @@ -218,7 +218,7 @@ def is_different_from( if self.path != path: return True - if self.metadata != metadata: + if self.metadata != metadata: # noqa: SIM103 return True return False From 8daeb79b0b9616c86c59044b16a0bea4e73d3592 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 11 Jul 2024 15:42:27 -0400 Subject: [PATCH 037/131] Pin dandischema to 0.10.1 (schema version 0.6.7) --- setup.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 15738bcab..ba8aafa90 100644 --- a/setup.py +++ b/setup.py @@ -40,7 +40,8 @@ include_package_data=True, install_requires=[ 'celery', - 'dandischema~=0.10.1', + # Pin dandischema to exact version to make explicit which schema version is being used + 'dandischema==0.10.1', # schema version 0.6.7 'django~=4.1.0', 'django-admin-display', # Require 0.58.0 as it is the first version to support postgres' native From 524c835a82ffea9c23a90b128f04c42f9580480e Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 11 Jul 2024 15:42:57 -0400 Subject: [PATCH 038/131] Pin django-storages to 1.14.3 --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index ba8aafa90..4a988ff8d 100644 --- a/setup.py +++ b/setup.py @@ -68,7 +68,7 @@ # Production-only 'django-composed-configuration[prod]>=0.23.0', 'django-s3-file-field[s3]>=1.0.0', - 'django-storages[s3]>=1.14.3', + 'django-storages[s3]==1.14.3', 'gunicorn', # Development-only, but required 'django-minio-storage', From 0dcef6820d526c4923ac41a39219e8c1e713072a Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 11 Jul 2024 15:05:49 -0400 Subject: [PATCH 039/131] Bump dandischema to 0.10.2 (schema version 0.6.8) --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 4a988ff8d..e7db9cc0a 100644 --- a/setup.py +++ b/setup.py @@ -41,7 +41,7 @@ install_requires=[ 'celery', # Pin dandischema to exact version to make explicit which schema version is being used - 'dandischema==0.10.1', # schema version 0.6.7 + 'dandischema==0.10.2', # schema version 0.6.8 'django~=4.1.0', 'django-admin-display', # Require 0.58.0 as it is the first version to support postgres' native From e255828fe1838dd06acdb0dd1d10ea1778774132 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Thu, 11 Jul 2024 15:23:49 -0400 Subject: [PATCH 040/131] Fix draft version test factory Include contact person email in metadata --- dandiapi/api/tests/factories.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dandiapi/api/tests/factories.py b/dandiapi/api/tests/factories.py index ac0998590..7b3f3e09a 100644 --- a/dandiapi/api/tests/factories.py +++ b/dandiapi/api/tests/factories.py @@ -108,6 +108,7 @@ def metadata(self): { 'name': f'{faker.Faker().last_name()}, {faker.Faker().first_name()}', 'roleName': ['dcite:ContactPerson'], + 'email': faker.Faker().email(), 'schemaKey': 'Person', } ], From 359e70fe8395fdd14b66b0a53664c0a98251fe2b Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 15 Jul 2024 12:43:56 -0400 Subject: [PATCH 041/131] Remove redundant check against type field --- web/src/components/Meditor/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/Meditor/types.ts b/web/src/components/Meditor/types.ts index 5ca93b13f..0910a98c2 100644 --- a/web/src/components/Meditor/types.ts +++ b/web/src/components/Meditor/types.ts @@ -65,7 +65,7 @@ export const isJSONSchema = (schema: JSONSchemaUnionType): schema is JSONSchema7 export const isBasicSchema = (schema: JSONSchemaUnionType): schema is BasicSchema => ( isJSONSchema(schema) - && (isBasicType(schema.type) || schema.type === undefined) + && (isBasicType(schema.type)) ); export const isObjectSchema = (schema: JSONSchemaUnionType): schema is ObjectSchema => ( From 460da4d49e5510772f865ccaf71c4dacd0ad40a8 Mon Sep 17 00:00:00 2001 From: dandibot Date: Mon, 15 Jul 2024 18:50:00 +0000 Subject: [PATCH 042/131] auto shipit - CHANGELOG.md etc --- CHANGELOG.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75119ee38..21649c92d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,19 @@ +# v0.3.88 (Mon Jul 15 2024) + +#### 🐛 Bug Fix + +- Bump dandischema to 0.10.2 (schema version 0.6.8) [#1976](https://github.com/dandi/dandi-archive/pull/1976) ([@jjnesbitt](https://github.com/jjnesbitt)) +- Pin updated dependencies [#1977](https://github.com/dandi/dandi-archive/pull/1977) ([@jjnesbitt](https://github.com/jjnesbitt)) +- Suppress lint error (SIM103) [#1973](https://github.com/dandi/dandi-archive/pull/1973) ([@jjnesbitt](https://github.com/jjnesbitt)) +- Remove File to avoid confusion [#1972](https://github.com/dandi/dandi-archive/pull/1972) ([@yarikoptic](https://github.com/yarikoptic)) + +#### Authors: 2 + +- Jacob Nesbitt ([@jjnesbitt](https://github.com/jjnesbitt)) +- Yaroslav Halchenko ([@yarikoptic](https://github.com/yarikoptic)) + +--- + # v0.3.87 (Fri Jun 21 2024) #### 🐛 Bug Fix From b816ea2a0a4c6e1356e98b0f59db0a300c014ffc Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Tue, 4 Jun 2024 14:14:11 -0400 Subject: [PATCH 043/131] 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 044/131] 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 045/131] 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 046/131] 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 @@ - +