doc: Documenting code, and add a test for PATCH /fields

Author: Robin-Van-de-Merghel

diracx-routers/src/diracx/routers/pilots/auth.py

@@ -50,7 +50,7 @@ async def pilot_login(
         dict[str, BaseAccessPolicy], Depends(BaseAccessPolicy.all_used_access_policies)
     ],
 ) -> TokenResponse:
-    """Endpoint without policy, the pilot uses only its secret."""
+    """This endpoint is used by the pilot to exchange a secret for a token."""
     try:
         access_payload, refresh_payload = await verify_pilot_credentials(
             pilot_db=pilot_db,

diracx-routers/src/diracx/routers/pilots/fields.py

@@ -53,12 +53,7 @@ async def create_pilot_secrets(
     pilot_db: PilotAgentsDB,
     settings: AuthSettings,
 ) -> list[PilotSecretsInfo]:
-
-    # TODO: Check max for pilot_max
-    # [SQL: INSERT INTO `PilotSecrets` (`HashedSecret`, `SecretGlobalUseCount`, `SecretGlobalUseCountMax`, `SecretVO`)
-    # VALUES (%s, %s, %s, %s)]
-    # [parameters: ('debc69a1f3ada36060b7fea2d1f5bf09339971b089cc2b8037bc3ecba527babf', 0, 1000000000, 'diracAdmin')]
-
+    """Endpoint to create secrets."""
     await check_permissions(action=ActionType.CREATE_PILOT_OR_SECRET, vo=vo)
 
     if expiration_minutes and expiration_minutes <= 0:
@@ -141,17 +136,46 @@ async def associate_pilots_with_secrets(
     )
 
 
+EXAMPLE_UPDATE_FIELDS = {
+    "Update the BenchMark field": {
+        "summary": "Update BenchMark",
+        "description": "Update only the BenchMark for one pilot.",
+        "value": {
+            "pilot_stamps_to_fields_mapping": [
+                {"PilotStamp": "the_pilot_stamp", "BenchMark": 1.0}
+            ]
+        },
+    },
+    "Update multiple statuses": {
+        "summary": "Update multiple pilots",
+        "description": "Update multiple pilots statuses.",
+        "value": {
+            "pilot_stamps_to_fields_mapping": [
+                {"PilotStamp": "the_first_pilot_stamp", "Status": "Waiting"},
+                {"PilotStamp": "the_second_pilot_stamp", "Status": "Waiting"},
+            ]
+        },
+    },
+}
+
+
 @router.patch("/fields", status_code=HTTPStatus.NO_CONTENT)
 async def update_pilot_fields(
     pilot_stamps_to_fields_mapping: Annotated[
         list[PilotFieldsMapping],
-        Body(description="(pilot_stamp, pilot_fields) mapping to change.", embed=True),
+        Body(
+            description="(pilot_stamp, pilot_fields) mapping to change.",
+            embed=True,
+            openapi_examples=EXAMPLE_UPDATE_FIELDS,
+        ),
     ],
     pilot_agents_db: PilotAgentsDB,
     check_permissions: CheckPilotManagementPolicyCallable,
 ):
-    """Modify a field of a pilot."""
-    # TODO: Test this route
+    """Modify a field of a pilot.
+
+    Note: Only the fields in PilotFieldsMapping are mutable, except for the PilotStamp.
+    """
     # TODO: Add an example for openapi
     pilot_stamps = [mapping.PilotStamp for mapping in pilot_stamps_to_fields_mapping]
 
@@ -177,6 +201,7 @@ async def associate_pilot_with_jobs(
     ],
     check_permissions: CheckDiracServicesPolicyCallable,
 ):
+    """Endpoint only for DIRAC services, to associate a pilot with a job."""
     await check_permissions()
 
     try:

diracx-routers/src/diracx/routers/pilots/management.py

@@ -24,6 +24,7 @@
 from ..utils.users import AuthorizedUserInfo, verify_dirac_access_token
 from .access_policies import (
     ActionType,
+    CheckDiracServicesPolicyCallable,
     CheckPilotManagementPolicyCallable,
 )
 
@@ -95,7 +96,10 @@ async def delete_pilots(
     pilot_agents_db: PilotAgentsDB,
     check_permissions: CheckPilotManagementPolicyCallable,
 ):
-    """Endpoint to delete a pilot."""
+    """Endpoint to delete a pilot.
+
+    If at least one pilot is not found, it WILL rollback.
+    """
     await check_permissions(
         action=ActionType.CHANGE_PILOT_FIELD,
         pilot_stamps=pilot_stamps,
@@ -125,7 +129,7 @@ async def clear_pilots(
             )
         ),
     ],
-    check_permissions: CheckPilotManagementPolicyCallable,
+    check_permissions: CheckDiracServicesPolicyCallable,
     delete_only_aborted: Annotated[
         bool,
         Query(
@@ -137,13 +141,8 @@ async def clear_pilots(
         ),
     ] = True,
 ):
-    """Delete all pilots that lived more than age_in_days."""
-    # TODO: Be stricter here and only allow admins?
-    # TODO: Add test (how to test? Millisec?)
-    await check_permissions(
-        action=ActionType.CREATE_PILOT_OR_SECRET,
-        pilot_db=pilot_agents_db,
-    )
+    """Endpoint for DIRAC to delete all pilots that lived more than age_in_days."""
+    await check_permissions()
 
     if age_in_days < 0:
         raise HTTPException(

diracx-routers/src/diracx/routers/pilots/query.py

@@ -29,8 +29,8 @@
         "value": {"search": [{"parameter": "PilotID", "operator": "eq", "value": "5"}]},
     },
     "Get ordered pilot statuses": {
-        "summary": "Get ordered job statuses",
-        "description": "Get only job statuses for specific jobs, ordered by status",
+        "summary": "Get ordered pilot statuses",
+        "description": "Get only pilot statuses for specific pilots, ordered by status",
         "value": {
             "parameters": ["PilotID", "Status"],
             "search": [
@@ -115,7 +115,7 @@ async def search(
     ] = None,
 ) -> list[dict[str, Any]]:
     """Retrieve information about pilots."""
-    # TODO: Test this route
+    # Inspired by /api/jobs/query
     await check_permissions(action=ActionType.READ_PILOT_FIELDS)
 
     total, pilots = await get_pilot_info_bl(

diracx-routers/tests/pilots/test_pilot_creation.py

@@ -297,12 +297,12 @@ async def test_create_pilot_and_delete_it(normal_test_client):
 
 
 async def test_create_pilot_and_modify_it(normal_test_client):
-    pilot_stamp = "stamps_1"
+    pilot_stamps = ["stamps_1", "stamp_2"]
 
     #  -------------- Insert --------------
-    body = {"vo": MAIN_VO, "pilot_stamps": [pilot_stamp]}
+    body = {"vo": MAIN_VO, "pilot_stamps": pilot_stamps}
 
-    # Create a pilot
+    # Create pilots
     r = normal_test_client.post(
         "/api/pilots/",
         json=body,
@@ -311,10 +311,11 @@ async def test_create_pilot_and_modify_it(normal_test_client):
     assert r.status_code == 200, r.json()
 
     #  -------------- Modify --------------
+    # We modify only the first pilot
     body = {
         "pilot_stamps_to_fields_mapping": [
             PilotFieldsMapping(
-                PilotStamp=pilot_stamp,
+                PilotStamp=pilot_stamps[0],
                 BenchMark=1.0,
                 StatusReason="NewReason",
                 AccountingSent=True,
@@ -326,3 +327,25 @@ async def test_create_pilot_and_modify_it(normal_test_client):
     r = normal_test_client.patch("/api/pilots/fields", json=body)
 
     assert r.status_code == 204
+
+    body = {
+        "parameters": [],
+        "search": [],
+        "sort": [],
+        "distinct": True,
+    }
+
+    r = normal_test_client.post("/api/pilots/search", json=body)
+    assert r.status_code == 200, r.json()
+    pilot1 = r.json()[0]
+    pilot2 = r.json()[1]
+
+    assert pilot1["BenchMark"] == 1.0
+    assert pilot1["StatusReason"] == "NewReason"
+    assert pilot1["AccountingSent"]
+    assert pilot1["Status"] == "Waiting"
+
+    assert pilot2["BenchMark"] != pilot1["BenchMark"]
+    assert pilot2["StatusReason"] != pilot1["StatusReason"]
+    assert pilot2["AccountingSent"] != pilot1["AccountingSent"]
+    assert pilot2["Status"] != pilot1["Status"]