Add release notes functionality to dandiset publishing process #2606
Conversation
mvandenburgh
left a comment
mvandenburgh
left a comment
There was a problem hiding this comment.
We may want to rethink how we're passing release_notes around, but I'll get back to you on that. Aside from that, this generally looks good.
Note - we will need dandi/dandi-schema#185 to be released before this PR can be merged.
dandiapi/api/views/serializers.py
Outdated
| def get_release_notes(self, version: Version) -> str | None: | ||
| """Extract release notes from version metadata.""" | ||
| return version.metadata.get('releaseNotes') | ||
|
|
||
| def to_representation(self, instance): | ||
| """Remove release_notes from representation if it's None.""" | ||
| representation = super().to_representation(instance) | ||
| if representation.get('release_notes') is None: | ||
| representation.pop('release_notes', None) | ||
| return representation | ||
|
|
There was a problem hiding this comment.
We should be able to access the releaseNotes directly from the metadata in the frontend, which would remove the need for this extra logic in this serializer. To do that, we'll first need dandi/dandi-schema#185 to be released so we can update https://github.com/dandi/dandi-archive/blob/master/web/src/types/schema.ts.
dandiapi/api/tasks/__init__.py
Outdated
|
|
||
| @shared_task | ||
| def publish_dandiset_task(dandiset_id: int, user_id: int): | ||
| def publish_dandiset_task(dandiset_id: int, user_id: int, release_notes: str | None = None): |
There was a problem hiding this comment.
I'm not sure about the implications of accepting an arbitrary string as a Celery task argument w.r.t. serialization/deserialization from RabbitMQ. Normally, we try to avoid this and store such values in the DB for the Celery task to query for itself, passing only primary keys to the tasks themselves. On the other hand, I don't see a natural way for release_notes to fit into that paradigm, so maybe this is the best solution. I'll look into this
dandiapi/api/views/serializers.py
Outdated
|
|
||
|
|
||
| class PublishVersionSerializer(serializers.Serializer): | ||
| release_notes = serializers.CharField(required=False, allow_blank=True, source='releaseNotes') |
There was a problem hiding this comment.
We should set a max_length here.
|
Thanks for looking a this, @mvandenburgh! |
mvandenburgh
force-pushed
the
enh/add-release-notes
branch
3 times, most recently
from
b2e86d8 to
268c4c9
Compare
|
I rebased this on |
mvandenburgh
force-pushed
the
enh/add-release-notes
branch
2 times, most recently
from
6427e5e to
7757d45
Compare
mvandenburgh
force-pushed
the
enh/add-release-notes
branch
2 times, most recently
from
9472b1e to
fb5e66e
Compare
- Updated publish functions to accept optional release notes. - Introduced PublishVersionSerializer for handling release notes in requests. - Enhanced UI to allow users to input release notes during publishing. - Display release notes for published versions in the UI.
This is unnecessary, as we can simply set the field to `null` to simplify the response type interface
This has the benefits of not having to pass `release_notes` around through multiple nested function calls + trusting celery's python string (de)serialization functionality (since we were placing the entire `release_notes` string onto the queue before).
- Remove `test_version_publish_without_release_notes` test. This is just testing the existing publish functionality, which we already have tests for. - Fix `test_version_build_publishable_with_release_notes` so that it passes - Remove `test_version_rest_publish_empty_release_notes` test, as it is no longer applicable. `Version.release_notes` is no longer a nullable field, instead defaulting to empty string (best practice for CharField)
jjnesbitt
force-pushed
the
enh/add-release-notes
branch
from
349810d to
1a5118d
Compare
fix #189
Screen.Recording.2025-10-20.at.11.25.55.AM.mov
Following dandi/dandi-schema#185
Requires dandi/dandi-schema#185 to be released