fix updating behavior and clean-up tests

api/institutions/utils.py

@@ -20,19 +20,35 @@ def get_institutions_to_add_remove(institutions, new_institutions):
  return insts_to_add, diff['remove'].values()
 
 
-def update_institutions(node, new_institutions, user, post=False):
+def update_institutions(resource, new_institutions, user, post=False):
  add, remove = get_institutions_to_add_remove(
- institutions=node.affiliated_institutions,
+ institutions=resource.affiliated_institutions,
  new_institutions=new_institutions,
  )
 
  if not post:
  for inst in remove:
- if not user.is_affiliated_with_institution(inst) and not node.has_permission(user, osf_permissions.ADMIN):
+ if not user.is_affiliated_with_institution(inst) and not resource.has_permission(user, osf_permissions.ADMIN):
  raise exceptions.PermissionDenied(detail=f'User needs to be affiliated with {inst.name}')
- node.remove_affiliated_institution(inst, user)
+ resource.remove_affiliated_institution(inst, user)
 
  for inst in add:
  if not user.is_affiliated_with_institution(inst):
  raise exceptions.PermissionDenied(detail=f'User needs to be affiliated with {inst.name}',)
- node.add_affiliated_institution(inst, user)
+ resource.add_affiliated_institution(inst, user)
+
+
+def update_institutions_if_user_associated(resource, desired_institutions_data, user):
+ desired_institutions = Institution.objects.filter(_id__in=[item['_id'] for item in desired_institutions_data])
+
+ # If a user wants to affiliate with a resource check that they have it.
+ for inst in desired_institutions:
+ if user.is_affiliated_with_institution(inst):
+ resource.add_affiliated_institution(inst, user)
+ else:
+ raise exceptions.PermissionDenied(detail=f'User needs to be affiliated with {inst.name}')
+
+ # If a user doesn't include an affiliation they have, then remove it.
+ for inst in user.get_affiliated_institutions():
+ if inst in resource.affiliated_institutions.all() and inst not in desired_institutions:
+ resource.remove_affiliated_institution(inst, user)

api/preprints/serializers.py

@@ -34,10 +34,9 @@
  NodeContributorDetailSerializer,
  get_license_details,
  NodeTagField,
- update_institutions,
-
 )
 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 website.project import signals as project_signals
@@ -560,7 +559,7 @@ def make_instance_obj(self, obj):
  def update(self, instance, validated_data):
  preprint = instance['self']
  user = self.context['request'].user
- update_institutions(preprint, validated_data['data'], user)
+ update_institutions_if_user_associated(preprint, validated_data['data'], user)
  preprint.save()
 
  return self.make_instance_obj(preprint)
@@ -569,7 +568,7 @@ def create(self, validated_data):
  instance = self.context['view'].get_object()
  user = self.context['request'].user
  preprint = instance['self']
- update_institutions(preprint, validated_data['data'], user, post=True)
+ update_institutions_if_user_associated(preprint, validated_data['data'], user, post=True)
  preprint.save()
 
  return self.make_instance_obj(preprint)

api/preprints/views.py

@@ -678,17 +678,5 @@ def get_object(self):
  self.check_object_permissions(self.request, obj)
  return obj
 
- def perform_destroy(self, instance):
- data = self.request.data['data']
- user = self.request.user
- current_insts = {inst._id: inst for inst in instance['data']}
- node = instance['self']
-
- for val in data:
- if val['id'] in current_insts:
- if not user.is_affiliated_with_institution(current_insts[val['id']]) and not node.has_permission(user, 'admin'):
- raise PermissionDenied
- node.remove_affiliated_institution(inst=current_insts[val['id']], user=user)
- node.save()
-
- def patch(self):
+ def patch(self, *args, **kwargs):
+ raise MethodNotAllowed(self.request.method)

api_tests/preprints/views/test_preprint_institutions_relationship.py

@@ -10,6 +10,7 @@
 
 @pytest.mark.django_db
 class TestPreprintInstitutionsRelationship:
+ """Test suite for managing preprint institution relationships."""
 
  @pytest.fixture()
  def user(self):
@@ -26,6 +27,7 @@ def admin_with_institutional_affilation(self, institution, preprint):
  def no_auth_with_institutional_affilation(self, institution):
  user = AuthUserFactory()
  user.add_or_update_affiliated_institution(institution)
+ user.save()
  return user
 
  @pytest.fixture()
@@ -48,10 +50,19 @@ def preprint(self):
 
  @pytest.fixture()
  def url(self, preprint):
+ """Fixture that returns the URL for the preprint-institutions relationship endpoint."""
  return f'/{API_BASE}preprints/{preprint._id}/relationships/institutions/'
 
  def test_update_affiliated_institutions_add(self, app, user, admin_with_institutional_affilation, admin_without_institutional_affilation, preprint, url,
  institution):
+ """
+ Test adding affiliated institutions to a preprint.
+
+ Verifies:
+ - Unauthorized users cannot add institutions.
+ - Admins without affiliation cannot add institutions.
+ - Admins with affiliation can add institutions.
+ """
  update_institutions_payload = {
  'data': [{'type': 'institutions', 'id': institution._id}]
  }
@@ -92,7 +103,15 @@ def test_update_affiliated_institutions_add(self, app, user, admin_with_institut
 
  def test_update_affiliated_institutions_remove(self, app, user, admin_with_institutional_affilation, no_auth_with_institutional_affilation, admin_without_institutional_affilation, preprint, url,
  institution):
-
+ """
+ Test removing affiliated institutions from a preprint.
+
+ Verifies:
+ - Unauthorized users cannot remove institutions.
+ - Non-admin users cannot remove institutions.
+ - Admins without affiliation can remove institutions.
+ - Admins with affiliation can remove institutions.
+ """
  preprint.affiliated_institutions.add(institution)
  preprint.save()
 
@@ -143,7 +162,15 @@ def test_update_affiliated_institutions_remove(self, app, user, admin_with_insti
 
  def test_preprint_institutions_list_get(self, app, user, admin_with_institutional_affilation, admin_without_institutional_affilation, preprint, url,
  institution):
- # For testing purposes
+ """
+ Test retrieving the list of affiliated institutions for a preprint.
+
+ Verifies:
+ - Unauthenticated users cannot retrieve the list.
+ - Users without permissions cannot retrieve the list.
+ - Admins without affiliation can retrieve the list.
+ - Admins with affiliation can retrieve the list.
+ """
  preprint.is_public = False
  preprint.save()
 
@@ -168,6 +195,12 @@ def test_preprint_institutions_list_get(self, app, user, admin_with_institutiona
 
  def test_post_affiliated_institutions(self, app, user, admin_with_institutional_affilation, preprint, url,
  institutions, institution):
+ """
+ Test that POST method is not allowed for affiliated institutions.
+
+ Verifies:
+ - POST requests return a 405 Method Not Allowed status.
+ """
  add_institutions_payload = {
  'data': [{'type': 'institutions', 'id': institution._id} for institution in institutions]
  }
@@ -180,9 +213,34 @@ def test_post_affiliated_institutions(self, app, user, admin_with_institutional_
  )
  assert res.status_code == 405
 
+ def test_patch_affiliated_institutions(self, app, user, admin_with_institutional_affilation, preprint, url,
+ institutions, institution):
+ """
+ Test that PATCH method is not allowed for affiliated institutions.
+
+ Verifies:
+ - PATCH requests return a 405 Method Not Allowed status.
+ """
+ add_institutions_payload = {
+ 'data': [{'type': 'institutions', 'id': institution._id} for institution in institutions]
+ }
+
+ res = app.patch_json_api(
+ url,
+ add_institutions_payload,
+ auth=admin_with_institutional_affilation.auth,
+ expect_errors=True
+ )
+ assert res.status_code == 405
+
  def test_delete_affiliated_institution(self, app, user, admin_with_institutional_affilation, admin_without_institutional_affilation, preprint, url,
  institution):
+ """
+ Test that DELETE method is not allowed for affiliated institutions.
 
+ Verifies:
+ - DELETE requests return a 405 Method Not Allowed status.
+ """
  preprint.affiliated_institutions.add(institution)
  preprint.save()
 
@@ -195,8 +253,13 @@ def test_delete_affiliated_institution(self, app, user, admin_with_institutional
  assert res.status_code == 405
 
  def test_add_multiple_institutions_affiliations(self, app, admin_with_institutional_affilation, admin_without_institutional_affilation, preprint, url,
- institutions):
+ institutions):
+ """
+ Test adding multiple institution affiliations to a preprint.
 
+ Verifies:
+ - Admins with multiple affiliations can add them to a preprint.
+ """
  admin_with_institutional_affilation.add_or_update_affiliated_institution(institutions[0])
  admin_with_institutional_affilation.add_or_update_affiliated_institution(institutions[1])
  admin_with_institutional_affilation.add_or_update_affiliated_institution(institutions[2])
@@ -211,28 +274,35 @@ def test_add_multiple_institutions_affiliations(self, app, admin_with_institutio
  add_institutions_payload,
  auth=admin_with_institutional_affilation.auth,
  )
- print(res.json)
- assert False
  assert res.status_code == 200
  assert preprint.affiliated_institutions.all().count() == 3
 
  preprint.reload()
 
+ def test_remove_only_institutions_affiliations_that_user_has(self, app, user, admin_with_institutional_affilation, preprint, url,
+ institutions, institution):
+ """
+ Test removing only institutions that the user is affiliated with from a preprint.
 
- def test_remove_only_institutions_affiliations_that_user_has(self, app, user, admin_with_institutional_affilation,
- admin_without_institutional_affilation, preprint, url,
- institutions):
+ Verifies:
+ - Admins with multiple affiliations only remove their own affiliations, leaving others unchanged.
+ """
+ preprint.affiliated_institutions.add(*institutions)
+ assert preprint.affiliated_institutions.all().count() == 3
+
+ admin_with_institutional_affilation.add_or_update_affiliated_institution(institutions[0])
+ admin_with_institutional_affilation.add_or_update_affiliated_institution(institutions[1])
 
- remove_institution_payload = {
- 'data': [{'type': 'institutions', 'id': institutions[0]._id}]
+ update_institution_payload = {
+ 'data': [{'type': 'institutions', 'id': institution._id}]
  }
 
  res = app.put_json_api(
  url,
- remove_institution_payload,
+ update_institution_payload,
  auth=admin_with_institutional_affilation.auth
  )
- assert res.status_code == 201
-
- preprint.reload()
- assert len(preprint.affiliated_institutions.all()) == 1
+ assert res.status_code == 200
+ assert preprint.affiliated_institutions.all().count() == 2
+ assert institution in preprint.affiliated_institutions.all()
+ assert institutions[2] in preprint.affiliated_institutions.all()