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-6668] Add Contributor Page Improvements for Institutional Access #10825

Open
wants to merge 9 commits into
base: feature/institutional_access
Choose a base branch
from
2 changes: 1 addition & 1 deletion api/requests/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def has_permission(self, request, view):
if not institution.institutional_request_access_enabled:
raise exceptions.PermissionDenied({'institution': 'Institutional request access is not enabled.'})

if get_user_auth(request).user.is_institutional_admin(institution):
if get_user_auth(request).user.is_institutional_admin_or_curator(institution):
return True
else:
raise exceptions.PermissionDenied({'institution': 'You do not have permission to perform this action for this institution.'})
Expand Down
2 changes: 1 addition & 1 deletion api/users/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,6 @@ def has_permission(self, request, view) -> bool:

message_type = request.data.get('message_type')
if message_type == MessageTypes.INSTITUTIONAL_REQUEST:
return user.is_institutional_admin(institution) and institution.institutional_request_access_enabled
return user.is_institutional_admin_or_curator(institution) and institution.institutional_request_access_enabled
else:
raise exceptions.ValidationError('Not valid message type.')
2 changes: 1 addition & 1 deletion api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ def create(self, validated_data: Dict[str, Any]) -> UserMessage:
'institution',
)

if not sender.is_institutional_admin(institution):
if not sender.is_institutional_admin_or_curator(institution):
raise Conflict({'sender': 'Only institutional administrators can create messages.'})

if not recipient.is_affiliated_with_institution(institution):
Expand Down
67 changes: 67 additions & 0 deletions api_tests/nodes/views/test_node_contributor_insti_admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import pytest
from osf.models import Contributor
from osf_tests.factories import (
AuthUserFactory,
ProjectFactory,
InstitutionFactory,
)
from api.base.settings.defaults import API_BASE


@pytest.mark.django_db
class TestChangeInstitutionalAdminContributor:

@pytest.fixture()
def project_admin(self):
return AuthUserFactory()

@pytest.fixture()
def project_admin2(self):
return AuthUserFactory()

@pytest.fixture()
def institution(self):
return InstitutionFactory()

@pytest.fixture()
def institutional_admin(self, institution):
admin_user = AuthUserFactory()
institution.get_group('institutional_admins').user_set.add(admin_user)
return admin_user

@pytest.fixture()
def project(self, project_admin, project_admin2, institutional_admin):
project = ProjectFactory(creator=project_admin)
project.add_contributor(project_admin2, permissions='admin', visible=False)
project.add_contributor(institutional_admin, visible=False)
return project

@pytest.fixture()
def url_contrib(self, project, institutional_admin):
return f'/{API_BASE}nodes/{project._id}/contributors/{institutional_admin._id}/'

def test_cannot_set_institutional_admin_contributor_bibliographic(self, app, project_admin, project,
institutional_admin, url_contrib):
res = app.put_json_api(
url_contrib,
{
'data': {
'id': f'{project._id}-{institutional_admin._id}',
'type': 'contributors',
'attributes': {
'bibliographic': True,
}
}
},
auth=project_admin.auth,
expect_errors=True
)

assert res.status_code == 409
assert res.json['errors'][0]['detail'] == 'Curators cannot be made bibliographic contributors'

contributor = Contributor.objects.get(
node=project,
user=institutional_admin
)
assert not contributor.visible
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Generated by Django 4.2.15 on 2025-01-08 12:24
# Generated by Django 4.2.13 on 2025-01-09 19:19

from django.conf import settings
from django.db import migrations, models
Expand All @@ -14,6 +14,11 @@ class Migration(migrations.Migration):
]

operations = [
migrations.AddField(
model_name='contributor',
name='is_curator',
field=models.BooleanField(default=False),
),
migrations.AddField(
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
model_name='institution',
name='institutional_request_access_enabled',
Expand Down
11 changes: 10 additions & 1 deletion osf/models/contributor.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.db import models
from django.db import models, IntegrityError
from osf.utils.fields import NonNaiveDateTimeField
from osf.utils import permissions

Expand Down Expand Up @@ -30,6 +30,7 @@ def permission(self):

class Contributor(AbstractBaseContributor):
node = models.ForeignKey('AbstractNode', on_delete=models.CASCADE)
is_curator = models.BooleanField(default=False)

@property
def _id(self):
Expand All @@ -41,6 +42,14 @@ class Meta:
# NOTE: Adds an _order column
order_with_respect_to = 'node'

def save(self, *args, **kwargs):
if not self.user.is_institutional_admin_or_curator():
return super().save(*args, **kwargs)
elif self.visible:
raise IntegrityError('Curators cannot be made bibliographic contributors')
else:
return super().save(*args, **kwargs)

Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved

class PreprintContributor(AbstractBaseContributor):
preprint = models.ForeignKey('Preprint', on_delete=models.CASCADE)
Expand Down
7 changes: 6 additions & 1 deletion osf/models/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -1327,7 +1327,7 @@ def _get_admin_contributors_query(self, users, require_active=True):
return qs

def add_contributor(self, contributor, permissions=None, visible=True,
send_email=None, auth=None, log=True, save=False):
send_email=None, auth=None, log=True, save=False, make_curator=False):
"""Add a contributor to the project.

:param User contributor: The contributor to be added
Expand Down Expand Up @@ -1393,6 +1393,11 @@ def add_contributor(self, contributor, permissions=None, visible=True,
if getattr(self, 'get_identifier_value', None) and self.get_identifier_value('doi'):
request, user_id = get_request_and_user_id()
self.update_or_enqueue_on_resource_updated(user_id, first_save=False, saved_fields=['contributors'])

if make_curator:
contributor_obj.is_curator = True
contributor_obj.save()

return contrib_to_add

def add_contributors(self, contributors, auth=None, log=True, save=False):
Expand Down
16 changes: 14 additions & 2 deletions osf/models/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from django.core.exceptions import ImproperlyConfigured
from django.core.paginator import Paginator
from django.urls import reverse
from django.db import models, connection
from django.db import models, connection, IntegrityError
from django.db.models.signals import post_save
from django.dispatch import receiver
from django.utils import timezone
Expand Down Expand Up @@ -77,6 +77,7 @@
from website.util.metrics import OsfSourceTags, CampaignSourceTags
from website.util import api_url_for, api_v2_url, web_url_for
from .base import BaseModel, GuidMixin, GuidMixinQuerySet
from api.base.exceptions import Conflict
from api.caching.tasks import update_storage_usage
from api.caching import settings as cache_settings
from api.caching.utils import storage_usage_cache
Expand Down Expand Up @@ -1079,7 +1080,18 @@ def set_visible(self, user, visible, log=True, auth=None, save=False):
if not self.is_contributor(user):
raise ValueError(f'User {user} not in contributors')
if visible and not Contributor.objects.filter(node=self, user=user, visible=True).exists():
Contributor.objects.filter(node=self, user=user, visible=False).update(visible=True)
Copy link
Contributor Author

@Johnetordoff Johnetordoff Dec 5, 2024

Choose a reason for hiding this comment

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

Overwriting save doesn't always cut it, but this looks like that only place we .update visibility, arguably we want to retain the ability to change the visibility which leans away from placing in DB constraints. However I think this is all visibility updates in the existing code.

contributor = Contributor.objects.get(
node=self,
user=user,
visible=False
)
try:
contributor.visible = True
contributor.save()
except IntegrityError as e:
if 'Curators cannot be made bibliographic contributors' in str(e):
raise Conflict(str(e)) from e
raise e
elif not visible and Contributor.objects.filter(node=self, user=user, visible=True).exists():
if Contributor.objects.filter(node=self, visible=True).count() == 1:
raise ValueError('Must have at least one visible contributor')
Expand Down
18 changes: 17 additions & 1 deletion osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,23 @@ def osf_groups(self):
OSFGroup = apps.get_model('osf.OSFGroup')
return get_objects_for_user(self, 'member_group', OSFGroup, with_superuser=False)

def is_institutional_admin(self, institution):
def is_institutional_admin_or_curator(self, institution=None, node=None):
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
"""
Checks if user is admin of any or of a specific institution, or and curator on a specific node.
"""
if node:
return Contributor.objects.filter(
node=node,
user=self,
is_curator=True,
).exists()

if not institution:
return self.groups.filter(
name__startswith='institution_',
name__endswith='_institutional_admins'
).exists()

group_name = institution.format_group('institutional_admins')
return self.groups.filter(name=group_name).exists()

Expand Down
24 changes: 17 additions & 7 deletions osf/utils/machines.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from django.utils import timezone
from django.db import IntegrityError
from transitions import Machine, MachineError

from api.providers.workflows import Workflows
Expand All @@ -7,7 +8,6 @@
from osf.exceptions import InvalidTransitionError
from osf.models.preprintlog import PreprintLog
from osf.models.action import ReviewAction, NodeRequestAction, PreprintRequestAction

from osf.utils import permissions
from osf.utils.workflows import (
DefaultStates,
Expand All @@ -19,13 +19,16 @@
APPROVAL_TRANSITIONS,
CollectionSubmissionStates,
COLLECTION_SUBMISSION_TRANSITIONS,
NodeRequestTypes
)
from website.mails import mails
from website.reviews import signals as reviews_signals
from website.settings import DOMAIN, OSF_SUPPORT_EMAIL, OSF_CONTACT_EMAIL

from osf.utils import notifications as notify

from api.base.exceptions import Conflict

class BaseMachine(Machine):

action = None
Expand Down Expand Up @@ -199,12 +202,19 @@ def save_changes(self, ev):
if ev.event.name == DefaultTriggers.ACCEPT.value:
if not self.machineable.target.is_contributor(self.machineable.creator):
contributor_permissions = ev.kwargs.get('permissions', permissions.READ)
self.machineable.target.add_contributor(
self.machineable.creator,
auth=Auth(ev.kwargs['user']),
permissions=contributor_permissions,
visible=ev.kwargs.get('visible', True),
send_email=f'{self.machineable.request_type}_request')
try:
self.machineable.target.add_contributor(
self.machineable.creator,
auth=Auth(ev.kwargs['user']),
permissions=contributor_permissions,
visible=ev.kwargs.get('visible', True),
send_email=f'{self.machineable.request_type}_request',
make_curator=self.machineable.request_type == NodeRequestTypes.INSTITUTIONAL_REQUEST.value,
)
except IntegrityError as e:
if 'Curators cannot be made bibliographic contributors' in str(e):
raise Conflict(str(e)) from e
raise e

def resubmission_allowed(self, ev):
# TODO: [PRODUCT-395]
Expand Down
42 changes: 42 additions & 0 deletions osf_tests/test_institutional_admin_contributors.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import pytest
from osf.models import Contributor
from osf_tests.factories import (
AuthUserFactory,
ProjectFactory,
InstitutionFactory
)
from django.db.utils import IntegrityError

@pytest.mark.django_db
class TestContributorModel:

@pytest.fixture()
def user(self):
return AuthUserFactory()

@pytest.fixture()
def project(self):
return ProjectFactory()

@pytest.fixture()
def institution(self):
return InstitutionFactory()

@pytest.fixture()
def institutional_admin(self, institution):
admin_user = AuthUserFactory()
institution.get_group('institutional_admins').user_set.add(admin_user)
return admin_user

def test_contributor_with_visible_and_institutional_admin_raises_error(self, institutional_admin, project, institution):
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
contributor = Contributor(
user=institutional_admin,
node=project,
visible=False,
)
contributor.save()

contributor.visible = True

with pytest.raises(IntegrityError, match='Curators cannot be made bibliographic contributors'):
contributor.save()
6 changes: 5 additions & 1 deletion website/profile/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def serialize_user(user, node=None, admin=False, full=False, is_profile=False, i
'surname': user.family_name,
'fullname': fullname,
'shortname': fullname if len(fullname) < 50 else fullname[:23] + '...' + fullname[-23:],
'is_curator': user.is_institutional_admin_or_curator(node=node),
'profile_image_url': user.profile_image_url(size=settings.PROFILE_IMAGE_MEDIUM),
'active': user.is_active,
}
Expand Down Expand Up @@ -199,7 +200,10 @@ def serialize_access_requests(node):
'requested_permissions': access_request.requested_permissions,
'id': access_request._id
} for access_request in node.requests.filter(
request_type=workflows.RequestTypes.ACCESS.value,
request_type__in=[
workflows.NodeRequestTypes.ACCESS.value,
workflows.NodeRequestTypes.INSTITUTIONAL_REQUEST.value
],
machine_state=workflows.DefaultStates.PENDING.value
).select_related('creator')
]
8 changes: 8 additions & 0 deletions website/project/views/contributor.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from flask import request
from django.core.exceptions import ValidationError
from django.utils import timezone
from django.db import IntegrityError

from framework import forms, status
from framework.auth import cas
Expand Down Expand Up @@ -287,6 +288,13 @@ def project_manage_contributors(auth, node, **kwargs):
node.manage_contributors(contributors, auth=auth, save=True)
except (ValueError, NodeStateError) as error:
raise HTTPError(http_status.HTTP_400_BAD_REQUEST, data={'message_long': error.args[0]})
except IntegrityError as error:
status.push_status_message(
'You can not make an institutional admin a bibliographic contributor.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'You can not make an institutional admin a bibliographic contributor.',
'You can not make an institutional curator a bibliographic contributor.',

kind='error',
trust=False
)
raise HTTPError(http_status.HTTP_409_CONFLICT, data={'message_long': error.args[0]})

# If user has removed herself from project, alert; redirect to
# node summary if node is public, else to user's dashboard page
Expand Down
4 changes: 2 additions & 2 deletions website/static/js/accessRequestManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ var AccessRequestModel = function(accessRequest, pageOwner, isRegistration, isPa

self.permissionText = ko.observable(self.options.permissionMap[self.permission()]);

self.visible = ko.observable(true);

self.is_curator = ko.observable(accessRequest.user.is_curator || false);
self.visible = ko.observable(!accessRequest.user.is_curator);
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
self.pageOwner = pageOwner;

self.expanded = ko.observable(false);
Expand Down
2 changes: 2 additions & 0 deletions website/static/js/contribManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ var ContributorModel = function(contributor, currentUserCanEdit, pageOwner, isRe

self.permission = ko.observable(contributor.permission);

self.is_curator = ko.observable(contributor.is_curator || false);

self.permissionText = ko.observable(self.options.permissionMap[self.permission()]);

self.visible = ko.observable(contributor.visible);
Expand Down
Loading
Loading