Skip to content

Commit 18c1276

Browse files
committed
feat(api): add by-job-id PII approve/deny endpoints
POST /api/v1/documents/{job_id}/pii/approve POST /api/v1/documents/{job_id}/pii/deny The token-based equivalent at /api/v1/approval/{token}/decision works for the operator-driven flow that already exists. These by-job-id variants exist so machine clients (the reflow-canvas-lti Canvas connector) can forward a faculty decision after a status poll — they already authenticated with an API key and they already know the job_id, so a token round-trip isn't needed. Body shape mirrors ApprovalDecisionInput minus `decision` (implicit from the URL): `{justification?, reviewed_by}`. Response is the same shape as the token endpoint returns. Status codes: 200 approval recorded; quick_approve/quick_deny ran; background task continues with S3 cleanup / queue dispatch. 404 job_id not found. 409 job exists but isn't in `awaiting_approval` — race with another reviewer in a parallel tab, or a timeout sweeper already decided. The Canvas LTI connector surfaces this verbatim to faculty. 422 body validation (justification < 10 chars when present, reviewed_by missing) — FastAPI handles automatically. The 409 pre-check is intentional: quick_approve/quick_deny set the job status unconditionally, which is safe for the token-based endpoint (the token proves the job was in awaiting_approval at issue time) but unsafe by job_id where two concurrent calls could otherwise both succeed and overwrite each other. Tests cover happy approve, happy deny, 404, and 409, all against the live testcontainers stack via the integration tier. Verified locally with `pytest -m integration` inside the api-gateway image.
1 parent f6dbec8 commit 18c1276

2 files changed

Lines changed: 395 additions & 0 deletions

File tree

src/api/documents.py

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
get_storage_service,
2121
)
2222
from ..services import JobService, QueueService, S3URLService, StorageService
23+
from ..services.approval_service import ApprovalService
2324
from ..services.document_processing_service import DocumentProcessingService
2425
from ..services.metrics_service import jobs_submitted_total
2526
from .schemas import (
@@ -711,3 +712,201 @@ async def get_ledger(
711712
processing_duration_ms=0,
712713
final_markdown_url=final_markdown_url,
713714
)
715+
716+
717+
# ---------------------------------------------------------------------------
718+
# PII approval / denial — by-job-id endpoints.
719+
#
720+
# The token-based equivalent lives at /api/v1/approval/{token}/decision in
721+
# api.approval. These by-job-id variants exist so machine clients (e.g. the
722+
# reflow-canvas-lti connector) can forward a faculty decision without having
723+
# to juggle an approval token — they already authenticated with an API key
724+
# and they already know the job_id from a prior status poll.
725+
# ---------------------------------------------------------------------------
726+
727+
728+
class PIIDecisionInput(BaseModel):
729+
"""Input for the by-job-id PII decision endpoints."""
730+
731+
justification: str | None = Field(
732+
None,
733+
min_length=10,
734+
max_length=1000,
735+
description="Optional explanation for the decision (10–1000 chars when provided).",
736+
)
737+
reviewed_by: str = Field(
738+
...,
739+
min_length=3,
740+
description="Reviewer identifier (email or stable user id).",
741+
)
742+
743+
744+
class PIIDecisionResponse(BaseModel):
745+
"""Response for the by-job-id PII decision endpoints."""
746+
747+
message: str
748+
job_id: str
749+
decision: Literal["approved", "denied"]
750+
751+
752+
@router.post(
753+
"/{job_id}/pii/approve",
754+
response_model=PIIDecisionResponse,
755+
summary="Approve PII gate for a document (by job_id)",
756+
description=(
757+
"Records a faculty approval of the PII findings on a job that is in "
758+
"``awaiting_approval`` status, then resumes processing. Symmetric "
759+
"counterpart of ``POST /{job_id}/pii/deny``. Intended for connector "
760+
"consumption (the Canvas LTI connector forwards a faculty decision "
761+
"here after the panorama PII gate)."
762+
),
763+
)
764+
async def approve_pii(
765+
job_id: str,
766+
body: PIIDecisionInput,
767+
background_tasks: BackgroundTasks,
768+
redis_client: Any = Depends(get_redis_client),
769+
storage_service: StorageService = Depends(get_storage_service),
770+
s3_url_service: S3URLService = Depends(get_s3_url_service),
771+
) -> PIIDecisionResponse:
772+
return await _process_pii_decision(
773+
job_id=job_id,
774+
decision="approved",
775+
body=body,
776+
background_tasks=background_tasks,
777+
redis_client=redis_client,
778+
storage_service=storage_service,
779+
s3_url_service=s3_url_service,
780+
)
781+
782+
783+
@router.post(
784+
"/{job_id}/pii/deny",
785+
response_model=PIIDecisionResponse,
786+
summary="Deny PII gate for a document (by job_id)",
787+
description=(
788+
"Records a faculty denial of the PII findings on a job that is in "
789+
"``awaiting_approval`` status, then cleans up the document. Symmetric "
790+
"counterpart of ``POST /{job_id}/pii/approve``."
791+
),
792+
)
793+
async def deny_pii(
794+
job_id: str,
795+
body: PIIDecisionInput,
796+
background_tasks: BackgroundTasks,
797+
redis_client: Any = Depends(get_redis_client),
798+
storage_service: StorageService = Depends(get_storage_service),
799+
s3_url_service: S3URLService = Depends(get_s3_url_service),
800+
) -> PIIDecisionResponse:
801+
return await _process_pii_decision(
802+
job_id=job_id,
803+
decision="denied",
804+
body=body,
805+
background_tasks=background_tasks,
806+
redis_client=redis_client,
807+
storage_service=storage_service,
808+
s3_url_service=s3_url_service,
809+
)
810+
811+
812+
async def _process_pii_decision(
813+
*,
814+
job_id: str,
815+
decision: Literal["approved", "denied"],
816+
body: PIIDecisionInput,
817+
background_tasks: BackgroundTasks,
818+
redis_client: Any,
819+
storage_service: StorageService,
820+
s3_url_service: S3URLService,
821+
) -> PIIDecisionResponse:
822+
"""Shared body for approve_pii + deny_pii.
823+
824+
Pre-validates the job's current status (404 if missing, 409 if not
825+
awaiting_approval) before invoking the approval_service. That contract is
826+
what the Canvas LTI connector relies on to surface "another instructor
827+
decided in a parallel tab" as a 409 to the operator. The token-based
828+
sibling endpoint in api.approval lets quick_approve / quick_deny set the
829+
status unconditionally; that's safe there because the token already
830+
proves the job is in awaiting_approval, but it isn't safe by job_id.
831+
832+
Connector contract pinned by:
833+
https://github.com/oshrizak/reflow-canvas-lti — the Canvas LTI
834+
connector tries this endpoint first when forwarding a faculty PII
835+
decision; on 404/405 it falls back to
836+
``POST /api/v1/approval/{token}/decision`` so the connector keeps
837+
working against Core deployments that pre-date this PR. Both
838+
branches are covered in the connector's
839+
``tests/integration/test_pii_decision.py``
840+
(``test_pii_approve_prefers_by_job_id_endpoint`` +
841+
``test_pii_approve_falls_back_to_token_endpoint_on_405``).
842+
Net effect once this PR merges + Core ships: connector drops the
843+
approval-token round-trip and submits decisions in one POST.
844+
"""
845+
job_service = JobService(redis_client)
846+
queue_service = QueueService(redis_client)
847+
approval_service = ApprovalService(
848+
redis_client=redis_client,
849+
s3_client=None, # Lazy-loaded inside the background task on the denial path.
850+
job_service=job_service,
851+
queue_service=queue_service,
852+
storage_service=storage_service,
853+
s3_url_service=s3_url_service,
854+
)
855+
856+
job = await job_service.get_job(job_id)
857+
if not job:
858+
raise HTTPException(status_code=404, detail=f"Job {job_id} not found")
859+
860+
current_status = str(job.get("status") or "")
861+
if current_status != "awaiting_approval":
862+
raise HTTPException(
863+
status_code=409,
864+
detail=(
865+
f"Job {job_id} is in {current_status!r}, not 'awaiting_approval' "
866+
f"— a decision was already recorded."
867+
),
868+
)
869+
870+
s3_key = job.get("s3_key", "")
871+
justification = body.justification or ""
872+
873+
try:
874+
if decision == "approved":
875+
await approval_service.quick_approve(job_id)
876+
background_tasks.add_task(
877+
approval_service.process_approval_background,
878+
job_id=job_id,
879+
s3_key=s3_key,
880+
justification=justification,
881+
reviewed_by=body.reviewed_by,
882+
)
883+
return PIIDecisionResponse(
884+
message="Job approved - processing started",
885+
job_id=job_id,
886+
decision="approved",
887+
)
888+
889+
await approval_service.quick_deny(job_id)
890+
background_tasks.add_task(
891+
approval_service.process_denial_background,
892+
job_id=job_id,
893+
s3_key=s3_key,
894+
justification=justification,
895+
reviewed_by=body.reviewed_by,
896+
)
897+
return PIIDecisionResponse(
898+
message="Job denied - cleanup started",
899+
job_id=job_id,
900+
decision="denied",
901+
)
902+
except ValueError as exc:
903+
# Defensive: approval_service can raise ValueError on data checks
904+
# (e.g. missing s3_key). The job exists but isn't in a decidable
905+
# state — surface as 409 not 500.
906+
raise HTTPException(status_code=409, detail=str(exc)) from exc
907+
except Exception as exc: # noqa: BLE001
908+
logger.exception("PII decision (%s) failed for job=%s", decision, job_id)
909+
raise HTTPException(
910+
status_code=500,
911+
detail=f"Failed to process PII decision: {exc}",
912+
) from exc

0 commit comments

Comments
 (0)