From 69381207a2b4dde975eaff8c6616079f138fc55e Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Thu, 6 Nov 2025 14:09:07 +0200 Subject: [PATCH 1/3] add include_children to delete contributors --- api/nodes/views.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/api/nodes/views.py b/api/nodes/views.py index 8ae38017b17..b0f58e05c94 100644 --- a/api/nodes/views.py +++ b/api/nodes/views.py @@ -549,9 +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') + + 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 1cd3de7dcb4a3211aa002dc5d2565cc65c8adc06 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Thu, 6 Nov 2025 17:57:17 +0200 Subject: [PATCH 2/3] atomic contributor remove; add tests --- admin_tests/preprints/test_views.py | 1 - api/nodes/views.py | 22 +++++++--- .../views/test_node_contributors_detail.py | 43 +++++++++++++++++++ 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/admin_tests/preprints/test_views.py b/admin_tests/preprints/test_views.py index 357ff643a06..5731c5c9aac 100644 --- a/admin_tests/preprints/test_views.py +++ b/admin_tests/preprints/test_views.py @@ -23,7 +23,6 @@ from osf.models.spam import SpamStatus from osf.utils.workflows import DefaultStates, RequestTypes from osf.utils.permissions import ADMIN -from framework.auth import Auth from admin_tests.utilities import setup_view, setup_log_view, handle_post_view_request diff --git a/api/nodes/views.py b/api/nodes/views.py index b0f58e05c94..5284352bab5 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 898370b6c28e839ace5222d84b21893df00c2bb2 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Thu, 6 Nov 2025 18:14:57 +0200 Subject: [PATCH 3/3] 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 5284352bab5..9bf2404d806 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(