[ENG-11909] Retain OSFI ROR IDs on content/profiles for institutions even after the institution has been turned off

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)

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,
         }
 

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)

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,
         }
 

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)

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')

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:

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
 

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

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))
         }
     )
 

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."""

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())

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()

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)

website/identifiers/clients/crossref.py

@@ -212,14 +212,16 @@ 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),
                     element.institution_id(
                         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))

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,