diff --git a/api/institutions/utils.py b/api/institutions/utils.py index 3defb74b031..5fa76cee40e 100644 --- a/api/institutions/utils.py +++ b/api/institutions/utils.py @@ -52,7 +52,7 @@ def update_institutions_if_user_associated(resource, desired_institutions_data, 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. - resource_institutions = resource.affiliated_institutions.all() + resource_institutions = resource.get_affiliated_institutions() for inst in user.get_affiliated_institutions(): if inst in resource_institutions and inst not in desired_institutions: resource.remove_affiliated_institution(inst, user) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index 1e68d1ff2e4..807e9e98aea 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -1551,7 +1551,7 @@ class Meta: def make_instance_obj(self, obj): return { - 'data': obj.affiliated_institutions.all(), + 'data': obj.get_affiliated_institutions(), 'self': obj, } diff --git a/api/nodes/views.py b/api/nodes/views.py index 931220a6f88..f540afecd74 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -1678,7 +1678,7 @@ def get_resource(self): def get_queryset(self): resource = self.get_resource() - return resource.affiliated_institutions.all() or [] + return resource.get_affiliated_institutions() or [] class NodeInstitutionsRelationship(JSONAPIBaseView, generics.RetrieveUpdateDestroyAPIView, generics.CreateAPIView, NodeMixin): @@ -1757,7 +1757,7 @@ def get_resource(self): def get_object(self): node = self.get_resource() obj = { - 'data': node.affiliated_institutions.all(), + 'data': node.get_affiliated_institutions(), 'self': node, } self.check_object_permissions(self.request, obj) diff --git a/api/preprints/serializers.py b/api/preprints/serializers.py index b901aa093f3..8bd75c34a53 100644 --- a/api/preprints/serializers.py +++ b/api/preprints/serializers.py @@ -739,7 +739,7 @@ class Meta: def make_instance_obj(self, obj): return { - 'data': obj.affiliated_institutions.all(), + 'data': obj.get_affiliated_institutions(), 'self': obj, } diff --git a/api/preprints/views.py b/api/preprints/views.py index 5d5e13c3b1f..415018ca874 100644 --- a/api/preprints/views.py +++ b/api/preprints/views.py @@ -798,7 +798,7 @@ def get_resource(self): return self.get_preprint() def get_queryset(self): - return self.get_resource().affiliated_institutions.all() + return self.get_resource().get_affiliated_institutions() class PreprintInstitutionsRelationship(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin): @@ -822,7 +822,7 @@ def get_resource(self): def get_object(self): preprint = self.get_resource() obj = { - 'data': preprint.affiliated_institutions.all(), + 'data': preprint.get_affiliated_institutions(), 'self': preprint, } self.check_object_permissions(self.request, obj) diff --git a/api/users/serializers.py b/api/users/serializers.py index 05f7c27bc77..e4184723ed7 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -242,9 +242,7 @@ def get_draft_preprint_count(self, obj): return Preprint.objects.can_view(user_preprints_query, auth_user, allow_contribs=False).count() def get_institutions_count(self, obj): - if isinstance(obj, OSFUser): - return obj.get_affiliated_institutions().count() - return obj.affiliated_institutions.count() + return obj.get_affiliated_institutions().count() def get_can_view_reviews(self, obj): group_qs = AbstractProviderGroupObjectPermission.objects.filter(group__user=obj, permission__codename='view_submissions') diff --git a/osf/metadata/osf_gathering.py b/osf/metadata/osf_gathering.py index 14c637955aa..7987473638e 100644 --- a/osf/metadata/osf_gathering.py +++ b/osf/metadata/osf_gathering.py @@ -871,10 +871,10 @@ def gather_verified_link(focus): @gather.er(OSF.affiliation) def gather_affiliated_institutions(focus): - if hasattr(focus.dbmodel, 'get_affiliated_institutions'): # like OSFUser - institution_qs = focus.dbmodel.get_affiliated_institutions() - elif hasattr(focus.dbmodel, 'affiliated_institutions'): # like AbstractNode or Preprint - institution_qs = focus.dbmodel.affiliated_institutions.all() + if hasattr(focus.dbmodel, 'get_affiliated_institutions'): # like OSFUser, AbstractNode or Preprint + # Deactivated institutions are included on purpose: metadata and DOIs that already + # have institution's ROR id should keep it after the institution is turned off + institution_qs = focus.dbmodel.get_affiliated_institutions(include_deactivated=True) else: institution_qs = () for osf_institution in institution_qs: diff --git a/osf/models/mixins.py b/osf/models/mixins.py index d39bd1a19d2..c231f540e02 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -373,6 +373,16 @@ def remove_affiliated_institution(self, inst, user, save=False, log=True, notify def is_affiliated_with_institution(self, institution): return self.affiliated_institutions.filter(id=institution.id).exists() + def get_affiliated_institutions(self, *, include_deactivated: bool = False): + """ + Return a queryset of the institutions affiliated with this object. Deactivated are hidden + by the default Institution manager, so pass include_deactivated to keep them, + so that metadata and DOIs do not lose ROR ids they already have + """ + if include_deactivated: + return self.affiliated_institutions(manager='_base_manager').all() + return self.affiliated_institutions.all() + class Meta: abstract = True diff --git a/osf/models/preprint.py b/osf/models/preprint.py index fe1e1ae8e59..5bc55a1431b 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -539,8 +539,9 @@ def create_version(cls, create_from_guid, auth, assign_version_number=None, igno sentry.log_message(f'Unregistered contributor was not added to new preprint version due to error: ' f'[preprint={preprint._id}, user={contributor.user._id}]') - # Add affiliated institutions - for institution in latest_version.affiliated_institutions.all(): + # Add affiliated institutions. Deactivated institutions are carried over on purpose so a + # new version does not silently drop an affiliation and its ROR id the previous one had + for institution in latest_version.get_affiliated_institutions(include_deactivated=True): preprint.add_affiliated_institution(institution, auth.user, ignore_user_affiliation=True) # Update Guid obj to point to the new version if there is no moderation and new version is bigger diff --git a/osf/models/registrations.py b/osf/models/registrations.py index e9e74cd1db7..7be64c67cc0 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -1649,7 +1649,7 @@ def sync_internet_archive_institutions(sender, instance, action, **kwargs): update_ia_metadata( instance, { - 'affiliated_institutions': list(instance.affiliated_institutions.all().values_list('name', flat=True)) + 'affiliated_institutions': list(instance.get_affiliated_institutions().values_list('name', flat=True)) } ) diff --git a/osf/models/user.py b/osf/models/user.py index 98461a3cf0a..89edb6efa20 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1811,10 +1811,15 @@ def has_affiliated_institutions(self): """Return if the current user is affiliated with any institutions.""" return InstitutionAffiliation.objects.filter(user__id=self.id).exists() - def get_affiliated_institutions(self): - """Return a queryset of all affiliated institutions for the current user.""" + def get_affiliated_institutions(self, *, include_deactivated: bool = False): + """ + Return a queryset of all affiliated institutions for the current user. Deactivated + are hidden by the default Institution manager; pass include_deactivated to keep them, so + that metadata and DOIs do not lose ROR ids they already have + """ qs = InstitutionAffiliation.objects.filter(user__id=self.id).values_list('institution', flat=True) - return Institution.objects.filter(pk__in=qs) + institutions = Institution.objects.get_all_institutions() if include_deactivated else Institution.objects.all() + return institutions.filter(pk__in=qs) def get_institution_affiliations(self): """Return a queryset of all institution affiliations for the current user.""" diff --git a/osf_tests/metadata/test_osf_gathering.py b/osf_tests/metadata/test_osf_gathering.py index 23095b066ee..d0fc41cde95 100644 --- a/osf_tests/metadata/test_osf_gathering.py +++ b/osf_tests/metadata/test_osf_gathering.py @@ -3,6 +3,7 @@ import pytest from django.test import TestCase +from django.utils import timezone import rdflib from rdflib import Literal, URIRef @@ -597,6 +598,29 @@ def test_gather_affiliated_institutions(self): (institution_iri, DCTERMS.identifier, Literal(institution.ror_uri)), }) + def test_gather_affiliated_institutions_survives_deactivation(self): + """ + Turning an institution off hides it from the front end and search, but metadata and + DOIs that already have its ROR id keep that id, even though the record is updated + """ + institution = factories.InstitutionFactory() + institution_iri = URIRef(institution.ror_uri) + self.user__admin.add_or_update_affiliated_institution(institution) + with capture_notifications(): + self.project.add_affiliated_institution(institution, self.user__admin) + self.preprint.add_affiliated_institution(institution, self.user__admin) + institution.deactivated = timezone.now() + institution.save() + for focus in (self.projectfocus, self.preprintfocus, self.userfocus__admin): + assert_triples(osf_gathering.gather_affiliated_institutions(focus), { + (focus.iri, OSF.affiliation, institution_iri), + (institution_iri, RDF.type, DCTERMS.Agent), + (institution_iri, RDF.type, FOAF.Organization), + (institution_iri, FOAF.name, Literal(institution.name)), + (institution_iri, DCTERMS.identifier, Literal(institution.identifier_domain)), + (institution_iri, DCTERMS.identifier, Literal(institution.ror_uri)), + }) + def test_gather_funding(self): # focus: project assert_triples(osf_gathering.gather_funding(self.projectfocus), set()) diff --git a/osf_tests/test_institutional_affiliation.py b/osf_tests/test_institutional_affiliation.py index 86bb6cebff6..1190e7c6d45 100644 --- a/osf_tests/test_institutional_affiliation.py +++ b/osf_tests/test_institutional_affiliation.py @@ -1,6 +1,12 @@ import pytest +from django.utils import timezone + +from framework.auth import Auth +from osf.models import Preprint from osf_tests.factories import ( + DraftRegistrationFactory, PreprintFactory, + ProjectFactory, UserFactory, InstitutionFactory, ) @@ -53,3 +59,122 @@ def test_add_and_remove_affiliated_institution(self, preprint, institution, user def test_permission_errors_during_affiliation_update(self, preprint, institution, user_without_affiliation): with pytest.raises(UserNotAffiliatedError): preprint.add_affiliated_institution(institution, user_without_affiliation) + + +@pytest.mark.django_db +class TestGetAffiliatedInstitutions: + """ + get_affiliated_institutions() hides deactivated institutions by default, so they stay off + the front end and out of search. Callers that must not drop an affiliation an object already + has metadata and DOIs carrying a ROR id pass include_deactivated=True + """ + + @staticmethod + def _deactivate(institution): + institution.deactivated = timezone.now() + institution.save() + + @pytest.fixture() + def active_institution(self): + return InstitutionFactory() + + @pytest.fixture() + def institution_to_deactivate(self): + return InstitutionFactory() + + @pytest.fixture() + def user(self, active_institution, institution_to_deactivate): + user = UserFactory() + user.add_or_update_affiliated_institution(active_institution) + user.add_or_update_affiliated_institution(institution_to_deactivate) + return user + + @pytest.fixture(params=['preprint', 'node', 'draft_registration']) + def resource(self, request, user, active_institution, institution_to_deactivate): + if request.param == 'preprint': + resource = PreprintFactory(creator=user) + elif request.param == 'node': + resource = ProjectFactory(creator=user) + else: + resource = DraftRegistrationFactory(initiator=user) + resource.affiliated_institutions.set([active_institution, institution_to_deactivate]) + return resource + + @pytest.fixture() + def affiliated_preprint(self, user, active_institution, institution_to_deactivate): + preprint = PreprintFactory(creator=user) + preprint.affiliated_institutions.set([active_institution, institution_to_deactivate]) + return preprint + + def test_include_deactivated_on_every_model(self, resource, active_institution, institution_to_deactivate): + self._deactivate(institution_to_deactivate) + institutions = resource.get_affiliated_institutions(include_deactivated=True) + assert set(institutions) == {active_institution, institution_to_deactivate} + + def test_resource_excludes_deactivated_by_default(self, affiliated_preprint, active_institution, institution_to_deactivate): + self._deactivate(institution_to_deactivate) + assert list(affiliated_preprint.get_affiliated_institutions()) == [active_institution] + + def test_resource_include_deactivated_returns_a_queryset(self, affiliated_preprint, institution_to_deactivate): + self._deactivate(institution_to_deactivate) + names = affiliated_preprint.get_affiliated_institutions(include_deactivated=True).values_list('name', flat=True) + assert institution_to_deactivate.name in names + + def test_resource_include_deactivated_is_noop_while_active(self, affiliated_preprint, active_institution, institution_to_deactivate): + both = {active_institution, institution_to_deactivate} + assert set(affiliated_preprint.get_affiliated_institutions()) == both + assert set(affiliated_preprint.get_affiliated_institutions(include_deactivated=True)) == both + + def test_user_excludes_deactivated_by_default(self, user, active_institution, institution_to_deactivate): + self._deactivate(institution_to_deactivate) + assert list(user.get_affiliated_institutions()) == [active_institution] + + def test_user_include_deactivated(self, user, active_institution, institution_to_deactivate): + self._deactivate(institution_to_deactivate) + institutions = user.get_affiliated_institutions(include_deactivated=True) + assert set(institutions) == {active_institution, institution_to_deactivate} + + def test_user_include_deactivated_returns_a_queryset(self, user, institution_to_deactivate): + self._deactivate(institution_to_deactivate) + names = user.get_affiliated_institutions(include_deactivated=True).values_list('name', flat=True) + assert institution_to_deactivate.name in names + + def test_user_include_deactivated_is_noop_while_active(self, user, active_institution, institution_to_deactivate): + both = {active_institution, institution_to_deactivate} + assert set(user.get_affiliated_institutions()) == both + assert set(user.get_affiliated_institutions(include_deactivated=True)) == both + + +@pytest.mark.django_db +class TestPreprintVersionAffiliations: + """ + A new preprint version inherits the affiliations of the version it was created from, including + institutions that have since been turned off, so that its metadata keeps their ROR ids + """ + + @pytest.fixture() + def institution(self): + return InstitutionFactory() + + @pytest.fixture() + def user(self, institution): + user = UserFactory() + user.add_or_update_affiliated_institution(institution) + return user + + @pytest.fixture() + def preprint(self, user, institution): + preprint = PreprintFactory(creator=user) + preprint.affiliated_institutions.set([institution]) + return preprint + + def test_new_version_keeps_deactivated_affiliation(self, preprint, user, institution): + institution.deactivated = timezone.now() + institution.save() + new_preprint, _ = Preprint.create_version( + create_from_guid=preprint._id, + auth=Auth(user), + ignore_permission=True, + ) + assert institution in new_preprint.get_affiliated_institutions(include_deactivated=True) + assert institution not in new_preprint.get_affiliated_institutions() diff --git a/tests/identifiers/test_crossref.py b/tests/identifiers/test_crossref.py index 87c81822e2c..173e8f9a5a0 100644 --- a/tests/identifiers/test_crossref.py +++ b/tests/identifiers/test_crossref.py @@ -3,6 +3,7 @@ import lxml import pytest import responses +from django.utils import timezone from tests.utils import capture_notifications from website import settings @@ -337,6 +338,22 @@ def test_metadata_for_affiliated_institutions(self, crossref_client, preprint): assert contributors.find('.//{%s}institution_name' % crossref.CROSSREF_NAMESPACE).text == institution.name assert contributors.find('.//{%s}institution_id' % crossref.CROSSREF_NAMESPACE).text == institution.ror_uri + def test_metadata_for_affiliated_institutions_survives_deactivation(self, crossref_client, preprint): + institution = InstitutionFactory() + institution.ror_uri = 'http://ror.org/WHATisITgoodFOR/' + institution.save() + preprint.creator.add_or_update_affiliated_institution(institution) + preprint.creator.save() + + institution.deactivated = timezone.now() + institution.save() + + crossref_xml = crossref_client.build_metadata(preprint) + root = lxml.etree.fromstring(crossref_xml) + contributors = root.find('.//{%s}contributors' % crossref.CROSSREF_NAMESPACE) + assert contributors.find('.//{%s}institution_name' % crossref.CROSSREF_NAMESPACE).text == institution.name + assert contributors.find('.//{%s}institution_id' % crossref.CROSSREF_NAMESPACE).text == institution.ror_uri + def test_metadata_uses_existing_identifier(self, crossref_client, preprint): doi = settings.DOI_FORMAT.format(prefix=preprint.provider.doi_prefix, guid=preprint.id) preprint.set_identifier_values(doi, save=True) diff --git a/website/identifiers/clients/crossref.py b/website/identifiers/clients/crossref.py index f7e20a618c2..309a994abc0 100644 --- a/website/identifiers/clients/crossref.py +++ b/website/identifiers/clients/crossref.py @@ -212,6 +212,8 @@ def _crossref_format_contributors(self, element, preprint): person.append(element.surname(name_parts['surname'])) if name_parts.get('suffix'): person.append(element.suffix(remove_control_characters(name_parts['suffix']))) + # Deactivated institutions are included on purpose: a DOI that already carries an + # institution's ROR id should keep it after the institution is turned off affiliations = [ element.institution( element.institution_name(institution.name), @@ -219,7 +221,7 @@ def _crossref_format_contributors(self, element, preprint): institution.ror_uri, type='ror' ), - ) for institution in contributor.get_affiliated_institutions() if institution.ror_uri + ) for institution in contributor.get_affiliated_institutions(include_deactivated=True) if institution.ror_uri ] if affiliations: person.append(element.affiliations(*affiliations)) diff --git a/website/project/views/node.py b/website/project/views/node.py index 9315eb59065..4d6a7678545 100644 --- a/website/project/views/node.py +++ b/website/project/views/node.py @@ -20,7 +20,6 @@ from api.waffle.utils import flag_is_active, storage_i18n_flag_active, storage_usage_flag_active from framework.exceptions import HTTPError from osf.models.nodelog import NodeLog -from osf.models.user import OSFUser from osf.utils.functional import rapply from osf.utils.registrations import strip_registered_meta_comments from osf.utils import sanitize @@ -907,11 +906,7 @@ def _view_project(node, auth, primary=False, def get_affiliated_institutions(obj): ret = [] - if isinstance(obj, OSFUser): - institutions = obj.get_affiliated_institutions() - else: - institutions = obj.affiliated_institutions.all() - for institution in institutions: + for institution in obj.get_affiliated_institutions(): ret.append({ 'name': institution.name, 'logo_path': institution.logo_path,