Add Dandiset star functionality with UI components #2123
Conversation
- Introduced DandisetStar model and admin interface for managing starred Dandisets. - Implemented star/unstar actions in the DandisetViewSet and corresponding API endpoints. - Added star count and starred status to DandisetSerializer. - Created StarButton component for toggling star status in the UI. - Developed StarredDandisetsView to display Dandisets starred by the user. - Updated DandisetList and DandisetLandingView to include star functionality. - Enhanced DandisetFilterBackend to support ordering by star count.
- Updated test cases in test_dandiset.py and test_version.py to include 'star_count' and 'is_starred' fields in the expected responses. - Ensured consistency across various test scenarios for Dandiset and Version APIs regarding star functionality.
- Implemented new test cases in `test_dandiset.py` to verify star/unstar actions, star count, and starred status for Dandisets. - Updated API responses in `DandisetViewSet` to return the current star count when starring or unstarring a Dandiset. - Added tests for unauthenticated access to star functionality and listing starred Dandisets.
…ionality - Added 'star_count' and 'is_starred' fields to the Dandiset interface in types. - Updated StarredDandisetsView to process and display starred Dandisets with their most recent published version. - Refactored data handling in StarredDandisetsView to ensure proper mapping of Dandiset data.
Updated the way Dandiset properties are managed by creating a shallow copy of the Dandiset object and removing 'most_recent_published_version' and 'draft_version' before returning the data.
Simplified the mapping of Dandiset objects by directly returning the original Dandiset instead of creating a shallow copy and removing specific properties. This change improves code clarity and maintains the integrity of the Dandiset data structure.
There was a problem hiding this comment.
Very interesting!
I'm a dinosaur who's not ready to trust AI-generated code just yet, so I'll want to get careful reviews from @jjnesbitt and @mvandenburgh (both to identify problems with the code, and so we have team members with a mental model of how it works) before merging. Furthermore, I think this will benefit from some enhancements (like using a service layer to manage stars), as well as some minor stuff that I can push fixes for myself. Finally, I would like for @jtomeck to help out with the frontend design here as well.
Thanks @bendichter for kicking this feature off!
|
@waxlamp yes, I mentioned Cursor because I agree that this will need careful eyes on it before integrating. But looking at the changes the majority of them seem straightforward |
|
ok yeah the npm-related issues are my fault. I am used to installing with npm and having large automatically generated files as a result of that. I'll make sure to use yarn in the future. |
- Removed legacy star/unstar methods from the Dandiset model. - Introduced new star_dandiset and unstar_dandiset service functions to handle starring and unstarring Dandisets, including transaction management and star count updates. - Updated DandisetViewSet to integrate star/unstar actions, returning the updated star count in API responses. - Enhanced the Dandiset model to support star functionality, ensuring proper user authentication checks.
|
I don't know what to do about the remaining CI check failures |
There was a problem hiding this comment.
Thanks for the work, @bendichter. I've done a first pass of this PR and identified some necessary changes, but nonetheless it's great to have the outline of this feature out of the way.
I'd be happy to address some of these changes myself, if that makes more sense. I'll still leave the comments either way.
| operation_description='Star a dandiset. User must be authenticated.', | ||
| ) | ||
| @action(methods=['POST'], detail=True) | ||
| def star(self, request, dandiset__pk) -> Response: |
There was a problem hiding this comment.
I think this PR should be merged after #2115, and one of the reasons is that this PR could take advantage of the new permission service layer. For example, a decorator that requires authentication could be used, similar to the require_dandiset_owner_or_403 decorator introduced in that PR.
Co-authored-by: Jacob Nesbitt <[email protected]>
Co-authored-by: Jacob Nesbitt <[email protected]>
Co-authored-by: Jacob Nesbitt <[email protected]>
|
@jjnesbitt thanks for the prompt and thorough review. I can confirm that I can replicate those UI bugs. I might have time to address them myself but feel free to take this over if you have the bandwidth to do so. |
I think I have the bandwidth, so today I can make a pass at changing someone of the things I mentioned. |
jjnesbitt
force-pushed
the
stars
branch
2 times, most recently
from
a62f371 to
c9d8c21
Compare
To fix this behavior for the dandiset list endpoint, create a new method on the DandisetViewSet class, `_get_dandiset_star_context`, which collects dandiset star information. This information is then passed to the DandisetListSerializer as context.
| serializer = VersionDetailSerializer( | ||
| instance=version, context=self.get_serializer_context() | ||
| ) |
There was a problem hiding this comment.
@mvandenburgh @waxlamp I don't love this solution, but it works. I was curious how the other serializers work without this (e.g. the Dandiset retrieve endpoint), and it turns out the DRFViewSet class we inherit from (specifically ReadOnlyModelViewSet -> GenericViewSet -> generics.GenericAPIView) automatically populate this context when the get_serializer method is called. This means that on all untouched methods we inherit and use, it's populated, while on any methods we've overwritten, it's missing (unless we explicitly specify it, like it done here).
To me this suggests that we should do an overarching refactor of our view and serialization code, since I think we have a lot of technical debt there. But for now, this is what seems to be the easiest change that's in scope.
unique_together is deprecated
For posterity: We met offline and agreed to exclude dandiset stars from audit events. |
fix #1119
Feature developed with Cursor
I haven't been able to log in on my dev server so I can't test that behavior, but the unlogged in web UI and the admin interface appear to work as expected