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

[ENG-6445] [ENG-6457] Preprints Version Creation and API Implementation - Part 2 #10807

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
37 changes: 27 additions & 10 deletions api/preprints/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
from api.base.metrics import MetricsSerializerMixin
from api.institutions.utils import update_institutions_if_user_associated
from api.taxonomies.serializers import TaxonomizableSerializerMixin
from framework.exceptions import PermissionsError
from framework.exceptions import PermissionsError, PendingVersionExists
from website.project import signals as project_signals
from osf.exceptions import NodeStateError, PreprintStateError
from osf.models import (
Expand Down Expand Up @@ -142,6 +142,7 @@ class PreprintSerializer(TaxonomizableSerializerMixin, MetricsSerializerMixin, J
)
reviews_state = ser.CharField(source='machine_state', read_only=True, max_length=15)
date_last_transitioned = NoneIfWithdrawal(VersionedDateTimeField(read_only=True))
version = ser.IntegerField(read_only=True)

citation = NoneIfWithdrawal(
RelationshipField(
Expand Down Expand Up @@ -221,6 +222,7 @@ class PreprintSerializer(TaxonomizableSerializerMixin, MetricsSerializerMixin, J
'self': 'get_preprint_url',
'html': 'get_absolute_html_url',
'doi': 'get_article_doi_url',
'preprint_versions': 'get_preprint_versions',
'preprint_doi': 'get_preprint_doi_url',
},
)
Expand Down Expand Up @@ -253,11 +255,21 @@ def subjects_self_view(self):
# Overrides TaxonomizableSerializerMixin
return 'preprints:preprint-relationships-subjects'

def get_preprint_versions(self, obj):
return absolute_reverse(
'preprints:preprint-versions',
kwargs={
'preprint_id': obj._id,
'version': self.context['request'].parser_context['kwargs']['version'],
},
)

def get_preprint_url(self, obj):
return absolute_reverse(
'preprints:preprint-detail', kwargs={
'preprint_id': obj._id, 'version':
self.context['request'].parser_context['kwargs']['version'],
'preprints:preprint-detail',
kwargs={
'preprint_id': obj._id,
'version': self.context['request'].parser_context['kwargs']['version'],
},
)

Expand Down Expand Up @@ -559,21 +571,26 @@ def create(self, validated_data):

title = validated_data.pop('title')
description = validated_data.pop('description', '')
# preprint = Preprint(provider=provider, title=title, creator=creator, description=description)
# preprint.save()
preprint = Preprint.create(provider=provider, title=title, creator=creator, description=description)

return self.update(preprint, validated_data)


class PreprintCreateVersionSerializer(PreprintCreateSerializer):
dupliate_from_guid = ser.CharField(required=True, write_only=True)
class PreprintCreateVersionSerializer(PreprintSerializer):
# Overrides PreprintSerializer to make id and title nullable
id = IDField(source='_id', required=False, allow_null=True)
title = ser.CharField(required=False)

dupliate_from_guid = ser.CharField(required=True, write_only=True)
cslzchen marked this conversation as resolved.
Show resolved Hide resolved

def create(self, validated_data):
dupliate_from_guid = validated_data.pop('dupliate_from_guid', None)
preprint, update_data = Preprint.create_version(dupliate_from_guid)

auth = get_user_auth(self.context['request'])
try:
preprint, update_data = Preprint.create_version(dupliate_from_guid, auth)
except PendingVersionExists:
raise Conflict(detail='Before creating a new version, you must publish the latest version.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reminder for myself: need to check with FE if this detail is surfaced back directly to the user; if so, we need Product to provide the exact language; if not, we may not need to provide the detail.

# TODO add more checks
return self.update(preprint, update_data)


Expand Down
2 changes: 1 addition & 1 deletion api/preprints/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

urlpatterns = [
re_path(r'^$', views.PreprintList.as_view(), name=views.PreprintList.view_name),
re_path(r'^(?P<preprint_id>[A-Za-z0-9_]+)/versions/$', views.ListCreatePreprintVersion.as_view(), name=views.ListCreatePreprintVersion.view_name),
re_path(r'^(?P<preprint_id>[A-Za-z0-9_]+)/versions/$', views.PreprintVersionsList.as_view(), name=views.PreprintVersionsList.view_name),
re_path(r'^(?P<preprint_id>[A-Za-z0-9_]+)/$', views.PreprintDetail.as_view(), name=views.PreprintDetail.view_name),
re_path(r'^(?P<preprint_id>[A-Za-z0-9_]+)/bibliographic_contributors/$', views.PreprintBibliographicContributorsList.as_view(), name=views.PreprintBibliographicContributorsList.view_name),
re_path(r'^(?P<preprint_id>[A-Za-z0-9_]+)/citation/$', views.PreprintCitationDetail.as_view(), name=views.PreprintCitationDetail.view_name),
Expand Down
24 changes: 23 additions & 1 deletion api/preprints/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,29 @@ def get_annotated_queryset_with_metrics(self, queryset, metric_class, metric_nam
)


class ListCreatePreprintVersion(PreprintList):
class PreprintVersionsList(PreprintMetricsViewMixin, JSONAPIBaseView, generics.ListCreateAPIView, PreprintFilterMixin):
# These permissions are not checked for the list of preprints, permissions handled by the query
permission_classes = (
drf_permissions.IsAuthenticatedOrReadOnly,
base_permissions.TokenHasScope,
ContributorOrPublic,
)
Comment on lines +168 to +173
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reminder for myself: it seems neither the create nor the list checks the permission ... if the comment is correct ... need to double check ... I think these are still checked but just not strict enough, which finer-grained access control are done in get_queryset().


parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON)

required_read_scopes = [CoreScopes.PREPRINTS_READ]
required_write_scopes = [CoreScopes.PREPRINTS_WRITE]

serializer_class = PreprintSerializer

ordering = ('-created')
ordering_fields = ('created', 'date_last_transitioned')
view_category = 'preprints'
view_name = 'preprint-versions'
metric_map = {
'downloads': PreprintDownload,
'views': PreprintView,
}

def get_serializer_class(self):
if self.request.method == 'POST':
Expand Down
3 changes: 3 additions & 0 deletions framework/exceptions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,6 @@ class TemplateHTTPError(HTTPError):
def __init__(self, code, message=None, redirect_url=None, data=None, template=None):
self.template = template
super().__init__(code, message, redirect_url, data)

class PendingVersionExists(FrameworkError):
pass
cslzchen marked this conversation as resolved.
Show resolved Hide resolved
20 changes: 13 additions & 7 deletions osf/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -494,18 +494,24 @@ def version(self):

# Override load in order to load by Versioned GUID
@classmethod
def load(cls, data, select_for_update=False):
def load(cls, guid, select_for_update=False):
try:
base_guid = data.split(VersionedGuidMixin.GUID_VERSION_DELIMITER)[0]
version = data.split(VersionedGuidMixin.GUID_VERSION_DELIMITER)[1] if len(data.split(VersionedGuidMixin.GUID_VERSION_DELIMITER)) > 1 else None
guid_split_list = guid.split(VersionedGuidMixin.GUID_VERSION_DELIMITER)
base_guid = guid_split_list[0]
version = guid_split_list[1] if len(guid_split_list) > 1 else None
if version:
return cls.objects.get(versioned_guids__guid___id=base_guid, versioned_guids__version=version) if not select_for_update else cls.objects.filter(
versioned_guids__guid___id=base_guid, versioned_guids__version=version).select_for_update().get()
if not select_for_update:
return cls.objects.get(versioned_guids__guid___id=base_guid, versioned_guids__version=version)
return cls.objects.filter(versioned_guids__guid___id=base_guid, versioned_guids__version=version).select_for_update().get()

if not select_for_update:
return cls.objects.filter(versioned_guids__guid___id=base_guid).order_by('-versioned_guids__version').first()
return cls.objects.filter(versioned_guids__guid___id=guid).order_by('-versioned_guids__version').select_for_update().get()

return cls.objects.filter(versioned_guids__guid___id=base_guid).order_by('-versioned_guids__version').first() if not select_for_update else cls.objects.filter(
versioned_guids__guid___id=data).order_by('-versioned_guids__version').select_for_update().get()
except cls.DoesNotExist:
return None
except cls.MultipleObjectsReturned:
return None
Comment on lines +513 to +514
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future improvements: let's return None for now, but we may need to add some sentry log to detect this and maybe re-raise the exception afterwards.


_primary_key = _id

Expand Down
4 changes: 2 additions & 2 deletions osf/models/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ class AffiliatedInstitutionMixin(models.Model):

affiliated_institutions = models.ManyToManyField('Institution', related_name='nodes')

def add_affiliated_institution(self, inst, user, log=True):
if not user.is_affiliated_with_institution(inst):
def add_affiliated_institution(self, inst, user, log=True, ignore_user_affiliation=False):
if not user.is_affiliated_with_institution(inst) and not ignore_user_affiliation:
raise UserNotAffiliatedError(f'User is not affiliated with {inst.name}')
if not self.is_affiliated_with_institution(inst):
self.affiliated_institutions.add(inst)
Expand Down
38 changes: 34 additions & 4 deletions osf/models/preprint.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from django.db.models.signals import post_save

from framework.auth import Auth
from framework.exceptions import PermissionsError
from framework.exceptions import PermissionsError, PendingVersionExists
from framework.auth import oauth_scopes
from rest_framework.exceptions import NotFound

Expand Down Expand Up @@ -297,15 +297,31 @@ def create(cls, provider, title, creator, description):

return preprint

def has_create_version_permission(self, user):
from guardian.shortcuts import get_group_perms
object_type = self.guardian_object_type

if not user or user.is_anonymous:
return False
perm = f'{ADMIN}_{object_type}'
# Using get_group_perms to get permissions that are inferred through
# group membership - not inherited from superuser status
has_permission = perm in get_group_perms(user, self)

return has_permission

@classmethod
def create_version(cls, dupliate_from_guid):
def create_version(cls, dupliate_from_guid, auth):
source_preprint = cls.load(dupliate_from_guid.split(cls.GUID_VERSION_DELIMITER)[0])
if not source_preprint:
raise NotFound
if not source_preprint.has_create_version_permission(auth.user):
cslzchen marked this conversation as resolved.
Show resolved Hide resolved
raise PermissionsError('You must have admin permissions to create new version.')
if not source_preprint.is_published:
raise PendingVersionExists

base_guid = Guid.load(dupliate_from_guid.split(cls.GUID_VERSION_DELIMITER)[0])
last_version = base_guid.versions.order_by('-version').first().version

data_for_update = {}
data_for_update['tags'] = source_preprint.tags.all().values_list('name', flat=True)
data_for_update['license_type'] = source_preprint.license.node_license
Expand All @@ -315,18 +331,32 @@ def create_version(cls, dupliate_from_guid):
data_for_update['custom_publication_citation'] = source_preprint.custom_publication_citation
data_for_update['article_doi'] = source_preprint.article_doi

data_for_update['has_coi'] = source_preprint.has_coi
data_for_update['conflict_of_interest_statement'] = source_preprint.conflict_of_interest_statement
data_for_update['has_data_links'] = source_preprint.has_data_links
data_for_update['why_no_data'] = source_preprint.why_no_data
data_for_update['data_links'] = source_preprint.data_links
data_for_update['has_prereg_links'] = source_preprint.has_prereg_links
data_for_update['why_no_prereg'] = source_preprint.why_no_prereg
data_for_update['prereg_links'] = source_preprint.prereg_links

if source_preprint.node:
data_for_update['node'] = source_preprint.node

preprint = cls(
provider=source_preprint.provider,
title=source_preprint.title,
creator=source_preprint.creator,
creator=auth.user,
description=source_preprint.description,
)
preprint.save()
# add contributors
for contributor_info in source_preprint.contributor_set.exclude(user=source_preprint.creator).values('visible', 'user_id', '_order'):
preprint.contributor_set.create(**{**contributor_info, 'preprint_id': preprint.id})

# add affiliated institutions
for institution in source_preprint.affiliated_institutions.all():
preprint.add_affiliated_institution(institution, auth.user, ignore_user_affiliation=True)

base_guid.referent = preprint
base_guid.object_id = preprint.pk
Expand Down
Loading