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_at(institution):
return True
else:
raise exceptions.PermissionDenied({'institution': 'You do not have permission to perform this action for this institution.'})
Expand Down
2 changes: 2 additions & 0 deletions api/requests/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ def create(self, validated_data) -> NodeRequest:
def make_node_institutional_access_request(self, node, validated_data) -> NodeRequest:
sender = self.context['request'].user
node_request = self._create_node_request(node, validated_data)
node_request.is_institutional_request = True
node_request.save()
institution = Institution.objects.get(_id=validated_data['institution'])
recipient = OSFUser.load(validated_data.get('message_recipient'))

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_at(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_at(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, make_curator=True)
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 Down
28 changes: 28 additions & 0 deletions osf/migrations/0026_contributor_is_curator_and_more.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Generated by Django 4.2.13 on 2025-01-10 19:27

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('osf', '0025_contributor_is_curator_and_more'),
]

operations = [
migrations.AddField(
model_name='contributor',
name='is_curator',
field=models.BooleanField(default=False),
),
migrations.AddField(
model_name='noderequest',
name='is_institutional_request',
field=models.BooleanField(default=False),
),
migrations.AddField(
model_name='preprintrequest',
name='is_institutional_request',
field=models.BooleanField(default=False),
),
]
9 changes: 8 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,12 @@ class Meta:
# NOTE: Adds an _order column
order_with_respect_to = 'node'

def save(self, *args, **kwargs):
if self.is_curator and self.visible:
raise IntegrityError('Curators cannot be made bibliographic contributors')

return super().save(*args, **kwargs)


class PreprintContributor(AbstractBaseContributor):
preprint = models.ForeignKey('Preprint', on_delete=models.CASCADE)
Expand Down
14 changes: 12 additions & 2 deletions 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 All @@ -1338,6 +1338,7 @@ def add_contributor(self, contributor, permissions=None, visible=True,
:param Auth auth: All the auth information including user, API key
:param bool log: Add log to self
:param bool save: Save after adding contributor
:param bool make_curator incicates whether the user should be an institituional curator
:returns: Whether contributor was added
"""
send_email = send_email or self.contributor_email_template
Expand Down Expand Up @@ -1393,6 +1394,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 Expand Up @@ -1839,7 +1845,11 @@ def set_visible(self, user, visible, log=True, auth=None, save=False):
if visible and not self.contributor_class.objects.filter(**kwargs).exists():
set_visible_kwargs = kwargs
set_visible_kwargs['visible'] = False
self.contributor_class.objects.filter(**set_visible_kwargs).update(visible=True)
contribs = self.contributor_class.objects.filter(**set_visible_kwargs)
if self.guardian_object_type == 'node' and contribs.filter(is_curator=True).exists():
raise ValueError('Curators cannot be made bibliographic contributors')
contribs.update(visible=True)

elif not visible and self.contributor_class.objects.filter(**kwargs).exists():
num_visible_kwargs = self.contributor_kwargs
num_visible_kwargs['visible'] = True
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
1 change: 1 addition & 0 deletions osf/models/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class Meta:
request_type = models.CharField(max_length=31, choices=RequestTypes.choices())
creator = models.ForeignKey('OSFUser', related_name='submitted_%(class)s', on_delete=models.CASCADE)
comment = models.TextField(null=True, blank=True)
is_institutional_request = models.BooleanField(default=False)

@property
def target(self):
Expand Down
30 changes: 27 additions & 3 deletions osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -644,9 +644,33 @@ 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):
group_name = institution.format_group('institutional_admins')
return self.groups.filter(name=group_name).exists()
def is_institutional_admin_at(self, institution):
"""
Checks if user is admin of a specific institution.
"""
return self.has_perms(
institution.groups['institutional_admins'],
institution
)

def is_institutional_admin(self):
"""
Checks if user is admin of any institution.
"""
return self.groups.filter(
name__startswith='institution_',
name__endswith='_institutional_admins'
).exists()

def is_institutional_curator(self, node):
"""
Checks if user is user has curator permissions for a node.
"""
return Contributor.objects.filter(
node=node,
user=self,
is_curator=True,
).exists()

def group_role(self, group):
"""
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
Loading
Loading