From 7c45129e38843ccb6c1731a70dd0dc6c5a3f6432 Mon Sep 17 00:00:00 2001 From: sh-andriy <105591819+sh-andriy@users.noreply.github.com> Date: Thu, 25 Jun 2026 19:52:52 +0300 Subject: [PATCH 1/5] [ENG-11068] | bound external HTTP in celery tasks to stop worker stalls (#11777) --- api/share/utils.py | 4 + api_tests/share/test_request_timeout.py | 36 +++++++++ framework/sessions/__init__.py | 15 +--- osf/external/askismet/client.py | 9 ++- osf/external/askismet/tasks.py | 11 ++- osf/external/cedar/client.py | 4 +- osf/external/gravy_valet/request_helpers.py | 2 +- osf/external/oopspam/client.py | 3 +- osf/external/spam/tasks.py | 7 +- osf/management/commands/force_archive.py | 4 +- osf/metrics/reporters/preprint_count.py | 6 +- osf_tests/users/test_last_login_date.py | 82 --------------------- website/archiver/tasks.py | 2 +- website/identifiers/clients/crossref.py | 2 + website/settings/defaults.py | 7 ++ 15 files changed, 84 insertions(+), 110 deletions(-) create mode 100644 api_tests/share/test_request_timeout.py delete mode 100644 osf_tests/users/test_last_login_date.py diff --git a/api/share/utils.py b/api/share/utils.py index d9ffd388771..14a739e5631 100644 --- a/api/share/utils.py +++ b/api/share/utils.py @@ -170,6 +170,8 @@ def _schedule_cedar_record_updates(guid_instance): acks_late=True, max_retries=4, retry_backoff=True, + soft_time_limit=settings.SHARE_UPDATE_TASK_SOFT_TIME_LIMIT, + time_limit=settings.SHARE_UPDATE_TASK_HARD_TIME_LIMIT, ) def task__update_share(self, guid: str, is_backfill=False, osfmap_partition_name='MAIN'): """ @@ -278,6 +280,7 @@ def pls_send_trove_record(osf_item, *, is_backfill: bool, osfmap_partition: Osfm **_shtrove_auth_headers(osf_item), }, data=ensure_bytes(_serialized_record), + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) @@ -288,6 +291,7 @@ def pls_delete_trove_record(osf_item, osfmap_partition: OsfmapPartition): 'record_identifier': _shtrove_record_identifier(osf_item, osfmap_partition), }, headers=_shtrove_auth_headers(osf_item), + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) diff --git a/api_tests/share/test_request_timeout.py b/api_tests/share/test_request_timeout.py new file mode 100644 index 00000000000..ff7f2670482 --- /dev/null +++ b/api_tests/share/test_request_timeout.py @@ -0,0 +1,36 @@ +from unittest import mock + +import pytest + +from api.share import utils as share_utils +from osf.metadata.osf_gathering import OsfmapPartition +from osf_tests.factories import ProjectFactory +from website import settings + + +@pytest.mark.django_db +class TestShareRequestTimeout: + + @pytest.fixture() + def public_node(self): + return ProjectFactory(is_public=True) + + def test_delete_trove_record_passes_timeout(self, public_node): + with mock.patch.object(share_utils.requests, 'delete') as mock_delete: + share_utils.pls_delete_trove_record(public_node, osfmap_partition=OsfmapPartition.MAIN) + assert mock_delete.call_args.kwargs['timeout'] == settings.EXTERNAL_REQUEST_TIMEOUT + + def test_send_trove_record_passes_timeout(self, public_node): + fake_serializer = mock.Mock(mediatype='text/turtle') + fake_serializer.serialize.return_value = b'' + with ( + mock.patch.object(share_utils, 'pls_get_magic_metadata_basket'), + mock.patch.object(share_utils, 'get_metadata_serializer', return_value=fake_serializer), + mock.patch.object(share_utils.requests, 'post') as mock_post, + ): + share_utils.pls_send_trove_record( + public_node, + is_backfill=False, + osfmap_partition=OsfmapPartition.MAIN, + ) + assert mock_post.call_args.kwargs['timeout'] == settings.EXTERNAL_REQUEST_TIMEOUT diff --git a/framework/sessions/__init__.py b/framework/sessions/__init__.py index 72939c1ec5d..ee52e2a4543 100644 --- a/framework/sessions/__init__.py +++ b/framework/sessions/__init__.py @@ -4,13 +4,11 @@ from urllib.parse import urlparse, parse_qs, urlunparse, urlencode from django.apps import apps -from django.utils import timezone from django.conf import settings as django_conf_settings import itsdangerous from flask import request, g from furl import furl -from framework.celery_tasks.handlers import enqueue_task from framework.flask import redirect from osf.utils.fields import ensure_str from osf.exceptions import InvalidCookieOrSessionError @@ -213,7 +211,7 @@ def before_request(): cookie = request.cookies.get(settings.COOKIE_NAME) if cookie: try: - user_session = flask_get_session_from_cookie(cookie) + flask_get_session_from_cookie(cookie) except InvalidCookieOrSessionError: # If invalid session/cookie happens, only remove the invalid cookie and redirect to the same request. # This ensures users landing on the page/link they previously clicked. @@ -226,17 +224,6 @@ def before_request(): response = redirect(redirect_url) response.delete_cookie(settings.COOKIE_NAME, domain=settings.OSF_COOKIE_DOMAIN) return response - # Case 1: anonymous session that is used for first time external (e.g. ORCiD) login only - if user_session.get('auth_user_external_first_login', False) is True: - return - # Case 2: session without authenticated user - user_id = user_session.get('auth_user_id', None) - if not user_id: - return - # Case 3: authenticated session with user - # Update date last login when making non-api requests - from framework.auth.tasks import update_user_from_activity - enqueue_task(update_user_from_activity.s(user_id, timezone.now().timestamp(), cas_login=False)) def after_request(response): diff --git a/osf/external/askismet/client.py b/osf/external/askismet/client.py index db57b1d3cfa..eaf726dd740 100644 --- a/osf/external/askismet/client.py +++ b/osf/external/askismet/client.py @@ -38,7 +38,8 @@ def _is_apikey_valid(self): 'key': self.apikey, 'blog': self.website }, - headers=self._default_headers + headers=self._default_headers, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) self._apikey_is_valid = res.text == 'valid' return self._is_apikey_valid() @@ -108,7 +109,8 @@ def submit_spam(self, user_ip, user_agent, **kwargs): res = requests.post( f'{self.API_PROTOCOL}{self.apikey}.{self.API_HOST}/1.1/submit-spam', data=data, - headers=self._default_headers + headers=self._default_headers, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) if res.status_code != requests.codes.ok: raise AkismetClientError(reason=res.text) @@ -129,7 +131,8 @@ def submit_ham(self, user_ip, user_agent, **kwargs): res = requests.post( f'{self.API_PROTOCOL}{self.apikey}.{self.API_HOST}/1.1/submit-ham', data=data, - headers=self._default_headers + headers=self._default_headers, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) if res.status_code != requests.codes.ok: raise AkismetClientError(reason=res.text) diff --git a/osf/external/askismet/tasks.py b/osf/external/askismet/tasks.py index f77516204b1..083f4bb86d3 100644 --- a/osf/external/askismet/tasks.py +++ b/osf/external/askismet/tasks.py @@ -1,9 +1,13 @@ from framework.celery_tasks import app as celery_app from osf.external.askismet.client import AkismetClient +from website import settings -@celery_app.task() +@celery_app.task( + soft_time_limit=settings.SPAM_SUBMIT_TASK_SOFT_TIME_LIMIT, + time_limit=settings.SPAM_SUBMIT_TASK_HARD_TIME_LIMIT, +) def submit_spam(guid): from osf.models import Guid resource = Guid.load(guid).referent @@ -20,7 +24,10 @@ def submit_spam(guid): ) -@celery_app.task() +@celery_app.task( + soft_time_limit=settings.SPAM_SUBMIT_TASK_SOFT_TIME_LIMIT, + time_limit=settings.SPAM_SUBMIT_TASK_HARD_TIME_LIMIT, +) def submit_ham(guid): from osf.models import Guid resource = Guid.load(guid).referent diff --git a/osf/external/cedar/client.py b/osf/external/cedar/client.py index e748d7ccc9c..130be44b171 100644 --- a/osf/external/cedar/client.py +++ b/osf/external/cedar/client.py @@ -19,7 +19,7 @@ class CedarClient: def retrieve_all_template_ids(self): url = f'{self.host}folders/{self.home_folder_id}/contents/?resource_types=template' try: - r = requests.get(url, headers=self.headers) + r = requests.get(url, headers=self.headers, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) r.raise_for_status() except RequestException: raise CedarClientRequestError( @@ -36,7 +36,7 @@ def retrieve_all_template_ids(self): def retrieve_template_by_id(self, template_id): url = f'{self.host}templates/{quote_plus(template_id)}' try: - r = requests.get(url, headers=self.headers) + r = requests.get(url, headers=self.headers, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) r.raise_for_status() except RequestException: raise CedarClientRequestError(reason=f'Fail to complete Cedar API request: template_id={template_id}') diff --git a/osf/external/gravy_valet/request_helpers.py b/osf/external/gravy_valet/request_helpers.py index a4f7d37694b..48096ef8429 100644 --- a/osf/external/gravy_valet/request_helpers.py +++ b/osf/external/gravy_valet/request_helpers.py @@ -283,7 +283,7 @@ def _make_gv_request( ) assert not (request_method == 'GET' and json_data is not None) try: - response = requests.request(url=endpoint_url, headers=auth_headers, params=params, method=request_method, json=json_data) + response = requests.request(url=endpoint_url, headers=auth_headers, params=params, method=request_method, json=json_data, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) except RequestException as e: logger.error(f"Cannot reach GravyValet: {e}") return None diff --git a/osf/external/oopspam/client.py b/osf/external/oopspam/client.py index 0abdfdd021f..151a102ec37 100644 --- a/osf/external/oopspam/client.py +++ b/osf/external/oopspam/client.py @@ -34,7 +34,8 @@ def check_content(self, user_ip, content, **kwargs): 'content-type': 'application/json', 'x-rapidapi-key': self.apikey, 'x-rapidapi-host': 'oopspam.p.rapidapi.com' - } + }, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT ) if response.status_code != requests.codes.ok: diff --git a/osf/external/spam/tasks.py b/osf/external/spam/tasks.py index 4cb4c7784b5..d67b21912dc 100644 --- a/osf/external/spam/tasks.py +++ b/osf/external/spam/tasks.py @@ -24,6 +24,7 @@ def reclassify_domain_references(notable_domain_id, current_note, previous_note) from osf.models.notable_domain import DomainReference, NotableDomain domain = NotableDomain.load(notable_domain_id) references = DomainReference.objects.filter(domain=domain) + referrers = [] with transaction.atomic(): for item in references: item.is_triaged = current_note != NotableDomain.Note.UNKNOWN @@ -37,7 +38,11 @@ def reclassify_domain_references(notable_domain_id, current_note, previous_note) if not item.referrer.spam_data.get('domains') and not item.referrer.spam_data.get('who_flagged'): item.referrer.unspam(save=False) item.save() - item.referrer.save() + referrers.append(item.referrer) + # Reindexing triggers SHARE/search HTTP calls; run it after the transaction + # commits so those requests never hold row locks. + for referrer in referrers: + referrer.save() def _check_resource_for_domains(resource, content): diff --git a/osf/management/commands/force_archive.py b/osf/management/commands/force_archive.py index 7b6d18b6c65..4a8d04feb28 100644 --- a/osf/management/commands/force_archive.py +++ b/osf/management/commands/force_archive.py @@ -45,7 +45,7 @@ from api.waffle.utils import flag_is_active from scripts import utils as script_utils from website.archiver import ARCHIVER_SUCCESS -from website.settings import ARCHIVE_TIMEOUT_TIMEDELTA, ARCHIVE_PROVIDER, COOKIE_NAME +from website.settings import ARCHIVE_TIMEOUT_TIMEDELTA, ARCHIVE_PROVIDER, COOKIE_NAME, EXTERNAL_REQUEST_TIMEOUT from website.files.utils import attach_versions logger = logging.getLogger(__name__) @@ -174,7 +174,7 @@ def perform_wb_copy(reg, node_settings, delete_collisions=False, skip_collisions 'provider': ARCHIVE_PROVIDER, } url = waterbutler_api_url_for(src._id, node_settings.short_name, _internal=True, base_url=src.osfstorage_region.waterbutler_url, **params) - res = requests.post(url, data=json.dumps(data), cookies={COOKIE_NAME: cookie}) + res = requests.post(url, data=json.dumps(data), cookies={COOKIE_NAME: cookie}, timeout=EXTERNAL_REQUEST_TIMEOUT) if res.status_code not in (http_status.HTTP_200_OK, http_status.HTTP_201_CREATED, http_status.HTTP_202_ACCEPTED): http_exception = HTTPError(res.status_code) sentry.log_exception(http_exception) diff --git a/osf/metrics/reporters/preprint_count.py b/osf/metrics/reporters/preprint_count.py index 6cafa063c62..99dbede560f 100644 --- a/osf/metrics/reporters/preprint_count.py +++ b/osf/metrics/reporters/preprint_count.py @@ -47,7 +47,11 @@ def report(self, date): for preprint_provider in PreprintProvider.objects.all(): elastic_query = get_elastic_query(date, preprint_provider) - resp = requests.post(f'{settings.SHARE_URL}api/v2/search/creativeworks/_search', json=elastic_query).json() + resp = requests.post( + f'{settings.SHARE_URL}api/v2/search/creativeworks/_search', + json=elastic_query, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, + ).json() yield DailyPreprintSummaryReport( cycle_coverage=cycle_coverage_date(date), diff --git a/osf_tests/users/test_last_login_date.py b/osf_tests/users/test_last_login_date.py deleted file mode 100644 index 33eebc17e1e..00000000000 --- a/osf_tests/users/test_last_login_date.py +++ /dev/null @@ -1,82 +0,0 @@ -from unittest import mock -import pytz -import pytest -import itsdangerous -from datetime import datetime, timedelta -from importlib import import_module - -from django.utils import timezone -from django.conf import settings as django_conf_settings - -from website import settings - -from osf_tests.factories import ( - AuthUserFactory, -) -from tests.base import OsfTestCase -from tests.utils import run_celery_tasks - -SessionStore = import_module(django_conf_settings.SESSION_ENGINE).SessionStore - -@pytest.mark.django_db -@pytest.mark.enable_enqueue_task -class TestUserLastLoginDate(OsfTestCase): - - def setUp(self): - super().setUp() - - self.user = AuthUserFactory() - - self.session = SessionStore() - self.session['auth_user_id'] = self.user._id - self.session['auth_user_username'] = self.user.username - self.session.create() - self.cookie = itsdangerous.Signer(settings.SECRET_KEY).sign(self.session.session_key).decode() - - @mock.patch.object(timezone, 'now') - def test_date_last_login_updated_from_none(self, mock_time): - now = datetime(2018, 2, 4, tzinfo=pytz.utc) - mock_time.return_value = now - assert self.user.date_last_login is None - - self.app.set_cookie(settings.COOKIE_NAME, self.cookie) - with run_celery_tasks(): - self.app.get(f'{settings.DOMAIN}{self.user._id}') # user page will fail because not emberized - - self.user.refresh_from_db() - assert self.user.date_last_login == now - - @mock.patch.object(timezone, 'now') - def test_date_last_login_updated_below_threshold(self, mock_time): - now = datetime(2018, 2, 4, tzinfo=pytz.utc) - mock_time.return_value = now - self.user.date_last_login = now - self.user.save() - - # Time is mocked one second below the last login date threshold, so it should not change. - mock_time.return_value = now + (settings.DATE_LAST_LOGIN_THROTTLE_DELTA - timedelta(seconds=1)) - self.app.set_cookie(settings.COOKIE_NAME, self.cookie) - with run_celery_tasks(): - self.app.get(f'{settings.DOMAIN}{self.user._id}') # user page will fail because not emberized - - self.user.refresh_from_db() - # date_last_login is unchanged - assert self.user.date_last_login == now - - @mock.patch.object(timezone, 'now') - def test_date_last_login_updated_above_threshold(self, mock_time): - now = datetime(2018, 2, 4, tzinfo=pytz.utc) - mock_time.return_value = now - self.user.date_last_login = now - self.user.save() - - # Time is mocked one second below the last login date threshold, so it should not change. - new_time = now + (settings.DATE_LAST_LOGIN_THROTTLE_DELTA + timedelta(seconds=1)) - mock_time.return_value = new_time - self.app.set_cookie(settings.COOKIE_NAME, self.cookie) - with run_celery_tasks(): - self.app.get(f'{settings.DOMAIN}{self.user._id}') # user page will fail because not emberized - - self.user.refresh_from_db() - # date_last_login is changed! - assert self.user.date_last_login == new_time diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index ded49d5bcb4..bc531c4d56d 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -264,7 +264,7 @@ def make_copy_request(self, job_pk, url, data): src, dst, user = job.info() logger.info(f"Sending copy request for addon: {data['provider']} on node: {dst._id}") cookie = furl(url).query.params.get('cookie') - res = requests.post(url, data=json.dumps(data), cookies={settings.COOKIE_NAME: cookie}) + res = requests.post(url, data=json.dumps(data), cookies={settings.COOKIE_NAME: cookie}, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) if res.status_code not in (http_status.HTTP_200_OK, http_status.HTTP_201_CREATED, http_status.HTTP_202_ACCEPTED): raise HTTPError(res.status_code) diff --git a/website/identifiers/clients/crossref.py b/website/identifiers/clients/crossref.py index 8f496ce363b..f7e20a618c2 100644 --- a/website/identifiers/clients/crossref.py +++ b/website/identifiers/clients/crossref.py @@ -263,6 +263,7 @@ def create_identifier(self, preprint, category, include_relation=True): fname=f'{preprint._id}.xml' ), files={'file': (f'{preprint._id}.xml', metadata)}, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) if response.status_code == 429: raise CrossRefRateLimitError(response.text) @@ -291,6 +292,7 @@ def bulk_create(self, metadata, filename): fname=f'{filename}.xml' ), files={'file': (f'{filename}.xml', metadata)}, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, ) logger.info('Sent a bulk update of metadata to CrossRef') diff --git a/website/settings/defaults.py b/website/settings/defaults.py index dfc78bb07ab..4b88b16bf1f 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -376,6 +376,13 @@ def parent_dir(path): SHARE_URL = 'https://share.osf.io/' SHARE_API_TOKEN = None # Required to send project updates to SHARE +EXTERNAL_REQUEST_TIMEOUT = (10, 30) # (connect, read) timeout for outbound requests to external services + +SHARE_UPDATE_TASK_SOFT_TIME_LIMIT = 90 +SHARE_UPDATE_TASK_HARD_TIME_LIMIT = 120 +SPAM_SUBMIT_TASK_SOFT_TIME_LIMIT = 60 +SPAM_SUBMIT_TASK_HARD_TIME_LIMIT = 90 + CAS_SERVER_URL = 'http://localhost:8080' MFR_SERVER_URL = 'http://localhost:7778' From bae13a0877f16850d56f10af9683bc8acf0ca5c6 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Mon, 29 Jun 2026 17:21:17 +0300 Subject: [PATCH 2/5] [ENG-10365] Make waterbutled callback after addons archiving is finished (#11787) https://openscience.atlassian.net/browse/ENG-10365 Implement solution from analysis ticket. Here is the issue description and proposed solution: https://openscience.atlassian.net/browse/ENG-10851?focusedCommentId=121306 Call `archive_callback` only after all addons are archived --- api/registrations/views.py | 4 +- osf_tests/test_archiver.py | 95 +++++++++++++++++++++++++---------- osf_tests/utils.py | 5 +- tests/utils.py | 4 +- website/archiver/listeners.py | 30 ----------- website/archiver/tasks.py | 89 ++++++++++++++++++++++++++++---- website/project/signals.py | 2 - 7 files changed, 153 insertions(+), 76 deletions(-) diff --git a/api/registrations/views.py b/api/registrations/views.py index 67705b5eace..72b216d923c 100644 --- a/api/registrations/views.py +++ b/api/registrations/views.py @@ -6,7 +6,7 @@ from addons.base.views import DOWNLOAD_ACTIONS from website.archiver import signals, ARCHIVER_NETWORK_ERROR, ARCHIVER_SUCCESS, ARCHIVER_FAILURE -from website.project import signals as project_signals +from website.archiver.tasks import archive_callback from osf.models import Registration, OSFUser, RegistrationProvider, OutcomeArtifact, CedarMetadataRecord from osf.models.spam import SpamStatus @@ -1114,7 +1114,7 @@ def update(self, request, *args, **kwargs): src_provider, ARCHIVER_SUCCESS, ) - project_signals.archive_callback.send(registration) + archive_callback(registration._id) return Response(status=status.HTTP_200_OK) except HTTPError as e: registration.archive_status = ARCHIVER_NETWORK_ERROR diff --git a/osf_tests/test_archiver.py b/osf_tests/test_archiver.py index 2ee659e00f5..078ad11ac64 100644 --- a/osf_tests/test_archiver.py +++ b/osf_tests/test_archiver.py @@ -542,7 +542,7 @@ def test_archiver_task_load_archive_job_final_failure_logs_context(self, mock_lo ) mock_log_exception.assert_called_once() - @mock.patch('website.archiver.tasks.archive_addon.delay') + @mock.patch('website.archiver.tasks.archive_addon.si') def test_archive_node_pass(self, mock_archive_addon): settings.MAX_ARCHIVE_SIZE = 1024 ** 3 with mock.patch.object(BaseStorageAddon, '_get_file_tree') as mock_file_tree: @@ -561,8 +561,8 @@ def test_archive_node_fail(self): with pytest.raises(ArchiverSizeExceeded): # Note: Requires task_eager_propagates = True in celery archive_node.apply(args=(results, self.archive_job._id)) - @mock.patch('website.project.signals.archive_callback.send') - @mock.patch('website.archiver.tasks.archive_addon.delay') + @mock.patch('website.archiver.tasks.archive_callback.si') + @mock.patch('website.archiver.tasks.archive_addon.si') def test_archive_node_does_not_archive_empty_addons(self, mock_archive_addon, mock_send): with mock.patch('osf.models.mixins.AddonModelMixin.get_addon') as mock_get_addon: mock_addon = MockAddon() @@ -582,7 +582,7 @@ def empty_file_tree(user, version): assert mock_send.called @use_fake_addons - @mock.patch('website.archiver.tasks.archive_addon.delay') + @mock.patch('website.archiver.tasks.archive_addon.si') def test_archive_node_no_archive_size_limit(self, mock_archive_addon): settings.MAX_ARCHIVE_SIZE = 100 self.archive_job.initiator.add_system_tag(NO_ARCHIVE_LIMIT) @@ -596,23 +596,64 @@ def test_archive_node_no_archive_size_limit(self, mock_archive_addon): job_pk=self.archive_job._id, ) - @mock.patch('website.archiver.tasks.make_copy_request.delay') - def test_archive_addon(self, mock_make_copy_request): - archive_addon('osfstorage', self.archive_job._id) + def test_archive_addon(self): + params = archive_addon('osfstorage', self.archive_job._id) assert self.archive_job.get_target('osfstorage').status == ARCHIVER_INITIATED cookie = self.user.get_or_create_cookie() - mock_make_copy_request.assert_called_with( + assert params['addon_short_name'] == 'osfstorage' + assert params['url'] == f'{settings.WATERBUTLER_URL}/v1/resources/{self.src._id}/providers/osfstorage/?cookie={cookie.decode()}' + assert params['data'] == { + 'action': 'copy', + 'path': '/', + 'rename': 'Archive of OSF Storage', + 'resource': self.archive_job.info()[1]._id, + 'provider': 'osfstorage', + } + + @mock.patch('website.archiver.tasks.archive_callback.delay') + def test_archive_addon_does_not_trigger_callback_immediately(self, mock_archive_callback): + archive_addon('osfstorage', self.archive_job._id) + + mock_archive_callback.assert_not_called() + + @mock.patch('website.archiver.tasks.handlers.enqueue_task') + @mock.patch('website.archiver.tasks.archive_callback.si') + @mock.patch('website.archiver.tasks.make_copy_request.s') + @mock.patch('website.archiver.tasks.celery.chain') + @mock.patch('website.archiver.tasks.celery.group') + @mock.patch('website.archiver.tasks.archive_addon.si') + def test_archive_node_only_enqueues_addon_work_before_callback( + self, + mock_archive_addon, + mock_group, + mock_chain, + mock_make_copy_request_s, + mock_archive_callback, + mock_enqueue_task, + ): + settings.MAX_ARCHIVE_SIZE = 1024 ** 3 + with mock.patch.object(BaseStorageAddon, '_get_file_tree') as mock_file_tree: + mock_file_tree.return_value = FILE_TREE + results = [stat_addon(addon, self.archive_job._id) for addon in ['osfstorage']] + + archive_node(results, self.archive_job._id) + + mock_archive_addon.assert_called_once_with( + addon_short_name='osfstorage', job_pk=self.archive_job._id, - url=f'{settings.WATERBUTLER_URL}/v1/resources/{self.src._id}/providers/osfstorage/?cookie={cookie.decode()}', - data={ - 'action': 'copy', - 'path': '/', - 'rename': 'Archive of OSF Storage', - 'resource': self.archive_job.info()[1]._id, - 'provider': 'osfstorage', - } ) + self.assertEqual(mock_chain.call_count, 2) + mock_chain.assert_any_call([ + mock_archive_addon.return_value, + mock_make_copy_request_s.return_value, + ]) + mock_group.assert_called_once_with([mock_chain.return_value]) + mock_chain.assert_any_call([ + mock_group.return_value, + mock_archive_callback.return_value, + ]) + mock_enqueue_task.assert_called_once_with(mock_chain.return_value) @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links') def test_archive_success(self): @@ -1009,7 +1050,7 @@ def test_archive_callback_pending(self, mock_delay): ) self.dst.archive_job.save() with mock.patch('website.archiver.utils.handle_archive_fail') as mock_fail: - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) assert not mock_fail.called assert mock_delay.called @@ -1017,7 +1058,7 @@ def test_archive_callback_pending(self, mock_delay): def test_archive_callback_done_success(self, mock_archive_success): self.dst.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) self.dst.archive_job.save() - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) @mock.patch('website.archiver.tasks.archive_success.delay') def test_archive_callback_done_embargoed(self, mock_archive_success): @@ -1031,13 +1072,13 @@ def test_archive_callback_done_embargoed(self, mock_archive_success): self.dst.embargo_registration(self.user, end_date) self.dst.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) self.dst.save() - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) def test_archive_callback_done_errors(self): self.dst.archive_job.update_target('osfstorage', ARCHIVER_FAILURE) self.dst.archive_job.save() with mock.patch('website.archiver.utils.handle_archive_fail') as mock_fail: - listeners.archive_callback(self.dst) + archive_callback(self.dst._id) call_args = mock_fail.call_args[0] assert call_args[0] == ARCHIVER_UNCAUGHT_ERROR assert call_args[1] == self.src @@ -1053,7 +1094,7 @@ def test_archive_callback_updates_archiving_state_when_done(self): child = reg.nodes[0] child.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) child.save() - listeners.archive_callback(child) + archive_callback(child._id) assert not child.archiving def test_archive_tree_finished_d1(self): @@ -1121,13 +1162,13 @@ def test_archive_callback_on_tree_sends_only_one_email(self, mock_archive_succes node.archive_job.update_target('osfstorage', ARCHIVER_INITIATED) rchild.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) rchild.save() - listeners.archive_callback(rchild) + archive_callback(rchild._id) reg.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) reg.save() - listeners.archive_callback(reg) + archive_callback(reg._id) rchild2.archive_job.update_target('osfstorage', ARCHIVER_SUCCESS) rchild2.save() - listeners.archive_callback(rchild2) + archive_callback(rchild2._id) class TestArchiverScripts(ArchiverTestCase): @@ -1194,7 +1235,7 @@ def test_archiving_nodes_added_to_search_on_archive_success_if_public(self, mock mock.patch('osf.models.ArchiveJob.archive_tree_finished', mock.Mock(return_value=True)), mock.patch('osf.models.ArchiveJob.success', mock.PropertyMock(return_value=True)) ) as (mock_finished, mock_success): - listeners.archive_callback(reg) + archive_callback(reg._id) assert mock_update_search.call_count == 1 @pytest.mark.enable_search @@ -1208,7 +1249,7 @@ def test_archiving_nodes_not_added_to_search_on_archive_failure(self, mock_delet mock.patch('osf.models.archive.ArchiveJob.archive_tree_finished', mock.Mock(return_value=True)), mock.patch('osf.models.archive.ArchiveJob.success', mock.PropertyMock(return_value=False)) ) as (mock_finished, mock_success): - listeners.archive_callback(reg) + archive_callback(reg._id) @mock.patch('osf.models.AbstractNode.update_search') def test_archiving_nodes_not_added_to_search_on_archive_incomplete(self, mock_update_search): @@ -1216,7 +1257,7 @@ def test_archiving_nodes_not_added_to_search_on_archive_incomplete(self, mock_up reg = factories.RegistrationFactory(project=proj) reg.save() with mock.patch('osf.models.ArchiveJob.archive_tree_finished', mock.Mock(return_value=False)): - listeners.archive_callback(reg) + archive_callback(reg._id) assert not mock_update_search.called diff --git a/osf_tests/utils.py b/osf_tests/utils.py index adb00482168..0d93d1421c7 100644 --- a/osf_tests/utils.py +++ b/osf_tests/utils.py @@ -10,7 +10,8 @@ import blinker from website.signals import ALL_SIGNALS from website.archiver import ARCHIVER_SUCCESS -from website.archiver import listeners as archiver_listeners +from website.archiver.tasks import archive_callback + from osf.models import ( Sanction, @@ -154,7 +155,7 @@ def mock_archive(project, schema=None, auth=None, data=None, parent=None, sanction = registration.sanction mock.patch.object(root_job, 'archive_tree_finished', mock.Mock(return_value=True)), mock.patch('website.archiver.tasks.archive_success.delay', mock.Mock()) - archiver_listeners.archive_callback(registration) + archive_callback(registration._id) if autoapprove: sanction = registration.sanction diff --git a/tests/utils.py b/tests/utils.py index 9f0aba2f4cf..cacd6d3fcd3 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -20,7 +20,7 @@ from osf.models import Sanction, NotificationType, Notification from tests.base import get_default_metaschema from website.archiver import ARCHIVER_SUCCESS -from website.archiver import listeners as archiver_listeners +from website.archiver.tasks import archive_callback from website import settings as website_settings from osf import features @@ -155,7 +155,7 @@ def mock_archive(project, schema=None, auth=None, draft_registration=None, paren # Ensure patches actually apply: with mock.patch.object(root_job, 'archive_tree_finished', mock.Mock(return_value=True)), \ mock.patch('website.archiver.tasks.archive_success.delay', mock.Mock()): - archiver_listeners.archive_callback(registration) + archive_callback(registration._id) if autoapprove: sanction = registration.root.sanction diff --git a/website/archiver/listeners.py b/website/archiver/listeners.py index d0f8f3b3c03..a9e562291da 100644 --- a/website/archiver/listeners.py +++ b/website/archiver/listeners.py @@ -3,9 +3,6 @@ from framework.celery_tasks import handlers from website.archiver import utils as archiver_utils -from website.archiver import ( - ARCHIVER_UNCAUGHT_ERROR, -) from website.archiver import signals as archiver_signals from website.project import signals as project_signals @@ -30,33 +27,6 @@ def after_register(src, dst, user): ) -@project_signals.archive_callback.connect -def archive_callback(dst): - """Blinker listener for updates to the archive task. When the tree of ArchiveJob - instances is complete, proceed to send success or failure mails - - :param dst: registration Node - """ - root = dst.root - root_job = root.archive_job - if not root_job.archive_tree_finished(): - return - if root_job.sent: - return - if root_job.success: - # Prevent circular import with app.py - from website.archiver import tasks - tasks.archive_success.delay(dst_pk=root._id, job_pk=root_job._id) - else: - archiver_utils.handle_archive_fail( - ARCHIVER_UNCAUGHT_ERROR, - root.registered_from, - root, - root.registered_user, - dst.archive_job.target_info(), - ) - - @archiver_signals.archive_fail.connect def archive_fail(dst, errors): reason = dst.archive_status diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index bc531c4d56d..84c9d91ea44 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -7,7 +7,7 @@ import celery from celery.utils.log import get_task_logger -from framework.celery_tasks import app as celery_app +from framework.celery_tasks import app as celery_app, handlers from framework.celery_tasks.utils import logged from framework.exceptions import HTTPError from framework import sentry @@ -27,11 +27,11 @@ ) from website.archiver import utils from website.archiver.utils import normalize_unicode_filenames +from website.archiver import utils as archiver_utils from website.archiver import signals as archiver_signals from scripts.check_manual_restart_approval import delayed_manual_restart_approval -from website.project import signals as project_signals from website import settings from website.app import init_addons @@ -250,22 +250,35 @@ def stat_addon(self, addon_short_name, job_pk): default_retry_delay=60, ) @logged('make_copy_request') -def make_copy_request(self, job_pk, url, data): +def make_copy_request(self, params, job_pk): """Make the copy request to the WaterButler API and handle successful and failed responses + :param params: dict returned by archive_addon containing addon_short_name, url, and data :param job_pk: primary key of ArchiveJob - :param url: URL to send request to - :param data: of setting to send in POST to WaterButler API :return: None """ create_app_context() job = self.load_archive_job(job_pk) src, dst, user = job.info() + addon_short_name = params['addon_short_name'] + url = params['url'] + data = params['data'] logger.info(f"Sending copy request for addon: {data['provider']} on node: {dst._id}") cookie = furl(url).query.params.get('cookie') - res = requests.post(url, data=json.dumps(data), cookies={settings.COOKIE_NAME: cookie}, timeout=settings.EXTERNAL_REQUEST_TIMEOUT) + try: + res = requests.post( + url, + data=json.dumps(data), + cookies={settings.COOKIE_NAME: cookie}, + timeout=settings.EXTERNAL_REQUEST_TIMEOUT, + ) + except requests.RequestException as exc: + job.update_target(addon_short_name, ARCHIVER_FAILURE, errors=[str(exc)]) + raise + if res.status_code not in (http_status.HTTP_200_OK, http_status.HTTP_201_CREATED, http_status.HTTP_202_ACCEPTED): + job.update_target(addon_short_name, ARCHIVER_FAILURE, errors=[res.text or f'WaterButler request failed with status {res.status_code}']) raise HTTPError(res.status_code) def make_waterbutler_payload(dst_id, rename): @@ -320,7 +333,11 @@ def archive_addon(self, addon_short_name, job_pk): rename = f'{folder_name_nfd}{rename_suffix}' url = waterbutler_api_url_for(src._id, addon_short_name, _internal=True, base_url=src.osfstorage_region.waterbutler_url, **params) data = make_waterbutler_payload(dst._id, rename) - make_copy_request.delay(job_pk=job_pk, url=url, data=data) + return { + 'addon_short_name': addon_short_name, + 'url': url, + 'data': data, + } @celery_app.task( bind=True, @@ -357,15 +374,38 @@ def archive_node(self, stat_results, job_pk): if not stat_result.targets: job.status = ARCHIVER_SUCCESS job.save() + + addon_tasks = [] for result in stat_result.targets: if not result['num_files']: job.update_target(result['target_name'], ARCHIVER_SUCCESS) else: - archive_addon.delay( - addon_short_name=result['target_name'], - job_pk=job_pk + addon_tasks.append( + celery.chain( + [ + archive_addon.si( + addon_short_name=result['target_name'], + job_pk=job_pk, + ), + make_copy_request.s( + job_pk=job_pk, + ), + ] + ) ) - project_signals.archive_callback.send(dst) + + if not addon_tasks: + handlers.enqueue_task(archive_callback.si(dst_id=dst._id)) + return + + handlers.enqueue_task( + celery.chain( + [ + celery.group(addon_tasks), + archive_callback.si(dst_id=dst._id), + ] + ) + ) def archive(job_pk): @@ -474,3 +514,30 @@ def force_archive(self, registration_id, permissible_addons, allow_unconfigured= sentry.log_message(f'Archive task failed for {registration_id}: {exc}') sentry.log_exception(exc) return f'{exc.__class__.__name__}: {str(exc)}' + + +@celery_app.task +@logged('archive_callback') +def archive_callback(dst_id): + """Blinker task for updates to the archive task. When the tree of ArchiveJob + instances is complete, proceed to send success or failure mails + + :param dst: registration Node + """ + dst = Registration.load(dst_id) + root = dst.root + root_job = root.archive_job + if not root_job.archive_tree_finished(): + return + if root_job.sent: + return + if root_job.success: + archive_success.delay(dst_pk=root._id, job_pk=root_job._id) + else: + archiver_utils.handle_archive_fail( + ARCHIVER_UNCAUGHT_ERROR, + root.registered_from, + root, + root.registered_user, + dst.archive_job.target_info(), + ) diff --git a/website/project/signals.py b/website/project/signals.py index a9b1e0e735a..5306f7e4e7f 100644 --- a/website/project/signals.py +++ b/website/project/signals.py @@ -9,5 +9,3 @@ node_deleted = signals.signal('node-deleted') after_create_registration = signals.signal('post-create-registration') - -archive_callback = signals.signal('archive-callback') From b9dc87d790ab6be5b5880fdfddcd5056a5b8d187 Mon Sep 17 00:00:00 2001 From: sh-andriy <105591819+sh-andriy@users.noreply.github.com> Date: Tue, 30 Jun 2026 17:42:21 +0300 Subject: [PATCH 3/5] fix(osf): ENG-11362 add admin tool to recover preprints deleted by create-version bug (#11788) add admin tool to recover preprints deleted by create-version bug --- admin/preprints/forms.py | 19 ++++++- admin/preprints/urls.py | 1 + admin/preprints/views.py | 51 +++++++++++++++++- .../templates/preprints/recover_preprint.html | 34 ++++++++++++ admin/templates/preprints/search.html | 3 ++ admin_tests/preprints/test_views.py | 54 ++++++++++++++++++- osf/models/admin_log_entry.py | 1 + 7 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 admin/templates/preprints/recover_preprint.html diff --git a/admin/preprints/forms.py b/admin/preprints/forms.py index 2ea91931018..dec5dba324d 100644 --- a/admin/preprints/forms.py +++ b/admin/preprints/forms.py @@ -1,8 +1,25 @@ from django import forms -from osf.models import Preprint +from osf.models import Preprint, PreprintProvider from osf.utils.workflows import ReviewStates + +class RecoverDeletedPreprintForm(forms.Form): + """Inputs needed to recreate a preprint that was hard-deleted by the + Create-New-Version bug (ENG-11012). The GUID and DOI are recreated exactly + as they were so existing DOIs and links keep resolving. + """ + provider = forms.ModelChoiceField(queryset=PreprintProvider.objects.all()) + guid = forms.CharField(max_length=5, min_length=5, label='Base GUID') + doi = forms.CharField(max_length=255, required=False) + title = forms.CharField(max_length=512) + description = forms.CharField(widget=forms.Textarea, required=False) + ticket_reference = forms.CharField( + max_length=255, + label='Support ticket / JIRA reference', + help_text='Recorded in the admin log so the recreated preprint is traceable.', + ) + class ChangeProviderForm(forms.ModelForm): class Meta: model = Preprint diff --git a/admin/preprints/urls.py b/admin/preprints/urls.py index 960e2e297e5..f5e48f676d9 100644 --- a/admin/preprints/urls.py +++ b/admin/preprints/urls.py @@ -9,6 +9,7 @@ re_path(r'^known_spam$', views.PreprintKnownSpamList.as_view(), name='known-spam'), re_path(r'^known_ham$', views.PreprintKnownHamList.as_view(), name='known-ham'), re_path(r'^withdrawal_requests$', views.PreprintWithdrawalRequestList.as_view(), name='withdrawal-requests'), + re_path(r'^recover/$', views.RecoverDeletedPreprintView.as_view(), name='recover'), re_path(r'^(?P\w+)/$', views.PreprintView.as_view(), name='preprint'), re_path(r'^(?P\w+)/change_provider/$', views.PreprintProviderChangeView.as_view(), name='preprint-provider'), re_path(r'^(?P\w+)/machine_state/$', views.PreprintMachineStateView.as_view(), name='preprint-machine-state'), diff --git a/admin/preprints/views.py b/admin/preprints/views.py index 3153ae301c8..8fb71e1a99e 100644 --- a/admin/preprints/views.py +++ b/admin/preprints/views.py @@ -1,6 +1,6 @@ from django.db import transaction from django.db.models import F -from django.core.exceptions import PermissionDenied +from django.core.exceptions import PermissionDenied, ValidationError from django.http import HttpResponse, JsonResponse from django.contrib import messages from django.contrib.auth.mixins import PermissionRequiredMixin @@ -16,7 +16,7 @@ from admin.base.views import GuidView from admin.base.forms import GuidForm from admin.nodes.views import NodeRemoveContributorView, NodeAddSystemTag, NodeRemoveSystemTag, NodeUpdatePermissionsView -from admin.preprints.forms import ChangeProviderForm, MachineStateForm +from admin.preprints.forms import ChangeProviderForm, MachineStateForm, RecoverDeletedPreprintForm from admin.base.utils import osf_staff_check from api.share.utils import update_share @@ -42,6 +42,7 @@ REINDEX_SHARE, PREPRINT_REMOVED, PREPRINT_RESTORED, + PREPRINT_RECOVERED, CONFIRM_SPAM, CONFIRM_HAM, APPROVE_WITHDRAWAL, @@ -249,6 +250,52 @@ def post(self, request, *args, **kwargs): return JsonResponse({'redirect': self.get_success_url(new_preprint._id)}) +class RecoverDeletedPreprintView(PermissionRequiredMixin, FormView): + """Recreate a preprint that was hard-deleted by the Create-New-Version bug + (ENG-11012), restoring its original GUID and DOI so existing links and the + registered DOI keep resolving. Runs in an atomic transaction and records the + acting admin, timestamp and ticket reference in the admin log. + + Only the base preprint record/GUID/DOI is recreated here; the admin then + attaches the file, sets contributors/metadata, publishes and resyncs CrossRef + using the existing preprint admin actions. + """ + template_name = 'preprints/recover_preprint.html' + permission_required = ('osf.change_preprint',) + raise_exception = True + form_class = RecoverDeletedPreprintForm + + def form_valid(self, form): + data = form.cleaned_data + try: + with transaction.atomic(): + preprint = Preprint.create( + provider=data['provider'], + title=data['title'], + creator=self.request.user, + description=data['description'], + manual_guid=data['guid'], + manual_doi=data['doi'] or None, + ) + update_admin_log( + user_id=self.request.user.id, + object_id=preprint._id, + object_repr='Preprint', + message=f'Preprint {preprint._id} recreated after deletion (ref: {data["ticket_reference"]}).', + action_flag=PREPRINT_RECOVERED, + ) + except ValidationError as exc: + messages.error(self.request, f'Could not recover preprint: {"; ".join(exc.messages)}') + return redirect(reverse_lazy('preprints:recover')) + + messages.success( + self.request, + f'Recreated preprint {preprint._id}. Attach the file, set contributors/metadata, ' + 'then publish and resync CrossRef.', + ) + return redirect(reverse_lazy('preprints:preprint', kwargs={'guid': preprint._id})) + + class PreprintReindexElastic(PreprintMixin, View): """ Allows an authorized user to reindex a node in ElasticSearch. """ diff --git a/admin/templates/preprints/recover_preprint.html b/admin/templates/preprints/recover_preprint.html new file mode 100644 index 00000000000..9c188a16e0d --- /dev/null +++ b/admin/templates/preprints/recover_preprint.html @@ -0,0 +1,34 @@ + +{% extends 'base.html' %} +{% load static %} +{% block title %} + Recover Deleted Preprint +{% endblock title %} +{% block content %} +
+

Recover Deleted Preprint

+

+ Recreates a preprint that was hard-deleted by the Create-New-Version bug, + restoring its original GUID and DOI. After recovering, attach the file, + set contributors/metadata, then publish and resync CrossRef. +

+
+
+ {% csrf_token %} + {% if form.errors %} +
{{ form.errors }}
+ {% endif %} + {% for field in form %} +
+ + {{ field }} + {% if field.help_text %} +

{{ field.help_text }}

+ {% endif %} +
+ {% endfor %} + +
+
+
+{% endblock content %} diff --git a/admin/templates/preprints/search.html b/admin/templates/preprints/search.html index 5be0a566600..3a5c5631adb 100644 --- a/admin/templates/preprints/search.html +++ b/admin/templates/preprints/search.html @@ -19,5 +19,8 @@ +
+ Recover deleted preprint +
{% endblock content %} diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index 02519290733..623252189e2 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -19,7 +19,7 @@ SubjectFactory, ) -from osf.models.admin_log_entry import AdminLogEntry +from osf.models.admin_log_entry import AdminLogEntry, PREPRINT_RECOVERED from osf.models.spam import SpamStatus from osf.utils.workflows import DefaultStates, RequestTypes from osf.utils.permissions import ADMIN @@ -1000,3 +1000,55 @@ def test_admin_user_can_fix_coi_only_when_coi_is_set(self, user, preprint, plain assert preprint.has_coi assert preprint.conflict_of_interest_statement == '' + + +@pytest.mark.urls('admin.base.urls') +class TestRecoverDeletedPreprintView(AdminTestCase): + def setUp(self): + super().setUp() + self.user = AuthUserFactory() + self.user.user_permissions.add(Permission.objects.get(codename='change_preprint')) + self.provider = PreprintProviderFactory() + + def _post(self, data): + request = RequestFactory().post(reverse('preprints:recover'), data=data) + request.user = self.user + patch_messages(request) + return views.RecoverDeletedPreprintView.as_view()(request) + + def test_recreates_preprint_at_guid_with_doi_and_audit(self): + data = { + 'provider': self.provider.id, + 'guid': 'abcde', + 'doi': '10.31219/osf.io/abcde_v1', + 'title': 'Recovered Title', + 'description': 'desc', + 'ticket_reference': 'ENG-1234', + } + response = self._post(data) + assert response.status_code == 302 + + preprint = Preprint.load('abcde') + assert preprint is not None + assert preprint.title == 'Recovered Title' + assert preprint.get_identifier_value('doi') == '10.31219/osf.io/abcde_v1' + + log = AdminLogEntry.objects.get(action_flag=PREPRINT_RECOVERED) + assert log.user_id == self.user.id + assert 'ENG-1234' in log.change_message + + def test_existing_guid_is_refused(self): + from osf.models import Guid + Guid.objects.create(_id='abcde') + data = { + 'provider': self.provider.id, + 'guid': 'abcde', + 'doi': '', + 'title': 'Should Not Apply', + 'description': '', + 'ticket_reference': 'ENG-1234', + } + response = self._post(data) + assert response.status_code == 302 + assert Preprint.load('abcde') is None + assert not AdminLogEntry.objects.filter(action_flag=PREPRINT_RECOVERED).exists() diff --git a/osf/models/admin_log_entry.py b/osf/models/admin_log_entry.py index 1597f514375..f6cd288cc43 100644 --- a/osf/models/admin_log_entry.py +++ b/osf/models/admin_log_entry.py @@ -33,6 +33,7 @@ PREPRINT_REMOVED = 70 PREPRINT_RESTORED = 71 +PREPRINT_RECOVERED = 72 DOI_CREATION_FAILED = 80 DOI_UPDATE_FAILED = 81 From cb249e80108a984a53d2d4992fa98278a7965df0 Mon Sep 17 00:00:00 2001 From: sh-andriy <105591819+sh-andriy@users.noreply.github.com> Date: Tue, 30 Jun 2026 19:56:54 +0300 Subject: [PATCH 4/5] ENG-11362 recover preprints: recreate all versions (follow-up) (#11789) --- admin/preprints/forms.py | 7 ++- admin/preprints/views.py | 75 ++++++++++++++++++++--------- admin_tests/preprints/test_views.py | 46 +++++++++++------- 3 files changed, 86 insertions(+), 42 deletions(-) diff --git a/admin/preprints/forms.py b/admin/preprints/forms.py index dec5dba324d..6c267bd4301 100644 --- a/admin/preprints/forms.py +++ b/admin/preprints/forms.py @@ -11,9 +11,14 @@ class RecoverDeletedPreprintForm(forms.Form): """ provider = forms.ModelChoiceField(queryset=PreprintProvider.objects.all()) guid = forms.CharField(max_length=5, min_length=5, label='Base GUID') - doi = forms.CharField(max_length=255, required=False) title = forms.CharField(max_length=512) description = forms.CharField(widget=forms.Textarea, required=False) + file_guid = forms.CharField( + required=False, + label='Source file GUID', + help_text='Optional: an existing file GUID; its latest version is copied into this ' + 'version as the primary file.', + ) ticket_reference = forms.CharField( max_length=255, label='Support ticket / JIRA reference', diff --git a/admin/preprints/views.py b/admin/preprints/views.py index 8fb71e1a99e..8050f24aa3c 100644 --- a/admin/preprints/views.py +++ b/admin/preprints/views.py @@ -33,7 +33,9 @@ Preprint, PreprintLog, PreprintRequest, - PreprintProvider + PreprintProvider, + Guid, + BaseFileNode, ) from osf.models.admin_log_entry import ( @@ -252,48 +254,75 @@ def post(self, request, *args, **kwargs): class RecoverDeletedPreprintView(PermissionRequiredMixin, FormView): """Recreate a preprint that was hard-deleted by the Create-New-Version bug - (ENG-11012), restoring its original GUID and DOI so existing links and the - registered DOI keep resolving. Runs in an atomic transaction and records the - acting admin, timestamp and ticket reference in the admin log. - - Only the base preprint record/GUID/DOI is recreated here; the admin then - attaches the file, sets contributors/metadata, publishes and resyncs CrossRef - using the existing preprint admin actions. + (ENG-11012), restoring its original GUID. All versions are recreated in order + within a single atomic transaction, and the acting admin, timestamp and ticket + reference are recorded in the admin log. + + Recovers one version at a time: the first run on a GUID recreates version 1, + each subsequent run on the same GUID adds the next version. This lets Product + give each version its own file and metadata. Optionally copies the latest + version of an existing file (referenced by GUID) into the recreated version as + its primary file, so admins copy from a project of uploaded files rather than + uploading here. The DOIs are deterministic from the GUID, so resyncing CrossRef + restores each version's original DOI. """ template_name = 'preprints/recover_preprint.html' permission_required = ('osf.change_preprint',) raise_exception = True form_class = RecoverDeletedPreprintForm + def _copy_primary_file(self, preprint, file_guid): + source_file = getattr(Guid.load(file_guid), 'referent', None) + if not isinstance(source_file, BaseFileNode): + raise ValueError(f'No file found for guid "{file_guid}".') + latest_version = source_file.versions.order_by('-created').first() + if latest_version is None: + raise ValueError(f'File "{file_guid}" has no versions to copy.') + copied = copy_files(source_file, target_node=preprint, identifier=latest_version.identifier) + preprint.set_primary_file(copied, auth=self.request, save=True) + def form_valid(self, form): data = form.cleaned_data + guid = data['guid'] + file_guid = data['file_guid'].strip() try: with transaction.atomic(): - preprint = Preprint.create( - provider=data['provider'], - title=data['title'], - creator=self.request.user, - description=data['description'], - manual_guid=data['guid'], - manual_doi=data['doi'] or None, - ) + if Preprint.load(guid): + # GUID already recovered: add the next version. + version, _ = Preprint.create_version( + create_from_guid=guid, + auth=self.request, + ignore_permission=True, + ignore_existing_versions=True, + ) + else: + version = Preprint.create( + provider=data['provider'], + title=data['title'], + creator=self.request.user, + description=data['description'], + manual_guid=guid, + ) + if file_guid: + self._copy_primary_file(version, file_guid) update_admin_log( user_id=self.request.user.id, - object_id=preprint._id, + object_id=version._id, object_repr='Preprint', - message=f'Preprint {preprint._id} recreated after deletion (ref: {data["ticket_reference"]}).', + message=f'Preprint {version._id} recreated after deletion (ref: {data["ticket_reference"]}).', action_flag=PREPRINT_RECOVERED, ) - except ValidationError as exc: - messages.error(self.request, f'Could not recover preprint: {"; ".join(exc.messages)}') + except (ValidationError, ValueError) as exc: + reason = '; '.join(exc.messages) if isinstance(exc, ValidationError) else str(exc) + messages.error(self.request, f'Could not recover preprint: {reason}') return redirect(reverse_lazy('preprints:recover')) messages.success( self.request, - f'Recreated preprint {preprint._id}. Attach the file, set contributors/metadata, ' - 'then publish and resync CrossRef.', + f'Recreated preprint version {version._id}. Set contributors/metadata, then publish ' + 'and resync CrossRef. Run again on the same GUID to add the next version.', ) - return redirect(reverse_lazy('preprints:preprint', kwargs={'guid': preprint._id})) + return redirect(reverse_lazy('preprints:preprint', kwargs={'guid': version._id})) class PreprintReindexElastic(PreprintMixin, View): diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index 623252189e2..d70eff1ef35 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -1008,7 +1008,7 @@ def setUp(self): super().setUp() self.user = AuthUserFactory() self.user.user_permissions.add(Permission.objects.get(codename='change_preprint')) - self.provider = PreprintProviderFactory() + self.provider = PreprintProviderFactory(doi_prefix='10.31219') def _post(self, data): request = RequestFactory().post(reverse('preprints:recover'), data=data) @@ -1016,39 +1016,49 @@ def _post(self, data): patch_messages(request) return views.RecoverDeletedPreprintView.as_view()(request) - def test_recreates_preprint_at_guid_with_doi_and_audit(self): + def _base_data(self, **overrides): data = { 'provider': self.provider.id, 'guid': 'abcde', - 'doi': '10.31219/osf.io/abcde_v1', 'title': 'Recovered Title', 'description': 'desc', + 'file_guid': '', 'ticket_reference': 'ENG-1234', } - response = self._post(data) + data.update(overrides) + return data + + def test_first_run_recreates_version_1_with_audit(self): + response = self._post(self._base_data()) assert response.status_code == 302 preprint = Preprint.load('abcde') assert preprint is not None + assert preprint._id == 'abcde_v1' assert preprint.title == 'Recovered Title' - assert preprint.get_identifier_value('doi') == '10.31219/osf.io/abcde_v1' log = AdminLogEntry.objects.get(action_flag=PREPRINT_RECOVERED) assert log.user_id == self.user.id assert 'ENG-1234' in log.change_message - def test_existing_guid_is_refused(self): + def test_second_run_adds_next_version(self): from osf.models import Guid - Guid.objects.create(_id='abcde') - data = { - 'provider': self.provider.id, - 'guid': 'abcde', - 'doi': '', - 'title': 'Should Not Apply', - 'description': '', - 'ticket_reference': 'ENG-1234', - } - response = self._post(data) + assert self._post(self._base_data()).status_code == 302 + assert self._post(self._base_data()).status_code == 302 + + versions = Guid.load('abcde').versions.order_by('version') + assert list(versions.values_list('version', flat=True)) == [1, 2] + for version_through in versions: + assert version_through.referent._id == f'abcde_v{version_through.version}' + + def test_copies_primary_file_from_source_guid(self): + source = PreprintFactory(provider=self.provider) + source_file = source.primary_file + file_guid = source_file.get_guid(create=True)._id + + response = self._post(self._base_data(file_guid=file_guid)) assert response.status_code == 302 - assert Preprint.load('abcde') is None - assert not AdminLogEntry.objects.filter(action_flag=PREPRINT_RECOVERED).exists() + + recovered = Preprint.load('abcde') + assert recovered.primary_file is not None + assert recovered.primary_file.copied_from_id == source_file.id From 3308e4ecd5c86cf2aaacc4ade90bfcee7f8a3f49 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Mon, 29 Jun 2026 17:22:55 +0300 Subject: [PATCH 5/5] [ENG-11457] require at least 1 subject for draft and usual registrations (#11786) ## Ticket https://openscience.atlassian.net/browse/ENG-11457 ## Purpose Draft registrations and usual registration should have at least 1 subject ## Changes Added validation --- admin_tests/nodes/test_views.py | 7 +++++++ .../draft_nodes/views/test_draft_node_detail.py | 4 +++- ...est_draft_registration_relationship_subjects.py | 13 +++++++++++++ .../registrations/views/test_registration_list.py | 8 +++++++- .../test_registration_relationship_subjects.py | 13 +++++++++++++ osf/models/mixins.py | 9 +++++++++ osf/models/registrations.py | 3 +++ osf_tests/test_draft_registration.py | 14 ++++++++++++++ tests/test_registrations/base.py | 3 ++- tests/test_registrations/test_embargoes.py | 2 +- tests/utils.py | 4 +++- 11 files changed, 75 insertions(+), 5 deletions(-) diff --git a/admin_tests/nodes/test_views.py b/admin_tests/nodes/test_views.py index d7cdcf8bece..b63618e8426 100644 --- a/admin_tests/nodes/test_views.py +++ b/admin_tests/nodes/test_views.py @@ -57,6 +57,7 @@ from tests.base import AdminTestCase from osf_tests.factories import ( + SubjectFactory, UserFactory, AuthUserFactory, ProjectFactory, @@ -657,6 +658,7 @@ def setUp(self): provider=RegistrationProviderFactory(reviews_workflow='pre-moderation'), creator=self.user ) + pre_moderation_draft.subjects.add(SubjectFactory()) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr3) @@ -670,6 +672,7 @@ def setUp(self): provider=RegistrationProviderFactory(reviews_workflow='post-moderation'), creator=self.user ) + post_moderation_draft.subjects.add(SubjectFactory()) self._add_contributor(post_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(post_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(post_moderation_draft, permissions.ADMIN, self.contr3) @@ -682,6 +685,7 @@ def setUp(self): registration_schema=get_default_metaschema(), creator=self.user ) + self.no_moderation_draft.subjects.add(SubjectFactory()) self._add_contributor(self.no_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(self.no_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(self.no_moderation_draft, permissions.ADMIN, self.contr3) @@ -881,6 +885,7 @@ def test_revert_node_based_registration(self): self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr1) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr2) self._add_contributor(pre_moderation_draft, permissions.ADMIN, self.contr3) + pre_moderation_draft.subjects.add(SubjectFactory()) pre_moderation_draft.register(auth=self.auth, save=True) pre_moderation_registration = pre_moderation_draft.registered_node @@ -901,6 +906,7 @@ def test_can_revert_embargo_registration_to_draft(self): registration_schema=get_default_metaschema(), creator=self.user ) + self.no_moderation_draft.subjects.add(SubjectFactory()) self.no_moderation_draft.register(auth=self.auth, save=True) self.registration = self.no_moderation_draft.registered_node @@ -929,6 +935,7 @@ def test_embargo_is_reset_after_reversion(self): registration_schema=get_default_metaschema(), creator=self.user ) + self.no_moderation_draft.subjects.add(SubjectFactory()) self.no_moderation_draft.register(auth=self.auth, save=True) self.registration = self.no_moderation_draft.registered_node diff --git a/api_tests/draft_nodes/views/test_draft_node_detail.py b/api_tests/draft_nodes/views/test_draft_node_detail.py index 777d77a948c..8b3d21accb6 100644 --- a/api_tests/draft_nodes/views/test_draft_node_detail.py +++ b/api_tests/draft_nodes/views/test_draft_node_detail.py @@ -5,7 +5,8 @@ from osf_tests.factories import ( DraftRegistrationFactory, AuthUserFactory, - ProjectFactory + ProjectFactory, + SubjectFactory ) @@ -23,6 +24,7 @@ def user_two(self): def test_detail_response(self, app, user, user_two): draft_reg = DraftRegistrationFactory(initiator=user) draft_reg.add_contributor(user_two) + draft_reg.subjects.add(SubjectFactory()) draft_reg.save() draft_node = draft_reg.branched_from diff --git a/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py b/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py index 4e982ad0275..1279df4da5c 100644 --- a/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py +++ b/api_tests/draft_registrations/views/test_draft_registration_relationship_subjects.py @@ -22,6 +22,19 @@ def resource(self, user_admin_contrib, user_write_contrib, user_read_contrib): def url(self, resource): return f'/{API_BASE}draft_registrations/{resource._id}/relationships/subjects/' + def test_update_subjects_empty_payload(self, app, user_admin_contrib, resource, url, subject): + # override test as draft registration must have at least one subject to be registered + resource.subjects.add(subject) + + payload = { + 'data': [] + } + + res = app.patch_json_api(url, payload, auth=user_admin_contrib.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Registration must have at least one subject to be registered' + assert resource.subjects.count() == 1 + # Overwrites SubjectsRelationshipMixin def test_update_subjects_relationship_permissions(self, app, user_write_contrib, user_read_contrib, user_non_contrib, resource, url, payload): diff --git a/api_tests/registrations/views/test_registration_list.py b/api_tests/registrations/views/test_registration_list.py index 26418574c6c..92ac11eea9e 100644 --- a/api_tests/registrations/views/test_registration_list.py +++ b/api_tests/registrations/views/test_registration_list.py @@ -637,7 +637,7 @@ def project_public_excluded_sibling(self, project_public): @pytest.fixture() def draft_registration(self, user, project_public, schema): - return DraftRegistrationFactory( + draft_registration = DraftRegistrationFactory( initiator=user, registration_schema=schema, branched_from=project_public, @@ -646,6 +646,8 @@ def draft_registration(self, user, project_public, schema): 'item33': {'value': 'success'} } ) + draft_registration.subjects.add(SubjectFactory()) + return draft_registration @pytest.fixture() def url_registrations(self, project_public): @@ -1396,6 +1398,7 @@ def test_assert_file_validity_on_registration_metadata( registration_schema=prereg_schema, branched_from=project_public ) + prereg_draft_registration.subjects.add(SubjectFactory()) prereg_registration_responses['q11.uploader'] = [] @@ -1598,6 +1601,7 @@ def test_need_admin_perms_on_draft( # User is an admin contributor on draft registration but not on node draft_registration = DraftRegistrationFactory(creator=user_two, registration_schema=schema) + draft_registration.subjects.add(SubjectFactory()) draft_registration.add_contributor(user, permissions.ADMIN) draft_registration.branched_from.add_contributor(user, permissions.WRITE) payload_ver['data']['attributes']['draft_registration_id'] = draft_registration._id @@ -1611,6 +1615,7 @@ def test_need_admin_perms_on_draft( # User is admin on draft and node draft_registration = DraftRegistrationFactory(creator=user, registration_schema=schema) + draft_registration.subjects.add(SubjectFactory()) assert draft_registration.branched_from.is_admin_contributor(user) is True assert draft_registration.has_permission(user, permissions.ADMIN) is True payload_ver['data']['attributes']['draft_registration_id'] = draft_registration._id @@ -1622,6 +1627,7 @@ def test_need_admin_perms_on_draft( # User is an admin contributor on node but not on draft registration draft_registration = DraftRegistrationFactory(creator=user_two, registration_schema=schema) + draft_registration.subjects.add(SubjectFactory()) draft_registration.add_contributor(user, permissions.WRITE) draft_registration.branched_from.add_contributor(user, permissions.ADMIN) payload_ver['data']['attributes']['draft_registration_id'] = draft_registration._id diff --git a/api_tests/registrations/views/test_registration_relationship_subjects.py b/api_tests/registrations/views/test_registration_relationship_subjects.py index 442ed39cc1a..d356d436352 100644 --- a/api_tests/registrations/views/test_registration_relationship_subjects.py +++ b/api_tests/registrations/views/test_registration_relationship_subjects.py @@ -21,3 +21,16 @@ def resource(self, user_admin_contrib, user_write_contrib, user_read_contrib): @pytest.fixture() def url(self, resource): return f'/{API_BASE}registrations/{resource._id}/relationships/subjects/' + + def test_update_subjects_empty_payload(self, app, user_admin_contrib, resource, url, subject): + # override test as registration must have at least one subject to be registered + resource.subjects.add(subject) + + payload = { + 'data': [] + } + + res = app.patch_json_api(url, payload, auth=user_admin_contrib.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'Registration must have at least one subject to be registered' + assert resource.subjects.count() == 1 diff --git a/osf/models/mixins.py b/osf/models/mixins.py index d8c0b976565..7851bce18e1 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -21,6 +21,7 @@ from framework.exceptions import PermissionsError from osf.exceptions import ( InvalidTriggerError, + NodeStateError, ValidationValueError, UserStateError, UserNotAffiliatedError, @@ -1184,6 +1185,10 @@ def set_subjects(self, new_subjects, auth, add_log=True, **kwargs): :return: None """ + from osf.models import Registration, DraftRegistration + if isinstance(self, (Registration, DraftRegistration)) and not new_subjects: + raise NodeStateError('Registration must have at least one subject to be registered') + if auth: self.check_subject_perms(auth, **kwargs) self.assert_subject_format(new_subjects, expect_list=True, error_msg='Expecting list of lists.') @@ -1217,6 +1222,10 @@ def set_subjects_from_relationships(self, subjects_list, auth, add_log=True, **k :return: None """ + from osf.models import Registration, DraftRegistration + if isinstance(self, (Registration, DraftRegistration)) and not subjects_list: + raise NodeStateError('Registration must have at least one subject to be registered') + self.check_subject_perms(auth, **kwargs) self.assert_subject_format(subjects_list, expect_list=True, error_msg='Expecting a list of subjects.') if subjects_list: diff --git a/osf/models/registrations.py b/osf/models/registrations.py index c05cbc7eba9..c7edb58ffb9 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -1456,6 +1456,9 @@ def register(self, auth, save=False, child_ids=None, manual_guid=None): if not self.title: raise NodeStateError('Draft Registration must have title to be registered') + if not self.subjects.exists(): + raise NodeStateError('Registration must have at least one subject to be registered') + # Create the registration registration = node.register_node( schema=self.registration_schema, diff --git a/osf_tests/test_draft_registration.py b/osf_tests/test_draft_registration.py index 7d6938814eb..f4b2feb3c1d 100644 --- a/osf_tests/test_draft_registration.py +++ b/osf_tests/test_draft_registration.py @@ -59,12 +59,26 @@ def test_factory(self): assert draft.registration_schema == schema assert draft.registration_metadata == data + @mock.patch('website.settings.ENABLE_ARCHIVER', False) + def test_no_subjects_register(self): + user = factories.UserFactory() + auth = Auth(user) + project = factories.ProjectFactory(creator=user) + draft = factories.DraftRegistrationFactory(branched_from=project) + try: + draft.register(auth) + except NodeStateError as e: + assert str(e) == 'Registration must have at least one subject to be registered' + else: + assert False, 'Expected NodeStateError must be raised' + @mock.patch('website.settings.ENABLE_ARCHIVER', False) def test_register(self): user = factories.UserFactory() auth = Auth(user) project = factories.ProjectFactory(creator=user) draft = factories.DraftRegistrationFactory(branched_from=project) + draft.subjects.add(factories.SubjectFactory()) assert not draft.registered_node draft.register(auth) assert draft.registered_node diff --git a/tests/test_registrations/base.py b/tests/test_registrations/base.py index 60d8b855808..bc8447b39a3 100644 --- a/tests/test_registrations/base.py +++ b/tests/test_registrations/base.py @@ -9,7 +9,7 @@ from osf.models import RegistrationSchema from tests.base import OsfTestCase -from osf_tests.factories import AuthUserFactory, ProjectFactory, DraftRegistrationFactory +from osf_tests.factories import AuthUserFactory, ProjectFactory, DraftRegistrationFactory, SubjectFactory class RegistrationsTestBase(OsfTestCase): def setUp(self): @@ -37,6 +37,7 @@ def setUp(self): 'summary': {'value': 'Some airy'} } ) + self.draft.subjects.add(SubjectFactory()) current_month = timezone.now().strftime('%B') current_year = timezone.now().strftime('%Y') diff --git a/tests/test_registrations/test_embargoes.py b/tests/test_registrations/test_embargoes.py index c58de3c450e..49e1da79073 100644 --- a/tests/test_registrations/test_embargoes.py +++ b/tests/test_registrations/test_embargoes.py @@ -14,7 +14,7 @@ from tests.base import fake, OsfTestCase from osf_tests.factories import ( AuthUserFactory, EmbargoFactory, NodeFactory, ProjectFactory, - RegistrationFactory, UserFactory, UnconfirmedUserFactory, DraftRegistrationFactory, + RegistrationFactory, SubjectFactory, UserFactory, UnconfirmedUserFactory, DraftRegistrationFactory, EmbargoTerminationApprovalFactory ) from tests import utils diff --git a/tests/utils.py b/tests/utils.py index cacd6d3fcd3..5a97433821b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -16,7 +16,7 @@ from framework.auth import Auth from framework.celery_tasks.handlers import celery_teardown_request from osf.email import _render_email_html -from osf_tests.factories import DraftRegistrationFactory +from osf_tests.factories import DraftRegistrationFactory, SubjectFactory from osf.models import Sanction, NotificationType, Notification from tests.base import get_default_metaschema from website.archiver import ARCHIVER_SUCCESS @@ -136,8 +136,10 @@ def mock_archive(project, schema=None, auth=None, draft_registration=None, paren draft_registration = draft_registration or DraftRegistrationFactory( branched_from=project, registration_schema=schema ) + draft_registration.subjects.add(SubjectFactory()) with mock.patch('framework.celery_tasks.handlers.enqueue_task'): + draft_registration.subjects.add(SubjectFactory()) registration = draft_registration.register(auth=auth, save=True) if embargo: