-
Notifications
You must be signed in to change notification settings - Fork 327
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
[ENG-2814] Allow Read-only and Read/Write contributors to view a project's draft registrations #10660
[ENG-2814] Allow Read-only and Read/Write contributors to view a project's draft registrations #10660
Changes from 5 commits
b91536e
1ae8f29
c0a70bb
b2e8268
f507af1
9037d7f
a2268f8
0d854bb
75e5d7d
3ebaba0
2f623d2
eea4c16
d8e9d0f
974dc34
59fdd3e
18dc873
6a45c15
bfdffe0
da8a4da
ae233ad
e961bf4
3897e7f
930e25d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
from api.base.pagination import DraftRegistrationContributorPagination | ||
from api.draft_registrations.permissions import ( | ||
DraftContributorDetailPermissions, | ||
IsContributorOrAdminContributor, | ||
DraftRegistrationPermission, | ||
IsAdminContributor, | ||
) | ||
from api.draft_registrations.serializers import ( | ||
|
@@ -50,7 +50,7 @@ def check_resource_permissions(self, resource): | |
|
||
class DraftRegistrationList(NodeDraftRegistrationsList): | ||
permission_classes = ( | ||
IsContributorOrAdminContributor, | ||
DraftRegistrationPermission, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the Permissions class actually does anything here. It only defines |
||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
) | ||
|
@@ -73,7 +73,7 @@ def get_queryset(self): | |
|
||
class DraftRegistrationDetail(NodeDraftRegistrationDetail, DraftRegistrationMixin): | ||
permission_classes = ( | ||
ContributorOrPublic, | ||
DraftRegistrationPermission, | ||
AdminDeletePermissions, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ | |
NodeCommentSerializer, | ||
) | ||
from api.draft_registrations.serializers import DraftRegistrationSerializer, DraftRegistrationDetailSerializer | ||
from api.draft_registrations.permissions import DraftRegistrationPermission | ||
from api.files.serializers import FileSerializer, OsfStorageFileSerializer | ||
from api.files import annotations as file_annotations | ||
from api.identifiers.serializers import NodeIdentifierSerializer | ||
|
@@ -75,7 +76,6 @@ | |
from api.nodes.filters import NodesFilterMixin | ||
from api.nodes.permissions import ( | ||
IsAdmin, | ||
IsAdminContributor, | ||
IsPublic, | ||
AdminOrPublic, | ||
WriteAdmin, | ||
|
@@ -625,7 +625,7 @@ class NodeDraftRegistrationsList(JSONAPIBaseView, generics.ListCreateAPIView, No | |
Use DraftRegistrationsList endpoint instead. | ||
""" | ||
permission_classes = ( | ||
IsAdminContributor, | ||
DraftRegistrationPermission, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't comment on the actually relevant line: i.e., the queryset should return We should probably get some additional clarification from Product about a couple of things: I'm happy to own this communication if you would like. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unaddressed. Let me know if you want a quick Zoom to clarify. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discussed the questions you raised with Mark.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for sorting this out! All of that being true, there's a bit of work left to do. Mostly, this endpoint ( Two fixes to get the correct behavior: Second, you'll need to update your Permission class to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I added some redundant-ish comments on tests to highlight where these requirements were being missed; we'll want/need to expand the test coverage, too) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some unaddressed comments here regarding the queryset. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I had missed this comment. I have updated the PR. |
||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
) | ||
|
@@ -659,9 +659,9 @@ class NodeDraftRegistrationDetail(JSONAPIBaseView, generics.RetrieveUpdateDestro | |
Use DraftRegistrationDetail endpoint instead. | ||
""" | ||
permission_classes = ( | ||
DraftRegistrationPermission, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given everything else I've said, I think the Detail views can go untouched for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need to update the permission of the detail view? |
||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
IsAdminContributor, | ||
) | ||
parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON,) | ||
|
||
|
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.
Making all permissions inheritable from the
branched_from
node can lead to some confusing situations down the line -- notably, when a Draft is Registered and suddenly a user who thought they had a certain permission level doesn't have those permissions anymore, but there are probably also other edge cases just because there are lots of skeletons in the registration workflow.There is a plan to some day kill off
DraftRegistrationContributors
(which were only created due to a confusion in requirements when No Project Registratsions were implemented).For now, let's constrain this PR to the specific issue identified in the ticket:
Users who view a Project's DraftRegistrations should see the Drafts on which they are contributors.