From 25e443c3f253a13b673ae98e399a3d8b39cf2db7 Mon Sep 17 00:00:00 2001 From: wailbentafat Date: Mon, 1 Jun 2026 00:12:14 +0100 Subject: [PATCH 1/4] fix(photo-approval): resolve group photo lifecycle gaps - Auto-approve group photos as public when no face matches any user - Add ExpireStaleApprovals SQL + service method to unblock photos where users never respond (default 7-day timeout via config) - Wire hourly expiry background task into FastAPI lifespan --- app/core/config.py | 2 ++ app/main.py | 16 ++++++++++++++++ app/service/photo_approval.py | 8 ++++++++ app/worker/photo_worker/main.py | 9 +++++++++ db/generated/photo_approvals.py | 24 ++++++++++++++++++++++++ db/queries/photo_approvals.sql | 17 +++++++++++++++++ 6 files changed, 76 insertions(+) diff --git a/app/core/config.py b/app/core/config.py index 9f51f5c..997eed3 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -33,6 +33,8 @@ class Settings(BaseSettings): POSTGRES_HOST: str = "localhost" POSTGRES_PORT: int = 5432 + PHOTO_APPROVAL_TIMEOUT_DAYS: int = 7 + # Mobile auth/session defaults MOBILE_SESSION_LIMIT: int = 3 MOBILE_SESSION_TTL_SECONDS: int = 180 diff --git a/app/main.py b/app/main.py index 1e06dcb..981aa8f 100644 --- a/app/main.py +++ b/app/main.py @@ -8,6 +8,7 @@ from fastapi.middleware.cors import CORSMiddleware from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint from app.core.config import settings +from app.infra.database import engine from app.infra.minio import init_minio_client from app.infra.nats import NatsClient from app.infra.redis import RedisClient @@ -48,6 +49,18 @@ async def dispatch( +async def _approval_expiry_loop() -> None: + while True: + await asyncio.sleep(3600) + try: + async with engine.begin() as conn: + from app.container import Container + container = Container(conn) + await container.photo_approval_service.expire_stale(settings.PHOTO_APPROVAL_TIMEOUT_DAYS) + except Exception as exc: + logger.warning("Approval expiry task failed: %s", exc) + + MAX_RETRIES = 5 RETRY_DELAY = 2 # seconds @asynccontextmanager @@ -77,8 +90,11 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: await NatsClient.connect() get_face_embedding_service() + expiry_task = asyncio.create_task(_approval_expiry_loop()) + yield + expiry_task.cancel() await RedisClient.get_instance().close() await NatsClient.close() diff --git a/app/service/photo_approval.py b/app/service/photo_approval.py index b6cb870..7b8617f 100644 --- a/app/service/photo_approval.py +++ b/app/service/photo_approval.py @@ -64,6 +64,14 @@ async def decide( await self._photo_querier.update_photo_status(id=photo_id, status="approved") return "approved" + async def expire_stale(self, timeout_days: int) -> int: + count = 0 + async for _ in self._approval_querier.expire_stale_approvals(timeout_days=timeout_days): + count += 1 + if count: + logger.info("Auto-expired %d stale pending photo(s)", count) + return count + async def _delete_photo_storage(self, photo_id: UUID) -> None: photo = await self._photo_querier.get_photo_by_id(id=photo_id) if photo is None: diff --git a/app/worker/photo_worker/main.py b/app/worker/photo_worker/main.py index 651b606..892e50b 100644 --- a/app/worker/photo_worker/main.py +++ b/app/worker/photo_worker/main.py @@ -123,6 +123,8 @@ async def _handle_single_face(self, event: PhotoProcessEvent, face: DetectedFace async def _handle_group_photo(self, event: PhotoProcessEvent, faces: list[DetectedFace]) -> None: logger.info("Processing group photo %s with %d faces", event.photo_id, len(faces)) + approvals_created = 0 + for face_index, face in enumerate(faces): bbox_json = json.dumps({ "x1": float(face.bbox[0]), @@ -155,6 +157,8 @@ async def _handle_group_photo(self, event: PhotoProcessEvent, faces: list[Detect logger.info("No match for face %d in photo %s", face_index, event.photo_id) continue + approvals_created += 1 + try: await self._notification_service.create_notification( user_id=approval.user_id, @@ -178,6 +182,11 @@ async def _handle_group_photo(self, event: PhotoProcessEvent, faces: list[Detect approval.user_id, event.photo_id, exc, ) + if approvals_created == 0: + logger.info("No users matched in group photo %s, auto-approving as public", event.photo_id) + await self._photo_querier.update_photo_status(id=event.photo_id, status="approved") + await self._photo_querier.update_photo_visibility(id=event.photo_id, visibility="public") + async def _create_job(self, event: PhotoProcessEvent) -> models.ProcessingJob | None: if self._pj_querier is None: diff --git a/db/generated/photo_approvals.py b/db/generated/photo_approvals.py index f712392..d3776c3 100644 --- a/db/generated/photo_approvals.py +++ b/db/generated/photo_approvals.py @@ -11,6 +11,25 @@ from db.generated import models +EXPIRE_STALE_APPROVALS = """-- name: expire_stale_approvals \\:many +WITH stale_photos AS ( + SELECT id FROM photos + WHERE status = 'pending' + AND created_at < now() - make_interval(days => :p1::int) +), +_update_approvals AS ( + UPDATE photo_approvals + SET decision = 'approved', decided_at = now() + WHERE photo_id IN (SELECT id FROM stale_photos) + AND decision = 'pending' +) +UPDATE photos +SET status = 'approved' +WHERE id IN (SELECT id FROM stale_photos) +RETURNING id +""" + + CREATE_PHOTO_APPROVAL = """-- name: create_photo_approval \\:one INSERT INTO photo_approvals ( photo_id, @@ -49,6 +68,11 @@ class AsyncQuerier: def __init__(self, conn: sqlalchemy.ext.asyncio.AsyncConnection): self._conn = conn + async def expire_stale_approvals(self, *, timeout_days: int) -> AsyncIterator[uuid.UUID]: + result = await self._conn.stream(sqlalchemy.text(EXPIRE_STALE_APPROVALS), {"p1": timeout_days}) + async for row in result: + yield row[0] + async def create_photo_approval(self, *, photo_id: uuid.UUID, user_id: uuid.UUID, decision: str) -> Optional[models.PhotoApproval]: row = (await self._conn.execute(sqlalchemy.text(CREATE_PHOTO_APPROVAL), {"p1": photo_id, "p2": user_id, "p3": decision})).first() if row is None: diff --git a/db/queries/photo_approvals.sql b/db/queries/photo_approvals.sql index ac6b787..298f1fc 100644 --- a/db/queries/photo_approvals.sql +++ b/db/queries/photo_approvals.sql @@ -17,6 +17,23 @@ RETURNING *; -- name: GetPhotoApprovalsByPhotoId :many SELECT * FROM photo_approvals WHERE photo_id = $1; +-- name: ExpireStaleApprovals :many +WITH stale_photos AS ( + SELECT id FROM photos + WHERE status = 'pending' + AND created_at < now() - make_interval(days => $1::int) +), +_update_approvals AS ( + UPDATE photo_approvals + SET decision = 'approved', decided_at = now() + WHERE photo_id IN (SELECT id FROM stale_photos) + AND decision = 'pending' +) +UPDATE photos +SET status = 'approved' +WHERE id IN (SELECT id FROM stale_photos) +RETURNING id; + -- name: ListApprovalsByUserAndStatus :many SELECT * FROM photo_approvals WHERE user_id = $1 From e424ed27d447357b28e916ab541a2f7a2c6b637a Mon Sep 17 00:00:00 2001 From: ademboukabes Date: Tue, 2 Jun 2026 22:50:24 +0100 Subject: [PATCH 2/4] test(photo-approval): add unit tests for group photo lifecycle gaps --- tests/unit/test_photo_approval_lifecycle.py | 166 ++++++++++++++++++++ 1 file changed, 166 insertions(+) create mode 100644 tests/unit/test_photo_approval_lifecycle.py diff --git a/tests/unit/test_photo_approval_lifecycle.py b/tests/unit/test_photo_approval_lifecycle.py new file mode 100644 index 0000000..78ad71b --- /dev/null +++ b/tests/unit/test_photo_approval_lifecycle.py @@ -0,0 +1,166 @@ +import uuid +from unittest.mock import AsyncMock, MagicMock +import pytest + +from app.worker.photo_worker.main import PhotoWorker +from app.worker.photo_worker.schema.event import PhotoProcessEvent +from app.service.face_embedding import DetectedFace +from app.service.photo_approval import PhotoApprovalService +import numpy as np + +@pytest.fixture +def mock_conn(): + return AsyncMock() + +@pytest.fixture +def mock_face_embedding_service(): + return AsyncMock() + +@pytest.fixture +def mock_single_face_service(): + return AsyncMock() + +@pytest.fixture +def mock_notification_service(): + return AsyncMock() + +@pytest.fixture +def mock_photo_face_querier(): + return AsyncMock() + +@pytest.fixture +def mock_photo_querier(): + return AsyncMock() + +@pytest.fixture +def mock_photo_approval_querier(): + return AsyncMock() + +@pytest.fixture +def mock_processing_job_querier(): + return AsyncMock() + +@pytest.fixture +def mock_staged_upload_storage_service(): + return AsyncMock() + + +@pytest.mark.asyncio +async def test_group_photo_auto_approve_no_users( + mock_conn, + mock_face_embedding_service, + mock_single_face_service, + mock_notification_service, + mock_photo_face_querier, + mock_photo_querier, + mock_processing_job_querier, +): + """ + Test: Group photo with no enrolled users -> photo becomes approved + public. + """ + worker = PhotoWorker( + conn=mock_conn, + face_embedding_service=mock_face_embedding_service, + single_face_service=mock_single_face_service, + user_notification_service=mock_notification_service, + photo_face_querier=mock_photo_face_querier, + photo_querier=mock_photo_querier, + processing_job_querier=mock_processing_job_querier, + ) + + photo_id = uuid.uuid4() + event = PhotoProcessEvent(photo_id=photo_id, image_ref="test.jpg") + + # 2 faces detected + faces = [ + DetectedFace(bbox=np.array([0, 0, 10, 10]), embedding=np.array([0.1, 0.2])), + DetectedFace(bbox=np.array([10, 10, 20, 20]), embedding=np.array([0.3, 0.4])), + ] + + # No face matches any user -> insert_photo_face_with_approval returns None + mock_photo_face_querier.insert_photo_face_with_approval.return_value = None + + await worker._handle_group_photo(event, faces) + + # Verify notifications were NOT sent + mock_notification_service.create_notification.assert_not_called() + + # Verify photo is marked public and approved + mock_photo_querier.update_photo_status.assert_called_once_with(id=photo_id, status="approved") + mock_photo_querier.update_photo_visibility.assert_called_once_with(id=photo_id, visibility="public") + + +@pytest.mark.asyncio +async def test_group_photo_pending_with_enrolled_users( + mock_conn, + mock_face_embedding_service, + mock_single_face_service, + mock_notification_service, + mock_photo_face_querier, + mock_photo_querier, + mock_processing_job_querier, +): + """ + Test: Group photo with at least one enrolled user -> approval records created, notifications sent, photo stays pending. + """ + worker = PhotoWorker( + conn=mock_conn, + face_embedding_service=mock_face_embedding_service, + single_face_service=mock_single_face_service, + user_notification_service=mock_notification_service, + photo_face_querier=mock_photo_face_querier, + photo_querier=mock_photo_querier, + processing_job_querier=mock_processing_job_querier, + ) + + photo_id = uuid.uuid4() + event = PhotoProcessEvent(photo_id=photo_id, image_ref="test.jpg") + + faces = [ + DetectedFace(bbox=np.array([0, 0, 10, 10]), embedding=np.array([0.1, 0.2])), + ] + + # Mock DB returning an approval record + mock_approval = MagicMock() + mock_approval.user_id = uuid.uuid4() + mock_approval.photo_id = photo_id + mock_photo_face_querier.insert_photo_face_with_approval.return_value = mock_approval + + await worker._handle_group_photo(event, faces) + + # Verify notification WAS sent + mock_notification_service.create_notification.assert_called_once() + args, kwargs = mock_notification_service.create_notification.call_args + assert kwargs["user_id"] == mock_approval.user_id + + # Verify photo status is NOT updated to approved (stays pending) + mock_photo_querier.update_photo_status.assert_not_called() + mock_photo_querier.update_photo_visibility.assert_not_called() + + +@pytest.mark.asyncio +async def test_expire_stale_marks_photos_approved( + mock_photo_approval_querier, + mock_photo_querier, + mock_staged_upload_storage_service, +): + """ + Test: After PHOTO_APPROVAL_TIMEOUT_DAYS days, expire_stale marks photos approved + """ + service = PhotoApprovalService( + photo_approval_querier=mock_photo_approval_querier, + photo_querier=mock_photo_querier, + storage_service=mock_staged_upload_storage_service, + ) + + # Mock the generator for expire_stale_approvals + async def mock_generator(*args, **kwargs): + yield uuid.uuid4() + yield uuid.uuid4() + + mock_photo_approval_querier.expire_stale_approvals = MagicMock(side_effect=mock_generator) + + count = await service.expire_stale(timeout_days=7) + + assert count == 2 + mock_photo_approval_querier.expire_stale_approvals.assert_called_once_with(timeout_days=7) From 9b3883cfe9f360da51d7d70a6c32c270e79b278c Mon Sep 17 00:00:00 2001 From: ademboukabes Date: Tue, 2 Jun 2026 22:51:59 +0100 Subject: [PATCH 3/4] style: fix trailing whitespaces in tests --- tests/unit/test_photo_approval_lifecycle.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_photo_approval_lifecycle.py b/tests/unit/test_photo_approval_lifecycle.py index 78ad71b..e12281e 100644 --- a/tests/unit/test_photo_approval_lifecycle.py +++ b/tests/unit/test_photo_approval_lifecycle.py @@ -70,7 +70,7 @@ async def test_group_photo_auto_approve_no_users( photo_id = uuid.uuid4() event = PhotoProcessEvent(photo_id=photo_id, image_ref="test.jpg") - + # 2 faces detected faces = [ DetectedFace(bbox=np.array([0, 0, 10, 10]), embedding=np.array([0.1, 0.2])), @@ -115,7 +115,7 @@ async def test_group_photo_pending_with_enrolled_users( photo_id = uuid.uuid4() event = PhotoProcessEvent(photo_id=photo_id, image_ref="test.jpg") - + faces = [ DetectedFace(bbox=np.array([0, 0, 10, 10]), embedding=np.array([0.1, 0.2])), ] @@ -161,6 +161,6 @@ async def mock_generator(*args, **kwargs): mock_photo_approval_querier.expire_stale_approvals = MagicMock(side_effect=mock_generator) count = await service.expire_stale(timeout_days=7) - + assert count == 2 mock_photo_approval_querier.expire_stale_approvals.assert_called_once_with(timeout_days=7) From ba4b5f2ce51448da3bc8bdecdb357ef8f4cca333 Mon Sep 17 00:00:00 2001 From: ademboukabes Date: Tue, 2 Jun 2026 22:58:35 +0100 Subject: [PATCH 4/4] fix(test): resolve mypy type check errors and unused imports --- tests/unit/test_photo_approval_lifecycle.py | 68 ++++++++++----------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/tests/unit/test_photo_approval_lifecycle.py b/tests/unit/test_photo_approval_lifecycle.py index e12281e..52bcd68 100644 --- a/tests/unit/test_photo_approval_lifecycle.py +++ b/tests/unit/test_photo_approval_lifecycle.py @@ -6,55 +6,54 @@ from app.worker.photo_worker.schema.event import PhotoProcessEvent from app.service.face_embedding import DetectedFace from app.service.photo_approval import PhotoApprovalService -import numpy as np @pytest.fixture -def mock_conn(): +def mock_conn() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_face_embedding_service(): +def mock_face_embedding_service() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_single_face_service(): +def mock_single_face_service() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_notification_service(): +def mock_notification_service() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_photo_face_querier(): +def mock_photo_face_querier() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_photo_querier(): +def mock_photo_querier() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_photo_approval_querier(): +def mock_photo_approval_querier() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_processing_job_querier(): +def mock_processing_job_querier() -> AsyncMock: return AsyncMock() @pytest.fixture -def mock_staged_upload_storage_service(): +def mock_staged_upload_storage_service() -> AsyncMock: return AsyncMock() @pytest.mark.asyncio async def test_group_photo_auto_approve_no_users( - mock_conn, - mock_face_embedding_service, - mock_single_face_service, - mock_notification_service, - mock_photo_face_querier, - mock_photo_querier, - mock_processing_job_querier, -): + mock_conn: AsyncMock, + mock_face_embedding_service: AsyncMock, + mock_single_face_service: AsyncMock, + mock_notification_service: AsyncMock, + mock_photo_face_querier: AsyncMock, + mock_photo_querier: AsyncMock, + mock_processing_job_querier: AsyncMock, +) -> None: """ Test: Group photo with no enrolled users -> photo becomes approved + public. """ @@ -73,8 +72,8 @@ async def test_group_photo_auto_approve_no_users( # 2 faces detected faces = [ - DetectedFace(bbox=np.array([0, 0, 10, 10]), embedding=np.array([0.1, 0.2])), - DetectedFace(bbox=np.array([10, 10, 20, 20]), embedding=np.array([0.3, 0.4])), + DetectedFace(bbox=(0.0, 0.0, 10.0, 10.0), embedding=[0.1, 0.2]), + DetectedFace(bbox=(10.0, 10.0, 20.0, 20.0), embedding=[0.3, 0.4]), ] # No face matches any user -> insert_photo_face_with_approval returns None @@ -92,14 +91,14 @@ async def test_group_photo_auto_approve_no_users( @pytest.mark.asyncio async def test_group_photo_pending_with_enrolled_users( - mock_conn, - mock_face_embedding_service, - mock_single_face_service, - mock_notification_service, - mock_photo_face_querier, - mock_photo_querier, - mock_processing_job_querier, -): + mock_conn: AsyncMock, + mock_face_embedding_service: AsyncMock, + mock_single_face_service: AsyncMock, + mock_notification_service: AsyncMock, + mock_photo_face_querier: AsyncMock, + mock_photo_querier: AsyncMock, + mock_processing_job_querier: AsyncMock, +) -> None: """ Test: Group photo with at least one enrolled user -> approval records created, notifications sent, photo stays pending. """ @@ -117,7 +116,7 @@ async def test_group_photo_pending_with_enrolled_users( event = PhotoProcessEvent(photo_id=photo_id, image_ref="test.jpg") faces = [ - DetectedFace(bbox=np.array([0, 0, 10, 10]), embedding=np.array([0.1, 0.2])), + DetectedFace(bbox=(0.0, 0.0, 10.0, 10.0), embedding=[0.1, 0.2]), ] # Mock DB returning an approval record @@ -140,10 +139,10 @@ async def test_group_photo_pending_with_enrolled_users( @pytest.mark.asyncio async def test_expire_stale_marks_photos_approved( - mock_photo_approval_querier, - mock_photo_querier, - mock_staged_upload_storage_service, -): + mock_photo_approval_querier: AsyncMock, + mock_photo_querier: AsyncMock, + mock_staged_upload_storage_service: AsyncMock, +) -> None: """ Test: After PHOTO_APPROVAL_TIMEOUT_DAYS days, expire_stale marks photos approved """ @@ -153,8 +152,9 @@ async def test_expire_stale_marks_photos_approved( storage_service=mock_staged_upload_storage_service, ) + from typing import Any, AsyncIterator # Mock the generator for expire_stale_approvals - async def mock_generator(*args, **kwargs): + async def mock_generator(*args: Any, **kwargs: Any) -> AsyncIterator[uuid.UUID]: yield uuid.uuid4() yield uuid.uuid4()