diff --git a/api/features/views.py b/api/features/views.py index e4d25e761a59..43497af9698d 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -433,8 +433,8 @@ def remove_group_owners(self, request, *args, **kwargs): # type: ignore[no-unty serializer.is_valid(raise_exception=True) self._validate_owner_removal( feature, - owners_to_remove=0, - group_owners_to_remove=len(serializer.validated_data["group_ids"]), + owner_ids=set(), + group_owner_ids=set(serializer.validated_data["group_ids"]), ) serializer.remove_group_owners(feature) response = Response(self.get_serializer(instance=feature).data) @@ -471,8 +471,8 @@ def remove_owners(self, request, *args, **kwargs): # type: ignore[no-untyped-de feature = self.get_object() self._validate_owner_removal( feature, - owners_to_remove=len(serializer.validated_data["user_ids"]), - group_owners_to_remove=0, + owner_ids=set(serializer.validated_data["user_ids"]), + group_owner_ids=set(), ) serializer.remove_users(feature) @@ -481,18 +481,22 @@ def remove_owners(self, request, *args, **kwargs): # type: ignore[no-untyped-de def _validate_owner_removal( self, feature: Feature, - owners_to_remove: int, - group_owners_to_remove: int, + owner_ids: set[int], + group_owner_ids: set[int], ) -> None: if not feature.project.enforce_feature_owners: return - remaining = ( - feature.owners.count() - - owners_to_remove - + feature.group_owners.count() - - group_owners_to_remove - ) - if remaining < 1: + + existing_owners = feature.owners.all() + existing_group_owners = feature.group_owners.all() + + existing_owner_ids = {owner.id for owner in existing_owners} + existing_group_owner_ids = {group.id for group in existing_group_owners} + + if not ( + (existing_owner_ids - owner_ids) + or (existing_group_owner_ids - group_owner_ids) + ): raise serializers.ValidationError( "This project requires at least one owner or group owner per feature." ) diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index 26b85fdbdc5d..cd2c981171b8 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -5346,3 +5346,61 @@ def test_remove_group_owners__enforce_owners_user_owners_remain__returns_200( feature.refresh_from_db() assert group not in feature.group_owners.all() assert admin_user in feature.owners.all() + + +def test_remove_owners__enforce_owners_with_nonexistent_user_ids__still_blocks_when_needed( + admin_client_new: APIClient, + project: Project, + feature: Feature, + admin_user: FFAdminUser, +) -> None: + # Given + project.enforce_feature_owners = True + project.save() + non_owner = FFAdminUser.objects.create_user(email="nonowner@example.com") # type: ignore[no-untyped-call] + feature.owners.add(admin_user) + + # When + response = admin_client_new.post( + f"/api/v1/projects/{project.id}/features/{feature.id}/remove-owners/", + data={"user_ids": [admin_user.id, non_owner.id]}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + feature.refresh_from_db() + assert admin_user in feature.owners.all() + + +def test_remove_group_owners__enforce_owners_with_nonexistent_group_ids__allows_when_real_owner_remains( + admin_client_new: APIClient, + project: Project, + feature: Feature, + admin_user: FFAdminUser, + organisation: Organisation, +) -> None: + # Given + project.enforce_feature_owners = True + project.save() + group = UserPermissionGroup.objects.create( + name="Test Group", organisation=organisation + ) + non_owner_group = UserPermissionGroup.objects.create( + name="Non Owner Group", organisation=organisation + ) + feature.owners.add(admin_user) + feature.group_owners.add(group) + + # When + response = admin_client_new.post( + f"/api/v1/projects/{project.id}/features/{feature.id}/remove-group-owners/", + data={"group_ids": [group.id, non_owner_group.id]}, + format="json", + ) + + # Then + assert response.status_code == status.HTTP_200_OK + feature.refresh_from_db() + assert group not in feature.group_owners.all() + assert admin_user in feature.owners.all()