[ENG-9157] Add atomic ability to remove contributors from children projects in API (CenterForOpenScience#11425)

* add include_children to delete contributors

* atomic contributor remove; add tests

* remove from children only for nodes

* fix tests

---------

Co-authored-by: Yuhuai Liu <yuhuai@cos.io>

Author: 2 people

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
@@ -155,6 +156,7 @@
     CedarMetadataRecord,
     Preprint,
     Collection,
+    Contributor,
     NotificationType,
 )
 from addons.osfstorage.models import Region
@@ -551,13 +553,28 @@ 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 and isinstance(node, Node):
+            hierarchy = Node.objects.get_children(node, active=True, include_root=True)
+            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:
+                raise ValidationError('Must have at least one registered admin contributor')
 
 
 class NodeImplicitContributorsList(JSONAPIBaseView, generics.ListAPIView, ListFilterMixin, NodeMixin):

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():

api_tests/nodes/views/test_node_contributors_detail.py

@@ -457,3 +457,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