Merge pull request #10853 from cslzchen/feature/version-creation-rework

[ENG-6372] Rework version creation for model and factory

api/preprints/serializers.py

@@ -38,7 +38,7 @@
 from api.base.metrics import MetricsSerializerMixin
 from api.institutions.utils import update_institutions_if_user_associated
 from api.taxonomies.serializers import TaxonomizableSerializerMixin
-from framework.exceptions import PermissionsError, PendingPreprintVersionExists
+from framework.exceptions import PermissionsError, UnpublishedPendingPreprintVersionExists
 from website.project import signals as project_signals
 from osf.exceptions import NodeStateError, PreprintStateError
 from osf.models import (
@@ -591,7 +591,7 @@ def create(self, validated_data):
             preprint, update_data = Preprint.create_version(create_from_guid, auth)
         except PermissionsError:
             raise PermissionDenied(detail='You must have admin permissions to create new version.')
-        except PendingPreprintVersionExists:
+        except UnpublishedPendingPreprintVersionExists:
             raise Conflict(detail='Before creating a new version, you must publish the latest version.')
         # TODO add more checks
         return self.update(preprint, update_data)

framework/exceptions/__init__.py

@@ -111,7 +111,7 @@ def __init__(self, code, message=None, redirect_url=None, data=None, template=No
         self.template = template
         super().__init__(code, message, redirect_url, data)
 
-class PendingPreprintVersionExists(FrameworkError):
-    """Raised if an pending preprint version exists
+class UnpublishedPendingPreprintVersionExists(FrameworkError):
+    """Raised if an unpublished pending preprint version exists
     """
     pass

osf/models/preprint.py

@@ -16,7 +16,7 @@
 from django.db.models.signals import post_save
 
 from framework.auth import Auth
-from framework.exceptions import PermissionsError, PendingPreprintVersionExists
+from framework.exceptions import PermissionsError, UnpublishedPendingPreprintVersionExists
 from framework.auth import oauth_scopes
 from rest_framework.exceptions import NotFound
 
@@ -312,77 +312,79 @@ def create(cls, provider, title, creator, description):
 
         return preprint
 
-    def has_pending_version(self):
-        guid = self.guids.first()
-        last_version = guid.versions.order_by('-version').first().referent
-        return last_version.machine_state == 'pending'
+    def has_unpublished_pending_version(self):
+        guid_obj = self.guids.first()
+        last_not_rejected_version = guid_obj.versions.filter(is_rejected=False).order_by('-version').first().referent
+        return last_not_rejected_version.machine_state == 'pending' and not last_not_rejected_version.is_published
 
     @classmethod
     def create_version(cls, create_from_guid, auth):
-        base_guid = Guid.load(create_from_guid)
-        source_preprint = cls.load(base_guid._id)
+
+        guid_obj = Guid.load(create_from_guid)
+        # Guid object always points to the latest ever-published (i.e. either published or withdrawn) version
+        source_preprint = cls.load(guid_obj._id)
         if not source_preprint:
             raise NotFound
         if not source_preprint.has_permission(auth.user, ADMIN):
             raise PermissionsError
-        if source_preprint.has_pending_version():
-            raise PendingPreprintVersionExists
-
-        last_version = base_guid.versions.order_by('-version').first().version
-        data_for_update = {}
-        data_for_update['tags'] = source_preprint.tags.all().values_list('name', flat=True)
+        if source_preprint.has_unpublished_pending_version():
+            raise UnpublishedPendingPreprintVersionExists
+        # Note: last version may not be the latest version
+        last_version_number = guid_obj.versions.order_by('-version').first().version
+
+        # Prepare data to clone/update
+        data_for_update = {
+            'subjects': [[el] for el in source_preprint.subjects.all().values_list('_id', flat=True)],
+            'tags': source_preprint.tags.all().values_list('name', flat=True),
+            'original_publication_date': source_preprint.original_publication_date,
+            'custom_publication_citation': source_preprint.custom_publication_citation,
+            'article_doi': source_preprint.article_doi, 'has_coi': source_preprint.has_coi,
+            'conflict_of_interest_statement': source_preprint.conflict_of_interest_statement,
+            'has_data_links': source_preprint.has_data_links, 'why_no_data': source_preprint.why_no_data,
+            'data_links': source_preprint.data_links,
+            'has_prereg_links': source_preprint.has_prereg_links,
+            'why_no_prereg': source_preprint.why_no_prereg, 'prereg_links': source_preprint.prereg_links,
+        }
         if source_preprint.license:
             data_for_update['license_type'] = source_preprint.license.node_license
             data_for_update['license'] = {
                 'copyright_holders': source_preprint.license.copyright_holders,
                 'year': source_preprint.license.year
             }
-
-        data_for_update['subjects'] = [[el] for el in source_preprint.subjects.all().values_list('_id', flat=True)]
-
-        data_for_update['original_publication_date'] = source_preprint.original_publication_date
-        data_for_update['custom_publication_citation'] = source_preprint.custom_publication_citation
-        data_for_update['article_doi'] = source_preprint.article_doi
-
-        data_for_update['has_coi'] = source_preprint.has_coi
-        data_for_update['conflict_of_interest_statement'] = source_preprint.conflict_of_interest_statement
-        data_for_update['has_data_links'] = source_preprint.has_data_links
-        data_for_update['why_no_data'] = source_preprint.why_no_data
-        data_for_update['data_links'] = source_preprint.data_links
-        data_for_update['has_prereg_links'] = source_preprint.has_prereg_links
-        data_for_update['why_no_prereg'] = source_preprint.why_no_prereg
-        data_for_update['prereg_links'] = source_preprint.prereg_links
-
         if source_preprint.node:
             data_for_update['node'] = source_preprint.node
 
+        # Create a preprint obj for the new version
         preprint = cls(
             provider=source_preprint.provider,
             title=source_preprint.title,
             creator=auth.user,
             description=source_preprint.description,
         )
         preprint.save()
-        # add contributors
+
+        # Add contributors
         for contributor_info in source_preprint.contributor_set.exclude(user=source_preprint.creator).values('visible', 'user_id', '_order'):
             preprint.contributor_set.create(**{**contributor_info, 'preprint_id': preprint.id})
 
-        # add affiliated institutions
+        # Add affiliated institutions
         for institution in source_preprint.affiliated_institutions.all():
             preprint.add_affiliated_institution(institution, auth.user, ignore_user_affiliation=True)
 
+        # Update Guid obj to point to the new version if there is no moderation
         if not preprint.provider.reviews_workflow:
-            base_guid.referent = preprint
-            base_guid.object_id = preprint.pk
-            base_guid.content_type = ContentType.objects.get_for_model(preprint)
-            base_guid.save()
+            guid_obj.referent = preprint
+            guid_obj.object_id = preprint.pk
+            guid_obj.content_type = ContentType.objects.get_for_model(preprint)
+            guid_obj.save()
 
+        # Create an entry in the `GuidVersionsThrough` table
         guid_version = GuidVersionsThrough(
-            referent=base_guid.referent,
-            object_id=base_guid.object_id,
-            content_type=base_guid.content_type,
-            version=last_version + 1,
-            guid=base_guid
+            referent=guid_obj.referent,
+            object_id=guid_obj.object_id,
+            content_type=guid_obj.content_type,
+            version=last_version_number + 1,
+            guid=guid_obj
         )
         guid_version.save()
 

osf_tests/factories.py

@@ -4,11 +4,11 @@
 import datetime
 from random import randint
 from unittest import mock
+
 from factory import SubFactory
 from factory.fuzzy import FuzzyDateTime, FuzzyAttribute, FuzzyChoice
 from unittest.mock import patch, Mock
 
-import factory
 import pytz
 import factory.django
 from factory.django import DjangoModelFactory
@@ -794,41 +794,36 @@ def _create(cls, target_class, *args, **kwargs):
         instance.save()
         return instance
 
+    # TODO: maybe we should use model's `create_version` and use api's update_data
     @classmethod
-    def create_version(cls, *args, **kwargs):
-
-        update_task_patcher = mock.patch('website.preprints.tasks.on_preprint_updated.si')
-        update_task_patcher.start()
+    def create_version(cls, create_from, creator=None, final_machine_state='accepted', is_published=True, set_doi=True):
 
-        source_preprint = kwargs.pop('preprint')
-        # TODO: assert not source_preprint.has_unpublished_pending_version()
-        guid_obj = source_preprint.guids.first()
-        latest_version = guid_obj.versions.filter(is_rejected=False).order_by('-version').first().referent
+        # Run a few checks
+        assert final_machine_state in ['pending', 'accepted']
+        assert create_from and not create_from.has_unpublished_pending_version()
+        guid_obj = create_from.guids.first()
+        latest_version = guid_obj.referent
         assert latest_version.is_latest_version
         last_version_number = guid_obj.versions.order_by('-version').first().version
+        if not creator:
+            creator = latest_version.creator
 
-        finish = kwargs.pop('finish', True)
-        set_doi = kwargs.pop('set_doi', True)
-        is_published = kwargs.pop('is_published', True)
-        # TODO: machine_state = kwargs.pop('machine_state', 'initial')
-        license_details = None
-        if latest_version.license:
-            license_details = {
-                'id': latest_version.license.node_license.license_id,
-                'copyrightHolders': latest_version.license.copyright_holders,
-                'year': latest_version.license.year
-            }
-        subjects = [[el] for el in latest_version.subjects.all().values_list('_id', flat=True)]
+        update_task_patcher = mock.patch('website.preprints.tasks.on_preprint_updated.si')
+        update_task_patcher.start()
 
+        # Create new version from the latest version
         target_class = cls._meta.model
-        instance = cls._build(target_class, *args, **kwargs)
-        instance.article_doi = latest_version.article_doi
-        instance.date_published = timezone.now()
-        user = kwargs.pop('creator', None) or instance.creator
+        instance = cls._build(
+            target_class,
+            provider=latest_version.provider,
+            title=latest_version.title,
+            description=latest_version.description,
+            creator=creator,
+            node=latest_version.node,
+        )
         instance.machine_state = 'initial'
         instance.provider = latest_version.provider
         instance.save()
-
         models.GuidVersionsThrough.objects.create(
             referent=instance,
             content_type=ContentType.objects.get_for_model(instance),
@@ -837,8 +832,8 @@ def create_version(cls, *args, **kwargs):
             version=last_version_number + 1
         )
 
-        filename = kwargs.pop('filename', None) or f'preprint_file_{last_version_number + 1}_file.txt'
-        file_size = kwargs.pop('file_size', 1337)
+        # Prepare and add file
+        filename = f'preprint_file_{last_version_number + 1}.txt'
         preprint_file = OsfStorageFile.create(
             target_object_id=instance.id,
             target_content_type=ContentType.objects.get_for_model(instance),
@@ -847,36 +842,59 @@ def create_version(cls, *args, **kwargs):
             materialized_path=f'/{filename}'
         )
         preprint_file.save()
-        # TODO: why setting machine state here? it should happen in `if finish`
-        # TODO: instance.machine_state = machine_state
+        auth = Auth(instance.creator)
+        instance.set_primary_file(preprint_file, auth=auth, save=True)
         from addons.osfstorage import settings as osfstorage_settings
-        preprint_file.create_version(user, {
+        location =  {
             'object': '06d80e',
             'service': 'cloud',
             osfstorage_settings.WATERBUTLER_RESOURCE: 'osf',
-        }, {
-            'size': file_size,
+        }
+        metadata = {
+            'size': 1357,
             'contentType': 'img/png'
-        }).save()
+        }
+        preprint_file.create_version(creator, location, metadata=metadata).save()
+
+        # Set relationships
+        contributor_list = latest_version.contributor_set.exclude(
+            user=latest_version.creator
+        ).values('visible', 'user_id', '_order')
+        for contributor in contributor_list:
+            instance.contributor_set.create(**{**contributor, 'preprint_id': instance.id})
+        for institution in latest_version.affiliated_institutions.all():
+            instance.add_affiliated_institution(institution, auth.user, ignore_user_affiliation=True)
+
+        # Set other fields
+        # TODO: we should copy all fields
+        if latest_version.license:
+            license_details = {
+                'id': latest_version.license.node_license.license_id,
+                'copyrightHolders': latest_version.license.copyright_holders,
+                'year': latest_version.license.year
+            }
+            instance.set_preprint_license(license_details, auth=auth)
+        subjects = [[el] for el in latest_version.subjects.all().values_list('_id', flat=True)]
+        instance.set_subjects(subjects, auth=auth)
+        instance.article_doi = latest_version.article_doi
+        instance.set_published(is_published, auth=auth)
+        # Note: machine_state needs to be adjusted after `set_published` is called.
+        instance.machine_state = final_machine_state
+        instance.save()
         update_task_patcher.stop()
 
-        if finish:
-            auth = Auth(user)
-            # TODO: make sure user is an admin contributor
-            instance.set_primary_file(preprint_file, auth=auth, save=True)
-            instance.set_subjects(subjects, auth=auth)
-            if license_details:
-                instance.set_preprint_license(license_details, auth=auth)
-            instance.set_published(is_published, auth=auth)
-            create_task_patcher = mock.patch('website.identifiers.utils.request_identifiers')
-            mock_create_identifier = create_task_patcher.start()
-            if is_published and set_doi:
+        if not is_published:
+            assert not set_doi and final_machine_state != 'accepted'
+        else:
+            if set_doi:
+                create_task_patcher = mock.patch('website.identifiers.utils.request_identifiers')
+                mock_create_identifier = create_task_patcher.start()
                 mock_create_identifier.side_effect = sync_set_identifiers(instance)
-                guid_obj.referent = instance
-                guid_obj.object_id = instance.pk
-                guid_obj.save()
-            # TODO: instance.machine_state = machine_state
-            create_task_patcher.stop()
+                create_task_patcher.stop()
+            instance.is_published = True
+            guid_obj.referent = instance
+            guid_obj.object_id = instance.pk
+            guid_obj.save()
 
         instance.save()
         return instance

tests/identifiers/test_crossref.py

@@ -42,7 +42,7 @@ def preprint():
 
 @pytest.fixture()
 def preprint_version(preprint):
-    versioned_preprint = PreprintFactory.create_version(preprint=preprint)
+    versioned_preprint = PreprintFactory.create_version(preprint)
     return versioned_preprint
 
 @pytest.fixture()