diff --git a/api/nodes/views.py b/api/nodes/views.py index ccd26a16a47..bb38bcbab3c 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -240,11 +240,11 @@ def get_draft(self, draft_id=None, check_object_permissions=True): self.check_branched_from(draft) if self.request.method not in drf_permissions.SAFE_METHODS: - if draft.registered_node and not draft.registered_node.is_deleted: + if draft.has_active_registration: raise PermissionDenied('This draft has already been registered and cannot be modified.') else: - if draft.registered_node and not draft.registered_node.is_deleted: + if draft.has_active_registration: redirect_url = draft.registered_node.absolute_api_v2_url self.headers['location'] = redirect_url raise PermanentlyMovedError(detail='Draft has already been registered') diff --git a/api/schema_responses/permissions.py b/api/schema_responses/permissions.py index c5a8c3b2906..41025258633 100644 --- a/api/schema_responses/permissions.py +++ b/api/schema_responses/permissions.py @@ -11,7 +11,11 @@ from osf.utils.workflows import ApprovalStates -MODERATOR_VISIBLE_STATES = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] +MODERATOR_VISIBLE_STATES = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, +] class SchemaResponseParentPermission: @@ -20,7 +24,7 @@ class SchemaResponseParentPermission: To GET a SchemaResponse (or a subpath), one of three conditions must be met: * The user must have "read" permissions on the parent resource * The user must be a moderator on the parent resource's Provider and the - SchemaResponse must be in an APPROVED or PENDING_MODERATION state + SchemaResponse must be in an APPROVED, PENDING_MODERATION or MODERATOR_REJECTED state * The SchemaResponse must be APPROVED and the parent resource must be public For DELETE/PATCH/POST/PUT, the required permission should be added in the diff --git a/api_tests/actions/views/test_schema_response_action_detail.py b/api_tests/actions/views/test_schema_response_action_detail.py index 46fcbde63fa..a4d3938c3dc 100644 --- a/api_tests/actions/views/test_schema_response_action_detail.py +++ b/api_tests/actions/views/test_schema_response_action_detail.py @@ -110,10 +110,14 @@ def get_status_code_for_preconditions( if role == 'non-contributor': return 403 - # Moderators can GET PENDING_MODERATION and APPROVED SchemaResponses on + # Moderators can GET PENDING_MODERATION, APPROVED and MODERATOR_REJECTED SchemaResponses on # public or private registrations that are part of a moderated registry if role == 'moderator': - moderator_visible_states = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] if schema_response_state in moderator_visible_states and reviews_workflow is not None: return 200 else: diff --git a/api_tests/actions/views/test_schema_response_action_list.py b/api_tests/actions/views/test_schema_response_action_list.py index a4099965086..1763ce06f92 100644 --- a/api_tests/actions/views/test_schema_response_action_list.py +++ b/api_tests/actions/views/test_schema_response_action_list.py @@ -146,10 +146,14 @@ def get_status_code_for_preconditions( if role == 'non-contributor': return 403 - # Moderators can GET PENDING_MODERATION and APPROVED SchemaResponses on + # Moderators can GET PENDING_MODERATION, APPROVED and MODERATOR_REJECTED SchemaResponses on # public or private registrations that are part of a moderated registry if role == 'moderator': - moderator_visible_states = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] if schema_response_state in moderator_visible_states and reviews_workflow is not None: return 200 else: diff --git a/api_tests/registries_moderation/test_submissions.py b/api_tests/registries_moderation/test_submissions.py index cc44cbae4b6..07290373f7f 100644 --- a/api_tests/registries_moderation/test_submissions.py +++ b/api_tests/registries_moderation/test_submissions.py @@ -1,7 +1,9 @@ import pytest import datetime +from unittest import mock from api.base.settings.defaults import API_BASE +from framework.auth import Auth from api.providers.workflows import Workflows from osf.utils.workflows import NodeRequestTypes, RegistrationModerationTriggers, RegistrationModerationStates @@ -14,7 +16,8 @@ NodeRequestFactory, EmbargoFactory, RetractionFactory, - ProjectFactory + ProjectFactory, + SubjectFactory, ) @@ -351,6 +354,58 @@ def test_registries_moderation_post_reject_moderator(self, app, registration, re assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REJECT_SUBMISSION.db_name registration.refresh_from_db() assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not registration.is_deleted + + def test_moderator_can_view_rejected_registration(self, app, registration, moderator, moderator_wrong_provider, registration_actions_url, actions_payload_base): + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REJECT_SUBMISSION.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + with capture_notifications(): + resp = app.post_json_api(registration_actions_url, actions_payload_base, auth=moderator.auth) + assert resp.status_code == 201 + + registration_url = f'/{API_BASE}registrations/{registration._id}/' + assert app.get(registration_url, auth=moderator.auth).status_code == 200 + resp = app.get(f'{registration_url}schema_responses/', auth=moderator.auth) + assert [entry['id'] for entry in resp.json['data']] == [registration.schema_responses.last()._id] + + # Rejected registrations stay private for everyone else + assert app.get(registration_url, auth=moderator_wrong_provider.auth, expect_errors=True).status_code == 403 + assert app.get(registration_url, auth=AuthUserFactory().auth, expect_errors=True).status_code == 403 + assert app.get(registration_url, expect_errors=True).status_code == 401 + + def test_rejected_submission_draft_is_returned_for_resubmission(self, app, registration, reg_creator, moderator, registration_actions_url, actions_payload_base): + registration.require_approval(user=registration.creator) + with capture_notifications(): + registration.registration_approval.accept() + + actions_payload_base['data']['attributes']['trigger'] = RegistrationModerationTriggers.REJECT_SUBMISSION.db_name + actions_payload_base['data']['relationships']['target']['data']['id'] = registration._id + with capture_notifications(): + resp = app.post_json_api(registration_actions_url, actions_payload_base, auth=moderator.auth) + assert resp.status_code == 201 + + # The draft is back in the author's drafts and can be edited + draft = registration.draft_registration.get() + assert draft in reg_creator.draft_registrations_active + draft_url = f'/{API_BASE}draft_registrations/{draft._id}/' + assert app.get(draft_url, auth=reg_creator.auth).status_code == 200 + payload = {'data': {'type': 'draft_registrations', 'id': draft._id, 'attributes': {'title': 'Fixed title'}}} + assert app.patch_json_api(draft_url, payload, auth=reg_creator.auth).status_code == 200 + + # Resubmitting creates a new registration, the rejected one stays as it is + draft.subjects.add(SubjectFactory()) + with mock.patch('framework.celery_tasks.handlers.enqueue_task'): + resubmitted = draft.register(Auth(reg_creator), save=True) + draft.refresh_from_db() + registration.refresh_from_db() + assert draft.registered_node == resubmitted + assert draft not in reg_creator.draft_registrations_active + assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not registration.is_deleted def test_registries_moderation_post_embargo(self, app, embargo_registration, moderator, provider, embargo_registration_actions_url, actions_payload_base, reg_creator): assert embargo_registration.moderation_state == RegistrationModerationStates.INITIAL.db_name @@ -387,6 +442,7 @@ def test_registries_moderation_post_embargo_reject(self, app, embargo_registrati assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REJECT_SUBMISSION.db_name embargo_registration.refresh_from_db() assert embargo_registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not embargo_registration.is_deleted @pytest.mark.usefixtures('mock_gravy_valet_get_verified_links') def test_registries_moderation_post_withdraw_accept(self, app, retract_registration, moderator, retract_registration_actions_url, actions_payload_base, provider): @@ -627,6 +683,7 @@ def test_rejected_submission_doesnt_break_ham_functionality(self, app, registrat assert resp.json['data']['attributes']['trigger'] == RegistrationModerationTriggers.REJECT_SUBMISSION.db_name registration.refresh_from_db() assert registration.moderation_state == RegistrationModerationStates.REJECTED.db_name + assert not registration.is_deleted # ham the project creator user.confirm_ham(save=True) diff --git a/api_tests/schema_responses/views/test_registrations_schema_responses_list.py b/api_tests/schema_responses/views/test_registrations_schema_responses_list.py index 5f000d4a13f..8cbc963e52c 100644 --- a/api_tests/schema_responses/views/test_registrations_schema_responses_list.py +++ b/api_tests/schema_responses/views/test_registrations_schema_responses_list.py @@ -186,8 +186,8 @@ class TestRegistrationSchemaResponseListGETBehavior: Contributors on the base Registration should be able to see all SchemaResponses on a registration, whether approved or not, for both public and private Registrations. - Moderators should be able to see both PENDING_MODERATION and APPROVED SchemaResponses - on Registrations that are part of the moderated provider. + Moderators should be able to see PENDING_MODERATION, APPROVED and MODERATOR_REJECTED + SchemaResponses on Registrations that are part of the moderated provider. Non-contributors should only see APPROVED SchemaResponses on Public registrations (permissions tests verify 403/401 response for non-contributors on a private registration). @@ -242,7 +242,12 @@ def test_GET__moderated_registration_responses_as_moderator( # Always expect the APPROVED response expected_ids = {updated_response.previous_response._id} - if response_state in [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED]: + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] + if response_state in moderator_visible_states: expected_ids.add(updated_response._id) encountered_ids = {entry['id'] for entry in resp.json['data']} assert encountered_ids == expected_ids diff --git a/api_tests/schema_responses/views/test_schema_response_detail.py b/api_tests/schema_responses/views/test_schema_response_detail.py index 6b19255377b..13dec2daed5 100644 --- a/api_tests/schema_responses/views/test_schema_response_detail.py +++ b/api_tests/schema_responses/views/test_schema_response_detail.py @@ -136,10 +136,14 @@ def get_status_code_for_preconditions( if role == 'non-contributor': return 403 - # Moderators can GET PENDING_MODERATION and APPROVED SchemaResponses on + # Moderators can GET PENDING_MODERATION, APPROVED and MODERATOR_REJECTED SchemaResponses on # public or private registrations that are part of a moderated registry if role == 'moderator': - moderator_visible_states = [ApprovalStates.PENDING_MODERATION, ApprovalStates.APPROVED] + moderator_visible_states = [ + ApprovalStates.PENDING_MODERATION, + ApprovalStates.APPROVED, + ApprovalStates.MODERATOR_REJECTED, + ] if schema_response_state in moderator_visible_states and reviews_workflow is not None: return 200 else: diff --git a/osf/models/node.py b/osf/models/node.py index daf43b0e444..9c6776e758c 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -59,7 +59,7 @@ from osf.utils.datetime_aware_jsonfield import DateTimeAwareJSONField from osf.utils.fields import NonNaiveDateTimeField from osf.utils.requests import get_request_and_user_id, string_type_request_headers, get_current_request -from osf.utils.workflows import CollectionSubmissionStates +from osf.utils.workflows import CollectionSubmissionStates, RegistrationModerationStates from osf.utils import sanitize from website import language, settings from website.citations.utils import datetime_to_csl @@ -680,7 +680,11 @@ def draft_registrations_active(self): return DraftRegistration.objects.filter( models.Q(branched_from=self) & models.Q(deleted__isnull=True) & - (models.Q(registered_node=None) | models.Q(registered_node__deleted__isnull=False)), + ( + models.Q(registered_node=None) + | models.Q(registered_node__deleted__isnull=False) + | models.Q(registered_node__moderation_state=RegistrationModerationStates.REJECTED.db_name) + ), ) @property diff --git a/osf/models/registrations.py b/osf/models/registrations.py index 9aa14d278c4..b2232e8f22f 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -1161,6 +1161,25 @@ def branched_from_type(self): def has_project(self): return isinstance(self.branched_from, Node) + @property + def has_active_registration(self): + """ + Check whether this draft has an active registration, so it can no longer be edited. + A registration that was deleted or rejected by a moderator is not active: its draft is + returned to the contributors for resubmission. + """ + registration = self.registered_node + if registration is None: + # Not registered yet + return False + if registration.is_deleted: + # Rejected by a contributor or failed to archive + return False + if registration.moderation_state == RegistrationModerationStates.REJECTED.db_name: + # Rejected by a moderator + return False + return True + @property def url(self): return self.URL_TEMPLATE.format( diff --git a/osf/models/sanctions.py b/osf/models/sanctions.py index 7796456dbb6..b89c380655d 100644 --- a/osf/models/sanctions.py +++ b/osf/models/sanctions.py @@ -687,6 +687,9 @@ def _on_reject(self, event_data): 'embargo_id': self._id, }, auth=Auth(user) if user else Auth(self.initiated_by)) + # Registrations rejected by a moderator are kept in the REJECTED moderation state + if self.approval_stage is ApprovalStates.MODERATOR_REJECTED: + return # Remove backref to parent project if embargo was for a new registration if not self.for_existing_registration: parent_registration.delete_registration_tree(save=True) @@ -1052,7 +1055,9 @@ def _on_reject(self, event_data): NodeLog = apps.get_model('osf.NodeLog') registered_from = self.target_registration.registered_from - self.target_registration.delete_registration_tree(save=True) + # Registrations rejected by a moderator are kept in the REJECTED moderation state + if self.approval_stage is not ApprovalStates.MODERATOR_REJECTED: + self.target_registration.delete_registration_tree(save=True) registered_from.add_log( action=NodeLog.REGISTRATION_APPROVAL_CANCELLED, params={ diff --git a/osf/models/user.py b/osf/models/user.py index 67530a2c627..64ee2bca5b7 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -65,6 +65,7 @@ from osf.utils.names import impute_names from osf.utils.requests import check_select_for_update from osf.utils.permissions import API_CONTRIBUTOR_PERMISSIONS, MANAGER, MEMBER, ADMIN +from osf.utils.workflows import RegistrationModerationStates from website import settings as website_settings from website import filters from website.project import new_bookmark_collection @@ -971,7 +972,11 @@ def draft_registrations_active(self): """ return self.draft_registrations.filter( - (models.Q(registered_node__isnull=True) | models.Q(registered_node__deleted__isnull=False)), + ( + models.Q(registered_node__isnull=True) + | models.Q(registered_node__deleted__isnull=False) + | models.Q(registered_node__moderation_state=RegistrationModerationStates.REJECTED.db_name) + ), branched_from__deleted__isnull=True, deleted__isnull=True, ) diff --git a/osf/utils/workflows.py b/osf/utils/workflows.py index f562ff0aab3..51f96f5d266 100644 --- a/osf/utils/workflows.py +++ b/osf/utils/workflows.py @@ -128,6 +128,7 @@ def in_moderation_states(cls): cls.EMBARGO.db_name, cls.PENDING_EMBARGO_TERMINATION.db_name, cls.PENDING_WITHDRAW.db_name, + cls.REJECTED.db_name, ] diff --git a/osf_tests/test_draft_registration.py b/osf_tests/test_draft_registration.py index f4b2feb3c1d..a58d216711d 100644 --- a/osf_tests/test_draft_registration.py +++ b/osf_tests/test_draft_registration.py @@ -6,6 +6,7 @@ from osf.exceptions import UserNotAffiliatedError, DraftRegistrationStateError, NodeStateError from osf.models import RegistrationSchema, DraftRegistration, DraftRegistrationContributor, NodeLicense, Node, NodeLog from osf.utils.permissions import ADMIN, READ, WRITE +from osf.utils.workflows import RegistrationModerationStates from osf_tests.test_node import TestNodeEditableFieldsMixin, TestTagging, TestNodeSubjects from osf_tests.test_node_license import TestNodeLicenses from django.utils import timezone @@ -192,6 +193,36 @@ def test_draft_registrations_active(self): assert draft2 in project.draft_registrations_active.all() assert finished_draft not in project.draft_registrations_active.all() + def test_draft_registrations_includes_drafts_of_moderator_rejected_registrations(self): + project = factories.ProjectFactory() + rejected_registration = factories.RegistrationFactory(project=project) + rejected_registration.moderation_state = RegistrationModerationStates.REJECTED.db_name + rejected_registration.save() + + draft = factories.DraftRegistrationFactory(branched_from=project, user=project.creator) + draft.registered_node = rejected_registration + draft.save() + + assert draft in project.draft_registrations_active.all() + assert draft in project.creator.draft_registrations_active.all() + + def test_has_active_registration(self): + project = factories.ProjectFactory() + draft = factories.DraftRegistrationFactory(branched_from=project) + assert not draft.has_active_registration + + registration = factories.RegistrationFactory(project=project) + draft.registered_node = registration + assert draft.has_active_registration + + registration.moderation_state = RegistrationModerationStates.REJECTED.db_name + assert not draft.has_active_registration + + registration.moderation_state = RegistrationModerationStates.ACCEPTED.db_name + registration.is_deleted = True + registration.deleted = timezone.now() + assert not draft.has_active_registration + def test_draft_registration_url(self): project = factories.ProjectFactory() draft = factories.DraftRegistrationFactory(branched_from=project) diff --git a/tests/test_registrations/test_review_flows.py b/tests/test_registrations/test_review_flows.py index 9f6c4306cfd..cfab164b37c 100644 --- a/tests/test_registrations/test_review_flows.py +++ b/tests/test_registrations/test_review_flows.py @@ -355,6 +355,7 @@ def test_moderator_rejection_flow( registration.refresh_from_db() assert registration.moderation_state == end_state.db_name + assert not registration.is_deleted @pytest.mark.parametrize('sanction_object', [registration_approval, embargo, retraction]) def test_admin_cannot_give_moderator_approval(self, sanction_object, provider): diff --git a/website/project/views/drafts.py b/website/project/views/drafts.py index e4ecd1bc92a..56d5844e7bc 100644 --- a/website/project/views/drafts.py +++ b/website/project/views/drafts.py @@ -97,8 +97,7 @@ def validate_registration_choice(registration_choice): ) def check_draft_state(draft): - registered_and_deleted = draft.registered_node and draft.registered_node.is_deleted - if draft.registered_node and not registered_and_deleted: + if draft.has_active_registration: raise HTTPError(http_status.HTTP_403_FORBIDDEN, data={ 'message_short': 'This draft has already been registered', 'message_long': 'This draft has already been registered and cannot be modified.' @@ -192,7 +191,7 @@ def delete_draft_registration(auth, node, draft, *args, **kwargs): :return: None :rtype: NoneType """ - if draft.registered_node and not draft.registered_node.is_deleted: + if draft.has_active_registration: raise HTTPError( http_status.HTTP_403_FORBIDDEN, data={