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

Feature/ENG-6134 #10951

Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions api/nodes/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,15 @@ class ContributorDetailPermissions(permissions.BasePermission):
def load_resource(self, context, view):
return AbstractNode.load(context[view.node_lookup_url_kwarg])

def has_permission(self, request, view):
brianjgeiger marked this conversation as resolved.
Show resolved Hide resolved
auth = get_user_auth(request)
context = request.parser_context['kwargs']
resource = self.load_resource(context, view)
if request.method == 'POST' and not resource.has_permission(auth.user, osf_permissions.ADMIN):
return False

return super().has_permission(request, view)

def has_object_permission(self, request, view, obj):
assert_resource_type(obj, self.acceptable_models)
auth = get_user_auth(request)
Expand Down
15 changes: 7 additions & 8 deletions api/nodes/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -521,22 +521,21 @@ class NodeContributorDetail(BaseContributorDetail, generics.RetrieveUpdateDestro
def get_resource(self):
return self.get_node()

# overrides DestroyAPIView
def get_serializer_context(self):
context = JSONAPIBaseView.get_serializer_context(self)
context['resource'] = self.get_resource()
context['default_email'] = 'default'
return context

def perform_destroy(self, instance):
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said above don't bother using a mixin, it's okay to repeat yourself a little across views. It's just easier to see new behavior when you have a small number of methods for a view.

node = self.get_resource()
auth = get_user_auth(self.request)
if len(node.visible_contributors) == 1 and instance.visible:
if node.visible_contributors.count() == 1 and instance.visible:
raise ValidationError('Must have at least one visible contributor')
removed = node.remove_contributor(instance, auth)
if not removed:
raise ValidationError('Must have at least one registered admin contributor')

def get_serializer_context(self):
context = JSONAPIBaseView.get_serializer_context(self)
context['resource'] = self.get_resource()
context['default_email'] = 'default'
return context


class NodeImplicitContributorsList(JSONAPIBaseView, generics.ListAPIView, ListFilterMixin, NodeMixin):
permission_classes = (
Expand Down
37 changes: 36 additions & 1 deletion api/registrations/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
NodeLinksSerializer,
NodeLicenseSerializer,
NodeContributorsSerializer,
NodeContributorsCreateSerializer,
RegistrationProviderRelationshipField,
get_license_details,
)
Expand Down Expand Up @@ -83,7 +84,7 @@ class RegistrationSerializer(NodeSerializer):

ia_url = ser.URLField(read_only=True)
reviews_state = ser.CharField(source='moderation_state', read_only=True)
title = ser.CharField(read_only=True)
title = ser.CharField(required=False)
brianjgeiger marked this conversation as resolved.
Show resolved Hide resolved
description = ser.CharField(required=False, allow_blank=True, allow_null=True)
category_choices = NodeSerializer.category_choices
category_choices_string = NodeSerializer.category_choices_string
Expand Down Expand Up @@ -917,6 +918,40 @@ def get_absolute_url(self, obj):
},
)

def update(self, instance, validated_data):
index = None
if '_order' in validated_data:
index = validated_data.pop('_order')

auth = Auth(self.context['request'].user)
node = self.context['resource']

if 'bibliographic' in validated_data:
bibliographic = validated_data.get('bibliographic')
else:
bibliographic = node.get_visible(instance.user)
permission = validated_data.get('permission') or instance.permission
try:
if index is not None:
node.move_contributor(instance.user, auth, index, save=True)
node.update_contributor(instance.user, permission, bibliographic, auth, save=True)
except node.state_error as e:
raise exceptions.ValidationError(detail=str(e))
except ValueError as e:
raise exceptions.ValidationError(detail=str(e))
instance.refresh_from_db()
return instance
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you extract this into a helper function and call that function from both places it's used? It's significant enough functionality that it would be nice to have it DRY. Maybe put it in api/base/utils.py

Copy link
Author

Choose a reason for hiding this comment

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

Agree. Followed John's recommendations it's not always required to follow DRY and no need to create a separate mixin



class RegistrationContributorsCreateSerializer(NodeContributorsCreateSerializer, RegistrationContributorsSerializer):
"""
Overrides RegistrationContributorsSerializer to add email, full_name, send_email, and non-required index and users field.

id and index redefined because of the two serializers we've inherited
"""
id = IDField(source='_id', required=False, allow_null=True)
index = ser.IntegerField(required=False, source='_order')


class RegistrationFileSerializer(OsfStorageFileSerializer):

Expand Down
53 changes: 47 additions & 6 deletions api/registrations/views.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from rest_framework import generics, permissions as drf_permissions
from rest_framework import generics, mixins, permissions as drf_permissions
from rest_framework.exceptions import ValidationError, NotFound, PermissionDenied
from framework.auth.oauth_scopes import CoreScopes

Expand Down Expand Up @@ -58,6 +58,7 @@
RegistrationSerializer,
RegistrationDetailSerializer,
RegistrationContributorsSerializer,
RegistrationContributorsCreateSerializer,
RegistrationCreateSerializer,
RegistrationStorageProviderSerializer,
)
Expand Down Expand Up @@ -267,7 +268,7 @@ def get_serializer_context(self):
return context


class RegistrationContributorsList(BaseContributorList, RegistrationMixin, UserMixin):
class RegistrationContributorsList(BaseContributorList, mixins.CreateModelMixin, RegistrationMixin, UserMixin):
"""The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/registrations_contributors_list).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good use of mixin. this class looks correct.

"""
view_category = 'registrations'
Expand All @@ -282,16 +283,33 @@ class RegistrationContributorsList(BaseContributorList, RegistrationMixin, UserM
permission_classes = (
ContributorDetailPermissions,
drf_permissions.IsAuthenticatedOrReadOnly,
ReadOnlyIfRegistration,
base_permissions.TokenHasScope,
)

def get_resource(self):
return self.get_node(check_object_permissions=False)

def get_serializer_class(self):
if self.request.method == 'POST':
return RegistrationContributorsCreateSerializer

return self.serializer_class

def post(self, request, *args, **kwargs):
return super().create(request, *args, **kwargs)

def get_default_queryset(self):
node = self.get_node(check_object_permissions=False)
node = self.get_resource()
return node.contributor_set.all().prefetch_related('user__guids')

def get_serializer_context(self):
context = super().get_serializer_context()
context['resource'] = self.get_resource()
context['default_email'] = 'default'
return context


class RegistrationContributorDetail(BaseContributorDetail, RegistrationMixin, UserMixin):
class RegistrationContributorDetail(BaseContributorDetail, mixins.UpdateModelMixin, mixins.DestroyModelMixin, RegistrationMixin, UserMixin):
"""The documentation for this endpoint can be found [here](https://developer.osf.io/#operation/registrations_contributors_read).
"""
view_category = 'registrations'
Expand All @@ -304,10 +322,33 @@ class RegistrationContributorDetail(BaseContributorDetail, RegistrationMixin, Us
permission_classes = (
ContributorDetailPermissions,
drf_permissions.IsAuthenticatedOrReadOnly,
ReadOnlyIfRegistration,
base_permissions.TokenHasScope,
)

def get_resource(self):
return self.get_node()

def patch(self, request, *args, **kwargs):
return super().partial_update(request, *args, **kwargs)

def delete(self, request, *args, **kwargs):
return super().destroy(request, *args, **kwargs)

def get_serializer_context(self):
context = super().get_serializer_context()
context['resource'] = self.get_resource()
context['default_email'] = 'default'
return context

def perform_destroy(self, instance):
node = self.get_resource()
auth = get_user_auth(self.request)
if node.visible_contributors.count() == 1 and instance.visible:
raise ValidationError('Must have at least one visible contributor')
removed = node.remove_contributor(instance, auth)
if not removed:
raise ValidationError('Must have at least one registered admin contributor')


class RegistrationBibliographicContributorsList(NodeBibliographicContributorsList, RegistrationMixin):

Expand Down
6 changes: 4 additions & 2 deletions api_tests/registrations/views/test_registration_detail.py
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ def test_fields(
assert res.json['data']['attributes']['category'] == 'instrumentation'
assert res.json['data']['attributes']['description'] == description
assert res.json['data']['attributes']['tags'] == tags
assert res.json['data']['attributes']['title'] == private_registration.title
assert res.json['data']['attributes']['title'] == 'New title'
assert res.json['data']['attributes']['node_license']['copyright_holders'] == copyright_holders
assert res.json['data']['attributes']['node_license']['year'] == year
assert res.json['data']['attributes']['custom_citation'] == custom_citation
Expand Down Expand Up @@ -527,7 +527,7 @@ def test_fields(
assert res.json['data']['attributes']['category'] == ''
assert res.json['data']['attributes']['description'] == ''
assert res.json['data']['attributes']['tags'] == []
assert res.json['data']['attributes']['title'] == private_registration.title
assert res.json['data']['attributes']['title'] == 'New title'
assert res.json['data']['attributes']['node_license']['copyright_holders'] == []
assert res.json['data']['attributes']['node_license']['year'] == ''
assert res.json['data']['attributes']['custom_citation'] == ''
Expand Down Expand Up @@ -577,6 +577,7 @@ def test_turning_private_registrations_public(

def test_registration_fields_are_read_only(self):
writeable_fields = [
'title',
'type',
'public',
'draft_registration',
Expand All @@ -600,6 +601,7 @@ def test_registration_fields_are_read_only(self):

def test_registration_detail_fields_are_read_only(self):
writeable_fields = [
'title',
'type',
'public',
'draft_registration',
Expand Down
Loading
Loading