From b4958de33905d6a327bed44f14a0e67d48a4a004 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Thu, 6 Nov 2025 14:09:07 +0200 Subject: [PATCH 1/4] add include_children to delete contributors --- api/nodes/views.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/api/nodes/views.py b/api/nodes/views.py index 87758f0a75f..6c60167bc46 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -549,13 +549,20 @@ def perform_destroy(self, instance): auth = get_user_auth(self.request) if node.visible_contributors.count() == 1 and instance.visible: raise ValidationError('Must have at least one visible contributor') - removed = node.remove_contributor(instance, auth) - if not removed: - raise ValidationError('Must have at least one registered admin contributor') - propagate = self.request.query_params.get('propagate_to_children') == 'true' - if propagate: - for child_node in node.get_nodes(_contributors__in=[instance.user]): - child_node.remove_contributor(instance, auth) + + include_children = is_truthy(self.request.query_params.get('include_children', False)) + + if include_children: + hierarchy = Node.objects.get_children(node, active=True, include_root=True) + targets = hierarchy.filter(contributor_set__user=instance.user) + for descendant in targets: + removed = descendant.remove_contributor(instance, auth) + if not removed: + raise ValidationError('Children must have at least one registered admin contributor') + else: + removed = node.remove_contributor(instance, auth) + if not removed: + raise ValidationError('Must have at least one registered admin contributor') class NodeImplicitContributorsList(JSONAPIBaseView, generics.ListAPIView, ListFilterMixin, NodeMixin): From f7aeb3b3e52376b59197de1027f07748c6235efe Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Thu, 6 Nov 2025 17:57:17 +0200 Subject: [PATCH 2/4] atomic contributor remove; add tests --- api/nodes/views.py | 22 +++++++--- .../views/test_node_contributors_detail.py | 43 +++++++++++++++++++ 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/api/nodes/views.py b/api/nodes/views.py index 6c60167bc46..3206a66533b 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -9,7 +9,8 @@ from osf import features from packaging.version import Version from django.apps import apps -from django.db.models import F, Max, Q, Subquery +from django.db.models import F, Max, Q, Subquery, Exists, OuterRef +from django.db import transaction from django.utils import timezone from django.contrib.contenttypes.models import ContentType from rest_framework import generics, permissions as drf_permissions, exceptions @@ -154,6 +155,7 @@ Folder, CedarMetadataRecord, Preprint, Collection, + Contributor, ) from addons.osfstorage.models import Region from osf.utils.permissions import ADMIN, WRITE_NODE @@ -554,11 +556,19 @@ def perform_destroy(self, instance): if include_children: hierarchy = Node.objects.get_children(node, active=True, include_root=True) - targets = hierarchy.filter(contributor_set__user=instance.user) - for descendant in targets: - removed = descendant.remove_contributor(instance, auth) - if not removed: - raise ValidationError('Children must have at least one registered admin contributor') + targets = hierarchy.filter( + Exists( + Contributor.objects.filter( + node=OuterRef('pk'), + user=instance.user, + ), + ), + ) + with transaction.atomic(): + for descendant in targets: + removed = descendant.remove_contributor(instance, auth) + if not removed: + raise ValidationError(f'{descendant._id} must have at least one registered admin contributor') else: removed = node.remove_contributor(instance, auth) if not removed: diff --git a/api_tests/nodes/views/test_node_contributors_detail.py b/api_tests/nodes/views/test_node_contributors_detail.py index 57f7e41444f..004a591cc2c 100644 --- a/api_tests/nodes/views/test_node_contributors_detail.py +++ b/api_tests/nodes/views/test_node_contributors_detail.py @@ -459,3 +459,46 @@ def test_can_remove_self_as_contributor_not_unique_admin(self, app, user_write_c ) assert res.status_code == 204 assert user_write_contrib not in project.contributors + + def test_remove_contributor_include_children_removes_descendants(self, app, user, user_write_contrib, project): + child1 = ProjectFactory(parent=project, creator=user) + child2 = ProjectFactory(parent=project, creator=user) + child1.add_contributor(user_write_contrib, permissions=permissions.WRITE, visible=True, save=True) + child2.add_contributor(user_write_contrib, permissions=permissions.WRITE, visible=True, save=True) + + assert user_write_contrib in project.contributors + assert user_write_contrib in child1.contributors + assert user_write_contrib in child2.contributors + + url = f'/{API_BASE}nodes/{project._id}/contributors/{user_write_contrib._id}/?include_children=true' + with disconnected_from_listeners(contributor_removed): + res = app.delete(url, auth=user.auth) + assert res.status_code == 204 + + project.reload() + child1.reload() + child2.reload() + + assert user_write_contrib not in project.contributors + assert user_write_contrib not in child1.contributors + assert user_write_contrib not in child2.contributors + + def test_remove_contributor_include_children_is_atomic_on_violation(self, app, user, user_write_contrib, project): + child = ProjectFactory(parent=project, creator=user) + child.add_contributor(user_write_contrib, permissions=permissions.ADMIN, visible=True, save=True) + child.set_permissions(user, permissions.READ, save=True) + + assert user_write_contrib in project.contributors + assert user_write_contrib in child.contributors + + url = f'/{API_BASE}nodes/{project._id}/contributors/{user_write_contrib._id}/?include_children=true' + with disconnected_from_listeners(contributor_removed): + res = app.delete(url, auth=user.auth, expect_errors=True) + + assert res.status_code == 400 + + project.reload() + child.reload() + + assert user_write_contrib in project.contributors + assert user_write_contrib in child.contributors From c8dc257262384cbb1b2edc3a69f9aa96379ea570 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Thu, 6 Nov 2025 18:14:57 +0200 Subject: [PATCH 3/4] remove from children only for nodes --- api/nodes/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/nodes/views.py b/api/nodes/views.py index 3206a66533b..e92c0ab706b 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -554,7 +554,7 @@ def perform_destroy(self, instance): include_children = is_truthy(self.request.query_params.get('include_children', False)) - if include_children: + if include_children and isinstance(node, Node): hierarchy = Node.objects.get_children(node, active=True, include_root=True) targets = hierarchy.filter( Exists( From bdf306b3c32673d099485a79bc4559380ebd5810 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Wed, 7 Jan 2026 16:17:51 +0200 Subject: [PATCH 4/4] fix tests --- ...t_draft_registration_contributor_detail.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/api_tests/draft_registrations/views/test_draft_registration_contributor_detail.py b/api_tests/draft_registrations/views/test_draft_registration_contributor_detail.py index 0c2dce3501b..ff7907c85fc 100644 --- a/api_tests/draft_registrations/views/test_draft_registration_contributor_detail.py +++ b/api_tests/draft_registrations/views/test_draft_registration_contributor_detail.py @@ -8,12 +8,14 @@ ) from api_tests.nodes.views.test_node_contributors_detail_ordering import TestNodeContributorOrdering from api_tests.nodes.views.test_node_contributors_detail_update import TestNodeContributorUpdate +from api_tests.utils import disconnected_from_listeners from osf_tests.factories import ( DraftRegistrationFactory, ProjectFactory, AuthUserFactory ) from osf.utils import permissions +from website.project.signals import contributor_removed @pytest.fixture() @@ -251,6 +253,30 @@ def url_user_non_contrib(self, project, user_non_contrib): return '/{}draft_registrations/{}/contributors/{}/'.format( API_BASE, project._id, user_non_contrib._id) + def test_remove_contributor_include_children_removes_descendants(self, app, user, user_write_contrib, project): + assert user_write_contrib in project.contributors + + url = f'/{API_BASE}draft_registrations/{project._id}/contributors/{user_write_contrib._id}/?include_children=true' + with disconnected_from_listeners(contributor_removed): + res = app.delete(url, auth=user.auth) + assert res.status_code == 204 + + project.reload() + assert user_write_contrib not in project.contributors + + def test_remove_contributor_include_children_is_atomic_on_violation(self, app, user, user_write_contrib, project): + assert user_write_contrib in project.contributors + + # Draft registrations don't have children, so include_children parameter is ignored + # The contributor should be removed successfully since there are no children to cause violations + url = f'/{API_BASE}draft_registrations/{project._id}/contributors/{user_write_contrib._id}/?include_children=true' + with disconnected_from_listeners(contributor_removed): + res = app.delete(url, auth=user.auth) + assert res.status_code == 204 + + project.reload() + assert user_write_contrib not in project.contributors + @pytest.mark.django_db class TestDraftBibliographicContributorDetail():