[Migration Required] Feature/NPD Add Preprints to Collections [PLAT-1190] #18
Conversation
- AddCollectionLinkedPreprintsRelationship view and CollectionLinkedPreprints view. - Allow collection to take preprints. - UI changes on MyProjects page to send requests to new API preprints collections endpoints.
There was a problem hiding this comment.
No blockers, just a few minor comments and thoughts. Feel free to address/respond to whatever you like @pattisdr, but I'd be fine merging this as-is. Thank you for fixing this (much larger than expected) issue and for the extensive test coverage! 🐼
api/collections/permissions.py
Outdated
| @@ -96,7 +96,7 @@ def has_object_permission(self, request, view, obj): | |||
| return False | |||
| pointer_nodes = [] | |||
| for pointer in request.data.get('data', []): | |||
| node = AbstractNode.load(pointer['id']) | |||
| node = AbstractNode.load(pointer['id']) or Preprint.load(pointer['id']) | |||
There was a problem hiding this comment.
Minor: wouldn't be a bad idea to generalize this block of code:
obj = AbstractNode.load(pointer['id']) or Preprint.load(pointer['id'])
if not obj:
raise NotFound(detail='Object of type {} with id "{}" was not found'.format(type(obj), pointer['id']))
| auth = get_user_auth(self.request) | ||
| obj = { | ||
| 'data': [ | ||
| pointer for pointer in self.collection_preprints(collection, auth.user) |
There was a problem hiding this comment.
Very minor: could be list(self.collection_preprints(...)
api/collections/views.py
Outdated
| @@ -194,7 +206,7 @@ def perform_destroy(self, instance): | |||
| instance.delete() | |||
|
|
|||
|
|
|||
| class CollectionDetail(JSONAPIBaseView, generics.RetrieveUpdateDestroyAPIView, CollectionMixin): | |||
| class CollectionDetail(JSONAPIBaseView, generics.RetrieveUpdateDestroyAPIView, CollectionMixin, CollectionPreprintsMixin): | |||
There was a problem hiding this comment.
Does CollectionDetail need CollectionPreprintsMixin? I can't see where collection_preprints() is used here or in a CollectionDetail subclass.
There was a problem hiding this comment.
Ah nvm, it's used in the CollectionDetailSerializer
There was a problem hiding this comment.
(not a blocker) What do you think about adding the collection_preprints() to the CollectionMixin instead of creating another mixin? In theory CollectionMixin could also have collection_nodes() and collection_registrations() helpers
| class Meta: | ||
| type_ = 'linked_preprints' | ||
|
|
||
| def make_instance_obj(self, obj): |
There was a problem hiding this comment.
Not a blocker and probably out of scope for this PR, but would be nice if make_instance_obj took a type argument and didn't need to be redefined in each class.
There was a problem hiding this comment.
Makes sense but I didn't do that here, because the deleted fields are different on nodes and preprints so I just redefined.
pattisdr
changed the title
…ize CollectionWriteOrPublicForRelationshipPointers.
Merge branch 'develop' into testmon-regression-test
Purpose
After NPD, preprints cannot be dragged into Bookmarks or user collections.
Changes
Prior to NPD, we weren't actually dragging preprints, but rather, the node the preprint was stored on. Several changes needed to collections endpoints to allow adding preprints to collections.
LinkedPreprintsList view
collections/<collection_id>/linked_preprints/CollectionLinkedPreprintsRelationship view
collections/<collection_id>/relationships/linked_preprints/linked_preprints relationship added to CollectionSerializer
Tweaks to MyProjects page so when preprints are dragged to collections or "Remove from collection" is clicked, requests are made to CollectionLinkedPreprintsRelationship to add/remove the preprint
Disable requests from being made when you double click on a preprint (projects load children, preprints don't have children)
Data migration added to allow preprints to be added to existing collections
Side effects
⭐️ Not familiar with existing collections - data migration allows preprints to be added to all existing collections, but is this correct? Are there some collections we should not modify?
⭐️ Since dragging "preprints" previously mean dragging the preprint's node, existing collections will have nodes in them that was actually intended to be the preprint. No way to sort out if they actually wanted the project there, or the preprint, but a user can find the preprint and re-add.
Ticket
https://openscience.atlassian.net/browse/PLAT-1190