Skip to content
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

Duplicate Publishable attributes in CommonModel + DOI in Dandiset as Optional #285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yarikoptic
Copy link
Member

That is necessary to later be able to make our model more restrictive and disallow addition of arbitrary attributes as it does now.

ATM any Draft version of a dandiset which was already published at least once, would have those attributes although base Dandiset (similar in Asset model; @candleindark would check) does not have them defined. This is what this PR fixes.

More details on how those were found and in which dandisets could be obtained from https://github.com/dandi/dandi-schema-status/blob/main/reports/diff_reports/dandiset/jsonschema_errs_summary.md#errs-diff-detailed-tables

This should be "backward compatible" change at the level of the schema, as any prior valid record would remain valid. Hence just boosting the "patch" portion of the version

…Optional

That is necessary to later be able to make our model more restrictive
and disallow addition of arbitrary attributes as it does now.

ATM any Draft version of a dandiset which was already published at least once,
would have those attributes although base Dandiset (similar in Asset model;
@candleindark would check) does not have them defined. This is what this PR
fixes.

More details on how those were found and in which dandisets could be obtained from
https://github.com/dandi/dandi-schema-status/blob/main/reports/diff_reports/dandiset/jsonschema_errs_summary.md#errs-diff-detailed-tables

This should be "backward compatible" change at the level of the schema,
as any prior valid record would remain valid. Hence just boosting the "patch"
portion of the version
Comment on lines +1584 to +1587
# Our current draft dandisets could be coming from Published ones,
# and thus carry their attributes. So we would need to define them here as well
# but make them optional, and `Publishable` would overload to make
# them mandatory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when published, the draft state should remove the published by component. or we need a better flow that allows owners to create a new draft state from the latest published record (instead of defaulting to creating one). originally the process mimicked github in that a release is a tag on a branch. we can evolve that to be a staged evolution. draft/unpublished -> published -> create draft/unpublished when any change is made by the owner. i.e. any modification creates a new staged state. when that happens irrelevant attributes can be removed.

but it can continue to have derivedFrom to indicate that this new unpublished state did come from a previous published state.

@satra
Copy link
Member

satra commented Feb 8, 2025

in addition to the general comment, i think we really need to spend a bit of focused time to revise the models in relation to linkml. we should arrange a time in the next few weeks with @candleindark @djarecka @yarikoptic and @satra (cc: @kabilar)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants