Merge pull request #10892 from cslzchen/feature/rework-prerpint-save

[ENG-6800][ENG-6801][ENG-6802][ENG-6838] Rework `Preprint.save()` + make `_id` cached property + fix post moderation resubmission + fix my-preprints page

api/preprints/views.py

@@ -60,6 +60,7 @@
     PreprintFilesPermissions,
     PreprintInstitutionPermissionList,
 )
+from api.providers.workflows import Workflows
 from api.nodes.permissions import ContributorOrPublic
 from api.base.permissions import WriteOrPublicForRelationshipInstitutions
 from api.requests.permissions import PreprintRequestPermission
@@ -75,23 +76,32 @@ class PreprintOldVersionsImmutableMixin:
     """
     def update(self, request, *args, **kwargs):
         preprint = self.get_preprint(check_object_permissions=False)
-        if preprint.is_latest_version or preprint.machine_state == 'initial':
+        is_pre_mod_pending = (
+            preprint.provider.reviews_workflow == Workflows.PRE_MODERATION.value and preprint.machine_state == 'pending'
+        )
+        if preprint.is_latest_version or preprint.machine_state == 'initial' or is_pre_mod_pending:
             return super().update(request, *args, **kwargs)
         message = f'User can not edit previous versions of a preprint: [_id={preprint._id}]'
         sentry.log_message(message)
         raise Conflict(detail=message)
 
     def create(self, request, *args, **kwargs):
         preprint = self.get_preprint(check_object_permissions=False)
-        if preprint.is_latest_version or preprint.machine_state == 'initial':
+        is_pre_mod_pending = (
+            preprint.provider.reviews_workflow == Workflows.PRE_MODERATION.value and preprint.machine_state == 'pending'
+        )
+        if preprint.is_latest_version or preprint.machine_state == 'initial' or is_pre_mod_pending:
             return super().create(request, *args, **kwargs)
         message = f'User can not edit previous versions of a preprint: [_id={preprint._id}]'
         sentry.log_message(message)
         raise Conflict(detail=message)
 
     def destroy(self, request, *args, **kwargs):
         preprint = self.get_preprint(check_object_permissions=False)
-        if preprint.is_latest_version or preprint.machine_state == 'initial':
+        is_pre_mod_pending = (
+            preprint.provider.reviews_workflow == Workflows.PRE_MODERATION.value and preprint.machine_state == 'pending'
+        )
+        if preprint.is_latest_version or preprint.machine_state == 'initial' or is_pre_mod_pending:
             return super().destroy(request, *args, **kwargs)
         message = f'User can not edit previous versions of a preprint: [_id={preprint._id}]'
         sentry.log_message(message)

api/users/views.py

@@ -407,7 +407,7 @@ def get_default_queryset(self):
 
         # Permissions on the list objects are handled by the query
         default_qs = Preprint.objects.filter(_contributors__guids___id=target_user._id).exclude(machine_state='initial')
-        return self.preprints_queryset(default_qs, auth_user, allow_contribs=False)
+        return self.preprints_queryset(default_qs, auth_user, allow_contribs=False, latest_only=True)
 
     def get_queryset(self):
         return self.get_queryset_from_request()

osf/models/base.py

@@ -524,9 +524,8 @@ class Meta:
 
     versioned_guids = GenericRelation('GuidVersionsThrough', related_name='referent', related_query_name='referents')
 
-    @property
+    @cached_property
     def _id(self):
-        # TODO: maybe a cached property?
         try:
             versioned_guid = self.versioned_guids
             if not versioned_guid.exists():
@@ -551,9 +550,9 @@ def _id(self, value):
 
     _primary_key = _id
 
-    @property
+    @cached_property
     def version(self):
-        # TODO: maybe a cached property?
+        # Once assigned, version number never changes
         return self.versioned_guids.first().version
 
     @classmethod

osf/models/preprint.py

@@ -4,7 +4,7 @@
 import re
 
 from dirtyfields import DirtyFieldsMixin
-from django.db import models
+from django.db import models, IntegrityError
 from django.db.models import Q
 from django.utils import timezone
 from django.contrib.contenttypes.fields import GenericRelation
@@ -296,7 +296,7 @@ def create(cls, provider, title, creator, description):
             creator=creator,
             description=description,
         )
-        preprint.save()
+        preprint.save(guid_ready=False)
         # Step 2: Create the base guid obj
         base_guid_obj = Guid.objects.create()
         base_guid_obj.referent = preprint
@@ -312,6 +312,7 @@ def create(cls, provider, title, creator, description):
             guid=base_guid_obj
         )
         versioned_guid.save()
+        preprint.save(guid_ready=True, first_save=True)
 
         return preprint
 
@@ -412,7 +413,7 @@ def create_version(cls, create_from_guid, auth):
             creator=auth.user,
             description=latest_version.description,
         )
-        preprint.save()
+        preprint.save(guid_ready=False)
         # Create a new entry in the `GuidVersionsThrough` table to store version information, which must happen right
         # after the first `.save()` of the new preprint version object, which enables `preprint._id` to be computed.
         guid_version = GuidVersionsThrough(
@@ -423,6 +424,7 @@ def create_version(cls, create_from_guid, auth):
             guid=guid_obj
         )
         guid_version.save()
+        preprint.save(guid_ready=True, first_save=True)
 
         # Add contributors
         for contributor_info in latest_version.contributor_set.exclude(user=latest_version.creator).values('visible', 'user_id', '_order'):
@@ -822,9 +824,33 @@ def get_doi_client(self):
             return None
 
     def save(self, *args, **kwargs):
-        # TODO: document how `.save()` is customized for preprint
-        # TODO: insert a bare-minimum condition for saving preprint before guid and/or versioned guid is created
-        first_save = not bool(self.pk)
+        """Customize preprint save process, which has three steps.
+
+        1. Initial: this save happens before guid and versioned guid are created for the preprint; this save
+        creates the pk; after this save, none of `guids`, `versioned_guids` or `._id` is available.
+        2. First: this save happens and must happen right after versioned guid have been created; this is the
+        same "first save" as it was before preprint became versioned; the only change is that `pk` already exists
+        3. This is the case for all subsequent saves after initial and first save.
+
+        Note: When creating a preprint or new version , must use Preprint.create() or Preprint.create_version()
+        respectively, which handles the save process automatically.
+        """
+        initial_save = not kwargs.pop('guid_ready', True)
+        if initial_save:
+            # Save when guid and versioned guid are not ready
+            return super().save(*args, **kwargs)
+
+        # Preprint must have PK and _id set before continue
+        if not bool(self.pk):
+            err_msg = 'Preprint must have pk!'
+            sentry.log_message(err_msg)
+            raise IntegrityError(err_msg)
+        if not self._id:
+            err_msg = 'Preprint must have _id!'
+            sentry.log_message(err_msg)
+            raise IntegrityError(err_msg)
+
+        first_save = kwargs.pop('first_save', False)
         saved_fields = self.get_dirty_fields() or []
 
         if not first_save and ('ever_public' in saved_fields and saved_fields['ever_public']):

osf/utils/machines.py

@@ -123,7 +123,17 @@ def save_changes(self, ev):
         self.machineable.save()
 
     def resubmission_allowed(self, ev):
-        return self.machineable.provider.reviews_workflow == Workflows.PRE_MODERATION.value
+        """Allow resubmission 1) if the preprint uses the PRE_MODERATION workflow, or 2) if it uses the POST_MODERATION
+        workflow and is in a pending state.
+        """
+        workflow = self.machineable.provider.reviews_workflow
+        result = any(
+            [
+                workflow == Workflows.PRE_MODERATION.value,
+                workflow == Workflows.POST_MODERATION.value and self.machineable.machine_state == 'pending'
+            ]
+        )
+        return result
 
     def perform_withdraw(self, ev):
         self.machineable.date_withdrawn = self.action.created if self.action is not None else timezone.now()

osf_tests/factories.py

@@ -727,44 +727,48 @@ def _build(cls, target_class, *args, **kwargs):
 
     @classmethod
     def _create(cls, target_class, *args, **kwargs):
+
         update_task_patcher = mock.patch('website.preprints.tasks.on_preprint_updated.si')
         update_task_patcher.start()
 
-        finish = kwargs.pop('finish', True)
-        set_doi = kwargs.pop('set_doi', True)
-        is_published = kwargs.pop('is_published', True)
+        # Step 1: create prerpint, guid and versioned guid
         instance = cls._build(target_class, *args, **kwargs)
-        file_size = kwargs.pop('file_size', 1337)
-
-        doi = kwargs.pop('doi', None)
-        license_details = kwargs.pop('license_details', None)
-        filename = kwargs.pop('filename', None) or 'preprint_file.txt'
-        subjects = kwargs.pop('subjects', None) or [[SubjectFactory()._id]]
-        instance.article_doi = doi
-
-        user = kwargs.pop('creator', None) or instance.creator
-        instance.save()
+        instance.save(guid_ready=False)
         guid = models.Guid.objects.create(
             referent=instance,
             content_type=ContentType.objects.get_for_model(instance),
             object_id=instance.pk,
         )
-
         models.GuidVersionsThrough.objects.create(
             referent=instance,
             content_type=ContentType.objects.get_for_model(instance),
             object_id=instance.pk,
             version=1,
             guid=guid
         )
+        instance.save(guid_ready=True, first_save=True)
+
+        # Step 2: finish preprint creation work flow
+        finish = kwargs.pop('finish', True)
+        set_doi = kwargs.pop('set_doi', True)
+        user = kwargs.pop('creator', None) or instance.creator
+        is_published = kwargs.pop('is_published', True)
+        file_size = kwargs.pop('file_size', 1337)
+        doi = kwargs.pop('doi', None)
+        license_details = kwargs.pop('license_details', None)
+        filename = kwargs.pop('filename', None) or 'preprint_file.txt'
+        subjects = kwargs.pop('subjects', None) or [[SubjectFactory()._id]]
+        instance.article_doi = doi
+        machine_state = kwargs.pop('machine_state', 'initial')
+
         preprint_file = OsfStorageFile.create(
             target_object_id=instance.id,
             target_content_type=ContentType.objects.get_for_model(instance),
             path=f'/{filename}',
             name=filename,
             materialized_path=f'/{filename}')
 
-        instance.machine_state = kwargs.pop('machine_state', 'initial')
+        instance.machine_state = machine_state
         preprint_file.save()
         from addons.osfstorage import settings as osfstorage_settings
 
@@ -818,19 +822,19 @@ def create_version(cls, create_from, creator=None, final_machine_state='accepted
             provider=latest_version.provider,
             title=latest_version.title,
             description=latest_version.description,
-            creator=creator,
-            node=latest_version.node,
+            creator=creator
         )
-        instance.machine_state = 'initial'
-        instance.provider = latest_version.provider
-        instance.save()
+        instance.save(guid_ready=False)
         models.GuidVersionsThrough.objects.create(
             referent=instance,
             content_type=ContentType.objects.get_for_model(instance),
             object_id=instance.pk,
             guid=guid_obj,
             version=last_version_number + 1
         )
+        instance.machine_state = 'initial'
+        instance.node = latest_version.node
+        instance.save(guid_ready=True, first_save=True)
 
         # Prepare and add file
         filename = f'preprint_file_{last_version_number + 1}.txt'

osf_tests/test_elastic_search.py

@@ -524,7 +524,7 @@ def setUp(self):
             search.delete_index(elastic_search.INDEX)
             search.create_index(elastic_search.INDEX)
             self.user = factories.UserFactory(fullname='John Deacon')
-            self.preprint = Preprint(
+            self.preprint = Preprint.create(
                 title='Red Special',
                 description='We are the champions',
                 creator=self.user,

tests/test_preprints.py

@@ -282,9 +282,10 @@ class TestPreprintCreation:
 
     def test_creator_is_added_as_contributor(self, fake):
         user = UserFactory()
-        preprint = Preprint(
+        preprint = Preprint.create(
             title=fake.bs(),
             creator=user,
+            description=fake.bs(),
             provider=PreprintProviderFactory()
         )
         preprint.save()
@@ -297,9 +298,10 @@ def test_creator_is_added_as_contributor(self, fake):
 
     def test_default_region_set_to_user_settings_osfstorage_default(self, fake):
         user = UserFactory()
-        preprint = Preprint(
+        preprint = Preprint.create(
             title=fake.bs,
             creator=user,
+            description=fake.bs(),
             provider=PreprintProviderFactory()
         )
         preprint.save()
@@ -308,9 +310,10 @@ def test_default_region_set_to_user_settings_osfstorage_default(self, fake):
 
     def test_root_folder_created_automatically(self, fake):
         user = UserFactory()
-        preprint = Preprint(
+        preprint = Preprint.create(
             title=fake.bs,
             creator=user,
+            description=fake.bs(),
             provider=PreprintProviderFactory()
         )
         preprint.save()