Feature/582 Update reference data upload to support multiple provenances - #660
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements location reference provenance tracking across the reference data upload system and blob ingestion APIs. It adds a new database table, CSV parsing and validation, location resolution via external references, and updates summary UI to display provenance details alongside validation errors. ChangesLocation Provenance Reference Data
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 51 ✅ 7m 10s ⏱️ Results for commit 923e752. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/system-admin-pages/src/pages/reference-data-upload-summary/index.njk (1)
14-30: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the
govukErrorSummarymacro rather than hand-rolled error summary markup.The template imports
govukErrorSummarybut renders a custom block. Please switch to the macro for consistency and accessibility compliance.Proposed fix
{% if hasErrors %} - <div class="govuk-error-summary" data-module="govuk-error-summary"> - <div role="alert"> - <h2 class="govuk-error-summary__title">{{ errorSummaryTitle }}</h2> - <div class="govuk-error-summary__body"> - <p class="govuk-body">{{ errorTitle }}</p> - <ul class="govuk-list govuk-error-summary__list"> - {% for error in errors %} - <li> - <a href="{{ error.href }}">{{ error.text }}</a> - </li> - {% endfor %} - </ul> - </div> - </div> - </div> + {{ govukErrorSummary({ + titleText: errorSummaryTitle, + errorList: errors + }) }} + <p class="govuk-body">{{ errorTitle }}</p> {% endif %}As per coding guidelines, "Nunjucks templates must extend
layouts/base-template.njk, use govuk macros from govuk/components, and include error handling with govukErrorSummary."
🧹 Nitpick comments (1)
libs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.ts (1)
186-197: ⚡ Quick winLock the no-write contract on validation failure.
Add an explicit assertion that persistence is not triggered in this path.
Proposed test assertion
expect(mockResponse.redirect).not.toHaveBeenCalled(); + expect(repository.upsertLocations).not.toHaveBeenCalled(); expect(mockRequest.session!.uploadData).toBeDefined();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66751057-2bcd-4f21-900b-b97b0c5d2830
⛔ Files ignored due to path filters (1)
e2e-tests/fixtures/test-reference-data.csvis excluded by!**/*.csv
📒 Files selected for processing (46)
apps/postgres/prisma/migrations/20260520113517_add_location_reference/migration.sqldocs/tickets/582/plan.mddocs/tickets/582/tasks.mddocs/tickets/582/ticket.mde2e-tests/tests/api/blob-ingestion-notifications.spec.tslibs/api/src/blob-ingestion/repository/model.tslibs/api/src/blob-ingestion/repository/queries.test.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/list-types/administrative-court-daily-cause-list/src/pages/cy.tslibs/list-types/administrative-court-daily-cause-list/src/pages/en.tslibs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/cy.tslibs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/en.tslibs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/cy.tslibs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/en.tslibs/list-types/london-administrative-court-daily-cause-list/src/pages/cy.tslibs/list-types/london-administrative-court-daily-cause-list/src/pages/en.tslibs/list-types/rcj-standard-daily-cause-list/src/pages/cy.tslibs/list-types/rcj-standard-daily-cause-list/src/pages/en.tslibs/location/prisma/schema.prismalibs/location/src/index.tslibs/location/src/repository/location-reference-model.tslibs/location/src/repository/location-reference-queries.tslibs/location/src/seed-data.test.tslibs/location/src/seed-data.tslibs/publication/src/provenance.tslibs/publication/src/repository/queries.test.tslibs/system-admin-pages/src/assets/css/reference-data-upload.scsslibs/system-admin-pages/src/pages/reference-data-upload-summary/cy.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/en.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/index.njklibs/system-admin-pages/src/pages/reference-data-upload-summary/index.test.tslibs/system-admin-pages/src/pages/reference-data-upload-summary/index.tslibs/system-admin-pages/src/pages/reference-data-upload/index.njklibs/system-admin-pages/src/reference-data-upload/model.tslibs/system-admin-pages/src/reference-data-upload/parsers/csv-parser.test.tslibs/system-admin-pages/src/reference-data-upload/parsers/csv-parser.tslibs/system-admin-pages/src/reference-data-upload/repository/upload-repository.test.tslibs/system-admin-pages/src/reference-data-upload/repository/upload-repository.tslibs/system-admin-pages/src/reference-data-upload/services/download-service.test.tslibs/system-admin-pages/src/reference-data-upload/services/download-service.tslibs/system-admin-pages/src/reference-data-upload/validation/validation.test.tslibs/system-admin-pages/src/reference-data-upload/validation/validation.tsvitest.setup.ts
💤 Files with no reviewable changes (11)
- libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/cy.ts
- libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/cy.ts
- libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/en.ts
- libs/list-types/rcj-standard-daily-cause-list/src/pages/en.ts
- libs/list-types/administrative-court-daily-cause-list/src/pages/cy.ts
- libs/list-types/rcj-standard-daily-cause-list/src/pages/cy.ts
- libs/system-admin-pages/src/assets/css/reference-data-upload.scss
- libs/list-types/london-administrative-court-daily-cause-list/src/pages/en.ts
- libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/en.ts
- libs/list-types/london-administrative-court-daily-cause-list/src/pages/cy.ts
- libs/list-types/administrative-court-daily-cause-list/src/pages/en.ts
| **Enums** (kept as TypeScript string literal unions, not Prisma enums, to remain consistent with how `provenance` is already stored as a plain `String` column in the `artefact` and `list_types` tables): | ||
|
|
||
| ```typescript | ||
| // libs/location/src/location-reference/model.ts |
There was a problem hiding this comment.
Align documented file paths with the implemented module path.
Line 57 and Line 283 reference libs/location/src/location-reference/model.ts, but the stack context for this PR points to libs/location/src/repository/location-reference-model.ts. Please update one side so the ticket plan matches the actual code locations.
Also applies to: 283-285
| ``` | ||
| libs/location/ | ||
| prisma/schema.prisma MODIFY - add LocationReference model, locationType to ListType | ||
| src/location-reference/ | ||
| model.ts NEW - LOCATION_REFERENCE_PROVENANCES, LOCATION_REFERENCE_TYPES constants | ||
| queries.ts NEW - getLocationByProvenanceLocationId() | ||
| src/index.ts MODIFY - export getLocationByProvenanceLocationId | ||
|
|
||
| apps/postgres/prisma/migrations/ | ||
| <timestamp>_add_location_reference/migration.sql NEW | ||
| <timestamp>_add_list_type_location_type/migration.sql NEW | ||
|
|
||
| libs/system-admin-pages/src/reference-data-upload/ | ||
| model.ts MODIFY - add provenance fields to CsvRow, ParsedLocationData | ||
| parsers/csv-parser.ts MODIFY - add PROVENANCE, PROVENANCE_LOCATION_ID, PROVENANCE_LOCATION_TYPE to REQUIRED_HEADERS; parse and pass through | ||
| validation/validation.ts MODIFY - fix name-uniqueness logic; add provenance field validation | ||
| repository/upload-repository.ts MODIFY - upsert location_reference rows | ||
|
|
||
| libs/api/src/blob-ingestion/ | ||
| repository/model.ts MODIFY - add resolvedLocationId to BlobValidationResult | ||
| repository/service.ts MODIFY - use resolvedLocationId for locationId passed to createArtefact/processPublication | ||
| validation.ts MODIFY - add external provenance resolution; extend ALLOWED_PROVENANCES | ||
|
|
||
| libs/publication/src/ | ||
| provenance.ts MODIFY - add CP_CATH, PDDA to Provenance enum and PROVENANCE_LABELS | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block.
Line 279 opens a fenced code block without a language, which triggers markdown linting (MD040). Please annotate it (for example, text) to keep docs lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 279-279: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| - PROVENANCE | ||
| - PROVENANCE LOCATION ID | ||
| - PROVENANCE LOCATION TYPE |
There was a problem hiding this comment.
Use the exact CSV header names consistently across ticket docs.
Lines 13–15 use spaced names (PROVENANCE LOCATION ID, PROVENANCE LOCATION TYPE), while the plan/tasks and parser changes use underscore headers (PROVENANCE_LOCATION_ID, PROVENANCE_LOCATION_TYPE). Please standardise these to avoid ambiguity in implementation and test data.
🧰 Tools
🪛 LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word.
Context: ...ant model classes and the database: - PROVENANCE - PROVENANCE LOCATION ID - PROVENANCE LOCATION TYP...
(ENGLISH_WORD_REPEAT_RULE)
| regions: loc.locationRegions.map((lr: any) => lr.region.regionId), | ||
| subJurisdictions: loc.locationSubJurisdictions.map((lsj: any) => lsj.subJurisdiction.subJurisdictionId) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit `any` usage in this module.
rg -nP --type=ts '\b:\s*any\b' libs/location/src/repository/location-reference-queries.tsRepository: hmcts/cath-service
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libs/location/src/repository/location-reference-queries.ts"
echo "== any occurrences =="
rg -nP --type=ts '\b:\s*any\b' "$FILE" || true
echo
echo "== context around lines 35-70 =="
nl -ba "$FILE" | sed -n '30,75p'Repository: hmcts/cath-service
Length of output: 365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libs/location/src/repository/location-reference-queries.ts"
echo "== file info =="
wc -l "$FILE"
echo
echo "== any occurrences (typed) =="
rg -nP --type=ts '\b:\s*any\b' "$FILE" || true
echo
echo "== any occurrences (broader) =="
rg -n --type=ts '\bany\b' "$FILE" || true
echo
echo "== context around lines 35-70 =="
cat -n "$FILE" | sed -n '35,70p'Repository: hmcts/cath-service
Length of output: 1091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="libs/location/src/repository/location-reference-queries.ts"
cat -n "$FILE"Repository: hmcts/cath-service
Length of output: 1618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the imported Location type definition
FILE_JS="libs/location/src/repository/model.js"
FILE_TS="libs/location/src/repository/model.ts"
if [ -f "$FILE_JS" ]; then
echo "== cat $FILE_JS =="
cat -n "$FILE_JS"
elif [ -f "$FILE_TS" ]; then
echo "== cat $FILE_TS =="
cat -n "$FILE_TS"
else
echo "Neither $FILE_JS nor $FILE_TS exists; searching for Location type..."
fd -e ts -e tsx -e js -e jsx -e njk "model" libs/location/src/repository | head -n 50
fd -e ts -e tsx "export (type|interface) Location" libs/location/src/repository
fiRepository: hmcts/cath-service
Length of output: 1880
Remove explicit any from relation mapping callbacks (lines 46-47).
Those callback parameters bypass strict typing; rely on the Prisma-inferred types instead.
Suggested diff
- regions: loc.locationRegions.map((lr: any) => lr.region.regionId),
- subJurisdictions: loc.locationSubJurisdictions.map((lsj: any) => lsj.subJurisdiction.subJurisdictionId)
+ regions: loc.locationRegions.map((lr) => lr.region.regionId),
+ subJurisdictions: loc.locationSubJurisdictions.map((lsj) => lsj.subJurisdiction.subJurisdictionId)📝 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.
| regions: loc.locationRegions.map((lr: any) => lr.region.regionId), | |
| subJurisdictions: loc.locationSubJurisdictions.map((lsj: any) => lsj.subJurisdiction.subJurisdictionId) | |
| regions: loc.locationRegions.map((lr) => lr.region.regionId), | |
| subJurisdictions: loc.locationSubJurisdictions.map((lsj) => lsj.subJurisdiction.subJurisdictionId) |
| provenanceHeader: "Welsh placeholder", | ||
| provenanceLocationIdHeader: "Welsh placeholder", | ||
| provenanceLocationTypeHeader: "Welsh placeholder", | ||
| errorSummaryTitle: "Welsh placeholder", | ||
| errorTitle: "Welsh placeholder", | ||
| errorMessage: "Welsh placeholder" | ||
| errorTitle: "Welsh placeholder" |
There was a problem hiding this comment.
Replace new Welsh placeholder labels with real translations.
These values are user-facing and currently render as placeholders, which weakens Welsh-language support.
As per coding guidelines, "Every page must support both English and Welsh by providing en and cy content objects to the renderer, and templates should test with ?lng=cy query parameter."
| async function buildPreviewData(data: any[], page: number) { | ||
| const enrichedData = await enrichLocationData(data); | ||
| const itemsPerPage = 10; | ||
| const totalItems = enrichedData.length; | ||
| const totalPages = Math.ceil(totalItems / itemsPerPage); | ||
| const startIndex = (page - 1) * itemsPerPage; | ||
| const paginatedData = enrichedData.slice(startIndex, startIndex + itemsPerPage); |
There was a problem hiding this comment.
Normalise and clamp page before slicing and pagination metadata.
NaN, negative, or out-of-range pages can render empty preview data despite valid rows.
Suggested guard inside helper
async function buildPreviewData(data: any[], page: number) {
const enrichedData = await enrichLocationData(data);
const itemsPerPage = 10;
const totalItems = enrichedData.length;
const totalPages = Math.ceil(totalItems / itemsPerPage);
- const startIndex = (page - 1) * itemsPerPage;
+ const maxPage = Math.max(totalPages, 1);
+ const safePage = Number.isInteger(page) && page > 0 ? Math.min(page, maxPage) : 1;
+ const startIndex = (safePage - 1) * itemsPerPage;
const paginatedData = enrichedData.slice(startIndex, startIndex + itemsPerPage);
@@
- current: i === page
+ current: i === safePage
@@
- previous: page > 1 ? { href: `/reference-data-upload-summary?page=${page - 1}` } : undefined,
- next: page < totalPages ? { href: `/reference-data-upload-summary?page=${page + 1}` } : undefined
+ previous: safePage > 1 ? { href: `/reference-data-upload-summary?page=${safePage - 1}` } : undefined,
+ next: safePage < totalPages ? { href: `/reference-data-upload-summary?page=${safePage + 1}` } : undefinedAlso applies to: 36-44
| }); | ||
| } | ||
|
|
||
| async function buildPreviewData(data: any[], page: number) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify available row types and related function signatures before replacing `any[]`.
rg -n "export (type|interface) (CsvRow|ParsedLocationData)" libs/system-admin-pages/src/reference-data-upload/model.ts
rg -n "buildPreviewData|enrichLocationData\\(" libs/system-admin-pages/src/pages/reference-data-upload-summary/index.ts libs/system-admin-pages/src/reference-data-upload/services/enrichment-service.tsRepository: hmcts/cath-service
Length of output: 870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show helper implementation and its usages (pagination + typing)
sed -n '1,200p' libs/system-admin-pages/src/pages/reference-data-upload-summary/index.ts
# Show the row types and how parsed data is typed
sed -n '1,120p' libs/system-admin-pages/src/reference-data-upload/model.ts
# Show enrichment function signature (expected input type)
sed -n '1,160p' libs/system-admin-pages/src/reference-data-upload/services/enrichment-service.tsRepository: hmcts/cath-service
Length of output: 7331
Fix type-safety, pagination bounds, and Welsh locale handling in reference-data-upload-summary
- Replace
buildPreviewData(data: any[], page: number)withbuildPreviewData(data: ParsedLocationData[], page: number)(it currently passesany[]intoenrichLocationData, which expectsParsedLocationData[]). - Normalise/clamp
page(handleNaN,< 1, and> totalPages) before calculatingstartIndex/pagination state. - Remove hardcoded
locale = "en"in both GET and the POST validation error render path; derive locale from?lng=cy(so the renderer receives matchingen/cycontent for Welsh).
| const locale = "en"; | ||
| const t = getTranslations(locale); | ||
| const preview = await buildPreviewData(parseResult.data, 1); | ||
| return res.render("reference-data-upload-summary/index", { | ||
| ...t, | ||
| ...preview, | ||
| fileName: uploadData.fileName, | ||
| errors: validationErrors, | ||
| hasErrors: true, | ||
| locale | ||
| }); |
There was a problem hiding this comment.
Do not hardcode English in POST validation-error rendering.
This path ignores ?lng=cy and renders Welsh users back in English after submit errors.
Suggested locale selection
- const locale = "en";
+ const locale = req.query.lng === "cy" ? "cy" : "en";As per coding guidelines, "Every page must support both English and Welsh by providing en and cy content objects to the renderer, and templates should test with ?lng=cy query parameter."
📝 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 locale = "en"; | |
| const t = getTranslations(locale); | |
| const preview = await buildPreviewData(parseResult.data, 1); | |
| return res.render("reference-data-upload-summary/index", { | |
| ...t, | |
| ...preview, | |
| fileName: uploadData.fileName, | |
| errors: validationErrors, | |
| hasErrors: true, | |
| locale | |
| }); | |
| const locale = req.query.lng === "cy" ? "cy" : "en"; | |
| const t = getTranslations(locale); | |
| const preview = await buildPreviewData(parseResult.data, 1); | |
| return res.render("reference-data-upload-summary/index", { | |
| ...t, | |
| ...preview, | |
| fileName: uploadData.fileName, | |
| errors: validationErrors, | |
| hasErrors: true, | |
| locale | |
| }); |
| existing.locationReferences = [...existing.locationReferences, ...row.locationReferences]; | ||
| existing.subJurisdictionNames = [...new Set([...existing.subJurisdictionNames, ...row.subJurisdictionNames])]; | ||
| existing.regionNames = [...new Set([...existing.regionNames, ...row.regionNames])]; |
There was a problem hiding this comment.
De-duplicate merged provenance references before createMany.
Concatenating references blindly can cause insert failures if duplicate reference tuples reach this layer (e.g., repeated rows for the same location). Please normalise by key before persisting.
Proposed fix
function mergeByLocationId(data: ParsedLocationData[]): ParsedLocationData[] {
const merged = new Map<number, ParsedLocationData>();
for (const row of data) {
const existing = merged.get(row.locationId);
if (existing) {
- existing.locationReferences = [...existing.locationReferences, ...row.locationReferences];
+ const refByKey = new Map(
+ existing.locationReferences.map((ref) => [
+ `${ref.provenance}::${ref.provenanceLocationId}::${ref.provenanceLocationType}`,
+ ref
+ ])
+ );
+ for (const ref of row.locationReferences) {
+ refByKey.set(
+ `${ref.provenance}::${ref.provenanceLocationId}::${ref.provenanceLocationType}`,
+ ref
+ );
+ }
+ existing.locationReferences = [...refByKey.values()];
existing.subJurisdictionNames = [...new Set([...existing.subJurisdictionNames, ...row.subJurisdictionNames])];
existing.regionNames = [...new Set([...existing.regionNames, ...row.regionNames])];
} else {
merged.set(row.locationId, { ...row, locationReferences: [...row.locationReferences] });
}Also applies to: 110-118
| for (const ref of row.locationReferences) { | ||
| if (ref.provenance && ref.provenanceLocationId) { | ||
| const provenanceKey = `${ref.provenance}::${ref.provenanceLocationId}`; | ||
| if (provenanceKeys.has(provenanceKey)) { | ||
| errors.push({ | ||
| text: `Duplicate (PROVENANCE, PROVENANCE_LOCATION_ID) combination "${ref.provenance}, ${ref.provenanceLocationId}" in the file (rows ${provenanceKeys.get(provenanceKey)} and ${rowNumber})`, | ||
| href: "#file" | ||
| }); | ||
| } else { | ||
| provenanceKeys.set(provenanceKey, rowNumber); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add database conflict validation for (PROVENANCE, PROVENANCE_LOCATION_ID) before upload.
You only detect duplicate provenance keys within the CSV. The migration enforces global uniqueness in DB, so conflicts against existing rows can still fail later with a unique-constraint error. Add a pre-check against persisted location_reference rows (excluding same locationId) and return row-level validation errors.
Suggested approach
+ const provenanceKeyToRows = new Map<string, number[]>();
+ for (let i = 0; i < data.length; i++) {
+ const row = data[i];
+ const rowNumber = i + 1;
+ for (const ref of row.locationReferences) {
+ if (ref.provenance && ref.provenanceLocationId) {
+ const key = `${ref.provenance}::${ref.provenanceLocationId}`;
+ provenanceKeyToRows.set(key, [...(provenanceKeyToRows.get(key) ?? []), rowNumber]);
+ }
+ }
+ }
+
+ const existingReferences = await prisma.locationReference.findMany({
+ where: {
+ OR: [...provenanceKeyToRows.keys()].map((key) => {
+ const [provenance, provenanceLocationId] = key.split("::");
+ return { provenance, provenanceLocationId };
+ })
+ },
+ select: { provenance: true, provenanceLocationId: true, locationId: true }
+ });
+
+ for (const existing of existingReferences) {
+ const key = `${existing.provenance}::${existing.provenanceLocationId}`;
+ for (const rowNumber of provenanceKeyToRows.get(key) ?? []) {
+ const row = data[rowNumber - 1];
+ if (row.locationId !== existing.locationId) {
+ errors.push({
+ text: `Row ${rowNumber}: (PROVENANCE, PROVENANCE_LOCATION_ID) "${existing.provenance}, ${existing.provenanceLocationId}" already exists for a different location ID`,
+ href: "`#file`"
+ });
+ }
+ }
+ }Also applies to: 170-218
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
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.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5f52b87-aebf-4c08-a462-12ca4af13599
📒 Files selected for processing (14)
e2e-tests/tests/api/blob-ingestion.spec.tslibs/list-types/administrative-court-daily-cause-list/src/pages/cy.tslibs/list-types/administrative-court-daily-cause-list/src/pages/en.tslibs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/cy.tslibs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/en.tslibs/list-types/common/src/index.tslibs/list-types/common/src/locales/cy.tslibs/list-types/common/src/locales/en.tslibs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/cy.tslibs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/en.tslibs/list-types/london-administrative-court-daily-cause-list/src/pages/cy.tslibs/list-types/london-administrative-court-daily-cause-list/src/pages/en.tslibs/list-types/rcj-standard-daily-cause-list/src/pages/cy.tslibs/list-types/rcj-standard-daily-cause-list/src/pages/en.ts
| test("submits publication using provenance location ID as court_id for SNL provenance @nightly", async ({ request }) => { | ||
| const token = await getApiAuthToken(); | ||
|
|
||
| const response = await request.post(ENDPOINT, { | ||
| data: { | ||
| ...validPayload, | ||
| court_id: "9001", | ||
| provenance: "SNL" | ||
| }, | ||
| headers: { | ||
| Authorization: `Bearer ${token}` | ||
| } | ||
| }); | ||
|
|
||
| // 201 = ingested and matched to a location, 200 = ingested but no matching location found | ||
| expect([200, 201]).toContain(response.status()); | ||
| const body = await response.json(); | ||
| expect(body.success).toBe(true); | ||
| expect(body.artefact_id).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Assertion is too permissive to catch the regression this test targets.
The test title says it verifies the provenance location ID is resolved as court_id for SNL, but accepting both 200 and 201 means the test passes even when the location lookup silently fails to match (200 path). If the SNL location seed for 9001 is missing or the resolution logic regresses, this test will go green instead of red.
Consider tightening to expect(response.status()).toBe(201), or alternatively asserting a body field that confirms location resolution actually happened.
🧪 Proposed tightening
- // 201 = ingested and matched to a location, 200 = ingested but no matching location found
- expect([200, 201]).toContain(response.status());
+ // 201 confirms the SNL provenance location ID was resolved to a known location.
+ expect(response.status()).toBe(201);
const body = await response.json();
expect(body.success).toBe(true);
expect(body.artefact_id).toBeDefined();📝 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.
| test("submits publication using provenance location ID as court_id for SNL provenance @nightly", async ({ request }) => { | |
| const token = await getApiAuthToken(); | |
| const response = await request.post(ENDPOINT, { | |
| data: { | |
| ...validPayload, | |
| court_id: "9001", | |
| provenance: "SNL" | |
| }, | |
| headers: { | |
| Authorization: `Bearer ${token}` | |
| } | |
| }); | |
| // 201 = ingested and matched to a location, 200 = ingested but no matching location found | |
| expect([200, 201]).toContain(response.status()); | |
| const body = await response.json(); | |
| expect(body.success).toBe(true); | |
| expect(body.artefact_id).toBeDefined(); | |
| }); | |
| test("submits publication using provenance location ID as court_id for SNL provenance `@nightly`", async ({ request }) => { | |
| const token = await getApiAuthToken(); | |
| const response = await request.post(ENDPOINT, { | |
| data: { | |
| ...validPayload, | |
| court_id: "9001", | |
| provenance: "SNL" | |
| }, | |
| headers: { | |
| Authorization: `Bearer ${token}` | |
| } | |
| }); | |
| // 201 confirms the SNL provenance location ID was resolved to a known location. | |
| expect(response.status()).toBe(201); | |
| const body = await response.json(); | |
| expect(body.success).toBe(true); | |
| expect(body.artefact_id).toBeDefined(); | |
| }); |
| @@ -1,3 +1,5 @@ | |||
| import { provenanceLabelsCy as provenanceLabels } from "@hmcts/list-types-common"; | |||
There was a problem hiding this comment.
Build failing: test mock for @hmcts/list-types-common is missing provenanceLabelsCy.
Same root cause as the sibling court-of-appeal-civil-daily-cause-list failure — src/pages/index.test.ts mocks @hmcts/list-types-common without the new provenanceLabelsCy export, so importing cy.ts blows up under test. Extend the mock with importOriginal (or add the missing export explicitly) to unblock CI.
🔧 Suggested mock fix (apply in `src/pages/index.test.ts`)
vi.mock("`@hmcts/list-types-common`", async (importOriginal) => {
const actual = await importOriginal<typeof import("`@hmcts/list-types-common`")>();
return {
...actual,
// keep any existing overrides here
};
});| @@ -1,3 +1,5 @@ | |||
| import { provenanceLabelsCy as provenanceLabels } from "@hmcts/list-types-common"; | |||
There was a problem hiding this comment.
Build failing: test mock for @hmcts/list-types-common is missing provenanceLabelsCy.
Per the CI failure on src/pages/index.test.ts, the vi.mock("@hmcts/list-types-common") in the test does not expose provenanceLabelsCy, so importing this module under test throws. Update the mock to include the new export (or use importOriginal) so the suite passes.
🔧 Suggested mock fix (apply in `src/pages/index.test.ts`)
vi.mock("`@hmcts/list-types-common`", async (importOriginal) => {
const actual = await importOriginal<typeof import("`@hmcts/list-types-common`")>();
return {
...actual,
// keep any existing overrides here
};
});
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…ce-data-upload # Conflicts: # libs/system-admin-pages/src/reference-data-upload/services/download-service.ts
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)
libs/postgres-prisma/prisma/schema/location.prisma (1)
83-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an index on
locationId.Line 90 adds the FK, but the model never indexes
location_id. The new upload path mutates references per location, and cascade deletes will also hit this column; without an index those operations will degrade badly as the table grows.Suggested fix
model LocationReference { locationReferenceId String `@id` `@default`(cuid()) `@map`("location_reference_id") locationId Int `@map`("location_id") provenance String `@map`("provenance") `@db.VarChar`(50) provenanceLocationId String `@map`("provenance_location_id") `@db.VarChar`(255) provenanceLocationType String `@map`("provenance_location_type") `@db.VarChar`(50) location Location `@relation`(fields: [locationId], references: [locationId], onDelete: Cascade) + @@index([locationId]) @@unique([provenance, provenanceLocationId]) @@map("location_reference") }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4de6fa42-29b3-4897-8133-c4d9109d007e
📒 Files selected for processing (10)
e2e-tests/tests/api/blob-ingestion-notifications.spec.tslibs/api/src/blob-ingestion/repository/queries.test.tslibs/api/src/blob-ingestion/validation.test.tslibs/api/src/blob-ingestion/validation.tslibs/list-types/common/src/index.tslibs/location/src/index.tslibs/location/src/seed-data.tslibs/postgres-prisma/prisma/schema/location.prismalibs/publication/src/repository/queries.test.tslibs/system-admin-pages/src/reference-data-upload/services/download-service.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e-tests/tests/api/blob-ingestion-notifications.spec.ts
- libs/location/src/seed-data.ts
- libs/system-admin-pages/src/reference-data-upload/services/download-service.ts
- libs/api/src/blob-ingestion/repository/queries.test.ts
- libs/publication/src/repository/queries.test.ts
- libs/location/src/index.ts
- libs/list-types/common/src/index.ts
- libs/api/src/blob-ingestion/validation.test.ts
- libs/api/src/blob-ingestion/validation.ts
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
3 participants
Jira link
#582
Change description
Update reference data upload to support multiple provenances
Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation