[ENG-10043] make to be possible to edit metadata on previous versions of a preprint

api/preprints/views.py

@@ -74,36 +74,6 @@
 from api.base.metrics import UsageMetricsViewMixin
 
 
-class PreprintOldVersionsImmutableMixin:
-    """Override method to reject modify requests for old preprint versions (except for withdrawal)"""
-
-    @staticmethod
-    def is_edit_allowed(preprint):
-        if preprint.is_latest_version or preprint.machine_state == DefaultStates.INITIAL.value:
-            return True
-        if preprint.provider.reviews_workflow == Workflows.PRE_MODERATION.value:
-            if preprint.machine_state == DefaultStates.PENDING.value or preprint.machine_state == DefaultStates.REJECTED.value:
-                return True
-        return False
-
-    def handle_request(self, request, method, *args, **kwargs):
-        preprint = self.get_preprint(check_object_permissions=False)
-        if PreprintOldVersionsImmutableMixin.is_edit_allowed(preprint):
-            return method(request, *args, **kwargs)
-        message = f'User cannot edit previous versions of a preprint: [_id={preprint._id}]'
-        sentry.log_message(message)
-        raise Conflict(detail=message)
-
-    def update(self, request, *args, **kwargs):
-        return self.handle_request(request, super().update, *args, **kwargs)
-
-    def create(self, request, *args, **kwargs):
-        return self.handle_request(request, super().create, *args, **kwargs)
-
-    def destroy(self, request, *args, **kwargs):
-        return self.handle_request(request, super().destroy, *args, **kwargs)
-
-
 class PreprintMixin(NodeMixin):
     serializer_class = PreprintSerializer
     preprint_lookup_url_kwarg = 'preprint_id'
@@ -266,7 +236,7 @@ def create(self, request, *args, **kwargs):
         return super().create(request, *args, **kwargs)
 
 
-class PreprintDetail(PreprintOldVersionsImmutableMixin, UsageMetricsViewMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin, WaterButlerMixin):
+class PreprintDetail(UsageMetricsViewMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin, WaterButlerMixin):
     """See [documentation for this endpoint](https://developer.osf.io/#operation/preprints_read).
 
     Note: The resource now exposes a `versions` relationship pointing to
@@ -307,7 +277,7 @@ def get_parser_context(self, http_request):
         res['legacy_type_allowed'] = True
         return res
 
-class PreprintNodeRelationship(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin):
+class PreprintNodeRelationship(JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin):
     permission_classes = (
         drf_permissions.IsAuthenticatedOrReadOnly,
         base_permissions.TokenHasScope,
@@ -335,7 +305,7 @@ def get_object(self):
         return obj
 
 
-class PreprintCitationDetail(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveAPIView, PreprintMixin):
+class PreprintCitationDetail(JSONAPIBaseView, generics.RetrieveAPIView, PreprintMixin):
     """See [documentation for this endpoint](https://developer.osf.io/#operation/preprints_citation_list).
     """
     permission_classes = (
@@ -445,7 +415,7 @@ def get_object(self, check_object_permissions=True):
         return self.get_preprint(check_object_permissions=check_object_permissions)
 
 
-class PreprintContributorsList(PreprintOldVersionsImmutableMixin, NodeContributorsList, PreprintMixin):
+class PreprintContributorsList(NodeContributorsList, PreprintMixin):
     permission_classes = (
         AdminOrPublic,
         drf_permissions.IsAuthenticatedOrReadOnly,
@@ -489,7 +459,7 @@ def get_serializer_context(self):
         return context
 
 
-class PreprintContributorDetail(PreprintOldVersionsImmutableMixin, NodeContributorDetail, PreprintMixin):
+class PreprintContributorDetail(NodeContributorDetail, PreprintMixin):
 
     permission_classes = (
         ContributorDetailPermissions,
@@ -581,7 +551,7 @@ def get_resource(self):
         return self.get_preprint()
 
 
-class PreprintSubjectsRelationship(PreprintOldVersionsImmutableMixin, SubjectRelationshipBaseView, PreprintMixin):
+class PreprintSubjectsRelationship(SubjectRelationshipBaseView, PreprintMixin):
     """See [documentation for this endpoint](https://developer.osf.io/#operation/preprint_subjects_list).
     """
     permission_classes = (
@@ -801,7 +771,7 @@ def get_queryset(self):
         return self.get_resource().get_affiliated_institutions()
 
 
-class PreprintInstitutionsRelationship(PreprintOldVersionsImmutableMixin, JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin):
+class PreprintInstitutionsRelationship(JSONAPIBaseView, generics.RetrieveUpdateAPIView, PreprintMixin):
     """ """
     permission_classes = (
         drf_permissions.IsAuthenticatedOrReadOnly,

api_tests/preprints/views/test_preprint_versions.py

@@ -9,7 +9,7 @@
 from osf.models import Preprint
 from osf.utils import permissions
 from osf.utils.workflows import DefaultStates, RequestTypes
-from osf_tests.factories import ProjectFactory, PreprintFactory, AuthUserFactory, PreprintRequestFactory
+from osf_tests.factories import ProjectFactory, PreprintFactory, AuthUserFactory, PreprintRequestFactory, SubjectFactory
 from tests.base import ApiTestCase
 from tests.utils import capture_notifications
 
@@ -225,9 +225,10 @@ def test_return_409_if_unpublished_pending_version_exists(self):
         res = self.app.post_json_api(self.pre_mode_version_create_url, auth=self.user.auth, expect_errors=True)
         assert res.status_code == 409
 
-    def test_return_409_if_try_to_edit_old_versions(self):
+    def test_edit_old_version_after_new_version_is_accepted(self):
         res = self.app.post_json_api(self.post_mod_version_create_url, auth=self.user.auth)
         new_version = Preprint.load(res.json['data']['id'])
+        new_version_title = new_version.title
         filename = 'preprint_file.txt'
         preprint_file = OsfStorageFile.create(
             target_object_id=new_version.id,
@@ -247,13 +248,17 @@ def test_return_409_if_try_to_edit_old_versions(self):
         with capture_notifications():
             new_version.run_accept(self.moderator, 'comment')
         assert new_version.machine_state == 'accepted'
+        assert not self.post_mod_preprint.is_latest_version
         res = self.app.patch_json_api(
             self.post_mod_preprint_update_url,
             self.post_mod_preprint_update_payload,
             auth=self.user.auth,
-            expect_errors=True
         )
-        assert res.status_code == 409
+        assert res.status_code == 200
+        self.post_mod_preprint.reload()
+        new_version.reload()
+        assert self.post_mod_preprint.title == 'new_title'
+        assert new_version.title == new_version_title
 
     def test_reuse_version_if_unfinished_version_exists(self):
         res = self.app.post_json_api(self.post_mod_version_create_url, auth=self.user.auth)
@@ -902,3 +907,211 @@ def test_version_indifference(self):
         res_2 = self.app.get(versions_url_valid_version, auth=self.creator.auth)
         assert res_2.status_code == 200
         assert len(res_1.json['data']) == len(res_2.json['data'])
+
+
+class TestPreprintPreviousVersionUpdate(ApiTestCase):
+    """
+    Previous versions follow the same edit permissions as the latest version: ADMIN contributors can edit
+    metadata and contributors, WRITE contributors can edit metadata only and READ contributors cannot edit.
+    """
+
+    def setUp(self):
+        super().setUp()
+        self.creator = AuthUserFactory()
+        self.admin_contrib = AuthUserFactory()
+        self.write_contrib = AuthUserFactory()
+        self.read_contrib = AuthUserFactory()
+        self.non_contrib = AuthUserFactory()
+        self.preprint_v1 = PreprintFactory(
+            reviews_workflow='post-moderation',
+            is_published=True,
+            creator=self.creator
+        )
+        self.preprint_v1.add_contributor(self.admin_contrib, permissions=permissions.ADMIN)
+        self.preprint_v1.add_contributor(self.write_contrib, permissions=permissions.WRITE)
+        self.preprint_v1.add_contributor(self.read_contrib, permissions=permissions.READ)
+        with capture_notifications():
+            self.preprint_v2 = PreprintFactory.create_version(
+                create_from=self.preprint_v1,
+                creator=self.creator,
+                set_doi=False
+            )
+        assert not self.preprint_v1.is_latest_version
+        assert self.preprint_v2.is_latest_version
+
+        self.v1_url = f'/{API_BASE}preprints/{self.preprint_v1._id}/'
+        self.v1_contributors_url = f'{self.v1_url}contributors/'
+        self.v1_subjects_url = f'{self.v1_url}relationships/subjects/'
+
+    def update_payload(self, **attributes):
+        return {
+            'data': {
+                'id': self.preprint_v1._id,
+                'type': 'preprints',
+                'attributes': attributes,
+            }
+        }
+
+    def add_contributor_payload(self, user):
+        return {
+            'data': {
+                'type': 'contributors',
+                'attributes': {
+                    'bibliographic': True,
+                    'permission': permissions.WRITE,
+                },
+                'relationships': {
+                    'users': {
+                        'data': {
+                            'id': user._id,
+                            'type': 'users',
+                        }
+                    }
+                }
+            }
+        }
+
+    def update_contributor_payload(self, user, **attributes):
+        return {
+            'data': {
+                'id': f'{self.preprint_v1._id}-{user._id}',
+                'type': 'contributors',
+                'attributes': attributes,
+            }
+        }
+
+    def test_admin_and_write_can_edit_metadata_of_previous_version(self):
+        v2_title = self.preprint_v2.title
+        for user in [self.creator, self.admin_contrib, self.write_contrib]:
+            title = f'Corrected title by {user._id}'
+            res = self.app.patch_json_api(
+                self.v1_url,
+                self.update_payload(title=title, description='Corrected abstract', tags=['corrected']),
+                auth=user.auth
+            )
+            assert res.status_code == 200
+            assert res.json['data']['attributes']['title'] == title
+
+            self.preprint_v1.reload()
+            assert self.preprint_v1.title == title
+            assert self.preprint_v1.description == 'Corrected abstract'
+            assert list(self.preprint_v1.tags.values_list('name', flat=True)) == ['corrected']
+
+        self.preprint_v2.reload()
+        assert self.preprint_v2.title == v2_title
+        assert not self.preprint_v2.tags.filter(name='corrected').exists()
+
+    def test_admin_and_write_can_edit_subjects_of_previous_version(self):
+        subject = SubjectFactory()
+        for user in [self.admin_contrib, self.write_contrib]:
+            res = self.app.patch_json_api(
+                self.v1_subjects_url,
+                {'data': [{'type': 'subjects', 'id': subject._id}]},
+                auth=user.auth
+            )
+            assert res.status_code == 200
+            assert list(self.preprint_v1.subjects.values_list('_id', flat=True)) == [subject._id]
+        assert not self.preprint_v2.subjects.filter(_id=subject._id).exists()
+
+    def test_read_and_non_contributors_cannot_edit_previous_version(self):
+        original_title = self.preprint_v1.title
+        for user in [self.read_contrib, self.non_contrib]:
+            res = self.app.patch_json_api(
+                self.v1_url,
+                self.update_payload(title='Not allowed'),
+                auth=user.auth,
+                expect_errors=True
+            )
+            assert res.status_code == 403
+
+            res = self.app.patch_json_api(
+                self.v1_subjects_url,
+                {'data': [{'type': 'subjects', 'id': SubjectFactory()._id}]},
+                auth=user.auth,
+                expect_errors=True
+            )
+            assert res.status_code == 403
+
+        res = self.app.patch_json_api(self.v1_url, self.update_payload(title='Not allowed'), expect_errors=True)
+        assert res.status_code == 401
+
+        self.preprint_v1.reload()
+        assert self.preprint_v1.title == original_title
+
+    def test_admin_can_edit_contributors_of_previous_version(self):
+        new_contrib = AuthUserFactory()
+        with capture_notifications():
+            res = self.app.post_json_api(
+                self.v1_contributors_url,
+                self.add_contributor_payload(new_contrib),
+                auth=self.admin_contrib.auth
+            )
+        assert res.status_code == 201
+        assert self.preprint_v1.is_contributor(new_contrib)
+        assert not self.preprint_v2.is_contributor(new_contrib)
+
+        res = self.app.patch_json_api(
+            f'{self.v1_contributors_url}{self.read_contrib._id}/',
+            self.update_contributor_payload(self.read_contrib, permission=permissions.WRITE, index=0),
+            auth=self.admin_contrib.auth
+        )
+        assert res.status_code == 200
+        assert self.preprint_v1.has_permission(self.read_contrib, permissions.WRITE)
+        assert self.preprint_v1.get_contributor_order()[0] == self.preprint_v1.preprintcontributor_set.get(user=self.read_contrib).id
+        assert not self.preprint_v2.has_permission(self.read_contrib, permissions.WRITE)
+
+        # bulk update, as sent by the preprint edit flow
+        res = self.app.patch_json_api(
+            self.v1_contributors_url,
+            {'data': [self.update_contributor_payload(self.write_contrib, permission=permissions.READ)['data']]},
+            auth=self.admin_contrib.auth,
+            bulk=True
+        )
+        assert res.status_code == 200
+        assert not self.preprint_v1.has_permission(self.write_contrib, permissions.WRITE)
+        assert self.preprint_v2.has_permission(self.write_contrib, permissions.WRITE)
+
+        res = self.app.delete_json_api(
+            f'{self.v1_contributors_url}{new_contrib._id}/',
+            auth=self.admin_contrib.auth
+        )
+        assert res.status_code == 204
+        assert not self.preprint_v1.is_contributor(new_contrib)
+
+    def test_write_and_read_cannot_edit_contributors_of_previous_version(self):
+        new_contrib = AuthUserFactory()
+        for user in [self.write_contrib, self.read_contrib]:
+            res = self.app.post_json_api(
+                self.v1_contributors_url,
+                self.add_contributor_payload(new_contrib),
+                auth=user.auth,
+                expect_errors=True
+            )
+            assert res.status_code == 403
+
+            res = self.app.patch_json_api(
+                f'{self.v1_contributors_url}{self.admin_contrib._id}/',
+                self.update_contributor_payload(self.admin_contrib, permission=permissions.READ),
+                auth=user.auth,
+                expect_errors=True
+            )
+            assert res.status_code == 403
+
+            res = self.app.patch_json_api(
+                self.v1_contributors_url,
+                {'data': [self.update_contributor_payload(self.admin_contrib, permission=permissions.READ)['data']]},
+                auth=user.auth,
+                expect_errors=True,
+                bulk=True
+            )
+            assert res.status_code == 403
+
+            res = self.app.delete_json_api(
+                f'{self.v1_contributors_url}{self.admin_contrib._id}/',
+                auth=user.auth,
+                expect_errors=True
+            )
+            assert res.status_code == 403
+
+        assert not self.preprint_v1.is_contributor(new_contrib)
+        assert self.preprint_v1.has_permission(self.admin_contrib, permissions.ADMIN)