feat: store source_artefact_id and add flat file upload to /v1/publication (#797) - #833
Conversation
…ension Replaces the file_extension column on the artefact table with source_artefact_id which stores the original uploaded file name. Downloads now use the original name instead of the artefact UUID. Migration backfills existing rows and drops the old column. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…quest body When provided, the value is stored as source_artefact_id on the artefact row instead of the default "upload.json". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…v1/publication Blob is still stored as upload.json; only the DB column uses the supplied value or empty string when source_artefact_id is absent from the request body. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds multipart/form-data flat file ingestion path to the publication endpoint. A request without a `type` form field is treated as a flat file upload (vs PDDA HTML which requires `type`). Metadata fields are sent as form parts; the file is stored in blob storage with isFlatFile=true. Extracts shared validation into validateCommonFields to avoid duplicating the nine common field checks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lobIngestion Covers all branches of the two new functions added for flat file upload: validation of common fields without hearing_list, source_artefact_id handling, isFlatFile flag, no_match path, error logging, and locale mapping. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirrors the existing blob-ingestion.spec.ts structure. Three journey tests cover authentication validation (missing/invalid/malformed tokens), payload validation (missing file, missing required fields, invalid enums, hearing_list not required), and successful ingestion with both MANUAL_UPLOAD and SNL provenance. All tests are @nightly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR replaces artefact file-extension storage with source artefact IDs, adds flat-file multipart ingestion to ChangesSource artefact ID migration and flat-file ingestion
Sequence Diagram(s)sequenceDiagram
participant Client
participant PublicationAPI
participant IngestionService
participant BlobStorage
participant ArtefactRepository
Client->>PublicationAPI: Multipart flat-file upload
PublicationAPI->>IngestionService: File buffer and metadata
IngestionService->>BlobStorage: Save uploaded file
IngestionService->>ArtefactRepository: Store source artefact ID
ArtefactRepository-->>IngestionService: Updated artefact
IngestionService-->>PublicationAPI: Ingestion result
PublicationAPI-->>Client: HTTP response
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results84 tests 52 ✅ 6m 16s ⏱️ Results for commit 9f47396. ♻️ This comment has been updated with latest results. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/pages/(admin)/non-strategic-upload-summary/index.ts (1)
130-155: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
updateSourceArtefactIdis not called when an Excel file has no converter.When
isExcelFile && selectedListType?.isNonStrategicis true but neithercanConvertByIdnorcanConvertByNameis true, the innerifblock (lines 139–155) is skipped and theelsebranch (line 156) is not entered either. This leavessourceArtefactIdnull in the database.The blob was already saved at line 123 as
${artefactId}${path.extname(uploadData.fileName)}(e.g.,${artefactId}.xlsx), but downstreamgetSourceArtefactIdwill fall back to${artefactId}.pdf, causinggetFileBufferto download a non-existent blob.🐛 Proposed fix: add else clause for no-converter path
if (canConvertById || canConvertByName) { jsonData = canConvertById ? await convertExcelForListType(listTypeId, uploadData.file) : await convertExcelForListTypeName(listTypeName!, uploadData.file); await saveUploadedFile(artefactId, `${artefactId}.json`, Buffer.from(JSON.stringify(jsonData))); await updateSourceArtefactId(artefactId, `${artefactId}.json`); // Extract and store artefact search data from converted JSON try { await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData); } catch (error) { console.error("[Non-Strategic Upload] Failed to extract artefact search data from converted Excel", { artefactId, error: error instanceof Error ? error.message : String(error) }); } + } else { + await updateSourceArtefactId(artefactId, uploadData.fileName); } } else {libs/publication/src/repository/queries.ts (1)
169-186: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDelete the JSON blob when
sourceArtefactIdis empty.deleteArtefacts()currently falls back tosourceArtefactId, but JSON artefacts are stored as${artefactId}.json, so the real blob is left orphaned. Use the artefact type to choose the default extension here.
🧹 Nitpick comments (10)
libs/api/src/blob-ingestion/validation.ts (1)
146-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid duplicate
findAllListTypes()database query.
validateCommonFieldscallsfindAllListTypes()at line 49, andvalidateBlobRequestcalls it again at line 154. Each JSON ingestion validation triggers two database queries for the same list type data.Return
listTypesfromvalidateCommonFieldssovalidateBlobRequestcan reuse the already-fetched result.♻️ Proposed refactor
async function validateCommonFields(request: FlatFileIngestionRequest, payloadSize: number): Promise<BlobValidationResult & { listTypes: Awaited<ReturnType<typeof findAllListTypes>> }> { const errors: ValidationError[] = []; // ... existing validation code ... const listTypes = await findAllListTypes(); // ... existing list type lookup code ... return { isValid: errors.length === 0, errors, locationExists, listTypeId: listTypeId ? Number.parseInt(listTypeId, 10) : undefined, resolvedLocationId, + listTypes }; } export async function validateBlobRequest(request: BlobIngestionRequest, rawBodySize: number): Promise<BlobValidationResult> { const result = await validateCommonFields(request, rawBodySize); const errors = [...result.errors]; if (!request.hearing_list) { errors.push({ field: "hearing_list", message: "hearing_list is required" }); } - const listTypes = await findAllListTypes(); + const listTypes = result.listTypes; const listTypeId = result.listTypeId; if (listTypeId && request.hearing_list && errors.length === 0) { try { const listTypesInfo = listTypes.map((lt) => ({ id: lt.id, name: lt.name, friendlyName: lt.friendlyName })); const validationResult = await validateListTypeJson(listTypeId.toString(), request.hearing_list, listTypesInfo); if (!validationResult.isValid) { for (const error of validationResult.errors) { errors.push({ field: "hearing_list", message: (error as { message?: string }).message || "Invalid hearing_list structure" }); } } } catch (_error) { errors.push({ field: "hearing_list", message: "Failed to validate hearing_list against schema" }); } } - return { - ...result, - isValid: errors.length === 0, - errors - }; + const { listTypes: _, ...resultWithoutListTypes } = result; + return { + ...resultWithoutListTypes, + isValid: errors.length === 0, + errors + }; }apps/web/src/pages/(admin)/manual-upload-summary/index.test.ts (1)
83-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
updateSourceArtefactIdcalls.
updateSourceArtefactIdis mocked at line 83 but not imported (line 109 omits it), so no test verifies it is called with(artefactId, uploadData.fileName). This is the core behaviour introduced by this PR — the original filename should be persisted tosourceArtefactId. At minimum, the success-path tests (e.g., lines 458–495, 817–857) should assertupdateSourceArtefactIdwas called with the correct arguments.Additionally,
saveUploadedFileis mocked to return string values (e.g.,mockResolvedValue(".pdf")at lines 460, 634, 666, etc.) but the actual function now returnsPromise<void>. These stale return values should be updated tomockResolvedValue(undefined)to match the current contract.♻️ Proposed fix: import updateSourceArtefactId and add assertions
import { createArtefact, extractAndStoreArtefactSearch, processPublication } from "`@hmcts/publication`"; +import { updateSourceArtefactId } from "`@hmcts/publication`";Then in the success-path test (e.g., lines 458–495):
expect(saveUploadedFile).toHaveBeenCalledWith("test-artefact-id-123", "test-hearing-list.pdf", mockUploadData.file); + expect(updateSourceArtefactId).toHaveBeenCalledWith("test-artefact-id-123", "test-hearing-list.pdf"); expect(createArtefact).toHaveBeenCalledWith(And update stale
saveUploadedFilemocks:- vi.mocked(saveUploadedFile).mockResolvedValue(".pdf"); + vi.mocked(saveUploadedFile).mockResolvedValue(undefined);apps/web/src/pages/(admin)/non-strategic-upload-summary/index.test.ts (1)
95-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
updateSourceArtefactIdand fix stalesaveUploadedFilemock.
updateSourceArtefactIdis mocked at line 98 but not imported (line 106 omits it), so no test verifies it is called. The Excel conversion test (lines 503–533) should assertupdateSourceArtefactIdwas called with("artefact-id-123", "artefact-id-123.json"), and the non-Excel success test (lines 297–353) should assert it was called with("artefact-id-123", "test.xlsx").Additionally, the
saveUploadedFilemock at line 91 returnsPromise.resolve(".xlsx"), but the actual function now returnsPromise<void>. This should be updated toPromise.resolve()to match the current contract.♻️ Proposed fix
import { createArtefact, extractAndStoreArtefactSearch, processPublication } from "`@hmcts/publication`"; +import { updateSourceArtefactId } from "`@hmcts/publication`";Update the mock factory:
- saveUploadedFile: vi.fn(() => Promise.resolve(".xlsx")) + saveUploadedFile: vi.fn(() => Promise.resolve())Add assertion in the Excel conversion test (after line 531):
expect(extractAndStoreArtefactSearch).toHaveBeenCalledWith("artefact-id-123", 7, { cases: [] }); + expect(updateSourceArtefactId).toHaveBeenCalledWith("artefact-id-123", "artefact-id-123.json"); expect(res.redirect).toHaveBeenCalledWith("/non-strategic-upload-success");Add assertion in the non-Excel success test (after line 339):
expect(saveUploadedFile).toHaveBeenCalledWith("artefact-id-123", "test.xlsx", mockUploadData.file); + expect(updateSourceArtefactId).toHaveBeenCalledWith("artefact-id-123", "test.xlsx"); expect(processPublication).toHaveBeenCalledWith(apps/api/src/routes/v1/publication.ts (2)
34-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared field validation to reduce type-guard duplication.
isFlatFileIngestionRequestandisBlobIngestionRequest(lines 15–32) share 8 identical field checks. Extracting a common helper would ensure both guards stay in sync when new required metadata fields are added.♻️ Suggested refactor
+function hasRequiredMetadataFields(req: Record<string, unknown>): boolean { + return ( + typeof req.court_id === "string" && + typeof req.provenance === "string" && + typeof req.content_date === "string" && + typeof req.list_type === "string" && + typeof req.sensitivity === "string" && + typeof req.language === "string" && + typeof req.display_from === "string" && + typeof req.display_to === "string" + ); +} + function isBlobIngestionRequest(body: unknown): body is BlobIngestionRequest { if (typeof body !== "object" || body === null) { return false; } - const req = body as Record<string, unknown>; - return ( - typeof req.court_id === "string" && - typeof req.provenance === "string" && - typeof req.content_date === "string" && - typeof req.list_type === "string" && - typeof req.sensitivity === "string" && - typeof req.language === "string" && - typeof req.display_from === "string" && - typeof req.display_to === "string" && - req.hearing_list !== undefined - ); + const req = body as Record<string, unknown>; + return hasRequiredMetadataFields(req) && req.hearing_list !== undefined; } function isFlatFileIngestionRequest(body: unknown): body is FlatFileIngestionRequest { if (typeof body !== "object" || body === null) { return false; } const req = body as Record<string, unknown>; - return ( - typeof req.court_id === "string" && - typeof req.provenance === "string" && - typeof req.content_date === "string" && - typeof req.list_type === "string" && - typeof req.sensitivity === "string" && - typeof req.language === "string" && - typeof req.display_from === "string" && - typeof req.display_to === "string" - ); + return hasRequiredMetadataFields(req); }
71-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared response mapping.
Both
handleJsonBlobIngestionandhandleFlatFileUploadcontain identical status-code mapping logic (no_match→200, "Validation failed"→400, other→500, success→201). A shared helper would centralise this contract and prevent drift.♻️ Suggested refactor
+function sendIngestionResponse(res: Response, result: BlobIngestionResponse): Response { + if (!result.success) { + if ("no_match" in result && result.no_match) { + return res.status(200).json(result); + } + if (result.message === "Validation failed") { + return res.status(400).json(result); + } + return res.status(500).json(result); + } + return res.status(201).json(result); +}Also applies to: 148-159
libs/public-pages/src/flat-file/flat-file-service.ts (1)
26-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEliminate redundant
getSourceArtefactIdDB query in both functions.
getFlatFileForDisplayandgetFileForDownloadeach callgetFileBuffer(which internally callsgetSourceArtefactId) and then callgetSourceArtefactIdagain separately. This results in 2 DB queries for the same data on every successful flat-file access.Adding an optional
sourceArtefactIdparameter togetFileBufferallows the caller to pass a pre-fetched value, avoiding the duplicate query.♻️ Proposed fix for `getFileBuffer` in file-retrieval.ts
-export async function getFileBuffer(artefactId: string): Promise<Buffer | null> { - const sourceArtefactId = await getSourceArtefactId(artefactId); +export async function getFileBuffer(artefactId: string, sourceArtefactId?: string): Promise<Buffer | null> { + const sourceId = sourceArtefactId ?? await getSourceArtefactId(artefactId); const extension = path.extname(sourceId) || ".pdf"; return downloadBlob(`${artefactId}${extension}`, CONTAINER.ARTEFACT);♻️ Proposed fix for `getFlatFileForDisplay` in flat-file-service.ts
const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); - const fileBuffer = await getFileBuffer(artefact.artefactId); + const fileBuffer = await getFileBuffer(artefact.artefactId, sourceArtefactId); if (!fileBuffer) { return { error: "FILE_NOT_FOUND" as const }; } const location = await getLocationById(Number.parseInt(artefact.locationId, 10)); const listType = await findListTypeById(artefact.listTypeId); const courtName = locale === "cy" ? location?.welshName || location?.name || "Unknown" : location?.name || "Unknown"; const listTypeName = locale === "cy" ? listType?.welshFriendlyName || "Unknown" : listType?.friendlyName || "Unknown"; - const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); - return { success: true, artefactId: artefact.artefactId, courtName, listTypeName, contentDate: artefact.contentDate, language: artefact.language, sourceArtefactId };♻️ Proposed fix for `getFileForDownload` in flat-file-service.ts
const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); - const fileBuffer = await getFileBuffer(artefact.artefactId); + const fileBuffer = await getFileBuffer(artefact.artefactId, sourceArtefactId); if (!fileBuffer) { return { error: "FILE_NOT_FOUND" as const }; } - const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); - return { success: true, fileBuffer, contentType: getContentType(path.extname(sourceArtefactId) || ".pdf"), fileName: getFileName(sourceArtefactId) };Also applies to: 67-80
libs/api/src/blob-ingestion/repository/service.test.ts (1)
683-701: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo assertion on
processPublication's payload for flat files.Existing tests check
locale/locationIdviaobjectContaining, but none assert whatjsonData/flatFilePathis passed for flat-file ingestion. Given the concern raised inservice.ts(Lines 229-251) aboutprocessPublicationreceiving an emptyjsonDataobject instead offlatFilePath, a test pinning the exact call payload would catch regressions here.libs/publication/src/repository/queries.ts (1)
180-185: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
"statusCode" in errorcan throw on non-object rejections.If
deleteBlobever rejects with a non-object value (e.g. a string),"statusCode" in errorthrows aTypeErrorinside this unhandled/fire-and-forget.catchcallback, turning a benign 404 case into an unhandled rejection. Guard with a type check first.🛡️ Suggested fix
- if (!("statusCode" in error) || (error as { statusCode: number }).statusCode !== 404) { + if (!(error instanceof Object) || !("statusCode" in error) || (error as { statusCode: number }).statusCode !== 404) { console.error(`Failed to delete PDF blob for artefact ${artefact.artefactId}:`, error); }libs/publication/src/repository/queries.test.ts (1)
589-622: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-
sourceArtefactIdcase.Current tests cover
"upload.json",null, and an explicit flat-file name, but notsourceArtefactId: ""— the default stored for JSON artefacts whensource_artefact_idis omitted. This is the scenario flagged inqueries.ts'sdeleteArtefacts(Lines 169-186) where the wrong blob extension (.json) would be targeted for deletion.libs/admin-pages/src/manual-upload/file-storage.ts (1)
4-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating duplicated
saveUploadedFileinto a shared package.This function is byte-for-byte identical to the one in
libs/api/src/blob-ingestion/file-storage.ts. Both copies were updated identically in this PR. Extracting it into a shared@hmcts/*package would prevent future divergence and reduce maintenance burden.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1695f28-8570-49dc-89e6-d034dd96c74c
📒 Files selected for processing (34)
apps/api/src/routes/v1/publication.test.tsapps/api/src/routes/v1/publication.tsapps/postgres/prisma/migrations/20260707000000_replace_file_extension_with_source_artefact_id/migration.sqlapps/web/src/pages/(admin)/manual-upload-summary/index.test.tsapps/web/src/pages/(admin)/manual-upload-summary/index.tsapps/web/src/pages/(admin)/non-strategic-upload-summary/index.test.tsapps/web/src/pages/(admin)/non-strategic-upload-summary/index.tsapps/web/src/pages/(public)/hearing-lists/[locationId]/[artefactId]/index.test.tsapps/web/src/pages/(public)/hearing-lists/[locationId]/[artefactId]/index.tsdocs/tickets/797/plan.mddocs/tickets/797/review.mddocs/tickets/797/tasks.mddocs/tickets/797/ticket.mde2e-tests/tests/api/flat-file-ingestion.spec.tse2e-tests/tests/flat-file-viewing.spec.tse2e-tests/utils/test-support-api.tslibs/admin-pages/src/manual-upload/file-storage.test.tslibs/admin-pages/src/manual-upload/file-storage.tslibs/api/src/blob-ingestion/file-storage.test.tslibs/api/src/blob-ingestion/file-storage.tslibs/api/src/blob-ingestion/repository/model.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/api/src/blob-ingestion/repository/service.tslibs/api/src/blob-ingestion/validation.test.tslibs/api/src/blob-ingestion/validation.tslibs/postgres-prisma/prisma/schema/base.prismalibs/public-pages/src/flat-file/flat-file-service.test.tslibs/public-pages/src/flat-file/flat-file-service.tslibs/publication/src/file-storage/file-retrieval.test.tslibs/publication/src/file-storage/file-retrieval.tslibs/publication/src/index.tslibs/publication/src/repository/queries.test.tslibs/publication/src/repository/queries.tslibs/test-support/src/routes/test-support/flat-files.ts
| if (!noMatch) { | ||
| processPublication({ | ||
| artefactId, | ||
| locationId, | ||
| listTypeId: validation.listTypeId, | ||
| contentDate: new Date(request.content_date), | ||
| locale: request.language === "WELSH" ? "cy" : "en", | ||
| jsonData: {} as CauseListData, | ||
| provenance: PROVENANCE_MAP[request.provenance] || request.provenance, | ||
| sensitivity: request.sensitivity, | ||
| language: request.language, | ||
| displayFrom: new Date(request.display_from), | ||
| displayTo: new Date(request.display_to), | ||
| isUpdate, | ||
| logPrefix: "[flat-file-ingestion]" | ||
| }).catch((error) => { | ||
| console.error("[flat-file-ingestion] Failed to process publication:", { | ||
| artefactId, | ||
| courtId: request.court_id, | ||
| error: error instanceof Error ? error.message : String(error) | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the implementation around the cited lines.
git ls-files 'libs/api/src/blob-ingestion/**' 'libs/api/src/**/service.test.ts' | sed -n '1,200p'
echo
echo '--- outline: service.ts ---'
ast-grep outline libs/api/src/blob-ingestion/repository/service.ts --view expanded
echo
echo '--- outline: processPublication implementation file(s) if reachable by search ---'
rg -n "function processPublication|const processPublication|processPublication\\(" libs/api/src -g '!**/dist/**' -g '!**/build/**'
echo
echo '--- tests mentioning flat-file ingestion / processPublication / flatFilePath / jsonData ---'
rg -n "flat-file-ingestion|flatFilePath|jsonData: \\{\\} as CauseListData|processPublication|sendThirdPartyPublications|generatePublicationPdf" libs/api/src -g '*test.ts' -g '*spec.ts'Repository: hmcts/cath-service
Length of output: 3489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding implementation for the flat-file path and the publication call.
sed -n '157,260p' libs/api/src/blob-ingestion/repository/service.ts
echo
echo '--- service.test.ts around flat-file expectations ---'
sed -n '300,520p' libs/api/src/blob-ingestion/repository/service.test.ts
echo
echo '--- search for publication package source in repo ---'
git ls-files | rg '^libs/.*/publication|^packages/.*/publication|publication/src|`@hmcts/publication`'
echo
echo '--- locate processPublication / generatePublicationPdf / sendThirdPartyPublications definitions ---'
rg -n "export function processPublication|function processPublication|generatePublicationPdf|sendThirdPartyPublications|extractAndStoreArtefactSearch" . -g '!**/dist/**' -g '!**/build/**'Repository: hmcts/cath-service
Length of output: 28757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the publication processing flow and tests for empty jsonData / flatFilePath handling.
sed -n '440,530p' libs/publication/src/processing/service.ts
echo
echo '--- generatePublicationPdf / processPublication tests around jsonData and flatFilePath ---'
sed -n '900,1110p' libs/publication/src/processing/service.test.tsRepository: hmcts/cath-service
Length of output: 9681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect PDF generation to see whether empty jsonData can fail or produce output.
sed -n '275,440p' libs/publication/src/processing/service.ts
echo
echo '--- generatePublicationPdf tests around empty/invalid jsonData ---'
rg -n "empty|invalid|jsonData.*undefined|jsonData.*\\{\\}|generatePublicationPdf\\(" libs/publication/src/processing/service.test.tsRepository: hmcts/cath-service
Length of output: 8419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how third-party fulfilment treats jsonData and flatFilePath.
sed -n '1,220p' libs/legacy-third-party-fulfilment/src/service.ts
echo
echo '--- tests for flatFilePath / jsonData in third-party fulfilment ---'
sed -n '1,180p' libs/legacy-third-party-fulfilment/src/service.test.tsRepository: hmcts/cath-service
Length of output: 11619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect pushWithRetry to determine how flatFilePath is used.
sed -n '1,240p' libs/legacy-third-party-fulfilment/src/push/retry.ts
echo
echo '--- tests around pushWithRetry file-path handling ---'
sed -n '1,220p' libs/legacy-third-party-fulfilment/src/push/retry.test.tsRepository: hmcts/cath-service
Length of output: 7361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how executePush uses pdfPath / flatFilePath in the outgoing request.
sed -n '1,260p' libs/legacy-third-party-fulfilment/src/push/http-client.ts
echo
echo '--- tests around executePush file-path payloads ---'
sed -n '1,260p' libs/legacy-third-party-fulfilment/src/push/http-client.test.tsRepository: hmcts/cath-service
Length of output: 13425
Use flatFilePath for flat-file pushes. processPublication only guards PDF/search generation on jsonData; it still forwards flatFilePath to Courtel, and the push layer uses that path to build the multipart file body. Passing {} here is the wrong shape for flat-file ingestion, and fileName should be forwarded instead.
Suggested fix
- jsonData: {} as CauseListData,
+ flatFilePath: fileName,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!noMatch) { | |
| processPublication({ | |
| artefactId, | |
| locationId, | |
| listTypeId: validation.listTypeId, | |
| contentDate: new Date(request.content_date), | |
| locale: request.language === "WELSH" ? "cy" : "en", | |
| jsonData: {} as CauseListData, | |
| provenance: PROVENANCE_MAP[request.provenance] || request.provenance, | |
| sensitivity: request.sensitivity, | |
| language: request.language, | |
| displayFrom: new Date(request.display_from), | |
| displayTo: new Date(request.display_to), | |
| isUpdate, | |
| logPrefix: "[flat-file-ingestion]" | |
| }).catch((error) => { | |
| console.error("[flat-file-ingestion] Failed to process publication:", { | |
| artefactId, | |
| courtId: request.court_id, | |
| error: error instanceof Error ? error.message : String(error) | |
| }); | |
| }); | |
| } | |
| if (!noMatch) { | |
| processPublication({ | |
| artefactId, | |
| locationId, | |
| listTypeId: validation.listTypeId, | |
| contentDate: new Date(request.content_date), | |
| locale: request.language === "WELSH" ? "cy" : "en", | |
| flatFilePath: fileName, | |
| provenance: PROVENANCE_MAP[request.provenance] || request.provenance, | |
| sensitivity: request.sensitivity, | |
| language: request.language, | |
| displayFrom: new Date(request.display_from), | |
| displayTo: new Date(request.display_to), | |
| isUpdate, | |
| logPrefix: "[flat-file-ingestion]" | |
| }).catch((error) => { | |
| console.error("[flat-file-ingestion] Failed to process publication:", { | |
| artefactId, | |
| courtId: request.court_id, | |
| error: error instanceof Error ? error.message : String(error) | |
| }); | |
| }); | |
| } |
| const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); | ||
|
|
||
| return { | ||
| success: true, | ||
| fileBuffer, | ||
| contentType: getContentType(fileExtension), | ||
| fileName: getFileName(artefact.artefactId, fileExtension) | ||
| contentType: getContentType(path.extname(sourceArtefactId) || null), | ||
| fileName: getFileName(sourceArtefactId) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use ".pdf" fallback instead of null for content-type derivation.
getFileForDownload uses path.extname(sourceArtefactId) || null, but getFileExtension and getFileBuffer both use || ".pdf". When sourceArtefactId has no extension, getContentType(null) returns "application/octet-stream", while getContentTypeFromExtension("") returns "application/pdf". This causes the download content type to diverge from the blob name (<artefactId>.pdf) and the hearing-lists page controller's PDF detection logic.
🐛 Proposed fix
- contentType: getContentType(path.extname(sourceArtefactId) || null),
+ contentType: getContentType(path.extname(sourceArtefactId) || ".pdf"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); | |
| return { | |
| success: true, | |
| fileBuffer, | |
| contentType: getContentType(fileExtension), | |
| fileName: getFileName(artefact.artefactId, fileExtension) | |
| contentType: getContentType(path.extname(sourceArtefactId) || null), | |
| fileName: getFileName(sourceArtefactId) | |
| const sourceArtefactId = await getSourceArtefactId(artefact.artefactId); | |
| return { | |
| success: true, | |
| fileBuffer, | |
| contentType: getContentType(path.extname(sourceArtefactId) || ".pdf"), | |
| fileName: getFileName(sourceArtefactId) |
…ategic uploads For Excel uploads, source_artefact_id now records the original uploaded file name (e.g. hearing-list.xlsx) instead of the synthetic JSON blob name. The Excel file is no longer saved to blob storage — only the converted JSON is stored, since the Excel itself has no value after conversion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…tension Non-strategic Excel uploads store the original .xlsx filename in source_artefact_id but the blob in storage is the converted .json. getFileBuffer now falls back to <artefactId>.json when the primary extension-based lookup returns nothing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
# Conflicts: # apps/web/src/pages/(admin)/non-strategic-upload-summary/index.ts
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Merged import sets from both branches — keeps getSourceArtefactId (HEAD) alongside canAccessPublicationData, getFileExtension, and resolveListType (master). Fixes listTypeInfo variable name and removes duplicate null check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| export async function processFlatFileBlobIngestion(request: FlatFileIngestionRequest, file: Buffer, fileSize: number): Promise<BlobIngestionResponse> { |
There was a problem hiding this comment.
This new method for flat file is very similar to the previous method processBlobIngestion(). Can we move some of the common areas to new methods?
There was a problem hiding this comment.
thanks Kian, I didn't realised. refactoring is done.
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…duplication Pulls four repeated patterns — validation failure logging, missing list-type error handling, artefact param construction, and fire-and-forget processPublication — into dedicated functions shared by processBlobIngestion and processFlatFileBlobIngestion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…SJP storage
- Store artefact blobs using artefactId only (no file extension) for both
frontend and /v1/publication API uploads; add backward-compat fallback
in download and delete paths for legacy blobs
- Store null instead of empty string in source_artefact_id when not provided
via the API (use || null to handle empty string from multipart fields)
- Widen updateSourceArtefactId signature to accept string | null
- Fix SJP service to read JSON from blob storage instead of local temp dir
- Fix flat file ingestion to skip PDF generation (was passing {} as jsonData)
- Set lastReceivedDate explicitly on artefact create instead of relying on
DB default
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/pages/(admin)/non-strategic-upload-summary/index.ts (1)
132-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle missing converters to prevent empty artefacts.
If a non-strategic Excel file is uploaded but no converter is found for its list type, the execution skips both the
ifbody and the outerelseblock. This results in the file not being saved to blob storage and thesource_artefact_idnot being updated, leaving an orphaned artefact record in the database.Throw an error to explicitly handle this scenario and prevent silent failures.
🐛 Proposed fix
if (listTypeName && hasConverterForListTypeName(listTypeName)) { jsonData = await convertExcelForListTypeName(listTypeName, uploadData.file); // Store converted JSON in blob — original Excel is not stored (no value after conversion) await saveUploadedFile(artefactId, artefactId, Buffer.from(JSON.stringify(jsonData))); // Track the original uploaded Excel file name, not the synthetic JSON blob name await updateSourceArtefactId(artefactId, uploadData.fileName); // Extract and store artefact search data from converted JSON try { await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData); } catch (error) { console.error("[Non-Strategic Upload] Failed to extract artefact search data from converted Excel", { artefactId, error: error instanceof Error ? error.message : String(error) }); } + } else { + throw new Error(`No converter found for list type: ${listTypeName}`); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4186f70d-05e4-4919-9fb4-10a14079326c
📒 Files selected for processing (17)
apps/web/src/pages/(admin)/manual-upload-summary/index.test.tsapps/web/src/pages/(admin)/manual-upload-summary/index.tsapps/web/src/pages/(admin)/non-strategic-upload-summary/index.test.tsapps/web/src/pages/(admin)/non-strategic-upload-summary/index.tslibs/admin-pages/src/manual-upload/file-storage.test.tslibs/admin-pages/src/manual-upload/file-storage.tslibs/api/src/blob-ingestion/file-storage.test.tslibs/api/src/blob-ingestion/file-storage.tslibs/api/src/blob-ingestion/repository/service.test.tslibs/api/src/blob-ingestion/repository/service.tslibs/list-types/common/src/sjp/sjp-service.test.tslibs/list-types/common/src/sjp/sjp-service.tslibs/publication/src/file-storage/file-retrieval.test.tslibs/publication/src/file-storage/file-retrieval.tslibs/publication/src/index.tslibs/publication/src/repository/queries.test.tslibs/publication/src/repository/queries.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/publication/src/index.ts
- apps/web/src/pages/(admin)/manual-upload-summary/index.ts
- libs/publication/src/file-storage/file-retrieval.ts
- libs/publication/src/repository/queries.ts
- apps/web/src/pages/(admin)/manual-upload-summary/index.test.ts
- libs/publication/src/repository/queries.test.ts
- libs/publication/src/file-storage/file-retrieval.test.ts
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
2 participants
Summary
file_extensioncolumn onartefacttable withsource_artefact_id, storing the original uploaded file name end-to-end (manual upload, non-strategic upload, and API)multipart/form-dataflat file upload path toPOST /v1/publication— triggered when notypeform field is present,isFlatFile: true,hearing_listnot required/v1/publicationto accept an optionalsource_artefact_idbody fieldsource_artefact_id = <artefact_id><file_extension>Test plan
yarn test(25 new tests acrossvalidation.test.tsandservice.test.ts)e2e-tests/tests/api/flat-file-ingestion.spec.ts— run withyarn test:e2e:allPOST /v1/publicationJSON path: send request withsource_artefact_idfield, verify stored in DBPOST /v1/publicationflat file path (Postman): sendmultipart/form-datawith metadata fields +filepart (notypefield), expect 201 withartefact_idhearing_listis not required on the flat file pathyarn db:migrateCloses #797
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Database / Data Migration
source_artefact_idto retain original filenames for artefacts, and backfilled existing rows accordingly (replacing the prior extension-based approach).