-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API support for Embargoed Zarrs #2069
base: master
Are you sure you want to change the base?
Conversation
- Remove EmbargoedZarrArchive model - Remove BaseZarrArchive abstract model - Add ZarrArchive.embargoed column - Fix imports and model usage
@@ -162,6 +162,13 @@ def is_blob(self): | |||
def is_zarr(self): | |||
return self.zarr is not None | |||
|
|||
@property | |||
def is_embargoed(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pylance shows the return type here as (bool | Any)
for me, I assume due to it not being able to infer the type of self.zarr.embargoed
.
def is_embargoed(self): | |
def is_embargoed(self) -> bool: |
if ( | ||
asset_blob is not None | ||
asset_blob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for removing the is not None
condition here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, i'll revert that.
# Generated by Django 4.1.13 on 2024-08-21 16:06 | ||
from __future__ import annotations | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
dependencies = [ | ||
('zarr', '0003_alter_embargoedzarrarchive_options_and_more'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='zarrarchive', | ||
name='embargoed', | ||
field=models.BooleanField(default=False), | ||
), | ||
migrations.DeleteModel( | ||
name='EmbargoedZarrArchive', | ||
), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to migrate the existing EmbargoedZarrArchive
objects to the ZarrArchive
table before deleting the EmbargoedZarrArchive
table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no existing EmbargoedZarrArchive
s, and aren't supported in any real way even if they did exist. So I think this achieves what we want.
@@ -106,6 +106,7 @@ def create_zarr(*, dandiset: Dandiset, user: User, zarr_archive: ZarrArchive) -> | |||
details = { | |||
'zarr_id': str(zarr_archive.zarr_id), | |||
'name': zarr_archive.name, | |||
'embargoed': dandiset.embargoed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waxlamp does this make sense? I figured it's good to store this since I don't know if it's readily available on the zarr otherwise.
This PR adds API support for embargoed zarrs, with the following notable changes:
ZarrArchive
,EmbargoedZarrArchive
, andBaseZarrArchive
have all been merged into oneZarrArchive
model.Asset
model now has anis_embargoed
property, which will account for both asset blob and zarr embargo status.unembargo_dandiset
function has been extended to include zarrs.404 Not Found
is returned.This does not address any changes in the GUI, as I'm unsure what exactly that entails. These changes are necessary for embargoed zarr support in any case, and so can be merged before any GUI re-work.