Refine user deletion tests and logging

Author: igennova

src/routers/openml/users.py

@@ -1,5 +1,6 @@
 """User account HTTP endpoints."""
 
+from http import HTTPStatus
 from typing import Annotated
 
 from fastapi import APIRouter, Depends, Path, Response
@@ -23,11 +24,13 @@
 @router.delete(
     "/{user_id}",
     responses={
-        204: {"description": "User account deleted."},
-        401: {"description": "Authentication failed or missing."},
-        403: {"description": "Not allowed to delete this account."},
-        404: {"description": "User id not found."},
-        409: {"description": "User still has datasets, flows, runs, or studies."},
+        HTTPStatus.NO_CONTENT: {"description": "User account deleted."},
+        HTTPStatus.UNAUTHORIZED: {"description": "Authentication failed or missing."},
+        HTTPStatus.FORBIDDEN: {"description": "Not allowed to delete this account."},
+        HTTPStatus.NOT_FOUND: {"description": "User id not found."},
+        HTTPStatus.CONFLICT: {
+            "description": "User still has datasets, flows, runs, or studies.",
+        },
     },
 )
 async def delete_user_account(
@@ -42,6 +45,8 @@ async def delete_user_account(
     datasets, tasks, or tags). Users may only delete their own account.
     Administrators may delete any account that satisfies the no-resources rule.
     """
+    # How to handle users that do have associated resources is an ongoing discussion,
+    # see also: https://github.com/openml/server-api/issues/194
     if current_user.user_id != user_id and not await current_user.is_admin():
         msg = "You may only delete your own user account."
         raise ForbiddenError(msg)
@@ -63,4 +68,4 @@ async def delete_user_account(
         raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc
 
     logger.info("User account {user_id} was removed.", user_id=user_id)
-    return Response(status_code=204)
+    return Response(status_code=HTTPStatus.NO_CONTENT)

tests/routers/openml/users_delete_test.py

@@ -1,7 +1,6 @@
-"""Tests for DELETE /users/{user_id} (Phase 1: no resources, self or admin)."""
+"""Tests for DELETE /users/{user_id}."""
 
 import uuid
-from collections.abc import AsyncGenerator
 from http import HTTPStatus
 from typing import NamedTuple
 
@@ -15,7 +14,7 @@
 from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError
 from database.users import UserGroup
 from routers.openml.users import delete_user_account
-from tests.users import ADMIN_USER, SOME_USER, ApiKey
+from tests.users import ADMIN_USER, OWNER_USER, SOME_USER, ApiKey
 
 
 async def test_delete_user_missing_auth(py_api: httpx.AsyncClient) -> None:
@@ -32,7 +31,7 @@ class DisposableUser(NamedTuple):
 
 
 @pytest.fixture
-async def disposable_user(user_test: AsyncConnection) -> AsyncGenerator[DisposableUser]:
+async def disposable_user(user_test: AsyncConnection) -> DisposableUser:
     api_key = uuid.uuid4().hex
     suffix = uuid.uuid4().hex[:10]
     username = f"tmp_user_{suffix}"
@@ -58,23 +57,20 @@ async def disposable_user(user_test: AsyncConnection) -> AsyncGenerator[Disposab
         text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"),
         parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value},
     )
-    yield DisposableUser(user_id=new_id, api_key=api_key)
-    await user_test.execute(
-        text("DELETE FROM users_groups WHERE user_id = :uid"),
-        parameters={"uid": new_id},
-    )
-    await user_test.execute(
-        text("DELETE FROM users WHERE id = :uid"),
-        parameters={"uid": new_id},
-    )
+    return DisposableUser(user_id=new_id, api_key=api_key)
+    # No explicit teardown: the ``user_test`` fixture rolls back at the end
+    # of the test, which removes the rows inserted above.
 
 
 @pytest.mark.mut
 async def test_delete_user_api_success_self_delete(
     py_api: httpx.AsyncClient,
     user_test: AsyncConnection,
     disposable_user: DisposableUser,
+    mocker: pytest_mock.MockerFixture,
 ) -> None:
+    log_info = mocker.patch("routers.openml.users.logger.info")
+
     response = await py_api.delete(
         f"/users/{disposable_user.user_id}",
         params={"api_key": disposable_user.api_key},
@@ -88,6 +84,11 @@ async def test_delete_user_api_success_self_delete(
     )
     assert exists.one_or_none() is None
 
+    log_info.assert_any_call(
+        "User account {user_id} was removed.",
+        user_id=disposable_user.user_id,
+    )
+
 
 @pytest.mark.mut
 async def test_delete_user_api_success_admin_deletes_disposable_user(
@@ -143,43 +144,32 @@ async def test_delete_user_direct_forbidden(
     assert exc_info.value.status_code == HTTPStatus.FORBIDDEN
     assert exc_info.value.uri == ForbiddenError.uri
 
+    admin_row = await user_test.execute(
+        text("SELECT 1 FROM users WHERE id = :id LIMIT 1"),
+        parameters={"id": ADMIN_USER.user_id},
+    )
+    assert admin_row.one_or_none() is not None
+
 
 async def test_delete_user_direct_conflict_has_resources(
     user_test: AsyncConnection,
     expdb_test: AsyncConnection,
 ) -> None:
     with pytest.raises(AccountHasResourcesError, match="Cannot delete this account") as exc_info:
         await delete_user_account(
-            user_id=16,
+            user_id=OWNER_USER.user_id,
             current_user=ADMIN_USER,
             expdb=expdb_test,
             userdb=user_test,
         )
     assert exc_info.value.status_code == HTTPStatus.CONFLICT
     assert exc_info.value.uri == AccountHasResourcesError.uri
 
-
-@pytest.mark.mut
-async def test_delete_user_direct_success_logs_info(
-    user_test: AsyncConnection,
-    expdb_test: AsyncConnection,
-    disposable_user: DisposableUser,
-    mocker: pytest_mock.MockerFixture,
-) -> None:
-    log_info = mocker.patch("routers.openml.users.logger.info")
-
-    response = await delete_user_account(
-        user_id=disposable_user.user_id,
-        current_user=ADMIN_USER,
-        expdb=expdb_test,
-        userdb=user_test,
-    )
-
-    assert response.status_code == HTTPStatus.NO_CONTENT
-    log_info.assert_called_once_with(
-        "User account {user_id} was removed.",
-        user_id=disposable_user.user_id,
+    owner_row = await user_test.execute(
+        text("SELECT 1 FROM users WHERE id = :id LIMIT 1"),
+        parameters={"id": OWNER_USER.user_id},
     )
+    assert owner_row.one_or_none() is not None
 
 
 @pytest.mark.mut