feat(lab-4291): resolve comments, remove deleted users from count

florianlega · florianlega · commit 6734ffcaa65a · 2026-03-30T17:56:21.000+02:00
diff --git a/recipes/copy_workflow_between_projects.ipynb b/recipes/copy_workflow_between_projects.ipynb
@@ -89,15 +89,7 @@
      "skip"
     ]
    },
-   "outputs": [
-    {
-     "name": "stdout",
-     "output_type": "stream",
-     "text": [
-      "Source project: cmn8z1aoz004rme4i4zx8c81w\n"
-     ]
-    }
-   ],
+   "outputs": [],
    "source": [
     "json_interface = {\n",
     "    \"jobs\": {\n",
@@ -168,18 +160,7 @@
      "skip"
     ]
    },
-   "outputs": [
-    {
-     "name": "stdout",
-     "output_type": "stream",
-     "text": [
-      "Source workflow steps:\n",
-      " - {'assignees': [{'email': 'test+edouard@kili-technology.com', 'id': 'user-2'}, {'email': 'labeler1@example.com', 'id': 'cmn8yvbve000lme4i2yxa252z'}, {'email': 'labeler2@example.com', 'id': 'cmn8yvbzi000tme4ifga589w7'}, {'email': 'reviewer1@example.com', 'id': 'cmn8yvc2h0011me4iajuh8v1r'}], 'type': 'DEFAULT', 'name': 'Labeling Renamed', 'id': 'cmn8z1apd004tme4i9bvbfwrv'}\n",
-      " - {'assignees': [{'email': 'test+edouard@kili-technology.com', 'id': 'user-2'}, {'email': 'reviewer1@example.com', 'id': 'cmn8yvc2h0011me4iajuh8v1r'}], 'type': 'REVIEW', 'name': 'Review', 'id': 'cmn8z1apn004ume4i8hwm0kce'}\n",
-      " - {'assignees': [{'email': 'test+edouard@kili-technology.com', 'id': 'user-2'}, {'email': 'test+max@kili-technology.com', 'id': 'user-7'}, {'email': 'test+queues@kili-technology.com', 'id': 'user-9'}, {'email': 'test+github@kili-technology.com', 'id': 'user-6'}, {'email': 'reviewer1@example.com', 'id': 'cmn8yvc2h0011me4iajuh8v1r'}], 'type': 'REVIEW', 'name': 'Review 2', 'id': 'cmn8z1e77005tme4ia3r05pk2'}\n"
-     ]
-    }
-   ],
+   "outputs": [],
    "source": [
     "from kili.use_cases.project_workflow import ProjectWorkflowUseCases  # noqa: F401\n",
     "\n",
@@ -239,15 +220,7 @@
      "skip"
     ]
    },
-   "outputs": [
-    {
-     "name": "stdout",
-     "output_type": "stream",
-     "text": [
-      "Destination project: cmn8z1hbg005yme4i3e6sc8mv\n"
-     ]
-    }
-   ],
+   "outputs": [],
    "source": [
     "destination_project_id = kili.create_project(\n",
     "    input_type=\"IMAGE\",\n",
@@ -303,15 +276,7 @@
    "cell_type": "code",
    "execution_count": null,
    "metadata": {},
-   "outputs": [
-    {
-     "name": "stdout",
-     "output_type": "stream",
-     "text": [
-      "{'enforceStepSeparation': True, 'steps': [{'id': 'cmn8z1hbw0060me4iek7parym'}, {'id': 'cmn8z1kio0070me4i7nrta6fc'}, {'id': 'cmn8z1kiu0071me4ie9kg0tn9'}]}\n"
-     ]
-    }
-   ],
+   "outputs": [],
    "source": [
     "result = kili.copy_workflow_from_project(\n",
     "    destination_project_id=destination_project_id,\n",
@@ -338,18 +303,7 @@
    "cell_type": "code",
    "execution_count": null,
    "metadata": {},
-   "outputs": [
-    {
-     "name": "stdout",
-     "output_type": "stream",
-     "text": [
-      "Destination workflow steps after copy:\n",
-      " - {'name': 'Labeling Renamed', 'type': 'DEFAULT', 'consensusCoverage': 50, 'numberOfExpectedLabelsForConsensus': 2, 'stepCoverage': None, 'sendBackStepId': None}\n",
-      " - {'name': 'Review', 'type': 'REVIEW', 'consensusCoverage': None, 'numberOfExpectedLabelsForConsensus': None, 'stepCoverage': None, 'sendBackStepId': None}\n",
-      " - {'name': 'Review 2', 'type': 'REVIEW', 'consensusCoverage': None, 'numberOfExpectedLabelsForConsensus': None, 'stepCoverage': 80, 'sendBackStepId': 'cmn8z1hbw0060me4iek7parym'}\n"
-     ]
-    }
-   ],
+   "outputs": [],
    "source": [
     "dest_steps = kili.get_steps(\n",
     "    project_id=destination_project_id,\n",
@@ -393,18 +347,7 @@
      "skip"
     ]
    },
-   "outputs": [
-    {
-     "data": {
-      "text/plain": [
-       "'cmn8z0wln0044me4icsblhtja'"
-      ]
-     },
-     "execution_count": null,
-     "metadata": {},
-     "output_type": "execute_result"
-    }
-   ],
+   "outputs": [],
    "source": [
     "kili.delete_project(source_project_id)\n",
     "kili.delete_project(destination_project_id)"
diff --git a/src/kili/adapters/kili_api_gateway/project_workflow/mappers.py b/src/kili/adapters/kili_api_gateway/project_workflow/mappers.py
@@ -10,39 +10,39 @@ def project_input_mapper(data: ProjectWorkflowDataKiliAPIGatewayInput) -> dict:
     return {
         "enforceStepSeparation": data.enforce_step_separation,
         "steps": {
-            "creates": [
-                update_step_mapper(step, for_copy=data.for_copy) for step in data.create_steps
-            ]
+            "creates": [update_step_mapper(step, data.null_fields) for step in data.create_steps]
             if data.create_steps
             else [],
-            "updates": [
-                update_step_mapper(step, for_copy=data.for_copy) for step in data.update_steps
-            ]
+            "updates": [update_step_mapper(step, data.null_fields) for step in data.update_steps]
             if data.update_steps
             else [],
-            "deletes": data.delete_steps if data.delete_steps else [],
+            "deletes": data.delete_steps or [],
         },
     }
 
 
 def update_step_mapper(
-    data: WorkflowStepCreate | WorkflowStepUpdate, for_copy: bool = False
+    data: WorkflowStepCreate | WorkflowStepUpdate,
+    null_fields: frozenset[str] = frozenset(),
 ) -> dict:
-    """Build the GraphQL create StepData variable to be sent in an operation."""
-    ## In copy worklow use case, we want to copy as well
-    # consensusCoverage and numberOfExpectedLabelsForConsensus properties, even if they are None
+    """Build the GraphQL StepData variable to be sent in an operation.
 
-    step = {
-        "id": data.get("id"),
-        "name": data.get("name"),
-        "consensusCoverage": data.get("consensus_coverage"),
-        "numberOfExpectedLabelsForConsensus": data.get("number_of_expected_labels_for_consensus"),
-        "stepCoverage": data.get("step_coverage"),
-        "type": data.get("type"),
-        "assignees": data.get("assignees"),
-        "sendBackStepId": data.get("send_back_step_id"),
+    A field is included when its value is not None, or when its GQL name appears in
+    null_fields (meaning the caller explicitly wants to send null to clear that field).
+    Fields absent from the TypedDict are never included.
+    """
+    mapping = {
+        "id": "id",
+        "name": "name",
+        "consensusCoverage": "consensus_coverage",
+        "numberOfExpectedLabelsForConsensus": "number_of_expected_labels_for_consensus",
+        "stepCoverage": "step_coverage",
+        "type": "type",
+        "assignees": "assignees",
+        "sendBackStepId": "send_back_step_id",
+    }
+    return {
+        gql_key: data[py_key]
+        for gql_key, py_key in mapping.items()
+        if py_key in data and (data[py_key] is not None or gql_key in null_fields)
     }
-    if for_copy:
-        special_keys = ["consensusCoverage", "numberOfExpectedLabelsForConsensus"]
-        return {k: v for k, v in step.items() if v is not None or k in special_keys}
-    return {k: v for k, v in step.items() if v is not None}
diff --git a/src/kili/adapters/kili_api_gateway/project_workflow/operations_mixin.py b/src/kili/adapters/kili_api_gateway/project_workflow/operations_mixin.py
@@ -1,4 +1,5 @@
 """Mixin extending Kili API Gateway class with Projects related operations."""
+
 import warnings
 
 from kili.adapters.kili_api_gateway.base import BaseOperationMixin
@@ -62,12 +63,12 @@ def get_steps(self, project_id: str, fields: ListOrTuple[str]) -> list[dict]:
 
     def count_activated_project_users(self, project_id: str) -> int:
         """Count project users with ACTIVATED status."""
-        where = ProjectUserWhere(project_id=project_id, status="ACTIVATED")
+        where = ProjectUserWhere(project_id=project_id, status="ACTIVATED", deleted=False)
         return ProjectUserQuery(self.graphql_client, self.http_client).count(where)
 
     def list_activated_project_users(self, project_id: str) -> list[dict]:
         """List project users with ACTIVATED status, returning role and user id."""
-        where = ProjectUserWhere(project_id=project_id, status="ACTIVATED")
+        where = ProjectUserWhere(project_id=project_id, status="ACTIVATED", deleted=False)
         return list(
             ProjectUserQuery(self.graphql_client, self.http_client)(
                 where=where,
@@ -81,7 +82,7 @@ def add_reviewers_to_step(
     ) -> list[str]:
         """Add reviewers to a specific step."""
         existing_members = ProjectUserQuery(self.graphql_client, self.http_client)(
-            where=ProjectUserWhere(project_id=project_id, status="ACTIVATED"),
+            where=ProjectUserWhere(project_id=project_id, status="ACTIVATED", deleted=False),
             fields=["role", "user.email", "user.id", "activated"],
             options=QueryOptions(None),
         )
diff --git a/src/kili/adapters/kili_api_gateway/project_workflow/types.py b/src/kili/adapters/kili_api_gateway/project_workflow/types.py
@@ -14,4 +14,4 @@ class ProjectWorkflowDataKiliAPIGatewayInput:
     create_steps: Optional[list[WorkflowStepCreate]]
     update_steps: Optional[list[WorkflowStepUpdate]]
     delete_steps: Optional[list[str]]
-    for_copy: bool = False
+    null_fields: frozenset[str] = frozenset()
diff --git a/src/kili/use_cases/project_workflow/__init__.py b/src/kili/use_cases/project_workflow/__init__.py
@@ -107,8 +107,8 @@ def copy_workflow_from_project(
         # 3. Validate destination has no labels
         self._validate_destination_has_no_labels(destination_project_id)
 
-        # 3b. Validate destination has enough labelers for consensus on the first step
-        self._validate_consensus_labelers(destination_project_id, source_steps[0])
+        # 3b. Validate destination has enough labelers for the most demanding consensus step
+        self._validate_consensus_labelers(destination_project_id, source_steps)
 
         # 4. Get existing destination steps and activated users
         dest_steps = self._get_destination_steps(destination_project_id)
@@ -168,7 +168,7 @@ def copy_workflow_from_project(
                 create_steps=steps_to_create or None,
                 update_steps=update_steps or None,
                 delete_steps=delete_steps,
-                for_copy=True,
+                null_fields=frozenset({"consensusCoverage", "numberOfExpectedLabelsForConsensus"}),
             ),
         )
 
@@ -188,10 +188,18 @@ def copy_workflow_from_project(
         return result
 
     def _validate_consensus_labelers(
-        self, destination_project_id: ProjectId, first_source_step: dict[str, object]
+        self, destination_project_id: ProjectId, source_steps: list[dict[str, object]]
     ) -> None:
-        """Validate the destination has enough activated labelers for the source consensus setting."""
-        required = cast("int", first_source_step.get("numberOfExpectedLabelsForConsensus"))
+        """Validate the destination has enough activated labelers for every consensus step."""
+        required = max(
+            (
+                cast("int", step.get("numberOfExpectedLabelsForConsensus"))
+                for step in source_steps
+                if step.get("numberOfExpectedLabelsForConsensus")
+            ),
+            default=0,
+        )
+
         if not required:
             return
         activated_count = self._kili_api_gateway.count_activated_project_users(
@@ -306,17 +314,14 @@ def _make_create_step(step: dict[str, object], assignees: list[str]) -> Workflow
 
 def _build_step_update(dest_step_id: str, source_step: dict[str, object]) -> WorkflowStepUpdate:
     """Build a WorkflowStepUpdate for the first dest step based on source step properties."""
-    update: WorkflowStepUpdate = {
+    return {
         "id": dest_step_id,
         "name": cast("str", source_step["name"]),
+        "consensus_coverage": cast("int | None", source_step.get("consensusCoverage")),
+        "number_of_expected_labels_for_consensus": cast(
+            "int | None", source_step.get("numberOfExpectedLabelsForConsensus")
+        ),
     }
-    if source_step.get("consensusCoverage") is not None:
-        update["consensus_coverage"] = cast("int", source_step["consensusCoverage"])
-    if source_step.get("numberOfExpectedLabelsForConsensus") is not None:
-        update["number_of_expected_labels_for_consensus"] = cast(
-            "int", source_step["numberOfExpectedLabelsForConsensus"]
-        )
-    return update
 
 
 def _build_send_back_updates(
diff --git a/tests/unit/use_cases/project_workflow/test_copy_workflow.py b/tests/unit/use_cases/project_workflow/test_copy_workflow.py
@@ -205,6 +205,46 @@ def test_raises_when_destination_has_too_few_labelers(self, use_cases, mock_gate
                 destination_project_id=dest_id,
             )
 
+    def test_raises_when_later_step_requires_more_labelers(self, use_cases, mock_gateway):
+        """Test that validation checks all steps, not just the first one."""
+        source_id = ProjectId("source-project")
+        dest_id = ProjectId("dest-project")
+
+        # First step: no consensus; second step: requires 5 labelers
+        steps = [
+            {
+                "id": "source-step-1",
+                "name": "Labeling",
+                "type": "DEFAULT",
+                "consensusCoverage": None,
+                "numberOfExpectedLabelsForConsensus": None,
+                "stepCoverage": None,
+                "sendBackStepId": None,
+            },
+            {
+                "id": "source-step-2",
+                "name": "Second Pass",
+                "type": "DEFAULT",
+                "consensusCoverage": 80,
+                "numberOfExpectedLabelsForConsensus": 5,
+                "stepCoverage": None,
+                "sendBackStepId": None,
+            },
+        ]
+        mock_gateway.get_steps.return_value = steps
+        mock_gateway.get_project.side_effect = [
+            {"enforceStepSeparation": None},
+            {"workflowVersion": "V2"},
+        ]
+        mock_gateway.count_labels.return_value = 0
+        mock_gateway.count_activated_project_users.return_value = 3  # fewer than 5
+
+        with pytest.raises(ValueError, match="3 activated labeler"):
+            use_cases.copy_workflow_from_project(
+                source_project_id=source_id,
+                destination_project_id=dest_id,
+            )
+
     def test_skips_consensus_check_when_not_set(self, use_cases, mock_gateway):
         """Test consensus labeler check is skipped when numberOfExpectedLabelsForConsensus is None."""
         source_id = ProjectId("source-project")
