From 8adc2df341cc7ef3eeb7273884d333e70d3ba657 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Fri, 12 Jul 2024 11:53:58 -0400 Subject: [PATCH] fix updating behavior and clean-up tests --- api/institutions/utils.py | 26 ++++- api/preprints/serializers.py | 7 +- api/preprints/views.py | 16 +-- ...test_preprint_institutions_relationship.py | 100 +++++++++++++++--- 4 files changed, 111 insertions(+), 38 deletions(-) diff --git a/api/institutions/utils.py b/api/institutions/utils.py index b17509e1a65..ae41ce757e2 100644 --- a/api/institutions/utils.py +++ b/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) diff --git a/api/preprints/serializers.py b/api/preprints/serializers.py index c44ccfc8b6f..15a2b2c2267 100644 --- a/api/preprints/serializers.py +++ b/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) diff --git a/api/preprints/views.py b/api/preprints/views.py index a62b4c4378b..d6f88b801da 100644 --- a/api/preprints/views.py +++ b/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): \ No newline at end of file + def patch(self, *args, **kwargs): + raise MethodNotAllowed(self.request.method) diff --git a/api_tests/preprints/views/test_preprint_institutions_relationship.py b/api_tests/preprints/views/test_preprint_institutions_relationship.py index 3515b9f9901..d8ef0d7e1b3 100644 --- a/api_tests/preprints/views/test_preprint_institutions_relationship.py +++ b/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()