From 094decfaf9a03420a6ae59160f91b4648a900c49 Mon Sep 17 00:00:00 2001 From: John Tordoff Date: Fri, 2 May 2025 09:46:18 -0400 Subject: [PATCH 01/11] improve action creation test (#11109) ## Purpose Fix's a 502 that happened when the permission's class hit a NoneType target id ## Changes - improves tests - adds validation for target ids ## Ticket https://openscience.atlassian.net/browse/ENG-7810 --- api/actions/views.py | 4 +- api_tests/actions/views/test_action_list.py | 111 ++++++++++++++++++-- 2 files changed, 104 insertions(+), 11 deletions(-) diff --git a/api/actions/views.py b/api/actions/views.py index 9e1d6d76ebb..5a9f8803781 100644 --- a/api/actions/views.py +++ b/api/actions/views.py @@ -84,7 +84,6 @@ def get_serializer_class(self): return ReviewActionSerializer def get_object(self): - action = None action_id = self.kwargs['action_id'] if NodeRequestAction.objects.filter(_id=action_id).exists() or PreprintRequestAction.objects.filter(_id=action_id).exists(): @@ -169,6 +168,9 @@ class ReviewActionListCreate(JSONAPIBaseView, generics.ListCreateAPIView, Target # overrides ListCreateAPIView def perform_create(self, serializer): target = serializer.validated_data['target'] + if not target: + raise NotFound(f'Unable to find specified Action {target}') + self.check_object_permissions(self.request, target) if not target.provider.is_reviewed: diff --git a/api_tests/actions/views/test_action_list.py b/api_tests/actions/views/test_action_list.py index bda6be6890a..c33048656f1 100644 --- a/api_tests/actions/views/test_action_list.py +++ b/api_tests/actions/views/test_action_list.py @@ -57,54 +57,106 @@ def moderator(self, provider): moderator.groups.add(provider.get_group('moderator')) return moderator - def test_create_permissions( - self, app, url, preprint, node_admin, moderator - ): + def test_create_permissions_unauthorized(self, app, url, preprint, node_admin, moderator): assert preprint.machine_state == 'initial' - submit_payload = self.create_payload(preprint._id, trigger='submit') + submit_payload = self.create_payload( + preprint._id, + trigger='submit' + ) # Unauthorized user can't submit - res = app.post_json_api(url, submit_payload, expect_errors=True) + res = app.post_json_api( + url, + submit_payload, + expect_errors=True + ) assert res.status_code == 401 + def test_create_permissions_forbidden(self, app, url, preprint, node_admin, moderator): + assert preprint.machine_state == 'initial' + + submit_payload = self.create_payload( + preprint._id, + trigger='submit' + ) + # A random user can't submit some_rando = AuthUserFactory() res = app.post_json_api( - url, submit_payload, + url, + submit_payload, auth=some_rando.auth, expect_errors=True ) assert res.status_code == 403 + def test_create_permissions_success(self, app, url, preprint, node_admin, moderator): + assert preprint.machine_state == 'initial' + + submit_payload = self.create_payload( + preprint._id, + trigger='submit' + ) + # Node admin can submit - res = app.post_json_api(url, submit_payload, auth=node_admin.auth) + res = app.post_json_api( + url, + submit_payload, + auth=node_admin.auth + ) assert res.status_code == 201 preprint.refresh_from_db() assert preprint.machine_state == 'pending' assert not preprint.is_published + def test_accept_permissions_unauthorized(self, app, url, preprint, node_admin, moderator): + preprint.machine_state = 'pending' + preprint.save() + assert preprint.machine_state == 'pending' + accept_payload = self.create_payload( - preprint._id, trigger='accept', comment='This is good.' + preprint._id, + trigger='accept', + comment='This is good.' ) # Unauthorized user can't accept res = app.post_json_api(url, accept_payload, expect_errors=True) assert res.status_code == 401 + def test_accept_permissions_forbidden(self, app, url, preprint, node_admin, moderator): + preprint.machine_state = 'pending' + preprint.save() + assert preprint.machine_state == 'pending' + + accept_payload = self.create_payload( + preprint._id, + trigger='accept', + comment='This is good.' + ) + + some_rando = AuthUserFactory() + # A random user can't accept res = app.post_json_api( - url, accept_payload, + url, + accept_payload, auth=some_rando.auth, expect_errors=True ) assert res.status_code == 403 - # Moderator from another provider can't accept + def test_accept_permissions_other_mod(self, app, url, preprint, node_admin, moderator): another_moderator = AuthUserFactory() another_moderator.groups.add( PreprintProviderFactory().get_group('moderator') ) + accept_payload = self.create_payload( + preprint._id, + trigger='accept', + comment='This is good.' + ) res = app.post_json_api( url, accept_payload, auth=another_moderator.auth, @@ -120,6 +172,15 @@ def test_create_permissions( ) assert res.status_code == 403 + def test_accept_permissions_accept(self, app, url, preprint, node_admin, moderator): + preprint.machine_state = 'pending' + preprint.save() + accept_payload = self.create_payload( + preprint._id, + trigger='accept', + comment='This is good.' + ) + # Still unchanged after all those tries preprint.refresh_from_db() assert preprint.machine_state == 'pending' @@ -275,3 +336,33 @@ def test_valid_transitions( assert preprint.date_last_transitioned is None else: assert preprint.date_last_transitioned == action.created + + def test_invalid_target_id(self, app, moderator): + """ + This test simulates the issue reported where using either an invalid + action ID or an unrelated node ID as the target ID results in a 502 error. + + It attempts to create a review action with a bad `target.id`. + """ + res = app.post_json_api( + f'/{API_BASE}actions/reviews/', + { + 'data': { + 'type': 'actions', + 'attributes': { + 'trigger': 'submit' + }, + 'relationships': { + 'target': { + 'data': { + 'type': 'preprints', + 'id': 'deadbeef1234' + } + } + } + } + }, + auth=moderator.auth, + expect_errors=True + ) + assert res.status_code == 404 From 7afb742df0a3d8ff9f20ee2af86ab2bf70a37257 Mon Sep 17 00:00:00 2001 From: antkryt Date: Fri, 2 May 2025 16:46:51 +0300 Subject: [PATCH 02/11] [ENG-7811] Reviews Dashboard not displaying contributors for preprint provider moderators (#11116) ## Purpose fix preprint visibility for admin reviewer ## Ticket https://openscience.atlassian.net/browse/ENG-7811 --- api/preprints/views.py | 14 +- .../preprints/views/test_preprint_versions.py | 127 ++++++++++-------- 2 files changed, 84 insertions(+), 57 deletions(-) diff --git a/api/preprints/views.py b/api/preprints/views.py index af1cb3af974..cd5dba8ba8d 100644 --- a/api/preprints/views.py +++ b/api/preprints/views.py @@ -1,6 +1,7 @@ import re from packaging.version import Version +from django.contrib.auth.models import AnonymousUser from rest_framework import generics from rest_framework.exceptions import MethodNotAllowed, NotFound, PermissionDenied, NotAuthenticated, ValidationError from rest_framework import permissions as drf_permissions @@ -136,12 +137,17 @@ def get_preprint(self, check_object_permissions=True, ignore_404=False): if check_object_permissions: self.check_object_permissions(self.request, preprint) - user_is_moderator = preprint.provider.get_group('moderator').user_set.filter(id=self.request.user.id).exists() - user_is_contributor = preprint.contributors.filter(id=self.request.user.id).exists() + user = self.request.user + if isinstance(user, AnonymousUser): + user_is_reviewer = user_is_contributor = False + else: + user_is_reviewer = user.has_groups(preprint.provider.group_names) + user_is_contributor = preprint.is_contributor(user) + if ( preprint.machine_state == DefaultStates.INITIAL.value and not user_is_contributor and - user_is_moderator + user_is_reviewer ): raise NotFound @@ -149,7 +155,7 @@ def get_preprint(self, check_object_permissions=True, ignore_404=False): preprint.machine_state in PUBLIC_STATES[preprint.provider.reviews_workflow] or preprint.machine_state == ReviewStates.WITHDRAWN.value, ) - if not preprint_is_public and not user_is_contributor and not user_is_moderator: + if not preprint_is_public and not user_is_contributor and not user_is_reviewer: raise NotFound return preprint diff --git a/api_tests/preprints/views/test_preprint_versions.py b/api_tests/preprints/views/test_preprint_versions.py index 090f180721c..efb8a092327 100644 --- a/api_tests/preprints/views/test_preprint_versions.py +++ b/api_tests/preprints/views/test_preprint_versions.py @@ -20,6 +20,7 @@ def setUp(self): self.user = AuthUserFactory() self.project = ProjectFactory(creator=self.user) self.moderator = AuthUserFactory() + self.admin = AuthUserFactory() self.post_mod_preprint = PreprintFactory( reviews_workflow='post-moderation', is_published=True, @@ -30,8 +31,10 @@ def setUp(self): is_published=False, creator=self.user ) - self.post_mod_preprint.provider.get_group('moderator').user_set.add(self.moderator) - self.pre_mod_preprint.provider.get_group('moderator').user_set.add(self.moderator) + self.post_mod_preprint.provider.add_to_group(self.moderator, 'moderator') + self.pre_mod_preprint.provider.add_to_group(self.moderator, 'moderator') + self.post_mod_preprint.provider.add_to_group(self.admin, 'admin') + self.pre_mod_preprint.provider.add_to_group(self.admin, 'admin') self.post_mod_version_create_url = f'/{API_BASE}preprints/{self.post_mod_preprint._id}/versions/?version=2.20' self.pre_mode_version_create_url = f'/{API_BASE}preprints/{self.pre_mod_preprint._id}/versions/?version=2.20' self.post_mod_preprint_update_url = f'/{API_BASE}preprints/{self.post_mod_preprint._id}/?version=2.20' @@ -513,7 +516,7 @@ def test_accepted_preprint_in_pre_moderation_and_withdrawn_is_shown_for_everyone assert res.status_code == 200 assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' - def test_moderator_sees_pending_preprint(self): + def test_reviewer_sees_pending_preprint(self): preprint_id = self.pre_mod_preprint._id.split('_')[0] contributor = AuthUserFactory() @@ -526,17 +529,20 @@ def test_moderator_sees_pending_preprint(self): ) pre_mod_preprint_v2.run_submit(self.user) - # preprint - res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'pending' + for reviewer in [self.admin, self.moderator]: + assert not pre_mod_preprint_v2.is_contributor(reviewer) - # preprint version - res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'pending' + # preprint + res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'pending' - def test_moderator_sees_pending_withdrawn_preprint(self): + # preprint version + res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'pending' + + def test_reviewer_sees_pending_withdrawn_preprint(self): preprint_id = self.pre_mod_preprint._id.split('_')[0] contributor = AuthUserFactory() @@ -558,17 +564,20 @@ def test_moderator_sees_pending_withdrawn_preprint(self): withdrawal_request.run_submit(self.user) withdrawal_request.run_accept(self.moderator, withdrawal_request.comment) - # preprint - res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' + for reviewer in [self.admin, self.moderator]: + assert not pre_mod_preprint_v2.is_contributor(reviewer) - # preprint version - res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' + # preprint + res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' + + # preprint version + res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' - def test_moderator_sees_accepted_preprint(self): + def test_reviewer_sees_accepted_preprint(self): preprint_id = self.pre_mod_preprint._id.split('_')[0] contributor = AuthUserFactory() @@ -582,17 +591,20 @@ def test_moderator_sees_accepted_preprint(self): pre_mod_preprint_v2.run_submit(self.user) pre_mod_preprint_v2.run_accept(self.moderator, 'comment') - # preprint - res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'accepted' + for reviewer in [self.admin, self.moderator]: + assert not pre_mod_preprint_v2.is_contributor(reviewer) - # preprint version - res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'accepted' + # preprint + res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'accepted' + + # preprint version + res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'accepted' - def test_moderator_sees_withdrawn_preprint(self): + def test_reviewer_sees_withdrawn_preprint(self): preprint_id = self.pre_mod_preprint._id.split('_')[0] contributor = AuthUserFactory() @@ -614,17 +626,20 @@ def test_moderator_sees_withdrawn_preprint(self): withdrawal_request.run_submit(self.user) withdrawal_request.run_accept(self.moderator, withdrawal_request.comment) - # preprint - res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' + for reviewer in [self.admin, self.moderator]: + assert not pre_mod_preprint_v2.is_contributor(reviewer) - # preprint version - res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=self.moderator.auth) - assert res.status_code == 200 - assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' + # preprint + res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' + + # preprint version + res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=reviewer.auth) + assert res.status_code == 200 + assert res.json['data']['attributes']['reviews_state'] == 'withdrawn' - def test_moderator_does_not_see_initial_preprint(self): + def test_reviewer_does_not_see_initial_preprint(self): preprint_id = self.pre_mod_preprint._id.split('_')[0] contributor = AuthUserFactory() @@ -636,30 +651,36 @@ def test_moderator_does_not_see_initial_preprint(self): set_doi=False ) - # preprint - res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=self.moderator.auth, expect_errors=True) - assert res.status_code == 404 + for reviewer in [self.admin, self.moderator]: + assert not pre_mod_preprint_v2.is_contributor(reviewer) - # preprint version - res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=self.moderator.auth, expect_errors=True) - assert res.status_code == 404 + # preprint + res = self.app.get(f'/{API_BASE}preprints/{preprint_id}/', auth=reviewer.auth, expect_errors=True) + assert res.status_code == 404 + + # preprint version + res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=reviewer.auth, expect_errors=True) + assert res.status_code == 404 - def test_moderator_can_contribute(self): + def test_reviewer_can_contribute(self): pre_mod_preprint_v2 = PreprintFactory.create_version( create_from=self.pre_mod_preprint, final_machine_state='initial', creator=self.user, set_doi=False ) - pre_mod_preprint_v2.add_contributor(self.moderator, permissions.READ) - # preprint - res = self.app.get(f'/{API_BASE}preprints/{self.pre_mod_preprint._id.split('_')[0]}/', auth=self.moderator.auth) - assert res.status_code == 200 + for reviewer in [self.admin, self.moderator]: + pre_mod_preprint_v2.add_contributor(reviewer, permissions.READ) + assert pre_mod_preprint_v2.is_contributor(reviewer) - # preprint version - res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=self.moderator.auth) - assert res.status_code == 200 + # preprint + res = self.app.get(f'/{API_BASE}preprints/{self.pre_mod_preprint._id.split('_')[0]}/', auth=reviewer.auth) + assert res.status_code == 200 + + # preprint version + res = self.app.get(f'/{API_BASE}preprints/{pre_mod_preprint_v2._id}/', auth=reviewer.auth) + assert res.status_code == 200 class TestPreprintVersionsListRetrieve(ApiTestCase): From d12a1c2c20b85b5be0628ad997df16af3d251258 Mon Sep 17 00:00:00 2001 From: antkryt Date: Fri, 2 May 2025 16:47:38 +0300 Subject: [PATCH 03/11] do not make users unregistered when disable (#11112) ## Purpose fix broken users ## Changes - do not unregister users when deactivate spam ## QA Notes How I was able to recreate it: In admin go to managment commands -> Ban spam users by regular expression and ban some users (Actually there may be other ways to recreate it using spam feature in different ways like management commands, scheduled spam checks and so on). They will become unregistered, disabled and confirmed. Then we can reactivate account in admin to make it enabled but not registered ## Ticket https://openscience.atlassian.net/browse/ENG-7704 --- admin/users/views.py | 3 --- osf/management/commands/deactivate_requested_accounts.py | 1 - osf/models/mixins.py | 1 - osf/models/user.py | 2 +- 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/admin/users/views.py b/admin/users/views.py index 85b6eac4ee6..6976c978eec 100644 --- a/admin/users/views.py +++ b/admin/users/views.py @@ -456,9 +456,6 @@ def get_context_data(self, **kwargs): class GetUserConfirmationLink(GetUserLink): def get_link(self, user): - if user.is_confirmed: - return f'User {user._id} is already confirmed' - if user.deleted or user.is_merged: return f'User {user._id} is deleted or merged' diff --git a/osf/management/commands/deactivate_requested_accounts.py b/osf/management/commands/deactivate_requested_accounts.py index 9a3ddcf5356..512fb34eeef 100644 --- a/osf/management/commands/deactivate_requested_accounts.py +++ b/osf/management/commands/deactivate_requested_accounts.py @@ -31,7 +31,6 @@ def deactivate_requested_accounts(dry_run=True): logger.info(f'Disabling user {user._id}.') if not dry_run: user.deactivate_account() - user.is_registered = False mails.send_mail( to_addr=user.username, mail=mails.REQUEST_DEACTIVATION_COMPLETE, diff --git a/osf/models/mixins.py b/osf/models/mixins.py index facc39af119..ac8d5a094d8 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -2222,7 +2222,6 @@ def suspend_spam_user(self, user): user.flag_spam() if not user.is_disabled: user.deactivate_account() - user.is_registered = False mails.send_mail( to_addr=user.username, mail=mails.SPAM_USER_BANNED, diff --git a/osf/models/user.py b/osf/models/user.py index 97d444698fd..600b563dd76 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -1284,7 +1284,7 @@ def add_unconfirmed_email(self, email, expiration=None, external_identity=None, validate_email(email) if not external_identity and self.emails.filter(address=email).exists(): - if not force or self.is_confirmed: + if not force and self.is_confirmed: raise ValueError('Email already confirmed to this user.') # If the unconfirmed email is already present, refresh the token From 2508c9b431e15cbecc66b39aff0864c090f20482 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Fri, 2 May 2025 16:48:07 +0300 Subject: [PATCH 04/11] return number of contact messages of each admin (#11115) ## Purpose Admins should be able to see number of messages that each admin sent to the same user ## Changes Added contact property, added a test and fixed existing ones Contacts are ordered alphabetically Additional scope: this property was added in export files as well ## Ticket https://openscience.atlassian.net/jira/software/c/projects/ENG/boards/145 --- api/institutions/serializers.py | 21 ++++- .../test_institution_user_metric_list.py | 93 +++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/api/institutions/serializers.py b/api/institutions/serializers.py index 0262f9dddd7..2af640f3b48 100644 --- a/api/institutions/serializers.py +++ b/api/institutions/serializers.py @@ -1,7 +1,10 @@ +from django.db.models import Count, F + from rest_framework import serializers as ser from rest_framework import exceptions -from osf.models import Node, Registration +from framework import sentry +from osf.models import Node, Registration, OSFUser from osf.utils import permissions as osf_permissions from api.base.serializers import ( @@ -355,12 +358,28 @@ class Meta: related_view='institutions:institution-detail', related_view_kwargs={'institution_id': ''}, ) + contacts = ser.SerializerMethodField() links = LinksField({}) def get_absolute_url(self): return None # there is no detail view for institution-users + def get_contacts(self, obj): + user = OSFUser.load(obj._d_['user_id']) + if not user: + sentry.log_message(f'Institutional report with id {obj.meta['id']} has missing user') + return [] + + results = user.received_user_messages.annotate( + sender_name=F('sender__fullname'), + ).values( + 'sender_name', + ).annotate( + count=Count('sender_name'), + ).order_by('sender_name') + return list(results) + class NewInstitutionSummaryMetricsSerializer(JSONAPISerializer): '''serializer for institution-summary metrics diff --git a/api_tests/institutions/views/test_institution_user_metric_list.py b/api_tests/institutions/views/test_institution_user_metric_list.py index b1bf3490788..3c0e1bf055f 100644 --- a/api_tests/institutions/views/test_institution_user_metric_list.py +++ b/api_tests/institutions/views/test_institution_user_metric_list.py @@ -17,6 +17,8 @@ from osf.metrics import UserInstitutionProjectCounts from osf.metrics.reports import InstitutionalUserReport +from osf.models import UserMessage + @pytest.mark.es_metrics @pytest.mark.django_db @@ -350,6 +352,10 @@ def test_get_reports(self, app, url, institutional_admin, institution, reports, _expected_user_ids = {_report.user_id for _report in reports} assert set(_user_ids(_resp)) == _expected_user_ids + # # users haven't any received messages from institutional admins + for response_object in _resp.json['data']: + assert len(response_object['attributes']['contacts']) == 0 + def test_filter_reports(self, app, url, institutional_admin, institution, reports, unshown_reports): for _query, _expected_user_ids in ( ({'filter[department]': 'nunavum'}, set()), @@ -447,6 +453,7 @@ def test_get_report_formats_csv_tsv(self, app, url, institutional_admin, institu [ 'report_yearmonth', 'account_creation_date', + 'contacts', 'department', 'embargoed_registration_count', 'month_last_active', @@ -463,6 +470,7 @@ def test_get_report_formats_csv_tsv(self, app, url, institutional_admin, institu [ '2024-08', '2018-02', + '[]', 'Center, \t Greatest Ever', '1', '2018-02', @@ -516,6 +524,7 @@ def test_csv_tsv_ignores_pagination(self, app, url, institutional_admin, institu expected_data.append([ '2024-08', '2018-02', + '[]', 'QBatman', '1', '2018-02', @@ -557,6 +566,7 @@ def test_csv_tsv_ignores_pagination(self, app, url, institutional_admin, institu expected_header = [ 'report_yearmonth', 'account_creation_date', + 'contacts', 'department', 'embargoed_registration_count', 'month_last_active', @@ -612,6 +622,7 @@ def test_get_report_format_table_json(self, app, url, institutional_admin, insti { 'report_yearmonth': '2024-08', 'account_creation_date': '2018-02', + 'contacts': [], 'department': 'Safety "The Wolverine" Weapon X', 'embargoed_registration_count': 1, 'month_last_active': '2018-02', @@ -628,6 +639,88 @@ def test_get_report_format_table_json(self, app, url, institutional_admin, insti ] assert response_data == expected_data + def test_correct_number_of_contact_messages(self, app, url, institutional_admin, institution): + user1 = AuthUserFactory() + user2 = AuthUserFactory() + user3 = AuthUserFactory() + user4 = AuthUserFactory() + _report_factory( + '2024-08', institution, + user_id=user1._id, + storage_byte_count=53, + ), + _report_factory( + '2024-08', institution, + user_id=user2._id, + orcid_id='5555-4444-3333-2222', + storage_byte_count=8277, + ), + _report_factory( + '2024-08', institution, + user_id=user3._id, + department_name='blargl', + storage_byte_count=34834834, + ), + _report_factory( + '2024-08', institution, + user_id=user4._id, + orcid_id='4444-3333-2222-1111', + department_name='a department, or so, that happens, incidentally, to have commas', + storage_byte_count=736662999298, + ) + + receiver = user1 + UserMessage.objects.create( + sender=institutional_admin, + recipient=receiver, + message_text='message1', + message_type='institutional_request', + institution=institution + ) + UserMessage.objects.create( + sender=institutional_admin, + recipient=receiver, + message_text='message2', + message_type='institutional_request', + institution=institution + ) + UserMessage.objects.create( + sender=institutional_admin, + recipient=receiver, + message_text='message3', + message_type='institutional_request', + institution=institution + ) + + new_admin = AuthUserFactory() + institution.get_group('institutional_admins').user_set.add(new_admin) + + # messages from another admin + UserMessage.objects.create( + sender=new_admin, + recipient=receiver, + message_text='message4', + message_type='institutional_request', + institution=institution + ) + UserMessage.objects.create( + sender=new_admin, + recipient=receiver, + message_text='message5', + message_type='institutional_request', + institution=institution + ) + + res = app.get(f'/{API_BASE}institutions/{institution._id}/metrics/users/', auth=institutional_admin.auth) + contact_object = list(filter(lambda contact: receiver._id in contact['relationships']['user']['links']['related']['href'], res.json['data']))[0] + contacts = contact_object['attributes']['contacts'] + + first_admin_contact = list(filter(lambda x: x['sender_name'] == institutional_admin.fullname, contacts))[0] + assert first_admin_contact['count'] == 3 + + another_admin_contact = list(filter(lambda x: x['sender_name'] == new_admin.fullname, contacts))[0] + assert another_admin_contact['count'] == 2 + def _user_ids(api_response): for _datum in api_response.json['data']: From b2efad24ffb315be97565ad749da5d0030a9f3ca Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Fri, 2 May 2025 16:48:48 +0300 Subject: [PATCH 05/11] [ENG-7879] Ability to resync preprints that have missing doi with Crossref (#11121) ## Purpose Admins should be able to resync preprints without minted doi with Crossref instead of all preprints. ## Changes 1. Added check mark for syncing only preprints without minted DOI. Fixed text coloring that has red background and red text color 2. Improved retry functionality for sync_identifier_doi task 3. Created a new celery task to resync preprints missing DOI and after that exclude all preprints from identifiers queryset ## Ticket https://openscience.atlassian.net/browse/ENG-7879 --- admin/management/views.py | 4 ++- admin/templates/management/commands.html | 3 +- osf/management/commands/sync_doi_metadata.py | 35 +++++++++++++++++--- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/admin/management/views.py b/admin/management/views.py index 16057f147ca..525f0d8d64a 100644 --- a/admin/management/views.py +++ b/admin/management/views.py @@ -150,10 +150,12 @@ def post(self, request): class BulkResync(ManagementCommandPermissionView): def post(self, request): + missing_dois_only = request.POST.get('missing_preprint_dois_only', False) sync_doi_metadata.apply_async(kwargs={ 'modified_date': timezone.now(), 'batch_size': None, - 'dry_run': False + 'dry_run': False, + 'missing_preprint_dois_only': missing_dois_only }) messages.success(request, 'Resyncing with CrossRef and DataCite! It will take some time.') return redirect(reverse('management:commands')) diff --git a/admin/templates/management/commands.html b/admin/templates/management/commands.html index beaaf9cfb5d..93eeaf24c18 100644 --- a/admin/templates/management/commands.html +++ b/admin/templates/management/commands.html @@ -74,7 +74,7 @@

Ban spam users by regular expression


- +
@@ -133,6 +133,7 @@

Resync with CrossRef and DataCite

{% csrf_token %} +
diff --git a/osf/management/commands/sync_doi_metadata.py b/osf/management/commands/sync_doi_metadata.py index 8002feeb961..e6b079100f3 100644 --- a/osf/management/commands/sync_doi_metadata.py +++ b/osf/management/commands/sync_doi_metadata.py @@ -5,7 +5,7 @@ from django.contrib.contenttypes.models import ContentType from django.core.management.base import BaseCommand -from osf.models import GuidMetadataRecord, Identifier, Registration +from osf.models import GuidMetadataRecord, Identifier, Registration, Preprint from framework.celery_tasks import app logger = logging.getLogger(__name__) @@ -14,8 +14,8 @@ RATE_LIMIT_RETRY_DELAY = 60 * 5 -@app.task(name='osf.management.commands.sync_doi_metadata', max_retries=5, default_retry_delay=RATE_LIMIT_RETRY_DELAY) -def sync_identifier_doi(identifier_id): +@app.task(name='osf.management.commands.sync_doi_metadata', bind=True, acks_late=True, max_retries=5, default_retry_delay=RATE_LIMIT_RETRY_DELAY) +def sync_identifier_doi(self, identifier_id): try: identifier = Identifier.objects.get(id=identifier_id) identifier.referent.request_identifier_update('doi') @@ -23,17 +23,21 @@ def sync_identifier_doi(identifier_id): logger.info(f'Doi update for {identifier.value} complete') except Exception as err: logger.warning(f'[{err.__class__.__name__}] Doi update for {identifier.value} failed because of error: {err}') - sync_identifier_doi.retry(exc=err, countdown=RATE_LIMIT_RETRY_DELAY) + self.retry() @app.task(name='osf.management.commands.sync_doi_metadata_command', max_retries=5, default_retry_delay=RATE_LIMIT_RETRY_DELAY) -def sync_doi_metadata(modified_date, batch_size=100, dry_run=True, sync_private=False, rate_limit=100): +def sync_doi_metadata(modified_date, batch_size=100, dry_run=True, sync_private=False, rate_limit=100, missing_preprint_dois_only=False): identifiers = Identifier.objects.filter( category='doi', deleted__isnull=True, modified__lte=modified_date, object_id__isnull=False, ) + if missing_preprint_dois_only: + sync_preprint_missing_dois.apply_async(kwargs={'rate_limit': rate_limit}) + identifiers = identifiers.exclude(content_type=ContentType.objects.get_for_model(Preprint)) + if batch_size: identifiers = identifiers[:batch_size] rate_limit = batch_size if batch_size > rate_limit else rate_limit @@ -55,6 +59,27 @@ def sync_doi_metadata(modified_date, batch_size=100, dry_run=True, sync_private= sync_identifier_doi.apply_async(kwargs={'identifier_id': identifier.id}) +@app.task(name='osf.management.commands.sync_preprint_missing_dois', max_retries=5, default_retry_delay=RATE_LIMIT_RETRY_DELAY) +def sync_preprint_missing_dois(rate_limit): + preprints = Preprint.objects.filter(preprint_doi_created=None) + for record_number, preprint in enumerate(preprints, 1): + # in order to not reach rate limit that CrossRef has, we make delay + if not record_number % rate_limit: + time.sleep(RATE_LIMIT_RETRY_DELAY) + + async_request_identifier_update.apply_async(kwargs={'preprint_id': preprint._id}) + + +@app.task(name='osf.management.commands.async_request_identifier_update', bind=True, acks_late=True, max_retries=5, default_retry_delay=RATE_LIMIT_RETRY_DELAY) +def async_request_identifier_update(self, preprint_id): + preprint = Preprint.load(preprint_id) + try: + preprint.request_identifier_update('doi', create=True) + except Exception as err: + logger.warning(f'[{err.__class__.__name__}] Doi creation failed for the preprint with id {preprint._id} because of error: {err}') + self.retry() + + @app.task(name='osf.management.commands.sync_doi_empty_metadata_dataarchive_registrations_command', max_retries=5, default_retry_delay=RATE_LIMIT_RETRY_DELAY) def sync_doi_empty_metadata_dataarchive_registrations(modified_date, batch_size=100, dry_run=True, sync_private=False, rate_limit=100): registrations_ids = list( From f19bf228b7dc43ba27063a3a5fb318a9964f4aa1 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Fri, 2 May 2025 16:49:40 +0300 Subject: [PATCH 06/11] added share update on preprint withdrawal (#11123) ## Purpose Share update is not called when user withdrawals preprint and updates `date_withdrawn` and `withdrawal_justification` fields ## Changes Updated SEARCH_UPDATE_FIELDS, added a test ## Ticket https://openscience.atlassian.net/browse/ENG-5037 --- admin_tests/preprints/test_views.py | 9 ++++++++- osf/models/preprint.py | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index 06ec5fa8d04..1273a3a6363 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -536,7 +536,8 @@ def withdrawal_request(self, preprint, submitter): withdrawal_request.run_submit(submitter) return withdrawal_request - def test_can_approve_withdrawal_request(self, withdrawal_request, submitter, preprint, admin): + @mock.patch('osf.models.preprint.update_or_enqueue_on_preprint_updated') + def test_can_approve_withdrawal_request(self, mocked_function, withdrawal_request, submitter, preprint, admin): assert withdrawal_request.machine_state == DefaultStates.PENDING.value original_comment = withdrawal_request.comment @@ -552,6 +553,12 @@ def test_can_approve_withdrawal_request(self, withdrawal_request, submitter, pre assert withdrawal_request.machine_state == DefaultStates.ACCEPTED.value assert original_comment == withdrawal_request.target.withdrawal_justification + # share update is triggered when update "date_withdrawn" and "withdrawal_justification" throughout withdrawal process + updated_fields = mocked_function.call_args[1]['saved_fields'] + assert 'date_withdrawn' in updated_fields + assert 'withdrawal_justification' in updated_fields + assert preprint.SEARCH_UPDATE_FIELDS.intersection(updated_fields) + def test_can_reject_withdrawal_request(self, withdrawal_request, admin, preprint): assert withdrawal_request.machine_state == DefaultStates.PENDING.value diff --git a/osf/models/preprint.py b/osf/models/preprint.py index cfe6e56afe6..ed8aa0af380 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -201,6 +201,8 @@ class Preprint(DirtyFieldsMixin, VersionedGuidMixin, IdentifierMixin, Reviewable 'primary_file', 'contributors', 'tags', + 'date_withdrawn', + 'withdrawal_justification' } PREREG_LINK_INFO_CHOICES = [('prereg_designs', 'Pre-registration of study designs'), From 9e2fc2c3fc623b0a303987a364e0ccf5c634eaf0 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Fri, 2 May 2025 17:25:38 +0300 Subject: [PATCH 07/11] try to handle an unknown error (#11127) ## Purpose This isn't actual solution, just a way to handle error as it isn't reproducible locally, potentially because of smtp that is not used locally ## Changes Added exception handler, swapped sync call for async version ## Ticket https://openscience.atlassian.net/browse/ENG-6206 --- framework/auth/views.py | 65 +++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 31 deletions(-) diff --git a/framework/auth/views.py b/framework/auth/views.py index e83f47b5db2..3c9d1f5b663 100644 --- a/framework/auth/views.py +++ b/framework/auth/views.py @@ -970,39 +970,42 @@ def resend_confirmation_post(auth): View for user to submit resend confirmation form. HTTP Method: POST """ + try: + # If user is already logged in, log user out + if auth.logged_in: + return auth_logout(redirect_url=request.url) - # If user is already logged in, log user out - if auth.logged_in: - return auth_logout(redirect_url=request.url) - - form = ResendConfirmationForm(request.form) + form = ResendConfirmationForm(request.form) - if form.validate(): - clean_email = form.email.data - user = get_user(email=clean_email) - status_message = ( - f'If there is an OSF account associated with this unconfirmed email address {clean_email}, ' - 'a confirmation email has been resent to it. If you do not receive an email and believe ' - 'you should have, please contact OSF Support.' - ) - kind = 'success' - if user: - if throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE): - try: - send_confirm_email(user, clean_email, renew=True) - except KeyError: - # already confirmed, redirect to dashboard - status_message = f'This email {clean_email} has already been confirmed.' - kind = 'warning' - user.email_last_sent = timezone.now() - user.save() - else: - status_message = ('You have recently requested to resend your confirmation email. ' - 'Please wait a few minutes before trying again.') - kind = 'error' - status.push_status_message(status_message, kind=kind, trust=False) - else: - forms.push_errors_to_status(form.errors) + if form.validate(): + clean_email = form.email.data + user = get_user(email=clean_email) + status_message = ( + f'If there is an OSF account associated with this unconfirmed email address {clean_email}, ' + 'a confirmation email has been resent to it. If you do not receive an email and believe ' + 'you should have, please contact OSF Support.' + ) + kind = 'success' + if user: + if throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE): + try: + send_confirm_email_async(user, clean_email, renew=True) + # send_confirm_email(user, clean_email, renew=True) + except KeyError: + # already confirmed, redirect to dashboard + status_message = f'This email {clean_email} has already been confirmed.' + kind = 'warning' + user.email_last_sent = timezone.now() + user.save() + else: + status_message = ('You have recently requested to resend your confirmation email. ' + 'Please wait a few minutes before trying again.') + kind = 'error' + status.push_status_message(status_message, kind=kind, trust=False) + else: + forms.push_errors_to_status(form.errors) + except Exception as err: + sentry.log_exception(f'Async email confirmation failed because of the error: {err}') # Don't go anywhere return {'form': form} From 1fe3759c3e278801e30a2095f1fc8d48c2433d04 Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Wed, 7 May 2025 15:43:59 +0300 Subject: [PATCH 08/11] [ENG-7871] Removed restrictions that prohibit options deletion (#11128) ## Purpose Admins should be able to remove collections options without any restrictions ## Changes Removed validation ## Ticket https://openscience.atlassian.net/browse/ENG-7871 --- admin/collection_providers/forms.py | 86 +---------------------------- 1 file changed, 1 insertion(+), 85 deletions(-) diff --git a/admin/collection_providers/forms.py b/admin/collection_providers/forms.py index feae16aa695..7d0e8dadf46 100644 --- a/admin/collection_providers/forms.py +++ b/admin/collection_providers/forms.py @@ -3,7 +3,7 @@ from django import forms from framework.utils import sanitize_html -from osf.models import CollectionProvider, CollectionSubmission +from osf.models import CollectionProvider from admin.base.utils import get_nodelicense_choices, get_defaultlicense_choices, validate_slug @@ -74,12 +74,6 @@ def clean_collected_type_choices(self): type_choices_new = {c.strip(' ') for c in json.loads(self.data.get('collected_type_choices'))} type_choices_added = type_choices_new - type_choices_old type_choices_removed = type_choices_old - type_choices_new - for item in type_choices_removed: - if CollectionSubmission.objects.filter(collection=collection_provider.primary_collection, - collected_type=item).exists(): - raise forms.ValidationError( - f'Cannot delete "{item}" because it is used as metadata on objects.' - ) else: # if this is creating a CollectionProvider type_choices_added = [] @@ -104,12 +98,6 @@ def clean_status_choices(self): status_choices_new = {c.strip(' ') for c in json.loads(self.data.get('status_choices'))} status_choices_added = status_choices_new - status_choices_old status_choices_removed = status_choices_old - status_choices_new - for item in status_choices_removed: - if CollectionSubmission.objects.filter(collection=collection_provider.primary_collection, - status=item).exists(): - raise forms.ValidationError( - f'Cannot delete "{item}" because it is used as metadata on objects.' - ) else: # if this is creating a CollectionProvider status_choices_added = [] @@ -134,12 +122,6 @@ def clean_volume_choices(self): volume_choices_new = {c.strip(' ') for c in json.loads(self.data.get('volume_choices'))} volume_choices_added = volume_choices_new - volume_choices_old volume_choices_removed = volume_choices_old - volume_choices_new - for item in volume_choices_removed: - if CollectionSubmission.objects.filter(collection=collection_provider.primary_collection, - volume=item).exists(): - raise forms.ValidationError( - f'Cannot delete "{item}" because it is used as metadata on objects.' - ) else: # if this is creating a CollectionProvider volume_choices_added = [] @@ -164,12 +146,6 @@ def clean_issue_choices(self): issue_choices_new = {c.strip(' ') for c in json.loads(self.data.get('issue_choices'))} issue_choices_added = issue_choices_new - issue_choices_old issue_choices_removed = issue_choices_old - issue_choices_new - for item in issue_choices_removed: - if CollectionSubmission.objects.filter(collection=collection_provider.primary_collection, - issue=item).exists(): - raise forms.ValidationError( - f'Cannot delete "{item}" because it is used as metadata on objects.' - ) else: # if this is creating a CollectionProvider issue_choices_added = [] @@ -194,12 +170,6 @@ def clean_program_area_choices(self): program_area_choices_new = {c.strip(' ') for c in json.loads(self.data.get('program_area_choices'))} program_area_choices_added = program_area_choices_new - program_area_choices_old program_area_choices_removed = program_area_choices_old - program_area_choices_new - for item in program_area_choices_removed: - if CollectionSubmission.objects.filter(collection=collection_provider.primary_collection, - program_area=item).exists(): - raise forms.ValidationError( - f'Cannot delete "{item}" because it is used as metadata on objects.' - ) else: # if this is creating a CollectionProvider program_area_choices_added = [] @@ -224,16 +194,6 @@ def clean_school_type_choices(self): updated_choices = {c.strip(' ') for c in json.loads(self.data.get('school_type_choices'))} added_choices = updated_choices - old_choices removed_choices = old_choices - updated_choices - active_removed_choices = set( - primary_collection.collectionsubmission_set.filter( - school_type__in=removed_choices - ).values_list('school_type', flat=True) - ) - if active_removed_choices: - raise forms.ValidationError( - 'Cannot remove the following choices for "school_type", as they are ' - f'currently in use: {active_removed_choices}' - ) else: # Creating a new CollectionProvider added_choices = set() removed_choices = set() @@ -253,17 +213,6 @@ def clean_study_design_choices(self): updated_choices = {c.strip(' ') for c in json.loads(self.data.get('study_design_choices'))} added_choices = updated_choices - old_choices removed_choices = old_choices - updated_choices - - active_removed_choices = set( - primary_collection.collectionsubmission_set.filter( - study_design__in=removed_choices - ).values_list('school_type', flat=True) - ) - if active_removed_choices: - raise forms.ValidationError( - 'Cannot remove the following choices for "study_design", as they are ' - f'currently in use: {active_removed_choices}' - ) else: # Creating a new CollectionProvider added_choices = set() removed_choices = set() @@ -283,17 +232,6 @@ def clean_disease_choices(self): updated_choices = {c.strip(' ') for c in json.loads(self.data.get('disease_choices'))} added_choices = updated_choices - old_choices removed_choices = old_choices - updated_choices - - active_removed_choices = set( - primary_collection.collectionsubmission_set.filter( - disease__in=removed_choices - ).values_list('disease', flat=True) - ) - if active_removed_choices: - raise forms.ValidationError( - 'Cannot remove the following choices for "disease", as they are ' - f'currently in use: {active_removed_choices}' - ) else: # Creating a new CollectionProvider added_choices = set() removed_choices = set() @@ -313,17 +251,6 @@ def clean_data_type_choices(self): updated_choices = {c.strip(' ') for c in json.loads(self.data.get('data_type_choices'))} added_choices = updated_choices - old_choices removed_choices = old_choices - updated_choices - - active_removed_choices = set( - primary_collection.collectionsubmission_set.filter( - data_type__in=removed_choices - ).values_list('data_type', flat=True) - ) - if active_removed_choices: - raise forms.ValidationError( - 'Cannot remove the following choices for "data_type", as they are ' - f'currently in use: {active_removed_choices}' - ) else: # Creating a new CollectionProvider added_choices = set() removed_choices = set() @@ -343,17 +270,6 @@ def clean_grade_levels_choices(self): updated_choices = {c.strip(' ') for c in json.loads(self.data.get('grade_levels_choices'))} added_choices = updated_choices - old_choices removed_choices = old_choices - updated_choices - - active_removed_choices = set( - primary_collection.collectionsubmission_set.filter( - data_type__in=removed_choices - ).values_list('grade_levels', flat=True) - ) - if active_removed_choices: - raise forms.ValidationError( - 'Cannot remove the following choices for "grade_levels", as they are ' - f'currently in use: {active_removed_choices}' - ) else: # Creating a new CollectionProvider added_choices = set() removed_choices = set() From b046a63a339a5624d2baa05ee418aee4549836d3 Mon Sep 17 00:00:00 2001 From: antkryt Date: Wed, 7 May 2025 22:29:10 +0300 Subject: [PATCH 09/11] [ENG-7928] Unable to add a user as an admin or moderator to a Socarxive preprint provider. (#11131) ## Purpose Show only relevant groups for preprint provider ## Ticket https://openscience.atlassian.net/browse/ENG-7928 --- admin/preprint_providers/forms.py | 7 ++++--- admin/preprint_providers/views.py | 17 ++++++++--------- osf/models/mixins.py | 9 +++++++-- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/admin/preprint_providers/forms.py b/admin/preprint_providers/forms.py index 06d27052098..1393aae41ef 100644 --- a/admin/preprint_providers/forms.py +++ b/admin/preprint_providers/forms.py @@ -112,11 +112,12 @@ class PreprintProviderRegisterModeratorOrAdminForm(forms.Form): """ A form that finds an existing OSF User, and grants permissions to that user so that they can use the admin app""" - def __init__(self, *args, **kwargs): - provider_id = kwargs.pop('provider_id') + def __init__(self, *args, provider_groups=None, **kwargs): super().__init__(*args, **kwargs) + + provider_groups = provider_groups or Group.objects.none() self.fields['group_perms'] = forms.ModelMultipleChoiceField( - queryset=Group.objects.filter(name__startswith=f'reviews_preprint_{provider_id}'), + queryset=provider_groups, required=False, widget=forms.CheckboxSelectMultiple ) diff --git a/admin/preprint_providers/views.py b/admin/preprint_providers/views.py index e98aed1ecfa..4c7439f4554 100644 --- a/admin/preprint_providers/views.py +++ b/admin/preprint_providers/views.py @@ -12,6 +12,7 @@ from django.contrib.auth.mixins import PermissionRequiredMixin from django.forms.models import model_to_dict from django.shortcuts import redirect, render +from django.utils.functional import cached_property from admin.base import settings from admin.base.forms import ImportFileForm @@ -459,14 +460,18 @@ class PreprintProviderRegisterModeratorOrAdmin(PermissionRequiredMixin, FormView template_name = 'preprint_providers/register_moderator_admin.html' form_class = PreprintProviderRegisterModeratorOrAdminForm + @cached_property + def target_provider(self): + return PreprintProvider.objects.get(id=self.kwargs['preprint_provider_id']) + def get_form_kwargs(self): kwargs = super().get_form_kwargs() - kwargs['provider_id'] = self.kwargs['preprint_provider_id'] + kwargs['provider_groups'] = self.target_provider.group_objects return kwargs def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - context['provider_name'] = PreprintProvider.objects.get(id=self.kwargs['preprint_provider_id']).name + context['provider_name'] = self.target_provider.name return context def form_valid(self, form): @@ -477,13 +482,7 @@ def form_valid(self, form): raise Http404(f'OSF user with id "{user_id}" not found. Please double check.') for group in form.cleaned_data.get('group_perms'): - osf_user.groups.add(group) - split = group.name.split('_') - group_type = split[0] - if group_type == 'reviews': - provider_id = split[2] - provider = PreprintProvider.objects.get(id=provider_id) - provider.notification_subscriptions.get(event_name='new_pending_submissions').add_user_to_subscription(osf_user, 'email_transactional') + self.target_provider.add_to_group(osf_user, group) osf_user.save() messages.success(self.request, f'Permissions update successful for OSF User {osf_user.username}!') diff --git a/osf/models/mixins.py b/osf/models/mixins.py index ac8d5a094d8..b7fe97b7ece 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1061,12 +1061,17 @@ def get_request_state_counts(self): return counts def add_to_group(self, user, group): + if isinstance(group, Group): + group.user_set.add(user) + elif isinstance(group, str): + self.get_group(group).user_set.add(user) + else: + raise TypeError(f"Unsupported group type: {type(group)}") + # Add default notification subscription for subscription in self.DEFAULT_SUBSCRIPTIONS: self.add_user_to_subscription(user, f'{self._id}_{subscription}') - return self.get_group(group).user_set.add(user) - def remove_from_group(self, user, group, unsubscribe=True): _group = self.get_group(group) if group == ADMIN: From 8cb38c797d712d3d53a9e3941f2a6f1f1c8c7c04 Mon Sep 17 00:00:00 2001 From: antkryt Date: Thu, 8 May 2025 17:28:30 +0300 Subject: [PATCH 10/11] [ENG-4673] Add search by ORCID functionality to admin (#11129) ## Purpose Add search by ORCID field to users search admin page ## Ticket https://openscience.atlassian.net/browse/ENG-4673 --- admin/users/forms.py | 1 + admin/users/views.py | 14 +++++++++++ admin_tests/users/test_views.py | 43 ++++++++++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/admin/users/forms.py b/admin/users/forms.py index 737b8c8f4a3..e64a648ff77 100644 --- a/admin/users/forms.py +++ b/admin/users/forms.py @@ -14,6 +14,7 @@ class UserSearchForm(forms.Form): guid = forms.CharField(label='guid', min_length=5, max_length=5, required=False) # TODO: Move max to 6 when needed name = forms.CharField(label='name', required=False) email = forms.EmailField(label='email', required=False) + orcid = forms.CharField(label='orcid', required=False) class MergeUserForm(forms.Form): diff --git a/admin/users/views.py b/admin/users/views.py index 6976c978eec..38787a84a23 100644 --- a/admin/users/views.py +++ b/admin/users/views.py @@ -25,6 +25,7 @@ from framework.auth.core import generate_verification_key from website import search +from website.settings import EXTERNAL_IDENTITY_PROFILE from osf.models.admin_log_entry import ( update_admin_log, @@ -126,6 +127,7 @@ def form_valid(self, form): guid = form.cleaned_data['guid'] name = form.cleaned_data['name'] email = form.cleaned_data['email'] + orcid = form.cleaned_data['orcid'] if name: return redirect(reverse('users:search-list', kwargs={'name': name})) @@ -148,6 +150,18 @@ def form_valid(self, form): return redirect(reverse('users:user', kwargs={'guid': guid})) + if orcid: + external_id_provider = EXTERNAL_IDENTITY_PROFILE.get('OrcidProfile') + user = get_user(external_id_provider=external_id_provider, external_id=orcid) + + if not user: + return page_not_found( + self.request, + AttributeError(f'resource with id "{orcid}" not found.') + ) + + return redirect(reverse('users:user', kwargs={'guid': user._id})) + return super().form_valid(form) diff --git a/admin_tests/users/test_views.py b/admin_tests/users/test_views.py index 1b878f00509..d22211fe848 100644 --- a/admin_tests/users/test_views.py +++ b/admin_tests/users/test_views.py @@ -402,7 +402,14 @@ def test_correct_view_permissions(self): class TestUserSearchView(AdminTestCase): def setUp(self): - self.user_1 = AuthUserFactory(fullname='Broken Matt Hardy') + self.user_1 = AuthUserFactory( + fullname='Broken Matt Hardy', + external_identity={ + settings.EXTERNAL_IDENTITY_PROFILE.get('OrcidProfile'): { + '1234-5678': 'VERIFIED' + } + } + ) self.user_2 = AuthUserFactory(fullname='Jeff Hardy') self.user_3 = AuthUserFactory(fullname='Reby Sky') self.user_4 = AuthUserFactory(fullname='King Maxel Hardy') @@ -425,6 +432,14 @@ def test_search_user_by_guid(self): assert response.status_code == 302 assert response.headers['location'] == f'/users/{self.user_1.guids.first()._id}/' + form_data = { + 'guid': 'wrong' + } + form = UserSearchForm(data=form_data) + assert form.is_valid() + response = self.view.form_valid(form) + assert response.status_code == 404 + def test_search_user_by_name(self): form_data = { 'name': 'Hardy' @@ -455,6 +470,14 @@ def test_search_user_by_username(self): assert response.status_code == 302 assert response.headers['location'] == f'/users/{self.user_1.guids.first()._id}/' + form_data = { + 'email': 'wrong@email.com' + } + form = UserSearchForm(data=form_data) + assert form.is_valid() + response = self.view.form_valid(form) + assert response.status_code == 404 + def test_search_user_by_alternate_email(self): form_data = { 'email': self.user_2_alternate_email @@ -487,6 +510,24 @@ def test_search_user_list_case_insensitive(self): for user in results: assert 'Hardy' in user.fullname + def test_search_user_by_orcid(self): + form_data = { + 'orcid': '1234-5678' + } + form = UserSearchForm(data=form_data) + assert form.is_valid() + response = self.view.form_valid(form) + assert response.status_code == 302 + assert response.headers['location'] == f'/users/{self.user_1.guids.first()._id}/' + + form_data = { + 'orcid': '1234-5678-90' + } + form = UserSearchForm(data=form_data) + assert form.is_valid() + response = self.view.form_valid(form) + assert response.status_code == 404 + class TestGetLinkView(AdminTestCase): From 9f366cd22d71fb01f9084c517b0e15c2463edf9d Mon Sep 17 00:00:00 2001 From: ihorsokhanexoft Date: Thu, 8 May 2025 20:20:41 +0300 Subject: [PATCH 11/11] revert async email sending (#11134) ## Purpose Revert async function that causes 502 --- framework/auth/views.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/framework/auth/views.py b/framework/auth/views.py index 3c9d1f5b663..26aa494ddd4 100644 --- a/framework/auth/views.py +++ b/framework/auth/views.py @@ -989,8 +989,7 @@ def resend_confirmation_post(auth): if user: if throttle_period_expired(user.email_last_sent, settings.SEND_EMAIL_THROTTLE): try: - send_confirm_email_async(user, clean_email, renew=True) - # send_confirm_email(user, clean_email, renew=True) + send_confirm_email(user, clean_email, renew=True) except KeyError: # already confirmed, redirect to dashboard status_message = f'This email {clean_email} has already been confirmed.'