From f39417410b883eaa0bd4e65fae68eb66ac76daa3 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Mon, 25 Jul 2022 13:18:05 -0400 Subject: [PATCH 1/4] Model-level changes to support deleting Artifacts and updating Artifact DOIs --- osf/exceptions.py | 4 + .../0247_artifact_finalized_and_deleted.py | 36 ++++ osf/models/identifiers.py | 8 + osf/models/nodelog.py | 7 +- osf/models/outcome_artifacts.py | 134 +++++++++++--- osf/models/outcomes.py | 24 ++- osf/utils/outcomes.py | 31 +++- osf_tests/test_outcomes.py | 172 ++++++++++++++---- 8 files changed, 349 insertions(+), 67 deletions(-) create mode 100644 osf/migrations/0247_artifact_finalized_and_deleted.py diff --git a/osf/exceptions.py b/osf/exceptions.py index f250be49ab3..dd41ed93c6c 100644 --- a/osf/exceptions.py +++ b/osf/exceptions.py @@ -223,3 +223,7 @@ def __init__(self, response, invalid_responses=None, unsupported_keys=None): ) super().__init__(error_message) + + +class IdentifierHasReferencesError(OSFError): + pass diff --git a/osf/migrations/0247_artifact_finalized_and_deleted.py b/osf/migrations/0247_artifact_finalized_and_deleted.py new file mode 100644 index 00000000000..e8b3194993d --- /dev/null +++ b/osf/migrations/0247_artifact_finalized_and_deleted.py @@ -0,0 +1,36 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.29 on 2022-07-25 15:39 +from __future__ import unicode_literals + +from django.db import migrations, models +import osf.utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0246_add_outcomes_and_artifacts'), + ] + + operations = [ + migrations.AddField( + model_name='outcomeartifact', + name='deleted', + field=osf.utils.fields.NonNaiveDateTimeField(blank=True, null=True), + ), + migrations.AddField( + model_name='outcomeartifact', + name='finalized', + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name='outcomeartifact', + name='description', + field=models.TextField(blank=True), + ), + migrations.AlterField( + model_name='outcomeartifact', + name='title', + field=models.TextField(blank=True), + ), + ] diff --git a/osf/models/identifiers.py b/osf/models/identifiers.py index 5d871a74c0d..c7aa2cdfa46 100644 --- a/osf/models/identifiers.py +++ b/osf/models/identifiers.py @@ -2,6 +2,8 @@ from django.contrib.contenttypes.models import ContentType from django.db import models from django.utils import timezone + +from osf.exceptions import IdentifierHasReferencesError from osf.models.base import BaseModel, ObjectIDMixin from osf.utils.fields import NonNaiveDateTimeField @@ -28,6 +30,12 @@ def remove(self, save=True): if save: self.save() + def delete(self): + '''Used to delete an orphaned Identifier (distinct from setting `deleted`)''' + if self.object_id or self.artifact_metadata.exists(): + raise IdentifierHasReferencesError + super().delete() + class IdentifierMixin(models.Model): """Model mixin that adds methods for getting and setting Identifier objects diff --git a/osf/models/nodelog.py b/osf/models/nodelog.py index 11c5091f229..0d94653b3f9 100644 --- a/osf/models/nodelog.py +++ b/osf/models/nodelog.py @@ -143,6 +143,10 @@ class NodeLog(ObjectIDMixin, BaseModel): MIGRATED_QUICK_FILES = 'migrated_quickfiles' + RESOURCE_ADDED = 'resource_identifier_added' + RESOURCE_UPDATED = 'resource_identifier_udpated' + RESOURCE_REMOVED = 'resource_identifier_removed' + actions = ([CHECKED_IN, CHECKED_OUT, FILE_TAG_REMOVED, FILE_TAG_ADDED, CREATED_FROM, PROJECT_CREATED, PROJECT_REGISTERED, PROJECT_DELETED, NODE_CREATED, NODE_FORKED, NODE_REMOVED, NODE_ACCESS_REQUESTS_ENABLED, NODE_ACCESS_REQUESTS_DISABLED, @@ -161,7 +165,8 @@ class NodeLog(ObjectIDMixin, BaseModel): PREREG_REGISTRATION_INITIATED, PROJECT_CREATED_FROM_DRAFT_REG, GROUP_ADDED, GROUP_UPDATED, GROUP_REMOVED, AFFILIATED_INSTITUTION_ADDED, AFFILIATED_INSTITUTION_REMOVED, PREPRINT_INITIATED, - PREPRINT_FILE_UPDATED, PREPRINT_LICENSE_UPDATED, VIEW_ONLY_LINK_ADDED, VIEW_ONLY_LINK_REMOVED] + list(sum([ + PREPRINT_FILE_UPDATED, PREPRINT_LICENSE_UPDATED, VIEW_ONLY_LINK_ADDED, VIEW_ONLY_LINK_REMOVED, + RESOURCE_ADDED, RESOURCE_UPDATED, RESOURCE_REMOVED] + list(sum([ config.actions for config in apps.get_app_configs() if config.name.startswith('addons.') ], tuple()))) action_choices = [(action, action.upper()) for action in actions] diff --git a/osf/models/outcome_artifacts.py b/osf/models/outcome_artifacts.py index 87b73a7df55..aa4de5e23a4 100644 --- a/osf/models/outcome_artifacts.py +++ b/osf/models/outcome_artifacts.py @@ -1,8 +1,11 @@ from django.db import models +from django.utils import timezone +from osf.exceptions import IdentifierHasReferencesError from osf.models.base import BaseModel, ObjectIDMixin from osf.models.identifiers import Identifier -from osf.utils.outcomes import ArtifactTypes, NoPIDError +from osf.utils import outcomes as outcome_utils +from osf.utils.fields import NonNaiveDateTimeField ''' @@ -13,31 +16,36 @@ for the research effort described by the Outcome. ''' + +ArtifactTypes = outcome_utils.ArtifactTypes +OutcomeActions = outcome_utils.OutcomeActions + + class ArtifactManager(models.Manager): def get_queryset(self): - return super().get_queryset().annotate( - pid=models.F('identifier__value') - ) + '''Overrides default `get_queryset` behavior to add custom logic. - def create_for_identifier_value( - self, outcome, pid_value, pid_type='doi', create_identifier=False, **kwargs - ): - if create_identifier: - identifier, _ = Identifier.objects.get_or_create( - value=pid_value, category=pid_type - ) - else: - try: - identifier = Identifier.objects.get( - value=pid_value, category=pid_type - ) - except Identifier.DoesNotExist: - raise NoPIDError('No PID with value {pid_value} found for PID type {pid_type}') + Automatically annotates the `pid` from any linked identifier and the + GUID of the primary resource for the parent artifact. + + Automatically filters out deleted entries + ''' + base_queryset = super().get_queryset().select_related('identifier') + return base_queryset.annotate( + pid=models.F('identifier__value'), + primary_resource_guid=outcome_utils.make_primary_resource_guid_annotation(base_queryset) + ).filter(deleted__isnull=True) - return self.create(outcome=outcome, identifier=identifier, **kwargs) + def include_deleted(self): + '''Returns all OutcomeArtifacts, including those marked as `deleted`. + + Excludes other API-relevant annotations. + ''' + return super().get_queryset() def for_registration(self, registration, identifier_type='doi'): + '''Retrieves all OutcomeArtifacts sharing an Outcome, given the Primary Registration.''' registration_identifier = registration.get_identifier(identifier_type) artifact_qs = self.get_queryset() return artifact_qs.annotate( @@ -84,8 +92,10 @@ class OutcomeArtifact(ObjectIDMixin, BaseModel): default=ArtifactTypes.UNDEFINED, ) - title = models.TextField(null=False) - description = models.TextField(null=False) + title = models.TextField(null=False, blank=True) + description = models.TextField(null=False, blank=True) + finalized = models.BooleanField(default=False) + deleted = NonNaiveDateTimeField(null=True, blank=True) objects = ArtifactManager() @@ -95,3 +105,85 @@ class Meta: models.Index(fields=['outcome', 'artifact_type']) ] ordering = ['artifact_type', 'title'] + + def update_identifier(self, new_pid_value, pid_type='doi', api_request=None): + '''Changes the linked Identifer to one matching the new pid_value and handles callbacks. + + If `finalized` is True, will also log the change on the parent Outcome if invoked via API. + Will attempt to delete the previous identifier to avoid orphaned entries. + + Parameters: + new_pid_value: The string value of the new PID + pid_type (str): The string "type" of the new PID (for now, only "doi" is supported) + api_request: The api_request data from the API call that initiated the change. + ''' + + previous_identifier = self.identifier + self.identifier, _ = Identifier.objects.get_or_create( + value=new_pid_value, category=pid_type + ) + self.save() + if previous_identifier: + try: + previous_identifier.delete() + except IdentifierHasReferencesError: + pass + + if self.finalized and api_request: + self.outcome.log_artifact_change( + action=OutcomeActions.UPDATE, + artifact=self, + api_request=api_request, + obsolete_identifier=previous_identifier.value if previous_identifier else None, + new_identifier=new_pid_value + ) + + def finalize(self, api_request=None): + '''Sets `finalized` to True and handles callbacks. + + Logs the change on the parent Outcome if invoked via the API. + + Parameters: + api_request: The api_request data from the API call that initiated the change. + ''' + if not (self.identifier and self.identifier.value and self.artifact_type): + return False + self.finalized = True + self.save() + + if api_request: + self.outcome.log_artifact_change( + action=OutcomeActions.ADD, + artifact=self, + api_request=api_request, + new_identifier=self.identifier.value + ) + + def delete(self, api_request=None, **kwargs): + '''Intercept `delete` behavior on the model instance and handles callbacks. + + Deletes from database if not `finalized` otherwise sets the `deleted` timestamp. + Logs the change on the parent Outcome if invoked via the API. + Attempts to delete the linked Identifier to avoid orphaned entries. + + Parameters: + api_request: The api_request data from the API call that initiated the change. + ''' + identifier = self.identifier + if self.finalized: + if api_request: + self.outcome.log_artifact_change( + action=OutcomeActions.REMOVE, + artifact=self, + api_request=api_request, + obsolete_identifier=identifier.value + ) + self.deleted = timezone.now() + self.save() + else: + super().delete(**kwargs) + + try: + identifier.delete() + except IdentifierHasReferencesError: + pass diff --git a/osf/models/outcomes.py b/osf/models/outcomes.py index 368dc0a30bd..6f7839d7b93 100644 --- a/osf/models/outcomes.py +++ b/osf/models/outcomes.py @@ -3,7 +3,8 @@ from osf.models.base import BaseModel, ObjectIDMixin from osf.models.mixins import EditableFieldsMixin -from osf.utils.outcomes import ArtifactTypes, NoPIDError +from osf.models.nodelog import NodeLog +from osf.utils.outcomes import ArtifactTypes, NoPIDError, OutcomeActions ''' This module defines the Outcome model and its custom manager. @@ -13,6 +14,12 @@ stored in the OutcomeArtifact through table. ''' +NODE_LOGS_FOR_OUTCOME_ACTION = { + OutcomeActions.ADD: NodeLog.RESOURCE_ADDED, + OutcomeActions.UPDATE: NodeLog.RESOURCE_UPDATED, + OutcomeActions.REMOVE: NodeLog.RESOURCE_REMOVED, +} + class OutcomeManager(models.Manager): @@ -72,11 +79,12 @@ class Outcome(ObjectIDMixin, EditableFieldsMixin, BaseModel): def primary_osf_resource(self): return self.artifact_metadata.get(artifact_type=ArtifactTypes.PRIMARY).identifier.referent - def add_artifact_by_pid(self, pid, artifact_type, pid_type='doi', create_identifier=False): - return self.artifacts.through.objects.create_for_identifier_value( - outcome=self, - pid_value=pid, - pid_type=pid_type, - artifact_type=artifact_type, - create_identifier=create_identifier + def log_artifact_change(self, action, artifact, api_request, **log_params): + nodelog_action = NODE_LOGS_FOR_OUTCOME_ACTION[action] + nodelog_params = {'artifact_id': artifact._id, **log_params} + + self.primary_osf_resource.add_log( + action=nodelog_action, + params=nodelog_params, + request=api_request, ) diff --git a/osf/utils/outcomes.py b/osf/utils/outcomes.py index f3f67ec1a51..f2b1d7a0b34 100644 --- a/osf/utils/outcomes.py +++ b/osf/utils/outcomes.py @@ -1,5 +1,6 @@ -from enum import IntEnum +from enum import Enum, IntEnum +from django.db.models import CharField, OuterRef, Subquery class NoPIDError(Exception): pass @@ -24,3 +25,31 @@ class ArtifactTypes(IntEnum): @classmethod def choices(cls): return tuple((entry.value, entry.name) for entry in cls) + + +class OutcomeActions(Enum): + ADD = 0 + UPDATE = 1 + REMOVE = 2 + + +def make_primary_resource_guid_annotation(base_queryset): + from osf.models import Guid + primary_artifacts_and_guids = base_queryset.filter( + artifact_type=ArtifactTypes.PRIMARY + ).annotate( + resource_guid=Subquery( + Guid.objects.filter( + content_type=OuterRef('identifier__content_type'), + object_id=OuterRef('identifier__object_id') + ).order_by('-created').values('_id')[:1], + output_field=CharField(), + ) + ) + + return Subquery( + primary_artifacts_and_guids.filter( + outcome_id=OuterRef('outcome_id') + ).values('resource_guid')[:1], + output_field=CharField() + ) diff --git a/osf_tests/test_outcomes.py b/osf_tests/test_outcomes.py index 87fc74f30d6..9a7f6e3b253 100644 --- a/osf_tests/test_outcomes.py +++ b/osf_tests/test_outcomes.py @@ -65,6 +65,7 @@ def test_outcome_for_registration__create_creates_primary_artifact( assert primary_artifact.identifier == registration_doi assert primary_artifact.pid == registration_doi.value assert primary_artifact.artifact_type == ArtifactTypes.PRIMARY + assert primary_artifact.primary_resource_guid == registration._id def test_outcome_for_registration__create_copies_metadata(self, registration, registration_doi): outcome = Outcome.objects.for_registration(registration, create=True) @@ -105,42 +106,6 @@ def external_doi(self): category='doi' ) - @pytest.mark.parametrize('doi_value', [TEST_PROJECT_DOI, TEST_EXTERNAL_DOI]) - def test_create_artifact_from_known_identifier( - self, outcome, project_doi, external_doi, doi_value - ): - new_artifact = OutcomeArtifact.objects.create_for_identifier_value( - outcome=outcome, - pid_value=TEST_PROJECT_DOI, - artifact_type=ArtifactTypes.MATERIALS - ) - - assert new_artifact.identifier == project_doi - assert new_artifact.outcome == outcome - assert new_artifact.artifact_type == ArtifactTypes.MATERIALS - - def test_create_artifact_from_unknown_identifier__create(self, outcome): - assert not Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() - - new_artifact = OutcomeArtifact.objects.create_for_identifier_value( - outcome=outcome, - pid_value=TEST_EXTERNAL_DOI, - create_identifier=True, - artifact_type=ArtifactTypes.CODE, - ) - - created_identifier = Identifier.objects.get(value=TEST_EXTERNAL_DOI) - assert new_artifact.identifier == created_identifier - - def test_create_artifact_from_unknown_identifer__no_create(self, outcome): - with pytest.raises(NoPIDError): - OutcomeArtifact.objects.create_for_identifier_value( - outcome=outcome, - pid_value=TEST_EXTERNAL_DOI, - create_identifier=False, - artifact_type=ArtifactTypes.SUPPLEMENTS - ) - def test_get_artifacts_for_registration(self, outcome, registration, project_doi, external_doi): assert not OutcomeArtifact.objects.for_registration(registration).exists() @@ -163,6 +128,141 @@ def test_get_artifacts_for_registration(self, outcome, registration, project_doi retrieved_project_artifact = registration_artifacts.get(identifier=project_doi) assert retrieved_project_artifact == project_artifact + assert retrieved_project_artifact.pid == TEST_PROJECT_DOI + assert retrieved_project_artifact.primary_resource_guid == registration._id retrieved_external_artifact = registration_artifacts.get(identifier=external_doi) assert retrieved_external_artifact == external_artifact + assert retrieved_external_artifact.pid == TEST_EXTERNAL_DOI + assert retrieved_external_artifact.primary_resource_guid == registration._id + + def test_update_identifier__get_existing_identifier(self, outcome, project_doi, external_doi): + test_artifact = outcome.artifact_metadata.create(artifact_type=ArtifactTypes.DATA) + test_artifact.update_identifier(new_pid_value=TEST_PROJECT_DOI) + assert test_artifact.identifier == project_doi + + def test_update_identifier__create_new_identifier(self, outcome, project_doi): + assert not Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + + test_artifact = outcome.artifact_metadata.create(artifact_type=ArtifactTypes.DATA) + test_artifact.update_identifier(new_pid_value=TEST_EXTERNAL_DOI) + + assert test_artifact.identifier.value == TEST_EXTERNAL_DOI + assert Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + + def test_update_identifier__deletes_previous_identifier_if_unreferenced(self, outcome, project_doi, external_doi): + assert Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + test_artifact = outcome.artifact_metadata.create( + identifier=external_doi, artifact_type=ArtifactTypes.DATA + ) + assert test_artifact.identifier != project_doi + + test_artifact.update_identifier(new_pid_value=project_doi.value) + assert test_artifact.identifier == project_doi + assert not Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + + def test_update_identifier__keeps_previous_identifier_if_osf_referent_exists(self, outcome, project_doi): + assert Identifier.objects.filter(value=TEST_PROJECT_DOI).exists() + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA + ) + + test_artifact.update_identifier(new_pid_value=TEST_EXTERNAL_DOI) + assert test_artifact.identifier != project_doi + assert Identifier.objects.filter(value=TEST_PROJECT_DOI).exists() + + def test_update_identifier__keeps_previous_identifier_if_part_of_other_outcomes( + self, outcome, project_doi, external_doi + ): + test_artifact = outcome.artifact_metadata.create( + identifier=external_doi, artifact_type=ArtifactTypes.DATA + ) + alternate_outcome = Outcome.objects.create() + alternate_outcome.artifact_metadata.create( + identifier=external_doi, artifact_type=ArtifactTypes.CODE + ) + + test_artifact.update_identifier(new_pid_value=project_doi.value) + assert test_artifact.identifier == project_doi + assert Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + + def test_update_identifier__no_change_if_same_pid(self, outcome, project_doi): + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA + ) + + test_artifact.update_identifier(new_pid_value=project_doi.value) + assert test_artifact.identifier == project_doi + + def test_delete_artifact__deletes_from_db_if_not_finalized(self, outcome, project_doi): + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=False + ) + + test_artifact.delete() + assert not OutcomeArtifact.objects.include_deleted().filter(id=test_artifact.id).exists() + + def test_delete_artifact__sets_deleted_if_finalized(self, outcome, project_doi): + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=True + ) + assert not test_artifact.deleted + + test_artifact.delete() + assert test_artifact.deleted + assert OutcomeArtifact.objects.include_deleted().filter(id=test_artifact.id).exists() + + @pytest.mark.parametrize('is_finalized', [True, False]) + def test_delete_artifact__deletes_identifier_if_unreferenced(self, outcome, external_doi, is_finalized): + assert Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + test_artifact = outcome.artifact_metadata.create( + identifier=external_doi, artifact_type=ArtifactTypes.DATA, finalized=is_finalized + ) + + test_artifact.delete() + assert not Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + + @pytest.mark.parametrize('is_finalized', [True, False]) + def test_delete_artifact__keeps_identifier_if_osf_referent_exists(self, outcome, project_doi, is_finalized): + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=is_finalized + ) + + test_artifact.delete() + assert Identifier.objects.filter(value=TEST_PROJECT_DOI).exists() + + @pytest.mark.parametrize('is_finalized', [True, False]) + def test_delete_artifact__keeps_identifier_if_part_of_other_outcomes(self, outcome, external_doi, is_finalized): + test_artifact = outcome.artifact_metadata.create( + identifier=external_doi, artifact_type=ArtifactTypes.DATA, finalized=is_finalized + ) + + alternate_outcome = Outcome.objects.create() + alternate_outcome.artifact_metadata.create( + identifier=external_doi, artifact_type=ArtifactTypes.CODE + ) + + test_artifact.delete() + assert Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() + + def test_deleted_artifact_excluded_from_outcome(self, outcome, project_doi): + project_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=True + ) + assert outcome.artifacts.count() == 2 + assert outcome.artifact_metadata.count() == 2 + + project_artifact.delete() + + assert outcome.artifacts.count() == 2 # Identifier links cannot be magically filtered out + assert outcome.artifact_metadata.count() == 1 # OutcomeArtifact links, however, should be disappeared + + def test_deleted_artifact_excluded_from_artifacts_for_registration(self, registration, outcome, project_doi): + project_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=True + ) + assert OutcomeArtifact.objects.for_registration(registration).exists() + + project_artifact.delete() + + assert not OutcomeArtifact.objects.for_registration(registration).exists() From 4010639217d988f8de1b55c752750828b3b82144 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Mon, 25 Jul 2022 14:06:19 -0400 Subject: [PATCH 2/4] Remove implicit exclusion of deleted OutcomeArtifacts --- osf/models/identifiers.py | 2 +- osf/models/outcome_artifacts.py | 9 +-------- osf_tests/test_outcomes.py | 26 ++------------------------ 3 files changed, 4 insertions(+), 33 deletions(-) diff --git a/osf/models/identifiers.py b/osf/models/identifiers.py index c7aa2cdfa46..a98b53a0a02 100644 --- a/osf/models/identifiers.py +++ b/osf/models/identifiers.py @@ -32,7 +32,7 @@ def remove(self, save=True): def delete(self): '''Used to delete an orphaned Identifier (distinct from setting `deleted`)''' - if self.object_id or self.artifact_metadata.exists(): + if self.object_id or self.artifact_metadata.filter(deleted__isnull=True).exists(): raise IdentifierHasReferencesError super().delete() diff --git a/osf/models/outcome_artifacts.py b/osf/models/outcome_artifacts.py index aa4de5e23a4..6729dd64b50 100644 --- a/osf/models/outcome_artifacts.py +++ b/osf/models/outcome_artifacts.py @@ -35,14 +35,7 @@ def get_queryset(self): return base_queryset.annotate( pid=models.F('identifier__value'), primary_resource_guid=outcome_utils.make_primary_resource_guid_annotation(base_queryset) - ).filter(deleted__isnull=True) - - def include_deleted(self): - '''Returns all OutcomeArtifacts, including those marked as `deleted`. - - Excludes other API-relevant annotations. - ''' - return super().get_queryset() + ) def for_registration(self, registration, identifier_type='doi'): '''Retrieves all OutcomeArtifacts sharing an Outcome, given the Primary Registration.''' diff --git a/osf_tests/test_outcomes.py b/osf_tests/test_outcomes.py index 9a7f6e3b253..0fbf98014f4 100644 --- a/osf_tests/test_outcomes.py +++ b/osf_tests/test_outcomes.py @@ -200,7 +200,7 @@ def test_delete_artifact__deletes_from_db_if_not_finalized(self, outcome, projec ) test_artifact.delete() - assert not OutcomeArtifact.objects.include_deleted().filter(id=test_artifact.id).exists() + assert not OutcomeArtifact.objects.filter(id=test_artifact.id).exists() def test_delete_artifact__sets_deleted_if_finalized(self, outcome, project_doi): test_artifact = outcome.artifact_metadata.create( @@ -210,7 +210,7 @@ def test_delete_artifact__sets_deleted_if_finalized(self, outcome, project_doi): test_artifact.delete() assert test_artifact.deleted - assert OutcomeArtifact.objects.include_deleted().filter(id=test_artifact.id).exists() + assert OutcomeArtifact.objects.filter(id=test_artifact.id).exists() @pytest.mark.parametrize('is_finalized', [True, False]) def test_delete_artifact__deletes_identifier_if_unreferenced(self, outcome, external_doi, is_finalized): @@ -244,25 +244,3 @@ def test_delete_artifact__keeps_identifier_if_part_of_other_outcomes(self, outco test_artifact.delete() assert Identifier.objects.filter(value=TEST_EXTERNAL_DOI).exists() - - def test_deleted_artifact_excluded_from_outcome(self, outcome, project_doi): - project_artifact = outcome.artifact_metadata.create( - identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=True - ) - assert outcome.artifacts.count() == 2 - assert outcome.artifact_metadata.count() == 2 - - project_artifact.delete() - - assert outcome.artifacts.count() == 2 # Identifier links cannot be magically filtered out - assert outcome.artifact_metadata.count() == 1 # OutcomeArtifact links, however, should be disappeared - - def test_deleted_artifact_excluded_from_artifacts_for_registration(self, registration, outcome, project_doi): - project_artifact = outcome.artifact_metadata.create( - identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=True - ) - assert OutcomeArtifact.objects.for_registration(registration).exists() - - project_artifact.delete() - - assert not OutcomeArtifact.objects.for_registration(registration).exists() From 3c053e97c17d53ea27da023bc7bff980e2e28068 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Mon, 25 Jul 2022 16:15:43 -0400 Subject: [PATCH 3/4] Add nodelog translations; validation (and tests) for finalize --- osf/exceptions.py | 14 ++++++ osf/models/outcome_artifacts.py | 18 ++++++-- osf/models/outcomes.py | 3 +- osf/utils/outcomes.py | 3 -- osf_tests/test_outcomes.py | 45 ++++++++++++++++++- .../static/js/anonymousLogActionsList.json | 5 ++- website/static/js/logActionsList.json | 5 ++- 7 files changed, 83 insertions(+), 10 deletions(-) diff --git a/osf/exceptions.py b/osf/exceptions.py index dd41ed93c6c..8b83bf5d174 100644 --- a/osf/exceptions.py +++ b/osf/exceptions.py @@ -227,3 +227,17 @@ def __init__(self, response, invalid_responses=None, unsupported_keys=None): class IdentifierHasReferencesError(OSFError): pass + + +class NoPIDError(OSFError): + pass + + +class CannotFinalizeArtifactError(OSFError): + + def __init__(self, artifact, incomplete_fields): + self.incomplete_fields = incomplete_fields + self.message = ( + f'Could not set `finalized=True` for OutcomeArtifact with id [{artifact._id}]. ' + f'The following required fields are not set: {incomplete_fields}' + ) diff --git a/osf/models/outcome_artifacts.py b/osf/models/outcome_artifacts.py index 6729dd64b50..c459af7e08c 100644 --- a/osf/models/outcome_artifacts.py +++ b/osf/models/outcome_artifacts.py @@ -1,7 +1,11 @@ from django.db import models from django.utils import timezone -from osf.exceptions import IdentifierHasReferencesError +from osf.exceptions import ( + CannotFinalizeArtifactError, + IdentifierHasReferencesError, + NoPIDError +) from osf.models.base import BaseModel, ObjectIDMixin from osf.models.identifiers import Identifier from osf.utils import outcomes as outcome_utils @@ -110,6 +114,8 @@ def update_identifier(self, new_pid_value, pid_type='doi', api_request=None): pid_type (str): The string "type" of the new PID (for now, only "doi" is supported) api_request: The api_request data from the API call that initiated the change. ''' + if not new_pid_value: + raise NoPIDError(f'Cannot assign an empty PID to OutcomeArtifact with ID {self._id}') previous_identifier = self.identifier self.identifier, _ = Identifier.objects.get_or_create( @@ -139,8 +145,14 @@ def finalize(self, api_request=None): Parameters: api_request: The api_request data from the API call that initiated the change. ''' - if not (self.identifier and self.identifier.value and self.artifact_type): - return False + incomplete_fields = [] + if not (self.identifier and self.identifier.value): + incomplete_fields.append('identifier__value') + if not self.artifact_type: + incomplete_fields.append('artifact_type') + if incomplete_fields: + raise CannotFinalizeArtifactError(self, incomplete_fields) + self.finalized = True self.save() diff --git a/osf/models/outcomes.py b/osf/models/outcomes.py index 6f7839d7b93..695aaf9b5d4 100644 --- a/osf/models/outcomes.py +++ b/osf/models/outcomes.py @@ -1,10 +1,11 @@ from django.db import models from django.utils.functional import cached_property +from osf.exceptions import NoPIDError from osf.models.base import BaseModel, ObjectIDMixin from osf.models.mixins import EditableFieldsMixin from osf.models.nodelog import NodeLog -from osf.utils.outcomes import ArtifactTypes, NoPIDError, OutcomeActions +from osf.utils.outcomes import ArtifactTypes, OutcomeActions ''' This module defines the Outcome model and its custom manager. diff --git a/osf/utils/outcomes.py b/osf/utils/outcomes.py index f2b1d7a0b34..c0b655ee7ea 100644 --- a/osf/utils/outcomes.py +++ b/osf/utils/outcomes.py @@ -2,9 +2,6 @@ from django.db.models import CharField, OuterRef, Subquery -class NoPIDError(Exception): - pass - class ArtifactTypes(IntEnum): '''Labels used to classify artifacts. diff --git a/osf_tests/test_outcomes.py b/osf_tests/test_outcomes.py index 0fbf98014f4..6fabb29fb03 100644 --- a/osf_tests/test_outcomes.py +++ b/osf_tests/test_outcomes.py @@ -1,6 +1,8 @@ import pytest + +from osf.exceptions import CannotFinalizeArtifactError, NoPIDError from osf.models import Identifier, Outcome, OutcomeArtifact -from osf.utils.outcomes import ArtifactTypes, NoPIDError +from osf.utils.outcomes import ArtifactTypes from osf_tests.factories import ProjectFactory, RegistrationFactory @@ -194,6 +196,47 @@ def test_update_identifier__no_change_if_same_pid(self, outcome, project_doi): test_artifact.update_identifier(new_pid_value=project_doi.value) assert test_artifact.identifier == project_doi + @pytest.mark.parametrize('empty_value', ['', None]) + def test_update_identifier__raises_if_empty_pid(self, outcome, project_doi, empty_value): + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA + ) + + with pytest.raises(NoPIDError): + test_artifact.update_identifier(new_pid_value=empty_value) + + def test_finalize__raises__missing_identifier(self, outcome): + test_artifact = outcome.artifact_metadata.create(artifact_type=ArtifactTypes.DATA) + + with pytest.raises(CannotFinalizeArtifactError) as caught: + test_artifact.finalize() + assert caught.value.incomplete_fields == ['identifier__value'] + + def test_finalize__raises__missing_identifier_value(self, outcome, project_doi): + test_artifact = outcome.artifact_metadata.create( + identifier=project_doi, artifact_type=ArtifactTypes.DATA + ) + project_doi.value = '' + project_doi.save() + + with pytest.raises(CannotFinalizeArtifactError) as caught: + test_artifact.finalize() + assert caught.value.incomplete_fields == ['identifier__value'] + + def test_finalize__raises__missing_artifact_type(self, outcome, project_doi): + test_artifact = outcome.artifact_metadata.create(identifier=project_doi) + + with pytest.raises(CannotFinalizeArtifactError) as caught: + test_artifact.finalize() + assert caught.value.incomplete_fields == ['artifact_type'] + + def test_finalize__raises__missing_both(self, outcome): + test_artifact = outcome.artifact_metadata.create() + + with pytest.raises(CannotFinalizeArtifactError) as caught: + test_artifact.finalize() + assert caught.value.incomplete_fields == ['identifier__value', 'artifact_type'] + def test_delete_artifact__deletes_from_db_if_not_finalized(self, outcome, project_doi): test_artifact = outcome.artifact_metadata.create( identifier=project_doi, artifact_type=ArtifactTypes.DATA, finalized=False diff --git a/website/static/js/anonymousLogActionsList.json b/website/static/js/anonymousLogActionsList.json index 6e7f5ebed26..4f222c86725 100644 --- a/website/static/js/anonymousLogActionsList.json +++ b/website/static/js/anonymousLogActionsList.json @@ -91,5 +91,8 @@ "subjects_updated": "A user updated the subjects", "view_only_link_added": "A user created a view-only link to a project", "view_only_link_removed": "A user removed a view-only link to a project", - "migrated_quickfiles": "QuickFiles were migrated into a public project" + "migrated_quickfiles": "QuickFiles were migrated into a public project", + "resource_identifier_added": "A Resource has been added to the Node", + "resource_identifier_removed": "A Resource has been removed from the Node", + "resource_identifier_updated": "A Resource on the Node has had its PID updated" } diff --git a/website/static/js/logActionsList.json b/website/static/js/logActionsList.json index da34947d7c5..56d3aa925d4 100644 --- a/website/static/js/logActionsList.json +++ b/website/static/js/logActionsList.json @@ -101,5 +101,8 @@ "prereg_links_updated": "${user} has updated their preregistration data links", "why_no_prereg_updated": "${user} has updated their preregistration data availability statement", "prereg_links_info_updated": "${user} has updated their preregistration links to ${value}", - "migrated_quickfiles": "${user} had their QuickFiles migrated into ${node}" + "migrated_quickfiles": "${user} had their QuickFiles migrated into ${node}", + "resource_identifier_added": "${user} has added a Resource with DOI ${new_identifier} to Registration ${node}", + "resource_identifier_removed": "${user} has removed a Resource with DOI ${obsolete_identifier} to Registration ${node}", + "resource_identifier_updated": "${user} has updated a Resource DOI on Registration ${node} from ${obsolete_identifier} to ${new_identifier}" } From b787304b79a2c709f51efe4aaa410b793689ced6 Mon Sep 17 00:00:00 2001 From: Jon Walz Date: Tue, 26 Jul 2022 11:36:07 -0400 Subject: [PATCH 4/4] ArtifactTypes.CODE -> ArtifactTypes.ANALYTIC_CODE --- osf/migrations/0247_artifact_finalized_and_deleted.py | 5 +++++ osf/models/outcomes.py | 3 ++- osf/utils/outcomes.py | 2 +- osf_tests/test_outcomes.py | 6 +++--- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/osf/migrations/0247_artifact_finalized_and_deleted.py b/osf/migrations/0247_artifact_finalized_and_deleted.py index e8b3194993d..24f4a19fe26 100644 --- a/osf/migrations/0247_artifact_finalized_and_deleted.py +++ b/osf/migrations/0247_artifact_finalized_and_deleted.py @@ -33,4 +33,9 @@ class Migration(migrations.Migration): name='title', field=models.TextField(blank=True), ), + migrations.AlterField( + model_name='outcomeartifact', + name='artifact_type', + field=models.IntegerField(choices=[(0, 'UNDEFINED'), (1, 'DATA'), (11, 'ANALYTIC_CODE'), (21, 'MATERIALS'), (31, 'PAPERS'), (41, 'SUPPLEMENTS'), (1001, 'PRIMARY')], default=osf.utils.outcomes.ArtifactTypes(0)), + ), ] diff --git a/osf/models/outcomes.py b/osf/models/outcomes.py index 695aaf9b5d4..b620b630841 100644 --- a/osf/models/outcomes.py +++ b/osf/models/outcomes.py @@ -41,7 +41,8 @@ def for_registration(self, registration, identifier_type='doi', create=False, ** new_outcome.copy_editable_fields(registration, include_contributors=False) new_outcome.artifact_metadata.create( identifier=registration_identifier, - artifact_type=ArtifactTypes.PRIMARY + artifact_type=ArtifactTypes.PRIMARY, + finalized=True, ) return new_outcome diff --git a/osf/utils/outcomes.py b/osf/utils/outcomes.py index c0b655ee7ea..1dde6c9e169 100644 --- a/osf/utils/outcomes.py +++ b/osf/utils/outcomes.py @@ -13,7 +13,7 @@ class ArtifactTypes(IntEnum): ''' UNDEFINED = 0 DATA = 1 - CODE = 11 + ANALYTIC_CODE = 11 MATERIALS = 21 PAPERS = 31 SUPPLEMENTS = 41 diff --git a/osf_tests/test_outcomes.py b/osf_tests/test_outcomes.py index 6fabb29fb03..7d0e0bcce99 100644 --- a/osf_tests/test_outcomes.py +++ b/osf_tests/test_outcomes.py @@ -121,7 +121,7 @@ def test_get_artifacts_for_registration(self, outcome, registration, project_doi # Add another Artifact for one of the identifiers to make sure it doesn't get picked up, too bogus_outcome = Outcome.objects.create() bogus_outcome.artifact_metadata.create( - identifier=external_doi, artifact_type=ArtifactTypes.CODE + identifier=external_doi, artifact_type=ArtifactTypes.ANALYTIC_CODE ) registration_artifacts = OutcomeArtifact.objects.for_registration(registration) @@ -181,7 +181,7 @@ def test_update_identifier__keeps_previous_identifier_if_part_of_other_outcomes( ) alternate_outcome = Outcome.objects.create() alternate_outcome.artifact_metadata.create( - identifier=external_doi, artifact_type=ArtifactTypes.CODE + identifier=external_doi, artifact_type=ArtifactTypes.ANALYTIC_CODE ) test_artifact.update_identifier(new_pid_value=project_doi.value) @@ -282,7 +282,7 @@ def test_delete_artifact__keeps_identifier_if_part_of_other_outcomes(self, outco alternate_outcome = Outcome.objects.create() alternate_outcome.artifact_metadata.create( - identifier=external_doi, artifact_type=ArtifactTypes.CODE + identifier=external_doi, artifact_type=ArtifactTypes.ANALYTIC_CODE ) test_artifact.delete()