From 37902b603f70052464bf4670d2284b516b2a727c Mon Sep 17 00:00:00 2001 From: mkovalua Date: Fri, 30 Oct 2026 17:36:34 +0200 Subject: [PATCH] Remove Primary Preprint File from draft state --- addons/base/views.py | 7 +++ addons/osfstorage/models.py | 7 ++- addons/osfstorage/tests/test_models.py | 29 +++++++++++++ addons/osfstorage/tests/test_views.py | 37 ++++++++++++++++ api_tests/actions/views/test_action_list.py | 34 +++++++++++++++ osf/models/preprint.py | 19 +++++++- osf_tests/test_reviewable.py | 48 +++++++++++++++++++++ tests/test_preprints.py | 20 +++++++++ website/views.py | 3 ++ 9 files changed, 201 insertions(+), 3 deletions(-) diff --git a/addons/base/views.py b/addons/base/views.py index 2c47a132768..b945da373ad 100644 --- a/addons/base/views.py +++ b/addons/base/views.py @@ -1134,6 +1134,13 @@ def persistent_file_download(auth, **kwargs): referent, _ = Guid.load_referent(id_or_guid) file = referent.primary_file + # Initial and rejected preprints may have no primary file + if file is None: + raise HTTPError(http_status.HTTP_404_NOT_FOUND, data={ + 'message_short': 'File Not Found', + 'message_long': 'The requested file could not be found.' + }) + if not file.is_file: raise HTTPError(http_status.HTTP_400_BAD_REQUEST, data={ 'message_long': 'Downloading folders is not permitted.' diff --git a/addons/osfstorage/models.py b/addons/osfstorage/models.py index 0da2e02ac0e..79706aac27c 100644 --- a/addons/osfstorage/models.py +++ b/addons/osfstorage/models.py @@ -145,7 +145,8 @@ def current_version_number(self): return int(latest_identifier) if latest_identifier else 1 def _check_delete_allowed(self): - if self.is_preprint_primary: + # Initial and rejected preprints may be left without a primary file, so authors can replace a wrong one + if self.is_preprint_primary and self.target.requires_primary_file: raise exceptions.FileNodeIsPrimaryFile() if self.is_checked_out: raise exceptions.FileNodeCheckedOutError() @@ -439,7 +440,11 @@ def remove_tags(self, tags, auth, save=True): self.remove_tag(tag, auth, save) def delete(self, user=None, **kwargs): + preprint = self.target if self.is_preprint_primary else None ret = super().delete(user, **kwargs) + if preprint: + # reachable when the preprint allows having no primary file + preprint.remove_primary_file() self.update_search() return ret diff --git a/addons/osfstorage/tests/test_models.py b/addons/osfstorage/tests/test_models.py index 5a7f7bd0456..852fed90a70 100644 --- a/addons/osfstorage/tests/test_models.py +++ b/addons/osfstorage/tests/test_models.py @@ -311,6 +311,35 @@ def test_delete_preprint_primary_file(self): with pytest.raises(FileNodeIsPrimaryFile): file.delete() + def test_delete_primary_file_of_pending_preprint(self): + preprint = PreprintFactory(is_published=False, machine_state='pending') + file = preprint.files.get() + + with pytest.raises(FileNodeIsPrimaryFile): + file.delete() + preprint.reload() + assert preprint.primary_file == file + + def test_delete_primary_file_of_initial_or_rejected_preprint(self): + for machine_state in ('initial', 'rejected'): + preprint = PreprintFactory(is_published=False, machine_state=machine_state) + file = preprint.files.get() + + file.delete() + preprint.reload() + assert preprint.primary_file is None + assert preprint.machine_state == machine_state + assert models.TrashedFile.load(file._id) + + def test_delete_folder_with_primary_file_of_initial_preprint(self): + preprint = PreprintFactory(is_published=False) + folder = preprint.root_folder.append_folder('Drafts') + preprint.primary_file.move_under(folder) + + folder.delete() + preprint.reload() + assert preprint.primary_file is None + def test_delete_file_no_guid(self): child = self.node_settings.get_root().append_file('Test') diff --git a/addons/osfstorage/tests/test_views.py b/addons/osfstorage/tests/test_views.py index f7de3389975..b204c41a61f 100644 --- a/addons/osfstorage/tests/test_views.py +++ b/addons/osfstorage/tests/test_views.py @@ -1091,6 +1091,30 @@ def test_attempt_delete_while_preprint(self): res = self.delete(self.preprint.primary_file) assert res.status_code == 403 + def test_attempt_delete_while_preprint_pending(self): + self.node = self.preprint = PreprintFactory(creator=self.user, is_published=False, machine_state='pending') + res = self.delete(self.preprint.primary_file) + assert res.status_code == 403 + assert res.json['message_long'] == 'Cannot delete file as it is the primary file of preprint.' + + def test_delete_while_preprint_initial(self): + self.node = self.preprint = PreprintFactory(creator=self.user, is_published=False, machine_state='initial') + primary_file = self.preprint.primary_file + res = self.delete(primary_file) + assert res.status_code == 200 + self.preprint.reload() + assert self.preprint.primary_file is None + assert models.TrashedFileNode.load(primary_file._id) + + def test_delete_while_preprint_rejected(self): + self.node = self.preprint = PreprintFactory(creator=self.user, is_published=False, machine_state='rejected') + primary_file = self.preprint.primary_file + res = self.delete(primary_file) + assert res.status_code == 200 + self.preprint.reload() + assert self.preprint.primary_file is None + assert models.TrashedFileNode.load(primary_file._id) + def test_attempt_delete_folder_with_preprint(self): folder = self.root_node.append_folder('Fishes') file = folder.append_file('Fish') @@ -1659,6 +1683,19 @@ def test_download_file(self): redirect = self.app.get(url, auth=self.user.auth) assert redirect.status_code == 400 + def test_download_preprint_without_primary_file(self): + self.preprint = PreprintFactory(creator=self.user, is_published=False, machine_state='rejected') + res = self.app.get(f'/download/{self.preprint._id}/', auth=self.user.auth) + assert res.status_code == 302 + res = self.app.get(f'/{self.preprint._id}/download/', auth=self.user.auth) + assert res.status_code == 302 + + self.preprint.remove_primary_file() + res = self.app.get(f'/download/{self.preprint._id}/', auth=self.user.auth) + assert res.status_code == 404 + res = self.app.get(f'/{self.preprint._id}/download/', auth=self.user.auth) + assert res.status_code == 404 + @responses.activate @mock.patch('framework.auth.cas.get_client') def test_download_file_with_token(self, mock_get_client): diff --git a/api_tests/actions/views/test_action_list.py b/api_tests/actions/views/test_action_list.py index 46e77d2fc16..efc59a5b6ef 100644 --- a/api_tests/actions/views/test_action_list.py +++ b/api_tests/actions/views/test_action_list.py @@ -393,6 +393,40 @@ def test_submit_preprint_without_files_returns_400(self, app, url, preprint, nod expect_errors=True ) assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'A primary file is required to submit a preprint for moderation.' + + def test_resubmit_rejected_preprint_without_file_returns_400(self, app, url, preprint, node_admin): + preprint.machine_state = 'rejected' + preprint.primary_file = None + preprint.save() + + res = app.post_json_api( + url, + self.create_payload(preprint._id, trigger='submit'), + auth=node_admin.auth, + expect_errors=True + ) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'A primary file is required to submit a preprint for moderation.' + preprint.refresh_from_db() + assert preprint.machine_state == 'rejected' + assert not preprint.actions.exists() + + def test_accept_rejected_preprint_without_file_returns_400(self, app, url, preprint, moderator): + preprint.machine_state = 'rejected' + preprint.primary_file = None + preprint.save() + + res = app.post_json_api( + url, + self.create_payload(preprint._id, trigger='accept', comment='Looks good.'), + auth=moderator.auth, + expect_errors=True + ) + assert res.status_code == 400 + preprint.refresh_from_db() + assert preprint.machine_state == 'rejected' + assert not preprint.is_published def test_provider_not_reviewed_returns_409(self, app, url, preprint, node_admin): preprint.provider = PreprintProviderFactory(reviews_workflow=None) diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 38312b7975d..cf5b112351f 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -708,6 +708,12 @@ def is_preprint_orphan(self): def has_submitted_preprint(self): return self.machine_state != DefaultStates.INITIAL.value + @property + def requires_primary_file(self): + # Authors may leave a preprint without a primary file while they are still working on it: before the first + # submission or after a moderator rejected it + return self.machine_state not in (DefaultStates.INITIAL.value, DefaultStates.REJECTED.value) + @property def is_pending_moderation(self): if self.machine_state == DefaultStates.INITIAL.value: @@ -927,6 +933,12 @@ def set_primary_file(self, preprint_file, auth, save=False, **kwargs): self.save() update_or_enqueue_on_preprint_updated(preprint_id=self._id, saved_fields=['primary_file']) + def remove_primary_file(self): + """Unlink the primary file, after it was deleted""" + self.primary_file = None + self.save() + update_or_enqueue_on_preprint_updated(preprint_id=self._id, saved_fields=['primary_file']) + @require_permission([ADMIN]) def set_published(self, published, auth, save=False, **kwargs): if self.is_published and not published: @@ -1045,8 +1057,8 @@ def save(self, *args, **kwargs): if not first_save and ('ever_public' in saved_fields and saved_fields['ever_public']): raise ValidationError('Cannot set "ever_public" to False') - if self.has_submitted_preprint and not self.primary_file: - raise ValidationError('Cannot save non-initial preprint without primary file.') + if self.requires_primary_file and not self.primary_file: + raise ValidationError('Only initial or rejected preprints can be saved without a primary file.') ret = super().save(*args, **kwargs) @@ -1697,6 +1709,9 @@ def run_submit(self, user): Params: user: The user triggering this transition. """ + # Initial and rejected preprints may have no primary file, but one is required to (re)submit for moderation + if self.is_preprint_orphan: + raise ValueError('A primary file is required to submit a preprint for moderation.') ret = super().run_submit(user=user) provider = self.provider reviews_workflow = provider.reviews_workflow diff --git a/osf_tests/test_reviewable.py b/osf_tests/test_reviewable.py index 954abd6ee1f..ec09c6139c2 100644 --- a/osf_tests/test_reviewable.py +++ b/osf_tests/test_reviewable.py @@ -1,10 +1,20 @@ from unittest import mock import pytest +from addons.osfstorage.models import OsfStorageFile +from api_tests.utils import create_test_preprint_file +from framework.auth import Auth from osf.models import Preprint, NotificationTypeEnum from osf.utils.workflows import DefaultStates from osf_tests.factories import PreprintFactory, AuthUserFactory from tests.utils import capture_notifications +from website.files.exceptions import FileNodeIsPrimaryFile + + +def delete_primary_file(preprint): + # Load the file fresh, like WaterButler's delete hook does, so it sees the preprint's current state + OsfStorageFile.objects.get(id=preprint.primary_file_id).delete() + preprint.refresh_from_db() @pytest.mark.django_db @@ -61,3 +71,41 @@ def test_reject_resubmission_sends_emails(self): assert len(notifications['emits']) == 1 assert notifications['emits'][0]['type'] == NotificationTypeEnum.PROVIDER_REVIEWS_RESUBMISSION_CONFIRMATION assert preprint.machine_state == DefaultStates.PENDING.value + + def test_submit_requires_primary_file(self): + user = AuthUserFactory() + preprint = PreprintFactory(reviews_workflow='pre-moderation', is_published=False, creator=user) + delete_primary_file(preprint) + assert preprint.primary_file is None + + with pytest.raises(ValueError, match='A primary file is required'): + preprint.run_submit(user) + preprint.refresh_from_db() + assert preprint.machine_state == DefaultStates.INITIAL.value + assert not preprint.actions.exists() + + def test_rejected_preprint_can_replace_primary_file_and_resubmit(self): + user = AuthUserFactory() + preprint = PreprintFactory(reviews_workflow='pre-moderation', is_published=False, creator=user) + with capture_notifications(): + preprint.run_submit(user) + with pytest.raises(FileNodeIsPrimaryFile): + delete_primary_file(preprint) + + with capture_notifications(): + preprint.run_reject(user, 'wrong file') + delete_primary_file(preprint) + assert preprint.machine_state == DefaultStates.REJECTED.value + assert preprint.primary_file is None + + with pytest.raises(ValueError, match='A primary file is required'): + preprint.run_submit(user) + preprint.refresh_from_db() + assert preprint.machine_state == DefaultStates.REJECTED.value + + new_file = create_test_preprint_file(preprint, user, filename='correct_file.pdf') + preprint.set_primary_file(new_file, auth=Auth(user), save=True) + with capture_notifications(): + preprint.run_submit(user) + assert preprint.machine_state == DefaultStates.PENDING.value + assert preprint.primary_file == new_file diff --git a/tests/test_preprints.py b/tests/test_preprints.py index 7d669ee3e3f..6dba85544ad 100644 --- a/tests/test_preprints.py +++ b/tests/test_preprints.py @@ -1521,6 +1521,26 @@ def test_cant_update_without_file(self): with pytest.raises(ValidationError): self.preprint.save() + def test_can_save_rejected_preprint_without_file(self): + self.preprint.set_primary_file(self.file, auth=self.auth, save=True) + self.preprint.machine_state = DefaultStates.REJECTED.value + self.preprint.save() + + self.preprint.remove_primary_file() + self.preprint.reload() + assert self.preprint.primary_file is None + assert self.preprint.is_preprint_orphan + + def test_cant_save_pending_preprint_without_file(self): + self.preprint.set_primary_file(self.file, auth=self.auth, save=True) + self.preprint.machine_state = DefaultStates.PENDING.value + self.preprint.save() + + with pytest.raises(ValidationError): + self.preprint.remove_primary_file() + self.preprint.reload() + assert self.preprint.primary_file == self.file + class TestPreprintPermissions(OsfTestCase): def setUp(self): diff --git a/website/views.py b/website/views.py index d068e48f929..995b49666c8 100644 --- a/website/views.py +++ b/website/views.py @@ -224,6 +224,9 @@ def resolve_guid_download(guid, provider=None): resource.has_permission(auth.user, permissions.ADMIN)): raise HTTPError(http_status.HTTP_404_NOT_FOUND) resource = resource.primary_file + # Initial and rejected preprints may have no primary file + if resource is None: + raise HTTPError(http_status.HTTP_404_NOT_FOUND) request.args = request.args.copy() if 'revision' not in request.args: # This is to maintain legacy behavior