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() 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/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/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/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 85b6eac4ee6..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) @@ -456,9 +470,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/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/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): 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/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/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/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 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']: 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): diff --git a/framework/auth/views.py b/framework/auth/views.py index e83f47b5db2..26aa494ddd4 100644 --- a/framework/auth/views.py +++ b/framework/auth/views.py @@ -970,39 +970,41 @@ 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(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} 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/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( diff --git a/osf/models/mixins.py b/osf/models/mixins.py index facc39af119..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: @@ -2222,7 +2227,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/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'), 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