From 51916ffb368b3f1db252cce10116c5bbba8c365e Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Thu, 11 Jul 2024 09:13:30 -0400 Subject: [PATCH] refactor reviews machine and fix validation for reviews machine --- api/actions/permissions.py | 12 ++-- api/actions/serializers.py | 16 +++--- api/actions/views.py | 14 ++--- api/preprints/views.py | 8 +-- osf/external/chronos/chronos.py | 4 +- .../commands/create_fake_preprint_actions.py | 4 +- osf/metadata/osf_gathering.py | 2 +- ...0022_rename_reviewaction_preprintaction.py | 17 ++++++ osf/models/__init__.py | 2 +- osf/models/action.py | 12 ++-- osf/models/mixins.py | 14 ++--- osf/models/preprint.py | 4 +- osf/utils/machines.py | 28 +++++---- osf/utils/workflows.py | 57 ++++++++++++++++--- osf_tests/factories.py | 2 +- osf_tests/metadata/test_osf_gathering.py | 2 +- 16 files changed, 131 insertions(+), 67 deletions(-) create mode 100644 osf/migrations/0022_rename_reviewaction_preprintaction.py diff --git a/api/actions/permissions.py b/api/actions/permissions.py index 01525369a60..71dc5b5a578 100644 --- a/api/actions/permissions.py +++ b/api/actions/permissions.py @@ -7,16 +7,16 @@ from osf.models import CollectionSubmissionAction from osf.models.action import BaseAction from osf.models.mixins import ReviewableMixin, ReviewProviderMixin -from osf.utils.workflows import ReviewTriggers +from osf.utils.workflows import PreprintTriggers from osf.utils import permissions as osf_permissions # Required permission to perform each action. `None` means no permissions required. TRIGGER_PERMISSIONS = { - ReviewTriggers.SUBMIT.value: None, - ReviewTriggers.ACCEPT.value: 'accept_submissions', - ReviewTriggers.REJECT.value: 'reject_submissions', - ReviewTriggers.WITHDRAW.value: 'withdraw_submissions', - ReviewTriggers.EDIT_COMMENT.value: 'edit_review_comments', + PreprintTriggers.SUBMIT.value: None, + PreprintTriggers.ACCEPT.value: 'accept_submissions', + PreprintTriggers.REJECT.value: 'reject_submissions', + PreprintTriggers.WITHDRAW.value: 'withdraw_submissions', + PreprintTriggers.EDIT_COMMENT.value: 'edit_review_comments', } diff --git a/api/actions/serializers.py b/api/actions/serializers.py index 866334a0af5..c2ae9352fb0 100644 --- a/api/actions/serializers.py +++ b/api/actions/serializers.py @@ -31,8 +31,8 @@ from osf.utils.workflows import ( DefaultStates, DefaultTriggers, - ReviewStates, - ReviewTriggers, + PreprintStates, + PreprintTriggers, RegistrationModerationTriggers, SchemaResponseTriggers, ) @@ -167,7 +167,7 @@ class Meta: type_ = 'actions' abstract = True -class ReviewActionSerializer(BaseActionSerializer): +class PreprintActionSerializer(BaseActionSerializer): class Meta: type_ = 'review-actions' @@ -183,9 +183,9 @@ class Meta: ]) comment = HideIfProviderCommentsPrivate(ser.CharField(max_length=65535, required=False)) - trigger = ser.ChoiceField(choices=ReviewTriggers.choices()) - from_state = ser.ChoiceField(choices=ReviewStates.choices(), read_only=True) - to_state = ser.ChoiceField(choices=ReviewStates.choices(), read_only=True) + trigger = ser.ChoiceField(choices=PreprintTriggers.choices()) + from_state = ser.ChoiceField(choices=PreprintStates.choices(), read_only=True) + to_state = ser.ChoiceField(choices=PreprintStates.choices(), read_only=True) provider = RelationshipField( read_only=True, @@ -212,8 +212,8 @@ class Meta: def create(self, validated_data): trigger = validated_data.get('trigger') - if trigger != ReviewTriggers.WITHDRAW.value: - return super(ReviewActionSerializer, self).create(validated_data) + if trigger != PreprintTriggers.WITHDRAW.value: + return super().create(validated_data) user = validated_data.pop('user') target = validated_data.pop('target') comment = validated_data.pop('comment', '') diff --git a/api/actions/views.py b/api/actions/views.py index be0f47a0a8d..d5fe322acce 100644 --- a/api/actions/views.py +++ b/api/actions/views.py @@ -7,7 +7,7 @@ from rest_framework.exceptions import NotFound, PermissionDenied from api.actions.permissions import ReviewActionPermission -from api.actions.serializers import NodeRequestActionSerializer, ReviewActionSerializer, PreprintRequestActionSerializer +from api.actions.serializers import NodeRequestActionSerializer, PreprintActionSerializer, PreprintRequestActionSerializer from api.base.exceptions import Conflict from api.base.filters import ListFilterMixin from api.base.views import JSONAPIBaseView @@ -22,7 +22,7 @@ from framework.auth.oauth_scopes import CoreScopes from osf.models import ( PreprintProvider, - ReviewAction, + PreprintAction, NodeRequestAction, PreprintRequestAction, BaseAction, @@ -31,7 +31,7 @@ def get_review_actions_queryset(): - return ReviewAction.objects.prefetch_related( + return PreprintAction.objects.prefetch_related( 'creator__guids', 'target__guids', 'target__provider', @@ -78,13 +78,13 @@ class ActionDetail(JSONAPIBaseView, generics.RetrieveAPIView): required_read_scopes = [CoreScopes.ACTIONS_READ] required_write_scopes = [CoreScopes.ACTIONS_WRITE] - serializer_class = ReviewActionSerializer + serializer_class = PreprintActionSerializer view_category = 'actions' view_name = 'action-detail' def get_serializer_class(self): # Not allowed to view NodeRequestActions yet, making extra logic unnecessary - return ReviewActionSerializer + return PreprintActionSerializer def get_object(self): action = None @@ -162,8 +162,8 @@ class ReviewActionListCreate(JSONAPIBaseView, generics.ListCreateAPIView, ListFi required_write_scopes = [CoreScopes.ACTIONS_WRITE] parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON,) - serializer_class = ReviewActionSerializer - model_class = ReviewAction + serializer_class = PreprintActionSerializer + model_class = PreprintAction ordering = ('-created',) view_category = 'actions' diff --git a/api/preprints/views.py b/api/preprints/views.py index 302b8623102..a96694c3dbc 100644 --- a/api/preprints/views.py +++ b/api/preprints/views.py @@ -6,11 +6,11 @@ from rest_framework import permissions as drf_permissions from framework.auth.oauth_scopes import CoreScopes -from osf.models import ReviewAction, Preprint, PreprintContributor +from osf.models import PreprintAction, Preprint, PreprintContributor from osf.utils.requests import check_select_for_update from api.actions.permissions import ReviewActionPermission -from api.actions.serializers import ReviewActionSerializer +from api.actions.serializers import PreprintActionSerializer from api.actions.views import get_review_actions_queryset from api.base.pagination import PreprintContributorPagination from api.base.exceptions import Conflict @@ -535,8 +535,8 @@ class PreprintActionList(JSONAPIBaseView, generics.ListCreateAPIView, ListFilter required_write_scopes = [CoreScopes.ACTIONS_WRITE] parser_classes = (JSONAPIMultipleRelationshipsParser, JSONAPIMultipleRelationshipsParserForRegularJSON,) - serializer_class = ReviewActionSerializer - model_class = ReviewAction + serializer_class = PreprintActionSerializer + model_class = PreprintAction ordering = ('-created',) view_category = 'preprints' diff --git a/osf/external/chronos/chronos.py b/osf/external/chronos/chronos.py index 08a9ac1aed2..31cd20202c1 100644 --- a/osf/external/chronos/chronos.py +++ b/osf/external/chronos/chronos.py @@ -6,7 +6,7 @@ from api.files.serializers import get_file_download_link from osf.models import ChronosJournal from osf.models import ChronosSubmission -from osf.utils.workflows import ChronosSubmissionStatus, ReviewStates +from osf.utils.workflows import ChronosSubmissionStatus, PreprintStates from website.settings import ( CHRONOS_USE_FAKE_FILE, CHRONOS_FAKE_FILE_URL, CHRONOS_API_KEY, CHRONOS_USERNAME, CHRONOS_PASSWORD, CHRONOS_HOST, VERIFY_CHRONOS_SSL_CERT @@ -210,7 +210,7 @@ def submit_manuscript(self, journal, preprint, submitter): raise ValueError('Cannot submit because your submission was accepted') if submission_qs.filter(status=4).exists(): raise ValueError('Cannot submit because your submission was published') - if preprint.machine_state != ReviewStates.ACCEPTED.value: + if preprint.machine_state != PreprintStates.ACCEPTED.value: raise ValueError('Cannot submit to Chronos if the preprint is not accepted by moderators') body = ChronosSerializer.serialize_manuscript(journal.journal_id, preprint) diff --git a/osf/management/commands/create_fake_preprint_actions.py b/osf/management/commands/create_fake_preprint_actions.py index 5373cbeb143..08186f5c180 100644 --- a/osf/management/commands/create_fake_preprint_actions.py +++ b/osf/management/commands/create_fake_preprint_actions.py @@ -8,7 +8,7 @@ from django.core.management.base import BaseCommand -from osf.models import ReviewAction, Preprint, OSFUser +from osf.models import PreprintAction, Preprint, OSFUser from osf.utils.workflows import DefaultStates, DefaultTriggers logger = logging.getLogger(__name__) @@ -48,7 +48,7 @@ def handle(self, *args, **options): states = [s.value for s in DefaultStates] for preprint in Preprint.objects.filter(actions__isnull=True): for i in range(num_actions): - action = ReviewAction( + action = PreprintAction( target=preprint, creator=user, trigger=random.choice(triggers), diff --git a/osf/metadata/osf_gathering.py b/osf/metadata/osf_gathering.py index 6e5e25c6d0b..bb300d42043 100644 --- a/osf/metadata/osf_gathering.py +++ b/osf/metadata/osf_gathering.py @@ -436,7 +436,7 @@ def gather_preprint_withdrawal(focus): _preprint = focus.dbmodel yield (OSF.dateWithdrawn, _preprint.date_withdrawn) _withdrawal_request = _preprint.requests.filter( - machine_state=osfworkflows.ReviewStates.ACCEPTED.value, + machine_state=osfworkflows.PreprintStates.ACCEPTED.value, request_type=osfworkflows.RequestTypes.WITHDRAWAL.value, ).last() if _withdrawal_request: diff --git a/osf/migrations/0022_rename_reviewaction_preprintaction.py b/osf/migrations/0022_rename_reviewaction_preprintaction.py new file mode 100644 index 00000000000..234aae199d5 --- /dev/null +++ b/osf/migrations/0022_rename_reviewaction_preprintaction.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.17 on 2024-07-11 13:34 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('osf', '0021_preprint_custom_publication_citation'), + ] + + operations = [ + migrations.RenameModel( + old_name='ReviewAction', + new_name='PreprintAction', + ), + ] diff --git a/osf/models/__init__.py b/osf/models/__init__.py index cad31ea323f..2d9379c33eb 100644 --- a/osf/models/__init__.py +++ b/osf/models/__init__.py @@ -5,7 +5,7 @@ NodeRequestAction, PreprintRequestAction, RegistrationAction, - ReviewAction, + PreprintAction, SchemaResponseAction, ) from .admin_log_entry import AdminLogEntry diff --git a/osf/models/action.py b/osf/models/action.py index 6de1bc24b63..30ef4229b99 100644 --- a/osf/models/action.py +++ b/osf/models/action.py @@ -8,8 +8,8 @@ ApprovalStates, DefaultStates, DefaultTriggers, - ReviewStates, - ReviewTriggers, + PreprintStates, + PreprintTriggers, RegistrationModerationTriggers, RegistrationModerationStates, SchemaResponseTriggers, @@ -40,12 +40,12 @@ def target(self): raise NotImplementedError() -class ReviewAction(BaseAction): +class PreprintAction(BaseAction): target = models.ForeignKey('Preprint', related_name='actions', on_delete=models.CASCADE) - trigger = models.CharField(max_length=31, choices=ReviewTriggers.choices()) - from_state = models.CharField(max_length=31, choices=ReviewStates.choices()) - to_state = models.CharField(max_length=31, choices=ReviewStates.choices()) + trigger = models.CharField(max_length=31, choices=PreprintTriggers.choices()) + from_state = models.CharField(max_length=31, choices=PreprintStates.choices()) + to_state = models.CharField(max_length=31, choices=PreprintStates.choices()) class NodeRequestAction(BaseAction): diff --git a/osf/models/mixins.py b/osf/models/mixins.py index dfc00364f97..207d0fd8053 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -35,7 +35,7 @@ from osf.utils.fields import NonNaiveDateTimeField from osf.utils.datetime_aware_jsonfield import DateTimeAwareJSONField from osf.utils.machines import ( - ReviewsMachine, + PreprintStateMachine, NodeRequestMachine, PreprintRequestMachine, ) @@ -45,8 +45,8 @@ from osf.utils.workflows import ( DefaultStates, DefaultTriggers, - ReviewStates, - ReviewTriggers, + PreprintStates, + PreprintTriggers, ) from osf.utils.requests import get_request_and_user_id @@ -874,14 +874,14 @@ class Meta: class ReviewableMixin(MachineableMixin): """Something that may be included in a reviewed collection and is subject to a reviews workflow. """ - TriggersClass = ReviewTriggers + TriggersClass = PreprintTriggers - machine_state = models.CharField(max_length=15, db_index=True, choices=ReviewStates.choices(), default=ReviewStates.INITIAL.value) + machine_state = models.CharField(max_length=15, db_index=True, choices=PreprintStates.choices(), default=PreprintStates.INITIAL.value) class Meta: abstract = True - MachineClass = ReviewsMachine + MachineClass = PreprintStateMachine @property def in_public_reviews_state(self): @@ -960,7 +960,7 @@ class ReviewProviderMixin(GuardianMixin): """ REVIEWABLE_RELATION_NAME = None - REVIEW_STATES = ReviewStates + REVIEW_STATES = PreprintStates STATE_FIELD_NAME = 'machine_state' groups = REVIEW_GROUPS diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 0a188e9f28b..f802f08d6bd 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -29,7 +29,7 @@ from .mixins import ReviewableMixin, Taggable, Loggable, GuardianMixin from .validators import validate_doi from osf.utils.fields import NonNaiveDateTimeField -from osf.utils.workflows import DefaultStates, ReviewStates +from osf.utils.workflows import DefaultStates, PreprintStates from osf.utils import sanitize from osf.utils.permissions import ADMIN, WRITE from osf.utils.requests import get_request_and_user_id, string_type_request_headers @@ -581,7 +581,7 @@ def set_published(self, published, auth, save=False): self.set_privacy('public', log=False, save=False) # In case this provider is ever set up to use a reviews workflow, put this preprint in a sensible state - self.machine_state = ReviewStates.ACCEPTED.value + self.machine_state = PreprintStates.ACCEPTED.value self.date_last_transitioned = self.date_published # This preprint will have a tombstone page when it's withdrawn. diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 1e261ab7e34..5f446afff9a 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -6,16 +6,16 @@ from osf.exceptions import InvalidTransitionError from osf.models.preprintlog import PreprintLog -from osf.models.action import ReviewAction, NodeRequestAction, PreprintRequestAction +from osf.models.action import PreprintAction, NodeRequestAction, PreprintRequestAction from osf.utils import permissions from osf.utils.workflows import ( DefaultStates, DefaultTriggers, - ReviewStates, + PreprintStates, ApprovalStates, DEFAULT_TRANSITIONS, - REVIEWABLE_TRANSITIONS, + PREPRINT_STATE_TRANSITIONS, APPROVAL_TRANSITIONS, CollectionSubmissionStates, COLLECTION_SUBMISSION_TRANSITIONS, @@ -98,16 +98,15 @@ def update_last_transitioned(self, ev): self.machineable.date_last_transitioned = now -class ReviewsMachine(BaseMachine): - ActionClass = ReviewAction - States = ReviewStates - Transitions = REVIEWABLE_TRANSITIONS +class PreprintStateMachine(BaseMachine): + ActionClass = PreprintAction + States = PreprintStates + Transitions = PREPRINT_STATE_TRANSITIONS - def save_changes(self, ev): - now = self.action.created if self.action is not None else timezone.now() + def validate_transitions(self, ev): should_publish = self.machineable.in_public_reviews_state if self.machineable.is_retracted: - pass # Do not alter published state + raise ValueError('Do not alter published state.') elif should_publish and not self.machineable.is_published: if not (self.machineable.primary_file and self.machineable.primary_file.target == self.machineable): raise ValueError('Preprint is not a valid preprint; cannot publish.') @@ -115,11 +114,20 @@ def save_changes(self, ev): raise ValueError('Preprint provider not specified; cannot publish.') if not self.machineable.subjects.exists(): raise ValueError('Preprint must have at least one subject to be published.') + + def save_changes(self, ev): + should_publish = self.machineable.in_public_reviews_state + now = self.action.created if self.action is not None else timezone.now() + + if self.machineable.is_retracted: + pass + elif should_publish and not self.machineable.is_published: self.machineable.date_published = now self.machineable.is_published = True self.machineable.ever_public = True elif not should_publish and self.machineable.is_published: self.machineable.is_published = False + self.machineable.save() def resubmission_allowed(self, ev): diff --git a/osf/utils/workflows.py b/osf/utils/workflows.py index eb53ef31645..7e16b210055 100644 --- a/osf/utils/workflows.py +++ b/osf/utils/workflows.py @@ -220,14 +220,14 @@ def db_name(self): ('REJECT', 'reject'), ('EDIT_COMMENT', 'edit_comment'), ] -REVIEW_STATES = DEFAULT_STATES + [ +PREPRINT_STATES = DEFAULT_STATES + [ ('WITHDRAWN', 'withdrawn'), ] -REVIEW_TRIGGERS = DEFAULT_TRIGGERS + [ +PREPRINT_TRIGGERS = DEFAULT_TRIGGERS + [ ('WITHDRAW', 'withdraw') ] -REGISTRATION_STATES = REVIEW_STATES + [ +REGISTRATION_STATES = PREPRINT_STATES + [ ('EMBARGO', 'embargo'), ('PENDING_EMBARGO_TERMINATION', 'pending_embargo_termination'), ('PENDING_WITHDRAW_REQUEST', 'pending_withdraw_request'), @@ -235,10 +235,10 @@ def db_name(self): ] DefaultStates = ChoiceEnum('DefaultStates', DEFAULT_STATES) -ReviewStates = ChoiceEnum('ReviewStates', REVIEW_STATES) +PreprintStates = ChoiceEnum('PreprintStates', PREPRINT_STATES) RegistrationStates = ChoiceEnum('RegistrationStates', REGISTRATION_STATES) DefaultTriggers = ChoiceEnum('DefaultTriggers', DEFAULT_TRIGGERS) -ReviewTriggers = ChoiceEnum('ReviewTriggers', REVIEW_TRIGGERS) +PreprintTriggers = ChoiceEnum('PreprintTriggers', PREPRINT_TRIGGERS) CHRONOS_STATUS_STATES = [ ('DRAFT', 1), @@ -285,15 +285,54 @@ def db_name(self): }, ] -REVIEWABLE_TRANSITIONS = DEFAULT_TRANSITIONS + [ + +PREPRINT_STATE_TRANSITIONS = [ + { + 'trigger': PreprintTriggers.SUBMIT.value, + 'source': [PreprintStates.INITIAL.value], + 'dest': PreprintStates.PENDING.value, + 'before': ['validate_transitions'], + 'after': ['save_action', 'update_last_transitioned', 'save_changes', 'notify_submit'], + }, + { + 'trigger': PreprintTriggers.SUBMIT.value, + 'source': [PreprintStates.PENDING.value, PreprintStates.REJECTED.value], + 'conditions': 'resubmission_allowed', + 'dest': PreprintStates.PENDING.value, + 'before': ['_validate_transitions'], + 'after': ['save_action', 'update_last_transitioned', 'save_changes', 'notify_resubmit'], + }, { - 'trigger': ReviewTriggers.WITHDRAW.value, - 'source': [ReviewStates.PENDING.value, ReviewStates.ACCEPTED.value], - 'dest': ReviewStates.WITHDRAWN.value, + 'trigger': PreprintTriggers.ACCEPT.value, + 'source': [PreprintStates.PENDING.value, DefaultStates.REJECTED.value], + 'dest': PreprintStates.ACCEPTED.value, + 'before': ['validate_transitions'], + 'after': ['save_action', 'update_last_transitioned', 'save_changes', 'notify_accept_reject'], + }, + { + 'trigger': PreprintTriggers.REJECT.value, + 'source': [PreprintStates.PENDING.value, DefaultStates.ACCEPTED.value], + 'dest': PreprintStates.REJECTED.value, + 'before': ['validate_transitions'], + 'after': ['save_action', 'update_last_transitioned', 'save_changes', 'notify_accept_reject'], + }, + { + 'trigger': PreprintTriggers.EDIT_COMMENT.value, + 'source': [PreprintStates.PENDING.value, PreprintStates.REJECTED.value, PreprintStates.ACCEPTED.value], + 'dest': '=', + 'before': ['validate_transitions'], + 'after': ['save_action', 'save_changes', 'notify_edit_comment'], + }, + { + 'trigger': PreprintTriggers.WITHDRAW.value, + 'source': [PreprintStates.PENDING.value, PreprintStates.ACCEPTED.value], + 'dest': PreprintStates.WITHDRAWN.value, + 'before': ['validate_transitions'], 'after': ['save_action', 'update_last_transitioned', 'perform_withdraw', 'save_changes', 'notify_withdraw'] } ] + APPROVAL_TRANSITIONS = [ { # Submit an approvable resource diff --git a/osf_tests/factories.py b/osf_tests/factories.py index f05effa3d8a..46ee1ff8168 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -955,7 +955,7 @@ class Meta: class ReviewActionFactory(DjangoModelFactory): class Meta: - model = models.ReviewAction + model = models.PreprintAction trigger = FuzzyChoice(choices=DefaultTriggers.values()) comment = factory.Faker('text') diff --git a/osf_tests/metadata/test_osf_gathering.py b/osf_tests/metadata/test_osf_gathering.py index 1f9497294a6..5bd101ebb58 100644 --- a/osf_tests/metadata/test_osf_gathering.py +++ b/osf_tests/metadata/test_osf_gathering.py @@ -709,7 +709,7 @@ def test_gather_preprint_withdrawal(self): # withdrawn via PreprintRequest _withdrawal_request = factories.PreprintRequestFactory( target=self.preprint, - machine_state=workflows.ReviewStates.ACCEPTED.value, + machine_state=workflows.PreprintStates.ACCEPTED.value, request_type=workflows.RequestTypes.WITHDRAWAL.value, creator=self.user__admin, comment='request unprint',