diff --git a/app/api/routes/form_generation.py b/app/api/routes/form_generation.py index 2a4a956..b6070c4 100644 --- a/app/api/routes/form_generation.py +++ b/app/api/routes/form_generation.py @@ -38,7 +38,11 @@ ) from app.core.errors.base import AppError from app.db.repositories import get_form, list_forms_by_batch -from app.services.form_generation import FormGenerationService +from app.services.form_generation import ( + FormGenerationService, + download_filename, + form_version, +) router = APIRouter(prefix="/forms", tags=["forms"]) @@ -151,7 +155,9 @@ def download_form_pdf(form_id: UUID, db: Session = Depends(get_db)): if not path.is_relative_to(DATA_DIR) or not path.is_file(): raise AppError(f"Form {form_id} not found", status_code=404, error_code="FORM_NOT_FOUND") - return FileResponse(path, media_type="application/pdf", filename=path.name) + return FileResponse( + path, media_type="application/pdf", filename=download_filename(db, form) + ) @router.get("/{form_id}/json", response_model=FormMappedJson) @@ -160,15 +166,30 @@ def get_form_json(form_id: UUID, db: Session = Depends(get_db)): if not form: raise AppError(f"Form {form_id} not found", status_code=404, error_code="FORM_NOT_FOUND") - if not form.json_ready or form.json_data is None: + # Same three answers as /pdf, so a client polling both after one generate + # call reads them the same way: 500 once the fill failed, 202 while it is + # still running, the payload once it is there. + if form.status == FormStatus.failed: raise AppError( - f"Form {form_id} has no JSON output yet", - status_code=404, - error_code="FORM_JSON_NOT_READY", + f"Form {form_id} failed to generate", + status_code=500, + error_code="FORM_GENERATION_FAILED", + detail={"reason": "Form generation failed"}, + ) + + if not form.json_ready or form.json_data is None: + return JSONResponse( + status_code=202, + content={ + "message": "Form generation is still in progress", + "status": form.status, + "retry_after_seconds": FORM_GENERATION_POLL_INTERVAL_SECONDS, + }, ) return FormMappedJson( form_type=form.form_type, + form_version=form_version(db, form), form_id=form.form_id, template_id=form.template_id, incident_id=form.incident_id, diff --git a/app/api/schemas/form_generation.py b/app/api/schemas/form_generation.py index 6aac81f..3a7653e 100644 --- a/app/api/schemas/form_generation.py +++ b/app/api/schemas/form_generation.py @@ -96,6 +96,9 @@ class FormMappedJson(BaseModel): """GET /forms/{form_id}/json response.""" form_type: str + # The template's version, read at request time. Null when the template has + # since been deleted out of the registry. + form_version: str | None = None form_id: UUID template_id: UUID incident_id: UUID diff --git a/app/services/form_generation.py b/app/services/form_generation.py index 6c2931e..d2f8276 100644 --- a/app/services/form_generation.py +++ b/app/services/form_generation.py @@ -10,6 +10,7 @@ from __future__ import annotations +import re from dataclasses import dataclass, field from datetime import datetime, timezone from uuid import UUID, uuid4 @@ -22,6 +23,7 @@ from app.db.repositories import ( create_generated_form, create_job, + get_form_template, get_incident, update_form, update_job, @@ -31,6 +33,12 @@ from app.services.form_templates import require_template from app.tasks.generate_forms import generate_forms_batch_task +# Anything outside this set is replaced in a download filename. Incident +# numbers are free text typed by a responder, so they can carry slashes, +# spaces or quotes, none of which belong in a Content-Disposition header. +# Dots go too: the only one in the name should be the one before "pdf". +_UNSAFE_IN_FILENAME = re.compile(r"[^A-Za-z0-9_-]+") + @dataclass class SkippedTemplate: @@ -56,6 +64,36 @@ def _skip_reason(gaps) -> str: return f"Not ready: {gap.field_name} ({gap.source.value}) has no value" +def _slug(value: str) -> str: + return _UNSAFE_IN_FILENAME.sub("-", value).strip("-") + + +def download_filename(session: Session, form: Form) -> str: + """The name a downloaded PDF is saved under. + + "{form_type}_{incident_number}.pdf", the same name the batch zip gives its + entries, so a form downloaded on its own and the same form pulled out of a + batch land as one file. Incident numbers are optional and are only assigned + once the department has one, so the form id stands in when it is missing. + """ + incident = get_incident(session, form.incident_id) + number = _slug(incident.incident_number) if incident and incident.incident_number else "" + if not number: + return f"{form.form_id}.pdf" + return f"{_slug(form.form_type)}_{number}.pdf" + + +def form_version(session: Session, form: Form) -> str | None: + """The version of the template the form was generated from. + + Read live off the template rather than stamped on the form: templates are + versioned in place, so this reports the registry's current version, not the + one in force at fill time. + """ + template = get_form_template(session, form.template_id) + return template.version if template else None + + class FormGenerationService: def start_generation(self, session: Session, request: GenerateFormsRequest) -> GenerationResult: incident = get_incident(session, request.incident_id) diff --git a/contracts/path/forms.yaml b/contracts/path/forms.yaml index 7b4a5ab..154a0ef 100644 --- a/contracts/path/forms.yaml +++ b/contracts/path/forms.yaml @@ -137,7 +137,9 @@ form_pdf: description: | Returns the filled PDF binary file for the specified form. If the form generation is still in progress, returns 202 Accepted with a retry_after - hint. The Content-Disposition header is set for browser download. + hint. The Content-Disposition header is set for browser download, naming + the file "{form_type}_{incident_number}.pdf", the same name the batch zip + uses for its entries. Incidents without a number fall back to the form id. tags: - forms parameters: @@ -235,12 +237,40 @@ form_json: alarm_time: "13:52" acres_burned: 1247 cause: "natural" + "202": + description: Form generation still in progress + content: + application/json: + schema: + type: object + properties: + message: + type: string + status: + type: string + retry_after_seconds: + type: integer + example: + message: "Form generation is still in progress" + status: "processing" + retry_after_seconds: 5 "404": description: Form not found content: application/json: schema: $ref: "../schemas/common.yaml#/ErrorResponse" + "500": + description: Form generation failed, so there is no JSON to return + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + example: + error_code: "FORM_GENERATION_FAILED" + message: "Failed to generate form" + detail: + reason: "Template file corrupted or missing" batch_by_id: get: diff --git a/tests/test_v1_form_generation.py b/tests/test_v1_form_generation.py index 17142d5..f319f82 100644 --- a/tests/test_v1_form_generation.py +++ b/tests/test_v1_form_generation.py @@ -61,7 +61,7 @@ def _field(name, source="schema", required=True, layout=None, **extra) -> dict: return field -def _incident(db, contract=None) -> Incident: +def _incident(db, contract=None, incident_number=None) -> Incident: now = datetime.now(timezone.utc) inp = create_input( db, @@ -88,6 +88,7 @@ def _incident(db, contract=None) -> Incident: extract_id=extraction.extract_id, status=ReportStatus.draft, incident_contract=_CONTRACT if contract is None else contract, + incident_number=incident_number, ), ) @@ -443,26 +444,50 @@ def test_500_when_form_failed(self, client, db): assert resp.status_code == 500 assert resp.json()["error_code"] == "PDF_GENERATION_FAILED" - def test_200_serves_the_pdf_file(self, client, db, monkeypatch, tmp_path, pdf_bytes): - monkeypatch.setattr("app.api.routes.form_generation.DATA_DIR", tmp_path) - incident = _incident(db) - template = _template(db) - + def _completed_pdf_form(self, db, tmp_path, pdf_bytes, incident, template): pdf_file = tmp_path / "forms" / "generated" / "x.pdf" - pdf_file.parent.mkdir(parents=True) + pdf_file.parent.mkdir(parents=True, exist_ok=True) pdf_file.write_bytes(pdf_bytes) - - form = _form( + return _form( db, incident, template, status=FormStatus.completed, pdf_ready=True, pdf_path="forms/generated/x.pdf", ) + def test_200_serves_the_pdf_file(self, client, db, monkeypatch, tmp_path, pdf_bytes): + monkeypatch.setattr("app.api.routes.form_generation.DATA_DIR", tmp_path) + incident = _incident(db, incident_number="FF-2024-CA-0157") + template = _template(db) + form = self._completed_pdf_form(db, tmp_path, pdf_bytes, incident, template) + resp = client.get(f"{FORMS_URL}/{form.form_id}/pdf") assert resp.status_code == 200 assert resp.headers["content-type"] == "application/pdf" assert resp.content == pdf_bytes + assert 'filename="neris_FF-2024-CA-0157.pdf"' in resp.headers["content-disposition"] + + def test_filename_falls_back_to_form_id_without_an_incident_number( + self, client, db, monkeypatch, tmp_path, pdf_bytes + ): + monkeypatch.setattr("app.api.routes.form_generation.DATA_DIR", tmp_path) + incident = _incident(db) + template = _template(db) + form = self._completed_pdf_form(db, tmp_path, pdf_bytes, incident, template) + + resp = client.get(f"{FORMS_URL}/{form.form_id}/pdf") + assert f'filename="{form.form_id}.pdf"' in resp.headers["content-disposition"] + + def test_filename_strips_characters_an_incident_number_should_not_carry( + self, client, db, monkeypatch, tmp_path, pdf_bytes + ): + monkeypatch.setattr("app.api.routes.form_generation.DATA_DIR", tmp_path) + incident = _incident(db, incident_number='../2024 "07"/0157') + template = _template(db) + form = self._completed_pdf_form(db, tmp_path, pdf_bytes, incident, template) + + resp = client.get(f"{FORMS_URL}/{form.form_id}/pdf") + assert 'filename="neris_2024-07-0157.pdf"' in resp.headers["content-disposition"] def test_404_path_escaping_data_dir_is_rejected(self, client, db, monkeypatch, tmp_path): monkeypatch.setattr("app.api.routes.form_generation.DATA_DIR", tmp_path) @@ -500,15 +525,26 @@ def test_200_returns_agency_fields(self, client, db): body = resp.json() assert body["form_id"] == str(form.form_id) assert body["agency_fields"]["incident_name"] == "Bear Creek Wildfire" + assert body["form_version"] == template.version - def test_404_when_not_ready_yet(self, client, db): + def test_202_while_still_generating(self, client, db): incident = _incident(db) template = _template(db) form = _form(db, incident, template, status=FormStatus.queued) resp = client.get(f"{FORMS_URL}/{form.form_id}/json") - assert resp.status_code == 404 - assert resp.json()["error_code"] == "FORM_JSON_NOT_READY" + assert resp.status_code == 202 + assert resp.json()["status"] == "queued" + assert resp.json()["retry_after_seconds"] == 5 + + def test_500_when_form_failed(self, client, db): + incident = _incident(db) + template = _template(db) + form = _form(db, incident, template, status=FormStatus.failed) + + resp = client.get(f"{FORMS_URL}/{form.form_id}/json") + assert resp.status_code == 500 + assert resp.json()["error_code"] == "FORM_GENERATION_FAILED" def test_404_unknown_form(self, client, db): resp = client.get(f"{FORMS_URL}/{uuid4()}/json")