From db5979faa7e7a06b2eee5dd956c47f857d21962d Mon Sep 17 00:00:00 2001 From: erinspace Date: Mon, 22 Oct 2018 21:54:47 -0400 Subject: [PATCH 01/59] Recursively update most recent fileversion for folder contents [#PLAT-1118] --- addons/osfstorage/models.py | 21 +++++++++++++++++---- addons/osfstorage/tests/test_models.py | 24 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/addons/osfstorage/models.py b/addons/osfstorage/models.py index d2e07d5f1ce..9717910eba0 100644 --- a/addons/osfstorage/models.py +++ b/addons/osfstorage/models.py @@ -160,16 +160,29 @@ def delete(self, user=None, parent=None, **kwargs): self._materialized_path = self.materialized_path return super(OsfStorageFileNode, self).delete(user=user, parent=parent) if self._check_delete_allowed() else None + def update_folder_regions(self, destination_parent): + for child in self.children.all(): + if child.is_file: + child.update_most_recent_fileversion(destination_parent) + else: + child.update_folder_regions(destination_parent) + + def update_most_recent_fileversion(self, destination_parent): + most_recent_fileversion = self.versions.select_related('region').order_by('-created').first() + if most_recent_fileversion and most_recent_fileversion.region != destination_parent.target.osfstorage_region: + most_recent_fileversion.region = destination_parent.target.osfstorage_region + most_recent_fileversion.save() + def move_under(self, destination_parent, name=None): if self.is_preprint_primary: if self.target != destination_parent.target or self.provider != destination_parent.provider: raise exceptions.FileNodeIsPrimaryFile() if self.is_checked_out: raise exceptions.FileNodeCheckedOutError() - most_recent_fileversion = self.versions.select_related('region').order_by('-created').first() - if most_recent_fileversion and most_recent_fileversion.region != destination_parent.target.osfstorage_region: - most_recent_fileversion.region = destination_parent.target.osfstorage_region - most_recent_fileversion.save() + if not self.is_file: + self.update_folder_regions(destination_parent) + else: + self.update_most_recent_fileversion(destination_parent) return super(OsfStorageFileNode, self).move_under(destination_parent, name) def check_in_or_out(self, user, checkout, save=False): diff --git a/addons/osfstorage/tests/test_models.py b/addons/osfstorage/tests/test_models.py index 30f2c1946a1..0fc078c2174 100644 --- a/addons/osfstorage/tests/test_models.py +++ b/addons/osfstorage/tests/test_models.py @@ -304,6 +304,30 @@ def test_move_nested(self): assert_equal(new_project, move_to.target) assert_equal(new_project, child.target) + def test_move_nested_between_regions(self): + canada = RegionFactory() + new_component = NodeFactory(parent=self.project) + component_node_settings = new_component.get_addon('osfstorage') + component_node_settings.region = canada + component_node_settings.save() + + move_to = component_node_settings.get_root() + to_move = self.node_settings.get_root().append_folder('Aaah').append_folder('Woop') + child = to_move.append_file('There it is') + + for _ in range(2): + version = factories.FileVersionFactory(region=self.node_settings.region) + child.versions.add(version) + child.save() + + moved = to_move.move_under(move_to) + child.reload() + + assert new_component == child.target + versions = child.versions.order_by('-created') + assert versions.first().region == component_node_settings.region + assert versions.last().region == self.node_settings.region + def test_copy_rename(self): to_copy = self.node_settings.get_root().append_file('Carp') copy_to = self.node_settings.get_root().append_folder('Cloud') From 40eebd08fd5455dccbba0cc055458d4cf5859cb5 Mon Sep 17 00:00:00 2001 From: erinspace Date: Mon, 22 Oct 2018 21:54:47 -0400 Subject: [PATCH 02/59] Recursively update most recent fileversion for folder contents [#PLAT-1118] --- addons/osfstorage/models.py | 21 +++++++++++++++++---- addons/osfstorage/tests/test_models.py | 24 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/addons/osfstorage/models.py b/addons/osfstorage/models.py index d2e07d5f1ce..d9e9ff6e99b 100644 --- a/addons/osfstorage/models.py +++ b/addons/osfstorage/models.py @@ -160,16 +160,29 @@ def delete(self, user=None, parent=None, **kwargs): self._materialized_path = self.materialized_path return super(OsfStorageFileNode, self).delete(user=user, parent=parent) if self._check_delete_allowed() else None + def update_folder_regions(self, destination_parent): + for child in self.children.all().prefetch_related('versions'): + if child.is_file: + child.update_most_recent_fileversion(destination_parent) + else: + child.update_folder_regions(destination_parent) + + def update_most_recent_fileversion(self, destination_parent): + most_recent_fileversion = self.versions.select_related('region').order_by('-created').first() + if most_recent_fileversion and most_recent_fileversion.region != destination_parent.target.osfstorage_region: + most_recent_fileversion.region = destination_parent.target.osfstorage_region + most_recent_fileversion.save() + def move_under(self, destination_parent, name=None): if self.is_preprint_primary: if self.target != destination_parent.target or self.provider != destination_parent.provider: raise exceptions.FileNodeIsPrimaryFile() if self.is_checked_out: raise exceptions.FileNodeCheckedOutError() - most_recent_fileversion = self.versions.select_related('region').order_by('-created').first() - if most_recent_fileversion and most_recent_fileversion.region != destination_parent.target.osfstorage_region: - most_recent_fileversion.region = destination_parent.target.osfstorage_region - most_recent_fileversion.save() + if not self.is_file: + self.update_folder_regions(destination_parent) + else: + self.update_most_recent_fileversion(destination_parent) return super(OsfStorageFileNode, self).move_under(destination_parent, name) def check_in_or_out(self, user, checkout, save=False): diff --git a/addons/osfstorage/tests/test_models.py b/addons/osfstorage/tests/test_models.py index 30f2c1946a1..0fc078c2174 100644 --- a/addons/osfstorage/tests/test_models.py +++ b/addons/osfstorage/tests/test_models.py @@ -304,6 +304,30 @@ def test_move_nested(self): assert_equal(new_project, move_to.target) assert_equal(new_project, child.target) + def test_move_nested_between_regions(self): + canada = RegionFactory() + new_component = NodeFactory(parent=self.project) + component_node_settings = new_component.get_addon('osfstorage') + component_node_settings.region = canada + component_node_settings.save() + + move_to = component_node_settings.get_root() + to_move = self.node_settings.get_root().append_folder('Aaah').append_folder('Woop') + child = to_move.append_file('There it is') + + for _ in range(2): + version = factories.FileVersionFactory(region=self.node_settings.region) + child.versions.add(version) + child.save() + + moved = to_move.move_under(move_to) + child.reload() + + assert new_component == child.target + versions = child.versions.order_by('-created') + assert versions.first().region == component_node_settings.region + assert versions.last().region == self.node_settings.region + def test_copy_rename(self): to_copy = self.node_settings.get_root().append_file('Carp') copy_to = self.node_settings.get_root().append_folder('Cloud') From 6c7e2becef0bbe04d7d4946e6e77c2ea8e6f135a Mon Sep 17 00:00:00 2001 From: erinspace Date: Tue, 6 Nov 2018 16:09:01 -0500 Subject: [PATCH 03/59] Create update_region_from_latest_version for files and folders Using the composite design pattern h/t @sloria --- addons/osfstorage/models.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/addons/osfstorage/models.py b/addons/osfstorage/models.py index d9e9ff6e99b..c929d1575da 100644 --- a/addons/osfstorage/models.py +++ b/addons/osfstorage/models.py @@ -160,18 +160,8 @@ def delete(self, user=None, parent=None, **kwargs): self._materialized_path = self.materialized_path return super(OsfStorageFileNode, self).delete(user=user, parent=parent) if self._check_delete_allowed() else None - def update_folder_regions(self, destination_parent): - for child in self.children.all().prefetch_related('versions'): - if child.is_file: - child.update_most_recent_fileversion(destination_parent) - else: - child.update_folder_regions(destination_parent) - - def update_most_recent_fileversion(self, destination_parent): - most_recent_fileversion = self.versions.select_related('region').order_by('-created').first() - if most_recent_fileversion and most_recent_fileversion.region != destination_parent.target.osfstorage_region: - most_recent_fileversion.region = destination_parent.target.osfstorage_region - most_recent_fileversion.save() + def update_region_from_latest_version(self, destination_parent): + raise NotImplementedError def move_under(self, destination_parent, name=None): if self.is_preprint_primary: @@ -179,10 +169,7 @@ def move_under(self, destination_parent, name=None): raise exceptions.FileNodeIsPrimaryFile() if self.is_checked_out: raise exceptions.FileNodeCheckedOutError() - if not self.is_file: - self.update_folder_regions(destination_parent) - else: - self.update_most_recent_fileversion(destination_parent) + self.update_region_from_latest_version(destination_parent) return super(OsfStorageFileNode, self).move_under(destination_parent, name) def check_in_or_out(self, user, checkout, save=False): @@ -300,6 +287,12 @@ def serialize(self, include_full=None, version=None): }) return ret + def update_region_from_latest_version(self, destination_parent): + most_recent_fileversion = self.versions.select_related('region').order_by('-created').first() + if most_recent_fileversion and most_recent_fileversion.region != destination_parent.target.osfstorage_region: + most_recent_fileversion.region = destination_parent.target.osfstorage_region + most_recent_fileversion.save() + def create_version(self, creator, location, metadata=None): latest_version = self.get_version() version = FileVersion(identifier=self.versions.count() + 1, creator=creator, location=location) @@ -458,6 +451,9 @@ def serialize(self, include_full=False, version=None): ret['fullPath'] = self.materialized_path return ret + def update_region_from_latest_version(self, destination_parent): + for child in self.children.all().prefetch_related('versions'): + child.update_region_from_latest_version(destination_parent) class Region(models.Model): _id = models.CharField(max_length=255, db_index=True) From f22e05b140dfa32112646c188e7a6c5d30ba097c Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 24 May 2019 11:02:14 -0400 Subject: [PATCH 04/59] Added node license creation capability to apiv2 --- api/nodes/serializers.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index c22499fe718..9c385f3e8e4 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -650,6 +650,8 @@ def create(self, validated_data): affiliated_institutions = validated_data.pop('affiliated_institutions') if 'region_id' in validated_data: region_id = validated_data.pop('region_id') + if 'license_type' in validated_data or 'license' in validated_data: + license_details = validated_data.pop('license') if 'tag_names' in validated_data: tags = validated_data.pop('tag_names') for tag in tags: @@ -699,6 +701,18 @@ def create(self, validated_data): node.subjects.add(parent.subjects.all()) node.save() + if license_details: + node.set_node_license( + { + 'id': license_details.get('id') if license_details.get('id') else 'NONE', + 'year': license_details.get('year'), + 'copyrightHolders': license_details.get('copyrightHolders') or license_details.get('copyright_holders', []), + }, + auth=get_user_auth(request), + save=True, + ) + node.save() + if not region_id: region_id = self.context.get('region_id') if region_id: From 2fb3435031fcf7c766ff1a00030de5572822e704 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 24 May 2019 11:48:49 -0400 Subject: [PATCH 05/59] Fix for build error. 5/24/2019 --- api/nodes/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index 9c385f3e8e4..12fd7623e0c 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -646,6 +646,7 @@ def create(self, validated_data): tag_instances = [] affiliated_institutions = None region_id = None + license_details = None if 'affiliated_institutions' in validated_data: affiliated_institutions = validated_data.pop('affiliated_institutions') if 'region_id' in validated_data: @@ -711,7 +712,6 @@ def create(self, validated_data): auth=get_user_auth(request), save=True, ) - node.save() if not region_id: region_id = self.context.get('region_id') From 13d7103ad57c72cc83e3e5ee8198cd6befaed2af Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Tue, 28 May 2019 15:17:36 -0400 Subject: [PATCH 06/59] Added tests for node license create 5/28/2019` --- api_tests/nodes/views/test_node_detail.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/api_tests/nodes/views/test_node_detail.py b/api_tests/nodes/views/test_node_detail.py index b2686c96de4..f0247e0df2d 100644 --- a/api_tests/nodes/views/test_node_detail.py +++ b/api_tests/nodes/views/test_node_detail.py @@ -1865,6 +1865,27 @@ def test_component_return_parent_license_if_no_license( API_BASE, node_license._id) assert expected_license_url in actual_license_url + def test_node_license_on_create( + self, app, user, copyright_holders, year): + url = '/{}nodes/'.format(API_BASE) + res = app.post_json_api( + url, { + 'data': { + 'type': 'nodes', + 'attributes': { + 'title': 'new title', + 'category': 'project', + 'tags': ['foo', 'bar'], + 'node_license': { + 'copyright_holders': copyright_holders, + 'year': year, + }, + } + } + }, auth=user.auth) + assert res.json['data']['attributes']['node_license']['year'] == year + assert res.json['data']['attributes']['node_license']['copyright_holders'] == copyright_holders + @pytest.mark.django_db class TestNodeUpdateLicense: From 59cdbaac666896c823f97be80c24bce839458cc5 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 31 May 2019 09:49:37 -0400 Subject: [PATCH 07/59] Added error handling for attempts to link a node to itself, and link a parent node to a child node. Also added tests for both behaviors. 5/31/2019 --- api/base/serializers.py | 9 +++-- api/nodes/serializers.py | 4 +-- .../nodes/views/test_node_linked_nodes.py | 34 ++++++++++++++++++- osf/models/mixins.py | 10 +++++- 4 files changed, 51 insertions(+), 6 deletions(-) diff --git a/api/base/serializers.py b/api/base/serializers.py index 3e0a06e2f88..f2e4fb4db53 100644 --- a/api/base/serializers.py +++ b/api/base/serializers.py @@ -1689,8 +1689,13 @@ def create(self, validated_data): raise api_exceptions.RelationshipPostMakesNoChanges for node in add: - collection.add_pointer(node, auth) - + try: + collection.add_pointer(node, auth) + except ValueError as e: + raise api_exceptions.InvalidModelValueError( + source={'pointer': '/data/relationships/node_links/data/id'}, + detail=str(e), + ) return self.make_instance_obj(collection) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index c22499fe718..15ae561b0e3 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -1211,10 +1211,10 @@ def create(self, validated_data): try: pointer = node.add_pointer(pointer_node, auth, save=True) return pointer - except ValueError: + except ValueError as e: raise InvalidModelValueError( source={'pointer': '/data/relationships/node_links/data/id'}, - detail='Target Node \'{}\' already pointed to by \'{}\'.'.format(target_node_id, node._id), + detail=str(e), ) def update(self, instance, validated_data): diff --git a/api_tests/nodes/views/test_node_linked_nodes.py b/api_tests/nodes/views/test_node_linked_nodes.py index 55211c5782f..cc58d7452c6 100644 --- a/api_tests/nodes/views/test_node_linked_nodes.py +++ b/api_tests/nodes/views/test_node_linked_nodes.py @@ -5,6 +5,7 @@ from osf_tests.factories import ( NodeFactory, AuthUserFactory, + NodeRelationFactory, ) from website.project.signals import contributor_removed from api_tests.utils import disconnected_from_listeners @@ -332,7 +333,7 @@ def test_delete_invalid_payload( def test_node_errors( self, app, user, node_private, node_contrib, node_public, node_other, make_payload, - url_private, url_public): + url_private, url_public, node_linking_private): # test_node_doesnt_exist res = app.post_json_api( @@ -444,6 +445,37 @@ def test_node_errors( assert res.status_code == 403 + # test_node_child_cannot_be_linked + node_child = NodeFactory(creator=user) + node_parent = NodeFactory(creator=user) + node_parent_child = NodeRelationFactory(child=node_child, parent=node_parent) + url = '/{}nodes/{}/relationships/linked_nodes/'.format( + API_BASE, node_parent_child.parent._id + ) + res = app.post_json_api( + url, { + 'data': [{ + 'type': 'linked_nodes', + 'id': node_parent_child.child._id + }] + }, + auth=user.auth, expect_errors=True + ) + + assert res.status_code == 400 + + # test_linking_node_to_itself + node_self = NodeFactory(creator=user) + url = '/{}nodes/{}/relationships/linked_nodes/'.format( + API_BASE, node_self._id + ) + res = app.post_json_api( + url_private, make_payload([node_linking_private._id]), + auth=user.auth, expect_errors=True + ) + + assert res.status_code == 400 + def test_node_links_and_relationship_represent_same_nodes( self, app, user, node_admin, node_contrib, node_linking_private, url_private): node_linking_private.add_pointer(node_admin, auth=Auth(user)) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index f19d76aeb9f..425bec05229 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -329,9 +329,17 @@ def add_node_link(self, node, auth, save=True): # Fail if node already in nodes / pointers. Note: cast node and node # to primary keys to test for conflicts with both nodes and pointers # contained in `self.nodes`. + if (self._id == node._id): + raise ValueError( + 'Cannot link node \'{}\' to itself.'.format(self._id) + ) if NodeRelation.objects.filter(parent=self, child=node, is_node_link=True).exists(): raise ValueError( - 'Link to node {0} already exists'.format(node._id) + 'Target Node \'{}\' already pointed to by \'{}\'.'.format(self._id, node._id) + ) + if NodeRelation.objects.filter(parent=self, child=node, is_node_link=False).exists(): + raise ValueError( + 'Target Node \'{}\' is already a child of \'{}\'.'.format(self._id, node._id) ) if self.is_registration: From daad865f6b5d0d4b805ecaa180c722e464cebec6 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 31 May 2019 10:23:07 -0400 Subject: [PATCH 08/59] Fixed order of format args --- osf/models/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 425bec05229..21a1e69022a 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -335,7 +335,7 @@ def add_node_link(self, node, auth, save=True): ) if NodeRelation.objects.filter(parent=self, child=node, is_node_link=True).exists(): raise ValueError( - 'Target Node \'{}\' already pointed to by \'{}\'.'.format(self._id, node._id) + 'Target Node \'{}\' already pointed to by \'{}\'.'.format(node._id, self._id) ) if NodeRelation.objects.filter(parent=self, child=node, is_node_link=False).exists(): raise ValueError( From 2cf368195e14c8f1026dd489f92b4fe2b9cc1b5d Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 31 May 2019 10:29:18 -0400 Subject: [PATCH 09/59] Fixed order of format args for error msg --- osf/models/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index 21a1e69022a..aa0938c6f52 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -339,7 +339,7 @@ def add_node_link(self, node, auth, save=True): ) if NodeRelation.objects.filter(parent=self, child=node, is_node_link=False).exists(): raise ValueError( - 'Target Node \'{}\' is already a child of \'{}\'.'.format(self._id, node._id) + 'Target Node \'{}\' is already a child of \'{}\'.'.format(node._id, self._id) ) if self.is_registration: From e24c14a37e1e763df7f6efd8e2553fd2271e4436 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 31 May 2019 13:42:44 -0400 Subject: [PATCH 10/59] Modified tests to reflect new responses. --- api_tests/nodes/views/test_node_links_list.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/api_tests/nodes/views/test_node_links_list.py b/api_tests/nodes/views/test_node_links_list.py index 22a689048ef..456fad3a3b3 100644 --- a/api_tests/nodes/views/test_node_links_list.py +++ b/api_tests/nodes/views/test_node_links_list.py @@ -485,11 +485,8 @@ def test_create_node_pointer_to_itself( public_url, point_to_itself_payload, auth=user.auth) - res_json = res.json['data'] - assert res.status_code == 201 + assert res.status_code == 400 assert res.content_type == 'application/vnd.api+json' - embedded = res_json['embeds']['target_node']['data']['id'] - assert embedded == public_project._id def test_create_node_pointer_errors( self, app, user, user_two, public_project, @@ -972,11 +969,8 @@ def test_bulk_creates_node_pointer_to_itself( res = app.post_json_api( public_url, point_to_itself_payload, auth=user.auth, bulk=True) - assert res.status_code == 201 + assert res.status_code == 400 assert res.content_type == 'application/vnd.api+json' - res_json = res.json['data'] - embedded = res_json[0]['embeds']['target_node']['data']['id'] - assert embedded == public_project._id def test_bulk_creates_node_pointer_already_connected( self, app, user, public_project, From 377adb28ed4ee981296dc828b7dc80ed3ce68791 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Fri, 31 May 2019 14:20:47 -0400 Subject: [PATCH 11/59] Fixing tests for new response codes --- api_tests/nodes/views/test_node_links_list.py | 46 +++++++++---------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/api_tests/nodes/views/test_node_links_list.py b/api_tests/nodes/views/test_node_links_list.py index 456fad3a3b3..3b6b0890954 100644 --- a/api_tests/nodes/views/test_node_links_list.py +++ b/api_tests/nodes/views/test_node_links_list.py @@ -479,14 +479,12 @@ def test_create_fake_node_pointing_to_contributing_node( def test_create_node_pointer_to_itself( self, app, user, public_project, public_url, make_payload): - with assert_latest_log(NodeLog.POINTER_CREATED, public_project): - point_to_itself_payload = make_payload(id=public_project._id) - res = app.post_json_api( - public_url, - point_to_itself_payload, - auth=user.auth) - assert res.status_code == 400 - assert res.content_type == 'application/vnd.api+json' + point_to_itself_payload = make_payload(id=public_project._id) + res = app.post_json_api( + public_url, + point_to_itself_payload, + auth=user.auth, expect_errors=True) + assert res.status_code == 400 def test_create_node_pointer_errors( self, app, user, user_two, public_project, @@ -951,26 +949,24 @@ def test_bulk_creates_node_pointers_contributing_node_to_non_contributing_node( def test_bulk_creates_node_pointer_to_itself( self, app, user, public_project, public_url): - with assert_latest_log(NodeLog.POINTER_CREATED, public_project): - point_to_itself_payload = { - 'data': [{ - 'type': 'node_links', - 'relationships': { - 'nodes': { - 'data': { - 'type': 'nodes', - 'id': public_project._id - } + point_to_itself_payload = { + 'data': [{ + 'type': 'node_links', + 'relationships': { + 'nodes': { + 'data': { + 'type': 'nodes', + 'id': public_project._id } } - }] - } + } + }] + } - res = app.post_json_api( - public_url, point_to_itself_payload, - auth=user.auth, bulk=True) - assert res.status_code == 400 - assert res.content_type == 'application/vnd.api+json' + res = app.post_json_api( + public_url, point_to_itself_payload, + auth=user.auth, bulk=True, expect_errors=True) + assert res.status_code == 400 def test_bulk_creates_node_pointer_already_connected( self, app, user, public_project, From 6f8ba53277089022b343062c309aeb77a3da5dcc Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Tue, 4 Jun 2019 09:47:30 -0400 Subject: [PATCH 12/59] Modified node license creation functionality to handle corner cases. 6/4/2019 --- api/nodes/serializers.py | 42 +++++--- api_tests/nodes/views/test_node_list.py | 135 ++++++++++++++++++++++++ 2 files changed, 163 insertions(+), 14 deletions(-) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index 12fd7623e0c..76121fdde66 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -141,8 +141,8 @@ def to_internal_value(self, data): class NodeLicenseSerializer(BaseAPISerializer): - copyright_holders = ser.ListField(allow_empty=True) - year = ser.CharField(allow_blank=True) + copyright_holders = ser.ListField(allow_empty=True, required=False) + year = ser.CharField(allow_blank=True, required=False) class Meta: type_ = 'node_licenses' @@ -198,8 +198,12 @@ class Meta: type_ = 'styled-citations' def get_license_details(node, validated_data): - license = node.license if isinstance(node, Preprint) else node.node_license - + if node: + license = node.license if isinstance(node, Preprint) else node.node_license + else: + license = None + if ('license_type' not in validated_data or (license and license.node_license.license_id)): + raise exceptions.ValidationError(detail='License ID must be provided for a Node License.') license_id = license.node_license.license_id if license else None license_year = license.year if license else None license_holders = license.copyright_holders if license else [] @@ -652,7 +656,14 @@ def create(self, validated_data): if 'region_id' in validated_data: region_id = validated_data.pop('region_id') if 'license_type' in validated_data or 'license' in validated_data: - license_details = validated_data.pop('license') + try: + license_details = get_license_details(None, validated_data) + except ValidationError as e: + raise InvalidModelValueError(detail=e.messages[0]) + if validated_data.get('license'): + validated_data.pop('license') + if validated_data.get('license_type'): + validated_data.pop('license_type') if 'tag_names' in validated_data: tags = validated_data.pop('tag_names') for tag in tags: @@ -703,15 +714,18 @@ def create(self, validated_data): node.save() if license_details: - node.set_node_license( - { - 'id': license_details.get('id') if license_details.get('id') else 'NONE', - 'year': license_details.get('year'), - 'copyrightHolders': license_details.get('copyrightHolders') or license_details.get('copyright_holders', []), - }, - auth=get_user_auth(request), - save=True, - ) + try: + node.set_node_license( + { + 'id': license_details.get('id') if license_details.get('id') else 'NONE', + 'year': license_details.get('year'), + 'copyrightHolders': license_details.get('copyrightHolders') or license_details.get('copyright_holders', []), + }, + auth=get_user_auth(request), + save=True, + ) + except ValidationError as e: + raise InvalidModelValueError(detail=e.message) if not region_id: region_id = self.context.get('region_id') diff --git a/api_tests/nodes/views/test_node_list.py b/api_tests/nodes/views/test_node_list.py index a98eca7c5ca..67b6a68a89a 100644 --- a/api_tests/nodes/views/test_node_list.py +++ b/api_tests/nodes/views/test_node_list.py @@ -5,6 +5,7 @@ from api_tests.nodes.filters.test_filters import NodesListFilteringMixin, NodesListDateFilteringMixin from framework.auth.core import Auth from osf.models import AbstractNode, Node, NodeLog +from osf.models.licenses import NodeLicense from osf.utils.sanitize import strip_html from osf.utils import permissions from osf_tests.factories import ( @@ -1722,6 +1723,140 @@ def test_create_project_errors( assert res.status_code == 400 assert res.json['errors'][0]['detail'] == 'Title cannot exceed 512 characters.' +@pytest.mark.django_db +class TestNodeLicenseOnCreate: + + @pytest.fixture() + def user(self): + return AuthUserFactory() + + @pytest.fixture() + def url(self): + return '/{}nodes/'.format(API_BASE) + + @pytest.fixture() + def license_name(self): + return 'MIT License' + + @pytest.fixture() + def node_license(self, license_name): + return NodeLicense.objects.filter(name=license_name).first() + + @pytest.fixture() + def make_payload(self): + def payload( + license_id=None, license_year=None, copyright_holders=None): + attributes = {} + + if license_year and copyright_holders: + attributes = { + 'title': 'new title', + 'category': 'project', + 'tags': ['foo', 'bar'], + 'node_license': { + 'copyright_holders': copyright_holders, + 'year': license_year, + } + } + elif license_year: + attributes = { + 'title': 'new title', + 'category': 'project', + 'tags': ['foo', 'bar'], + 'node_license': { + 'year': license_year, + } + } + elif copyright_holders: + attributes = { + 'title': 'new title', + 'category': 'project', + 'tags': ['foo', 'bar'], + 'node_license': { + 'copyright_holders': copyright_holders + } + } + + return { + 'data': { + 'type': 'nodes', + 'attributes': attributes, + 'relationships': { + 'license': { + 'data': { + 'type': 'licenses', + 'id': license_id + } + } + } + } + } if license_id else { + 'data': { + 'type': 'nodes', + 'attributes': attributes + } + } + return payload + + def test_node_license_on_create( + self, app, user, url, node_license, make_payload): + data = make_payload( + copyright_holders=['Haagen', 'Dazs'], + license_year='2200', + license_id=node_license._id + ) + res = app.post_json_api( + url, data, auth=user.auth) + assert res.json['data']['attributes']['node_license']['year'] == '2200' + assert res.json['data']['attributes']['node_license']['copyright_holders'] == ['Haagen', 'Dazs'] + assert res.json['data']['relationships']['license']['data']['id'] == node_license._id + + def test_create_node_license_errors( + self, app, url, user, node_license, make_payload): + + # test_creating_a_node_license_without_a_license_id + data = make_payload( + license_year='2200', + copyright_holders=['Ben', 'Jerry'] + ) + res = app.post_json_api( + url, data, auth=user.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'License ID must be provided for a Node License.' + + # test_creating_a_node_license_without_a_copyright_holder + data = make_payload( + license_year='2200', + license_id=node_license._id + ) + res = app.post_json_api( + url, data, + auth=user.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'copyrightHolders must be specified for this license' + + # test_creating_a_node_license_without_a_year + data = make_payload( + copyright_holders=['Baskin', 'Robbins'], + license_id=node_license._id + ) + res = app.post_json_api( + url, data, + auth=user.auth, expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'year must be specified for this license' + + # test_creating_a_node_license_with_an_invalid_ID + data = make_payload( + license_year='2200', + license_id='invalid id', + copyright_holders=['Ben', 'Jerry'] + ) + res = app.post_json_api( + url, data, + auth=user.auth, expect_errors=True) + assert res.status_code == 404 + assert res.json['errors'][0]['detail'] == 'Unable to find specified license.' @pytest.mark.django_db class TestNodeBulkCreate: From a916f4a90545e5a01ba850d4180fc31521acafb1 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Tue, 4 Jun 2019 09:54:12 -0400 Subject: [PATCH 13/59] Removed test from test_node_license --- api_tests/nodes/views/test_node_detail.py | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/api_tests/nodes/views/test_node_detail.py b/api_tests/nodes/views/test_node_detail.py index f0247e0df2d..b2686c96de4 100644 --- a/api_tests/nodes/views/test_node_detail.py +++ b/api_tests/nodes/views/test_node_detail.py @@ -1865,27 +1865,6 @@ def test_component_return_parent_license_if_no_license( API_BASE, node_license._id) assert expected_license_url in actual_license_url - def test_node_license_on_create( - self, app, user, copyright_holders, year): - url = '/{}nodes/'.format(API_BASE) - res = app.post_json_api( - url, { - 'data': { - 'type': 'nodes', - 'attributes': { - 'title': 'new title', - 'category': 'project', - 'tags': ['foo', 'bar'], - 'node_license': { - 'copyright_holders': copyright_holders, - 'year': year, - }, - } - } - }, auth=user.auth) - assert res.json['data']['attributes']['node_license']['year'] == year - assert res.json['data']['attributes']['node_license']['copyright_holders'] == copyright_holders - @pytest.mark.django_db class TestNodeUpdateLicense: From 1d01d8c964b25bda361f5df4c2d8206fbddff84c Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Tue, 4 Jun 2019 11:07:24 -0400 Subject: [PATCH 14/59] Fixing boolean logic for updating existing licenses --- api/nodes/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index 76121fdde66..99ada0623b7 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -202,7 +202,7 @@ def get_license_details(node, validated_data): license = node.license if isinstance(node, Preprint) else node.node_license else: license = None - if ('license_type' not in validated_data or (license and license.node_license.license_id)): + if ('license_type' not in validated_data and not (license and license.node_license.license_id)): raise exceptions.ValidationError(detail='License ID must be provided for a Node License.') license_id = license.node_license.license_id if license else None license_year = license.year if license else None From 54a61d9a321e70cc271c88d21dc040576d397344 Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Wed, 5 Jun 2019 09:34:43 -0400 Subject: [PATCH 15/59] Made minor modifications. Removed detail on JSON response for error, and optimized Node Relation query. --- api/base/serializers.py | 1 - osf/models/mixins.py | 8 +++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/api/base/serializers.py b/api/base/serializers.py index f2e4fb4db53..09aa38b6984 100644 --- a/api/base/serializers.py +++ b/api/base/serializers.py @@ -1693,7 +1693,6 @@ def create(self, validated_data): collection.add_pointer(node, auth) except ValueError as e: raise api_exceptions.InvalidModelValueError( - source={'pointer': '/data/relationships/node_links/data/id'}, detail=str(e), ) return self.make_instance_obj(collection) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index aa0938c6f52..ce0fbf15cd5 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -326,18 +326,16 @@ def add_node_link(self, node, auth, save=True): :param bool save: Save changes :return: Created pointer """ - # Fail if node already in nodes / pointers. Note: cast node and node - # to primary keys to test for conflicts with both nodes and pointers - # contained in `self.nodes`. if (self._id == node._id): raise ValueError( 'Cannot link node \'{}\' to itself.'.format(self._id) ) - if NodeRelation.objects.filter(parent=self, child=node, is_node_link=True).exists(): + existant_relation = NodeRelation.objects.filter(parent=self, child=node).first() + if existant_relation and existant_relation.is_node_link: raise ValueError( 'Target Node \'{}\' already pointed to by \'{}\'.'.format(node._id, self._id) ) - if NodeRelation.objects.filter(parent=self, child=node, is_node_link=False).exists(): + elif existant_relation and not existant_relation.is_node_link: raise ValueError( 'Target Node \'{}\' is already a child of \'{}\'.'.format(node._id, self._id) ) From 3f9384bb4df28f68423156d207838ec0dda92dba Mon Sep 17 00:00:00 2001 From: corbinSanders Date: Wed, 12 Jun 2019 13:13:28 -0400 Subject: [PATCH 16/59] Modified serializers.py to be compatible with python 3. Created test for updating nodes without a license. --- api/nodes/serializers.py | 10 ++++------ api_tests/nodes/views/test_node_detail.py | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index 99ada0623b7..5facd48f29c 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -659,11 +659,9 @@ def create(self, validated_data): try: license_details = get_license_details(None, validated_data) except ValidationError as e: - raise InvalidModelValueError(detail=e.messages[0]) - if validated_data.get('license'): - validated_data.pop('license') - if validated_data.get('license_type'): - validated_data.pop('license_type') + raise InvalidModelValueError(detail=str(e.messages[0])) + validated_data.pop('license', None) + validated_data.pop('license_type', None) if 'tag_names' in validated_data: tags = validated_data.pop('tag_names') for tag in tags: @@ -725,7 +723,7 @@ def create(self, validated_data): save=True, ) except ValidationError as e: - raise InvalidModelValueError(detail=e.message) + raise InvalidModelValueError(detail=str(e.message)) if not region_id: region_id = self.context.get('region_id') diff --git a/api_tests/nodes/views/test_node_detail.py b/api_tests/nodes/views/test_node_detail.py index b2686c96de4..b659d633f9c 100644 --- a/api_tests/nodes/views/test_node_detail.py +++ b/api_tests/nodes/views/test_node_detail.py @@ -2223,6 +2223,21 @@ def test_update_node_license_without_required_year_in_payload( assert res.status_code == 400 assert res.json['errors'][0]['detail'] == 'year must be specified for this license' + def test_update_node_license_without_license_id( + self, node, make_payload, make_request, url_node, user_admin_contrib): + data = make_payload( + node_id=node._id, + license_year='2015', + copyright_holders=['Ben, Jerry'] + ) + + res = make_request( + url_node, data, + auth=user_admin_contrib.auth, + expect_errors=True) + assert res.status_code == 400 + assert res.json['errors'][0]['detail'] == 'License ID must be provided for a Node License.' + def test_update_node_license_without_required_copyright_holders_in_payload_( self, user_admin_contrib, node, make_payload, make_request, license_no, url_node): data = make_payload( From 8bb8ca452bb9250be9b6eb7217df0e24ca515f3e Mon Sep 17 00:00:00 2001 From: UdayVarkhedkar Date: Wed, 12 Jun 2019 15:08:45 -0400 Subject: [PATCH 17/59] Updates Password Reset Logic --- website/templates/public/resetpassword.mako | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/website/templates/public/resetpassword.mako b/website/templates/public/resetpassword.mako index f417ea3495a..39d5edc96bf 100644 --- a/website/templates/public/resetpassword.mako +++ b/website/templates/public/resetpassword.mako @@ -96,6 +96,12 @@ +<%def name="javascript()"> + + + <%def name="javascript_bottom()"> + + <%def name="javascript_bottom()">