diff --git a/backend/app/api/approvals.py b/backend/app/api/approvals.py index ecb13a3a4..05b396172 100644 --- a/backend/app/api/approvals.py +++ b/backend/app/api/approvals.py @@ -417,6 +417,7 @@ async def create_approval( confidence=payload.confidence, rubric_scores=payload.rubric_scores, status=payload.status, + resolved_at=utcnow() if payload.status != "pending" else None, ) session.add(approval) await session.flush() @@ -443,29 +444,36 @@ async def update_approval( session: AsyncSession = SESSION_DEP, ) -> ApprovalRead: """Update an approval's status and resolution timestamp.""" - approval = await Approval.objects.by_id(approval_id).first(session) + try: + approval_lookup_id = UUID(approval_id) + except ValueError as exc: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) from exc + + approval = await Approval.objects.by_id(approval_lookup_id).first(session) if approval is None or approval.board_id != board.id: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) updates = payload.model_dump(exclude_unset=True) prior_status = approval.status if "status" in updates: target_status = updates["status"] - if target_status == "pending" and prior_status != "pending": - task_ids_by_approval = await load_task_ids_by_approval( - session, approval_ids=[approval.id] - ) - approval_task_ids = task_ids_by_approval.get(approval.id) - if not approval_task_ids and approval.task_id is not None: - approval_task_ids = [approval.task_id] - await _ensure_no_pending_approval_conflicts( - session, - board_id=board.id, - task_ids=approval_task_ids or [], - exclude_approval_id=approval.id, - ) - approval.status = target_status - if approval.status != "pending": - approval.resolved_at = utcnow() + if prior_status in ("approved", "rejected"): + # Legacy rows may be resolved without a timestamp; permit same-status backfill only. + if target_status == prior_status and approval.resolved_at is None: + approval.resolved_at = utcnow() + else: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail={ + "message": f"Approval is already {prior_status}.", + "approval_id": str(approval.id), + "current_status": prior_status, + "requested_status": target_status, + }, + ) + else: + approval.status = target_status + if approval.status != "pending": + approval.resolved_at = utcnow() session.add(approval) await session.commit() await session.refresh(approval) diff --git a/backend/tests/test_approvals_pending_conflicts.py b/backend/tests/test_approvals_pending_conflicts.py index 1d8199310..d071a6d95 100644 --- a/backend/tests/test_approvals_pending_conflicts.py +++ b/backend/tests/test_approvals_pending_conflicts.py @@ -9,6 +9,7 @@ from sqlmodel.ext.asyncio.session import AsyncSession from app.api import approvals as approvals_api +from app.models.approvals import Approval from app.models.boards import Board from app.models.organizations import Organization from app.models.tasks import Task @@ -131,23 +132,183 @@ async def test_create_approval_rejects_pending_conflict_from_linked_task_ids() - @pytest.mark.asyncio -async def test_update_approval_rejects_reopening_to_pending_with_existing_pending() -> None: +async def test_create_approval_sets_resolved_at_for_resolved_status() -> None: engine = await _make_engine() try: async with await _make_session(engine) as session: board, task_ids = await _seed_board_with_tasks(session, task_count=1) task_id = task_ids[0] - pending = await approvals_api.create_approval( + created = await approvals_api.create_approval( payload=ApprovalCreate( action_type="task.execute", task_id=task_id, - payload={"reason": "Primary pending approval is active."}, - confidence=83, - status="pending", + payload={"reason": "Resolved before persistence."}, + confidence=88, + status="approved", + ), + board=board, + session=session, + ) + + assert created.status == "approved" + assert created.resolved_at is not None + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_update_approval_backfills_resolved_at_for_legacy_resolved_approval() -> None: + engine = await _make_engine() + try: + async with await _make_session(engine) as session: + board, task_ids = await _seed_board_with_tasks(session, task_count=1) + task_id = task_ids[0] + approval = Approval( + board_id=board.id, + task_id=task_id, + action_type="task.execute", + payload={"reason": "Legacy resolved approval."}, + confidence=80, + status="approved", + ) + session.add(approval) + await session.commit() + + updated = await approvals_api.update_approval( + approval_id=str(approval.id), + payload=ApprovalUpdate(status="approved"), + board=board, + session=session, + ) + + assert updated.status == "approved" + assert updated.resolved_at is not None + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_update_approval_rejects_double_approve() -> None: + """Re-approving an already-approved approval must return 409.""" + engine = await _make_engine() + try: + async with await _make_session(engine) as session: + board, task_ids = await _seed_board_with_tasks(session, task_count=1) + task_id = task_ids[0] + approval = await approvals_api.create_approval( + payload=ApprovalCreate( + action_type="task.execute", + task_id=task_id, + payload={"reason": "Needs sign-off."}, + confidence=80, + status="approved", + ), + board=board, + session=session, + ) + + with pytest.raises(HTTPException) as exc: + await approvals_api.update_approval( + approval_id=str(approval.id), + payload=ApprovalUpdate(status="approved"), + board=board, + session=session, + ) + + assert exc.value.status_code == 409 + detail = exc.value.detail + assert isinstance(detail, dict) + assert detail["message"] == "Approval is already approved." + assert detail["current_status"] == "approved" + assert detail["requested_status"] == "approved" + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_update_approval_rejects_double_reject() -> None: + """Re-rejecting an already-rejected approval must return 409.""" + engine = await _make_engine() + try: + async with await _make_session(engine) as session: + board, task_ids = await _seed_board_with_tasks(session, task_count=1) + task_id = task_ids[0] + approval = await approvals_api.create_approval( + payload=ApprovalCreate( + action_type="task.execute", + task_id=task_id, + payload={"reason": "Needs sign-off."}, + confidence=80, + status="rejected", + ), + board=board, + session=session, + ) + + with pytest.raises(HTTPException) as exc: + await approvals_api.update_approval( + approval_id=str(approval.id), + payload=ApprovalUpdate(status="rejected"), + board=board, + session=session, + ) + + assert exc.value.status_code == 409 + detail = exc.value.detail + assert isinstance(detail, dict) + assert detail["message"] == "Approval is already rejected." + assert detail["current_status"] == "rejected" + assert detail["requested_status"] == "rejected" + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_update_approval_rejects_approving_rejected() -> None: + """Approving a rejected approval must return 409 (resolved approvals are immutable).""" + engine = await _make_engine() + try: + async with await _make_session(engine) as session: + board, task_ids = await _seed_board_with_tasks(session, task_count=1) + task_id = task_ids[0] + approval = await approvals_api.create_approval( + payload=ApprovalCreate( + action_type="task.execute", + task_id=task_id, + payload={"reason": "Needs sign-off."}, + confidence=80, + status="rejected", ), board=board, session=session, ) + + with pytest.raises(HTTPException) as exc: + await approvals_api.update_approval( + approval_id=str(approval.id), + payload=ApprovalUpdate(status="approved"), + board=board, + session=session, + ) + + assert exc.value.status_code == 409 + detail = exc.value.detail + assert isinstance(detail, dict) + assert detail["message"] == "Approval is already rejected." + assert detail["current_status"] == "rejected" + assert detail["requested_status"] == "approved" + finally: + await engine.dispose() + + +@pytest.mark.asyncio +async def test_update_approval_rejects_reopening_resolved_to_pending() -> None: + """Resolved approvals are immutable — cannot reopen to pending.""" + engine = await _make_engine() + try: + async with await _make_session(engine) as session: + board, task_ids = await _seed_board_with_tasks(session, task_count=1) + task_id = task_ids[0] resolved = await approvals_api.create_approval( payload=ApprovalCreate( action_type="task.review", @@ -162,7 +323,7 @@ async def test_update_approval_rejects_reopening_to_pending_with_existing_pendin with pytest.raises(HTTPException) as exc: await approvals_api.update_approval( - approval_id=resolved.id, # type: ignore[arg-type] + approval_id=str(resolved.id), payload=ApprovalUpdate(status="pending"), board=board, session=session, @@ -171,12 +332,8 @@ async def test_update_approval_rejects_reopening_to_pending_with_existing_pendin assert exc.value.status_code == 409 detail = exc.value.detail assert isinstance(detail, dict) - assert detail["message"] == "Each task can have only one pending approval." - assert detail["conflicts"] == [ - { - "task_id": str(task_id), - "approval_id": str(pending.id), - }, - ] + assert detail["message"] == "Approval is already approved." + assert detail["current_status"] == "approved" + assert detail["requested_status"] == "pending" finally: await engine.dispose()