From a2bcee1ab728ec9fb0d63320d044a488bf301f93 Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Mon, 9 Dec 2024 15:14:05 +0200 Subject: [PATCH 01/17] - ENG-6645 fix api1 failing tests in test_views.py --- admin/preprints/views.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/admin/preprints/views.py b/admin/preprints/views.py index d70630963dd..96274bb66a9 100644 --- a/admin/preprints/views.py +++ b/admin/preprints/views.py @@ -330,6 +330,18 @@ def get_object(self): target=target, ).first() + # def get_object(self): + # preprint = Preprint.load(self.kwargs['guid']) + # if preprint is None: + # return None + # + # return PreprintRequest.objects.filter( + # request_type='withdrawal', + # target=preprint + # ).exclude( + # machine_state='initial' + # ).first() + def get_success_url(self): return reverse_lazy('preprints:withdrawal-requests') From ab70669ef6d46b6da832e0f5ca08b6c5fd2b9157 Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Wed, 11 Dec 2024 16:22:49 +0200 Subject: [PATCH 02/17] - ENG-6645 fixed all failing tests api1 --- api_tests/preprints/filters/test_filters.py | 6 +- .../preprints/views/test_preprint_list.py | 15 ++++- .../test_preprint_provider_request_list.py | 6 +- api_tests/requests/mixins.py | 7 +-- api_tests/reviews/mixins/filter_mixins.py | 57 +++++++++++-------- api_tests/users/views/test_user_claim.py | 3 +- 6 files changed, 57 insertions(+), 37 deletions(-) diff --git a/api_tests/preprints/filters/test_filters.py b/api_tests/preprints/filters/test_filters.py index f52b7accea2..5871bec8bef 100644 --- a/api_tests/preprints/filters/test_filters.py +++ b/api_tests/preprints/filters/test_filters.py @@ -141,10 +141,10 @@ def test_id_filter_null(self, app, user, id_url): actual = [preprint['id'] for preprint in res.json['data']] assert expected == actual - def test_id_filter_equals_returns_one( - self, app, user, preprint_two, id_url): + def test_id_filter_equals_returns_one(self, app, user, preprint_two, id_url): expected = [preprint_two._id] - res = app.get(f'{id_url}{preprint_two._id}', auth=user.auth) + unversioned_id = preprint_two._id.split('_v')[0] + res = app.get(f'{id_url}{unversioned_id}', auth=user.auth) actual = [preprint['id'] for preprint in res.json['data']] assert expected == actual diff --git a/api_tests/preprints/views/test_preprint_list.py b/api_tests/preprints/views/test_preprint_list.py index 5fd8f224a01..a1128dacc21 100644 --- a/api_tests/preprints/views/test_preprint_list.py +++ b/api_tests/preprints/views/test_preprint_list.py @@ -16,6 +16,7 @@ PreprintIsValidListMixin, ) from api_tests.reviews.mixins.filter_mixins import ReviewableFilterMixin +from framework.auth import Auth from osf.models import Preprint, Node from osf import features from osf.utils.workflows import DefaultStates @@ -129,7 +130,8 @@ def test_create_preprint_with_supplementary_node( assert res.status_code == 201 preprint = Preprint.load(res.json['data']['id']) assert preprint.node == supplementary_project - assert Node.objects.filter(preprints__guids___id=res.json['data']['id']).exists() + preprint_id = res.json['data']['id'].split('_v')[0] + assert Node.objects.filter(preprints__guids___id=preprint_id).exists() def test_create_preprint_with_incorrectly_specified_node( self, app, user_one, provider, url, preprint_payload, supplementary_project @@ -509,9 +511,16 @@ def test_file_not_osfstorage(self): assert res.json['errors'][0]['detail'] == 'This file is not a valid primary file for this preprint.' def test_preprint_contributor_signal_sent_on_creation(self): - # Signal sent but bails out early without sending email + project_owner = AuthUserFactory() + other_user_project = ProjectFactory(creator=project_owner) + other_user_project.add_contributor(self.user, permissions=permissions.WRITE, visible=True, save=True) + other_user_project.remove_contributor(self.user, auth=Auth(project_owner)) with capture_signals() as mock_signals: - payload = build_preprint_create_payload(provider_id=self.provider._id) + payload = build_preprint_create_payload( + node_id=other_user_project._id, + provider_id=self.provider._id + ) + other_user_project.add_contributor(self.user, permissions=permissions.WRITE, visible=True, save=True) res = self.app.post_json_api(self.url, payload, auth=self.user.auth) assert res.status_code == 201 diff --git a/api_tests/providers/preprints/views/test_preprint_provider_request_list.py b/api_tests/providers/preprints/views/test_preprint_provider_request_list.py index 2fbe6c1bd9e..e09bd5eb33f 100644 --- a/api_tests/providers/preprints/views/test_preprint_provider_request_list.py +++ b/api_tests/providers/preprints/views/test_preprint_provider_request_list.py @@ -44,12 +44,14 @@ def test_list(self, app, admin, moderator, write_contrib, noncontrib, pre_mod_pr assert res.json['data'][0]['embeds']['target']['data']['id'] == post_mod_preprint._id # test_filter - res = app.get(f'{self.url(pre_mod_provider)}?filter[target]={pre_mod_preprint._id}', auth=moderator.auth) + pre_mod_preprint_id = pre_mod_preprint._id.split('_v')[0] if '_v' in pre_mod_preprint._id else pre_mod_preprint._id + res = app.get(f'{self.url(pre_mod_provider)}?filter[target]={pre_mod_preprint_id}', auth=moderator.auth) assert res.status_code == 200 assert len(res.json['data']) == 1 assert res.json['data'][0]['id'] == pre_request._id - res = app.get(f'{self.url(post_mod_provider)}?filter[target]={post_mod_preprint._id}', auth=moderator.auth) + post_mod_preprint_id = post_mod_preprint._id.split('_v')[0] if '_v' in post_mod_preprint._id else post_mod_preprint._id + res = app.get(f'{self.url(post_mod_provider)}?filter[target]={post_mod_preprint_id}', auth=moderator.auth) assert res.status_code == 200 assert len(res.json['data']) == 1 assert res.json['data'][0]['id'] == post_request._id diff --git a/api_tests/requests/mixins.py b/api_tests/requests/mixins.py index 4b281b0862d..70d5142692c 100644 --- a/api_tests/requests/mixins.py +++ b/api_tests/requests/mixins.py @@ -107,18 +107,17 @@ def pre_mod_preprint(self, admin, write_contrib, pre_mod_provider): pre = PreprintFactory( creator=admin, provider=pre_mod_provider, - is_published=False, - machine_state='pending' + is_published=True, + machine_state='accepted', ) pre.ever_public = True + pre.is_public = True pre.save() pre.add_contributor( contributor=write_contrib, permissions=permissions.WRITE, save=True ) - pre.is_public = True - pre.save() return pre @pytest.fixture() diff --git a/api_tests/reviews/mixins/filter_mixins.py b/api_tests/reviews/mixins/filter_mixins.py index a42a86efaa5..05c906621b7 100644 --- a/api_tests/reviews/mixins/filter_mixins.py +++ b/api_tests/reviews/mixins/filter_mixins.py @@ -84,9 +84,12 @@ def user(self, allowed_providers): return user def test_filter_actions(self, app, url, user, expected_actions): - # unfiltered - expected = {item._id for item in expected_actions} - actual = get_actual(app, url, user) + # Strip any version suffixes from expected IDs + def unversion_id(_id): + return _id.split('_v')[0] if '_v' in _id else _id + + expected = {unversion_id(item._id) for item in expected_actions} + actual = {unversion_id(a) for a in get_actual(app, url, user)} assert expected == actual if not expected_actions: @@ -95,61 +98,67 @@ def test_filter_actions(self, app, url, user, expected_actions): action = expected_actions[0] # filter by id - expected = {action._id} - actual = get_actual(app, url, user, id=action._id) + unversioned_action_id = unversion_id(action._id) + expected = {unversioned_action_id} + actual = {unversion_id(a) for a in get_actual(app, url, user, id=unversioned_action_id)} assert expected == actual # filter by trigger expected = { - item._id for item in expected_actions if item.trigger == action.trigger} - actual = get_actual(app, url, user, trigger=action.trigger) + unversion_id(item._id) for item in expected_actions if item.trigger == action.trigger + } + actual = {unversion_id(a) for a in get_actual(app, url, user, trigger=action.trigger)} assert expected == actual # filter by from_state expected = { - item._id for item in expected_actions if item.from_state == action.from_state} - actual = get_actual(app, url, user, from_state=action.from_state) + unversion_id(item._id) for item in expected_actions if item.from_state == action.from_state + } + actual = {unversion_id(a) for a in get_actual(app, url, user, from_state=action.from_state)} assert expected == actual # filter by to_state expected = { - item._id for item in expected_actions if item.to_state == action.to_state} - actual = get_actual(app, url, user, to_state=action.to_state) + unversion_id(item._id) for item in expected_actions if item.to_state == action.to_state + } + actual = {unversion_id(a) for a in get_actual(app, url, user, to_state=action.to_state)} assert expected == actual # filter by date_created - expected = {item._id for item in expected_actions} - actual = get_actual(app, url, user, date_created=action.created) + expected = {unversion_id(item._id) for item in expected_actions} + actual = {unversion_id(a) for a in get_actual(app, url, user, date_created=action.created)} assert expected == actual expected = set() - actual = get_actual( + actual = {unversion_id(a) for a in get_actual( app, url, user, - date_created=action.created - timedelta(days=1)) + date_created=action.created - timedelta(days=1))} assert expected == actual # filter by date_modified - expected = {item._id for item in expected_actions} - actual = get_actual(app, url, user, date_modified=action.modified) + expected = {unversion_id(item._id) for item in expected_actions} + actual = {unversion_id(a) for a in get_actual(app, url, user, date_modified=action.modified)} assert expected == actual expected = set() - actual = get_actual( + actual = {unversion_id(a) for a in get_actual( app, url, user, - date_modified=action.modified - timedelta(days=1)) + date_modified=action.modified - timedelta(days=1))} assert expected == actual # filter by target expected = { - item._id for item in expected_actions if item.target_id == action.target_id} - actual = get_actual(app, url, user, target=action.target._id) + unversion_id(item._id) for item in expected_actions if item.target_id == action.target_id + } + actual = {unversion_id(a) for a in get_actual(app, url, user, target=unversion_id(action.target._id))} assert expected == actual # filter by provider expected = { - item._id for item in expected_actions if item.target.provider_id == action.target.provider_id} - actual = get_actual( - app, url, user, provider=action.target.provider._id) + unversion_id(item._id) for item in expected_actions if item.target.provider_id == action.target.provider_id + } + actual = {unversion_id(a) for a in get_actual( + app, url, user, provider=action.target.provider._id)} assert expected == actual diff --git a/api_tests/users/views/test_user_claim.py b/api_tests/users/views/test_user_claim.py index 68e6cfd52dd..f35312c1748 100644 --- a/api_tests/users/views/test_user_claim.py +++ b/api_tests/users/views/test_user_claim.py @@ -146,9 +146,10 @@ def test_claim_unauth_success_with_unknown_email(self, app, url, project, unreg_ assert mock_mail.call_count == 2 def test_claim_unauth_success_with_preprint_id(self, app, url, preprint, unreg_user, mock_mail): + unversioned_id = preprint._id.split('_v')[0] res = app.post_json_api( url.format(unreg_user._id), - self.payload(email='david@david.son', id=preprint._id), + self.payload(email='david@david.son', id=unversioned_id), ) assert res.status_code == 204 assert mock_mail.call_count == 1 From 6591a475b100d02558372d236e057c365cf6996f Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Thu, 19 Dec 2024 15:20:04 +0200 Subject: [PATCH 03/17] - ENG-6645 fix failing tests api1 --- admin/preprints/views.py | 12 ------- api/base/filters.py | 22 ++++++++++++- api/preprints/serializers.py | 17 ++++++++++ api/users/views.py | 33 +++++++++++++++---- api_tests/preprints/filters/test_filters.py | 3 +- .../preprints/views/test_preprint_list.py | 11 ++----- api_tests/users/views/test_user_claim.py | 3 +- 7 files changed, 69 insertions(+), 32 deletions(-) diff --git a/admin/preprints/views.py b/admin/preprints/views.py index 96274bb66a9..d70630963dd 100644 --- a/admin/preprints/views.py +++ b/admin/preprints/views.py @@ -330,18 +330,6 @@ def get_object(self): target=target, ).first() - # def get_object(self): - # preprint = Preprint.load(self.kwargs['guid']) - # if preprint is None: - # return None - # - # return PreprintRequest.objects.filter( - # request_type='withdrawal', - # target=preprint - # ).exclude( - # machine_state='initial' - # ).first() - def get_success_url(self): return reverse_lazy('preprints:withdrawal-requests') diff --git a/api/base/filters.py b/api/base/filters.py index 412b1426cb4..5f90e5dc290 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -606,7 +606,27 @@ def postprocess_query_param(self, key, field_name, operation): operation['source_field_name'] = 'provider___id' if field_name == 'id': - operation['source_field_name'] = 'guids___id' + values = operation['value'] + object_ids = [] + for val in values: + base_guid, version = Guid.split_guid(val) + if version: + try: + version = int(version) + except ValueError: + continue + base_guid_obj = Guid.objects.filter(_id=base_guid).first() + if not base_guid_obj: + continue + obj_ids = base_guid_obj.versions.filter(version=version).values_list('object_id', flat=True) + object_ids.extend(obj_ids) + else: + obj_ids = Guid.objects.filter(_id=val).values_list('object_id', flat=True) + object_ids.extend(obj_ids) + + operation['source_field_name'] = 'id__in' + operation['value'] = list(object_ids) + operation['op'] = 'eq' if field_name == 'subjects': self.postprocess_subject_query_param(operation) diff --git a/api/preprints/serializers.py b/api/preprints/serializers.py index d4b7e9bc820..40c53aedd6d 100644 --- a/api/preprints/serializers.py +++ b/api/preprints/serializers.py @@ -576,6 +576,23 @@ def create(self, validated_data): description = validated_data.pop('description', '') preprint = Preprint.create(provider=provider, title=title, creator=creator, description=description) + if preprint.node: + preprint.node.add_contributor( + creator, + permissions=osf_permissions.ADMIN, + visible=True, + auth=get_user_auth(self.context['request']), + save=True, + send_signals=True + ) + else: + project_signals.contributor_added.send( + preprint, + contributor=creator, + auth=get_user_auth(self.context['request']), + email_template='preprint', + ) + return self.update(preprint, validated_data) diff --git a/api/users/views.py b/api/users/views.py index 927b5dc2f9b..a74cf50f139 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -792,23 +792,44 @@ def post(self, request, *args, **kwargs): record_id = (request.data.get('id', None) or '').lower().strip() if not record_id: raise ValidationError('Must specify record "id".') + claimed_user = self.get_user(check_permissions=True) # Ensures claimability if claimed_user.is_disabled: raise ValidationError('Cannot claim disabled account.') - try: - record_referent = Guid.objects.get(_id=record_id).referent - except Guid.DoesNotExist: - raise NotFound('Unable to find specified record.') + + base_guid, version = Guid.split_guid(record_id) + if version: + try: + version = int(version) + except ValueError: + raise NotFound('Unable to find specified record.') + + guid_obj = Guid.objects.filter(_id=base_guid).first() + if not guid_obj: + raise NotFound('Unable to find specified record.') + + versioned_entry = guid_obj.versions.filter(version=version).first() + if not versioned_entry: + raise NotFound('Unable to find specified record.') + + record_referent = versioned_entry.referent + else: + try: + record_referent = Guid.objects.get(_id=base_guid).referent + except Guid.DoesNotExist: + raise NotFound('Unable to find specified record.') try: unclaimed_record = claimed_user.unclaimed_records[record_referent._id] except KeyError: - if isinstance(record_referent, Preprint) and record_referent.node and record_referent.node._id in claimed_user.unclaimed_records: + if isinstance(record_referent, + Preprint) and record_referent.node and record_referent.node._id in claimed_user.unclaimed_records: record_referent = record_referent.node unclaimed_record = claimed_user.unclaimed_records[record_referent._id] else: raise NotFound('Unable to find specified record.') + # The rest of the logic remains unchanged if claimer.is_anonymous and email: claimer = get_user(email=email) try: @@ -825,9 +846,9 @@ def post(self, request, *args, **kwargs): self._send_claim_email(claimer, claimed_user, record_referent, registered=True) except HTTPError as e: raise ValidationError(e.data['message_long']) - else: raise ValidationError('Must either be logged in or specify claim email.') + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/api_tests/preprints/filters/test_filters.py b/api_tests/preprints/filters/test_filters.py index 5871bec8bef..9a483ad55ec 100644 --- a/api_tests/preprints/filters/test_filters.py +++ b/api_tests/preprints/filters/test_filters.py @@ -143,8 +143,7 @@ def test_id_filter_null(self, app, user, id_url): def test_id_filter_equals_returns_one(self, app, user, preprint_two, id_url): expected = [preprint_two._id] - unversioned_id = preprint_two._id.split('_v')[0] - res = app.get(f'{id_url}{unversioned_id}', auth=user.auth) + res = app.get(f'{id_url}{preprint_two._id}', auth=user.auth) actual = [preprint['id'] for preprint in res.json['data']] assert expected == actual diff --git a/api_tests/preprints/views/test_preprint_list.py b/api_tests/preprints/views/test_preprint_list.py index a1128dacc21..07b45c30304 100644 --- a/api_tests/preprints/views/test_preprint_list.py +++ b/api_tests/preprints/views/test_preprint_list.py @@ -511,16 +511,9 @@ def test_file_not_osfstorage(self): assert res.json['errors'][0]['detail'] == 'This file is not a valid primary file for this preprint.' def test_preprint_contributor_signal_sent_on_creation(self): - project_owner = AuthUserFactory() - other_user_project = ProjectFactory(creator=project_owner) - other_user_project.add_contributor(self.user, permissions=permissions.WRITE, visible=True, save=True) - other_user_project.remove_contributor(self.user, auth=Auth(project_owner)) + # Signal sent but bails out early without sending email with capture_signals() as mock_signals: - payload = build_preprint_create_payload( - node_id=other_user_project._id, - provider_id=self.provider._id - ) - other_user_project.add_contributor(self.user, permissions=permissions.WRITE, visible=True, save=True) + payload = build_preprint_create_payload(provider_id=self.provider._id) res = self.app.post_json_api(self.url, payload, auth=self.user.auth) assert res.status_code == 201 diff --git a/api_tests/users/views/test_user_claim.py b/api_tests/users/views/test_user_claim.py index f35312c1748..68e6cfd52dd 100644 --- a/api_tests/users/views/test_user_claim.py +++ b/api_tests/users/views/test_user_claim.py @@ -146,10 +146,9 @@ def test_claim_unauth_success_with_unknown_email(self, app, url, project, unreg_ assert mock_mail.call_count == 2 def test_claim_unauth_success_with_preprint_id(self, app, url, preprint, unreg_user, mock_mail): - unversioned_id = preprint._id.split('_v')[0] res = app.post_json_api( url.format(unreg_user._id), - self.payload(email='david@david.son', id=unversioned_id), + self.payload(email='david@david.son', id=preprint._id), ) assert res.status_code == 204 assert mock_mail.call_count == 1 From 35657e367e8025740ddba8c55fad74b43d61da03 Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Thu, 19 Dec 2024 15:21:56 +0200 Subject: [PATCH 04/17] - ENG-6645 fix failing tests api1 --- api/users/views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/users/views.py b/api/users/views.py index a74cf50f139..8e35a32510b 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -829,7 +829,6 @@ def post(self, request, *args, **kwargs): else: raise NotFound('Unable to find specified record.') - # The rest of the logic remains unchanged if claimer.is_anonymous and email: claimer = get_user(email=email) try: From febabf3640427eb7f8084dc11b4b569c02e5ba74 Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Thu, 19 Dec 2024 17:20:09 +0200 Subject: [PATCH 05/17] - ENG-6645 fix test_create_preprint_with_supplementary_node + removed unneccessery changes in mixins.py --- api_tests/preprints/views/test_preprint_list.py | 15 +++++++++++---- api_tests/requests/mixins.py | 7 ++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/api_tests/preprints/views/test_preprint_list.py b/api_tests/preprints/views/test_preprint_list.py index 07b45c30304..2448045c124 100644 --- a/api_tests/preprints/views/test_preprint_list.py +++ b/api_tests/preprints/views/test_preprint_list.py @@ -124,14 +124,21 @@ def test_create_preprint_does_not_create_a_node(self, app, user_one, provider, u def test_create_preprint_with_supplementary_node( self, app, user_one, provider, url, preprint_payload, supplementary_project ): - preprint_payload['data']['relationships']['node'] = {'data': {'id': supplementary_project._id, 'type': 'nodes'}} - res = app.post_json_api(url, preprint_payload, auth=user_one.auth) + preprint_payload['data']['relationships']['node'] = { + 'data': {'id': supplementary_project._id, 'type': 'nodes'} + } + res = app.post_json_api(url, preprint_payload, auth=user_one.auth) assert res.status_code == 201 preprint = Preprint.load(res.json['data']['id']) + preprint_id, version = res.json['data']['id'].split('_v') + assert preprint.node == supplementary_project - preprint_id = res.json['data']['id'].split('_v')[0] - assert Node.objects.filter(preprints__guids___id=preprint_id).exists() + filtered_nodes = Node.objects.filter( + preprints__versioned_guids__guid___id=preprint_id, + preprints__versioned_guids__version=version + ) + assert supplementary_project in filtered_nodes def test_create_preprint_with_incorrectly_specified_node( self, app, user_one, provider, url, preprint_payload, supplementary_project diff --git a/api_tests/requests/mixins.py b/api_tests/requests/mixins.py index 70d5142692c..4b281b0862d 100644 --- a/api_tests/requests/mixins.py +++ b/api_tests/requests/mixins.py @@ -107,17 +107,18 @@ def pre_mod_preprint(self, admin, write_contrib, pre_mod_provider): pre = PreprintFactory( creator=admin, provider=pre_mod_provider, - is_published=True, - machine_state='accepted', + is_published=False, + machine_state='pending' ) pre.ever_public = True - pre.is_public = True pre.save() pre.add_contributor( contributor=write_contrib, permissions=permissions.WRITE, save=True ) + pre.is_public = True + pre.save() return pre @pytest.fixture() From b47863403b81dfd9988c3dd9df7aa67dbbd9bcce Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Fri, 20 Dec 2024 16:29:02 +0200 Subject: [PATCH 06/17] - ENG-6645 fix test_list + test_filter_actions --- .../test_preprint_provider_request_list.py | 6 +- api_tests/reviews/mixins/filter_mixins.py | 57 ++++++++----------- 2 files changed, 26 insertions(+), 37 deletions(-) diff --git a/api_tests/providers/preprints/views/test_preprint_provider_request_list.py b/api_tests/providers/preprints/views/test_preprint_provider_request_list.py index e09bd5eb33f..2fbe6c1bd9e 100644 --- a/api_tests/providers/preprints/views/test_preprint_provider_request_list.py +++ b/api_tests/providers/preprints/views/test_preprint_provider_request_list.py @@ -44,14 +44,12 @@ def test_list(self, app, admin, moderator, write_contrib, noncontrib, pre_mod_pr assert res.json['data'][0]['embeds']['target']['data']['id'] == post_mod_preprint._id # test_filter - pre_mod_preprint_id = pre_mod_preprint._id.split('_v')[0] if '_v' in pre_mod_preprint._id else pre_mod_preprint._id - res = app.get(f'{self.url(pre_mod_provider)}?filter[target]={pre_mod_preprint_id}', auth=moderator.auth) + res = app.get(f'{self.url(pre_mod_provider)}?filter[target]={pre_mod_preprint._id}', auth=moderator.auth) assert res.status_code == 200 assert len(res.json['data']) == 1 assert res.json['data'][0]['id'] == pre_request._id - post_mod_preprint_id = post_mod_preprint._id.split('_v')[0] if '_v' in post_mod_preprint._id else post_mod_preprint._id - res = app.get(f'{self.url(post_mod_provider)}?filter[target]={post_mod_preprint_id}', auth=moderator.auth) + res = app.get(f'{self.url(post_mod_provider)}?filter[target]={post_mod_preprint._id}', auth=moderator.auth) assert res.status_code == 200 assert len(res.json['data']) == 1 assert res.json['data'][0]['id'] == post_request._id diff --git a/api_tests/reviews/mixins/filter_mixins.py b/api_tests/reviews/mixins/filter_mixins.py index 05c906621b7..a42a86efaa5 100644 --- a/api_tests/reviews/mixins/filter_mixins.py +++ b/api_tests/reviews/mixins/filter_mixins.py @@ -84,12 +84,9 @@ def user(self, allowed_providers): return user def test_filter_actions(self, app, url, user, expected_actions): - # Strip any version suffixes from expected IDs - def unversion_id(_id): - return _id.split('_v')[0] if '_v' in _id else _id - - expected = {unversion_id(item._id) for item in expected_actions} - actual = {unversion_id(a) for a in get_actual(app, url, user)} + # unfiltered + expected = {item._id for item in expected_actions} + actual = get_actual(app, url, user) assert expected == actual if not expected_actions: @@ -98,67 +95,61 @@ def unversion_id(_id): action = expected_actions[0] # filter by id - unversioned_action_id = unversion_id(action._id) - expected = {unversioned_action_id} - actual = {unversion_id(a) for a in get_actual(app, url, user, id=unversioned_action_id)} + expected = {action._id} + actual = get_actual(app, url, user, id=action._id) assert expected == actual # filter by trigger expected = { - unversion_id(item._id) for item in expected_actions if item.trigger == action.trigger - } - actual = {unversion_id(a) for a in get_actual(app, url, user, trigger=action.trigger)} + item._id for item in expected_actions if item.trigger == action.trigger} + actual = get_actual(app, url, user, trigger=action.trigger) assert expected == actual # filter by from_state expected = { - unversion_id(item._id) for item in expected_actions if item.from_state == action.from_state - } - actual = {unversion_id(a) for a in get_actual(app, url, user, from_state=action.from_state)} + item._id for item in expected_actions if item.from_state == action.from_state} + actual = get_actual(app, url, user, from_state=action.from_state) assert expected == actual # filter by to_state expected = { - unversion_id(item._id) for item in expected_actions if item.to_state == action.to_state - } - actual = {unversion_id(a) for a in get_actual(app, url, user, to_state=action.to_state)} + item._id for item in expected_actions if item.to_state == action.to_state} + actual = get_actual(app, url, user, to_state=action.to_state) assert expected == actual # filter by date_created - expected = {unversion_id(item._id) for item in expected_actions} - actual = {unversion_id(a) for a in get_actual(app, url, user, date_created=action.created)} + expected = {item._id for item in expected_actions} + actual = get_actual(app, url, user, date_created=action.created) assert expected == actual expected = set() - actual = {unversion_id(a) for a in get_actual( + actual = get_actual( app, url, user, - date_created=action.created - timedelta(days=1))} + date_created=action.created - timedelta(days=1)) assert expected == actual # filter by date_modified - expected = {unversion_id(item._id) for item in expected_actions} - actual = {unversion_id(a) for a in get_actual(app, url, user, date_modified=action.modified)} + expected = {item._id for item in expected_actions} + actual = get_actual(app, url, user, date_modified=action.modified) assert expected == actual expected = set() - actual = {unversion_id(a) for a in get_actual( + actual = get_actual( app, url, user, - date_modified=action.modified - timedelta(days=1))} + date_modified=action.modified - timedelta(days=1)) assert expected == actual # filter by target expected = { - unversion_id(item._id) for item in expected_actions if item.target_id == action.target_id - } - actual = {unversion_id(a) for a in get_actual(app, url, user, target=unversion_id(action.target._id))} + item._id for item in expected_actions if item.target_id == action.target_id} + actual = get_actual(app, url, user, target=action.target._id) assert expected == actual # filter by provider expected = { - unversion_id(item._id) for item in expected_actions if item.target.provider_id == action.target.provider_id - } - actual = {unversion_id(a) for a in get_actual( - app, url, user, provider=action.target.provider._id)} + item._id for item in expected_actions if item.target.provider_id == action.target.provider_id} + actual = get_actual( + app, url, user, provider=action.target.provider._id) assert expected == actual From e2a79c27c5b8a47d1cbb57d90642c808b88ba851 Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Tue, 24 Dec 2024 16:23:42 +0200 Subject: [PATCH 07/17] - ENG-6645 code adjustments --- api/base/filters.py | 20 +++++----------- api/preprints/serializers.py | 17 ------------- api/users/views.py | 24 +++---------------- .../preprints/views/test_preprint_list.py | 1 - osf/models/mixins.py | 3 ++- 5 files changed, 11 insertions(+), 54 deletions(-) diff --git a/api/base/filters.py b/api/base/filters.py index 5f90e5dc290..6ddf3af6b96 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -608,22 +608,14 @@ def postprocess_query_param(self, key, field_name, operation): if field_name == 'id': values = operation['value'] object_ids = [] + for val in values: - base_guid, version = Guid.split_guid(val) - if version: - try: - version = int(version) - except ValueError: - continue - base_guid_obj = Guid.objects.filter(_id=base_guid).first() - if not base_guid_obj: - continue - obj_ids = base_guid_obj.versions.filter(version=version).values_list('object_id', flat=True) - object_ids.extend(obj_ids) - else: - obj_ids = Guid.objects.filter(_id=val).values_list('object_id', flat=True) - object_ids.extend(obj_ids) + referent, version = Guid.load_referent(val) + if referent is None: + continue + object_ids.append(referent.id) + # Override the operation to filter id__in=these object_ids operation['source_field_name'] = 'id__in' operation['value'] = list(object_ids) operation['op'] = 'eq' diff --git a/api/preprints/serializers.py b/api/preprints/serializers.py index 40c53aedd6d..d4b7e9bc820 100644 --- a/api/preprints/serializers.py +++ b/api/preprints/serializers.py @@ -576,23 +576,6 @@ def create(self, validated_data): description = validated_data.pop('description', '') preprint = Preprint.create(provider=provider, title=title, creator=creator, description=description) - if preprint.node: - preprint.node.add_contributor( - creator, - permissions=osf_permissions.ADMIN, - visible=True, - auth=get_user_auth(self.context['request']), - save=True, - send_signals=True - ) - else: - project_signals.contributor_added.send( - preprint, - contributor=creator, - auth=get_user_auth(self.context['request']), - email_template='preprint', - ) - return self.update(preprint, validated_data) diff --git a/api/users/views.py b/api/users/views.py index 8e35a32510b..41a6ff523c8 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -797,27 +797,9 @@ def post(self, request, *args, **kwargs): if claimed_user.is_disabled: raise ValidationError('Cannot claim disabled account.') - base_guid, version = Guid.split_guid(record_id) - if version: - try: - version = int(version) - except ValueError: - raise NotFound('Unable to find specified record.') - - guid_obj = Guid.objects.filter(_id=base_guid).first() - if not guid_obj: - raise NotFound('Unable to find specified record.') - - versioned_entry = guid_obj.versions.filter(version=version).first() - if not versioned_entry: - raise NotFound('Unable to find specified record.') - - record_referent = versioned_entry.referent - else: - try: - record_referent = Guid.objects.get(_id=base_guid).referent - except Guid.DoesNotExist: - raise NotFound('Unable to find specified record.') + record_referent, version = Guid.load_referent(record_id) + if not record_referent: + raise NotFound('Unable to find specified record.') try: unclaimed_record = claimed_user.unclaimed_records[record_referent._id] diff --git a/api_tests/preprints/views/test_preprint_list.py b/api_tests/preprints/views/test_preprint_list.py index 2448045c124..8e566402082 100644 --- a/api_tests/preprints/views/test_preprint_list.py +++ b/api_tests/preprints/views/test_preprint_list.py @@ -16,7 +16,6 @@ PreprintIsValidListMixin, ) from api_tests.reviews.mixins.filter_mixins import ReviewableFilterMixin -from framework.auth import Auth from osf.models import Preprint, Node from osf import features from osf.utils.workflows import DefaultStates diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 8b62c5cd3c1..d68130185c3 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -24,6 +24,7 @@ InvalidTagError, BlockedEmailError, ) +from . import Preprint from .node_relation import NodeRelation from .nodelog import NodeLog from .subject import Subject @@ -1384,7 +1385,7 @@ def add_contributor(self, contributor, permissions=None, visible=True, if save: self.save() - if self._id and contrib_to_add: + if (self._id and contrib_to_add) or (isinstance(self, Preprint) and contrib_to_add): project_signals.contributor_added.send(self, contributor=contributor, auth=auth, email_template=send_email, permissions=permissions) From 0f3516bef8b2412406c4bf9eae6efad44d8b5672 Mon Sep 17 00:00:00 2001 From: Andriy Sheredko Date: Thu, 26 Dec 2024 17:10:39 +0200 Subject: [PATCH 08/17] - ENG-6645 pre-commit fix --- api/users/views.py | 6 ++++-- osf/models/mixins.py | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/api/users/views.py b/api/users/views.py index 41a6ff523c8..b19efe359f7 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -804,8 +804,10 @@ def post(self, request, *args, **kwargs): try: unclaimed_record = claimed_user.unclaimed_records[record_referent._id] except KeyError: - if isinstance(record_referent, - Preprint) and record_referent.node and record_referent.node._id in claimed_user.unclaimed_records: + if isinstance( + record_referent, + Preprint, + ) and record_referent.node and record_referent.node._id in claimed_user.unclaimed_records: record_referent = record_referent.node unclaimed_record = claimed_user.unclaimed_records[record_referent._id] else: diff --git a/osf/models/mixins.py b/osf/models/mixins.py index d68130185c3..92bdacb1120 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -24,7 +24,7 @@ InvalidTagError, BlockedEmailError, ) -from . import Preprint +from .preprint import Preprint from .node_relation import NodeRelation from .nodelog import NodeLog from .subject import Subject From ebdb8874bb8247f9b2ba886b4b34930548d0ea96 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Mon, 30 Dec 2024 16:19:30 +0200 Subject: [PATCH 09/17] Fix circular import for osf/models/mixins.py file --- osf/models/mixins.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 92bdacb1120..7280950c02e 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -24,7 +24,6 @@ InvalidTagError, BlockedEmailError, ) -from .preprint import Preprint from .node_relation import NodeRelation from .nodelog import NodeLog from .subject import Subject @@ -1384,7 +1383,7 @@ def add_contributor(self, contributor, permissions=None, visible=True, ) if save: self.save() - + Preprint = apps.get_model('osf.Preprint') if (self._id and contrib_to_add) or (isinstance(self, Preprint) and contrib_to_add): project_signals.contributor_added.send(self, contributor=contributor, From dc59d6a6c4f18b1c7f5e668f6e4b112d9b729255 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Thu, 2 Jan 2025 16:08:40 +0200 Subject: [PATCH 10/17] Fix some comments to pull request --- api/base/filters.py | 31 +++++++++++++++++-------------- api/users/views.py | 5 +++-- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/api/base/filters.py b/api/base/filters.py index 6ddf3af6b96..483e45d6b9f 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -606,25 +606,28 @@ def postprocess_query_param(self, key, field_name, operation): operation['source_field_name'] = 'provider___id' if field_name == 'id': - values = operation['value'] - object_ids = [] - - for val in values: - referent, version = Guid.load_referent(val) - if referent is None: - continue - object_ids.append(referent.id) - - # Override the operation to filter id__in=these object_ids - operation['source_field_name'] = 'id__in' - operation['value'] = list(object_ids) - operation['op'] = 'eq' + self.postprocess_version_id_query_param(operation) if field_name == 'subjects': self.postprocess_subject_query_param(operation) + def postprocess_version_id_query_param(self, operation): + values = operation['value'] + object_ids = [] + + for val in values: + referent, version = Guid.load_referent(val) + if referent is None: + continue + object_ids.append(referent.id) + + # Override the operation to filter id__in=these object_ids + operation['source_field_name'] = 'id__in' + operation['value'] = list(object_ids) + operation['op'] = 'eq' + def preprints_queryset(self, base_queryset, auth_user, allow_contribs=True, public_only=False, latest_only=False): - preprints = Preprint.objects.can_view( + return Preprint.objects.can_view( base_queryset=base_queryset, user=auth_user, allow_contribs=allow_contribs, diff --git a/api/users/views.py b/api/users/views.py index b19efe359f7..296826ca48b 100644 --- a/api/users/views.py +++ b/api/users/views.py @@ -797,8 +797,9 @@ def post(self, request, *args, **kwargs): if claimed_user.is_disabled: raise ValidationError('Cannot claim disabled account.') - record_referent, version = Guid.load_referent(record_id) - if not record_referent: + try: + record_referent = Guid.objects.get(_id=record_id).referent + except Guid.DoesNotExist: raise NotFound('Unable to find specified record.') try: From db2febe375dfb4c4f94c621efe8b8c4b4364b297 Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Thu, 2 Jan 2025 17:11:30 +0200 Subject: [PATCH 11/17] Fix merge conflicts after rebase --- api/base/filters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/base/filters.py b/api/base/filters.py index 483e45d6b9f..94c95254b28 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -627,7 +627,7 @@ def postprocess_version_id_query_param(self, operation): operation['op'] = 'eq' def preprints_queryset(self, base_queryset, auth_user, allow_contribs=True, public_only=False, latest_only=False): - return Preprint.objects.can_view( + preprints = Preprint.objects.can_view( base_queryset=base_queryset, user=auth_user, allow_contribs=allow_contribs, From bdb865f0777625919a91d276757c79e9aa8fc61d Mon Sep 17 00:00:00 2001 From: Vlad0n20 Date: Fri, 3 Jan 2025 17:26:45 +0200 Subject: [PATCH 12/17] Fix tests Rebased and resolved merge conflicts --- api_tests/users/views/test_user_claim.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_tests/users/views/test_user_claim.py b/api_tests/users/views/test_user_claim.py index 68e6cfd52dd..20e19dbf30d 100644 --- a/api_tests/users/views/test_user_claim.py +++ b/api_tests/users/views/test_user_claim.py @@ -148,7 +148,7 @@ def test_claim_unauth_success_with_unknown_email(self, app, url, project, unreg_ def test_claim_unauth_success_with_preprint_id(self, app, url, preprint, unreg_user, mock_mail): res = app.post_json_api( url.format(unreg_user._id), - self.payload(email='david@david.son', id=preprint._id), + self.payload(email='david@david.son', id=preprint.get_guid()._id), ) assert res.status_code == 204 assert mock_mail.call_count == 1 From 593699f9f70534cbc45f4b38f1e1e320d9a69bb4 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 16:10:46 -0500 Subject: [PATCH 13/17] Refactor api filters by id for prerpint --- api/base/filters.py | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/api/base/filters.py b/api/base/filters.py index 94c95254b28..4d9bc37a442 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -601,31 +601,33 @@ class PreprintFilterMixin(ListFilterMixin): Subclasses must define `get_default_queryset()`. """ - def postprocess_query_param(self, key, field_name, operation): - if field_name == 'provider': - operation['source_field_name'] = 'provider___id' - - if field_name == 'id': - self.postprocess_version_id_query_param(operation) - if field_name == 'subjects': - self.postprocess_subject_query_param(operation) - - def postprocess_version_id_query_param(self, operation): - values = operation['value'] + @staticmethod + def postprocess_versioned_guid_id_query_param(operation): + """Handle query parameters when filtering on `id` for preprint and versioned guid. With versioned guid, + preprint no longer has guid._id for every version, and thus must switch to filter by pk. + """ object_ids = [] - - for val in values: + for val in operation['value']: referent, version = Guid.load_referent(val) if referent is None: continue object_ids.append(referent.id) - - # Override the operation to filter id__in=these object_ids + # Override the operation to filter `id__in=object_ids` operation['source_field_name'] = 'id__in' - operation['value'] = list(object_ids) + operation['value'] = object_ids operation['op'] = 'eq' + def postprocess_query_param(self, key, field_name, operation): + if field_name == 'provider': + operation['source_field_name'] = 'provider___id' + + if field_name == 'id': + PreprintFilterMixin.postprocess_versioned_guid_id_query_param(operation) + + if field_name == 'subjects': + self.postprocess_subject_query_param(operation) + def preprints_queryset(self, base_queryset, auth_user, allow_contribs=True, public_only=False, latest_only=False): preprints = Preprint.objects.can_view( base_queryset=base_queryset, From 17fdb768aef677ed7a8b2a8810ff0b51c3c18155 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 16:21:41 -0500 Subject: [PATCH 14/17] Use Guid.split_guid() in test_create_preprint_with_supplementary_node --- api_tests/preprints/views/test_preprint_list.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api_tests/preprints/views/test_preprint_list.py b/api_tests/preprints/views/test_preprint_list.py index 8e566402082..181351eecd1 100644 --- a/api_tests/preprints/views/test_preprint_list.py +++ b/api_tests/preprints/views/test_preprint_list.py @@ -16,7 +16,7 @@ PreprintIsValidListMixin, ) from api_tests.reviews.mixins.filter_mixins import ReviewableFilterMixin -from osf.models import Preprint, Node +from osf.models import Guid, Node, Preprint from osf import features from osf.utils.workflows import DefaultStates from osf.utils import permissions @@ -130,7 +130,7 @@ def test_create_preprint_with_supplementary_node( res = app.post_json_api(url, preprint_payload, auth=user_one.auth) assert res.status_code == 201 preprint = Preprint.load(res.json['data']['id']) - preprint_id, version = res.json['data']['id'].split('_v') + preprint_id, version = Guid.split_guid(res.json['data']['id']) assert preprint.node == supplementary_project filtered_nodes = Node.objects.filter( From 33140fd3c154c83d7be48f9a11687d137a344386 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 16:43:57 -0500 Subject: [PATCH 15/17] Revert incorret fix, which should not remove version info to pass Test: test_claim_unauth_success_with_preprint_id --- api_tests/users/views/test_user_claim.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_tests/users/views/test_user_claim.py b/api_tests/users/views/test_user_claim.py index 20e19dbf30d..68e6cfd52dd 100644 --- a/api_tests/users/views/test_user_claim.py +++ b/api_tests/users/views/test_user_claim.py @@ -148,7 +148,7 @@ def test_claim_unauth_success_with_unknown_email(self, app, url, project, unreg_ def test_claim_unauth_success_with_preprint_id(self, app, url, preprint, unreg_user, mock_mail): res = app.post_json_api( url.format(unreg_user._id), - self.payload(email='david@david.son', id=preprint.get_guid()._id), + self.payload(email='david@david.son', id=preprint._id), ) assert res.status_code == 204 assert mock_mail.call_count == 1 From d535d5e59d5e9b206542e5a28f5937f27773f577 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 17:15:45 -0500 Subject: [PATCH 16/17] Revoke incorrect fix: should fix `Preprint.save()` instead Test: test_preprint_contributor_signal_sent_on_creation --- osf/models/mixins.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 7280950c02e..5147d68c1b6 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1383,8 +1383,11 @@ def add_contributor(self, contributor, permissions=None, visible=True, ) if save: self.save() - Preprint = apps.get_model('osf.Preprint') - if (self._id and contrib_to_add) or (isinstance(self, Preprint) and contrib_to_add): + if self._id and contrib_to_add: + # TODO: remove the comment after `Preprint.save()` has been fixed. + # There is a bug for Preprint when the first `.save()` is called. It triggers a series of events which + # includes adding creator as the contributor. However, with versioned guid, there is a gap between preprint + # is saved and guid is updated, during which `self._id` is None and the signal can not be be sent. project_signals.contributor_added.send(self, contributor=contributor, auth=auth, email_template=send_email, permissions=permissions) From e1f2e803780fbea417717e721296827ea27f2727 Mon Sep 17 00:00:00 2001 From: Longze Chen Date: Mon, 6 Jan 2025 17:51:20 -0500 Subject: [PATCH 17/17] Fix flake8 --- osf/models/mixins.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 5147d68c1b6..d47af6c3b68 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -1383,11 +1383,12 @@ def add_contributor(self, contributor, permissions=None, visible=True, ) if save: self.save() + # TODO: Remove this comment after `Preprint.save()` has been fixed. There is a bug for Preprint when the + # first `.save()` is called. It triggers a series of events which includes adding creator as one + # contributor. However, with versioned guid, there is a gap between when preprint os saved and when + # guid and versioned guid are saved. During this gap, `self._id` is unfortunately `None` and the + # signal can not be be sent. if self._id and contrib_to_add: - # TODO: remove the comment after `Preprint.save()` has been fixed. - # There is a bug for Preprint when the first `.save()` is called. It triggers a series of events which - # includes adding creator as the contributor. However, with versioned guid, there is a gap between preprint - # is saved and guid is updated, during which `self._id` is None and the signal can not be be sent. project_signals.contributor_added.send(self, contributor=contributor, auth=auth, email_template=send_email, permissions=permissions)