Skip to content

Commit

Permalink
Feature/ENG-6134 (#10951)
Browse files Browse the repository at this point in the history
## Purpose

Contributors added to a registration should be able to edit title. Admins should be able to edit contributors on registration page

## Changes

1. Common perform_destroy for NodeContributorDetail and RegistrationContributorDetail views moved to a separate NodeContributorRemoveMixin
2. Added has_permission method to ContributorDetailPermissions for RegistrationList so that contributors cannot do POST request (adding contributors)
3. Title field of Registration is not read only right now and added in WRITABLE_WHITELIST of Registration
4. Added tests

## Notes:

1. In RegistrationContributorsList.get_serializer_context I set default email = default assuming this is correct as we don't have a separate email template for registrations
2. I inherited from CreateModelMixin and DestroyModelMixin for some classes because didn't want to update base class as it may impact on v1 version

## Ticket

https://openscience.atlassian.net/jira/software/c/projects/ENG/boards/145?assignee=712020%3A7c7368dc-40cb-475f-bae8-b07a8bd2dd6c&selectedIssue=ENG-6134
  • Loading branch information
ihorsokhanexoft authored Feb 5, 2025
1 parent bd5ac9c commit d370e2b
Show file tree
Hide file tree
Showing 10 changed files with 322 additions and 44 deletions.
29 changes: 29 additions & 0 deletions api/base/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from django.core.exceptions import ObjectDoesNotExist
from django.db.models import QuerySet
from rest_framework import fields
from rest_framework import exceptions
from rest_framework.exceptions import NotFound
from rest_framework.reverse import reverse

Expand Down Expand Up @@ -300,3 +301,31 @@ def _view_by_flag(request, *args, **kwargs):
# in `api_tests.base.test_views` and `api.base.serializers.RelationshipField`
_view_by_flag.view_class = new_view.view_class # type: ignore[attr-defined]
return _view_by_flag


def update_contributors_permissions_and_bibliographic_status(serializer, instance, validated_data):
'''
Helper function for serializers to update permissions of contributors and their bibliographic status
'''
index = None
if '_order' in validated_data:
index = validated_data.pop('_order')

auth = Auth(serializer.context['request'].user)
node = serializer.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
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):
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
28 changes: 6 additions & 22 deletions api/nodes/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from api.base.utils import (
absolute_reverse, get_object_or_error,
get_user_auth, is_truthy,
update_contributors_permissions_and_bibliographic_status,
)
from api.base.versioning import get_kebab_snake_case_field
from api.institutions.utils import update_institutions
Expand Down Expand Up @@ -1268,28 +1269,11 @@ class NodeContributorDetailSerializer(NodeContributorsSerializer):
index = ser.IntegerField(required=False, read_only=False, source='_order')

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
return update_contributors_permissions_and_bibliographic_status(
self,
instance,
validated_data,
)


class NodeLinksSerializer(JSONAPISerializer):
Expand Down
15 changes: 7 additions & 8 deletions api/nodes/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -524,22 +524,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):
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
21 changes: 20 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 All @@ -30,6 +31,7 @@
ShowIfVersion, VersionedDateTimeField, ValuesListField,
HideIfWithdrawalOrWikiDisabled,
)
from api.base.utils import update_contributors_permissions_and_bibliographic_status
from api.institutions.utils import update_institutions
from framework.auth.core import Auth
from osf.exceptions import NodeStateError
Expand Down Expand Up @@ -83,7 +85,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)
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 +919,23 @@ def get_absolute_url(self, obj):
},
)

def update(self, instance, validated_data):
return update_contributors_permissions_and_bibliographic_status(
self,
instance,
validated_data,
)


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).
"""
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

0 comments on commit d370e2b

Please sign in to comment.