From 94be0088be00195a7b68f614e8646f69e540a732 Mon Sep 17 00:00:00 2001 From: mkovalua Date: Mon, 12 Oct 2026 16:05:33 +0300 Subject: [PATCH 01/22] [ENG-12332] BE: Ability for moderator to report users as spam (#11958) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Ticket https://openscience.atlassian.net/browse/ENG-12332 ## Purpose Recently users have started started creating “fake” accounts with ORCIDs in order to submit AI generated content to preprint providers. This is “spam” that our providers are then required to report the users (by email) to OSF support staff. A simpler solution would be to have a report spam functionality in the moderator dashboard. This would generate an email notification to [support@cos.io](mailto:support@cos.io) regarding the preprint in question, and would allow support team members to quickly go into admin and disable to creator account. Note: This is for registries and preprints ## Changes add BE option to send email if preprint or registration submission is spam related FE ticket https://openscience.atlassian.net/browse/ENG-10679 --- admin/preprints/views.py | 8 +- admin_tests/preprints/test_views.py | 32 ++++++- api/actions/permissions.py | 1 + api/actions/serializers.py | 18 +++- api_tests/actions/views/test_action_list.py | 96 +++++++++++++++++++ .../preprints/views/test_preprint_actions.py | 67 +++++++++++++ .../registries_moderation/test_submissions.py | 70 +++++++++++++- notifications.yaml | 7 ++ osf/migrations/0057_report_spam_trigger.py | 23 +++++ osf/models/mixins.py | 12 +++ osf/models/notification_type.py | 1 + osf/models/registrations.py | 36 ++++++- osf/utils/machines.py | 3 + osf/utils/notifications.py | 32 ++++++- osf/utils/workflows.py | 17 +++- website/settings/defaults.py | 2 + .../templates/moderator_spam_report.html.mako | 35 +++++++ 17 files changed, 449 insertions(+), 11 deletions(-) create mode 100644 osf/migrations/0057_report_spam_trigger.py create mode 100644 website/templates/moderator_spam_report.html.mako diff --git a/admin/preprints/views.py b/admin/preprints/views.py index 72c30675021..007b8d4863b 100644 --- a/admin/preprints/views.py +++ b/admin/preprints/views.py @@ -51,7 +51,7 @@ REJECT_WITHDRAWAL, UNFLAG_SPAM, ) -from osf.utils.workflows import DefaultStates +from osf.utils.workflows import DefaultStates, ReviewTriggers from osf.utils.permissions import API_CONTRIBUTOR_PERMISSIONS from website.search import search from website.files.utils import copy_files @@ -844,8 +844,10 @@ def post(self, request, *args, **kwargs): messages.error(request, f'Preprint {preprint._id} is not withdrawn') return redirect(self.get_success_url()) - withdraw_action = preprint.actions.filter(to_state='withdrawn').last() - last_action = preprint.actions.last() + # A spam report is logged with to_state='withdrawn' too, so state alone can't tell it from + # the withdrawal. Match the withdrawal by trigger + withdraw_action = preprint.actions.filter(trigger=ReviewTriggers.WITHDRAW.value).last() + last_action = preprint.actions.exclude(trigger=ReviewTriggers.REPORT_SPAM.value).last() preprint.withdrawal_justification = '' preprint.date_withdrawn = None diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index baea8f278c1..6aed85d9791 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -22,7 +22,7 @@ ) from osf.models.admin_log_entry import AdminLogEntry, PREPRINT_RECOVERED, PREPRINT_RESTORED from osf.models.spam import SpamStatus -from osf.utils.workflows import DefaultStates, RequestTypes +from osf.utils.workflows import DefaultStates, RequestTypes, ReviewTriggers from osf.utils.permissions import ADMIN from admin_tests.utilities import setup_view, setup_log_view, handle_post_view_request @@ -758,6 +758,36 @@ def test_can_unwithdraw_preprint_in_pre_moderation(self, withdrawal_request, sub preprint.refresh_from_db() assert preprint.machine_state == DefaultStates.ACCEPTED.value + def test_can_unwithdraw_preprint_reported_as_spam(self, submitter, admin): + provider = PreprintProviderFactory(reviews_workflow='pre-moderation') + preprint = PreprintFactory(project=NodeFactory(creator=submitter), provider=provider) + + withdrawal_request = PreprintRequestFactory( + creator=admin, + target=preprint, + request_type=RequestTypes.WITHDRAWAL.value, + machine_state=DefaultStates.INITIAL.value) + withdrawal_request.run_submit(admin) + with capture_notifications(): + withdrawal_request.run_accept(admin, withdrawal_request.comment) + assert preprint.machine_state == 'withdrawn' + + moderator = AuthUserFactory() + moderator.groups.add(provider.get_group('moderator')) + with capture_notifications(): + preprint.run_report_spam(user=moderator, comment='Spam account') + + request_unwithdraw = RequestFactory().post(reverse('preprints:unwithdraw', kwargs={'guid': preprint._id})) + request_unwithdraw.user = admin + response_unwithdraw = views.PreprintUnwithdrawView.as_view()(request_unwithdraw, guid=preprint._id) + assert response_unwithdraw.status_code == 302 + + preprint.refresh_from_db() + assert preprint.machine_state == DefaultStates.ACCEPTED.value + # the withdrawal is gone, the report is still there + assert preprint.actions.filter(trigger=ReviewTriggers.REPORT_SPAM.value).count() == 1 + assert not preprint.actions.filter(trigger=ReviewTriggers.WITHDRAW.value).exists() + def test_permissions_errors(self, user, submitter): # with auth, no permissions request = RequestFactory().get(reverse('preprints:withdrawal-requests')) diff --git a/api/actions/permissions.py b/api/actions/permissions.py index 9ee74565471..d56ea0d714f 100644 --- a/api/actions/permissions.py +++ b/api/actions/permissions.py @@ -14,6 +14,7 @@ ReviewTriggers.REJECT.value: 'reject_submissions', ReviewTriggers.WITHDRAW.value: 'withdraw_submissions', ReviewTriggers.EDIT_COMMENT.value: 'edit_review_comments', + ReviewTriggers.REPORT_SPAM.value: 'view_submissions', } diff --git a/api/actions/serializers.py b/api/actions/serializers.py index d572ee41654..144dbc42ef7 100644 --- a/api/actions/serializers.py +++ b/api/actions/serializers.py @@ -211,13 +211,15 @@ class Meta: def create(self, validated_data): trigger = validated_data.get('trigger') - if trigger != ReviewTriggers.WITHDRAW.value: + if trigger not in (ReviewTriggers.WITHDRAW.value, ReviewTriggers.REPORT_SPAM.value): return super().create(validated_data) user = validated_data.pop('user') target = validated_data.pop('target') comment = validated_data.pop('comment', '') try: - return target.run_withdraw(user=user, comment=comment) + if trigger == ReviewTriggers.WITHDRAW.value: + return target.run_withdraw(user=user, comment=comment) + return target.run_report_spam(user=user, comment=comment) except InvalidTriggerError as e: # Invalid transition from the current state raise Conflict(str(e)) @@ -278,6 +280,18 @@ def create(self, validated_data): comment = validated_data.pop('comment', '') user = validated_data.pop('user') + if trigger == RegistrationModerationTriggers.REPORT_SPAM.db_name: + # moderators only, unlike the rest of the triggers + if not user.has_perm('view_submissions', target.provider): + raise PermissionDenied('You do not have permission to report this registration as spam.') + try: + return target.report_spam_to_support(user=user, comment=comment) + except InvalidTriggerError: + raise Conflict( + 'Only submitted registrations can be reported as spam. ' + f'This one is in the "{target.moderation_state}" state.', + ) + sanction = target.sanction try: diff --git a/api_tests/actions/views/test_action_list.py b/api_tests/actions/views/test_action_list.py index fc5de8a1d02..46e77d2fc16 100644 --- a/api_tests/actions/views/test_action_list.py +++ b/api_tests/actions/views/test_action_list.py @@ -9,6 +9,7 @@ ) from osf.utils import permissions as osf_permissions from tests.utils import capture_notifications +from website.settings import OSF_ADMIN_URL, OSF_SUPPORT_EMAIL @pytest.mark.django_db @@ -409,3 +410,98 @@ def test_provider_not_reviewed_returns_409(self, app, url, preprint, node_admin) expect_errors=True ) assert res.status_code == 409 + + def test_report_spam_permissions_unauthorized(self, app, url, preprint): + preprint.machine_state = 'pending' + preprint.save() + res = app.post_json_api( + url, + self.create_payload(preprint._id, trigger='report_spam'), + expect_errors=True + ) + assert res.status_code == 401 + + def test_report_spam_permissions_forbidden(self, app, url, preprint, node_admin): + preprint.machine_state = 'pending' + preprint.save() + report_payload = self.create_payload(preprint._id, trigger='report_spam') + # A random user can't report spam + res = app.post_json_api( + url, + report_payload, + auth=AuthUserFactory().auth, + expect_errors=True + ) + assert res.status_code == 403 + # Neither can a moderator for a different provider + another_moderator = AuthUserFactory() + another_moderator.groups.add( + PreprintProviderFactory().get_group('moderator') + ) + res = app.post_json_api( + url, + report_payload, + auth=another_moderator.auth, + expect_errors=True + ) + assert res.status_code == 403 + res = app.post_json_api( + url, + report_payload, + auth=node_admin.auth, + expect_errors=True + ) + assert res.status_code == 403 + + def test_report_spam_notifies_support(self, app, url, preprint, moderator): + preprint.machine_state = 'pending' + preprint.save() + report_payload = self.create_payload( + preprint._id, + trigger='report_spam', + comment='This is spam.' + ) + with capture_notifications() as notifications: + res = app.post_json_api(url, report_payload, auth=moderator.auth) + assert res.status_code == 201 + assert res.json['data']['attributes']['trigger'] == 'report_spam' + assert res.json['data']['attributes']['from_state'] == 'pending' + assert res.json['data']['attributes']['to_state'] == 'pending' + assert len(notifications['emits']) == 1 + emit = notifications['emits'][0] + assert emit['type'] == NotificationTypeEnum.DESK_MODERATOR_SPAM_REPORT + assert emit['kwargs']['destination_address'] == OSF_SUPPORT_EMAIL + assert emit['kwargs']['event_context']['moderator__id'] == moderator._id + assert emit['kwargs']['event_context']['resource__id'] == preprint._id + assert emit['kwargs']['event_context']['comment'] == 'This is spam.' + context = emit['kwargs']['event_context'] + assert context['resource_creator__id'] == preprint.creator._id + admin_app = OSF_ADMIN_URL.rstrip('/') + assert context['creator_admin_app_url'] == (f'{admin_app}/users/{preprint.creator._id}/' if admin_app else '') + assert context['resource_admin_app_url'] == (f'{admin_app}/preprints/{preprint._id}/' if admin_app else '') + # report logged, preprint untouched + preprint.refresh_from_db() + assert preprint.machine_state == 'pending' + assert preprint.spam_status is None + assert preprint.actions.filter(trigger='report_spam').count() == 1 + + def test_report_spam_invalid_state_returns_409(self, app, url, preprint, moderator): + assert preprint.machine_state == 'initial' + res = app.post_json_api( + url, + self.create_payload(preprint._id, trigger='report_spam'), + auth=moderator.auth, + expect_errors=True + ) + assert res.status_code == 409 + assert not preprint.actions.filter(trigger='report_spam').exists() + + def test_report_spam_is_repeatable(self, app, url, preprint, moderator): + preprint.machine_state = 'pending' + preprint.save() + report_payload = self.create_payload(preprint._id, trigger='report_spam') + with capture_notifications() as notifications: + assert app.post_json_api(url, report_payload, auth=moderator.auth).status_code == 201 + assert app.post_json_api(url, report_payload, auth=moderator.auth).status_code == 201 + assert len(notifications['emits']) == 2 + assert preprint.actions.filter(trigger='report_spam').count() == 2 diff --git a/api_tests/preprints/views/test_preprint_actions.py b/api_tests/preprints/views/test_preprint_actions.py index 6df0255df0c..15d73a0a759 100644 --- a/api_tests/preprints/views/test_preprint_actions.py +++ b/api_tests/preprints/views/test_preprint_actions.py @@ -1,10 +1,14 @@ import pytest from api.base.settings.defaults import API_BASE +from osf.models import NotificationTypeEnum from osf_tests.factories import ( AuthUserFactory, + PreprintFactory, + PreprintProviderFactory, ) from osf.utils import permissions as osf_permissions +from tests.utils import capture_notifications from api_tests.reviews.mixins.filter_mixins import ReviewActionFilterMixin from api_tests.reviews.mixins.comment_settings import ReviewActionCommentSettingsMixin @@ -48,3 +52,66 @@ class TestReviewActionSettings(ReviewActionCommentSettingsMixin): @pytest.fixture() def url(self, preprint): return f'/{API_BASE}preprints/{preprint._id}/review_actions/' + + +@pytest.mark.django_db +class TestPreprintActionReportSpam: + + @pytest.fixture() + def provider(self): + return PreprintProviderFactory(reviews_workflow='pre-moderation') + + @pytest.fixture() + def preprint(self, provider): + preprint = PreprintFactory(provider=provider, is_published=False) + preprint.machine_state = 'pending' + preprint.save() + return preprint + + @pytest.fixture() + def moderator(self, provider): + moderator = AuthUserFactory() + moderator.groups.add(provider.get_group('moderator')) + return moderator + + @pytest.fixture() + def url(self, preprint): + return f'/{API_BASE}preprints/{preprint._id}/review_actions/' + + @pytest.fixture() + def payload(self, preprint): + return { + 'data': { + 'type': 'actions', + 'attributes': { + 'trigger': 'report_spam', + 'comment': 'This is spam.' + }, + 'relationships': { + 'target': { + 'data': { + 'type': 'preprints', + 'id': preprint._id + } + } + } + } + } + + def test_moderator_can_report_spam(self, app, url, payload, preprint, moderator): + with capture_notifications() as notifications: + res = app.post_json_api(url, payload, auth=moderator.auth) + assert res.status_code == 201 + assert res.json['data']['attributes']['trigger'] == 'report_spam' + assert len(notifications['emits']) == 1 + assert notifications['emits'][0]['type'] == NotificationTypeEnum.DESK_MODERATOR_SPAM_REPORT + preprint.refresh_from_db() + assert preprint.actions.filter(trigger='report_spam').exists() + assert preprint.machine_state == 'pending' + + def test_non_moderator_cannot_report_spam(self, app, url, payload, preprint): + node_admin = AuthUserFactory() + preprint.add_contributor(node_admin, permissions=osf_permissions.ADMIN) + res = app.post_json_api(url, payload, auth=node_admin.auth, expect_errors=True) + assert res.status_code == 403 + assert not preprint.actions.filter(trigger='report_spam').exists() diff --git a/api_tests/registries_moderation/test_submissions.py b/api_tests/registries_moderation/test_submissions.py index cc44cbae4b6..19da26b9046 100644 --- a/api_tests/registries_moderation/test_submissions.py +++ b/api_tests/registries_moderation/test_submissions.py @@ -20,11 +20,12 @@ from tests.base import get_default_metaschema -from osf.models import NodeRequest +from osf.models import NodeRequest, NotificationTypeEnum from django.contrib.auth.models import Group from osf.migrations import update_provider_auth_groups from tests.utils import capture_notifications +from website.settings import OSF_ADMIN_URL, OSF_SUPPORT_EMAIL @pytest.mark.django_db @@ -406,6 +407,73 @@ def test_registries_moderation_post_withdraw_accept(self, app, retract_registrat retract_registration.refresh_from_db() assert retract_registration.moderation_state == RegistrationModerationStates.WITHDRAWN.db_name + def test_registries_moderation_post_report_spam(self, app, registration, moderator, registration_actions_url, actions_payload_base, reg_creator): + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + registration.refresh_from_db() + assert registration.moderation_state == RegistrationModerationStates.PENDING.db_name + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REPORT_SPAM.db_name + actions_payload_base['data']['attributes']['comment'] = 'Spammiest registration Ive ever seen' + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + with capture_notifications() as notifications: + resp = app.post_json_api(registration_actions_url, actions_payload_base, auth=moderator.auth) + assert resp.status_code == 201 + assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REPORT_SPAM.db_name + assert len(notifications['emits']) == 1 + emit = notifications['emits'][0] + assert emit['type'] == NotificationTypeEnum.DESK_MODERATOR_SPAM_REPORT + assert emit['kwargs']['destination_address'] == OSF_SUPPORT_EMAIL + assert emit['kwargs']['event_context']['moderator__id'] == moderator._id + assert emit['kwargs']['event_context']['resource__id'] == registration._id + assert emit['kwargs']['event_context']['comment'] == 'Spammiest registration Ive ever seen' + admin_app = OSF_ADMIN_URL.rstrip('/') + context = emit['kwargs']['event_context'] + assert context['resource_admin_app_url'] == (f'{admin_app}/nodes/{registration._id}/' if admin_app else '') + assert context['creator_admin_app_url'] == (f'{admin_app}/users/{registration.creator._id}/' if admin_app else '') + # report logged, registration untouched + registration.refresh_from_db() + assert registration.moderation_state == RegistrationModerationStates.PENDING.db_name + assert registration.spam_status is None + action = registration.actions.get(trigger=RegistrationModerationTriggers.REPORT_SPAM.db_name) + assert action.from_state == action.to_state == RegistrationModerationStates.PENDING.db_name + + def test_registries_moderation_post_report_spam_forbidden(self, app, registration, moderator_wrong_provider, reg_creator, registration_actions_url, actions_payload_base): + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REPORT_SPAM.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + # A moderator for another provider can't report + resp = app.post_json_api( + registration_actions_url, + actions_payload_base, + auth=moderator_wrong_provider.auth, + expect_errors=True + ) + assert resp.status_code == 403 + resp = app.post_json_api( + registration_actions_url, + actions_payload_base, + auth=reg_creator.auth, + expect_errors=True + ) + assert resp.status_code == 403 + assert not registration.actions.filter(trigger=RegistrationModerationTriggers.REPORT_SPAM.db_name).exists() + + def test_registries_moderation_post_report_spam_invalid_state(self, app, registration, moderator, registration_actions_url, actions_payload_base): + assert registration.moderation_state == RegistrationModerationStates.INITIAL.db_name + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REPORT_SPAM.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + resp = app.post_json_api( + registration_actions_url, + actions_payload_base, + auth=moderator.auth, + expect_errors=True + ) + assert resp.status_code == 409 + assert not registration.actions.filter(trigger=RegistrationModerationTriggers.REPORT_SPAM.db_name).exists() + def test_registries_moderation_post_withdraw_reject(self, app, retract_registration, moderator, retract_registration_actions_url, actions_payload_base, provider): with capture_notifications(): retract_registration.sanction.accept() diff --git a/notifications.yaml b/notifications.yaml index c14324df931..18fbaab4347 100644 --- a/notifications.yaml +++ b/notifications.yaml @@ -734,6 +734,13 @@ notification_types: template: 'website/templates/crossref_doi_error.html.mako' tests: [] + - name: desk_moderator_spam_report + subject: 'Spam reported by moderator {moderator__id} for {resource__id}' + __docs__: Sent to support desk when a moderator reports a submission as spam. + object_content_type_model_name: desk + template: 'website/templates/moderator_spam_report.html.mako' + tests: ['api_tests/actions/views/test_action_list.py', 'api_tests/preprints/views/test_preprint_actions.py', 'api_tests/registries_moderation/test_submissions.py'] + ### Draft Registration diff --git a/osf/migrations/0057_report_spam_trigger.py b/osf/migrations/0057_report_spam_trigger.py new file mode 100644 index 00000000000..7e110f91291 --- /dev/null +++ b/osf/migrations/0057_report_spam_trigger.py @@ -0,0 +1,23 @@ +# Generated by Django 4.2.26 on 2026-09-30 13:18 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0056_merge_20260918'), + ] + + operations = [ + migrations.AlterField( + model_name='registrationaction', + name='trigger', + field=models.CharField(choices=[('submit', 'Submit'), ('accept_submission', 'AcceptSubmission'), ('reject_submission', 'RejectSubmission'), ('request_withdrawal', 'RequestWithdrawal'), ('accept_withdrawal', 'AcceptWithdrawal'), ('reject_withdrawal', 'RejectWithdrawal'), ('force_withdraw', 'ForceWithdraw'), ('report_spam', 'ReportSpam')], max_length=31), + ), + migrations.AlterField( + model_name='reviewaction', + name='trigger', + field=models.CharField(choices=[('submit', 'Submit'), ('accept', 'Accept'), ('reject', 'Reject'), ('edit_comment', 'Edit_Comment'), ('withdraw', 'Withdraw'), ('report_spam', 'Report_Spam')], max_length=31), + ), + ] diff --git a/osf/models/mixins.py b/osf/models/mixins.py index d69197b4518..2c3e39a3069 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -975,6 +975,18 @@ def run_withdraw(self, user, comment): """ return self._run_transition(self.TriggersClass.WITHDRAW.value, user=user, comment=comment) + def run_report_spam(self, user, comment): + """Run the 'report_spam' state transition and create a corresponding Action. + + send email to support so they can look at the submitter's account. Doesn't touch the + submission itself, report_abuse is the one that flags and hides it. + + Params: + user: The moderator making the report. + comment: Text describing why. + """ + return self._run_transition(self.TriggersClass.REPORT_SPAM.value, user=user, comment=comment) + class GuardianMixin(models.Model): """ Helper for managing object-level permissions with django-guardian diff --git a/osf/models/notification_type.py b/osf/models/notification_type.py index ad8603cf5cd..8bb62001a76 100644 --- a/osf/models/notification_type.py +++ b/osf/models/notification_type.py @@ -31,6 +31,7 @@ class NotificationTypeEnum(str, Enum): DESK_ARCHIVE_JOB_FILE_NOT_FOUND = 'desk_archive_job_file_not_found' DESK_ARCHIVE_JOB_UNCAUGHT_ERROR = 'desk_archive_job_uncaught_error' DESK_CROSSREF_ERROR = 'desk_crossref_error' + DESK_MODERATOR_SPAM_REPORT = 'desk_moderator_spam_report' # User notifications USER_PENDING_VERIFICATION = 'user_pending_verification' diff --git a/osf/models/registrations.py b/osf/models/registrations.py index 9aa14d278c4..26e92451e13 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -23,7 +23,7 @@ from osf.models import Identifier from osf.utils.fields import NonNaiveDateTimeField, LowercaseCharField from osf.utils.permissions import ADMIN, READ, WRITE -from osf.exceptions import NodeStateError, DraftRegistrationStateError +from osf.exceptions import NodeStateError, DraftRegistrationStateError, InvalidTriggerError from osf.external.internet_archive.tasks import archive_to_ia, update_ia_metadata from osf.metrics.events import RegistriesModerationEvent from osf.models.notification_type import NotificationTypeEnum @@ -764,6 +764,40 @@ def update_moderation_state(self, initiated_by=None, comment=''): node.moderation_state = to_state.db_name node.save() + def report_spam_to_support(self, user, comment=''): + '''Write a RegistrationAction for a moderator's spam report and email support. + + Doesn't touch the registration itself, report_abuse is the one that flags and hides it. + + :param OSFUser user: The moderator making the report. + :param str comment: Text describing why. + ''' + trigger = RegistrationModerationTriggers.REPORT_SPAM.db_name + state = RegistrationModerationStates.from_db_name(self.moderation_state) + if state in [RegistrationModerationStates.UNDEFINED, RegistrationModerationStates.INITIAL]: + raise InvalidTriggerError(trigger, self.moderation_state, valid_triggers=[]) + # nothing moves, so from_state and to_state are the same + action = RegistrationAction.objects.create( + target=self, + creator=user, + trigger=trigger, + from_state=state.db_name, + to_state=state.db_name, + comment=comment, + ) + if waffle.switch_is_active(features.ELASTICSEARCH_METRICS): + RegistriesModerationEvent.record( + registration_id=self._id, + provider_id=self.provider._id, + from_state=state.db_name, + to_state=state.db_name, + trigger=trigger, + user_id=user._id, + comment=comment, + ) + notify.notify_report_spam(resource=self, user=user, action=action) + return action + def _write_registration_action(self, from_state, to_state, initiated_by, comment): '''Write a new RegistrationAction on relevant state transitions.''' trigger = RegistrationModerationTriggers.from_transition(from_state, to_state) diff --git a/osf/utils/machines.py b/osf/utils/machines.py index eb7e93d450b..370ed69c807 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -164,6 +164,9 @@ def notify_accept_reject(self, ev): def notify_edit_comment(self, ev): notify.notify_edit_comment(self.machineable, ev.kwargs.get('user'), self.action, self.States) + def notify_report_spam(self, ev): + notify.notify_report_spam(self.machineable, ev.kwargs.get('user'), self.action) + def notify_withdraw(self, ev): context = self.get_context() context['force_withdrawal'] = False diff --git a/osf/utils/notifications.py b/osf/utils/notifications.py index 0d279479155..b2f41f3777b 100644 --- a/osf/utils/notifications.py +++ b/osf/utils/notifications.py @@ -2,7 +2,7 @@ from osf.models.notification_type import NotificationTypeEnum from website.reviews import signals as reviews_signals -from website.settings import DOMAIN, OSF_SUPPORT_EMAIL, OSF_CONTACT_EMAIL +from website.settings import DOMAIN, OSF_ADMIN_URL, OSF_SUPPORT_EMAIL, OSF_CONTACT_EMAIL from osf.utils.workflows import RegistrationModerationTriggers def get_email_template_context(resource): @@ -160,3 +160,33 @@ def notify_withdraw_registration(resource, action, *args, **kwargs): user=contributor, event_context=context ) + + +def notify_report_spam(resource, user, action, *args, **kwargs): + is_preprint = resource.provider.type == 'osf.preprintprovider' + document_type = resource.provider.preprint_word if is_preprint else 'registration' + creator = resource.creator + admin_app = OSF_ADMIN_URL.rstrip('/') + # link straight to the account page, where support go to disable it + creator_admin_app_url = f'{admin_app}/users/{creator._id}/' if admin_app else '' + admin_app_path = 'preprints' if is_preprint else 'nodes' + resource_admin_app_url = f'{admin_app}/{admin_app_path}/{resource._id}/' if admin_app else '' + NotificationTypeEnum.DESK_MODERATOR_SPAM_REPORT.instance.emit( + destination_address=OSF_SUPPORT_EMAIL, + event_context={ + 'document_type': document_type, + 'moderator__id': user._id, + 'moderator_fullname': user.fullname, + 'moderator_absolute_url': user.absolute_url, + 'resource__id': resource._id, + 'resource_title': resource.title, + 'resource_absolute_url': resource.absolute_url, + 'resource_creator__id': creator._id, + 'resource_creator_fullname': creator.fullname, + 'resource_creator_absolute_url': creator.absolute_url, + 'creator_admin_app_url': creator_admin_app_url, + 'resource_admin_app_url': resource_admin_app_url, + 'provider_name': resource.provider.name, + 'comment': action.comment, + }, + ) diff --git a/osf/utils/workflows.py b/osf/utils/workflows.py index f562ff0aab3..9a30d4c0097 100644 --- a/osf/utils/workflows.py +++ b/osf/utils/workflows.py @@ -141,6 +141,7 @@ class RegistrationModerationTriggers(ModerationEnum): ACCEPT_WITHDRAWAL = 4 REJECT_WITHDRAWAL = 5 FORCE_WITHDRAW = 6 + REPORT_SPAM = 7 @classmethod def from_transition(cls, from_state, to_state): @@ -230,7 +231,8 @@ def db_name(self): ('WITHDRAWN', 'withdrawn'), ] REVIEW_TRIGGERS = DEFAULT_TRIGGERS + [ - ('WITHDRAW', 'withdraw') + ('WITHDRAW', 'withdraw'), + ('REPORT_SPAM', 'report_spam'), ] REGISTRATION_STATES = REVIEW_STATES + [ @@ -297,7 +299,18 @@ def db_name(self): 'source': [ReviewStates.PENDING.value, ReviewStates.ACCEPTED.value], 'dest': ReviewStates.WITHDRAWN.value, 'after': ['save_action', 'update_last_transitioned', 'perform_withdraw', 'save_changes', 'notify_withdraw'] - } + }, + { + 'trigger': ReviewTriggers.REPORT_SPAM.value, + 'source': [ + ReviewStates.PENDING.value, + ReviewStates.ACCEPTED.value, + ReviewStates.REJECTED.value, + ReviewStates.WITHDRAWN.value + ], + 'dest': '=', + 'after': ['save_action', 'notify_report_spam'], + }, ] APPROVAL_TRANSITIONS = [ diff --git a/website/settings/defaults.py b/website/settings/defaults.py index 2a33333cfbd..2a991a9c1f4 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -163,6 +163,8 @@ def parent_dir(path): FROM_EMAIL = 'openscienceframework-noreply@osf.io' # support email OSF_SUPPORT_EMAIL = 'support@osf.io' +# admin app base url +OSF_ADMIN_URL = os.environ.get('OSF_ADMIN_URL', 'https://admin.osf.io') # contact email OSF_CONTACT_EMAIL = 'contact@osf.io' diff --git a/website/templates/moderator_spam_report.html.mako b/website/templates/moderator_spam_report.html.mako new file mode 100644 index 00000000000..68ec08d6aa7 --- /dev/null +++ b/website/templates/moderator_spam_report.html.mako @@ -0,0 +1,35 @@ +<%inherit file="notify_base.mako" /> + +## submitter name come from the account being reported + +<%def name="content()"> + + +

${moderator__id | h} has reported ${resource__id | h} as spam

+ + + + + Moderator: ${moderator_fullname | h} [${moderator__id | h}] +
+ Reported ${document_type | h}: ${resource_title | h} [${resource__id | h}] +
+ Provider: ${provider_name | h} +
+ Submitted by: ${resource_creator_fullname | h} [${resource_creator__id | h}] + % if resource_admin_app_url: +
+ Review in admin: ${resource_admin_app_url | h} + % endif + % if creator_admin_app_url: +
+ Disable this account: ${creator_admin_app_url | h} + % endif + + % if comment: +
+ Remarks from the moderator: ${comment | h} + % endif + + + From b7f727aea353f85aee47f483833fe2cedb5bf282 Mon Sep 17 00:00:00 2001 From: bodintsov Date: Mon, 12 Oct 2026 16:07:45 +0300 Subject: [PATCH 02/22] fix confirmation link fails (#11954) ## Ticket https://openscience.atlassian.net/browse/ENG-12261 ## Purpose Fix confirmation link failure ## Changes Fix confirmation link failure --- admin/templates/users/user.html | 6 +++--- tests/test_claim_views.py | 15 +++++++++++++++ website/project/views/contributor.py | 2 ++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/admin/templates/users/user.html b/admin/templates/users/user.html index 5cc9244d484..ff468db400b 100644 --- a/admin/templates/users/user.html +++ b/admin/templates/users/user.html @@ -22,10 +22,10 @@ {% include "users/reset_password.html" with user=user %} {% if perms.osf.change_osfuser %} Get password reset link - {% if user.confirmed %} - + {% if user.is_confirmed %} + {% else %} - {% if user.unclaimed %} + {% if user.unclaimed_records %} Get claim links {% else %} Get confirmation link diff --git a/tests/test_claim_views.py b/tests/test_claim_views.py index 031eaa6ac70..ab0c4982db5 100644 --- a/tests/test_claim_views.py +++ b/tests/test_claim_views.py @@ -3,6 +3,7 @@ from http.cookies import SimpleCookie from unittest import mock +from urllib.parse import parse_qs, urlparse from framework.auth import Auth, authenticate, cas from framework.auth.utils import impute_names_model @@ -290,6 +291,20 @@ def test_claim_user_form_redirects_to_password_confirm_page_if_user_is_logged_in ) assert res.request.path == expected + def test_user_logs_out_if_logged_in_user_is_the_claimed_user(self): + url = self.user.get_claim_url(self.project._primary_key) + g.current_session = None + self.app.set_cookie(settings.COOKIE_NAME, self.user.get_or_create_cookie().decode()) + res = self.app.get(url) + assert res.status_code == 302 + location = urlparse(res.location) + assert location.path == '/logout/' + return_url = parse_qs(location.query)['redirect_url'][0] + assert return_url.endswith(url) + res = self.app.get(res.location) + assert res.status_code == 302 + assert res.location == cas.get_logout_url(service_url=return_url) + @mock.patch('framework.auth.cas.make_response_from_ticket') def test_claim_user_when_user_is_registered_with_orcid(self, mock_response_from_ticket): # TODO: check in qa url encoding diff --git a/website/project/views/contributor.py b/website/project/views/contributor.py index 2fe7fc5486b..89093d1fa17 100644 --- a/website/project/views/contributor.py +++ b/website/project/views/contributor.py @@ -832,6 +832,8 @@ def claim_user_form(auth, **kwargs): # If user is logged in, redirect to 're-enter password' page if auth.logged_in: + if auth.user._id == uid: + return redirect(web_url_for('auth_logout', redirect_url=request.url)) return redirect(web_url_for('claim_user_registered', uid=uid, pid=pid, token=token)) From 902af1f5794fc892305d4f047b4f03c29a57d74c Mon Sep 17 00:00:00 2001 From: mkovalua Date: Mon, 12 Oct 2026 16:19:12 +0300 Subject: [PATCH 03/22] [ENG-12060] Rejected registrations disappearing for registration providers (#11965) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Ticket https://openscience.atlassian.net/browse/ENG-12060 ## Purpose When moderator rejects a new registration submission, the rejected registration is not shown rejected tab for EGAP provider. And registration moderator page shows “Not found” error immediately after rejection. --- api/nodes/views.py | 4 +- api/schema_responses/permissions.py | 8 ++- .../test_schema_response_action_detail.py | 8 ++- .../views/test_schema_response_action_list.py | 8 ++- .../registries_moderation/test_submissions.py | 59 ++++++++++++++++++- ...est_registrations_schema_responses_list.py | 11 +++- .../views/test_schema_response_detail.py | 8 ++- osf/models/node.py | 8 ++- osf/models/registrations.py | 19 ++++++ osf/models/sanctions.py | 7 ++- osf/models/user.py | 7 ++- osf/utils/workflows.py | 1 + osf_tests/test_draft_registration.py | 31 ++++++++++ tests/test_registrations/test_review_flows.py | 1 + website/project/views/drafts.py | 5 +- 15 files changed, 164 insertions(+), 21 deletions(-) diff --git a/api/nodes/views.py b/api/nodes/views.py index ccd26a16a47..bb38bcbab3c 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -240,11 +240,11 @@ def get_draft(self, draft_id=None, check_object_permissions=True): self.check_branched_from(draft) if self.request.method not in drf_permissions.SAFE_METHODS: - if draft.registered_node and not draft.registered_node.is_deleted: + if draft.has_active_registration: raise PermissionDenied('This draft has already been registered and cannot be modified.') else: - if draft.registered_node and not draft.registered_node.is_deleted: + if draft.has_active_registration: redirect_url = draft.registered_node.absolute_api_v2_url self.headers['location'] = redirect_url raise PermanentlyMovedError(detail='Draft has already been registered') diff --git a/api/schema_responses/permissions.py b/api/schema_responses/permissions.py index c5a8c3b2906..41025258633 100644 --- a/api/schema_responses/permissions.py +++ b/api/schema_responses/permissions.py @@ -11,7 +11,11 @@ from osf.utils.workflows import ApprovalStates -MODERATOR_VISIBLE_STATES = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] +MODERATOR_VISIBLE_STATES = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, +] class SchemaResponseParentPermission: @@ -20,7 +24,7 @@ class SchemaResponseParentPermission: To GET a SchemaResponse (or a subpath), one of three conditions must be met: * The user must have "read" permissions on the parent resource * The user must be a moderator on the parent resource's Provider and the - SchemaResponse must be in an APPROVED or PENDING_MODERATION state + SchemaResponse must be in an APPROVED, PENDING_MODERATION or MODERATOR_REJECTED state * The SchemaResponse must be APPROVED and the parent resource must be public For DELETE/PATCH/POST/PUT, the required permission should be added in the diff --git a/api_tests/actions/views/test_schema_response_action_detail.py b/api_tests/actions/views/test_schema_response_action_detail.py index 46fcbde63fa..a4d3938c3dc 100644 --- a/api_tests/actions/views/test_schema_response_action_detail.py +++ b/api_tests/actions/views/test_schema_response_action_detail.py @@ -110,10 +110,14 @@ def get_status_code_for_preconditions( if role == 'non-contributor': return 403 - # Moderators can GET PENDING_MODERATION and APPROVED SchemaResponses on + # Moderators can GET PENDING_MODERATION, APPROVED and MODERATOR_REJECTED SchemaResponses on # public or private registrations that are part of a moderated registry if role == 'moderator': - moderator_visible_states = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] if schema_response_state in moderator_visible_states and reviews_workflow is not None: return 200 else: diff --git a/api_tests/actions/views/test_schema_response_action_list.py b/api_tests/actions/views/test_schema_response_action_list.py index a4099965086..1763ce06f92 100644 --- a/api_tests/actions/views/test_schema_response_action_list.py +++ b/api_tests/actions/views/test_schema_response_action_list.py @@ -146,10 +146,14 @@ def get_status_code_for_preconditions( if role == 'non-contributor': return 403 - # Moderators can GET PENDING_MODERATION and APPROVED SchemaResponses on + # Moderators can GET PENDING_MODERATION, APPROVED and MODERATOR_REJECTED SchemaResponses on # public or private registrations that are part of a moderated registry if role == 'moderator': - moderator_visible_states = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] if schema_response_state in moderator_visible_states and reviews_workflow is not None: return 200 else: diff --git a/api_tests/registries_moderation/test_submissions.py b/api_tests/registries_moderation/test_submissions.py index 19da26b9046..abc85777595 100644 --- a/api_tests/registries_moderation/test_submissions.py +++ b/api_tests/registries_moderation/test_submissions.py @@ -1,7 +1,9 @@ import pytest import datetime +from unittest import mock from api.base.settings.defaults import API_BASE +from framework.auth import Auth from api.providers.workflows import Workflows from osf.utils.workflows import NodeRequestTypes, RegistrationModerationTriggers, RegistrationModerationStates @@ -14,7 +16,8 @@ NodeRequestFactory, EmbargoFactory, RetractionFactory, - ProjectFactory + ProjectFactory, + SubjectFactory, ) @@ -352,6 +355,58 @@ def test_registries_moderation_post_reject_moderator(self, app, registration, re assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REJECT_SUBMISSION.db_name registration.refresh_from_db() assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not registration.is_deleted + + def test_moderator_can_view_rejected_registration(self, app, registration, moderator, moderator_wrong_provider, registration_actions_url, actions_payload_base): + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REJECT_SUBMISSION.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + with capture_notifications(): + resp = app.post_json_api(registration_actions_url, actions_payload_base, auth=moderator.auth) + assert resp.status_code == 201 + + registration_url = f'/{API_BASE}registrations/{registration._id}/' + assert app.get(registration_url, auth=moderator.auth).status_code == 200 + resp = app.get(f'{registration_url}schema_responses/', auth=moderator.auth) + assert [entry['id'] for entry in resp.json['data']] == [registration.schema_responses.last()._id] + + # Rejected registrations stay private for everyone else + assert app.get(registration_url, auth=moderator_wrong_provider.auth, expect_errors=True).status_code == 403 + assert app.get(registration_url, auth=AuthUserFactory().auth, expect_errors=True).status_code == 403 + assert app.get(registration_url, expect_errors=True).status_code == 401 + + def test_rejected_submission_draft_is_returned_for_resubmission(self, app, registration, reg_creator, moderator, registration_actions_url, actions_payload_base): + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REJECT_SUBMISSION.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + with capture_notifications(): + resp = app.post_json_api(registration_actions_url, actions_payload_base, auth=moderator.auth) + assert resp.status_code == 201 + + # The draft is back in the author's drafts and can be edited + draft = registration.draft_registration.get() + assert draft in reg_creator.draft_registrations_active + draft_url = f'/{API_BASE}draft_registrations/{draft._id}/' + assert app.get(draft_url, auth=reg_creator.auth).status_code == 200 + payload = {'data': {'type': 'draft_registrations', 'id': draft._id, 'attributes': {'title': 'Fixed title'}}} + assert app.patch_json_api(draft_url, payload, auth=reg_creator.auth).status_code == 200 + + # Resubmitting creates a new registration, the rejected one stays as it is + draft.subjects.add(SubjectFactory()) + with mock.patch('framework.celery_tasks.handlers.enqueue_task'): + resubmitted = draft.register(Auth(reg_creator), save=True) + draft.refresh_from_db() + registration.refresh_from_db() + assert draft.registered_node == resubmitted + assert draft not in reg_creator.draft_registrations_active + assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not registration.is_deleted def test_registries_moderation_post_embargo(self, app, embargo_registration, moderator, provider, embargo_registration_actions_url, actions_payload_base, reg_creator): assert embargo_registration.moderation_state == RegistrationModerationStates.INITIAL.db_name @@ -388,6 +443,7 @@ def test_registries_moderation_post_embargo_reject(self, app, embargo_registrati assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REJECT_SUBMISSION.db_name embargo_registration.refresh_from_db() assert embargo_registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not embargo_registration.is_deleted @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links') def test_registries_moderation_post_withdraw_accept(self, app, retract_registration, moderator, retract_registration_actions_url, actions_payload_base, provider): @@ -695,6 +751,7 @@ def test_rejected_submission_doesnt_break_ham_functionality(self, app, registrat assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REJECT_SUBMISSION.db_name registration.refresh_from_db() assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not registration.is_deleted # ham the project creator user.confirm_ham(save=True) diff --git a/api_tests/schema_responses/views/test_registrations_schema_responses_list.py b/api_tests/schema_responses/views/test_registrations_schema_responses_list.py index 5f000d4a13f..8cbc963e52c 100644 --- a/api_tests/schema_responses/views/test_registrations_schema_responses_list.py +++ b/api_tests/schema_responses/views/test_registrations_schema_responses_list.py @@ -186,8 +186,8 @@ class TestRegistrationSchemaResponseListGETBehavior: Contributors on the base Registration should be able to see all SchemaResponses on a registration, whether approved or not, for both public and private Registrations. - Moderators should be able to see both PENDING_MODERATION and APPROVED SchemaResponses - on Registrations that are part of the moderated provider. + Moderators should be able to see PENDING_MODERATION, APPROVED and MODERATOR_REJECTED + SchemaResponses on Registrations that are part of the moderated provider. Non-contributors should only see APPROVED SchemaResponses on Public registrations (permissions tests verify 403/401 response for non-contributors on a private registration). @@ -242,7 +242,12 @@ def test_GET__moderated_registration_responses_as_moderator( # Always expect the APPROVED response expected_ids = {updated_response.previous_response._id} - if response_state in [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED]: + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] + if response_state in moderator_visible_states: expected_ids.add(updated_response._id) encountered_ids = {entry['id'] for entry in resp.json['data']} assert encountered_ids == expected_ids diff --git a/api_tests/schema_responses/views/test_schema_response_detail.py b/api_tests/schema_responses/views/test_schema_response_detail.py index 6b19255377b..13dec2daed5 100644 --- a/api_tests/schema_responses/views/test_schema_response_detail.py +++ b/api_tests/schema_responses/views/test_schema_response_detail.py @@ -136,10 +136,14 @@ def get_status_code_for_preconditions( if role == 'non-contributor': return 403 - # Moderators can GET PENDING_MODERATION and APPROVED SchemaResponses on + # Moderators can GET PENDING_MODERATION, APPROVED and MODERATOR_REJECTED SchemaResponses on # public or private registrations that are part of a moderated registry if role == 'moderator': - moderator_visible_states = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] if schema_response_state in moderator_visible_states and reviews_workflow is not None: return 200 else: diff --git a/osf/models/node.py b/osf/models/node.py index daf43b0e444..9c6776e758c 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -59,7 +59,7 @@ from osf.utils.datetime_aware_jsonfield import DateTimeAwareJSONField from osf.utils.fields import NonNaiveDateTimeField from osf.utils.requests import get_request_and_user_id, string_type_request_headers, get_current_request -from osf.utils.workflows import CollectionSubmissionStates +from osf.utils.workflows import CollectionSubmissionStates, RegistrationModerationStates from osf.utils import sanitize from website import language, settings from website.citations.utils import datetime_to_csl @@ -680,7 +680,11 @@ def draft_registrations_active(self): return DraftRegistration.objects.filter( models.Q(branched_from=self) & models.Q(deleted__isnull=True) & - (models.Q(registered_node=None) | models.Q(registered_node__deleted__isnull=False)), + ( + models.Q(registered_node=None) + | models.Q(registered_node__deleted__isnull=False) + | models.Q(registered_node__moderation_state=RegistrationModerationStates.REJECTED.db_name) + ), ) @property diff --git a/osf/models/registrations.py b/osf/models/registrations.py index 26e92451e13..43f90416899 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -1195,6 +1195,25 @@ def branched_from_type(self): def has_project(self): return isinstance(self.branched_from, Node) + @property + def has_active_registration(self): + """ + Check whether this draft has an active registration, so it can no longer be edited. + A registration that was deleted or rejected by a moderator is not active: its draft is + returned to the contributors for resubmission. + """ + registration = self.registered_node + if registration is None: + # Not registered yet + return False + if registration.is_deleted: + # Rejected by a contributor or failed to archive + return False + if registration.moderation_state == RegistrationModerationStates.REJECTED.db_name: + # Rejected by a moderator + return False + return True + @property def url(self): return self.URL_TEMPLATE.format( diff --git a/osf/models/sanctions.py b/osf/models/sanctions.py index 7796456dbb6..b89c380655d 100644 --- a/osf/models/sanctions.py +++ b/osf/models/sanctions.py @@ -687,6 +687,9 @@ def _on_reject(self, event_data): 'embargo_id': self._id, }, auth=Auth(user) if user else Auth(self.initiated_by)) + # Registrations rejected by a moderator are kept in the REJECTED moderation state + if self.approval_stage is ApprovalStates.MODERATOR_REJECTED: + return # Remove backref to parent project if embargo was for a new registration if not self.for_existing_registration: parent_registration.delete_registration_tree(save=True) @@ -1052,7 +1055,9 @@ def _on_reject(self, event_data): NodeLog = apps.get_model('osf.NodeLog') registered_from = self.target_registration.registered_from - self.target_registration.delete_registration_tree(save=True) + # Registrations rejected by a moderator are kept in the REJECTED moderation state + if self.approval_stage is not ApprovalStates.MODERATOR_REJECTED: + self.target_registration.delete_registration_tree(save=True) registered_from.add_log( action=NodeLog.REGISTRATION_APPROVAL_CANCELLED, params={ diff --git a/osf/models/user.py b/osf/models/user.py index 67530a2c627..64ee2bca5b7 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -65,6 +65,7 @@ from osf.utils.names import impute_names from osf.utils.requests import check_select_for_update from osf.utils.permissions import API_CONTRIBUTOR_PERMISSIONS, MANAGER, MEMBER, ADMIN +from osf.utils.workflows import RegistrationModerationStates from website import settings as website_settings from website import filters from website.project import new_bookmark_collection @@ -971,7 +972,11 @@ def draft_registrations_active(self): """ return self.draft_registrations.filter( - (models.Q(registered_node__isnull=True) | models.Q(registered_node__deleted__isnull=False)), + ( + models.Q(registered_node__isnull=True) + | models.Q(registered_node__deleted__isnull=False) + | models.Q(registered_node__moderation_state=RegistrationModerationStates.REJECTED.db_name) + ), branched_from__deleted__isnull=True, deleted__isnull=True, ) diff --git a/osf/utils/workflows.py b/osf/utils/workflows.py index 9a30d4c0097..3351716bcf5 100644 --- a/osf/utils/workflows.py +++ b/osf/utils/workflows.py @@ -128,6 +128,7 @@ def in_moderation_states(cls): cls.EMBARGO.db_name, cls.PENDING_EMBARGO_TERMINATION.db_name, cls.PENDING_WITHDRAW.db_name, + cls.REJECTED.db_name, ] diff --git a/osf_tests/test_draft_registration.py b/osf_tests/test_draft_registration.py index f4b2feb3c1d..a58d216711d 100644 --- a/osf_tests/test_draft_registration.py +++ b/osf_tests/test_draft_registration.py @@ -6,6 +6,7 @@ from osf.exceptions import UserNotAffiliatedError, DraftRegistrationStateError, NodeStateError from osf.models import RegistrationSchema, DraftRegistration, DraftRegistrationContributor, NodeLicense, Node, NodeLog from osf.utils.permissions import ADMIN, READ, WRITE +from osf.utils.workflows import RegistrationModerationStates from osf_tests.test_node import TestNodeEditableFieldsMixin, TestTagging, TestNodeSubjects from osf_tests.test_node_license import TestNodeLicenses from django.utils import timezone @@ -192,6 +193,36 @@ def test_draft_registrations_active(self): assert draft2 in project.draft_registrations_active.all() assert finished_draft not in project.draft_registrations_active.all() + def test_draft_registrations_includes_drafts_of_moderator_rejected_registrations(self): + project = factories.ProjectFactory() + rejected_registration = factories.RegistrationFactory(project=project) + rejected_registration.moderation_state = RegistrationModerationStates.REJECTED.db_name + rejected_registration.save() + + draft = factories.DraftRegistrationFactory(branched_from=project, user=project.creator) + draft.registered_node = rejected_registration + draft.save() + + assert draft in project.draft_registrations_active.all() + assert draft in project.creator.draft_registrations_active.all() + + def test_has_active_registration(self): + project = factories.ProjectFactory() + draft = factories.DraftRegistrationFactory(branched_from=project) + assert not draft.has_active_registration + + registration = factories.RegistrationFactory(project=project) + draft.registered_node = registration + assert draft.has_active_registration + + registration.moderation_state = RegistrationModerationStates.REJECTED.db_name + assert not draft.has_active_registration + + registration.moderation_state = RegistrationModerationStates.ACCEPTED.db_name + registration.is_deleted = True + registration.deleted = timezone.now() + assert not draft.has_active_registration + def test_draft_registration_url(self): project = factories.ProjectFactory() draft = factories.DraftRegistrationFactory(branched_from=project) diff --git a/tests/test_registrations/test_review_flows.py b/tests/test_registrations/test_review_flows.py index 9f6c4306cfd..cfab164b37c 100644 --- a/tests/test_registrations/test_review_flows.py +++ b/tests/test_registrations/test_review_flows.py @@ -355,6 +355,7 @@ def test_moderator_rejection_flow( registration.refresh_from_db() assert registration.moderation_state == end_state.db_name + assert not registration.is_deleted @pytest.mark.parametrize('sanction_object', [registration_approval, embargo, retraction]) def test_admin_cannot_give_moderator_approval(self, sanction_object, provider): diff --git a/website/project/views/drafts.py b/website/project/views/drafts.py index e4ecd1bc92a..56d5844e7bc 100644 --- a/website/project/views/drafts.py +++ b/website/project/views/drafts.py @@ -97,8 +97,7 @@ def validate_registration_choice(registration_choice): ) def check_draft_state(draft): - registered_and_deleted = draft.registered_node and draft.registered_node.is_deleted - if draft.registered_node and not registered_and_deleted: + if draft.has_active_registration: raise HTTPError(http_status.HTTP_403_FORBIDDEN, data={ 'message_short': 'This draft has already been registered', 'message_long': 'This draft has already been registered and cannot be modified.' @@ -192,7 +191,7 @@ def delete_draft_registration(auth, node, draft, *args, **kwargs): :return: None :rtype: NoneType """ - if draft.registered_node and not draft.registered_node.is_deleted: + if draft.has_active_registration: raise HTTPError( http_status.HTTP_403_FORBIDDEN, data={ From b539e0e603a3e6b21854213a9eaf71a485459970 Mon Sep 17 00:00:00 2001 From: futa-ikeda <51409893+futa-ikeda@users.noreply.github.com> Date: Mon, 12 Oct 2026 09:27:30 -0400 Subject: [PATCH 04/22] [ENG-11075] Prevent inclusion of registered_from relationship in metadata representation if registered_from node is private (#11964) ## Ticket https://openscience.atlassian.net/browse/ENG-11075 ## Purpose * Prevent failure of registration approval when `prevent_project_creation` flag is turned on * When this flag is turned on, registrations will have a `registered_from` relationship that points to a `draft_node` object that shouldn't be made public. This causes the metadata gathering to fail, and subsequently fails the registration approval process ## Changes * Add logic to skip serializing the registration.registered_from relationship if the registered_from relationship does not point to a public object --- osf/metadata/osf_gathering.py | 3 ++- tests/identifiers/test_datacite.py | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/osf/metadata/osf_gathering.py b/osf/metadata/osf_gathering.py index 7987473638e..7c08d7864f6 100644 --- a/osf/metadata/osf_gathering.py +++ b/osf/metadata/osf_gathering.py @@ -815,7 +815,8 @@ def gather_registration_related_items(focus): article_iri = DOI[related_article_doi] yield (DCTERMS.relation, article_iri) yield (article_iri, DCTERMS.identifier, str(article_iri)) - yield (DCTERMS.isVersionOf, OsfFocus(focus.dbmodel.registered_from)) + if focus.dbmodel.registered_from and focus.dbmodel.registered_from.is_public: + yield (DCTERMS.isVersionOf, OsfFocus(focus.dbmodel.registered_from)) # TODO: should the title/description/tags/etc fields on osf.models.Outcome # overwrite the same fields on osf.models.Registration? artifact_qs = ( diff --git a/tests/identifiers/test_datacite.py b/tests/identifiers/test_datacite.py index 1a774ceea71..e5576098f56 100644 --- a/tests/identifiers/test_datacite.py +++ b/tests/identifiers/test_datacite.py @@ -8,7 +8,7 @@ from framework.auth import Auth from osf.models import GuidMetadataRecord, Outcome from osf.utils.outcomes import ArtifactTypes -from osf_tests.factories import AuthUserFactory, IdentifierFactory, RegistrationFactory +from osf_tests.factories import AuthUserFactory, IdentifierFactory, ProjectFactory, RegistrationFactory from tests.base import OsfTestCase from tests.test_addons import assert_urls_equal from website import settings @@ -204,7 +204,7 @@ def test_datacite_format_contributors(self, datacite_client): assert f'{invisible_contrib.fullname}' not in metadata_xml def test_datacite_format_related_resources(self, datacite_client): - registration = RegistrationFactory(is_public=True, has_doi=True, article_doi='10.pub/lication') + registration = RegistrationFactory(project=ProjectFactory(is_public=True, article_doi='10.pub/lication'), is_public=True, has_doi=True) outcome = Outcome.objects.for_registration(registration, create=True) data_artifact = outcome.artifact_metadata.create( identifier=IdentifierFactory(category='doi'), artifact_type=ArtifactTypes.DATA, finalized=True @@ -242,7 +242,7 @@ def test_datacite_format_related_resources(self, datacite_client): _assert_unordered_list_of_dicts_equal(metadata_dict['relatedIdentifiers'], expected_relationships) def test_datacite_format_related_resources__ignores_duplicate_pids(self, datacite_client): - registration = RegistrationFactory(is_public=True, has_doi=True) + registration = RegistrationFactory(project=ProjectFactory(is_public=True), has_doi=True) outcome = Outcome.objects.for_registration(registration, create=True) identifier = IdentifierFactory(category='doi') outcome.artifact_metadata.create( @@ -269,7 +269,7 @@ def test_datacite_format_related_resources__ignores_duplicate_pids(self, datacit _assert_unordered_list_of_dicts_equal(metadata_dict['relatedIdentifiers'], expected_relationships) def test_datacite_format_related_resources__ignores_inactive_resources(self, datacite_client): - registration = RegistrationFactory(is_public=True, has_doi=True) + registration = RegistrationFactory(project=ProjectFactory(is_public=True), is_public=True, has_doi=True) outcome = Outcome.objects.for_registration(registration, create=True) active_artifact = outcome.artifact_metadata.create( identifier=IdentifierFactory(category='doi'), artifact_type=ArtifactTypes.DATA, finalized=True @@ -300,6 +300,14 @@ def test_datacite_format_related_resources__ignores_inactive_resources(self, dat ] _assert_unordered_list_of_dicts_equal(metadata_dict['relatedIdentifiers'], expected_relationships) + def test_datacite_format_related_resources__ignores_private_projects(self, datacite_client): + registration = RegistrationFactory(project=ProjectFactory(is_public=False), is_public=True, has_doi=True) + outcome = Outcome.objects.for_registration(registration, create=True) + + metadata_dict = datacite_client.build_metadata(registration, as_xml=False) + + assert metadata_dict['relatedIdentifiers'] == [] + def _set_funding_info(self, registration, funding_info): metadata_record = GuidMetadataRecord.objects.for_guid(registration._id) metadata_record.funding_info = funding_info From 5c49f3ed088239051b10a7abef63ddb40e8a02ea Mon Sep 17 00:00:00 2001 From: Vlad0n20 <137097005+Vlad0n20@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:10:49 +0200 Subject: [PATCH 05/22] [ENG-12157] Handle contributors without permission group in merge_user (#11963) ## Ticket https://openscience.atlassian.net/browse/ENG-12157 --- admin/users/views.py | 2 +- admin_tests/users/test_views.py | 2 +- api/users/tasks.py | 48 +++++-- api_tests/users/views/test_user_settings.py | 26 ++++ notifications.yaml | 7 + osf/models/notification_type.py | 1 + osf/models/user.py | 78 +++++++++-- osf_tests/test_merging_users.py | 127 +++++++++++++++++- osf_tests/test_user.py | 15 ++- tests/test_utils.py | 10 +- .../user_merge_failed_report.html.mako | 19 +++ 11 files changed, 296 insertions(+), 39 deletions(-) create mode 100644 website/templates/user_merge_failed_report.html.mako diff --git a/admin/users/views.py b/admin/users/views.py index 94d93f5ba87..3223b1b4015 100644 --- a/admin/users/views.py +++ b/admin/users/views.py @@ -432,7 +432,7 @@ def form_valid(self, form): guid_to_be_merged = form.cleaned_data['user_guid_to_be_merged'] user_to_be_merged = OSFUser.objects.get(guids___id=guid_to_be_merged, guids___id__isnull=False) - merge_users.delay(user._id, user_to_be_merged._id) + merge_users.delay(user._id, user_to_be_merged._id, initiator_guid=self.request.user._id) messages.success( self.request, f'Merge of user {user_to_be_merged._id} into {user._id} has been queued and will run in the background.', diff --git a/admin_tests/users/test_views.py b/admin_tests/users/test_views.py index ac982e1cd4b..2864edf40d1 100644 --- a/admin_tests/users/test_views.py +++ b/admin_tests/users/test_views.py @@ -717,4 +717,4 @@ def test_merge_user(self, mock_merge_users_delay): assert valid_form.is_valid() view.form_valid(valid_form) - mock_merge_users_delay.assert_called_with(user._id, user_merged._id) + mock_merge_users_delay.assert_called_with(user._id, user_merged._id, initiator_guid=view.request.user._id) diff --git a/api/users/tasks.py b/api/users/tasks.py index c207bd3adaa..091fccd2545 100644 --- a/api/users/tasks.py +++ b/api/users/tasks.py @@ -10,30 +10,54 @@ @celery_app.task(name='api.users.tasks.merge_users') -def merge_users(merger_guid: str, mergee_guid: str): +def merge_users(merger_guid: str, mergee_guid: str, initiator_guid: str | None = None): """ Background task to merge one user into another. :param merger_guid: GUID of the primary user that will receive content :param mergee_guid: GUID of the user being merged into the primary user + :param initiator_guid: GUID of the user who started the merge, notified if it fails """ - from osf.models import OSFUser + merger = OSFUser.load(merger_guid) + mergee = OSFUser.load(mergee_guid) + + if not merger or not mergee: + message = f'User merge task received invalid users: merger={merger_guid}, mergee={mergee_guid}' + sentry.log_message(message) + _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, message) + return + + if merger == mergee: + message = f'User merge task attempted to merge a user into itself: {merger_guid}' + sentry.log_message(message) + _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, message) + return try: - merger = OSFUser.load(merger_guid) - mergee = OSFUser.load(mergee_guid) + merger.merge_user(mergee) + except Exception as exc: + _notify_merge_failed(initiator_guid, merger_guid, mergee_guid, repr(exc)) - if not merger or not mergee: - sentry.log_message(f'User merge task received invalid users: merger={merger_guid}, mergee={mergee_guid}') - return - if merger == mergee: - sentry.log_message(f'User merge task attempted to merge a user into itself: {merger_guid}') +def _notify_merge_failed(initiator_guid: str | None, merger_guid: str, mergee_guid: str, error: str): + if not initiator_guid: + return + try: + initiator = OSFUser.load(initiator_guid) + if not initiator: return - - merger.merge_user(mergee) + NotificationTypeEnum.USER_MERGE_FAILED_REPORT.instance.emit( + user=initiator, + message_frequency='instantly', + event_context={ + 'merger_guid': merger_guid, + 'mergee_guid': mergee_guid, + 'error': error, + }, + save=False, + ) except Exception as exc: - logger.exception(f'Unexpected error during background user merge: merger={merger_guid}, mergee={mergee_guid}') + logger.exception('Failed to send user merge failure email') sentry.log_exception(exc) diff --git a/api_tests/users/views/test_user_settings.py b/api_tests/users/views/test_user_settings.py index d543c781427..6e70bef1da5 100644 --- a/api_tests/users/views/test_user_settings.py +++ b/api_tests/users/views/test_user_settings.py @@ -5,10 +5,12 @@ from api.base.utils import hashids from osf_tests.factories import ( AuthUserFactory, + ProjectFactory, UserFactory, ) from osf.models import Email, NotableDomain, NotificationTypeEnum from framework.auth.views import auth_email_logout +from osf.utils.permissions import READ from tests.utils import capture_notifications @pytest.fixture() @@ -579,6 +581,30 @@ def test_updating_verified_for_merge(self, app, user_one, user_two, payload): assert res.json['data']['attributes']['confirmed'] is True assert res.json['data']['attributes']['is_merge'] is False + def test_updating_verified_for_merge_contributor_without_permission_group(self, app, user_one, user_two, payload): + """Regression: merging used to fail with `ValueError: None is not in list` (HTTP 500) + when both accounts were contributors on a project and one had no permission group.""" + project = ProjectFactory(creator=user_two) + project.add_contributor(user_one, permissions=READ, save=True) + project.remove_permission(user_one, READ) + assert project.contributor_set.get(user=user_one).permission is None + + payload['data']['attributes'] = {'verified': True} + token = user_one.add_unconfirmed_email(user_two.username) + user_one.email_verifications[token]['confirmed'] = True + user_one.save() + url = f'/{API_BASE}users/{user_one._id}/settings/emails/{token}/' + + res = app.patch_json_api(url, payload, auth=user_one.auth, expect_errors=True) + + assert res.status_code == 200 + assert res.json['data']['attributes']['verified'] is True + user_two.reload() + assert user_two.merged_by == user_one + assert project.is_contributor(user_one) + assert not project.is_contributor(user_two) + assert project.contributor_set.get(user=user_one).permission == 'admin' + @mock.patch('api.users.views.send_confirm_email_async') def test_resend_confirmation_email(self, mock_send_confirm_email_async, app, user_one, unconfirmed_url, confirmed_url): url = f'{unconfirmed_url}?resend_confirmation=True' diff --git a/notifications.yaml b/notifications.yaml index 18fbaab4347..69dab308f94 100644 --- a/notifications.yaml +++ b/notifications.yaml @@ -325,6 +325,13 @@ notification_types: template: 'website/templates/user_confirm_ham_report.html.mako' tests: [] + - name: user_merge_failed_report + subject: 'Merge of user {mergee_guid} into {merger_guid} failed' + __docs__: Sent to the admin who triggered a user merge when the merge fails. + object_content_type_model_name: osfuser + template: 'website/templates/user_merge_failed_report.html.mako' + tests: [] + - name: desk_registration_bulk_upload_product_owner subject: 'Registry Could Not Bulk Upload Registrations' object_content_type_model_name: osfuser diff --git a/osf/models/notification_type.py b/osf/models/notification_type.py index 8bb62001a76..f898cb4c490 100644 --- a/osf/models/notification_type.py +++ b/osf/models/notification_type.py @@ -82,6 +82,7 @@ class NotificationTypeEnum(str, Enum): USER_CROSSREF_DOI_PENDING = 'user_crossref_doi_pending' USER_TERMS_OF_USE_UPDATED = 'user_terms_of_use_updated' # added as a placeholder USER_CONFIRM_HAM_REPORT = 'user_confirm_ham_report' + USER_MERGE_FAILED_REPORT = 'user_merge_failed_report' # Node notifications NODE_FILE_UPDATED = 'node_file_updated' diff --git a/osf/models/user.py b/osf/models/user.py index 64ee2bca5b7..525f94d9e1f 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -772,6 +772,24 @@ def merge_user(self, user): if self == user: raise ValueError('Cannot merge a user into itself') + try: + with transaction.atomic(): + nodes_to_reindex, preprints_to_reindex = self._merge_user(user) + except Exception as exc: + logger.exception(f'Failed to merge user {user._id} into {self._id}; the merge was rolled back') + sentry.log_exception(exc) + self.refresh_from_db() + user.refresh_from_db() + raise + + # Side effects outside the database only run once the merge is committed + transaction.on_commit(lambda: self._after_merge_user(user, nodes_to_reindex, preprints_to_reindex)) + + def _merge_user(self, user): + """Database part of `merge_user`. Must run inside a transaction. + + :return: nodes and preprints of `user` to reindex in SHARE after the merge is committed + """ # Capture content to SHARE reindex BEFORE merge transfers contributors # After merge, user.contributed and user.preprints will be empty nodes_to_reindex = list(user.contributed) @@ -862,8 +880,9 @@ def merge_user(self, user): # if both accounts are contributor of the same project if node.is_contributor(self) and node.is_contributor(user): self_perms = Contributor(node=node, user=self).permission - permissions = API_CONTRIBUTOR_PERMISSIONS[max(API_CONTRIBUTOR_PERMISSIONS.index(user_perms), API_CONTRIBUTOR_PERMISSIONS.index(self_perms))] - node.set_permissions(user=self, permissions=permissions) + permissions = self._highest_permission(node, user, user_perms, self_perms) + if permissions: + node.set_permissions(user=self, permissions=permissions) visible1 = self._id in node.visible_contributor_ids visible2 = user._id in node.visible_contributor_ids @@ -873,9 +892,12 @@ def merge_user(self, user): node.contributor_set.filter(user=user).delete() else: node.contributor_set.filter(user=user).update(user=self) - node.add_permission(self, user_perms) + self._warn_if_no_permission(node, user, user_perms) + if user_perms: + node.add_permission(self, user_perms) - node.remove_permission(user, user_perms) + if user_perms: + node.remove_permission(user, user_perms) node.save() # Skip bookmark collections @@ -898,8 +920,6 @@ def merge_user(self, user): self._merge_user_draft_registrations(user) # finalize the merge - remove_sessions_for_user(user) - # - username is set to the GUID so the merging user can set it primary # in the future (note: it cannot be set to None due to non-null constraint) user.set_unusable_username() @@ -909,6 +929,12 @@ def merge_user(self, user): user.merged_by = self user.save() + + return nodes_to_reindex, preprints_to_reindex + + def _after_merge_user(self, user, nodes_to_reindex, preprints_to_reindex): + """Side effects of `merge_user` outside the database, run after the merge is committed.""" + remove_sessions_for_user(user) signals.user_account_merged.send(user) from api.share.utils import update_share @@ -925,6 +951,22 @@ def merge_user(self, user): except Exception as e: logger.exception(f'Failed to SHARE reindex preprint {preprint._id} during user merge: {e}') + def _warn_if_no_permission(self, resource, user, permission): + """Contributor rows without a permission group are inconsistent data. Keep them visible without failing the merge.""" + if permission is None: + message = f'Merging user {user._id} into {self._id}: contributor of {resource._id} has no permission group' + logger.warning(message) + sentry.log_message(message) + + def _highest_permission(self, resource, user, *permissions): + """Return the highest of `permissions`, ignoring missing (`None`) ones. `None` if none are set.""" + if any(permission is None for permission in permissions): + self._warn_if_no_permission(resource, user, None) + known = [permission for permission in permissions if permission is not None] + if not known: + return None + return API_CONTRIBUTOR_PERMISSIONS[max(API_CONTRIBUTOR_PERMISSIONS.index(permission) for permission in known)] + def _merge_users_preprints(self, user): """ Preprints use guardian. The PreprintContributor table stores order and bibliographic information. @@ -943,9 +985,10 @@ def _merge_users_preprints(self, user): self_contributor = PreprintContributor.objects.get(preprint=preprint, user=self) self_perms = self_contributor.permission - max_perms_index = max(API_CONTRIBUTOR_PERMISSIONS.index(self_perms), API_CONTRIBUTOR_PERMISSIONS.index(user_perms)) # Add the highest of `self` perms or `user` perms to `self` - preprint.set_permissions(user=self, permissions=API_CONTRIBUTOR_PERMISSIONS[max_perms_index]) + highest_perms = self._highest_permission(preprint, user, self_perms, user_perms) + if highest_perms: + preprint.set_permissions(user=self, permissions=highest_perms) if not self_contributor.visible and user_contributor.visible: # if `self` is not visible, but `user` is visible, make `self` visible. @@ -957,12 +1000,15 @@ def _merge_users_preprints(self, user): # `self` is not a contributor, but `user` is. Transfer `user` permissions and # contributor information to `self`. Remove permissions from `user`. preprint.contributor_set.filter(user=user).update(user=self) - preprint.add_permission(self, user_perms) + self._warn_if_no_permission(preprint, user, user_perms) + if user_perms: + preprint.add_permission(self, user_perms) if preprint.creator == user: preprint.creator = self - preprint.remove_permission(user, user_perms) + if user_perms: + preprint.remove_permission(user, user_perms) preprint.save() @property @@ -999,9 +1045,10 @@ def _merge_user_draft_registrations(self, user): self_contributor = DraftRegistrationContributor.objects.get(draft_registration=draft_reg, user=self) self_perms = self_contributor.permission - max_perms_index = max(API_CONTRIBUTOR_PERMISSIONS.index(self_perms), API_CONTRIBUTOR_PERMISSIONS.index(user_perms)) # Add the highest of `self` perms or `user` perms to `self` - draft_reg.set_permissions(user=self, permissions=API_CONTRIBUTOR_PERMISSIONS[max_perms_index]) + highest_perms = self._highest_permission(draft_reg, user, self_perms, user_perms) + if highest_perms: + draft_reg.set_permissions(user=self, permissions=highest_perms) if not self_contributor.visible and user_contributor.visible: # if `self` is not visible, but `user` is visible, make `self` visible. @@ -1013,12 +1060,15 @@ def _merge_user_draft_registrations(self, user): # `self` is not a contributor, but `user` is. Transfer `user` permissions and # contributor information to `self`. Remove permissions from `user`. draft_reg.contributor_set.filter(user=user).update(user=self) - draft_reg.add_permission(self, user_perms) + self._warn_if_no_permission(draft_reg, user, user_perms) + if user_perms: + draft_reg.add_permission(self, user_perms) if draft_reg.initiator == user: draft_reg.initiator = self - draft_reg.remove_permission(user, user_perms) + if user_perms: + draft_reg.remove_permission(user, user_perms) draft_reg.save() @property diff --git a/osf_tests/test_merging_users.py b/osf_tests/test_merging_users.py index b27167f382b..c6d11b57602 100644 --- a/osf_tests/test_merging_users.py +++ b/osf_tests/test_merging_users.py @@ -1,9 +1,11 @@ import pytest from unittest import mock import datetime as dt +from django.test import TestCase from django.utils import timezone from tests.base import OsfTestCase +from api.users.tasks import merge_users from framework.celery_tasks import handlers from website import settings from website.util.metrics import OsfSourceTags @@ -16,10 +18,14 @@ ProjectFactory, UnregUserFactory, ExternalAccountFactory, + PreprintFactory, + DraftRegistrationFactory, ) from importlib import import_module from django.conf import settings as django_conf_settings from osf.models import UserSessionMap, NotificationTypeEnum +from osf.utils import permissions +from osf.utils.permissions import READ, WRITE from tests.utils import run_celery_tasks, capture_notifications from waffle.testutils import override_flag from osf.features import ENABLE_GV @@ -196,7 +202,7 @@ def is_mrm_field(value): mock_client = mock.MagicMock() mock_get_mailchimp_api.return_value = mock_client - with run_celery_tasks(): + with run_celery_tasks(), TestCase.captureOnCommitCallbacks(execute=True): # perform the merge with override_flag(ENABLE_GV, active=True): self.user.merge_user(other_user) @@ -328,3 +334,122 @@ def test_send_confirm_email_emits_merge_notification(self): assert len(notifications['emits']) == 1 assert notifications['emits'][0]['type'] == NotificationTypeEnum.USER_CONFIRM_MERGE assert notifications['emits'][0]['kwargs']['destination_address'] == target_email + + @mock.patch('framework.sentry.log_exception') + def test_failed_merge_is_rolled_back(self, mock_log_exception): + other_user = UserFactory() + other_user.add_system_tag('other') + project = ProjectFactory(creator=other_user) + error = Exception('missing permissions') + + with mock.patch('osf.models.user.OSFUser._merge_users_preprints', side_effect=error): + with pytest.raises(Exception, match='missing permissions'): + self.user.merge_user(other_user) + + mock_log_exception.assert_called_once_with(error) + project.reload() + self.user.reload() + other_user.reload() + assert project.is_contributor(other_user) + assert not project.is_contributor(self.user) + assert project.creator == other_user + assert other_user.merged_by is None + assert not other_user.is_merged + assert 'other' not in self.user.system_tags + assert other_user.emails.filter(address=other_user.username).exists() + + def test_merge_users_task_notifies_initiator_on_failure(self): + other_user = UserFactory() + initiator = UserFactory() + + with mock.patch('osf.models.user.OSFUser.merge_user', side_effect=Exception('missing permissions')): + with capture_notifications() as notifications: + merge_users(self.user._id, other_user._id, initiator_guid=initiator._id) + + assert len(notifications['emits']) == 1 + assert notifications['emits'][0]['type'] == NotificationTypeEnum.USER_MERGE_FAILED_REPORT + assert notifications['emits'][0]['kwargs']['user'] == initiator + assert notifications['emits'][0]['kwargs']['event_context']['merger_guid'] == self.user._id + assert notifications['emits'][0]['kwargs']['event_context']['mergee_guid'] == other_user._id + assert 'missing permissions' in notifications['emits'][0]['kwargs']['event_context']['error'] + + def test_merge_users_task_does_not_notify_on_success(self): + other_user = UserFactory() + initiator = UserFactory() + + with capture_notifications(expect_none=True): + with override_flag(ENABLE_GV, active=True): + merge_users(self.user._id, other_user._id, initiator_guid=initiator._id) + + other_user.reload() + assert other_user.merged_by == self.user + + def _contributor_resources(self): + other_user = UserFactory() + project = ProjectFactory(creator=other_user) + preprint = PreprintFactory(creator=other_user) + draft_registration = DraftRegistrationFactory(initiator=other_user) + return other_user, [project, preprint, draft_registration] + + def test_merge_with_contributor_missing_permission_group(self): + other_user, resources = self._contributor_resources() + for resource in resources: + resource.add_contributor(self.user, permissions=READ, save=True) + resource.remove_permission(self.user, READ) + assert resource.contributor_set.get(user=self.user).permission is None + + self.user.merge_user(other_user) + + other_user.reload() + assert other_user.merged_by == self.user + for resource in resources: + assert resource.is_contributor(self.user) + assert not resource.is_contributor(other_user) + assert resource.contributor_set.get(user=self.user).permission == permissions.ADMIN + + def test_merge_with_merged_user_contributor_missing_permission_group(self): + other_user, resources = self._contributor_resources() + for resource in resources: + resource.add_contributor(self.user, permissions=WRITE, save=True) + resource.remove_permission(other_user, permissions.ADMIN) + assert resource.contributor_set.get(user=other_user).permission is None + + self.user.merge_user(other_user) + + other_user.reload() + assert other_user.merged_by == self.user + for resource in resources: + assert resource.is_contributor(self.user) + assert not resource.is_contributor(other_user) + assert resource.contributor_set.get(user=self.user).permission == WRITE + + @mock.patch('framework.sentry.log_message') + def test_merge_when_neither_user_has_permission_group(self, mock_log_message): + other_user, resources = self._contributor_resources() + for resource in resources: + resource.add_contributor(self.user, permissions=READ, save=True) + resource.remove_permission(self.user, READ) + resource.remove_permission(other_user, permissions.ADMIN) + + self.user.merge_user(other_user) + + other_user.reload() + assert other_user.merged_by == self.user + for resource in resources: + assert resource.is_contributor(self.user) + assert not resource.is_contributor(other_user) + assert resource.contributor_set.get(user=self.user).permission is None + assert mock_log_message.called + + def test_merge_user_only_contributor_with_missing_permission_group(self): + other_user, resources = self._contributor_resources() + for resource in resources: + resource.remove_permission(other_user, permissions.ADMIN) + + self.user.merge_user(other_user) + + other_user.reload() + assert other_user.merged_by == self.user + for resource in resources: + assert resource.is_contributor(self.user) + assert not resource.is_contributor(other_user) diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 540f38e8243..8b1bc9ed8e1 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -447,7 +447,7 @@ def test_merge_drafts(self, user): assert not draft_five.is_contributor(user2) @mock.patch('api.share.utils.update_share') - def test_merge_user_triggers_share_reindex(self, mock_update_share): + def test_merge_user_triggers_share_reindex(self, mock_update_share, django_capture_on_commit_callbacks): from osf.models import Preprint user = AuthUserFactory() @@ -461,7 +461,8 @@ def test_merge_user_triggers_share_reindex(self, mock_update_share): preprint_two = PreprintFactory(title='preprint_two') preprint_two.add_contributor(user2) - user.merge_user(user2) + with django_capture_on_commit_callbacks(execute=True): + user.merge_user(user2) # Verify update_share was called for both nodes nodes_reindexed = [ @@ -1526,22 +1527,24 @@ def test_dupe_email_is_appended(self, master, merge_dupe): assert master.emails.filter(address='joseph123@hotmail.com').exists() @mock.patch('website.mailchimp_utils.get_mailchimp_api') - def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe): + def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe, django_capture_on_commit_callbacks): dupe.mailchimp_mailing_lists['foo'] = True dupe.save() with capture_signals() as mock_signals: - merge_dupe() + with django_capture_on_commit_callbacks(execute=True): + merge_dupe() assert mock_signals.signals_sent() == {user_account_merged} @pytest.mark.enable_enqueue_task @mock.patch('website.mailchimp_utils.unsubscribe_mailchimp_async') @mock.patch('website.mailchimp_utils.get_mailchimp_api') - def test_merged_user_unsubscribed_from_mailing_lists(self, mock_mailchimp_api, mock_unsubscribe, dupe, merge_dupe, email_subscriptions_enabled): + def test_merged_user_unsubscribed_from_mailing_lists(self, mock_mailchimp_api, mock_unsubscribe, dupe, merge_dupe, email_subscriptions_enabled, django_capture_on_commit_callbacks): list_name = settings.MAILCHIMP_GENERAL_LIST dupe.mailchimp_mailing_lists[list_name] = True dupe.save() - merge_dupe() + with django_capture_on_commit_callbacks(execute=True): + merge_dupe() assert mock_unsubscribe.called def test_inherits_projects_contributed_by_dupe(self, dupe, master, merge_dupe): diff --git a/tests/test_utils.py b/tests/test_utils.py index 8fb60a89ade..789c01ad071 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -486,14 +486,15 @@ def mock_receiver(user, **kwargs): mock_publish_deactivated_user.assert_called_once_with(user) @mock.patch('osf.external.messages.celery_publishers.publish_merged_user') - def test_user_account_merged_signal(self, mock_publish_merged_user, user, old_user): + def test_user_account_merged_signal(self, mock_publish_merged_user, user, old_user, django_capture_on_commit_callbacks): # Connect a mock receiver to the signal for testing @receiver(user_account_merged) def mock_receiver(user, **kwargs): return mock_publish_merged_user(user) # Trigger the signal - user.merge_user(old_user) + with django_capture_on_commit_callbacks(execute=True): + user.merge_user(old_user) # Verify that the mock receiver was called mock_publish_merged_user.assert_called_once_with(old_user) @@ -549,10 +550,11 @@ def test_publish_body_on_merger( mock_publish_user_status_change, user, old_user, - account_status_changes_exchange + account_status_changes_exchange, + django_capture_on_commit_callbacks, ): with mock.patch.object(settings, 'USE_CELERY', True): - with override_flag(features.ENABLE_GV, active=True): + with override_flag(features.ENABLE_GV, active=True), django_capture_on_commit_callbacks(execute=True): user.merge_user(old_user) mock_publish_user_status_change().__enter__().publish.assert_called_once_with( diff --git a/website/templates/user_merge_failed_report.html.mako b/website/templates/user_merge_failed_report.html.mako new file mode 100644 index 00000000000..8f4fe5656d2 --- /dev/null +++ b/website/templates/user_merge_failed_report.html.mako @@ -0,0 +1,19 @@ +<%inherit file="notify_base.mako" /> + +<%def name="content()"> + + +

+ Merge of user ${mergee_guid} into ${merger_guid} failed +

+ + + + +

No changes were made to either account.

+% if error: +

Error: ${error}

+% endif + + + From 822dc57ca6a2091ae37172d8b6ff66f4be5f0a61 Mon Sep 17 00:00:00 2001 From: Vlad0n20 <137097005+Vlad0n20@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:22:02 +0200 Subject: [PATCH 06/22] Optimize query planning bottleneck in MonthlyOsfstorageFileCountReporter (#11945) ## Ticket https://openscience.atlassian.net/browse/ENG-12212 --- osf/metrics/reporters/__init__.py | 10 ++++---- .../reporters/osfstorage_file_count.py | 23 +++++++++++-------- ...058_preprint_deleted_spam_created_index.py | 21 +++++++++++++++++ osf/models/preprint.py | 3 +++ 4 files changed, 42 insertions(+), 15 deletions(-) create mode 100644 osf/migrations/0058_preprint_deleted_spam_created_index.py diff --git a/osf/metrics/reporters/__init__.py b/osf/metrics/reporters/__init__.py index 67db0b76bc9..0506c8e803a 100644 --- a/osf/metrics/reporters/__init__.py +++ b/osf/metrics/reporters/__init__.py @@ -9,11 +9,9 @@ from .institution_summary_monthly import InstitutionalSummaryMonthlyReporter from .new_user_domain import NewUserDomainReporter from .node_count import NodeCountReporter -# queries need optimizing before these run again (ENG-12211, follow-up ENG-12212) -# from .osfstorage_file_count import ( -# OsfstorageFileCountReporter, -# MonthlyOsfstorageFileCountReporter, -# ) +# daily reporter has the same query-planning issue as the monthly one and is unused; leave disabled +# from .osfstorage_file_count import OsfstorageFileCountReporter +from .osfstorage_file_count import MonthlyOsfstorageFileCountReporter from .preprint_count import PreprintCountReporter from .public_item_usage import PublicItemUsageReporter from .user_count import UserCountReporter @@ -41,4 +39,4 @@ class AllMonthlyReporters(enum.Enum): ITEM_USAGE = PublicItemUsageReporter PRIVATE_SPAM_METRICS = PrivateSpamMetricsReporter ANGULAR_SSR_METRICS = AngularSSRMetricsReporter - # OSFSTORAGE_FILE_COUNT = MonthlyOsfstorageFileCountReporter + OSFSTORAGE_FILE_COUNT = MonthlyOsfstorageFileCountReporter diff --git a/osf/metrics/reporters/osfstorage_file_count.py b/osf/metrics/reporters/osfstorage_file_count.py index 42a5b2dfaa4..205b9852b5d 100644 --- a/osf/metrics/reporters/osfstorage_file_count.py +++ b/osf/metrics/reporters/osfstorage_file_count.py @@ -27,6 +27,11 @@ def osfstorage_file_qs(node_queryset, preprint_queryset, *, created_before=None, only_public=False): """get a queryset for non-deleted osfstorage files belonging to a public, non-spam osf object + + built as two disjoint per-content-type queries unioned on pk, rather than one query + or'd across content types -- postgres cannot plan `(content_type=a and id in subq_a) + or (content_type=b and id in subq_b)` as two independent semi-joins; one branch ends + up re-executed as an unhashed per-row subplan (ENG-12211/ENG-12212). """ from addons.osfstorage.models import OsfStorageFile if only_public: @@ -35,18 +40,18 @@ def osfstorage_file_qs(node_queryset, preprint_queryset, *, created_before=None, if created_before: node_queryset = node_queryset.filter(created__lt=created_before) preprint_queryset = preprint_queryset.filter(created__lt=created_before) - _target_node_q = Q( - target_object_id__in=node_queryset.values('pk'), + + _base_file_qs = OsfStorageFile.objects.filter(deleted__isnull=True, purged__isnull=True) + _node_file_qs = _base_file_qs.filter( target_content_type=ContentType.objects.get_for_model(AbstractNode), + target_object_id__in=node_queryset.values('pk'), ) - _target_preprint_q = Q( - target_object_id__in=preprint_queryset.values('pk'), + _preprint_file_qs = _base_file_qs.filter( target_content_type=ContentType.objects.get_for_model(Preprint), + target_object_id__in=preprint_queryset.values('pk'), ) - _file_qs = ( - OsfStorageFile.objects - .filter(deleted__isnull=True, purged__isnull=True) - .filter(_target_node_q | _target_preprint_q) + _file_qs = OsfStorageFile.objects.filter( + pk__in=_node_file_qs.values('pk').union(_preprint_file_qs.values('pk')), ) if created_before: _file_qs = _file_qs.filter(created__lt=created_before) @@ -74,7 +79,7 @@ def report(self, **report_kwargs) -> cabc.Iterator[BaseMonthlyReport]: _month_end = self.yearmonth.month_end() _node_qs = ( AbstractNode.objects - .filter(deleted__isnull=True) + .filter(is_deleted=False) .exclude(spam_status=SpamStatus.SPAM) ) _preprint_qs = ( diff --git a/osf/migrations/0058_preprint_deleted_spam_created_index.py b/osf/migrations/0058_preprint_deleted_spam_created_index.py new file mode 100644 index 00000000000..9e5ed87796a --- /dev/null +++ b/osf/migrations/0058_preprint_deleted_spam_created_index.py @@ -0,0 +1,21 @@ +from django.contrib.postgres.operations import AddIndexConcurrently +from django.db import migrations, models + + +class Migration(migrations.Migration): + + atomic = False + + dependencies = [ + ('osf', '0057_report_spam_trigger'), + ] + + operations = [ + AddIndexConcurrently( + model_name='preprint', + index=models.Index( + fields=['deleted', 'spam_status', 'created'], + name='preprint_del_spam_created_idx', + ), + ), + ] diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 24616e83a76..1743f818783 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -338,6 +338,9 @@ class Meta: ('write_preprint', 'Can write the preprint'), ('admin_preprint', 'Can manage the preprint'), ) + indexes = [ + models.Index(fields=['deleted', 'spam_status', 'created'], name='preprint_del_spam_created_idx'), + ] def __unicode__(self): return '{} ({} preprint) (guid={}){}'.format(self.title, 'published' if self.is_published else 'unpublished', self._id, ' with supplemental files on ' + self.node.__unicode__() if self.node else '') From f6a9406a469ac6f15038f51a61e75af8c7223ec2 Mon Sep 17 00:00:00 2001 From: bodintsov Date: Wed, 14 Oct 2026 15:42:51 +0300 Subject: [PATCH 07/22] fix issue with unregistered contributor (#11968) ## Ticket https://openscience.atlassian.net/browse/ENG-12261 ## Purpose Fix issue with unregistered contributor --- tests/test_claim_views.py | 33 ++++++++++------------------ website/project/views/contributor.py | 15 ++++--------- 2 files changed, 15 insertions(+), 33 deletions(-) diff --git a/tests/test_claim_views.py b/tests/test_claim_views.py index ab0c4982db5..7059f029adf 100644 --- a/tests/test_claim_views.py +++ b/tests/test_claim_views.py @@ -291,20 +291,6 @@ def test_claim_user_form_redirects_to_password_confirm_page_if_user_is_logged_in ) assert res.request.path == expected - def test_user_logs_out_if_logged_in_user_is_the_claimed_user(self): - url = self.user.get_claim_url(self.project._primary_key) - g.current_session = None - self.app.set_cookie(settings.COOKIE_NAME, self.user.get_or_create_cookie().decode()) - res = self.app.get(url) - assert res.status_code == 302 - location = urlparse(res.location) - assert location.path == '/logout/' - return_url = parse_qs(location.query)['redirect_url'][0] - assert return_url.endswith(url) - res = self.app.get(res.location) - assert res.status_code == 302 - assert res.location == cas.get_logout_url(service_url=return_url) - @mock.patch('framework.auth.cas.make_response_from_ticket') def test_claim_user_when_user_is_registered_with_orcid(self, mock_response_from_ticket): # TODO: check in qa url encoding @@ -472,21 +458,24 @@ def test_claim_url_with_bad_token_returns_400(self): uid=self.user._id, token='badtoken', ) - res = self.app.get(url, auth=self.referrer.auth) + res = self.app.get(url, auth=AuthUserFactory().auth) assert res.status_code == 400 - def test_cannot_claim_user_with_user_who_is_already_contributor(self): - # user who is already a contirbutor to the project + def test_logged_in_contributor_is_logged_out_and_redirected_to_claim_form(self): + # user who is already a contributor to the project contrib = AuthUserFactory() self.project.add_contributor(contrib, auth=Auth(self.project.creator)) self.project.save() # Claiming user goes to claim url, but contrib is already logged in url = self.user.get_claim_url(self.project._primary_key) - res = self.app.get( - url, - auth=contrib.auth, follow_redirects=True) - # Response is a 400 - assert res.status_code == 400 + res = self.app.get(url, auth=contrib.auth) + assert res.status_code == 302 + res = self.app.get(res.location, auth=contrib.auth) + assert res.status_code == 302 + location = urlparse(res.location) + assert location.path == '/logout/' + return_url = parse_qs(location.query)['redirect_url'][0] + assert return_url.endswith(url) def test_claim_user_with_project_id_adds_corresponding_claimed_tag_to_user(self): assert OsfClaimedTags.Osf.value not in self.user.system_tags diff --git a/website/project/views/contributor.py b/website/project/views/contributor.py index 89093d1fa17..2a9090f8e09 100644 --- a/website/project/views/contributor.py +++ b/website/project/views/contributor.py @@ -715,7 +715,7 @@ def claim_user_registered(auth, node, **kwargs): View that prompts user to enter their password in order to claim being a contributor on a project. A user must be logged in. """ - + uid, pid, token = kwargs['uid'], kwargs['pid'], kwargs['token'] current_user = auth.user current_session = get_session() @@ -723,14 +723,10 @@ def claim_user_registered(auth, node, **kwargs): if not current_user: return redirect(sign_out_url) - # Logged in user should not be a contributor the project + # Logged in user should not be a contributor the project. If logged in user, log out user and redirect to claim form if hasattr(node, 'is_contributor') and node.is_contributor(current_user): - data = { - 'message_short': 'Already a contributor', - 'message_long': ('The logged-in user is already a contributor to this ' - 'project. Would you like to log out?').format(sign_out_url) - } - raise HTTPError(http_status.HTTP_400_BAD_REQUEST, data=data) + claim_form_url = web_url_for('claim_user_form', uid=uid, pid=pid, token=token, _absolute=True) + return redirect(web_url_for('auth_logout', redirect_url=claim_form_url)) # Logged in user is already a member of the OSF Group if hasattr(node, 'is_member') and node.is_member(current_user): @@ -741,7 +737,6 @@ def claim_user_registered(auth, node, **kwargs): } raise HTTPError(http_status.HTTP_400_BAD_REQUEST, data=data) - uid, pid, token = kwargs['uid'], kwargs['pid'], kwargs['token'] unreg_user = OSFUser.load(uid) if not verify_claim_token(unreg_user, token, pid=node._primary_key): error_data = { @@ -832,8 +827,6 @@ def claim_user_form(auth, **kwargs): # If user is logged in, redirect to 're-enter password' page if auth.logged_in: - if auth.user._id == uid: - return redirect(web_url_for('auth_logout', redirect_url=request.url)) return redirect(web_url_for('claim_user_registered', uid=uid, pid=pid, token=token)) From f84610539a30e1e17992983554a395921be7aa3b Mon Sep 17 00:00:00 2001 From: mkovalua Date: Wed, 14 Oct 2026 15:45:04 +0300 Subject: [PATCH 08/22] Do not show rejected registrations for submitted tab (#11970) ## Ticket https://openscience.atlassian.net/browse/ENG-12060?focusedCommentId=123788 ## Purpose Do not show rejected registrations for submitted tab --- api/nodes/serializers.py | 6 ++- api/nodes/views.py | 6 ++- api/users/serializers.py | 6 ++- api/users/views.py | 6 ++- .../registries_moderation/test_submissions.py | 38 +++++++++++++++++++ 5 files changed, 57 insertions(+), 5 deletions(-) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index 807e9e98aea..4a89727083b 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -45,6 +45,7 @@ from website.project import new_private_link from website.project.model import NodeUpdateError from osf.utils import permissions as osf_permissions +from osf.utils.workflows import RegistrationModerationStates class RegistrationProviderRelationshipField(RelationshipField): @@ -703,8 +704,9 @@ def get_contrib_count(self, obj): def get_registration_count(self, obj): auth = get_user_auth(self.context['request']) - registrations = [node for node in obj.registrations_all if node.can_view(auth)] - return len(registrations) + # registrations rejected by a moderator are not listed + registrations = obj.registrations_all.exclude(moderation_state=RegistrationModerationStates.REJECTED.db_name) + return len([node for node in registrations if node.can_view(auth)]) def get_draft_registration_count(self, obj): auth = get_user_auth(self.context['request']) diff --git a/api/nodes/views.py b/api/nodes/views.py index bb38bcbab3c..4fc243ce44b 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -173,6 +173,7 @@ ) from addons.osfstorage.models import Region from osf.utils.permissions import ADMIN, WRITE_NODE +from osf.utils.workflows import RegistrationModerationStates from website import settings # This is used to rethrow v1 exceptions as v2 @@ -776,7 +777,10 @@ def get_serializer_class(self): # overrides ListCreateAPIView # TODO: Filter out withdrawals by default def get_queryset(self): - nodes = self.get_node().registrations_all.annotate( + # Registrations rejected by a moderator are returned to contributors as drafts, so they are not listed here + nodes = self.get_node().registrations_all.exclude( + moderation_state=RegistrationModerationStates.REJECTED.db_name, + ).annotate( revision_state=registration_annotations.REVISION_STATE, **resource_annotations.make_open_practice_badge_annotations(), ) diff --git a/api/users/serializers.py b/api/users/serializers.py index fb9fd777a85..5f7a4c6be6e 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -38,6 +38,7 @@ from osf.models.user_message import MessageTypes from osf.models.provider import AbstractProviderGroupObjectPermission from osf.utils.requests import string_type_request_headers +from osf.utils.workflows import RegistrationModerationStates from website import settings, mailchimp_utils from website.settings import MAILCHIMP_GENERAL_LIST, OSF_HELP_LIST, CONFIRM_REGISTRATIONS_BY_EMAIL from website.util import api_v2_url @@ -228,7 +229,10 @@ def get_node_count(self, obj): def get_registration_count(self, obj): auth = get_user_auth(self.context['request']) - user_registration = default_node_list_queryset(model_cls=Registration).filter(contributor__user__id=obj.id) + # registrations rejected by a moderator are not listed + user_registration = default_node_list_queryset(model_cls=Registration).filter( + contributor__user__id=obj.id, + ).exclude(moderation_state=RegistrationModerationStates.REJECTED.db_name) return user_registration.can_view(user=auth.user, private_link=auth.private_link).count() def get_preprint_count(self, obj): diff --git a/api/users/views.py b/api/users/views.py index ec1f6b0dfd0..2672e6ec0f6 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -106,6 +106,7 @@ ) from osf.utils.tokens import TokenHandler from osf.utils.tokens.handlers import sanction_handler +from osf.utils.workflows import RegistrationModerationStates from website import settings, language from website.project.views.contributor import send_claim_email, send_claim_registered_email from website.util.metrics import CampaignClaimedTags, CampaignSourceTags @@ -485,7 +486,10 @@ def get_default_queryset(self): **resource_annotations.make_open_practice_badge_annotations(), ) # OSF group members not copied to registration. Only registration contributors need to be checked here. - return qs.filter(contributor__user__id=user.id) + # Registrations rejected by a moderator are returned to contributors as drafts, so they are not listed here. + return qs.filter(contributor__user__id=user.id).exclude( + moderation_state=RegistrationModerationStates.REJECTED.db_name, + ) # overrides ListAPIView def get_queryset(self): diff --git a/api_tests/registries_moderation/test_submissions.py b/api_tests/registries_moderation/test_submissions.py index abc85777595..0a423a73944 100644 --- a/api_tests/registries_moderation/test_submissions.py +++ b/api_tests/registries_moderation/test_submissions.py @@ -408,6 +408,44 @@ def test_rejected_submission_draft_is_returned_for_resubmission(self, app, regis assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name assert not registration.is_deleted + def test_rejected_submission_is_not_listed_for_contributors(self, app, registration, reg_creator, moderator, provider, registration_actions_url, actions_payload_base): + user_registrations_url = f'/{API_BASE}users/{reg_creator._id}/registrations/' + node_registrations_url = f'/{API_BASE}nodes/{registration.registered_from._id}/registrations/' + rejected_tab_url = f'/{API_BASE}providers/registrations/{provider._id}/registrations/?filter[reviews_state]=rejected' + + def listed_ids(url, user): + return [entry['id'] for entry in app.get(url, auth=user.auth).json['data']] + + def registration_count(url, user): + resp = app.get(f'{url}?related_counts=True', auth=user.auth) + return resp.json['data']['relationships']['registrations']['links']['related']['meta']['count'] + + def assert_counts_match_lists(): + user_url = f'/{API_BASE}users/{reg_creator._id}/' + node_url = f'/{API_BASE}nodes/{registration.registered_from._id}/' + assert registration_count(user_url, reg_creator) == len(listed_ids(user_registrations_url, reg_creator)) + assert registration_count(node_url, reg_creator) == len(listed_ids(node_registrations_url, reg_creator)) + + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + assert registration._id in listed_ids(user_registrations_url, reg_creator) + assert registration._id in listed_ids(node_registrations_url, reg_creator) + assert_counts_match_lists() + + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REJECT_SUBMISSION.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + with capture_notifications(): + resp = app.post_json_api(registration_actions_url, actions_payload_base, auth=moderator.auth) + assert resp.status_code == 201 + + # Contributors get the draft back instead of seeing the rejected registration among submitted + assert registration._id not in listed_ids(user_registrations_url, reg_creator) + assert registration._id not in listed_ids(node_registrations_url, reg_creator) + assert_counts_match_lists() + # Moderators still see it under the Rejected tab + assert registration._id in listed_ids(rejected_tab_url, moderator) + def test_registries_moderation_post_embargo(self, app, embargo_registration, moderator, provider, embargo_registration_actions_url, actions_payload_base, reg_creator): assert embargo_registration.moderation_state == RegistrationModerationStates.INITIAL.db_name with capture_notifications(): From b82393bd8a54d4f47a5c1b77de5fdb60b57c1bd2 Mon Sep 17 00:00:00 2001 From: antkryt Date: Mon, 19 Oct 2026 19:25:45 +0300 Subject: [PATCH 09/22] [ENG-12491] Investigate and integrate dj-celery-panel - Part 1 (#11974) --- admin/base/apps.py | 40 +++++++ admin/base/celery_panel.py | 75 +++++++++++++ admin/base/settings/defaults.py | 9 ++ admin/base/urls.py | 1 + .../admin/dj_celery_panel/queue_detail.html | 32 ++++++ admin/templates/register.html | 1 + framework/celery_tasks/__init__.py | 2 + osf/migrations/__init__.py | 12 ++ poetry.lock | 105 ++++++++++++------ pyproject.toml | 1 + 10 files changed, 245 insertions(+), 33 deletions(-) create mode 100644 admin/base/celery_panel.py create mode 100644 admin/templates/admin/dj_celery_panel/queue_detail.html diff --git a/admin/base/apps.py b/admin/base/apps.py index ba73adc346b..d573377ef30 100644 --- a/admin/base/apps.py +++ b/admin/base/apps.py @@ -4,3 +4,43 @@ class BaseAdminAppConfig(BaseAppConfig): name = 'admin.base' label = 'admin_base' + + def ready(self): + from django.contrib import admin + from django.core.exceptions import PermissionDenied + from dj_control_room_base.models import BasePanelPlaceholder + from django_celery_beat.admin import PeriodicTaskAdmin + from django_celery_beat.models import ( + ClockedSchedule, + CrontabSchedule, + IntervalSchedule, + PeriodicTask, + SolarSchedule, + ) + + # dj_control_room_base is installed only for dj_celery_panel's templates/static; hide its demo panel + admin.site.unregister(BasePanelPlaceholder) + + # Schedules are managed through CeleryConfig.beat_schedule, so only the periodic task list is exposed + admin.site.unregister((ClockedSchedule, CrontabSchedule, IntervalSchedule, SolarSchedule)) + + # Read-only list for Periodic Tasks admin panel: no actions, add, change or delete pages + class ReadOnlyPeriodicTaskAdmin(PeriodicTaskAdmin): + actions = None + list_display_links = None + + def has_add_permission(self, request): + return False + + def has_change_permission(self, request, obj=None): + return False + + def has_delete_permission(self, request, obj=None): + return False + + def change_view(self, request, object_id, form_url='', extra_context=None): + # the read-only view page still builds beat's form, which fails in admin + raise PermissionDenied + + admin.site.unregister(PeriodicTask) + admin.site.register(PeriodicTask, ReadOnlyPeriodicTaskAdmin) diff --git a/admin/base/celery_panel.py b/admin/base/celery_panel.py new file mode 100644 index 00000000000..67280d19f8a --- /dev/null +++ b/admin/base/celery_panel.py @@ -0,0 +1,75 @@ +from dj_celery_panel.celery_utils import CeleryQueuesInspectBackend, QueueDetailPage, QueueListPage + + +class RoutedTasksQueuesBackend(CeleryQueuesInspectBackend): + """dj-celery-panel queues backend that also lists the tasks routed to each queue + and the configured queues that no active worker consumes. + + Routes are resolved the same way Celery does when sending a task, so explicit + queue= overrides passed to apply_async() at call time are not reflected. + """ + + BACKEND_DESCRIPTION = 'Queue bindings, routing configuration, routed tasks and unconsumed queues' + + def get_queues(self): + page = super().get_queues() + # Workers report only the queues they consume, so a configured queue nobody consumes + # would be missing from the list while tasks routed to it pile up + listed = {queue['name'] for queue in page.queues} + unconsumed = [ + self.describe_unconsumed_queue(queue) + for name, queue in sorted(self.app.amqp.queues.items()) + if name not in listed + ] + return QueueListPage(queues=page.queues + unconsumed, error=page.error) + + def get_queue_detail(self, queue_name): + page = super().get_queue_detail(queue_name) + queue, error = page.queue, page.error + if queue is None: + # membership check first: indexing Queues creates missing ones + if queue_name not in self.app.amqp.queues: + return page + queue = self.describe_unconsumed_queue(self.app.amqp.queues[queue_name]) + error = 'No active worker consumes this queue' + routed_tasks = self.get_routed_tasks(queue_name, self.get_task_names(queue['workers'])) + return QueueDetailPage( + queue={**queue, 'routed_tasks': routed_tasks}, + error=error, + ) + + def describe_unconsumed_queue(self, queue): + broker_info = self._get_queue_length_from_broker(queue.name) + return { + 'name': queue.name, + 'exchange': queue.exchange.name, + 'exchange_type': queue.exchange.type, + 'routing_key': queue.routing_key, + 'durable': queue.durable, + 'auto_delete': queue.auto_delete, + 'exclusive': queue.exclusive, + 'arguments': queue.queue_arguments, + 'workers': [], + 'worker_details': [], + 'message_count': broker_info.get('length'), + 'consumer_count': broker_info.get('consumer_count'), + 'broker_query_error': broker_info.get('error'), + } + + def get_task_names(self, workers): + # The admin process registers only the task modules it happens to import, + # so ask the workers for the tasks they have registered: the ones consuming + # this queue, or all of them when nobody consumes it + registered = self.app.control.inspect(destination=workers or None).registered() or {} + names = {name for worker_tasks in registered.values() for name in worker_tasks} + return {name for name in names | set(self.app.tasks) if not name.startswith('celery.')} + + def get_routed_tasks(self, queue_name, task_names): + router = self.app.amqp.router + routed_tasks = [] + for name in task_names: + task = self.app.tasks.get(name) + options = dict(task._get_exec_options()) if task else {} + if router.route(options, name)['queue'].name == queue_name: + routed_tasks.append(name) + return sorted(routed_tasks) diff --git a/admin/base/settings/defaults.py b/admin/base/settings/defaults.py index c255605fd55..98a970c4a00 100644 --- a/admin/base/settings/defaults.py +++ b/admin/base/settings/defaults.py @@ -80,7 +80,10 @@ 'django.contrib.staticfiles', # 3rd party + 'django_celery_beat', 'django_celery_results', + 'dj_control_room_base', + 'dj_celery_panel', 'webpack_loader', 'guardian', 'waffle', @@ -108,6 +111,12 @@ ) +DJ_CELERY_PANEL_SETTINGS = { + 'queues_backend': 'admin.base.celery_panel.RoutedTasksQueuesBackend', + # Superusers always have access; users in these groups must also be staff + 'ALLOWED_GROUPS': ['osf_admin', 'celery_panel'], +} + MIGRATION_MODULES = { 'osf': None, 'reviews': None, diff --git a/admin/base/urls.py b/admin/base/urls.py index 1eb840bc362..3d3c65fa6dc 100644 --- a/admin/base/urls.py +++ b/admin/base/urls.py @@ -12,6 +12,7 @@ base_pattern, include([ re_path(r'^$', views.home, name='home'), + re_path(r'^admin/dj-celery-panel/', include('dj_celery_panel.urls')), re_path(r'^admin/', admin.site.urls), re_path(r'^provider_asset_files/', include('admin.provider_asset_files.urls', namespace='provider_asset_files')), re_path(r'^institution_asset_files/', include('admin.institution_asset_files.urls', namespace='institution_asset_files')), diff --git a/admin/templates/admin/dj_celery_panel/queue_detail.html b/admin/templates/admin/dj_celery_panel/queue_detail.html new file mode 100644 index 00000000000..72765cc400d --- /dev/null +++ b/admin/templates/admin/dj_celery_panel/queue_detail.html @@ -0,0 +1,32 @@ +{% extends "admin/dj_celery_panel/queue_detail.html" %} +{% load i18n %} + +{% block panel_page_content %} +{{ block.super }} +{% if queue %} +
+
+
{% trans "Routed Tasks" %}
+
+ {% if queue.routed_tasks %} + {{ queue.routed_tasks|length }} {% trans "tasks" %} +
+ {% trans "Show routed tasks" %} + +
+ {% else %} + {% trans "No tasks are routed to this queue" %} + {% endif %} +
+
+
+{% endif %} +{% endblock %} diff --git a/admin/templates/register.html b/admin/templates/register.html index de587bb552a..4a445d6232f 100644 --- a/admin/templates/register.html +++ b/admin/templates/register.html @@ -36,6 +36,7 @@

Select Groups for Permissions

metrics_only:
User can only view metrics on the OSF Admin page

read_only
User can view details of nodes, preprints, users, meetings, metrics, and spam, but cannot make changes

osf_admin
User make changes to nodes, preprints, users, meetings, and mark spam

+

celery_panel
User can only view Celery monitoring pages in Django admin

{{ form.group_perms }}

{% if form.group_perms.errors %} {{ form.group_perms.errors }}{% endif %} diff --git a/framework/celery_tasks/__init__.py b/framework/celery_tasks/__init__.py index 67e59564c9e..ac68d10f192 100644 --- a/framework/celery_tasks/__init__.py +++ b/framework/celery_tasks/__init__.py @@ -11,6 +11,8 @@ app = Celery() app.config_from_object(CeleryConfig) +# Make this app the fallback for celery.current_app in every thread, not only the importing one +app.set_default() if SENTRY_DSN: init( diff --git a/osf/migrations/__init__.py b/osf/migrations/__init__.py index 6118efdd0c6..c65a4733c44 100644 --- a/osf/migrations/__init__.py +++ b/osf/migrations/__init__.py @@ -155,6 +155,18 @@ def update_admin_permissions(verbosity=0): metrics_group.permissions.add(metrics_permission) metrics_group.save() + # Add a celery_panel Group for users who may only monitor Celery in Django admin; + # dj-celery-panel pages are allowed for it and osf_admin via DJ_CELERY_PANEL_SETTINGS + celery_panel_group, created = Group.objects.get_or_create(name='celery_panel') + if created and should_log: + logger.info('Celery panel group created') + periodic_task_permission = Permission.objects.get( + content_type__app_label='django_celery_beat', + codename='view_periodictask', + ) + celery_panel_group.permissions.add(periodic_task_permission) + admin_group.permissions.add(periodic_task_permission) + def update_provider_auth_groups(verbosity=0): # TODO: determine efficient way to only do this if perms change diff --git a/poetry.lock b/poetry.lock index 6e17b4ae053..2f773f6340c 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 2.4.1 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.5.1 and should not be changed by hand. [[package]] name = "amqp" @@ -287,7 +287,7 @@ files = [ [package.dependencies] jmespath = ">=0.7.1,<2.0.0" python-dateutil = ">=2.1,<3.0.0" -urllib3 = {version = ">=1.25.4,<2.2.0 || >2.2.0,<3", markers = "python_version >= \"3.10\""} +urllib3 = {version = ">=1.25.4,!=2.2.0,<3", markers = "python_version >= \"3.10\""} [package.extras] crt = ["awscrt (==0.21.2)"] @@ -965,6 +965,45 @@ files = [ {file = "distlib-0.3.8.tar.gz", hash = "sha256:1530ea13e350031b6312d8580ddb6b27a104275a31106523b8f123787f494f64"}, ] +[[package]] +name = "dj-celery-panel" +version = "0.6.0" +description = "A Django Admin panel for browsing and inspecting Celery tasks and workers" +optional = false +python-versions = ">=3.9" +groups = ["main"] +files = [ + {file = "dj_celery_panel-0.6.0-py3-none-any.whl", hash = "sha256:b059af9edaedb64015357a0f520d389543dcf7e125495cbc8a3572c61afb9f8b"}, + {file = "dj_celery_panel-0.6.0.tar.gz", hash = "sha256:de915a8880474954f59f8af0a5462b8f647539c96f271ac26f22ab897218401d"}, +] + +[package.dependencies] +dj-control-room-base = ">=1.5.0" +Django = ">=4.2" + +[package.extras] +build = ["build (>=1.0.0)", "twine (>=4.0.0)"] +dev = ["django-redis (>=5.0.0)", "mkdocs-material (>=9.1.12)", "psycopg2-binary (>=2.9.0)", "pytest (>=7.0.0)", "pytest-cov (>=4.0.0)", "pytest-django (>=4.5.0)", "pytest-xdist (>=3.2.0)"] + +[[package]] +name = "dj-control-room-base" +version = "1.6.0" +description = "Core framework for Django Control Room panels" +optional = false +python-versions = ">=3.9" +groups = ["main"] +files = [ + {file = "dj_control_room_base-1.6.0-py3-none-any.whl", hash = "sha256:54e74e8655ff077f429722b956b2c574e5e5fb390da0882cfb41ebce1cab1551"}, + {file = "dj_control_room_base-1.6.0.tar.gz", hash = "sha256:250c11296baa7e84e4ea988783f70825598523d3fa30f8447db937dabb76b338"}, +] + +[package.dependencies] +Django = ">=4.2" + +[package.extras] +build = ["build (>=1.0.0)", "twine (>=4.0.0)"] +dev = ["django-redis (>=5.0.0)", "mkdocs-material (>=9.1.12)", "psycopg2-binary (>=2.9.0)", "pytest (>=7.0.0)", "pytest-cov (>=4.0.0)", "pytest-django (>=4.5.0)", "pytest-xdist (>=3.2.0)"] + [[package]] name = "django" version = "4.2.30" @@ -1155,7 +1194,7 @@ files = [ [package.dependencies] Django = ">=3.2" -redis = ">=3,<4.0.0 || >4.0.0,<4.0.1 || >4.0.1" +redis = ">=3,!=4.0.0,!=4.0.1" [package.extras] hiredis = ["redis[hiredis] (>=3,!=4.0.0,!=4.0.1)"] @@ -1721,16 +1760,16 @@ files = [ ] [package.dependencies] -google-auth = ">=2.14.1,<3.0.dev0" -googleapis-common-protos = ">=1.56.2,<2.0.dev0" -proto-plus = ">=1.22.3,<2.0.0.dev0" -protobuf = ">=3.19.5,<3.20.0 || >3.20.0,<3.20.1 || >3.20.1,<4.21.0 || >4.21.0,<4.21.1 || >4.21.1,<4.21.2 || >4.21.2,<4.21.3 || >4.21.3,<4.21.4 || >4.21.4,<4.21.5 || >4.21.5,<6.0.0.dev0" -requests = ">=2.18.0,<3.0.0.dev0" +google-auth = ">=2.14.1,<3.0" +googleapis-common-protos = ">=1.56.2,<2.0" +proto-plus = ">=1.22.3,<2.0.0" +protobuf = ">=3.19.5,!=3.20.0,!=3.20.1,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5,<6.0.0" +requests = ">=2.18.0,<3.0.0" [package.extras] -grpc = ["grpcio (>=1.33.2,<2.0.dev0)", "grpcio (>=1.49.1,<2.0.dev0) ; python_version >= \"3.11\"", "grpcio-status (>=1.33.2,<2.0.dev0)", "grpcio-status (>=1.49.1,<2.0.dev0) ; python_version >= \"3.11\""] -grpcgcp = ["grpcio-gcp (>=0.2.2,<1.0.dev0)"] -grpcio-gcp = ["grpcio-gcp (>=0.2.2,<1.0.dev0)"] +grpc = ["grpcio (>=1.33.2,<2.0)", "grpcio (>=1.49.1,<2.0) ; python_version >= \"3.11\"", "grpcio-status (>=1.33.2,<2.0)", "grpcio-status (>=1.49.1,<2.0) ; python_version >= \"3.11\""] +grpcgcp = ["grpcio-gcp (>=0.2.2,<1.0)"] +grpcio-gcp = ["grpcio-gcp (>=0.2.2,<1.0)"] [[package]] name = "google-api-python-client" @@ -1745,10 +1784,10 @@ files = [ ] [package.dependencies] -google-api-core = ">=1.31.5,<2.0.dev0 || >2.3.0,<3.0.0.dev0" -google-auth = ">=1.19.0,<3.0.0.dev0" +google-api-core = ">=1.31.5,<2.0 || >=2.3.dev0,!=2.3.0,<3.0.0" +google-auth = ">=1.19.0,<3.0.0" google-auth-httplib2 = ">=0.1.0" -httplib2 = ">=0.15.0,<1.dev0" +httplib2 = ">=0.15.0,<1" uritemplate = ">=3.0.1,<5" [[package]] @@ -1769,11 +1808,11 @@ pyasn1-modules = ">=0.2.1" rsa = ">=3.1.4,<5" [package.extras] -aiohttp = ["aiohttp (>=3.6.2,<4.0.0.dev0)", "requests (>=2.20.0,<3.0.0.dev0)"] +aiohttp = ["aiohttp (>=3.6.2,<4.0.0)", "requests (>=2.20.0,<3.0.0)"] enterprise-cert = ["cryptography (==36.0.2)", "pyopenssl (==22.0.0)"] pyopenssl = ["cryptography (>=38.0.3)", "pyopenssl (>=20.0.0)"] reauth = ["pyu2f (>=0.1.5)"] -requests = ["requests (>=2.20.0,<3.0.0.dev0)"] +requests = ["requests (>=2.20.0,<3.0.0)"] [[package]] name = "google-auth-httplib2" @@ -1804,11 +1843,11 @@ files = [ ] [package.dependencies] -google-api-core = ">=1.31.6,<2.0.dev0 || >2.3.0,<3.0.0.dev0" -google-auth = ">=1.25.0,<3.0.dev0" +google-api-core = ">=1.31.6,<2.0 || >=2.3.dev0,!=2.3.0,<3.0.0" +google-auth = ">=1.25.0,<3.0" [package.extras] -grpc = ["grpcio (>=1.38.0,<2.0.dev0)", "grpcio-status (>=1.38.0,<2.0.dev0)"] +grpc = ["grpcio (>=1.38.0,<2.0)", "grpcio-status (>=1.38.0,<2.0)"] [[package]] name = "google-cloud-storage" @@ -1823,15 +1862,15 @@ files = [ ] [package.dependencies] -google-api-core = ">=2.15.0,<3.0.0.dev0" -google-auth = ">=2.26.1,<3.0.dev0" -google-cloud-core = ">=2.3.0,<3.0.dev0" -google-crc32c = ">=1.0,<2.0.dev0" +google-api-core = ">=2.15.0,<3.0.0" +google-auth = ">=2.26.1,<3.0" +google-cloud-core = ">=2.3.0,<3.0" +google-crc32c = ">=1.0,<2.0" google-resumable-media = ">=2.6.0" -requests = ">=2.18.0,<3.0.0.dev0" +requests = ">=2.18.0,<3.0.0" [package.extras] -protobuf = ["protobuf (<5.0.0.dev0)"] +protobuf = ["protobuf (<5.0.0)"] [[package]] name = "google-crc32c" @@ -1886,11 +1925,11 @@ files = [ ] [package.dependencies] -google-crc32c = ">=1.0,<2.0.dev0" +google-crc32c = ">=1.0,<2.0" [package.extras] -aiohttp = ["aiohttp (>=3.6.2,<4.0.0.dev0)", "google-auth (>=1.22.0,<2.0.dev0)"] -requests = ["requests (>=2.18.0,<3.0.0.dev0)"] +aiohttp = ["aiohttp (>=3.6.2,<4.0.0)", "google-auth (>=1.22.0,<2.0)"] +requests = ["requests (>=2.18.0,<3.0.0)"] [[package]] name = "googleapis-common-protos" @@ -1905,10 +1944,10 @@ files = [ ] [package.dependencies] -protobuf = ">=3.20.2,<4.21.1 || >4.21.1,<4.21.2 || >4.21.2,<4.21.3 || >4.21.3,<4.21.4 || >4.21.4,<4.21.5 || >4.21.5,<6.0.0.dev0" +protobuf = ">=3.20.2,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5,<6.0.0" [package.extras] -grpc = ["grpcio (>=1.44.0,<2.0.0.dev0)"] +grpc = ["grpcio (>=1.44.0,<2.0.0)"] [[package]] name = "gprof2dot" @@ -2038,7 +2077,7 @@ files = [ ] [package.dependencies] -pyparsing = {version = ">=2.4.2,<3.0.0 || >3.0.0,<3.0.1 || >3.0.1,<3.0.2 || >3.0.2,<3.0.3 || >3.0.3,<4", markers = "python_version > \"3.0\""} +pyparsing = {version = ">=2.4.2,!=3.0.0,!=3.0.1,!=3.0.2,!=3.0.3,<4", markers = "python_version > \"3.0\""} [[package]] name = "identify" @@ -3026,7 +3065,7 @@ files = [ ] [package.dependencies] -protobuf = ">=3.19.0,<6.0.0.dev0" +protobuf = ">=3.19.0,<6.0.0" [package.extras] testing = ["google-api-core (>=1.31.5)"] @@ -4725,4 +4764,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.1" python-versions = "^3.12" -content-hash = "67f0c792607999f59d517f7d855c2d51fb2146d8314c84e14c4f8714253a909c" +content-hash = "4e57c6af0b86e4cac001ccf6aae63657f193ec215720a84e21706cdb9483a593" diff --git a/pyproject.toml b/pyproject.toml index 9979ca9b554..d4866537b02 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -64,6 +64,7 @@ hashids = "1.3.1" pyjwt = "2.8.0" django-celery-beat = "2.6.0" django-celery-results = "2.5.1" +dj-celery-panel = "0.6.0" django-timezone-field = "6.1.0" # pin to delay untested major version bump pyjwe = "1.0.0" # Required by pyjwe and ndg-httpsclient From f2baabad07fe7c2a1600cb599d363bf6c1d79d04 Mon Sep 17 00:00:00 2001 From: mkovalua Date: Tue, 20 Oct 2026 19:57:39 +0300 Subject: [PATCH 10/22] [ENG-10043] make to be possible to edit metadata on previous versions of a preprint (#11973) ## Ticket https://openscience.atlassian.net/browse/ENG-10043 ## Purpose Since metadata is distinct for different versions of a preprint, there should be the ability to edit the metadata on each version of a preprint. This is important to be able to correct any mistakes or unintentionally excluded in the metadata at later dates (including author order, funder information, licensing, etc.) The Edit option is only available on the most recent version of a preprint. ## Changes make to be possible to edit metadata on previous versions of a preprint related FE changings PR https://github.com/CenterForOpenScience/angular-osf/pull/1078 --- api/preprints/views.py | 44 +--- .../preprints/views/test_preprint_versions.py | 221 +++++++++++++++++- 2 files changed, 224 insertions(+), 41 deletions(-) diff --git a/api/preprints/views.py b/api/preprints/views.py index 415018ca874..0b3869d39db 100644 --- a/api/preprints/views.py +++ b/api/preprints/views.py @@ -74,36 +74,6 @@ from api.base.metrics import UsageMetricsViewMixin -class PreprintOldVersionsImmutableMixin: - """Override method to reject modify requests for old preprint versions (except for withdrawal)""" - - @staticmethod - def is_edit_allowed(preprint): - if preprint.is_latest_version or preprint.machine_state == DefaultStates.INITIAL.value: - return True - if preprint.provider.reviews_workflow == Workflows.PRE_MODERATION.value: - if preprint.machine_state == DefaultStates.PENDING.value or preprint.machine_state == DefaultStates.REJECTED.value: - return True - return False - - def handle_request(self, request, method, *args, **kwargs): - preprint = self.get_preprint(check_object_permissions=False) - if PreprintOldVersionsImmutableMixin.is_edit_allowed(preprint): - return method(request, *args, **kwargs) - message = f'User cannot edit previous versions of a preprint: [_id={preprint._id}]' - sentry.log_message(message) - raise Conflict(detail=message) - - def update(self, request, *args, **kwargs): - return self.handle_request(request, super().update, *args, **kwargs) - - def create(self, request, *args, **kwargs): - return self.handle_request(request, super().create, *args, **kwargs) - - def destroy(self, request, *args, **kwargs): - return self.handle_request(request, super().destroy, *args, **kwargs) - - class PreprintMixin(NodeMixin): serializer_class = PreprintSerializer preprint_lookup_url_kwarg = 'preprint_id' @@ -266,7 +236,7 @@ def create(self, request, *args, **kwargs): return super().create(request, *args, **kwargs) -class PreprintDetail(PreprintOldVersionsImmutableMixin, UsageMetricsViewMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin, WaterButlerMixin): +class PreprintDetail(UsageMetricsViewMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin, WaterButlerMixin): """See [documentation for this endpoint](https://developer.osf.io/#operation/preprints_read). Note: The resource now exposes a `versions` relationship pointing to @@ -307,7 +277,7 @@ def get_parser_context(self, http_request): res['legacy_type_allowed'] = True return res -class PreprintNodeRelationship(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin): +class PreprintNodeRelationship(JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin): permission_classes = ( drf_permissions.IsAuthenticatedOrReadOnly, base_permissions.TokenHasScope, @@ -335,7 +305,7 @@ def get_object(self): return obj -class PreprintCitationDetail(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveAPIView, PreprintMixin): +class PreprintCitationDetail(JSONAPIBaseView, generics.RetrieveAPIView, PreprintMixin): """See [documentation for this endpoint](https://developer.osf.io/#operation/preprints_citation_list). """ permission_classes = ( @@ -445,7 +415,7 @@ def get_object(self, check_object_permissions=True): return self.get_preprint(check_object_permissions=check_object_permissions) -class PreprintContributorsList(PreprintOldVersionsImmutableMixin, NodeContributorsList, PreprintMixin): +class PreprintContributorsList(NodeContributorsList, PreprintMixin): permission_classes = ( AdminOrPublic, drf_permissions.IsAuthenticatedOrReadOnly, @@ -489,7 +459,7 @@ def get_serializer_context(self): return context -class PreprintContributorDetail(PreprintOldVersionsImmutableMixin, NodeContributorDetail, PreprintMixin): +class PreprintContributorDetail(NodeContributorDetail, PreprintMixin): permission_classes = ( ContributorDetailPermissions, @@ -581,7 +551,7 @@ def get_resource(self): return self.get_preprint() -class PreprintSubjectsRelationship(PreprintOldVersionsImmutableMixin, SubjectRelationshipBaseView, PreprintMixin): +class PreprintSubjectsRelationship(SubjectRelationshipBaseView, PreprintMixin): """See [documentation for this endpoint](https://developer.osf.io/#operation/preprint_subjects_list). """ permission_classes = ( @@ -801,7 +771,7 @@ def get_queryset(self): return self.get_resource().get_affiliated_institutions() -class PreprintInstitutionsRelationship(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin): +class PreprintInstitutionsRelationship(JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin): """ """ permission_classes = ( drf_permissions.IsAuthenticatedOrReadOnly, diff --git a/api_tests/preprints/views/test_preprint_versions.py b/api_tests/preprints/views/test_preprint_versions.py index 9d0af151461..70bcb199817 100644 --- a/api_tests/preprints/views/test_preprint_versions.py +++ b/api_tests/preprints/views/test_preprint_versions.py @@ -9,7 +9,7 @@ from osf.models import Preprint from osf.utils import permissions from osf.utils.workflows import DefaultStates, RequestTypes -from osf_tests.factories import ProjectFactory, PreprintFactory, AuthUserFactory, PreprintRequestFactory +from osf_tests.factories import ProjectFactory, PreprintFactory, AuthUserFactory, PreprintRequestFactory, SubjectFactory from tests.base import ApiTestCase from tests.utils import capture_notifications @@ -225,9 +225,10 @@ def test_return_409_if_unpublished_pending_version_exists(self): res = self.app.post_json_api(self.pre_mode_version_create_url, auth=self.user.auth, expect_errors=True) assert res.status_code == 409 - def test_return_409_if_try_to_edit_old_versions(self): + def test_edit_old_version_after_new_version_is_accepted(self): res = self.app.post_json_api(self.post_mod_version_create_url, auth=self.user.auth) new_version = Preprint.load(res.json['data']['id']) + new_version_title = new_version.title filename = 'preprint_file.txt' preprint_file = OsfStorageFile.create( target_object_id=new_version.id, @@ -247,13 +248,17 @@ def test_return_409_if_try_to_edit_old_versions(self): with capture_notifications(): new_version.run_accept(self.moderator, 'comment') assert new_version.machine_state == 'accepted' + assert not self.post_mod_preprint.is_latest_version res = self.app.patch_json_api( self.post_mod_preprint_update_url, self.post_mod_preprint_update_payload, auth=self.user.auth, - expect_errors=True ) - assert res.status_code == 409 + assert res.status_code == 200 + self.post_mod_preprint.reload() + new_version.reload() + assert self.post_mod_preprint.title == 'new_title' + assert new_version.title == new_version_title def test_reuse_version_if_unfinished_version_exists(self): res = self.app.post_json_api(self.post_mod_version_create_url, auth=self.user.auth) @@ -902,3 +907,211 @@ def test_version_indifference(self): res_2 = self.app.get(versions_url_valid_version, auth=self.creator.auth) assert res_2.status_code == 200 assert len(res_1.json['data']) == len(res_2.json['data']) + + +class TestPreprintPreviousVersionUpdate(ApiTestCase): + """ + Previous versions follow the same edit permissions as the latest version: ADMIN contributors can edit + metadata and contributors, WRITE contributors can edit metadata only and READ contributors cannot edit. + """ + + def setUp(self): + super().setUp() + self.creator = AuthUserFactory() + self.admin_contrib = AuthUserFactory() + self.write_contrib = AuthUserFactory() + self.read_contrib = AuthUserFactory() + self.non_contrib = AuthUserFactory() + self.preprint_v1 = PreprintFactory( + reviews_workflow='post-moderation', + is_published=True, + creator=self.creator + ) + self.preprint_v1.add_contributor(self.admin_contrib, permissions=permissions.ADMIN) + self.preprint_v1.add_contributor(self.write_contrib, permissions=permissions.WRITE) + self.preprint_v1.add_contributor(self.read_contrib, permissions=permissions.READ) + with capture_notifications(): + self.preprint_v2 = PreprintFactory.create_version( + create_from=self.preprint_v1, + creator=self.creator, + set_doi=False + ) + assert not self.preprint_v1.is_latest_version + assert self.preprint_v2.is_latest_version + + self.v1_url = f'/{API_BASE}preprints/{self.preprint_v1._id}/' + self.v1_contributors_url = f'{self.v1_url}contributors/' + self.v1_subjects_url = f'{self.v1_url}relationships/subjects/' + + def update_payload(self, **attributes): + return { + 'data': { + 'id': self.preprint_v1._id, + 'type': 'preprints', + 'attributes': attributes, + } + } + + def add_contributor_payload(self, user): + return { + 'data': { + 'type': 'contributors', + 'attributes': { + 'bibliographic': True, + 'permission': permissions.WRITE, + }, + 'relationships': { + 'users': { + 'data': { + 'id': user._id, + 'type': 'users', + } + } + } + } + } + + def update_contributor_payload(self, user, **attributes): + return { + 'data': { + 'id': f'{self.preprint_v1._id}-{user._id}', + 'type': 'contributors', + 'attributes': attributes, + } + } + + def test_admin_and_write_can_edit_metadata_of_previous_version(self): + v2_title = self.preprint_v2.title + for user in [self.creator, self.admin_contrib, self.write_contrib]: + title = f'Corrected title by {user._id}' + res = self.app.patch_json_api( + self.v1_url, + self.update_payload(title=title, description='Corrected abstract', tags=['corrected']), + auth=user.auth + ) + assert res.status_code == 200 + assert res.json['data']['attributes']['title'] == title + + self.preprint_v1.reload() + assert self.preprint_v1.title == title + assert self.preprint_v1.description == 'Corrected abstract' + assert list(self.preprint_v1.tags.values_list('name', flat=True)) == ['corrected'] + + self.preprint_v2.reload() + assert self.preprint_v2.title == v2_title + assert not self.preprint_v2.tags.filter(name='corrected').exists() + + def test_admin_and_write_can_edit_subjects_of_previous_version(self): + subject = SubjectFactory() + for user in [self.admin_contrib, self.write_contrib]: + res = self.app.patch_json_api( + self.v1_subjects_url, + {'data': [{'type': 'subjects', 'id': subject._id}]}, + auth=user.auth + ) + assert res.status_code == 200 + assert list(self.preprint_v1.subjects.values_list('_id', flat=True)) == [subject._id] + assert not self.preprint_v2.subjects.filter(_id=subject._id).exists() + + def test_read_and_non_contributors_cannot_edit_previous_version(self): + original_title = self.preprint_v1.title + for user in [self.read_contrib, self.non_contrib]: + res = self.app.patch_json_api( + self.v1_url, + self.update_payload(title='Not allowed'), + auth=user.auth, + expect_errors=True + ) + assert res.status_code == 403 + + res = self.app.patch_json_api( + self.v1_subjects_url, + {'data': [{'type': 'subjects', 'id': SubjectFactory()._id}]}, + auth=user.auth, + expect_errors=True + ) + assert res.status_code == 403 + + res = self.app.patch_json_api(self.v1_url, self.update_payload(title='Not allowed'), expect_errors=True) + assert res.status_code == 401 + + self.preprint_v1.reload() + assert self.preprint_v1.title == original_title + + def test_admin_can_edit_contributors_of_previous_version(self): + new_contrib = AuthUserFactory() + with capture_notifications(): + res = self.app.post_json_api( + self.v1_contributors_url, + self.add_contributor_payload(new_contrib), + auth=self.admin_contrib.auth + ) + assert res.status_code == 201 + assert self.preprint_v1.is_contributor(new_contrib) + assert not self.preprint_v2.is_contributor(new_contrib) + + res = self.app.patch_json_api( + f'{self.v1_contributors_url}{self.read_contrib._id}/', + self.update_contributor_payload(self.read_contrib, permission=permissions.WRITE, index=0), + auth=self.admin_contrib.auth + ) + assert res.status_code == 200 + assert self.preprint_v1.has_permission(self.read_contrib, permissions.WRITE) + assert self.preprint_v1.get_contributor_order()[0] == self.preprint_v1.preprintcontributor_set.get(user=self.read_contrib).id + assert not self.preprint_v2.has_permission(self.read_contrib, permissions.WRITE) + + # bulk update, as sent by the preprint edit flow + res = self.app.patch_json_api( + self.v1_contributors_url, + {'data': [self.update_contributor_payload(self.write_contrib, permission=permissions.READ)['data']]}, + auth=self.admin_contrib.auth, + bulk=True + ) + assert res.status_code == 200 + assert not self.preprint_v1.has_permission(self.write_contrib, permissions.WRITE) + assert self.preprint_v2.has_permission(self.write_contrib, permissions.WRITE) + + res = self.app.delete_json_api( + f'{self.v1_contributors_url}{new_contrib._id}/', + auth=self.admin_contrib.auth + ) + assert res.status_code == 204 + assert not self.preprint_v1.is_contributor(new_contrib) + + def test_write_and_read_cannot_edit_contributors_of_previous_version(self): + new_contrib = AuthUserFactory() + for user in [self.write_contrib, self.read_contrib]: + res = self.app.post_json_api( + self.v1_contributors_url, + self.add_contributor_payload(new_contrib), + auth=user.auth, + expect_errors=True + ) + assert res.status_code == 403 + + res = self.app.patch_json_api( + f'{self.v1_contributors_url}{self.admin_contrib._id}/', + self.update_contributor_payload(self.admin_contrib, permission=permissions.READ), + auth=user.auth, + expect_errors=True + ) + assert res.status_code == 403 + + res = self.app.patch_json_api( + self.v1_contributors_url, + {'data': [self.update_contributor_payload(self.admin_contrib, permission=permissions.READ)['data']]}, + auth=user.auth, + expect_errors=True, + bulk=True + ) + assert res.status_code == 403 + + res = self.app.delete_json_api( + f'{self.v1_contributors_url}{self.admin_contrib._id}/', + auth=user.auth, + expect_errors=True + ) + assert res.status_code == 403 + + assert not self.preprint_v1.is_contributor(new_contrib) + assert self.preprint_v1.has_permission(self.admin_contrib, permissions.ADMIN) From a4d30bd5039964aabe96331611f359739f823533 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Wed, 21 Oct 2026 15:28:18 +0300 Subject: [PATCH 11/22] [ENG-12471] run postcommit tasks after db transaction save (#11971) ## Ticket https://openscience.atlassian.net/browse/ENG-12471 ## Purpose Flask runs handlers in LIFO order which means postcommit tasks are run before transaction commits. It makes possible the scenario when spam checks run -> transaction commits -> spam checks finish and override registration state with stale data. ## Changes Run postcommit tasks after transaction commits --- osf_tests/test_node.py | 83 ++++++++++++++++++++++++++++++++++++++++++ website/app.py | 6 ++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/osf_tests/test_node.py b/osf_tests/test_node.py index dd1e1edd991..186520d9f51 100644 --- a/osf_tests/test_node.py +++ b/osf_tests/test_node.py @@ -7,6 +7,8 @@ from django.utils import timezone from framework.celery_tasks import handlers from framework.exceptions import PermissionsError +from framework.postcommit_tasks import handlers as postcommit_handlers +from framework.transactions import handlers as transaction_handlers from api.caching import settings as cache_settings from api.caching.utils import storage_usage_cache from website.project.model import has_anonymous_link @@ -2587,6 +2589,87 @@ def test_check_resource_for_spam_postcommit_does_not_reset_approved_registration assert registration.moderation_state == RegistrationModerationStates.ACCEPTED.db_name mock_check_user.assert_called_once() + @mock.patch.object(settings, 'SPAM_SERVICES_ENABLED', False) + @mock.patch('osf.external.spam.tasks._check_resource_for_domains', return_value=[]) + def test_check_resource_for_spam_postcommit_handler_precedes_transaction_handler_for_registration( + self, mock_check_domains, user, app + ): + registration = RegistrationFactory(creator=user, is_public=True) + registration.require_approval(user) + + after_request_handlers = app.after_request_funcs[None] + postcommit_handler = postcommit_handlers.postcommit_after_request + transaction_handler = transaction_handlers.transaction_after_request + transaction_handler_index = after_request_handlers.index(transaction_handler) + assert after_request_handlers.index(postcommit_handler) < transaction_handler_index + + handler_execution_order = [] + + def tracked_transaction_handler(response): + response = transaction_handler(response) + handler_execution_order.append('transaction') + return response + + def tracked_postcommit_handler(response): + handler_execution_order.append('postcommit') + return postcommit_handler(response) + + tracked_handlers = { + transaction_handler: tracked_transaction_handler, + postcommit_handler: tracked_postcommit_handler, + } + + def check_domains_after_transaction(*args): + assert handler_execution_order == ['transaction', 'postcommit'] + persisted_registration = Registration.objects.get(id=registration.id) + assert persisted_registration.moderation_state == RegistrationModerationStates.ACCEPTED.db_name + return [] + + mock_check_domains.side_effect = check_domains_after_transaction + + was_testing = app.config['TESTING'] + app.config['TESTING'] = False + atomic = None + try: + with app.test_request_context('/'): + postcommit_handlers.postcommit_before_request() + transaction_handlers.transaction_before_request() + atomic = g.current_atomic + registration.registration_approval.accept() + + postcommit_handlers.postcommit_celery_queue()['registration-spam-check'] = ( + spam_tasks.check_resource_for_spam_postcommit.si( + guid=registration._id, + content='Check me for spam', + author=user.fullname, + author_email=user.username, + request_headers={}, + ) + ) + + with mock.patch.object(postcommit_handlers.settings, 'USE_CELERY', False): + with mock.patch.object(Registration, 'check_spam_user'): + response = mock.Mock(status_code=200) + relevant_handlers = [ + handler for handler in after_request_handlers + if handler in (postcommit_handler, transaction_handler) + ] + for handler in reversed(relevant_handlers): + response = tracked_handlers[handler](response) + if handler is transaction_handler: + atomic = None + + registration.refresh_from_db() + assert handler_execution_order == ['transaction', 'postcommit'] + assert registration.moderation_state == RegistrationModerationStates.ACCEPTED.db_name + mock_check_domains.assert_called_once() + finally: + if atomic is not None: + error = RuntimeError('Transaction handler did not complete') + atomic.__exit__(RuntimeError, error, None) + postcommit_handlers.postcommit_before_request() + app.config['TESTING'] = was_testing + @mock.patch.object(settings, 'SPAM_SERVICES_ENABLED', True) @mock.patch('osf.external.spam.tasks._check_resource_for_domains') def test_check_resource_for_spam_postcommit_no_spammy_domains_checks_services(self, mock_check_domains, project, user, request_headers): diff --git a/website/app.py b/website/app.py index f6feb77e9c2..1e23f94cff3 100644 --- a/website/app.py +++ b/website/app.py @@ -56,8 +56,12 @@ def attach_handlers(app, settings): # Add callback handlers to application add_handlers(app, django_handlers.handlers) add_handlers(app, celery_task_handlers.handlers) - add_handlers(app, transaction_handlers.handlers) + # flask uses LIFO run of handlers, so we actually run transaction handlers + # before postcommit handlers to avoid case when postcommit handler + # updates registration data before transaction is committed. + # postcommit and transaction handlers must not be swapped add_handlers(app, postcommit_handlers.handlers) + add_handlers(app, transaction_handlers.handlers) add_handlers(app, csrf_handlers.handlers) # Attach handler for checking view-only link keys. From fff173265790416415bcfec3ade64ee4781e21dd Mon Sep 17 00:00:00 2001 From: antkryt Date: Thu, 22 Oct 2026 16:44:14 +0300 Subject: [PATCH 12/22] [ENG-12551] Make "Exclude non-contributors" checked by default (#11975) * Make exclude non-contributors checked by default --- admin/templates/notifications/notification_campaing_create.html | 1 + 1 file changed, 1 insertion(+) diff --git a/admin/templates/notifications/notification_campaing_create.html b/admin/templates/notifications/notification_campaing_create.html index 2f1a6d2f525..543d1f28f82 100644 --- a/admin/templates/notifications/notification_campaing_create.html +++ b/admin/templates/notifications/notification_campaing_create.html @@ -183,6 +183,7 @@

Recipient Filters

Exclude non-contributors From b0958c3e250dd06a9c5f3ab651e395235ee92269 Mon Sep 17 00:00:00 2001 From: mkovalua Date: Mon, 26 Oct 2026 16:50:15 +0200 Subject: [PATCH 13/22] [ENG-10824] Make SHARE consistently index User content in admin app (#11978) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Ticket https://openscience.atlassian.net/browse/ENG-10824 ## Purpose Reindexing a user from admin doesn’t do what the button suggests. The current “Reindex” button for a user sends that user’s content (projects, preprints) to SHARE. It doesn’t send the user record itself. So on the SHARE Reindex tab, the user stays on the list even hours after the reindex runs. The fix is to split this into two separate actions: Reindex User sends only the user record, meaning their profile metadata (no projects or preprints). Reindex All User’s Content sends everything the user owns, which is what the current button does. Admins also need to see whether a user allows SHARE indexing, and change it if the user asks. --- admin/share_reindex/views.py | 3 +- admin/templates/share_reindex/list.html | 6 +- admin/templates/users/reindex_user_share.html | 26 +++++++- admin/templates/users/user.html | 19 ++++++ admin/users/urls.py | 4 ++ admin/users/views.py | 55 ++++++++++++++++ admin_tests/share_reindex/__init__.py | 0 admin_tests/share_reindex/test_views.py | 41 ++++++++++++ admin_tests/users/test_views.py | 66 +++++++++++++++++++ api/share/utils.py | 2 +- osf/models/admin_log_entry.py | 1 + 11 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 admin_tests/share_reindex/__init__.py create mode 100644 admin_tests/share_reindex/test_views.py diff --git a/admin/share_reindex/views.py b/admin/share_reindex/views.py index 0d68194f7e5..065dee69ccf 100644 --- a/admin/share_reindex/views.py +++ b/admin/share_reindex/views.py @@ -5,6 +5,7 @@ from osf.models import Guid from urllib.parse import urlencode from api.share.utils import get_not_indexed_guids_for_resource_with_no_indexed_guid, task__reindex_failed_or_not_indexed_resource_into_share +from website.settings import CeleryConfig class FailedShareIndexedGuidList(PermissionRequiredMixin, ListView): paginate_by = 25 @@ -46,7 +47,7 @@ class FailedShareIndexedGuidReindex(PermissionRequiredMixin, View): def post(self, request, *args, **kwargs): resource_type = self.kwargs.get('resource_type') # reindex 100_000 guids in background task for specific resource_type and resource is public - task__reindex_failed_or_not_indexed_resource_into_share.delay(resource_type) + task__reindex_failed_or_not_indexed_resource_into_share.apply_async(args=(resource_type,), queue=CeleryConfig.task_low_queue) base_url = reverse('share_reindex:list') query_string = urlencode({'type': resource_type, 'status': 'indexing'}) return redirect(f"{base_url}?{query_string}") diff --git a/admin/templates/share_reindex/list.html b/admin/templates/share_reindex/list.html index 34083b75696..e50cc5025ef 100644 --- a/admin/templates/share_reindex/list.html +++ b/admin/templates/share_reindex/list.html @@ -30,7 +30,7 @@

Share Reindex

- SHARE Reindex All {{selected_resource_type}} + {% if selected_resource_type == 'users' %}Reindex All Users{% else %}SHARE Reindex All {{selected_resource_type}}{% endif %}