From 503fbf0ea1933ef4d2cf48c53ea2edaff42c58bd Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 21:43:26 -0500 Subject: [PATCH 01/14] Rework save() and update create() & create_version() for Prerpint --- osf/models/preprint.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/osf/models/preprint.py b/osf/models/preprint.py index fbeb0645470..8f4012f1ff6 100644 --- a/osf/models/preprint.py +++ b/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(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(first_save=True) # Add contributors for contributor_info in latest_version.contributor_set.exclude(user=latest_version.creator).values('visible', 'user_id', '_order'): @@ -824,7 +826,13 @@ def get_doi_client(self): 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) + guid_ready = kwargs.pop('guid_ready', True) + if not guid_ready: + return super().save(*args, **kwargs) + + if not bool(self.pk): + raise IntegrityError('Preprint must have PK') + 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']): From 235dca72e1473c4b8427f83483855cfb695de674 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 21:46:02 -0500 Subject: [PATCH 02/14] Update prerpint factories --- osf_tests/factories.py | 44 +++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/osf_tests/factories.py b/osf_tests/factories.py index 7ec4f9e3eb8..0f1b6673561 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -727,29 +727,18 @@ 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), @@ -757,6 +746,21 @@ def _create(cls, target_class, *args, **kwargs): version=1, guid=guid ) + instance.save(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), @@ -764,7 +768,7 @@ def _create(cls, target_class, *args, **kwargs): 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,12 +822,9 @@ 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), @@ -831,6 +832,9 @@ def create_version(cls, create_from, creator=None, final_machine_state='accepted guid=guid_obj, version=last_version_number + 1 ) + instance.machine_state = 'initial' + instance.node = latest_version.node + instance.save(first_save=True) # Prepare and add file filename = f'preprint_file_{last_version_number + 1}.txt' From 7e0d89e5a0693b31574ca7682f1bcb28ea1749ab Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 21:48:12 -0500 Subject: [PATCH 03/14] Fix unit tests: must use factories or `.create()` to create preprint --- osf_tests/test_elastic_search.py | 2 +- tests/test_preprints.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osf_tests/test_elastic_search.py b/osf_tests/test_elastic_search.py index b2815fc169f..56c42391095 100644 --- a/osf_tests/test_elastic_search.py +++ b/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, diff --git a/tests/test_preprints.py b/tests/test_preprints.py index 7c92ad9b8e4..45df43eb6ac 100644 --- a/tests/test_preprints.py +++ b/tests/test_preprints.py @@ -282,7 +282,7 @@ class TestPreprintCreation: def test_creator_is_added_as_contributor(self, fake): user = UserFactory() - preprint = Preprint( + preprint = Preprint.create( title=fake.bs(), creator=user, provider=PreprintProviderFactory() @@ -297,7 +297,7 @@ 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, provider=PreprintProviderFactory() @@ -308,7 +308,7 @@ 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, provider=PreprintProviderFactory() From 43243ab64e7bdc50d80b1e5e287c5ea457b328d3 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 22:16:21 -0500 Subject: [PATCH 04/14] Add missing kwargs when using `Preprint.create()` in some tests --- tests/test_preprints.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_preprints.py b/tests/test_preprints.py index 45df43eb6ac..522ff146718 100644 --- a/tests/test_preprints.py +++ b/tests/test_preprints.py @@ -285,6 +285,7 @@ def test_creator_is_added_as_contributor(self, fake): preprint = Preprint.create( title=fake.bs(), creator=user, + description=fake.bs(), provider=PreprintProviderFactory() ) preprint.save() @@ -300,6 +301,7 @@ def test_default_region_set_to_user_settings_osfstorage_default(self, fake): preprint = Preprint.create( title=fake.bs, creator=user, + description=fake.bs(), provider=PreprintProviderFactory() ) preprint.save() @@ -311,6 +313,7 @@ def test_root_folder_created_automatically(self, fake): preprint = Preprint.create( title=fake.bs, creator=user, + description=fake.bs(), provider=PreprintProviderFactory() ) preprint.save() From 0164aec5aa99db4ad9bb4e8d9f3c4c9a66e387cb Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Mon, 6 Jan 2025 13:58:29 +0200 Subject: [PATCH 05/14] Refactor _id property to use cached_property and fixed related tests --- osf/models/base.py | 5 ++--- osf/models/preprint.py | 5 +++++ osf/utils/caching.py | 17 +++++++++++++++++ .../test_check_crossref_dois.py | 1 + osf_tests/test_guid.py | 1 + 5 files changed, 26 insertions(+), 3 deletions(-) diff --git a/osf/models/base.py b/osf/models/base.py index 9e4136565b0..59e34c9e9ac 100644 --- a/osf/models/base.py +++ b/osf/models/base.py @@ -16,7 +16,7 @@ from framework import sentry from osf.exceptions import ValidationError -from osf.utils.caching import cached_property +from osf.utils.caching import cached_property, cached_not_none_property from osf.utils.fields import LowercaseCharField, NonNaiveDateTimeField from website import settings as website_settings @@ -524,9 +524,8 @@ class Meta: versioned_guids = GenericRelation('GuidVersionsThrough', related_name='referent', related_query_name='referents') - @property + @cached_not_none_property def _id(self): - # TODO: maybe a cached property? try: versioned_guid = self.versioned_guids if not versioned_guid.exists(): diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 8f4012f1ff6..3fac2adf4f3 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -314,6 +314,8 @@ def create(cls, provider, title, creator, description): versioned_guid.save() preprint.save(first_save=True) + preprint._id = None # trigger _id cache update + return preprint def get_last_not_rejected_version(self): @@ -426,6 +428,8 @@ def create_version(cls, create_from_guid, auth): guid_version.save() preprint.save(first_save=True) + preprint._id = None # trigger _id cache update + # Add contributors for contributor_info in latest_version.contributor_set.exclude(user=latest_version.creator).values('visible', 'user_id', '_order'): preprint.contributor_set.create(**{**contributor_info, 'preprint_id': preprint.id}) @@ -440,6 +444,7 @@ def create_version(cls, create_from_guid, auth): guid_obj.object_id = preprint.pk guid_obj.content_type = ContentType.objects.get_for_model(preprint) guid_obj.save() + preprint._id = None # trigger _id cache update return preprint, data_to_update diff --git a/osf/utils/caching.py b/osf/utils/caching.py index cbace3b13bc..96c8567e785 100644 --- a/osf/utils/caching.py +++ b/osf/utils/caching.py @@ -61,6 +61,23 @@ def do_fdel(obj): return do_fdel +class _CachedNotNoneProperty(_CachedProperty): + + def _wrap_fget(self, fget): + @wraps(fget) + def do_fget(obj): + if hasattr(obj, self._cache_name): + if getattr(obj, self._cache_name): + return getattr(obj, self._cache_name) + # Generate the value to cache. + value = fget(obj) + if value: + setattr(obj, self._cache_name, value) + return value + + return do_fget + # Public name for the cached property decorator. Using a class as a decorator just looks plain ugly. :P cached_property = _CachedProperty +cached_not_none_property = _CachedNotNoneProperty diff --git a/osf_tests/management_commands/test_check_crossref_dois.py b/osf_tests/management_commands/test_check_crossref_dois.py index 8a1e45cc975..e2ab0740605 100644 --- a/osf_tests/management_commands/test_check_crossref_dois.py +++ b/osf_tests/management_commands/test_check_crossref_dois.py @@ -32,6 +32,7 @@ def stuck_preprint(self): guid._id = 'guid0' guid.save() + preprint._id = None preprint.save() return preprint diff --git a/osf_tests/test_guid.py b/osf_tests/test_guid.py index 7c467e2aa10..62b7968b80a 100644 --- a/osf_tests/test_guid.py +++ b/osf_tests/test_guid.py @@ -565,5 +565,6 @@ def test_versioned_preprint_id_property(self, creator, preprint_provider): assert preprint._id == expected_guid GuidVersionsThrough.objects.filter(guid=preprint_guid).delete() + preprint._id = None assert preprint._id is None From 317a5abff900748480619a35ee3b6701799949d9 Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Tue, 7 Jan 2025 17:55:33 +0200 Subject: [PATCH 06/14] update _id cache --- osf/models/preprint.py | 5 ----- osf_tests/management_commands/test_check_crossref_dois.py | 1 - osf_tests/test_guid.py | 1 - 3 files changed, 7 deletions(-) diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 3fac2adf4f3..8f4012f1ff6 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -314,8 +314,6 @@ def create(cls, provider, title, creator, description): versioned_guid.save() preprint.save(first_save=True) - preprint._id = None # trigger _id cache update - return preprint def get_last_not_rejected_version(self): @@ -428,8 +426,6 @@ def create_version(cls, create_from_guid, auth): guid_version.save() preprint.save(first_save=True) - preprint._id = None # trigger _id cache update - # Add contributors for contributor_info in latest_version.contributor_set.exclude(user=latest_version.creator).values('visible', 'user_id', '_order'): preprint.contributor_set.create(**{**contributor_info, 'preprint_id': preprint.id}) @@ -444,7 +440,6 @@ def create_version(cls, create_from_guid, auth): guid_obj.object_id = preprint.pk guid_obj.content_type = ContentType.objects.get_for_model(preprint) guid_obj.save() - preprint._id = None # trigger _id cache update return preprint, data_to_update diff --git a/osf_tests/management_commands/test_check_crossref_dois.py b/osf_tests/management_commands/test_check_crossref_dois.py index e2ab0740605..8a1e45cc975 100644 --- a/osf_tests/management_commands/test_check_crossref_dois.py +++ b/osf_tests/management_commands/test_check_crossref_dois.py @@ -32,7 +32,6 @@ def stuck_preprint(self): guid._id = 'guid0' guid.save() - preprint._id = None preprint.save() return preprint diff --git a/osf_tests/test_guid.py b/osf_tests/test_guid.py index 62b7968b80a..7c467e2aa10 100644 --- a/osf_tests/test_guid.py +++ b/osf_tests/test_guid.py @@ -565,6 +565,5 @@ def test_versioned_preprint_id_property(self, creator, preprint_provider): assert preprint._id == expected_guid GuidVersionsThrough.objects.filter(guid=preprint_guid).delete() - preprint._id = None assert preprint._id is None From fb6b5f6dd2b8d54e437c9da5073f286d358f181e Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Mon, 6 Jan 2025 11:49:01 +0200 Subject: [PATCH 07/14] updated PreprintOldVersionsImmutableMixin checks Rebased and fixed concflicts --- api/preprints/views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/preprints/views.py b/api/preprints/views.py index efdb351f42a..e0d9da66b40 100644 --- a/api/preprints/views.py +++ b/api/preprints/views.py @@ -75,7 +75,7 @@ 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': + if preprint.is_latest_version or preprint.machine_state in ['initial', '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) @@ -83,7 +83,7 @@ def update(self, request, *args, **kwargs): def create(self, request, *args, **kwargs): preprint = self.get_preprint(check_object_permissions=False) - if preprint.is_latest_version or preprint.machine_state == 'initial': + if preprint.is_latest_version or preprint.machine_state in ['initial', '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) @@ -91,7 +91,7 @@ def create(self, request, *args, **kwargs): def destroy(self, request, *args, **kwargs): preprint = self.get_preprint(check_object_permissions=False) - if preprint.is_latest_version or preprint.machine_state == 'initial': + if preprint.is_latest_version or preprint.machine_state in ['initial', '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) From 62f2a985f6a0f42e56f690a5f4ffd9351e774523 Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Mon, 6 Jan 2025 12:12:53 +0200 Subject: [PATCH 08/14] allow resubmission for POST_MODERATION workflow --- osf/utils/machines.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 8e4b8f07338..35540c4562d 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -123,7 +123,14 @@ def save_changes(self, ev): self.machineable.save() def resubmission_allowed(self, ev): - return self.machineable.provider.reviews_workflow == Workflows.PRE_MODERATION.value + 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() From ed31516fbecd12f3381ccbcb9d0c406b022f249a Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Tue, 7 Jan 2025 16:20:23 +0200 Subject: [PATCH 09/14] added comment --- osf/utils/machines.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 35540c4562d..9403f1d9b5f 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -123,6 +123,10 @@ def save_changes(self, ev): self.machineable.save() def resubmission_allowed(self, ev): + ''' + Allow resubmission if the preprint uses the PRE_MODERATION workflow or + uses the POST_MODERATION workflow and is in a pending state + ''' workflow = self.machineable.provider.reviews_workflow result = any( [ From 116cba99583b6c9f859d589d18caf3880e8b1d3c Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 7 Jan 2025 13:57:02 -0500 Subject: [PATCH 10/14] Improve PreprintOldVersionsImmutableMixin check --- api/preprints/views.py | 16 +++++++++++++--- osf/utils/machines.py | 7 +++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/api/preprints/views.py b/api/preprints/views.py index e0d9da66b40..9caba1c43ff 100644 --- a/api/preprints/views.py +++ b/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,7 +76,10 @@ 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 in ['initial', 'pending']: + 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) @@ -83,7 +87,10 @@ def update(self, request, *args, **kwargs): def create(self, request, *args, **kwargs): preprint = self.get_preprint(check_object_permissions=False) - if preprint.is_latest_version or preprint.machine_state in ['initial', 'pending']: + 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) @@ -91,7 +98,10 @@ def create(self, request, *args, **kwargs): def destroy(self, request, *args, **kwargs): preprint = self.get_preprint(check_object_permissions=False) - if preprint.is_latest_version or preprint.machine_state in ['initial', 'pending']: + 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) diff --git a/osf/utils/machines.py b/osf/utils/machines.py index 9403f1d9b5f..1b286f3cee3 100644 --- a/osf/utils/machines.py +++ b/osf/utils/machines.py @@ -123,10 +123,9 @@ def save_changes(self, ev): self.machineable.save() def resubmission_allowed(self, ev): - ''' - Allow resubmission if the preprint uses the PRE_MODERATION workflow or - uses the POST_MODERATION workflow and is in a pending state - ''' + """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( [ From f3393555bd913dfe5c874e8837615ad924b83968 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Tue, 7 Jan 2025 13:00:08 +0200 Subject: [PATCH 11/14] Fix my preprints page; UserPreprints now returns only latest versions --- api/users/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/users/views.py b/api/users/views.py index 296826ca48b..f3bbfc172cf 100644 --- a/api/users/views.py +++ b/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() From cb168cafd41f2a2a20d06916666d1488ca5328a1 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 7 Jan 2025 15:00:36 -0500 Subject: [PATCH 12/14] Renam and improve _CachedTruthyProperty --- osf/models/base.py | 4 ++-- osf/utils/caching.py | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/osf/models/base.py b/osf/models/base.py index 59e34c9e9ac..de4c51f1e97 100644 --- a/osf/models/base.py +++ b/osf/models/base.py @@ -16,7 +16,7 @@ from framework import sentry from osf.exceptions import ValidationError -from osf.utils.caching import cached_property, cached_not_none_property +from osf.utils.caching import cached_property, cached_truthy_property from osf.utils.fields import LowercaseCharField, NonNaiveDateTimeField from website import settings as website_settings @@ -524,7 +524,7 @@ class Meta: versioned_guids = GenericRelation('GuidVersionsThrough', related_name='referent', related_query_name='referents') - @cached_not_none_property + @cached_truthy_property def _id(self): try: versioned_guid = self.versioned_guids diff --git a/osf/utils/caching.py b/osf/utils/caching.py index 96c8567e785..d959ca4201b 100644 --- a/osf/utils/caching.py +++ b/osf/utils/caching.py @@ -61,14 +61,13 @@ def do_fdel(obj): return do_fdel -class _CachedNotNoneProperty(_CachedProperty): +class _CachedTruthyProperty(_CachedProperty): def _wrap_fget(self, fget): @wraps(fget) def do_fget(obj): - if hasattr(obj, self._cache_name): - if getattr(obj, self._cache_name): - return getattr(obj, self._cache_name) + if getattr(obj, self._cache_name, False): + return getattr(obj, self._cache_name) # Generate the value to cache. value = fget(obj) if value: @@ -80,4 +79,4 @@ def do_fget(obj): # Public name for the cached property decorator. Using a class as a decorator just looks plain ugly. :P cached_property = _CachedProperty -cached_not_none_property = _CachedNotNoneProperty +cached_truthy_property = _CachedTruthyProperty From c6659c97cc20e138d78b811d81b8310a1fb7e1a5 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 7 Jan 2025 15:52:46 -0500 Subject: [PATCH 13/14] Use @cached_property as is; make version cached too --- osf/models/base.py | 8 ++++---- osf/utils/caching.py | 16 ---------------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/osf/models/base.py b/osf/models/base.py index de4c51f1e97..594522ed63a 100644 --- a/osf/models/base.py +++ b/osf/models/base.py @@ -16,7 +16,7 @@ from framework import sentry from osf.exceptions import ValidationError -from osf.utils.caching import cached_property, cached_truthy_property +from osf.utils.caching import cached_property from osf.utils.fields import LowercaseCharField, NonNaiveDateTimeField from website import settings as website_settings @@ -524,7 +524,7 @@ class Meta: versioned_guids = GenericRelation('GuidVersionsThrough', related_name='referent', related_query_name='referents') - @cached_truthy_property + @cached_property def _id(self): try: versioned_guid = self.versioned_guids @@ -550,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 diff --git a/osf/utils/caching.py b/osf/utils/caching.py index d959ca4201b..cbace3b13bc 100644 --- a/osf/utils/caching.py +++ b/osf/utils/caching.py @@ -61,22 +61,6 @@ def do_fdel(obj): return do_fdel -class _CachedTruthyProperty(_CachedProperty): - - def _wrap_fget(self, fget): - @wraps(fget) - def do_fget(obj): - if getattr(obj, self._cache_name, False): - return getattr(obj, self._cache_name) - # Generate the value to cache. - value = fget(obj) - if value: - setattr(obj, self._cache_name, value) - return value - - return do_fget - # Public name for the cached property decorator. Using a class as a decorator just looks plain ugly. :P cached_property = _CachedProperty -cached_truthy_property = _CachedTruthyProperty From c0e3af140b76025107e3551e934ebe309ec1dc63 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Tue, 7 Jan 2025 16:45:10 -0500 Subject: [PATCH 14/14] Futher improve preprint save --- osf/models/preprint.py | 32 +++++++++++++++++++++++++------- osf_tests/factories.py | 4 ++-- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/osf/models/preprint.py b/osf/models/preprint.py index 8f4012f1ff6..d5066dc6088 100644 --- a/osf/models/preprint.py +++ b/osf/models/preprint.py @@ -312,7 +312,7 @@ def create(cls, provider, title, creator, description): guid=base_guid_obj ) versioned_guid.save() - preprint.save(first_save=True) + preprint.save(guid_ready=True, first_save=True) return preprint @@ -424,7 +424,7 @@ def create_version(cls, create_from_guid, auth): guid=guid_obj ) guid_version.save() - preprint.save(first_save=True) + 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'): @@ -824,14 +824,32 @@ 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 - guid_ready = kwargs.pop('guid_ready', True) - if not guid_ready: + """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): - raise IntegrityError('Preprint must have 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 [] diff --git a/osf_tests/factories.py b/osf_tests/factories.py index 0f1b6673561..9ae6a84db2c 100644 --- a/osf_tests/factories.py +++ b/osf_tests/factories.py @@ -746,7 +746,7 @@ def _create(cls, target_class, *args, **kwargs): version=1, guid=guid ) - instance.save(first_save=True) + instance.save(guid_ready=True, first_save=True) # Step 2: finish preprint creation work flow finish = kwargs.pop('finish', True) @@ -834,7 +834,7 @@ def create_version(cls, create_from, creator=None, final_machine_state='accepted ) instance.machine_state = 'initial' instance.node = latest_version.node - instance.save(first_save=True) + instance.save(guid_ready=True, first_save=True) # Prepare and add file filename = f'preprint_file_{last_version_number + 1}.txt'