From 9b4bbecdb6a035546177582ec4d0a6e35831602d Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 16 Oct 2023 21:26:41 +0800 Subject: [PATCH] fix draft registrations affiliated institution default value for no-project registrations and clean-up code --- api/nodes/serializers.py | 10 ---- .../views/test_draft_registration_list.py | 58 +++++++++++++++---- osf/models/draft_node.py | 2 +- osf/models/mixins.py | 45 ++++++++------ osf/models/node.py | 2 +- osf/models/registrations.py | 24 +++++--- 6 files changed, 95 insertions(+), 46 deletions(-) diff --git a/api/nodes/serializers.py b/api/nodes/serializers.py index aaff14ac1b9..918f156ce3d 100644 --- a/api/nodes/serializers.py +++ b/api/nodes/serializers.py @@ -1557,12 +1557,6 @@ class DraftRegistrationLegacySerializer(JSONAPISerializer): 'html': 'get_absolute_url', }) - affiliate_user_institutions = ser.BooleanField( - required=False, - default=True, - help_text='Specify whether user institution affiliations should be copied over to the draft registration.', - ) - def get_absolute_url(self, obj): return obj.absolute_url @@ -1603,7 +1597,6 @@ def create(self, validated_data): registration_responses = validated_data.pop('registration_responses', None) schema = validated_data.pop('registration_schema') provider = validated_data.pop('provider', None) - affiliate_user_institutions = validated_data.pop('affiliate_user_institutions', True) self.enforce_metadata_or_registration_responses(metadata, registration_responses) @@ -1618,9 +1611,6 @@ def create(self, validated_data): if registration_responses: self.update_registration_responses(draft, registration_responses) - if affiliate_user_institutions and draft.branched_from_type == DraftNode: - draft.affiliated_institutions.set(draft.creator.affiliated_institutions.all()) - return draft class Meta: diff --git a/api_tests/draft_registrations/views/test_draft_registration_list.py b/api_tests/draft_registrations/views/test_draft_registration_list.py index 509e8daacc2..f7d6a48ad35 100644 --- a/api_tests/draft_registrations/views/test_draft_registration_list.py +++ b/api_tests/draft_registrations/views/test_draft_registration_list.py @@ -9,7 +9,7 @@ from api.base.settings.defaults import API_BASE from osf.migrations import ensure_invisible_and_inactive_schema -from osf.models import DraftRegistration, NodeLicense, RegistrationProvider, Institution +from osf.models import DraftRegistration, NodeLicense, RegistrationProvider from osf_tests.factories import ( RegistrationFactory, CollectionFactory, @@ -260,27 +260,65 @@ def test_create_project_based_draft_does_not_email_initiator( assert not mock_send_mail.called - def test_affiliated_institutions_are_copied_from_user( - self, app, user, url_draft_registrations, payload): + def test_affiliated_institutions_are_copied_from_node_no_institutions(self, app, user, url_draft_registrations, payload): + """ + Draft registrations that are based on projects get those project's user institutional affiliation, + those "no-project" registrations inherit the user's institutional affiliation. + + This tests a scenario where a user bases a registration on a node without affiliations, and so the + draft registration has no institutional affiliation from the user or the node. + """ project = ProjectFactory(is_public=True, creator=user) - InstitutionFactory() payload['data']['relationships']['branched_from']['data']['id'] = project._id + res = app.post_json_api( - url_draft_registrations, payload, - auth=user.auth, expect_errors=True) + url_draft_registrations, + payload, + auth=user.auth, + ) assert res.status_code == 201 draft_registration = DraftRegistration.load(res.json['data']['id']) assert not draft_registration.affiliated_institutions.exists() + def test_affiliated_institutions_are_copied_from_node(self, app, user, url_draft_registrations, payload): + """ + Draft registrations that are based on projects get those project's user institutional affiliation, + those "no-project" registrations inherit the user's institutional affiliation. + + This tests a scenario where a user bases their registration on a project that has a current institutional + affiliation which is copied over to the draft registrations. + """ + institution = InstitutionFactory() + project = ProjectFactory(is_public=True, creator=user) + project.affiliated_institutions.add(institution) payload['data']['relationships']['branched_from']['data']['id'] = project._id - user.add_multiple_institutions_non_sso(Institution.objects.filter(id__lt=3)) res = app.post_json_api( - url_draft_registrations, payload, - auth=user.auth, expect_errors=True) + url_draft_registrations, + payload, + auth=user.auth, + ) + assert res.status_code == 201 + draft_registration = DraftRegistration.load(res.json['data']['id']) + assert list(draft_registration.affiliated_institutions.all()) == list(project.affiliated_institutions.all()) + + def test_affiliated_institutions_are_copied_from_user(self, app, user, url_draft_registrations, payload): + """ + Draft registrations that are based on projects get those project's user institutional affiliation, + those "no-project" registrations inherit the user's institutional affiliation. + """ + institution = InstitutionFactory() + user.add_or_update_affiliated_institution(institution) + + del payload['data']['relationships']['branched_from'] + res = app.post_json_api( + url_draft_registrations, + payload, + auth=user.auth, + ) assert res.status_code == 201 draft_registration = DraftRegistration.load(res.json['data']['id']) - assert not draft_registration.affiliated_institutions.all() == user.get_affiliated_institutions() + assert list(draft_registration.affiliated_institutions.all()) == list(user.get_affiliated_institutions()) class TestDraftRegistrationCreateWithoutNode(TestDraftRegistrationCreate): diff --git a/osf/models/draft_node.py b/osf/models/draft_node.py index a64b216c040..fec0bfc346e 100644 --- a/osf/models/draft_node.py +++ b/osf/models/draft_node.py @@ -66,7 +66,7 @@ def register_node(self, schema, auth, draft_registration, parent=None, child_ids """ self.convert_draft_node_to_node(auth) # Copies editable fields from the DraftRegistration back to the Node - self.copy_editable_fields(draft_registration, auth=auth, save=True) + self.copy_editable_fields(draft_registration, save=True) # Calls super on Node, since self is no longer a DraftNode return super(Node, self).register_node(schema, auth, draft_registration, parent, child_ids, provider) diff --git a/osf/models/mixins.py b/osf/models/mixins.py index daab45c8a53..1b7332ca115 100644 --- a/osf/models/mixins.py +++ b/osf/models/mixins.py @@ -2261,33 +2261,44 @@ def stage_m2m_values(self, fieldname, resource, alternative_resource=None): else: return [] - def copy_editable_fields(self, resource, auth=None, alternative_resource=None, include_contributors=True, save=True): + def copy_editable_fields( + self, resource, alternative_resource=None, include_contributors=True, save=True, excluded_attributes=None + ): """ - Copy various editable fields from the 'resource' object to the current object. - Includes, title, description, category, contributors, node_license, tags, subjects, and affiliated_institutions - The field on the resource will always supersede the field on the alternative_resource. For example, - copying fields from the draft_registration to the registration. resource will be a DraftRegistration object, - but the alternative_resource will be a Node. DraftRegistration fields will trump Node fields. - TODO, add optional logging parameter + Copy editable fields from 'resource' to the current object. + + :param Object resource: Primary resource to copy attributes from + :param Object alternative_resource: Backup resource for copying attributes + :param bool include_contributors: Whether to also copy contributors + :param bool save: Whether to save the changes immediately + :param List excluded_attributes: List of attributes to exclude from copying """ - self.set_editable_attribute('title', resource, alternative_resource) - self.set_editable_attribute('description', resource, alternative_resource) - self.set_editable_attribute('category', resource, alternative_resource) - self.set_editable_attribute('node_license', resource, alternative_resource) + if not excluded_attributes: + excluded_attributes = [] + + self._copy_basic_attributes(resource, alternative_resource, excluded_attributes) if include_contributors: - # Contributors will always come from "resource", as contributor constraints - # will require contributors to be present on the resource self.copy_contributors_from(resource) - # Copy unclaimed records for unregistered users self.copy_unclaimed_records(resource) - self.tags.add(*self.stage_m2m_values('all_tags', resource, alternative_resource)) - self.subjects.add(*self.stage_m2m_values('subjects', resource, alternative_resource)) - self.affiliated_institutions.add(*self.stage_m2m_values('affiliated_institutions', resource, alternative_resource)) + self._copy_m2m_fields(resource, alternative_resource, excluded_attributes) if save: self.save() + def _copy_basic_attributes(self, resource, alternative_resource, excluded_attributes): + # Copy basic attributes such as title, description, category, and node_license + for attribute in ['title', 'description', 'category', 'node_license']: + if attribute not in excluded_attributes: + self.set_editable_attribute(attribute, resource, alternative_resource) + + def _copy_m2m_fields(self, resource, alternative_resource, excluded_attributes): + # Copy m2m fields like tags, subjects, and affiliated_institutions + m2m_fields = ['tags', 'subjects', 'affiliated_institutions'] + for field in m2m_fields: + if field not in excluded_attributes: + getattr(self, field).add(*self.stage_m2m_values(field, resource, alternative_resource)) + class Meta: abstract = True diff --git a/osf/models/node.py b/osf/models/node.py index 2266c20d06b..0869e019ff2 100644 --- a/osf/models/node.py +++ b/osf/models/node.py @@ -1488,7 +1488,7 @@ def register_node(self, schema, auth, draft_registration, parent=None, child_ids resource = self alternative_resource = None - registered.copy_editable_fields(resource, auth=auth, alternative_resource=alternative_resource) + registered.copy_editable_fields(resource, alternative_resource=alternative_resource) registered.copy_registration_responses_into_schema_response(draft_registration) if settings.ENABLE_ARCHIVER: diff --git a/osf/models/registrations.py b/osf/models/registrations.py index acf50e6d385..aba81cf40f1 100644 --- a/osf/models/registrations.py +++ b/osf/models/registrations.py @@ -1261,25 +1261,35 @@ def create_from_node(cls, user, schema, node=None, data=None, provider=None): else: provider.validate_schema(schema) - if not node: - # If no node provided, a DraftNode is created for you - node = DraftNode.objects.create(creator=user, title='Untitled') + excluded_attributes = [] + + if node: + branched_from = node + else: + branched_from = DraftNode.objects.create(creator=user, title='Untitled') + excluded_attributes = ['affiliated_institutions'] - if not (isinstance(node, Node) or isinstance(node, DraftNode)): + if not isinstance(branched_from, (Node, DraftNode)): raise DraftRegistrationStateError() draft = cls( initiator=user, - branched_from=node, + branched_from=branched_from, registration_schema=schema, registration_metadata=data or {}, provider=provider, ) draft.save() - draft.copy_editable_fields(node, Auth(user), save=True) draft.update(data, auth=Auth(user)) + draft.copy_editable_fields( + branched_from, + Auth(user), + save=True, + excluded_attributes=excluded_attributes + ) - if node.type == 'osf.draftnode': + if not node: + draft.affiliated_institutions.add(*draft.creator.get_affiliated_institutions()) initiator_permissions = draft.contributor_set.get(user=user).permission signals.contributor_added.send( draft,