From 7403785a67a5dc19016ff253e46729791cc795b7 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/9] [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 7608b5c02c6..a16055bafca 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -268,7 +268,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 da607c2afb3..026187948a5 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -375,6 +375,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 c6ae06d5fd86969b55efce91574b26881861b312 Mon Sep 17 00:00:00 2001 From: "Brian J. Geiger" Date: Fri, 26 Jun 2026 11:43:36 -0400 Subject: [PATCH 2/9] Revert "Merge branch 'upstream/pbs-26-9' into upstream/pbs-26-13" This reverts commit 64edf5c07e7d6bd1cad0af20870bc21e932817ea, reversing changes made to 7403785a67a5dc19016ff253e46729791cc795b7. --- admin/base/urls.py | 1 - admin/maintenance/views.py | 14 +- admin/management/views.py | 12 +- admin/nodes/views.py | 170 +++------ admin/notifications/forms.py | 8 - admin/notifications/urls.py | 14 - admin/notifications/views.py | 327 +----------------- admin/templates/base.html | 22 -- admin/templates/maintenance/display.html | 15 - admin/templates/management/commands.html | 10 - .../notifications/email_tasks_list.html | 34 -- .../notification_subscriptions_list.html | 37 -- .../notification_type_detail.html | 109 ------ .../notification_type_preview.html | 30 -- .../notification_types_list.html | 37 -- .../notifications/notifications_list.html | 37 -- admin_tests/maintenance/test_views.py | 58 +--- admin_tests/nodes/test_views.py | 168 +-------- admin_tests/preprints/test_views.py | 5 +- api/base/authentication/drf.py | 15 - api/base/middleware.py | 19 - api/base/settings/defaults.py | 1 - api/logs/serializers.py | 2 - api/subscriptions/views.py | 10 - .../notifications/test_notification_digest.py | 45 --- .../views/test_subscriptions_detail.py | 2 - notifications/tasks.py | 8 - osf/email/__init__.py | 6 +- .../commands/populate_notification_types.py | 94 ++--- .../process_manual_restart_approvals.py | 7 +- osf/migrations/0040_maintenancemode.py | 39 --- osf/migrations/0041_notification_scheduled.py | 18 - osf/models/__init__.py | 2 +- osf/models/maintenance_state.py | 13 - osf/models/mixins.py | 19 +- osf/models/nodelog.py | 1 - osf/models/notification.py | 17 +- osf/models/notification_type.py | 1 - osf/models/registrations.py | 2 +- osf/models/sanctions.py | 4 - osf_tests/test_archiver.py | 16 - osf_tests/test_middleware.py | 105 ------ scripts/check_manual_restart_approval.py | 37 +- .../test_check_manual_restart_approval.py | 47 --- website/archiver/tasks.py | 22 +- .../pending_registration_admin.html.mako | 2 +- 46 files changed, 144 insertions(+), 1518 deletions(-) delete mode 100644 admin/notifications/forms.py delete mode 100644 admin/notifications/urls.py delete mode 100644 admin/templates/notifications/email_tasks_list.html delete mode 100644 admin/templates/notifications/notification_subscriptions_list.html delete mode 100644 admin/templates/notifications/notification_type_detail.html delete mode 100644 admin/templates/notifications/notification_type_preview.html delete mode 100644 admin/templates/notifications/notification_types_list.html delete mode 100644 admin/templates/notifications/notifications_list.html delete mode 100644 osf/migrations/0040_maintenancemode.py delete mode 100644 osf/migrations/0041_notification_scheduled.py delete mode 100644 osf_tests/test_middleware.py delete mode 100644 scripts/tests/test_check_manual_restart_approval.py diff --git a/admin/base/urls.py b/admin/base/urls.py index 1eb840bc362..9ff5e03a03e 100644 --- a/admin/base/urls.py +++ b/admin/base/urls.py @@ -38,7 +38,6 @@ re_path(r'^draft_registrations/', include('admin.draft_registrations.urls', namespace='draft_registrations')), re_path(r'^files/', include('admin.files.urls', namespace='files')), re_path(r'^share_reindex/', include('admin.share_reindex.urls', namespace='share_reindex')), - re_path(r'^notifications/', include('admin.notifications.urls', namespace='notifications')), ]), ), ] diff --git a/admin/maintenance/views.py b/admin/maintenance/views.py index 7ca7a53cdef..05e7a8372c9 100644 --- a/admin/maintenance/views.py +++ b/admin/maintenance/views.py @@ -1,7 +1,7 @@ import pytz import datetime -from osf.models import MaintenanceState, MaintenanceMode +from osf.models import MaintenanceState import website.maintenance as maintenance from admin.maintenance.forms import MaintenanceForm @@ -36,17 +36,15 @@ def get_context_data(self, **kwargs): maintenance = MaintenanceState.objects.first() kwargs['form'] = MaintenanceForm() kwargs['current_alert'] = model_to_dict(maintenance) if maintenance else None - kwargs['maintenance_mode'] = MaintenanceMode.is_under_maintenance() return super().get_context_data(**kwargs) def post(self, request, *args, **kwargs): data = request.POST - if maintenance_mode := data.get('maintenance_mode'): - MaintenanceMode(maintenance_mode=False if maintenance_mode == 'True' else True).save() - else: - start = convert_eastern_to_utc(data['start']).isoformat() if data.get('start') else None - end = convert_eastern_to_utc(data['end']).isoformat() if data.get('end') else None - maintenance.set_maintenance(data.get('message', ''), data['level'], start, end) + + start = convert_eastern_to_utc(data['start']).isoformat() if data.get('start') else None + end = convert_eastern_to_utc(data['end']).isoformat() if data.get('end') else None + + maintenance.set_maintenance(data.get('message', ''), data['level'], start, end) return redirect('maintenance:display') diff --git a/admin/management/views.py b/admin/management/views.py index 1610af88641..4b6e6b4c080 100644 --- a/admin/management/views.py +++ b/admin/management/views.py @@ -192,17 +192,7 @@ def post(self, request): class SyncNotificationTemplates(ManagementCommandPermissionView): def post(self, request): - run_type = request.POST.get('run_type') - if run_type == 'restore_one': - template_name = request.POST.get('template_name') - if not template_name: - messages.error(request, 'A template name must be specified when restoring one template. Check your inputs and try again') - return redirect(reverse('management:commands')) - populate_notification_types(restore_one=template_name) - elif run_type == 'restore_all': - populate_notification_types(restore_all=True) - else: - populate_notification_types() + populate_notification_types() messages.success(request, 'Notification templates have been successfully synced.') return redirect(reverse('management:commands')) diff --git a/admin/nodes/views.py b/admin/nodes/views.py index 2adec026b21..71db671620f 100644 --- a/admin/nodes/views.py +++ b/admin/nodes/views.py @@ -110,16 +110,13 @@ def post(self, request, *args, **kwargs): node.created = new_date node.save() - params = dict(node.log_params) - params.update({ - 'last_date': str(last_date), - 'new_date': str(new_date), - }) node.add_log( action=NodeLog.REGISTRATION_DATE_UPDATED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, + auth=request, + params={ + 'last_date': str(last_date), + 'new_date': str(new_date) + }, log_date=timezone.now(), should_hide=False, ) @@ -228,19 +225,23 @@ def post(self, request, *args, **kwargs): message=f'User {user.pk} removed from {node.__class__.__name__.lower()} {node.pk}.', action_flag=CONTRIBUTOR_REMOVED ) - params = dict(node.log_params) - params['contributors'] = [user._id] - node.add_log( - action=NodeLog.CONTRIB_REMOVED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, - log_date=timezone.now(), - should_hide=False, - ) - + # Log invisibly on the OSF. + self.add_contributor_removed_log(node, user) return redirect(self.get_success_url()) + def add_contributor_removed_log(self, node, user): + NodeLog( + action=NodeLog.CONTRIB_REMOVED, + user=None, + params={ + 'project': node.parent_id, + 'node': node.pk, + 'contributors': user.pk + }, + date=timezone.now(), + should_hide=True, + ).save() + class NodeUpdatePermissionsView(NodeMixin, View): permission_required = ('osf.view_node', 'osf.change_node') @@ -265,7 +266,6 @@ def post(self, request, *args, **kwargs): new_permissions_to_add = data.get('new-permissions', []) new_permission_indexes_to_remove = [] - added_contributor_ids = [] for email, permission in zip(new_emails_to_add, new_permissions_to_add): contributor_user = OSFUser.objects.filter(emails__address=email.lower()).first() if not contributor_user: @@ -281,10 +281,8 @@ def post(self, request, *args, **kwargs): auth=request, user_id=contributor_user._id, permissions=permission, - notification_type=None, - log=False, + notification_type=None ) - added_contributor_ids.append(contributor_user._id) messages.success(self.request, f'User with email {email} was successfully added.') # should remove permissions of invalid emails because @@ -293,19 +291,6 @@ def post(self, request, *args, **kwargs): for permission_index in new_permission_indexes_to_remove: new_permissions_to_add.pop(permission_index) - # Log support-added contributors, if any - if added_contributor_ids: - params = resource.log_params - params['contributors'] = added_contributor_ids - resource.add_log( - action=NodeLog.CONTRIB_ADDED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, - log_date=timezone.now(), - should_hide=False, - ) - updated_permissions = data.get('updated-permissions', []) all_permissions = updated_permissions + new_permissions_to_add has_admin = list(filter(lambda permission: ADMIN in permission, all_permissions)) @@ -313,7 +298,6 @@ def post(self, request, *args, **kwargs): messages.error(self.request, 'Must be at least one admin on this node.') return redirect(self.get_success_url()) - permissions_changed = {} for contributor_permission in updated_permissions: guid, permission = contributor_permission.split('-') user = OSFUser.load(guid) @@ -323,21 +307,7 @@ def post(self, request, *args, **kwargs): resource.get_visible(user), request, save=True, - skip_permission=True, - log=False, - ) - permissions_changed[user._id] = permission - - if permissions_changed: - params = resource.log_params - params['contributors'] = permissions_changed - resource.add_log( - action=NodeLog.PERMISSIONS_UPDATED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, - log_date=timezone.now(), - should_hide=False, + skip_permission=True ) return redirect(self.get_success_url()) @@ -362,14 +332,15 @@ def post(self, request, *args, **kwargs): message=f'Node {node.pk} restored.', action_flag=NODE_RESTORED ) - node.add_log( + NodeLog( action=NodeLog.NODE_CREATED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=dict(node.log_params), - log_date=timezone.now(), - should_hide=False, - ) + user=None, + params={ + 'project': node.parent_id, + }, + date=timezone.now(), + should_hide=True, + ).save() else: node.is_deleted = True node.deleted = timezone.now() @@ -381,14 +352,15 @@ def post(self, request, *args, **kwargs): message=f'Node {node.pk} removed.', action_flag=NODE_REMOVED ) - node.add_log( + NodeLog( action=NodeLog.NODE_REMOVED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=dict(node.log_params), - log_date=timezone.now(), - should_hide=False, - ) + user=None, + params={ + 'project': node.parent_id, + }, + date=timezone.now(), + should_hide=True, + ).save() node.save() return redirect(self.get_success_url()) @@ -888,15 +860,6 @@ def post(self, request, *args, **kwargs): node.save() - node.add_log( - action=NodeLog.MADE_PRIVATE, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=dict(node.log_params), - log_date=timezone.now(), - should_hide=False, - ) - return redirect(self.get_success_url()) @@ -908,18 +871,9 @@ class NodeMakePublic(NodeMixin, View): def post(self, request, *args, **kwargs): node = self.get_object() try: - node.set_privacy('public', auth=None, log=False) + node.set_privacy('public') except NodeStateError as e: messages.error(request, str(e)) - else: - node.add_log( - action=NodeLog.MADE_PUBLIC, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=dict(node.log_params), - log_date=timezone.now(), - should_hide=False, - ) return redirect(self.get_success_url()) @@ -946,26 +900,10 @@ def _remove_file_from_schema_response_blocks(registration, removed_file_id): # delete file from registration and metadata and keep it for original project if guid and (file := guid.referent) and (file.target == node) and not isinstance(file, TrashedFile): - file_id = file._id - file_path = getattr(file, 'materialized_path', None) or getattr(file, 'path', None) or '' - copied_from_id = getattr(file, 'copied_from_id', None) or getattr(getattr(file, 'copied_from', None), '_id', None) with transaction.atomic(): file.delete() _update_schema_meta(file.target) - _remove_file_from_schema_response_blocks(node, [file_id, copied_from_id]) - params = dict(node.log_params) - params.update({ - 'pathType': 'file', - 'path': file_path, - }) - node.add_log( - action=NodeLog.FILE_REMOVED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, - log_date=timezone.now(), - should_hide=False, - ) + _remove_file_from_schema_response_blocks(node, [file._id, file.copied_from._id]) return redirect(self.get_success_url()) @@ -1002,18 +940,7 @@ def post(self, request, *args, **kwargs): parent=osfstorage.get_root(), name=osfstorage.archive_folder_name ).first() - copied = file.copy_under(archive_folder) - copied_path = getattr(copied, 'materialized_path', None) or getattr(copied, 'path', None) or '' - params = dict(registration.log_params) - params['path'] = copied_path - registration.add_log( - action=NodeLog.FILE_ADDED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, - log_date=timezone.now(), - should_hide=False, - ) + file.copy_under(archive_folder) messages.success(request, 'The file was successfully added.') return redirect(self.get_success_url()) @@ -1040,21 +967,8 @@ def post(self, request, *args, **kwargs): if not registration_file.exists(): messages.error(request, 'The file with the provided guid is not part of the registration.') return redirect(self.get_success_url()) - file_path = getattr(file, 'materialized_path', None) or getattr(file, 'path', None) or '' + registration_file.delete() - params = dict(registration.log_params) - params.update({ - 'pathType': 'file', - 'path': file_path, - }) - registration.add_log( - action=NodeLog.FILE_REMOVED, - auth=None, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - params=params, - log_date=timezone.now(), - should_hide=False, - ) messages.success(request, 'The file was successfully removed.') return redirect(self.get_success_url()) diff --git a/admin/notifications/forms.py b/admin/notifications/forms.py deleted file mode 100644 index 946754415bb..00000000000 --- a/admin/notifications/forms.py +++ /dev/null @@ -1,8 +0,0 @@ -from django import forms -from osf.models import NotificationType - - -class NotificationTypeForm(forms.ModelForm): - class Meta: - model = NotificationType - fields = '__all__' diff --git a/admin/notifications/urls.py b/admin/notifications/urls.py deleted file mode 100644 index 236059a577e..00000000000 --- a/admin/notifications/urls.py +++ /dev/null @@ -1,14 +0,0 @@ -from django.urls import re_path -from . import views - -app_name = 'admin' - -urlpatterns = [ - re_path(r'$', views.NotificationsList.as_view(), name='list'), - re_path(r'types/$', views.NotificationTypeList.as_view(), name='types_list'), - re_path(r'type_display/(?P\d+)/$', views.NotificationTypeDisplay.as_view(), name='type_display'), - re_path(r'type_detail/(?P\d+)/$', views.NotificationTypeDetail.as_view(), name='type_detail'), - re_path(r'types_preview/(?P\d+)/$', views.NotificationTypePreview.as_view(), name='types_preview'), - re_path(r'subscriptions/$', views.NotificationSubscriptionsList.as_view(), name='subscriptions_list'), - re_path(r'email_tasks/$', views.EmailTasksList.as_view(), name='email_tasks_list'), -] diff --git a/admin/notifications/views.py b/admin/notifications/views.py index e1c55c05f47..6719ac90a8a 100644 --- a/admin/notifications/views.py +++ b/admin/notifications/views.py @@ -1,329 +1,4 @@ -from django.urls import reverse_lazy -from django.db.models import Q -from osf.models import NotificationSubscription, NotificationType, Notification, EmailTask -from django.views.generic import ListView, DetailView, UpdateView -from django.contrib.auth.mixins import PermissionRequiredMixin -from django.forms.models import model_to_dict -from .forms import NotificationTypeForm -from osf.email import _render_email_html -import json -from collections import defaultdict -from mako.lexer import Lexer -from mako.parsetree import ControlLine -import re +from osf.models.notification_subscription import NotificationSubscription def delete_selected_notifications(selected_ids): NotificationSubscription.objects.filter(id__in=selected_ids).delete() - -TEMPLATE_IDENTIFIER_BLACKLIST = { - 'if', 'else', 'and', 'or', 'not', 'in', - 'True', 'False', 'len', 'str', 'int', 'float', 'list', 'dict', 'set', 'tuple', -} - -def resolve_identifiers(identifier_structure): - structure = defaultdict(dict) - if hasattr(identifier_structure, 'nodes') and identifier_structure.nodes: - for node in identifier_structure.nodes: - if isinstance(node, ControlLine) and node.keyword == 'for': - match = re.match(r'for (\w+) in (.+):', node.text) - if match: - iterator, source = match.groups() - structure[node.text] = { - 'type': 'loop', - 'iterator': iterator, - 'source': source, - 'children': resolve_identifiers(node) - } - elif hasattr(node, 'text'): - field_match = re.match(r"(\w+)\['(.+)'\]", node.text) - if field_match: - source, field = field_match.groups() - structure[node.text] = { - 'type': 'field', - 'source': source, - 'field': field - } - return structure - -def generate_mock_json(structure, list_name=None): - item = {} - result = {} - for key, value in structure.items(): - # simple field - if isinstance(value, dict) and value.get('type') == 'field': - field_name = value['field'] - item[field_name] = f"mock_{field_name}" - - # nested loop - elif isinstance(value, dict) and value.get('type') == 'loop': - nested_source = value['source'] - nested_match = re.match(r"\w+\['(.+)'\]", nested_source) - if nested_match: - nested_field = nested_match.group(1) - item[nested_field] = [1, 2, 3, 4] - - # top-level loop wrapper - elif key.startswith('for '): - match = re.match(r'for (\w+) in (.+):', key) - if match: - _, source = match.groups() - # Extract final field name - field_match = re.search(r"(\w+)\['(.+?)'\]$", source) - if field_match: - field_name = field_match.group(1) - list_name = field_match.group(2) - return {field_name: generate_mock_json(value, list_name)} - else: - list_name = source - return generate_mock_json(value, list_name) - if list_name: - result[list_name] = [item, item, item] - - return result - - -def build_safe_context(template: str) -> dict: - templatenode = Lexer(text=template).parse() - identifiers_location = [] - for node in templatenode.get_children(): - if hasattr(node, 'nodes'): - identifiers_location.extend(node.nodes) - - if not identifiers_location: - identifiers_location = templatenode.get_children() - identifier_structure = defaultdict() - for control_structure in identifiers_location: - if isinstance(control_structure, ControlLine): - identifier_structure[control_structure.text] = resolve_identifiers(control_structure) - - identifiers = [x.undeclared_identifiers() for x in identifiers_location if hasattr(x, 'undeclared_identifiers')] - flatten_identifiers = set() - for indentifier_set in identifiers: - flatten_identifiers.update(indentifier_set) - mock_json = generate_mock_json(identifier_structure) - context = {identifier: f'mock_{identifier}' for identifier in flatten_identifiers if identifier not in TEMPLATE_IDENTIFIER_BLACKLIST} - context.update(mock_json) - return context - -class NotificationsList(PermissionRequiredMixin, ListView): - paginate_by = 25 - template_name = 'notifications/notifications_list.html' - ordering = 'id' - permission_required = 'osf.view_notification' - raise_exception = True - model = Notification - - def get_queryset(self): - qs = Notification.objects.all().order_by(self.ordering) - q = self.request.GET.get('q') - if q: - qs = qs.filter( - Q(subscription__notification_type__name__icontains=q) | - Q(subscription__user__username__icontains=q) | - Q(subscription__message_frequency__icontains=q) - ) - return qs - - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - q = self.request.GET.get('q', '') - context['q'] = q - # append search param to pagination links - if q: - context['extra_query_params'] = f"&q={q}" - else: - context['extra_query_params'] = '' - - context['notifications'] = context['object_list'] - context['page'] = context['page_obj'] - return context - -class NotificationSubscriptionsList(PermissionRequiredMixin, ListView): - paginate_by = 25 - template_name = 'notifications/notification_subscriptions_list.html' - ordering = 'id' - permission_required = 'osf.view_notificationsubscription' - raise_exception = True - model = NotificationSubscription - - def get_queryset(self): - qs = NotificationSubscription.objects.all().order_by(self.ordering) - q = self.request.GET.get('q') - if q: - qs = qs.filter( - Q(notification_type__name__icontains=q) | - Q(user__username__icontains=q) | - Q(message_frequency__icontains=q) - ) - return qs - - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - q = self.request.GET.get('q', '') - context['q'] = q - # append search param to pagination links - if q: - context['extra_query_params'] = f"&q={q}" - else: - context['extra_query_params'] = '' - context['subscriptions'] = context['object_list'] - context['page'] = context['page_obj'] - return context - -class EmailTasksList(PermissionRequiredMixin, ListView): - paginate_by = 25 - template_name = 'notifications/email_tasks_list.html' - ordering = 'task_id' - permission_required = 'osf.view_emailtask' - raise_exception = True - model = EmailTask - - def get_queryset(self): - qs = EmailTask.objects.all().order_by(self.ordering) - q = self.request.GET.get('q') - if q: - qs = qs.filter( - Q(task_id=q) | - Q(user__username__icontains=q) | - Q(status=q) - ) - return qs - - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - q = self.request.GET.get('q', '') - context['q'] = q - # append search param to pagination links - if q: - context['extra_query_params'] = f"&q={q}" - else: - context['extra_query_params'] = '' - context['email_tasks'] = context['object_list'] - context['page'] = context['page_obj'] - return context - -class NotificationTypeList(PermissionRequiredMixin, ListView): - paginate_by = 25 - template_name = 'notifications/notification_types_list.html' - ordering = 'name' - permission_required = 'osf.view_notificationtype' - raise_exception = True - model = NotificationType - - def get_queryset(self): - qs = NotificationType.objects.all().order_by(self.ordering) - q = self.request.GET.get('q') - if q: - qs = qs.filter( - Q(name__icontains=q) | - Q(subject__icontains=q) | - Q(notification_interval_choices__icontains=q) - ) - return qs - - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - q = self.request.GET.get('q', '') - context['q'] = q - # append search param to pagination links - if q: - context['extra_query_params'] = f"&q={q}" - else: - context['extra_query_params'] = '' - - context['notification_types'] = context['object_list'] - context['page'] = context['page_obj'] - return context - -class NotificationTypeDisplay(PermissionRequiredMixin, DetailView): - model = NotificationType - template_name = 'notifications/notification_type_detail.html' - permission_required = 'osf.view_notificationtype' - raise_exception = True - - def get_object(self, queryset=None): - return NotificationType.objects.get(id=self.kwargs.get('pk')) - - def get_context_data(self, *args, **kwargs): - notification_type = self.get_object() - notification_type_dict = model_to_dict(notification_type) - fields = notification_type_dict.copy() - kwargs.setdefault('page_number', self.request.GET.get('page', '1')) - notification_type_dict['is_digest_type'] = notification_type.is_digest_type - kwargs['notification_type'] = notification_type_dict - kwargs['template'] = notification_type_dict.pop('template', None) - kwargs['change_form'] = NotificationTypeForm(initial=fields) - - return kwargs - -class NotificationTypePreview(PermissionRequiredMixin, DetailView): - model = NotificationType - template_name = 'notifications/notification_type_preview.html' - permission_required = 'osf.view_notificationtype' - raise_exception = True - - def get_object(self, queryset=None): - return NotificationType.objects.get(id=self.kwargs.get('pk')) - - def get_context_data(self, *args, **kwargs): - notification_type = self.get_object() - raw_context = self.request.GET.get('context') - if raw_context: - try: - if notification_type.is_digest_type: - safe_context = {'notifications': [json.loads(raw_context)]} - else: - safe_context = json.loads(raw_context) - - return_context = json.loads(raw_context) - except json.JSONDecodeError as e: - kwargs['rendered_template'] = f"Error parsing JSON: {str(e)}" - kwargs['context'] = raw_context - return kwargs - else: - if notification_type.is_digest_type: - inner_context = build_safe_context(notification_type.template) - inner_template = _render_email_html(notification_type, ctx=inner_context, return_original_error=True) - safe_context = {'notifications': [inner_template]} - return_context = inner_context - else: - safe_context = build_safe_context(notification_type.template) - return_context = safe_context - - if notification_type.is_digest_type: - # Use user_digest template as a wrapper for digest notification preview. - template_obj = NotificationType.objects.get(name='user_digest') - else: - template_obj = notification_type - try: - kwargs['rendered_template'] = _render_email_html(template_obj, ctx=safe_context, return_original_error=True) - except Exception as e: - kwargs['rendered_template'] = f"Error rendering template: {str(e)}" - - kwargs['context'] = json.dumps(return_context, indent=4) - - return kwargs - -class NotificationTypeDetail(PermissionRequiredMixin, DetailView): - model = NotificationType - template_name = 'notifications/notification_type_detail.html' - permission_required = 'osf.view_notificationtype' - raise_exception = True - - def get(self, request, *args, **kwargs): - view = NotificationTypeDetail.as_view() - return view(request, *args, **kwargs) - - def post(self, request, *args, **kwargs): - view = NotificationTypeChangeForm.as_view() - return view(request, *args, **kwargs) - -class NotificationTypeChangeForm(PermissionRequiredMixin, UpdateView): - template_name = 'institutions/detail.html' - permission_required = 'osf.change_notificationtype' - raise_exception = True - model = NotificationType - form_class = NotificationTypeForm - - def get_success_url(self, *args, **kwargs): - return reverse_lazy('notifications:type_display', kwargs={'pk': self.kwargs.get('pk')}) diff --git a/admin/templates/base.html b/admin/templates/base.html index 5f645ffe267..31a89b74037 100644 --- a/admin/templates/base.html +++ b/admin/templates/base.html @@ -289,28 +289,6 @@ {% endif %} {% endif %} - {% if perms.osf.view_notification or perms.osf.view_notificationtype or perms.osf.view_notificationsubscription %} -
  • - Notifications -
    • -
    - -
    - {% endif %} {% if perms.osf.view_metrics %}
  • Metrics
    • {% endif %} diff --git a/admin/templates/maintenance/display.html b/admin/templates/maintenance/display.html index 6064cf6f131..b29028fcbba 100644 --- a/admin/templates/maintenance/display.html +++ b/admin/templates/maintenance/display.html @@ -66,21 +66,6 @@

    Put up an alert:

    - - -
    -
    -
    - {% csrf_token %} - - {% if maintenance_mode %} - - {% else %} - - {% endif %} -
    -
    -
    {% endif %} {% endblock content %} diff --git a/admin/templates/management/commands.html b/admin/templates/management/commands.html index b32005dfb28..fd9ceec9c1b 100644 --- a/admin/templates/management/commands.html +++ b/admin/templates/management/commands.html @@ -172,16 +172,6 @@

    Sync Notification Templates

    {% csrf_token %} - - -
    - -
    - diff --git a/admin/templates/notifications/email_tasks_list.html b/admin/templates/notifications/email_tasks_list.html deleted file mode 100644 index cf4345f8907..00000000000 --- a/admin/templates/notifications/email_tasks_list.html +++ /dev/null @@ -1,34 +0,0 @@ -{% extends "base.html" %} -{% load render_bundle from webpack_loader %} -{% load static %} -{% block title %} - List of Email Tasks -{% endblock title %} -{% block content %} -

    List of Email Tasks

    - - {% include "util/pagination.html" with items=page status=status %} - - - -
    - - - - - - - - - - {% for email_task in email_tasks %} - - - - - - {% endfor %} - -
    Task IDUserStatus
    {{ email_task.task_id }}{{ email_task.user }}{{ email_task.status }}
    - -{% endblock content %} diff --git a/admin/templates/notifications/notification_subscriptions_list.html b/admin/templates/notifications/notification_subscriptions_list.html deleted file mode 100644 index 7ed0febd384..00000000000 --- a/admin/templates/notifications/notification_subscriptions_list.html +++ /dev/null @@ -1,37 +0,0 @@ -{% extends "base.html" %} -{% load render_bundle from webpack_loader %} -{% load static %} -{% block title %} - List of Notification Subscriptions -{% endblock title %} -{% block content %} -

    List of Notification Subscriptions

    - - {% include "util/pagination.html" with items=page status=status %} -
    - - -
    - - - - - - - - - - - {% for subscription in subscriptions %} - - - - - - - - {% endfor %} - -
    Notification Type NameUserMessage FrequencySubscribed Object
    {{ subscription.notification_type.name }}{{ subscription.user }}{{ subscription.message_frequency }}{{ subscription.subscribed_object }}
    - -{% endblock content %} diff --git a/admin/templates/notifications/notification_type_detail.html b/admin/templates/notifications/notification_type_detail.html deleted file mode 100644 index 1a571099762..00000000000 --- a/admin/templates/notifications/notification_type_detail.html +++ /dev/null @@ -1,109 +0,0 @@ -{% extends "base.html" %} -{% load static %} -{% load render_bundle from webpack_loader %} -{% block title %} - Notification Type -{% endblock title %} -{% block content %} -
    -
    -
    - -
    -
    -
    -
    -

    {{ notification_type.name }}

    -
    -
    -
    -
    - -
    -
    -
    -
    - - {% for field, value in notification_type.items %} - - - - - {% endfor %} - - - - - -
    {{ field }}{{ value | safe }}
    template
    {{ template }}
    -
    - -
    -
    -
    -
    -
    - -{% endblock content %} - -{% block bottom_js %} - -{% endblock %} diff --git a/admin/templates/notifications/notification_type_preview.html b/admin/templates/notifications/notification_type_preview.html deleted file mode 100644 index e9aefbe3284..00000000000 --- a/admin/templates/notifications/notification_type_preview.html +++ /dev/null @@ -1,30 +0,0 @@ -

    Notification Template Preview

    -

    Rendered Template

    - - - -
    - {{ rendered_template|safe }} -
    -

    Mock Data

    -
    - Edit the mock data and click "Render Preview" to see how changes affect the rendered template.
    - Note: The mock data should be a JSON object matching the expected context for the template. Please ensure that the JSON is properly formatted if error occurs during rendering. -
    -
    - -
    diff --git a/admin/templates/notifications/notification_types_list.html b/admin/templates/notifications/notification_types_list.html deleted file mode 100644 index 655e8ec1ab2..00000000000 --- a/admin/templates/notifications/notification_types_list.html +++ /dev/null @@ -1,37 +0,0 @@ -{% extends "base.html" %} -{% load render_bundle from webpack_loader %} -{% load static %} -{% block title %} - List of Notification Types -{% endblock title %} -{% block content %} -

    List of Notification Types

    - - {% include "util/pagination.html" with items=page status=status %} -
    - - -
    - - - - - - - - - - - {% for notification_type in notification_types %} - - - - - - - - {% endfor %} - -
    NameSubjectInterval ChoicesIs Digest
    {{ notification_type.name }}{{ notification_type.subject }}{{ notification_type.notification_interval_choices }}{{ notification_type.is_digest_type }}
    - -{% endblock content %} diff --git a/admin/templates/notifications/notifications_list.html b/admin/templates/notifications/notifications_list.html deleted file mode 100644 index 76e747ca5e1..00000000000 --- a/admin/templates/notifications/notifications_list.html +++ /dev/null @@ -1,37 +0,0 @@ -{% extends "base.html" %} -{% load render_bundle from webpack_loader %} -{% load static %} -{% block title %} - List of Notifications -{% endblock title %} -{% block content %} -

    List of Notifications

    - - {% include "util/pagination.html" with items=page status=status %} -
    - - -
    - - - - - - - - - - - {% for notification in notifications %} - - - - - - - - {% endfor %} - -
    Notification Type NameUserSentIs fake sent
    {{ notification.subscription.notification_type.name }}{{ notification.subscription.user }}{{ notification.sent }}{{ notification.fake_sent }}
    - -{% endblock content %} diff --git a/admin_tests/maintenance/test_views.py b/admin_tests/maintenance/test_views.py index f42ac73c0a1..abeaa6af677 100644 --- a/admin_tests/maintenance/test_views.py +++ b/admin_tests/maintenance/test_views.py @@ -8,7 +8,7 @@ from django.core.exceptions import PermissionDenied import website.maintenance as maintenance -from osf.models import MaintenanceState, MaintenanceMode +from osf.models import MaintenanceState from osf_tests.factories import AuthUserFactory from admin_tests.utilities import setup_view @@ -105,59 +105,3 @@ def test_correct_view_permissions(self, req, user, plain_view): res = plain_view.as_view()(req) assert res.status_code == 200 - - -@pytest.mark.urls('admin.base.urls') -class TestMaintenanceMode: - - @pytest.fixture() - def user(self): - user = AuthUserFactory() - view_permission = Permission.objects.get(codename='change_maintenancestate') - user.user_permissions.add(view_permission) - user.save() - return user - - @pytest.fixture() - def plain_view(self): - return views.MaintenanceDisplay - - @pytest.fixture() - def view(self, user, plain_view): - req = RequestFactory().get('/fake_path') - req.user = user - view = plain_view() - setup_view(view, req) - return view - - def test_get_context_data_includes_maintenance_mode(self, view): - MaintenanceMode(maintenance_mode=True).save() - context = view.get_context_data() - assert context['maintenance_mode'] is True - MaintenanceMode(maintenance_mode=False).save() - context = view.get_context_data() - assert context['maintenance_mode'] is False - - def test_post_toggles_maintenance_mode_on(self, user, plain_view): - MaintenanceMode(maintenance_mode=False).save() - req = RequestFactory().post('/fake_path', data={'maintenance_mode': 'False'}) - req.user = user - view = plain_view() - setup_view(view, req) - response = view.post(req) - # It should redirect back to the display page - assert response.status_code == 302 - # The database state should now be True - assert MaintenanceMode.is_under_maintenance() is True - - def test_post_toggles_maintenance_mode_off(self, user, plain_view): - MaintenanceMode(maintenance_mode=True).save() - req = RequestFactory().post('/fake_path', data={'maintenance_mode': 'True'}) - req.user = user - view = plain_view() - setup_view(view, req) - response = view.post(req) - # It should redirect back to the display page - assert response.status_code == 302 - # The database state should now be False - assert MaintenanceMode.is_under_maintenance() is False diff --git a/admin_tests/nodes/test_views.py b/admin_tests/nodes/test_views.py index d7cdcf8bece..2c29b1af2c6 100644 --- a/admin_tests/nodes/test_views.py +++ b/admin_tests/nodes/test_views.py @@ -21,18 +21,13 @@ Sanction, SchemaResponse, DraftRegistration, - ) from admin.nodes.views import ( NodeAddOsfStorageFileView, NodeConfirmSpamView, NodeDeleteView, - NodeMakePrivate, - NodeMakePublic, NodeRemoveContributorView, NodeRemoveOsfStorageFileView, - NodeUpdatePermissionsView, - RegistrationUpdateDateView, NodeView, NodeReindexShare, NodeReindexElastic, @@ -82,14 +77,6 @@ def patch_messages(request): setattr(request, '_messages', messages) -def assert_support_attributed_log(log, admin_user=None): - """Support admin actions attribute logs to the support label, not the staff user.""" - assert log.user is None - assert log.foreign_user == NodeLog.SUPPORT_USER_LABEL - if admin_user is not None: - assert log.user_id is None - - class TestNodeView(AdminTestCase): def test_get_flagged_spam(self): @@ -189,9 +176,7 @@ class TestNodeDeleteView(AdminTestCase): def setUp(self): super().setUp() self.node = ProjectFactory() - self.admin_user = AuthUserFactory() self.request = RequestFactory().post('/fake_path') - self.request.user = self.admin_user self.plain_view = NodeDeleteView self.view = setup_log_view(self.plain_view(), self.request, guid=self.node._id) self.url = reverse('nodes:remove', kwargs={'guid': self.node._id}) @@ -205,33 +190,18 @@ def test_remove_node(self): assert self.node.is_deleted assert AdminLogEntry.objects.count() == count + 1 assert self.node.deleted == mock_now - log = self.node.logs.filter( - action=NodeLog.NODE_REMOVED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) def test_restore_node(self): self.view.post(self.request) self.node.refresh_from_db() assert self.node.is_deleted assert self.node.deleted is not None - remove_log = self.node.logs.filter( - action=NodeLog.NODE_REMOVED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(remove_log, self.admin_user) count = AdminLogEntry.objects.count() self.view.post(self.request) self.node.reload() assert not self.node.is_deleted assert self.node.deleted is None assert AdminLogEntry.objects.count() == count + 1 - restore_log = self.node.logs.filter( - action=NodeLog.NODE_CREATED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(restore_log, self.admin_user) def test_no_user_permissions_raises_error(self): user = AuthUserFactory() @@ -267,14 +237,12 @@ class TestRemoveContributor(AdminTestCase): def setUp(self): super().setUp() self.user = AuthUserFactory() - self.admin_user = AuthUserFactory() self.node = ProjectFactory(creator=self.user) self.user_2 = AuthUserFactory() self.node.add_contributor(self.user_2) self.node.save() self.view = NodeRemoveContributorView self.request = RequestFactory().post('/fake_path') - self.request.user = self.admin_user self.url = reverse('nodes:remove-user', kwargs={'guid': self.node._id, 'user_id': self.user.id}) def test_remove_contributor(self): @@ -304,16 +272,10 @@ def test_do_not_remove_last_admin(self): assert len(list(self.node.get_admin_contributors(self.node.contributors))) == 1 assert AdminLogEntry.objects.count() == count - def test_log(self): - assert not self.node.logs.filter(action=NodeLog.CONTRIB_REMOVED).exists() + def test_no_log(self): view = setup_log_view(self.view(), self.request, guid=self.node._id, user_id=self.user_2.id) view.post(self.request) - log = self.node.logs.filter( - action=NodeLog.CONTRIB_REMOVED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) - assert self.user_2._id in log.params['contributors'] + assert self.node.logs.latest().action != NodeLog.CONTRIB_REMOVED def test_no_user_permissions_raises_error(self): guid = self.node._id @@ -979,9 +941,7 @@ def setUp(self): self.registration_without_registered_from.registered_from = None self.registration_without_registered_from.save() - self.admin_user = AuthUserFactory() self.request = RequestFactory().get('/fake_path') - self.request.user = self.admin_user patch_messages(self.request) def test_no_guid_found(self): @@ -1030,11 +990,6 @@ def test_file_is_added_to_registration_osfstorage(self): assert registration_osfstorage.get_root().children.get( name=registration_osfstorage.archive_folder_name ).children.filter(name=file.name).exists() - log = self.registration_registered_from.logs.filter( - action=NodeLog.FILE_ADDED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) class TestOsfStorageRegistrationFileRemove(AdminTestCase): @@ -1060,9 +1015,7 @@ def setUp(self): self.project = ProjectFactory() self.registration_registered_from = RegistrationFactory(project=self.project) - self.admin_user = AuthUserFactory() self.request = RequestFactory().get('/fake_path') - self.request.user = self.admin_user patch_messages(self.request) def test_no_guid_found(self): @@ -1119,123 +1072,6 @@ def test_file_is_removed_from_registration_osfstorage(self): name=registration_osfstorage.archive_folder_name ).children.exists() assert not self.registration_registered_from.files.exists() - remove_log = self.registration_registered_from.logs.filter( - action=NodeLog.FILE_REMOVED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(remove_log, self.admin_user) - - -class TestNodeMakePrivate(AdminTestCase): - def setUp(self): - super().setUp() - self.admin_user = AuthUserFactory() - self.node = ProjectFactory(is_public=True) - self.request = RequestFactory().post('/fake_path') - self.request.user = self.admin_user - - def test_make_private(self): - view = setup_log_view(NodeMakePrivate(), self.request, guid=self.node._id) - view.post(self.request) - self.node.refresh_from_db() - assert self.node.is_public is False - log = self.node.logs.filter( - action=NodeLog.MADE_PRIVATE, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) - - -class TestNodeMakePublic(AdminTestCase): - def setUp(self): - super().setUp() - self.admin_user = AuthUserFactory() - self.node = ProjectFactory(is_public=False) - self.request = RequestFactory().post('/fake_path') - self.request.user = self.admin_user - - def test_make_public(self): - view = setup_log_view(NodeMakePublic(), self.request, guid=self.node._id) - view.post(self.request) - self.node.refresh_from_db() - assert self.node.is_public is True - log = self.node.logs.filter( - action=NodeLog.MADE_PUBLIC, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) - - -class TestNodeUpdatePermissions(AdminTestCase): - def setUp(self): - super().setUp() - self.admin_user = AuthUserFactory() - self.node = ProjectFactory(creator=self.admin_user) - self.other_admin = AuthUserFactory() - self.node.add_contributor(self.other_admin, permissions=permissions.ADMIN) - self.node.save() - self.request = RequestFactory().post('/fake_path') - patch_messages(self.request) - self.request.user = self.admin_user - - def test_update_permissions(self): - # Form must include at least one admin permission or the view rejects the POST. - self.request.POST = { - 'updated-permissions': [ - f'{self.admin_user._id}-{permissions.ADMIN}', - f'{self.other_admin._id}-{permissions.READ}', - ], - } - view = setup_log_view(NodeUpdatePermissionsView(), self.request, guid=self.node._id) - view.post(self.request) - log = self.node.logs.filter( - action=NodeLog.PERMISSIONS_UPDATED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) - assert log.params['contributors'] == { - self.admin_user._id: permissions.ADMIN, - self.other_admin._id: permissions.READ, - } - - def test_add_contributor(self): - new_user = AuthUserFactory() - self.request.POST = { - 'updated-permissions': [f'{self.admin_user._id}-{permissions.ADMIN}'], - 'new-emails': [new_user.emails.first().address], - 'new-permissions': [permissions.READ], - } - view = setup_log_view(NodeUpdatePermissionsView(), self.request, guid=self.node._id) - view.post(self.request) - log = self.node.logs.filter( - action=NodeLog.CONTRIB_ADDED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) - assert log.params['contributors'] == [new_user._id] - - -class TestRegistrationUpdateDate(AdminTestCase): - def setUp(self): - super().setUp() - self.admin_user = AuthUserFactory() - self.registration = RegistrationFactory() - self.request = RequestFactory().post('/fake_path') - patch_messages(self.request) - self.request.user = self.admin_user - - def test_update_registration_date(self): - new_date = (timezone.now() - datetime.timedelta(days=30)).replace(microsecond=0) - self.request.POST = {'registered_date': new_date.strftime('%Y-%m-%d %H:%M:%S')} - view = setup_log_view(RegistrationUpdateDateView(), self.request, guid=self.registration._id) - view.post(self.request) - self.registration.refresh_from_db() - assert self.registration.registered_date == new_date - log = self.registration.logs.filter( - action=NodeLog.REGISTRATION_DATE_UPDATED, - foreign_user=NodeLog.SUPPORT_USER_LABEL, - ).latest('date') - assert_support_attributed_log(log, self.admin_user) class TestEmbargoReportView(AdminTestCase): diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index 02519290733..5582c1b07eb 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -542,8 +542,7 @@ def test_do_not_remove_last_admin(self): assert len(list(self.preprint.get_admin_contributors(self.preprint.contributors))) == 1 assert AdminLogEntry.objects.count() == count - def test_log(self): - assert not self.preprint.logs.filter(action=PreprintLog.CONTRIB_REMOVED).exists() + def test_no_log(self): view = setup_log_view( self.view(), self.request, @@ -551,7 +550,7 @@ def test_log(self): user_id=self.user_2.id ) view.post(self.request) - assert self.preprint.logs.filter(action=PreprintLog.CONTRIB_REMOVED).exists() + assert self.preprint.logs.latest().action != PreprintLog.CONTRIB_REMOVED @pytest.mark.urls('admin.base.urls') diff --git a/api/base/authentication/drf.py b/api/base/authentication/drf.py index 9dd13ef15e6..9db2ca8faeb 100644 --- a/api/base/authentication/drf.py +++ b/api/base/authentication/drf.py @@ -21,7 +21,6 @@ from osf.models import OSFUser from osf.utils.fields import ensure_str from website import settings -from framework import sentry SessionStore = import_module(api_settings.SESSION_ENGINE).SessionStore @@ -93,20 +92,6 @@ def check_user(user): # For all other cases, the user status is invalid. Although such status can't be reached with # normal user-facing web application flow, it is still possible as a result of direct database # access, coding bugs, database corruption, etc. - extra_data = { - 'user_id': user.id, - 'user_guid': user._id, - 'is_active': user.is_active, - 'is_disabled': user.is_disabled, - 'is_merged': user.is_merged, - 'date_disabled': user.date_disabled, - 'is_confirmed': user.is_confirmed, - 'is_registered': user.is_registered, - 'can_login': user.has_usable_password() or ( - 'VERIFIED' in sum([list(each.values()) for each in user.external_identity.values()], []) - ), - } - sentry.log_message(f'Invalid user account status detected: user_id={user._id}', extra_data=extra_data) raise InvalidAccountError diff --git a/api/base/middleware.py b/api/base/middleware.py index 239d64194cb..bec771aba61 100644 --- a/api/base/middleware.py +++ b/api/base/middleware.py @@ -5,7 +5,6 @@ from importlib import import_module from django.conf import settings -from django.http import JsonResponse from django.contrib.sessions.middleware import SessionMiddleware from django.utils.deprecation import MiddlewareMixin from sentry_sdk import init @@ -25,7 +24,6 @@ from .api_globals import api_globals from api.base import settings as api_settings from api.base.authentication.drf import drf_get_session_from_cookie -from osf.models import MaintenanceMode SessionStore = import_module(settings.SESSION_ENGINE).SessionStore @@ -134,20 +132,3 @@ def process_request(self, request): request.session = drf_get_session_from_cookie(cookie) else: request.session = SessionStore() - - -class MaintenanceModeMiddleware: - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - if MaintenanceMode.is_under_maintenance(): - return JsonResponse( - { - 'meta': { - 'maintenance_mode': True, - 'status_page': 'https://status.cos.io', - }, - }, status=503, - ) - return self.get_response(request) diff --git a/api/base/settings/defaults.py b/api/base/settings/defaults.py index ff40837d3c4..215a2552dfe 100644 --- a/api/base/settings/defaults.py +++ b/api/base/settings/defaults.py @@ -232,7 +232,6 @@ 'django.middleware.common.CommonMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', 'api.base.middleware.UnsignCookieSessionMiddleware', - 'api.base.middleware.MaintenanceModeMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', diff --git a/api/logs/serializers.py b/api/logs/serializers.py index 7768074c6e0..51567b991f3 100644 --- a/api/logs/serializers.py +++ b/api/logs/serializers.py @@ -203,13 +203,11 @@ class NodeLogSerializer(JSONAPISerializer): 'id', 'date', 'action', - 'foreign_user', ] id = ser.CharField(read_only=True, source='_id') date = VersionedDateTimeField(read_only=True) action = ser.CharField(read_only=True) - foreign_user = ser.CharField(read_only=True) params = ser.SerializerMethodField(read_only=True) links = LinksField({'self': 'get_absolute_url'}) diff --git a/api/subscriptions/views.py b/api/subscriptions/views.py index 14a97d50489..52be8669231 100644 --- a/api/subscriptions/views.py +++ b/api/subscriptions/views.py @@ -306,9 +306,6 @@ def update(self, request, *args, **kwargs): raise PermissionDenied for instance in qs: - instance.legacy_id = self.kwargs['subscription_id'] - instance.event_name = 'global_file_updated' - serializer = self.get_serializer(instance=instance, data=request.data, partial=True) serializer.is_valid(raise_exception=True) self.perform_update(serializer) @@ -329,9 +326,6 @@ def update(self, request, *args, **kwargs): raise PermissionDenied for instance in qs: - instance.legacy_id = self.kwargs['subscription_id'] - instance.event_name = 'global_reviews' - serializer = self.get_serializer(instance=instance, data=request.data, partial=True) serializer.is_valid(raise_exception=True) self.perform_update(serializer) @@ -360,9 +354,6 @@ def update(self, request, *args, **kwargs): raise PermissionDenied for instance in qs: - instance.legacy_id = self.kwargs['subscription_id'] - instance.event_name = 'file_updated' - serializer = self.get_serializer(instance=instance, data=request.data, partial=True) serializer.is_valid(raise_exception=True) self.perform_update(serializer) @@ -370,7 +361,6 @@ def update(self, request, *args, **kwargs): else: instance.event_name = instance.notification_type.name # Set event_name for serializer to use - instance.legacy_id = instance.notification_type.name # Set legacy_id for serializer to use partial = kwargs.pop('partial', False) serializer = self.get_serializer(instance, data=request.data, partial=partial) diff --git a/api_tests/notifications/test_notification_digest.py b/api_tests/notifications/test_notification_digest.py index 73067456f27..a1065c27604 100644 --- a/api_tests/notifications/test_notification_digest.py +++ b/api_tests/notifications/test_notification_digest.py @@ -1,5 +1,4 @@ import pytest -from django.utils import timezone from django.contrib.contenttypes.models import ContentType from osf.models import Notification, NotificationType, NotificationTypeEnum, EmailTask, Email @@ -187,26 +186,6 @@ def test_get_users_emails(self): assert user_info['user_id'] == user._id assert any(msg['notification_id'] == notification1.id for msg in user_info['info']) - def test_get_users_emails_ignore_scheduled(self): - user = AuthUserFactory() - notification_type = NotificationType.objects.get(name=NotificationTypeEnum.USER_FILE_UPDATED) - notification1 = Notification.objects.create( - subscription=add_notification_subscription(user, notification_type, 'daily'), - event_context={}, - sent=None - ) - Notification.objects.create( - subscription=add_notification_subscription(user, notification_type, 'daily'), - event_context={}, - sent=None, - scheduled=timezone.now() - ) - res = list(get_users_emails('daily')) - assert len(res) == 1 - user_info = res[0] - assert user_info['user_id'] == user._id - assert any(msg['notification_id'] == notification1.id for msg in user_info['info']) - def test_get_moderators_emails(self): user = AuthUserFactory() provider = RegistrationProviderFactory() @@ -225,30 +204,6 @@ def test_get_moderators_emails(self): ] assert entry, 'Expected moderator digest group' - def test_get_moderators_emails_ignore_scheduled(self): - user = AuthUserFactory() - provider = RegistrationProviderFactory() - reg = RegistrationFactory(provider=provider) - notification_type = NotificationType.objects.get(name=NotificationTypeEnum.PROVIDER_NEW_PENDING_SUBMISSIONS) - subscription = add_notification_subscription(user, notification_type, 'daily', subscribed_object=reg) - Notification.objects.create( - subscription=subscription, - event_context={}, - sent=None - ) - Notification.objects.create( - subscription=subscription, - event_context={}, - sent=None, - scheduled=timezone.now() - ) - res = list(get_moderators_emails('daily')) - assert len(res) >= 1 - entry = [ - x for x in res if x['user_id'] == user._id and subscription.subscribed_object.id == reg.id - ] - assert entry, 'Expected moderator digest group' - def test_send_users_digest_email_end_to_end(self): user = AuthUserFactory() notification_type = NotificationType.objects.get(name=NotificationTypeEnum.USER_FILE_UPDATED) diff --git a/api_tests/subscriptions/views/test_subscriptions_detail.py b/api_tests/subscriptions/views/test_subscriptions_detail.py index f14eeec3cfa..a7246bbbd19 100644 --- a/api_tests/subscriptions/views/test_subscriptions_detail.py +++ b/api_tests/subscriptions/views/test_subscriptions_detail.py @@ -342,5 +342,3 @@ def test_subscription_detail_patch( res = app.patch_json_api(url_user_global_file_updated, payload, auth=user.auth) assert res.status_code == 200 assert res.json['data']['attributes']['frequency'] == 'none' - assert res.json['data'].get('id') is not None - assert res.json['data']['attributes'].get('event_name') is not None diff --git a/notifications/tasks.py b/notifications/tasks.py index 9775a44fc7a..4f802285c93 100644 --- a/notifications/tasks.py +++ b/notifications/tasks.py @@ -133,7 +133,6 @@ def send_user_email_task(self, user_id, notification_ids, **kwargs): email_task.error_message = email_task.error_message + f'Max retries reached: {str(e)} \n' email_task.save() logger.error(f'Max retries reached for send_moderator_email_task for user {user_id}. Task id: {self.request.id}. Errors: {email_task.error_message}') - notifications_qs.update(scheduled=None) return email_task, _ = EmailTask.objects.get_or_create(task_id=self.request.id) @@ -292,7 +291,6 @@ def send_moderator_email_task(self, user_id, notification_ids, provider_content_ email_task.error_message = email_task.error_message + f'\nMax retries reached: {str(e)}' email_task.save() logger.error(f'Max retries reached for send_moderator_email_task for user {user_id}. Task id: {self.request.id}. Errors: {email_task.error_message}') - notifications_qs.update(scheduled=None) return email_task.status = 'RETRY' @@ -316,8 +314,6 @@ def send_users_digest_email(dry_run=False): user_id = group['user_id'] notification_ids = [msg['notification_id'] for msg in group['info']] if not dry_run: - notifications_qs = Notification.objects.filter(id__in=notification_ids) - notifications_qs.update(scheduled=timezone.now()) send_user_email_task.delay(user_id, notification_ids) @celery_app.task(name='notifications.tasks.send_moderators_digest_email') @@ -338,8 +334,6 @@ def send_moderators_digest_email(dry_run=False): provider_content_type_id = group['provider_content_type_id'] notification_ids = [msg['notification_id'] for msg in group['info']] if not dry_run: - notifications_qs = Notification.objects.filter(id__in=notification_ids) - notifications_qs.update(scheduled=timezone.now()) send_moderator_email_task.delay(user_id, notification_ids, provider_content_type_id, provider_id) def get_moderators_emails(message_freq: str): @@ -364,7 +358,6 @@ def get_moderators_emails(message_freq: str): INNER JOIN osf_notificationtype AS nt ON ns.notification_type_id = nt.id LEFT JOIN osf_guid ON ns.user_id = osf_guid.object_id WHERE n.sent IS NULL - AND n.scheduled IS NULL AND ns.message_frequency = %s AND nt.name IN (%s, %s) AND nt.name NOT IN (%s, %s, %s) @@ -408,7 +401,6 @@ def get_users_emails(message_freq): INNER JOIN osf_notificationtype AS nt ON ns.notification_type_id = nt.id LEFT JOIN osf_guid ON ns.user_id = osf_guid.object_id WHERE n.sent IS NULL - AND n.scheduled IS NULL AND ns.message_frequency = %s AND nt.name NOT IN (%s, %s, %s, %s, %s) AND osf_guid.content_type_id = ( diff --git a/osf/email/__init__.py b/osf/email/__init__.py index 1cf39af809b..bb22bbcd637 100644 --- a/osf/email/__init__.py +++ b/osf/email/__init__.py @@ -149,7 +149,7 @@ def _read_lookup_uri(uri: str) -> str: 'domain': settings.DOMAIN, } -def _render_email_html(notification_type, ctx: dict, return_original_error: bool = False) -> str: +def _render_email_html(notification_type, ctx: dict) -> str: template_text = notification_type.template if not template_text: return '' @@ -172,9 +172,7 @@ def _render_email_html(notification_type, ctx: dict, return_original_error: bool strict_undefined=True, ).render(**(ctx or {})) - except Exception as e: - if return_original_error: - raise e + except Exception: logging.exception( f'Mako render failed. type {notification_type.name} provided_keys=%s inline_uri=%s base_uri=%s lookup_dirs=%s', sorted((ctx or {}).keys()), uri, NOTIFY_BASE_URI, LOOKUP_DIRS, diff --git a/osf/management/commands/populate_notification_types.py b/osf/management/commands/populate_notification_types.py index 5f0145f08a9..302c1069a17 100644 --- a/osf/management/commands/populate_notification_types.py +++ b/osf/management/commands/populate_notification_types.py @@ -1,5 +1,6 @@ import sys import yaml +from django.apps import apps from waffle import switch_is_active from osf import features @@ -18,7 +19,7 @@ 'email_transactional': 'instantly', } -def populate_notification_types(*args, restore_one=None, restore_all=False, **kwargs): +def populate_notification_types(*args, **kwargs): if kwargs.get('sender'): # exists when called as a post_migrate signal if not switch_is_active(features.POPULATE_NOTIFICATION_TYPES): if 'pytest' not in sys.modules: @@ -27,89 +28,64 @@ def populate_notification_types(*args, restore_one=None, restore_all=False, **kw logger.info('Populating notification types...') from django.contrib.contenttypes.models import ContentType from osf.models.notification_type import NotificationType - try: with open(settings.NOTIFICATION_TYPES_YAML) as stream: notification_types = yaml.safe_load(stream) - - notification_types_dict = { - nt['name']: nt for nt in notification_types['notification_types'] - } - - all_names = set(notification_types_dict.keys()) - existing_names = set( - NotificationType.objects.values_list('name', flat=True) - ) - - if restore_one: - if restore_one not in notification_types_dict: - raise ValueError(f'Notification type "{restore_one}" not found in YAML') - names_to_process = {restore_one} - - elif restore_all: - names_to_process = all_names - - else: - names_to_process = all_names - existing_names - - logger.info(f'Processing {len(names_to_process)} notification types') - - for name in names_to_process: - raw_nt = notification_types_dict[name].copy() - - raw_nt.pop('__docs__', None) - raw_nt.pop('tests', None) - - object_content_type_model_name = raw_nt.pop('object_content_type_model_name') + for notification_type in notification_types['notification_types']: + notification_type.pop('__docs__', None) + notification_type.pop('tests', None) + object_content_type_model_name = notification_type.pop('object_content_type_model_name') if object_content_type_model_name == 'desk': content_type = None + elif object_content_type_model_name == 'osfuser': + OSFUser = apps.get_model('osf', 'OSFUser') + content_type = ContentType.objects.get_for_model(OSFUser) + elif object_content_type_model_name == 'preprint': + Preprint = apps.get_model('osf', 'Preprint') + content_type = ContentType.objects.get_for_model(Preprint) + elif object_content_type_model_name == 'collectionsubmission': + CollectionSubmission = apps.get_model('osf', 'CollectionSubmission') + content_type = ContentType.objects.get_for_model(CollectionSubmission) + elif object_content_type_model_name == 'abstractprovider': + AbstractProvider = apps.get_model('osf', 'abstractprovider') + content_type = ContentType.objects.get_for_model(AbstractProvider) + elif object_content_type_model_name == 'osfuser': + OSFUser = apps.get_model('osf', 'OSFUser') + content_type = ContentType.objects.get_for_model(OSFUser) + elif object_content_type_model_name == 'draftregistration': + DraftRegistration = apps.get_model('osf', 'DraftRegistration') + content_type = ContentType.objects.get_for_model(DraftRegistration) else: try: - content_type = ContentType.objects.get_by_natural_key(app_label='osf', model=object_content_type_model_name) + content_type = ContentType.objects.get( + app_label='osf', + model=object_content_type_model_name + ) except ContentType.DoesNotExist: raise ValueError(f'No content type for osf.{object_content_type_model_name}') - template_path = raw_nt.pop('template') - template = None - + template_path = notification_type.pop('template') if template_path: with open(template_path) as stream: template = stream.read() nt, _ = NotificationType.objects.update_or_create( - name=name, - defaults=raw_nt, + name=notification_type['name'], + defaults=notification_type, ) - nt.object_content_type = content_type - if template: + if not nt.template or settings.DEV_MODE: nt.template = template - nt.save() - except ProgrammingError: logger.info('Notification types failed potential side effect of reverse migration') logger.info('Finished populating notification types.') -class Command(BaseCommand): - help = 'Populate notification types.' - def add_arguments(self, parser): - parser.add_argument( - '--restore-all', - action='store_true', - help='Restore all templates from files' - ) - parser.add_argument( - '--restore', - type=str, - help='Restore specific template by name' - ) +class Command(BaseCommand): + help = 'Population notification types.' def handle(self, *args, **options): with transaction.atomic(): - populate_notification_types( - restore_all=options['restore_all'], - restore_one=options['restore'] - ) + populate_notification_types(args, options) diff --git a/osf/management/commands/process_manual_restart_approvals.py b/osf/management/commands/process_manual_restart_approvals.py index 3d01da98262..e708320357f 100644 --- a/osf/management/commands/process_manual_restart_approvals.py +++ b/osf/management/commands/process_manual_restart_approvals.py @@ -4,6 +4,7 @@ from django.utils import timezone from osf.models import Registration from osf.models.admin_log_entry import AdminLogEntry, MANUAL_ARCHIVE_RESTART +from website import settings from scripts.approve_registrations import approve_past_pendings logger = logging.getLogger(__name__) @@ -133,9 +134,9 @@ def should_auto_approve(self, registration): if approval.is_rejected: return 'approval was rejected' - auto_approval_time = approval.auto_approval_time - if timezone.now() < auto_approval_time: - remaining = auto_approval_time - timezone.now() + time_since_initiation = timezone.now() - approval.initiation_date + if time_since_initiation < settings.REGISTRATION_APPROVAL_TIME: + remaining = settings.REGISTRATION_APPROVAL_TIME - time_since_initiation return f'not ready yet ({remaining} remaining)' if registration.is_stuck_registration: diff --git a/osf/migrations/0040_maintenancemode.py b/osf/migrations/0040_maintenancemode.py deleted file mode 100644 index 9d7a410ae0a..00000000000 --- a/osf/migrations/0040_maintenancemode.py +++ /dev/null @@ -1,39 +0,0 @@ -# Generated by Django 4.2.26 on 2026-05-22 14:57 -# Manually added `create_initial_record()` - -from django.db import migrations, models - - -def create_initial_record(apps, schema_editor): - MaintenanceMode = apps.get_model('osf', 'MaintenanceMode') - MaintenanceMode.objects.get_or_create( - pk=1, - defaults={'maintenance_mode': False} - ) - - -def reverse_initial_record(apps, schema_editor): - # the reverse 'reverse_initial_record' does nothing - # because the table will be removed - pass - - -class Migration(migrations.Migration): - - dependencies = [ - ('osf', '0039_merge_20260427_1359'), - ] - - operations = [ - migrations.CreateModel( - name='MaintenanceMode', - fields=[ - ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('maintenance_mode', models.BooleanField(default=False)), - ], - ), - migrations.RunPython( - create_initial_record, - reverse_code=reverse_initial_record - ), - ] diff --git a/osf/migrations/0041_notification_scheduled.py b/osf/migrations/0041_notification_scheduled.py deleted file mode 100644 index c4e2b18b909..00000000000 --- a/osf/migrations/0041_notification_scheduled.py +++ /dev/null @@ -1,18 +0,0 @@ -# Generated by Django 4.2.26 on 2026-06-08 09:32 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('osf', '0040_maintenancemode'), - ] - - operations = [ - migrations.AddField( - model_name='notification', - name='scheduled', - field=models.DateTimeField(blank=True, null=True), - ), - ] diff --git a/osf/models/__init__.py b/osf/models/__init__.py index 918ca9aa009..7f334a357cc 100644 --- a/osf/models/__init__.py +++ b/osf/models/__init__.py @@ -52,7 +52,7 @@ from .institution_affiliation import InstitutionAffiliation from .institution_storage_region import InstitutionStorageRegion from .licenses import NodeLicense, NodeLicenseRecord -from .maintenance_state import MaintenanceState, MaintenanceMode +from .maintenance_state import MaintenanceState from .metadata import GuidMetadataRecord from .metaschema import ( FileMetadataSchema, diff --git a/osf/models/maintenance_state.py b/osf/models/maintenance_state.py index 134979b5f86..ce8a5ce1786 100644 --- a/osf/models/maintenance_state.py +++ b/osf/models/maintenance_state.py @@ -14,16 +14,3 @@ class MaintenanceState(models.Model): start = NonNaiveDateTimeField() end = NonNaiveDateTimeField() message = models.TextField(blank=True) - - -class MaintenanceMode(models.Model): - maintenance_mode = models.BooleanField(default=False) - - def save(self, *args, **kwargs): - self.pk = 1 - super().save(*args, **kwargs) - - @classmethod - def is_under_maintenance(cls): - obj, _ = cls.objects.get_or_create(pk=1) - return obj.maintenance_mode diff --git a/osf/models/mixins.py b/osf/models/mixins.py index d8c0b976565..692075ef4c6 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1796,7 +1796,7 @@ def copy_unclaimed_records(self, resource): contributor.save() # TODO: optimize me - def update_contributor(self, user, permission, visible, auth, save=False, skip_permission=False, log=True): + def update_contributor(self, user, permission, visible, auth, save=False, skip_permission=False): """ TODO: this method should be updated as a replacement for the main loop of Node#manage_contributors. Right now there are redundancies, but to avoid major feature creep this will not be included as this time. @@ -1826,15 +1826,14 @@ def update_contributor(self, user, permission, visible, auth, save=False, skip_p permissions_changed = { user._id: permission } - if log: - params = self.log_params - params['contributors'] = permissions_changed - self.add_log( - action=self.log_class.PERMISSIONS_UPDATED, - params=params, - auth=auth, - save=False - ) + params = self.log_params + params['contributors'] = permissions_changed + self.add_log( + action=self.log_class.PERMISSIONS_UPDATED, + params=params, + auth=auth, + save=False + ) with transaction.atomic(): if [READ] in permissions_changed.values(): project_signals.write_permissions_revoked.send(self) diff --git a/osf/models/nodelog.py b/osf/models/nodelog.py index c40390ed6d4..6aa14f461e5 100644 --- a/osf/models/nodelog.py +++ b/osf/models/nodelog.py @@ -16,7 +16,6 @@ class NodeLog(ObjectIDMixin, BaseModel): } DATE_FORMAT = '%m/%d/%Y %H:%M UTC' - SUPPORT_USER_LABEL = 'an OSF Support Team Member' # Log action constants -- NOTE: templates stored in log_templates.mako CREATED_FROM = 'created_from' diff --git a/osf/models/notification.py b/osf/models/notification.py index d3071442af6..533a05a4e97 100644 --- a/osf/models/notification.py +++ b/osf/models/notification.py @@ -5,7 +5,6 @@ from django.utils import timezone from api.base import settings as api_settings -from website import settings as osf_settings from osf import email, features @@ -19,10 +18,10 @@ class Notification(models.Model): sent = models.DateTimeField(null=True, blank=True) created = models.DateTimeField(auto_now_add=True) fake_sent = models.BooleanField(default=False) - scheduled = models.DateTimeField(null=True, blank=True) def send( self, + protocol_type='email', destination_address=None, email_context=None, save=True, @@ -37,28 +36,22 @@ def send( f"\ncontext={self.event_context}" f"\nemail_context={email_context}" ) - - if waffle.switch_is_active(features.ENABLE_MAILHOG): + if protocol_type == 'email' and waffle.switch_is_active(features.ENABLE_MAILHOG): email.send_email_over_smtp( recipient_address, self.subscription.notification_type, self.event_context, email_context ) - - if not osf_settings.LOCAL_MODE: + elif protocol_type == 'email': email.send_email_with_send_grid( recipient_address, self.subscription.notification_type, self.event_context, email_context ) - - if osf_settings.LOCAL_MODE and not waffle.switch_is_active(features.ENABLE_MAILHOG): - logging.warning( - 'Both ENABLE_MAILHOG and LOCAL_MODE are disabled. Emails will not be sent to MailHog or real email addresses. ' - 'Turn on ENABLE_MAILHOG to send emails to MailHog for testing, or turn on LOCAL_MODE to send emails with SendGrid.' - ) + else: + raise NotImplementedError(f'protocol `{protocol_type}` is not supported.') if save: self.mark_sent() diff --git a/osf/models/notification_type.py b/osf/models/notification_type.py index f8162a08bce..e0afdab7aea 100644 --- a/osf/models/notification_type.py +++ b/osf/models/notification_type.py @@ -160,7 +160,6 @@ def is_digest_type(self): NotificationTypeEnum.ADDON_FILE_COPIED.value, NotificationTypeEnum.ADDON_FILE_MOVED.value, NotificationTypeEnum.ADDON_FILE_RENAMED.value, - NotificationTypeEnum.ADDON_FILE_REMOVED.value, NotificationTypeEnum.FILE_ADDED.value, NotificationTypeEnum.FILE_REMOVED.value, NotificationTypeEnum.FILE_UPDATED.value, diff --git a/osf/models/registrations.py b/osf/models/registrations.py index c05cbc7eba9..568a1a575ed 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -237,7 +237,7 @@ def is_collection(self): @property def archive_job(self): - return self.archive_jobs.first() + return self.archive_jobs.first() if self.archive_jobs.count() else None @property def sanction(self): diff --git a/osf/models/sanctions.py b/osf/models/sanctions.py index 9268a140b3d..43dd8aece7b 100644 --- a/osf/models/sanctions.py +++ b/osf/models/sanctions.py @@ -878,10 +878,6 @@ class RegistrationApproval(SanctionCallbackMixin, EmailApprovableSanction): initiated_by = models.ForeignKey(settings.AUTH_USER_MODEL, null=True, blank=True, on_delete=models.CASCADE) - @property - def auto_approval_time(self): - return self.initiation_date + osf_settings.REGISTRATION_APPROVAL_TIME - @staticmethod def find_approval_backlog(): """ diff --git a/osf_tests/test_archiver.py b/osf_tests/test_archiver.py index 2ee659e00f5..94c8b4f2827 100644 --- a/osf_tests/test_archiver.py +++ b/osf_tests/test_archiver.py @@ -451,22 +451,6 @@ def test_archive(self, mock_chain, mock_enqueue): ] ) - @mock.patch('website.archiver.tasks.delayed_manual_restart_approval.delay') - @mock.patch('osf.management.commands.force_archive.archive') - @mock.patch('osf.management.commands.force_archive.verify') - def test_force_archive_schedules_manual_restart_approval_check( - self, mock_verify, mock_archive, mock_delayed_check - ): - result = force_archive( - registration_id=self.dst._id, - permissible_addons=['osfstorage'], - ) - - assert result == f'Registration {self.dst._id} archive completed' - mock_verify.assert_called_once() - mock_archive.assert_called_once() - mock_delayed_check.assert_called_once_with(self.dst._id, delay_minutes=5) - def test_stat_addon(self): with mock.patch.object(BaseStorageAddon, '_get_file_tree') as mock_file_tree: mock_file_tree.return_value = FILE_TREE diff --git a/osf_tests/test_middleware.py b/osf_tests/test_middleware.py deleted file mode 100644 index 32c16cabd41..00000000000 --- a/osf_tests/test_middleware.py +++ /dev/null @@ -1,105 +0,0 @@ -import pytest -from unittest import mock -from tests.base import ApiTestCase -from osf.utils import permissions -from osf_tests.factories import ( - AuthUserFactory, - CollectionProviderFactory, - ProjectFactory, -) - - -@pytest.fixture() -def provider(): - provider = CollectionProviderFactory() - provider.update_group_permissions() - return provider - - -@pytest.fixture() -def admin(provider): - user = AuthUserFactory() - provider.get_group(permissions.ADMIN).user_set.add(user) - return user - - -@pytest.fixture() -def node(admin): - return ProjectFactory(creator=admin) - - -class TestMaintenanceModeMiddlewareIntegration(ApiTestCase): - MAINTENANCE_MOCK_PATH = 'api.base.middleware.MaintenanceMode.is_under_maintenance' - - def setUp(self): - super().setUp() - self.provider = CollectionProviderFactory() - self.provider.update_group_permissions() - self.admin = AuthUserFactory() - self.provider.get_group(permissions.ADMIN).user_set.add(self.admin) - self.node = ProjectFactory(creator=self.admin) - - @mock.patch(MAINTENANCE_MOCK_PATH, return_value=True) - def test_middleware_blocks_post_if_maintenance_mode_on(self, mock_maintenance): - url = f'/v2/nodes/{self.node._id}/' - response = self.app.post_json(url, {}, expect_errors=True) - assert response.status_code == 503 - assert response.json['meta']['maintenance_mode'] is True - assert response.json['meta']['status_page'] == 'https://status.cos.io' - - @mock.patch(MAINTENANCE_MOCK_PATH, return_value=True) - def test_middleware_blocks_patch_if_maintenance_mode_on(self, mock_maintenance): - url = f'/v2/nodes/{self.node._id}/' - original_title = self.node.title - payload = { - 'data': { - 'id': self.node._id, - 'type': 'nodes', - 'attributes': {'title': 'Updated Title'} - } - } - response = self.app.patch_json(url, payload, expect_errors=True) - assert response.status_code == 503 - assert response.json['meta']['maintenance_mode'] is True - self.node.reload() - assert self.node.title == original_title - - @mock.patch(MAINTENANCE_MOCK_PATH, return_value=True) - def test_middleware_blocks_delete_if_maintenance_mode_on(self, mock_maintenance): - url = f'/v2/nodes/{self.node._id}/' - response = self.app.delete(url, expect_errors=True) - assert response.status_code == 503 - assert response.json['meta']['maintenance_mode'] is True - self.node.reload() - assert self.node.is_deleted is False - - @mock.patch(MAINTENANCE_MOCK_PATH, return_value=False) - def test_go_to_post_view_if_maintenance_mode_off(self, mock_maintenance): - url = '/v2/nodes/' - payload = { - 'data': { - 'type': 'nodes', - 'attributes': {'title': 'New Node', 'category': 'project'} - } - } - response = self.app.post_json(url, payload, auth=self.admin.auth) - assert response.status_code == 201 - - @mock.patch(MAINTENANCE_MOCK_PATH, return_value=False) - def test_go_to_patch_view_if_maintenance_mode_off(self, mock_maintenance): - url = f'/v2/nodes/{self.node._id}/' - payload = { - 'data': { - 'id': self.node._id, - 'type': 'nodes', - 'attributes': {'title': 'Updated Title'} - } - } - response = self.app.patch_json(url, payload, auth=self.admin.auth) - assert response.status_code == 200 - - @mock.patch(MAINTENANCE_MOCK_PATH, return_value=False) - def test_go_to_delete_view_if_maintenance_mode_off(self, mock_maintenance): - url = f'/v2/nodes/{self.node._id}/' - response = self.app.delete(url, auth=self.admin.auth) - assert response.status_code == 204 diff --git a/scripts/check_manual_restart_approval.py b/scripts/check_manual_restart_approval.py index 5dfcbdfa2eb..e9e6c70de0c 100644 --- a/scripts/check_manual_restart_approval.py +++ b/scripts/check_manual_restart_approval.py @@ -1,10 +1,7 @@ import logging -from framework import sentry from framework.celery_tasks import app as celery_app from django.core.management import call_command -from django.utils import timezone from osf.models import Registration -from scripts.approve_registrations import approve_past_pendings logger = logging.getLogger(__name__) @@ -12,45 +9,37 @@ @celery_app.task(name='scripts.check_manual_restart_approval') def check_manual_restart_approval(registration_id): try: - registration = Registration.load(registration_id) - if not registration: + try: + registration = Registration.objects.get(_id=registration_id) + except Registration.DoesNotExist: logger.error(f"Registration {registration_id} not found") return f"Registration {registration_id} not found" if registration.is_public or registration.is_registration_approved: return f"Registration {registration_id} already approved/public" - approval = registration.registration_approval - if not approval: - logger.error(f"Registration {registration_id} has no registration approval object") - return f"Registration {registration_id} has no registration approval object" - - if approval.is_rejected: - logger.info(f"Registration {registration_id} approval was rejected") - return f"Registration {registration_id} approval was rejected" - if registration.archiving: - logger.debug(f"Registration {registration_id} still archiving, retrying in 10 minutes") + logger.info(f"Registration {registration_id} still archiving, retrying in 10 minutes") check_manual_restart_approval.apply_async( args=[registration_id], countdown=600 ) return f"Registration {registration_id} still archiving, scheduled retry" - if timezone.now() < approval.auto_approval_time: - logger.info(f"Registration {registration_id} not ready for auto-approval yet") - return f"Registration {registration_id} not ready for auto-approval yet" + logger.info(f"Processing manual restart approval for registration {registration_id}") - logger.debug(f"Processing manual restart approval for registration {registration_id}") - approve_past_pendings([approval], dry_run=False) + call_command( + 'process_manual_restart_approvals', + registration_id=registration_id, + dry_run=False, + hours_back=24, + verbosity=1 + ) return f"Processed manual restart approval check for registration {registration_id}" except Exception as e: - msg = f"Error processing manual restart approval for {registration_id}: {str(e)}" - logger.error(msg) - sentry.log_message(msg) - sentry.log_exception(e) + logger.error(f"Error processing manual restart approval for {registration_id}: {e}") raise diff --git a/scripts/tests/test_check_manual_restart_approval.py b/scripts/tests/test_check_manual_restart_approval.py deleted file mode 100644 index f30c99c4616..00000000000 --- a/scripts/tests/test_check_manual_restart_approval.py +++ /dev/null @@ -1,47 +0,0 @@ -from datetime import timedelta -from unittest import mock - -from django.utils import timezone - -from osf_tests.factories import RegistrationFactory, UserFactory -from scripts.check_manual_restart_approval import check_manual_restart_approval -from tests.base import OsfTestCase -from website import settings - - -class TestCheckManualRestartApproval(OsfTestCase): - - def setUp(self): - super().setUp() - self.user = UserFactory() - self.registration = RegistrationFactory(creator=self.user, archive=False) - self.registration.require_approval(self.user) - - @mock.patch('scripts.check_manual_restart_approval.approve_past_pendings') - def test_skips_if_approval_window_not_elapsed(self, mock_approve): - self.registration.registration_approval.initiation_date = timezone.now() - timedelta(hours=47) - self.registration.registration_approval.save() - - result = check_manual_restart_approval(self.registration._id) - - assert 'not ready for auto-approval' in result - mock_approve.assert_not_called() - - @mock.patch('scripts.check_manual_restart_approval.approve_past_pendings') - def test_approves_when_approval_window_elapsed(self, mock_approve): - self.registration.registration_approval.initiation_date = ( - timezone.now() - settings.REGISTRATION_APPROVAL_TIME - timedelta(minutes=1) - ) - self.registration.registration_approval.save() - - result = check_manual_restart_approval(self.registration._id) - - assert 'Processed manual restart approval check' in result - mock_approve.assert_called_once_with([self.registration.registration_approval], dry_run=False) - - @mock.patch('scripts.check_manual_restart_approval.Registration.load', return_value=None) - def test_returns_not_found_when_registration_missing(self, mock_load): - result = check_manual_restart_approval('abc12') - - assert result == 'Registration abc12 not found' - mock_load.assert_called_once_with('abc12') diff --git a/website/archiver/tasks.py b/website/archiver/tasks.py index bc531c4d56d..a16055bafca 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -7,6 +7,9 @@ import celery from celery.utils.log import get_task_logger +from django.utils import timezone +from datetime import timedelta + from framework.celery_tasks import app as celery_app from framework.celery_tasks.utils import logged from framework.exceptions import HTTPError @@ -35,6 +38,7 @@ from website import settings from website.app import init_addons +from osf.models.admin_log_entry import AdminLogEntry, MANUAL_ARCHIVE_RESTART from osf.models import ( ArchiveJob, AbstractNode, @@ -441,9 +445,23 @@ def archive_success(self, dst_pk, job_pk): job.save() dst.sanction.ask(dst.get_active_contributors_recursive(unique_users=True)) + if was_manually_restarted(dst): + logger.info(f'Registration {dst._id} was manually restarted, scheduling approval check') + delayed_manual_restart_approval.delay(dst._id, delay_minutes=5) + dst.update_search() +def was_manually_restarted(registration): + recent_logs = AdminLogEntry.objects.filter( + object_id=registration.pk, + action_flag=MANUAL_ARCHIVE_RESTART, + action_time__gte=timezone.now() - timedelta(hours=48) + ) + + return recent_logs.exists() + + @celery_app.task(bind=True) def force_archive(self, registration_id, permissible_addons, allow_unconfigured=False, skip_collisions=False, delete_collisions=False): from osf.management.commands.force_archive import archive, verify @@ -464,10 +482,6 @@ def force_archive(self, registration_id, permissible_addons, allow_unconfigured= skip_collisions=skip_collisions, delete_collisions=delete_collisions, ) - - logger.info(f'Registration {registration._id} was manually restarted, scheduling approval check') - delayed_manual_restart_approval.delay(registration._id, delay_minutes=5) - return f'Registration {registration_id} archive completed' except Exception as exc: diff --git a/website/templates/pending_registration_admin.html.mako b/website/templates/pending_registration_admin.html.mako index 694a670c5e2..ebf11fb1cd8 100644 --- a/website/templates/pending_registration_admin.html.mako +++ b/website/templates/pending_registration_admin.html.mako @@ -32,7 +32,7 @@ To cancel this registration: Click here.

    - % if reviewable_provider__id != 'gfs': + % if not reviewable_provider__id != 'gfs': Note: If any admin clicks their cancel link, the submission will be canceled immediately, and the pending registration will be reverted to draft state to revise and resubmit. This operation is irreversible. % else: From 3aa1b31403f04ccfd93025cc8a143cfbacf38fa3 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Mon, 29 Jun 2026 17:21:17 +0300 Subject: [PATCH 3/9] [ENG-10365] Make waterbutled callback after addons archiving is finished (#11787) ## Ticket https://openscience.atlassian.net/browse/ENG-10365 ## Changes 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 94c8b4f2827..4d6c0ac5be5 100644 --- a/osf_tests/test_archiver.py +++ b/osf_tests/test_archiver.py @@ -526,7 +526,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: @@ -545,8 +545,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() @@ -566,7 +566,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) @@ -580,23 +580,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): @@ -993,7 +1034,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 @@ -1001,7 +1042,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): @@ -1015,13 +1056,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 @@ -1037,7 +1078,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): @@ -1105,13 +1146,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): @@ -1178,7 +1219,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 @@ -1192,7 +1233,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): @@ -1200,7 +1241,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 a16055bafca..fad30d6e98b 100644 --- a/website/archiver/tasks.py +++ b/website/archiver/tasks.py @@ -10,7 +10,7 @@ from django.utils import timezone from datetime import timedelta -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 @@ -30,11 +30,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 @@ -254,22 +254,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): @@ -324,7 +337,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, @@ -361,15 +378,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): @@ -488,3 +528,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 cb10c394c3c998ea75304e1f5ee3695b76b18a2b Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Mon, 29 Jun 2026 17:22:55 +0300 Subject: [PATCH 4/9] [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 2c29b1af2c6..1c5f7e59abb 100644 --- a/admin_tests/nodes/test_views.py +++ b/admin_tests/nodes/test_views.py @@ -52,6 +52,7 @@ from tests.base import AdminTestCase from osf_tests.factories import ( + SubjectFactory, UserFactory, AuthUserFactory, ProjectFactory, @@ -619,6 +620,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) @@ -632,6 +634,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) @@ -644,6 +647,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) @@ -843,6 +847,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 @@ -863,6 +868,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 @@ -891,6 +897,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 692075ef4c6..2f741f147bf 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 568a1a575ed..5cf311b94bf 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: From 4c6cf2746f8514cb1f2a418315397c3380ee2814 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Tue, 30 Jun 2026 17:32:46 +0300 Subject: [PATCH 5/9] fix REGEX --- osf/external/spam/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osf/external/spam/tasks.py b/osf/external/spam/tasks.py index d67b21912dc..97cf230d044 100644 --- a/osf/external/spam/tasks.py +++ b/osf/external/spam/tasks.py @@ -15,7 +15,7 @@ logging.basicConfig(level=logging.INFO) -DOMAIN_REGEX = re.compile(r'\W*(?P\w+://)?(?Pwww\.)?(?P([\w-]+\.)+[a-zA-Z]+)(?P[/\-\.\w]*)?\W*') +DOMAIN_REGEX = re.compile(r'(?:'r'(?P\w+://)'r'|'r'(?Pwww\.)'r')'r'(?P([\w-]+\.)*[\w-]+)'r'(?P[/\-\.\w]*)?', re.IGNORECASE) REDIRECT_CODES = {301, 302, 303, 307, 308} From e83eace298729b33792f2760588584816fac9431 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Thu, 2 Jul 2026 16:43:06 +0300 Subject: [PATCH 6/9] revert REGEX, add tests, improve validator --- osf/external/spam/tasks.py | 2 +- osf/models/validators.py | 13 +++- osf_tests/test_validators.py | 117 ++++++++++++++++++++++++++++++++++- 3 files changed, 129 insertions(+), 3 deletions(-) diff --git a/osf/external/spam/tasks.py b/osf/external/spam/tasks.py index 97cf230d044..d67b21912dc 100644 --- a/osf/external/spam/tasks.py +++ b/osf/external/spam/tasks.py @@ -15,7 +15,7 @@ logging.basicConfig(level=logging.INFO) -DOMAIN_REGEX = re.compile(r'(?:'r'(?P\w+://)'r'|'r'(?Pwww\.)'r')'r'(?P([\w-]+\.)*[\w-]+)'r'(?P[/\-\.\w]*)?', re.IGNORECASE) +DOMAIN_REGEX = re.compile(r'\W*(?P\w+://)?(?Pwww\.)?(?P([\w-]+\.)+[a-zA-Z]+)(?P[/\-\.\w]*)?\W*') REDIRECT_CODES = {301, 302, 303, 307, 308} diff --git a/osf/models/validators.py b/osf/models/validators.py index c5acafd3ea7..ce0401a0d32 100644 --- a/osf/models/validators.py +++ b/osf/models/validators.py @@ -114,7 +114,18 @@ def validate_email(value): def has_domain_in_user_fields_for_names(user): - name_content = ' '.join([getattr(user, field) or '' for field in user.DOMAIN_VALIDATION_FIELDS]) + name_content = ' '.join(getattr(user, field) or '' for field in user.DOMAIN_VALIDATION_FIELDS).strip() + + if not name_content: + return False + + text = name_content.lower() + if any(suffix in text for suffix in ['m.sc.', 'msc.', 'phd.', 'ph.d.', 'msc.pt', 'pt.', 'prof.', 'dr.','md.', 'jd.', 'esq.']): + return False + + if re.search(r'\b[a-z]\.[a-z0-9-]+\b', text): + return False + return True if DOMAIN_REGEX.search(name_content) else False diff --git a/osf_tests/test_validators.py b/osf_tests/test_validators.py index dc561389356..5359e9da204 100644 --- a/osf_tests/test_validators.py +++ b/osf_tests/test_validators.py @@ -2,10 +2,17 @@ from osf.exceptions import ValidationValueError from osf.models import validators -from osf_tests.factories import SubjectFactory +from osf.models.validators import has_domain_in_user_fields_for_names +from osf_tests.factories import SubjectFactory, AuthUserFactory + # Ported from tests/framework/test_mongo.py +@pytest.fixture() +def user(user_data): + return AuthUserFactory(**user_data) + + def test_string_required_passes_with_string(): assert validators.string_required('Hi!') is True @@ -50,3 +57,111 @@ def test_validate_expand_subject_hierarchy(): subject_list = [fruit._id, '12345_bad_id'] with pytest.raises(ValidationValueError): validators.expand_subject_hierarchy(subject_list) + + +@pytest.mark.parametrize( + 'user_data', + [ + { + 'fullname': 'Judith Sarah Preuss, M.Sc.', + 'given_name': 'Judith', + 'family_name': 'Preuss', + 'middle_names': 'Sarah', + 'suffix': 'M.Sc.', + }, + { + 'fullname': 'J.H. van Hateren', + 'given_name': 'J.H.', + 'family_name': 'van Hateren', + 'middle_names': '', + }, + { + 'fullname': 'Sami-Egil Ahonen', + 'given_name': 'Sami-Egil', + 'family_name': 'Ahonen', + 'middle_names': '', + }, + { + 'fullname': 'Giovanni Luca Ciampaglia', + 'given_name': 'Giovanni Luca', + 'family_name': 'Ciampaglia', + 'middle_names': '', + }, + { + 'fullname': 'Joseph P.R.O. Orgel', + 'given_name': 'Joseph', + 'family_name': 'Orgel', + 'middle_names': 'P.R.O.', + }, + { + 'fullname': 'Andrew Daoust', + 'given_name': 'Andrew', + 'family_name': 'Daoust', + 'middle_names': '', + }, + { + 'fullname': 'Aidan G.C. Wright', + 'given_name': 'Aidan', + 'family_name': 'Wright', + 'middle_names': 'G.C.', + }, + { + 'fullname': 'Guillermo Perez Algorta', + 'given_name': 'Guillermo', + 'family_name': 'Perez Algorta', + 'middle_names': '', + }, + { + 'fullname': 'Sarah Wojkowski, MSc.PT, PhD.', + 'given_name': 'Sarah', + 'family_name': 'Wojkowski', + 'middle_names': 'MSc.PT', + }, + { + 'fullname': 'Brockmann, L.C. (Leon)', + 'given_name': 'Leon', + 'family_name': 'Brockmann', + 'middle_names': 'L.C.', + }, + { + 'fullname': 'Gragnolati, G.M. (Gaia Mariavittoria)', + 'given_name': 'Gaia', + 'family_name': 'Gragnolati', + 'middle_names': 'G.M.', + }, + { + 'fullname': 'F.H. Leeuwis', + 'given_name': 'F.H.', + 'family_name': 'Leeuwis', + 'middle_names': '', + }, + { + 'fullname': 'Grauss, S.E. (Sophie)', + 'given_name': 'Sophie', + 'family_name': 'Grauss', + 'middle_names': 'S.E.', + }, + { + 'fullname': 'Sandhya N.Sathesh', + 'given_name': 'Sandhya', + 'family_name': 'N.Sathesh', + 'middle_names': '', + }, + { + 'fullname': 'John Doe', + 'given_name': 'John', + 'family_name': 'Doe', + 'middle_names': '', + } + ] +) +def test_has_domain_in_user_fields_for_names_returns_false(user, user_data): + user.DOMAIN_VALIDATION_FIELDS = [ + 'fullname', + 'given_name', + 'middle_names', + 'family_name', + 'suffix', + ] + + assert has_domain_in_user_fields_for_names(user) is False \ No newline at end of file From 6a3ea8ef23998e7791bd199b42122ee0bb704ce4 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Thu, 2 Jul 2026 17:15:28 +0300 Subject: [PATCH 7/9] Flake8 --- osf/models/validators.py | 2 +- osf_tests/test_validators.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/osf/models/validators.py b/osf/models/validators.py index ce0401a0d32..787e511c8ad 100644 --- a/osf/models/validators.py +++ b/osf/models/validators.py @@ -120,7 +120,7 @@ def has_domain_in_user_fields_for_names(user): return False text = name_content.lower() - if any(suffix in text for suffix in ['m.sc.', 'msc.', 'phd.', 'ph.d.', 'msc.pt', 'pt.', 'prof.', 'dr.','md.', 'jd.', 'esq.']): + if any(suffix in text for suffix in ['m.sc.', 'msc.', 'phd.', 'ph.d.', 'msc.pt', 'pt.', 'prof.', 'dr.', 'md.', 'jd.', 'esq.']): return False if re.search(r'\b[a-z]\.[a-z0-9-]+\b', text): diff --git a/osf_tests/test_validators.py b/osf_tests/test_validators.py index 5359e9da204..5d428607244 100644 --- a/osf_tests/test_validators.py +++ b/osf_tests/test_validators.py @@ -164,4 +164,4 @@ def test_has_domain_in_user_fields_for_names_returns_false(user, user_data): 'suffix', ] - assert has_domain_in_user_fields_for_names(user) is False \ No newline at end of file + assert has_domain_in_user_fields_for_names(user) is False From 7027d210dfdce2cca697fc67627c36ba9e5b9ac0 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Fri, 3 Jul 2026 18:08:14 +0300 Subject: [PATCH 8/9] revert regex change, add tests, add urlsplit --- osf/models/user.py | 8 ++++-- osf/models/validators.py | 25 ++++++++-------- osf_tests/test_validators.py | 56 ++++++++++++++++++++++++++---------- 3 files changed, 59 insertions(+), 30 deletions(-) diff --git a/osf/models/user.py b/osf/models/user.py index 5739813cd48..e637e52a395 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1068,7 +1068,10 @@ def save(self, *args, **kwargs): dirty_fields = self.get_dirty_fields(check_relationship=True) ret = super().save(*args, **kwargs) # must save BEFORE spam check, as user needs guid. - if has_domain and not was_creating: + if has_domain and self.is_ham: + self.flag_spam() + + if has_domain and not was_creating and not self.is_ham: self.confirm_spam() if set(self.SPAM_USER_PROFILE_FIELDS.keys()).intersection(dirty_fields): @@ -1482,7 +1485,8 @@ def confirm_email(self, token, merge=False): return True def confirm_spam(self, domains=None, save=True, train_spam_services=False, skip_resources_spam=False): - self.deactivate_account() + if not self.is_ham: + self.deactivate_account() super().confirm_spam(domains=domains, save=save, train_spam_services=train_spam_services) if skip_resources_spam: diff --git a/osf/models/validators.py b/osf/models/validators.py index 787e511c8ad..7fe745685b8 100644 --- a/osf/models/validators.py +++ b/osf/models/validators.py @@ -1,4 +1,5 @@ import re +from urllib.parse import urlsplit import waffle from jsonschema import ValidationError as JsonSchemaValidationError, SchemaError, Draft7Validator, validate, validators @@ -115,19 +116,17 @@ def validate_email(value): def has_domain_in_user_fields_for_names(user): name_content = ' '.join(getattr(user, field) or '' for field in user.DOMAIN_VALIDATION_FIELDS).strip() - - if not name_content: - return False - - text = name_content.lower() - if any(suffix in text for suffix in ['m.sc.', 'msc.', 'phd.', 'ph.d.', 'msc.pt', 'pt.', 'prof.', 'dr.', 'md.', 'jd.', 'esq.']): - return False - - if re.search(r'\b[a-z]\.[a-z0-9-]+\b', text): - return False - - return True if DOMAIN_REGEX.search(name_content) else False - + for match in DOMAIN_REGEX.finditer(name_content): + candidate = match.group(0).strip() + if looks_like_url(candidate): + return True + return False + +def looks_like_url(candidate): + if candidate.startswith('www.'): + candidate = f'https://{candidate}' + parsed = urlsplit(candidate) + return bool(parsed.scheme and parsed.netloc) def validate_subject_hierarchy_length(parent): from osf.models import Subject diff --git a/osf_tests/test_validators.py b/osf_tests/test_validators.py index 5d428607244..6d02aaa38d3 100644 --- a/osf_tests/test_validators.py +++ b/osf_tests/test_validators.py @@ -1,17 +1,20 @@ import pytest from osf.exceptions import ValidationValueError -from osf.models import validators +from types import SimpleNamespace +from osf.models import validators, OSFUser from osf.models.validators import has_domain_in_user_fields_for_names -from osf_tests.factories import SubjectFactory, AuthUserFactory +from osf_tests.factories import SubjectFactory # Ported from tests/framework/test_mongo.py -@pytest.fixture() -def user(user_data): - return AuthUserFactory(**user_data) - +def mock_user(user_data): + user = SimpleNamespace() + user.DOMAIN_VALIDATION_FIELDS = OSFUser.DOMAIN_VALIDATION_FIELDS + for field in user.DOMAIN_VALIDATION_FIELDS: + setattr(user, field, user_data.get(field, '')) + return user def test_string_required_passes_with_string(): assert validators.string_required('Hi!') is True @@ -155,13 +158,36 @@ def test_validate_expand_subject_hierarchy(): } ] ) -def test_has_domain_in_user_fields_for_names_returns_false(user, user_data): - user.DOMAIN_VALIDATION_FIELDS = [ - 'fullname', - 'given_name', - 'middle_names', - 'family_name', - 'suffix', - ] - +def test_has_domain_in_user_fields(user_data): + user = mock_user(user_data) assert has_domain_in_user_fields_for_names(user) is False + + +@pytest.mark.parametrize( + 'user_data', + [ + { + 'fullname': 'Judith Sarah', + 'given_name': 'Judith', + 'family_name': 'Preuss', + 'middle_names': 'https://www.google.com', + 'suffix': 'M.Sc.', + }, + { + 'fullname': 'J.H. van Hateren', + 'given_name': 'J.H.', + 'family_name': 'https://google.com', + 'middle_names': '', + }, + { + 'fullname': 'Judith Sarah', + 'given_name': 'Judith', + 'family_name': 'Preuss', + 'middle_names': 'www.google.com', + 'suffix': 'M.Sc.', + }, + ] +) +def test_has_domain_in_user_fields_fail(user_data): + user = mock_user(user_data) + assert has_domain_in_user_fields_for_names(user) is True From f825b0debe412abd2f0eed8fd48d07f350a37b80 Mon Sep 17 00:00:00 2001 From: Bohdan Odintsov Date: Fri, 3 Jul 2026 18:40:29 +0300 Subject: [PATCH 9/9] to hammy --- osf/models/user.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osf/models/user.py b/osf/models/user.py index e637e52a395..4f69a30724e 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1068,10 +1068,10 @@ def save(self, *args, **kwargs): dirty_fields = self.get_dirty_fields(check_relationship=True) ret = super().save(*args, **kwargs) # must save BEFORE spam check, as user needs guid. - if has_domain and self.is_ham: + if has_domain and self.is_hammy: self.flag_spam() - if has_domain and not was_creating and not self.is_ham: + if has_domain and not was_creating and not self.is_hammy: self.confirm_spam() if set(self.SPAM_USER_PROFILE_FIELDS.keys()).intersection(dirty_fields): @@ -1485,7 +1485,7 @@ def confirm_email(self, token, merge=False): return True def confirm_spam(self, domains=None, save=True, train_spam_services=False, skip_resources_spam=False): - if not self.is_ham: + if not self.is_hammy: self.deactivate_account() super().confirm_spam(domains=domains, save=save, train_spam_services=train_spam_services)