Feature/410 system admin data management - #680
Conversation
# Conflicts: # apps/postgres/package.json # libs/system-admin-pages/src/reference-data-upload/services/download-service.ts
# Conflicts: # libs/system-admin-pages/src/reference-data-upload/services/download-service.ts
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces jurisdiction and location-jurisdiction data management for system admins: schema/migration for soft-delete and audit logging (later reverted to hard-delete), Prisma query and service layers with validation and audit logging, a full set of admin pages (create/list/modify/update/delete) for jurisdictions and regions, location-jurisdiction mapping pages, updated dashboard/e2e tests, expanded barrel exports, and supporting documentation. ChangesReference data admin: schema, service, pages, tests, and e2e
Sequence Diagram(s)sequenceDiagram
participant Page as "jurisdiction-data-delete page"
participant Service as "jurisdiction-management service"
participant Queries as "jurisdiction-management queries"
participant Prisma
participant Audit as "audit-log logger"
Page->>Service: deleteJurisdictionData(id, type, user)
Service->>Queries: findJurisdictionDataById(id, type)
Queries->>Prisma: findUnique
Prisma-->>Queries: record or null
alt record not found
Queries-->>Service: null
Service-->>Page: ValidationError "Record not found"
else record found
Service->>Queries: hasDependencies(id, type)
Queries->>Prisma: count related rows
Prisma-->>Queries: count
alt dependencies exist
Queries-->>Service: true
Service-->>Page: ValidationError "linked to locations"
else no dependencies
Service->>Queries: hardDeleteJurisdictionRecord(id, type)
Queries->>Prisma: delete
Service->>Audit: logAction(DELETE_*, details)
Service-->>Page: []
Page-->>Page: redirect to delete-success
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 10s ⏱️ Results for commit 19fb3fa. ♻️ 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/system-admin-dashboard/cy.ts (1)
25-27:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Blob Explorer href differs from English locale.
The Welsh locale uses
/blob-explorerbut the English locale uses/blob-explorer-locations. This causes Welsh users to navigate to a different page, breaking language consistency.🔧 Proposed fix
{ title: "Archwiliwr Blob", description: "Darganfod cynnwys wedi'i uwchlwytho i bob lleoliad", - href: "/blob-explorer" + href: "/blob-explorer-locations" },
🟡 Minor comments (8)
libs/system-admin-pages/src/pages/location-jurisdiction-update/index.ts-78-78 (1)
78-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
anytype cast used without justification.Line 78 casts
reqtoanyto access theuserproperty without justification. As per coding guidelines, either properly type the Request with an extended user property or add a comment explaining why the cast is necessary.As per coding guidelines: "No
anytype without justification - either avoid it or add a comment explaining why it's necessary."libs/system-admin-pages/src/pages/location-jurisdiction-update/index.ts-28-29 (1)
28-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
anytype used without justification.Lines 28-29 use
anytype forlsjandlrparameters without justification. As per coding guidelines, either properly type these parameters or add a comment explaining whyanyis necessary.🔧 Suggested improvement
Consider typing these properly based on the return type of
getLocationJurisdictionDetails, or add a justification comment if the type is genuinely unavailable.As per coding guidelines: "No
anytype without justification - either avoid it or add a comment explaining why it's necessary."docs/tickets/410/plan.md-207-218 (1)
207-218:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the documented
AdminAuditLogmodel with the implemented schema.The plan documents
adminAuditLogId, but the current schema contract usesidforAdminAuditLog. Please keep this section in sync to avoid integration confusion between docs and code.docs/tickets/410/plan.md-37-147 (1)
37-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
The fenced blocks at Line 37, Line 139, and Line 147 have no language tag, which triggers MD040 and reduces editor/lint clarity.
docs/tickets/410/plan.md-159-221 (1)
159-221:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the schema file path in the plan.
Line 159 and Line 221 point to
libs/location/prisma/schema.prisma, but the schema in this PR is underlibs/postgres-prisma/prisma/schema/location.prisma. This will misdirect follow-up implementation work.libs/system-admin-pages/src/jurisdiction-management/jurisdiction-management-service.ts-74-75 (1)
74-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDependency error text is inaccurate for jurisdiction deletes.
The message says “linked to one or more locations”, but for
Jurisdictionthe dependency check is sub-jurisdictions. This is misleading to admins troubleshooting delete failures.libs/system-admin-pages/src/pages/jurisdiction-data-create/index.test.ts-34-34 (1)
34-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unqualified
anycasts from test request/session setup.Lines 34 and 96 use
anyfor session typing, which can hide contract drift withJurisdictionDataSession.As per coding guidelines, “Use TypeScript strict mode. No
anytype without justification - either avoid it or add a comment explaining why it's necessary.”Also applies to: 96-96
libs/system-admin-pages/src/pages/location-jurisdiction-search/index.njk-9-9 (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the
backvariable for Welsh support.The back link text is hardcoded as "Back" instead of using a variable. This breaks Welsh localisation, as Welsh users will see "Back" instead of "Yn ôl".
🌐 Proposed fix for i18n support
- text: "Back", + text: back,Ensure the controller provides the
backvariable in the content object passed to the template (following the pattern used in other pages:back: language === "cy" ? "Yn ôl" : "Back").As per coding guidelines: "Every user-facing page must support both English and Welsh languages."
🧹 Nitpick comments (15)
e2e-tests/tests/system-admin/system-admin-dashboard.spec.ts (1)
55-60: ⚡ Quick winPrefer role-based selectors for the new navigation assertion.
These new checks use CSS/text locators. Use
getByRole()for both the link click and heading assertion to match test conventions and improve resilience.Suggested change
- await page.click('a:has-text("Reference Data")'); + await page.getByRole("link", { name: "Reference Data" }).click(); await page.waitForURL("**/reference-data"); - const referenceDataHeading = page.locator("h1"); - await expect(referenceDataHeading).toBeVisible(); - await expect(referenceDataHeading).toHaveText("What do you want to do?"); + await expect(page.getByRole("heading", { level: 1, name: "What do you want to do?" })).toBeVisible();As per coding guidelines, “Use selector priority: getByRole() first, then getByLabel() for form inputs, getByText() for specific text, and getByTestId() only as a last resort.”
docs/tickets/410/tasks.md (1)
5-6: ⚡ Quick winUpdate path references in task checklist.
Lines 5-6 reference
libs/location/prisma/schema.prismabut the guideline specifies schemas should be inlibs/postgres-prisma/prisma/schema/with kebab-case names. The actual implementation correctly useslibs/postgres-prisma/prisma/schema/location.prisma.Line 188 references
e2e-tests/tests/reference-data-management.spec.tsbut the actual file ise2e-tests/tests/system-admin/reference-data-management.spec.ts.📝 Suggested corrections
- [ ] Add `deleted_at` column to `jurisdiction`, `sub_jurisdiction`, and `region` tables in `libs/location/prisma/schema.prisma` + [ ] Add `deleted_at` column to `jurisdiction`, `sub_jurisdiction`, and `region` tables in `libs/postgres-prisma/prisma/schema/location.prisma` - [ ] Add `AdminAuditLog` model (`admin_audit_log` table) to `libs/location/prisma/schema.prisma` + [ ] Add `AdminAuditLog` model (`admin_audit_log` table) to `libs/postgres-prisma/prisma/schema/admin-audit-log.prisma` or appropriate schema file- [ ] Create `e2e-tests/tests/reference-data-management.spec.ts` covering: + [ ] Create `e2e-tests/tests/system-admin/reference-data-management.spec.ts` covering:As per coding guidelines: "Create one feature schema file per domain in
libs/postgres-prisma/prisma/schema/. Use kebab-case for file names."Also applies to: 188-188
libs/system-admin-pages/src/pages/location-jurisdiction-delete/index.ts (1)
53-53: ⚡ Quick winConsider documenting or typing the user property access.
The
as anycast is used to accessuseradded by authentication middleware. Consider either adding a comment explaining this or properly extending the Express Request interface to include the user property.💡 Suggested improvement
Option 1: Add a clarifying comment:
+ // User property is added by `@hmcts/auth` middleware const performedBy = (req as any).user?.email || "unknown";Option 2: Create a type extension (if not already defined elsewhere):
interface AuthenticatedRequest extends Request { user?: { email: string }; }As per coding guidelines: "No
anytype without justification - either avoid it or add a comment explaining why it's necessary."e2e-tests/tests/system-admin/reference-data-management.spec.ts (1)
15-23: 🏗️ Heavy liftPrefer role/label/text selectors over CSS locators for user actions.
This suite uses many class/text CSS locators where
getByRole/getByLabelwould be more stable and aligned with test standards.
As per coding guidelines: selector priority isgetByRole()first, thengetByLabel(), thengetByText(), using lower-priority selectors only as a last resort.Also applies to: 48-49, 64-66, 128-129, 184-186, 237-238
libs/system-admin-pages/src/pages/jurisdiction-data-modify/index.test.ts (1)
24-24: ⚡ Quick winReplace unqualified
anyin session assertions with a typed shape.This keeps the tests aligned with strict TypeScript expectations and avoids losing type-safety in request/session setup.
As per coding guidelines: use TypeScript strict mode and avoid
anyunless explicitly justified.Also applies to: 71-71
libs/system-admin-pages/src/jurisdiction-management/jurisdiction-management-queries.test.ts (1)
189-205: ⚡ Quick winTransaction-path tests are too shallow.
These cases only verify that
$transactionran, not that expecteddeleteMany/createManyor lock/max-id/create calls were executed with correct payloads. Please assert the transactional side effects explicitly to catch regressions.Also applies to: 317-354
libs/system-admin-pages/src/pages/location-jurisdiction-search/index.test.ts (1)
78-101: ⚡ Quick winAdd redirect assertions for the “not found” and “typed-but-not-selected” paths.
Those tests currently verify only session errors. Please also assert redirect to
/location-jurisdiction-searchso route behaviour regressions are caught.libs/system-admin-pages/src/pages/jurisdiction-data-create/cy.ts (1)
1-1: ⚡ Quick winUse SCREAMING_SNAKE_CASE for the exported locale constant.
Line 1 exports
cyas a module-level constant; this should follow the constant naming convention to keep TS files consistent.As per coding guidelines, “Constants must use SCREAMING_SNAKE_CASE (e.g.,
MAX_FILE_SIZE,DEFAULT_TIMEOUT).”libs/system-admin-pages/src/pages/jurisdiction-data-create/en.ts (1)
1-1: ⚡ Quick winUse SCREAMING_SNAKE_CASE for the exported locale constant.
Line 1 exports
enas a module-level constant; please align it with the constant naming rule used across TS modules.As per coding guidelines, “Constants must use SCREAMING_SNAKE_CASE (e.g.,
MAX_FILE_SIZE,DEFAULT_TIMEOUT).”libs/system-admin-pages/src/pages/jurisdiction-data-create-success/index.test.ts (2)
18-20: ⚡ Quick winAdd justification comment for
anytype.The
as anycast is used without explanation. As per coding guidelines, whenanyis necessary, add a comment explaining why.📝 Suggested comment
req = { query: {}, session: { jurisdictionData: { id: 0, type: "Jurisdiction", name: "Civil", welshName: "Sifil" } - } as any + } as any // Test session doesn't need full Express session interface };As per coding guidelines: Use TypeScript strict mode. No
anytype without justification - either avoid it or add a comment explaining why it's necessary.
54-54: ⚡ Quick winAdd justification comment for
anytype.The
as anycast is used without explanation. Add a comment explaining whyanyis necessary here.📝 Suggested comment
- req.session = {} as any; + req.session = {} as any; // Test session doesn't need full Express session interfaceAs per coding guidelines: No
anytype without justification.libs/system-admin-pages/src/pages/reference-data/index.ts (1)
31-44: ⚡ Quick winConsider validating that the selected option exists.
The current validation only checks if
selectedis truthy. If a user submits an invalid option value, the code falls back to redirecting to/reference-data(line 47). Whilst this is safe, it's inconsistent withjurisdiction-data/index.ts(line 32), which explicitly validates that the selected value exists inREDIRECT_MAP.♻️ Suggested validation improvement for consistency
const selected = req.body.action; + const option = content.options.find((o) => o.value === selected); - if (!selected) { + if (!selected || !option) { const errors = [{ text: content.noSelectionError, href: "`#action`" }]; return res.render("reference-data/index-radios", {Then remove line 46 as
optionis already defined.libs/system-admin-pages/src/pages/jurisdiction-data-list/index.test.ts (1)
83-83: 💤 Low valueAvoid
as anytype assertion.Consider typing the render call more precisely rather than using
as any.♻️ Suggested typing improvement
- const renderCall = vi.mocked(res.render!).mock.calls[0][1] as any; + const renderCall = vi.mocked(res.render!).mock.calls[0][1] as { tableRows: Array<Array<{ text?: string; html?: string }>> };As per coding guidelines: avoid
anytype without justification.libs/system-admin-pages/src/pages/location-jurisdiction-manage/index.ts (1)
22-22: ⚡ Quick winType the
lsjparameter properly or justifyany.The
anytype is used without justification. The data structure comes fromgetLocationJurisdictionDetails, which returns a typed Prisma result. Consider using the proper type from the service return value or Prisma's generated types.♻️ Suggested typing improvement
- const tableRows = - locationData?.locationSubJurisdictions?.map((lsj: any) => [ + const tableRows = + locationData?.locationSubJurisdictions?.map((lsj) => [TypeScript will infer the type from
locationData, or explicitly type it based on thegetLocationJurisdictionDetailsreturn type.As per coding guidelines: "Use TypeScript strict mode. No
anytype without justification - either avoid it or add a comment explaining why it's necessary."libs/system-admin-pages/src/pages/location-jurisdiction-update/index.test.ts (1)
34-35: ⚡ Quick winAdd justification comments for
anytype casts.Lines 34-35 use
as anywithout explanation. As per coding guidelines,anytypes require a comment explaining why they're necessary.📝 Proposed improvement
session: { locationJurisdiction: { locationId: 100, locationName: "Test Court", locationWelshName: "Llys Prawf" } - } as any, - user: { email: "admin@example.com" } as any + } as any, // Partial mock of Express session + user: { email: "admin@example.com" } as any // Partial mock of user object
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e32e6d00-57e0-4357-aa63-144c2f9faff4
📒 Files selected for processing (101)
apps/postgres/prisma/migrations/20260605141023_add_jurisdiction_soft_delete_and_audit_log/migration.sqldocs/tickets/410/plan.mddocs/tickets/410/tasks.mddocs/tickets/410/ticket.mde2e-tests/tests/system-admin/reference-data-management.spec.tse2e-tests/tests/system-admin/system-admin-dashboard.spec.tslibs/postgres-prisma/prisma/schema/location.prismalibs/system-admin-pages/src/jurisdiction-management/jurisdiction-management-queries.test.tslibs/system-admin-pages/src/jurisdiction-management/jurisdiction-management-queries.tslibs/system-admin-pages/src/jurisdiction-management/jurisdiction-management-service.test.tslibs/system-admin-pages/src/jurisdiction-management/jurisdiction-management-service.tslibs/system-admin-pages/src/pages/jurisdiction-data-create-success/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-create-success/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-create-success/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-create-success/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-create-success/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-create/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-create/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-create/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-create/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-create/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete-success/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete-success/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete-success/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-delete-success/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete-success/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-delete/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-delete/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-list/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-list/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-list/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-list/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-list/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-modify/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-modify/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-modify/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-modify/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-modify/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-session.tslibs/system-admin-pages/src/pages/jurisdiction-data-update-success/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-update-success/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-update-success/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-update-success/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-update-success/index.tslibs/system-admin-pages/src/pages/jurisdiction-data-update/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data-update/en.tslibs/system-admin-pages/src/pages/jurisdiction-data-update/index.njklibs/system-admin-pages/src/pages/jurisdiction-data-update/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data-update/index.tslibs/system-admin-pages/src/pages/jurisdiction-data/cy.tslibs/system-admin-pages/src/pages/jurisdiction-data/en.tslibs/system-admin-pages/src/pages/jurisdiction-data/index.njklibs/system-admin-pages/src/pages/jurisdiction-data/index.test.tslibs/system-admin-pages/src/pages/jurisdiction-data/index.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete-success/cy.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete-success/en.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete-success/index.njklibs/system-admin-pages/src/pages/location-jurisdiction-delete-success/index.test.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete-success/index.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete/cy.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete/en.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete/index.njklibs/system-admin-pages/src/pages/location-jurisdiction-delete/index.test.tslibs/system-admin-pages/src/pages/location-jurisdiction-delete/index.tslibs/system-admin-pages/src/pages/location-jurisdiction-manage/cy.tslibs/system-admin-pages/src/pages/location-jurisdiction-manage/en.tslibs/system-admin-pages/src/pages/location-jurisdiction-manage/index.njklibs/system-admin-pages/src/pages/location-jurisdiction-manage/index.test.tslibs/system-admin-pages/src/pages/location-jurisdiction-manage/index.tslibs/system-admin-pages/src/pages/location-jurisdiction-search/cy.tslibs/system-admin-pages/src/pages/location-jurisdiction-search/en.tslibs/system-admin-pages/src/pages/location-jurisdiction-search/index.njklibs/system-admin-pages/src/pages/location-jurisdiction-search/index.test.tslibs/system-admin-pages/src/pages/location-jurisdiction-search/index.tslibs/system-admin-pages/src/pages/location-jurisdiction-update-success/cy.tslibs/system-admin-pages/src/pages/location-jurisdiction-update-success/en.tslibs/system-admin-pages/src/pages/location-jurisdiction-update-success/index.njklibs/system-admin-pages/src/pages/location-jurisdiction-update-success/index.test.tslibs/system-admin-pages/src/pages/location-jurisdiction-update-success/index.tslibs/system-admin-pages/src/pages/location-jurisdiction-update/cy.tslibs/system-admin-pages/src/pages/location-jurisdiction-update/en.tslibs/system-admin-pages/src/pages/location-jurisdiction-update/index-accordions.njklibs/system-admin-pages/src/pages/location-jurisdiction-update/index-dropdowns.njklibs/system-admin-pages/src/pages/location-jurisdiction-update/index.test.tslibs/system-admin-pages/src/pages/location-jurisdiction-update/index.tslibs/system-admin-pages/src/pages/location-metadata-search/index.njklibs/system-admin-pages/src/pages/reference-data-upload/cy.tslibs/system-admin-pages/src/pages/reference-data-upload/en.tslibs/system-admin-pages/src/pages/reference-data-upload/index.njklibs/system-admin-pages/src/pages/reference-data/cy.tslibs/system-admin-pages/src/pages/reference-data/en.tslibs/system-admin-pages/src/pages/reference-data/index-radios.njklibs/system-admin-pages/src/pages/reference-data/index-tiles.njklibs/system-admin-pages/src/pages/reference-data/index.test.tslibs/system-admin-pages/src/pages/reference-data/index.tslibs/system-admin-pages/src/pages/system-admin-dashboard/cy.tslibs/system-admin-pages/src/pages/system-admin-dashboard/en.tslibs/system-admin-pages/src/pages/system-admin-dashboard/index.njk.test.ts
💤 Files with no reviewable changes (2)
- libs/system-admin-pages/src/pages/reference-data-upload/en.ts
- libs/system-admin-pages/src/pages/reference-data-upload/cy.ts
| test.describe | ||
| .skip("Reference Data Management", () => { |
There was a problem hiding this comment.
Remove the suite-level skip before merge.
This currently disables every reference-data E2E journey, so none of the new flow coverage executes in CI.
| test("system admin can navigate from reference data to upload page @nightly", async ({ page }) => { | ||
| await page.goto("/reference-data"); | ||
|
|
||
| // Click Upload Reference Data tile | ||
| const uploadTile = page.locator('a.admin-tile:has-text("Upload Reference Data")'); | ||
| await uploadTile.click(); | ||
|
|
||
| await expect(page).toHaveURL("/reference-data-upload"); | ||
|
|
||
| // Verify warning message is displayed | ||
| await expect(page.locator(".govuk-warning-text")).toBeVisible(); | ||
|
|
||
| // Verify back link points to /reference-data | ||
| const backLink = page.locator(".govuk-back-link"); | ||
| await expect(backLink).toHaveAttribute("href", "/reference-data"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add Welsh and Axe checks inline in each remaining journey test.
A few journeys include these checks, but several do not. The standard here requires both language and accessibility coverage within each journey.
As per coding guidelines: include validation checks, Welsh translations, and accessibility tests inline within each journey test using Axe-core.
Also applies to: 110-150, 152-179, 181-197, 199-252
| test("system admin can update location jurisdiction data @nightly", async ({ page }) => { | ||
| // Assume session already has location data (navigate through search first in real run) | ||
| await page.goto("/location-jurisdiction-manage"); | ||
|
|
||
| // If redirected to search (no session), that's expected behavior | ||
| const url = page.url(); | ||
| if (url.includes("location-jurisdiction-search")) { | ||
| return; // Can't proceed without a real location in the database | ||
| } |
There was a problem hiding this comment.
Replace early return guards with explicit skip/failure semantics.
Returning early marks incomplete journeys as passed. Use test.skip(...) with a reason (or deterministic data setup) so the result is accurate.
Suggested change
- if (url.includes("location-jurisdiction-search")) {
- return; // Can't proceed without a real location in the database
- }
+ test.skip(url.includes("location-jurisdiction-search"), "Requires seeded location-jurisdiction data");As per coding guidelines: E2E tests must cover complete user journeys and should not silently bypass journey assertions.
Also applies to: 232-234, 259-262
| regionId Int @id @map("region_id") | ||
| name String @unique | ||
| welshName String @unique @map("welsh_name") | ||
| deletedAt DateTime? @map("deleted_at") |
There was a problem hiding this comment.
Soft-delete and uniqueness are currently at odds.
name/welshName are still globally unique, while the service validates uniqueness only among active rows. That means a create can pass validation after a soft-delete and then fail on DB constraint at write time.
Proposed direction
model Region {
- name String `@unique`
- welshName String `@unique` `@map`("welsh_name")
+ name String
+ welshName String `@map`("welsh_name")
deletedAt DateTime? `@map`("deleted_at")
}Apply the same removal for Jurisdiction and SubJurisdiction, then add partial unique indexes in migration SQL (active rows only), e.g.:
CREATE UNIQUE INDEX ... ON region (name) WHERE deleted_at IS NULL;Also applies to: 13-16, 24-28
| const max = await prisma.jurisdiction.findFirst({ orderBy: { jurisdictionId: "desc" }, select: { jurisdictionId: true } }); | ||
| await prisma.jurisdiction.create({ | ||
| data: { jurisdictionId: (max?.jurisdictionId ?? 0) + 1, name: data.name.trim(), welshName: data.welshName.trim() } | ||
| }); |
There was a problem hiding this comment.
Manual ID allocation is race-prone for jurisdiction and region creates.
Concurrent requests can read the same max ID and attempt identical inserts, causing intermittent create failures. This path needs DB-level sequencing or a serialised transactional allocation strategy (like the sub-jurisdiction path).
Also applies to: 119-122
| <a href="{{ updateHref }}" role="button" draggable="false" class="govuk-button" data-module="govuk-button"> | ||
| {{ updateButton }} | ||
| </a> | ||
| <a href="{{ deleteHref }}" role="button" draggable="false" class="govuk-button govuk-button--warning" data-module="govuk-button"> | ||
| {{ deleteButton }} | ||
| </a> |
There was a problem hiding this comment.
Use govukButton macro for the action buttons instead of raw anchor markup.
This page currently bypasses the imported macro for key actions.
Suggested change
- <div class="govuk-button-group">
- <a href="{{ updateHref }}" role="button" draggable="false" class="govuk-button" data-module="govuk-button">
- {{ updateButton }}
- </a>
- <a href="{{ deleteHref }}" role="button" draggable="false" class="govuk-button govuk-button--warning" data-module="govuk-button">
- {{ deleteButton }}
- </a>
- </div>
+ <div class="govuk-button-group">
+ {{ govukButton({
+ text: updateButton,
+ href: updateHref
+ }) }}
+ {{ govukButton({
+ text: deleteButton,
+ href: deleteHref,
+ classes: "govuk-button--warning"
+ }) }}
+ </div>As per coding guidelines: Nunjucks templates must use GOV.UK component macros for forms and UI elements.
📝 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.
| <a href="{{ updateHref }}" role="button" draggable="false" class="govuk-button" data-module="govuk-button"> | |
| {{ updateButton }} | |
| </a> | |
| <a href="{{ deleteHref }}" role="button" draggable="false" class="govuk-button govuk-button--warning" data-module="govuk-button"> | |
| {{ deleteButton }} | |
| </a> | |
| <div class="govuk-button-group"> | |
| {{ govukButton({ | |
| text: updateButton, | |
| href: updateHref | |
| }) }} | |
| {{ govukButton({ | |
| text: deleteButton, | |
| href: deleteHref, | |
| classes: "govuk-button--warning" | |
| }) }} | |
| </div> |
| const formData = { | ||
| name: (req.body.name || "").trim(), | ||
| welshName: (req.body.welshName || "").trim() | ||
| }; |
There was a problem hiding this comment.
Guard request body fields before calling .trim().
Line 41 and Line 42 can throw if req.body is missing or the submitted value is not a string, turning bad input into a 500.
Suggested fix
+ const readBodyText = (value: unknown): string => (typeof value === "string" ? value.trim() : "");
+
const formData = {
- name: (req.body.name || "").trim(),
- welshName: (req.body.welshName || "").trim()
+ name: readBodyText(req.body?.name),
+ welshName: readBodyText(req.body?.welshName)
};As per coding guidelines, “All API endpoints must include input validation.”
📝 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 formData = { | |
| name: (req.body.name || "").trim(), | |
| welshName: (req.body.welshName || "").trim() | |
| }; | |
| const readBodyText = (value: unknown): string => (typeof value === "string" ? value.trim() : ""); | |
| const formData = { | |
| name: readBodyText(req.body?.name), | |
| welshName: readBodyText(req.body?.welshName) | |
| }; |
| const locationIdStr = req.body.locationId as string | undefined; | ||
| const displayValue = req.body["location-search-display"] as string | undefined; | ||
|
|
||
| const redirectWithError = (errorText: string) => { | ||
| session.locationJurisdictionSearchErrors = [{ text: errorText, href: "#location-search" }]; | ||
| return res.redirect(`/location-jurisdiction-search${getLanguageParam(language)}`); | ||
| }; | ||
|
|
||
| const userTypedButDidNotSelect = displayValue && displayValue.trim().length >= 3 && isEmpty(locationIdStr); | ||
| if (userTypedButDidNotSelect) { | ||
| return redirectWithError(content.locationNotFound); | ||
| } | ||
|
|
||
| if (isEmpty(locationIdStr)) { | ||
| return redirectWithError(content.locationNameRequired); | ||
| } | ||
|
|
||
| const locationId = Number.parseInt(locationIdStr!, 10); | ||
| if (Number.isNaN(locationId)) { | ||
| return redirectWithError(content.locationNotFound); | ||
| } |
There was a problem hiding this comment.
Harden POST body parsing before validation.
req.body values are asserted as strings, then used with .trim()/parseInt directly. Malformed non-string payloads can throw at runtime, and partial numeric strings (for example, 12abc) are currently accepted. Add runtime type checks and strict numeric validation before conversion.
Suggested fix
- const locationIdStr = req.body.locationId as string | undefined;
- const displayValue = req.body["location-search-display"] as string | undefined;
+ const locationIdStr = typeof req.body.locationId === "string" ? req.body.locationId : undefined;
+ const displayValue =
+ typeof req.body["location-search-display"] === "string" ? req.body["location-search-display"] : undefined;
@@
- const locationId = Number.parseInt(locationIdStr!, 10);
- if (Number.isNaN(locationId)) {
+ if (!locationIdStr || !/^\d+$/.test(locationIdStr)) {
+ return redirectWithError(content.locationNotFound);
+ }
+
+ const locationId = Number(locationIdStr);
+ if (!Number.isSafeInteger(locationId) || locationId <= 0) {
return redirectWithError(content.locationNotFound);
}As per coding guidelines, all API endpoints must include input validation.
📝 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 locationIdStr = req.body.locationId as string | undefined; | |
| const displayValue = req.body["location-search-display"] as string | undefined; | |
| const redirectWithError = (errorText: string) => { | |
| session.locationJurisdictionSearchErrors = [{ text: errorText, href: "#location-search" }]; | |
| return res.redirect(`/location-jurisdiction-search${getLanguageParam(language)}`); | |
| }; | |
| const userTypedButDidNotSelect = displayValue && displayValue.trim().length >= 3 && isEmpty(locationIdStr); | |
| if (userTypedButDidNotSelect) { | |
| return redirectWithError(content.locationNotFound); | |
| } | |
| if (isEmpty(locationIdStr)) { | |
| return redirectWithError(content.locationNameRequired); | |
| } | |
| const locationId = Number.parseInt(locationIdStr!, 10); | |
| if (Number.isNaN(locationId)) { | |
| return redirectWithError(content.locationNotFound); | |
| } | |
| const locationIdStr = typeof req.body.locationId === "string" ? req.body.locationId : undefined; | |
| const displayValue = | |
| typeof req.body["location-search-display"] === "string" ? req.body["location-search-display"] : undefined; | |
| const redirectWithError = (errorText: string) => { | |
| session.locationJurisdictionSearchErrors = [{ text: errorText, href: "`#location-search`" }]; | |
| return res.redirect(`/location-jurisdiction-search${getLanguageParam(language)}`); | |
| }; | |
| const userTypedButDidNotSelect = displayValue && displayValue.trim().length >= 3 && isEmpty(locationIdStr); | |
| if (userTypedButDidNotSelect) { | |
| return redirectWithError(content.locationNotFound); | |
| } | |
| if (isEmpty(locationIdStr)) { | |
| return redirectWithError(content.locationNameRequired); | |
| } | |
| if (!locationIdStr || !/^\d+$/.test(locationIdStr)) { | |
| return redirectWithError(content.locationNotFound); | |
| } | |
| const locationId = Number(locationIdStr); | |
| if (!Number.isSafeInteger(locationId) || locationId <= 0) { | |
| return redirectWithError(content.locationNotFound); | |
| } |
| <div class="govuk-accordion__section"> | ||
| <div class="govuk-accordion__section-header"> | ||
| <h3 class="govuk-accordion__section-heading"> | ||
| <span class="govuk-accordion__section-button" id="accordion-sub-jurisdictions">Sub-Jurisdictions</span> |
There was a problem hiding this comment.
Hardcoded English text breaks Welsh translation.
The accordion heading "Sub-Jurisdictions" is hardcoded in English. Should use {{ subJurisdictionLabel }} to support both languages.
🌍 Proposed fix
- <span class="govuk-accordion__section-button" id="accordion-sub-jurisdictions">Sub-Jurisdictions</span>
+ <span class="govuk-accordion__section-button" id="accordion-sub-jurisdictions">{{ subJurisdictionLabel }}</span>📝 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.
| <span class="govuk-accordion__section-button" id="accordion-sub-jurisdictions">Sub-Jurisdictions</span> | |
| <span class="govuk-accordion__section-button" id="accordion-sub-jurisdictions">{{ subJurisdictionLabel }}</span> |
| <div class="govuk-accordion__section"> | ||
| <div class="govuk-accordion__section-header"> | ||
| <h3 class="govuk-accordion__section-heading"> | ||
| <span class="govuk-accordion__section-button" id="accordion-regions">Regions</span> |
There was a problem hiding this comment.
Hardcoded English text breaks Welsh translation.
The accordion heading "Regions" is hardcoded in English. Should use {{ regionLabel }} to support both languages.
🌍 Proposed fix
- <span class="govuk-accordion__section-button" id="accordion-regions">Regions</span>
+ <span class="govuk-accordion__section-button" id="accordion-regions">{{ regionLabel }}</span>📝 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.
| <span class="govuk-accordion__section-button" id="accordion-regions">Regions</span> | |
| <span class="govuk-accordion__section-button" id="accordion-regions">{{ regionLabel }}</span> |
Resolve file-location conflicts caused by master renaming libs/system-admin-pages/src/pages/ to apps/web/src/pages/(system-admin)/. New pages added on this branch are now at the correct location. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move en.ts/cy.ts content files from apps/web/src/pages/ to libs/system-admin-pages/src/
- Move JurisdictionDataSession from apps/ to libs/system-admin-pages/src/session-types.ts
- Export all new content and types from libs/system-admin-pages/src/index.ts
- Update controllers to import from @hmcts/system-admin-pages (no relative lib imports)
- Replace req.query.lng locale detection with res.locals.locale pattern
- Fix templates to use {% block page_content %} instead of {% block content %}
- Update tests to mock @hmcts/system-admin-pages and use res.locals for locale
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-service Extract checkNameUniqueness helper to remove the repeated findFirst pattern across Jurisdiction, Sub-Jurisdiction, and Region branches, bringing duplication from 3.35% to 0%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The constraint was already added by migration 20260528115459_add_third_party_push_log from master, so the duplicate ADD CONSTRAINT statement in this branch's migration caused P3018 on apply. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…update form - Add dedicated /region-data-* pages (list, create, modify, update, delete, success) separate from jurisdiction flow - Move Manage Region Data tile to reference-data, move Manage Jurisdiction Data to system-admin-dashboard - Remove Region from jurisdiction-data-list filter and jurisdiction-data-create type options - Fix jurisdiction-data-update to show type dropdown only for Sub-Jurisdiction records; parent jurisdiction select shown conditionally based on session type - Fix jurisdiction-data-delete to embed record name in heading instead of summary list - Fix back link text on jurisdiction-data-list showing URL instead of "Back" - Sync dist content files and update all affected tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The dev DB has stale records for removed migrations (20260527140208 and 20260528115459_add_third_party_push_log) that cause migrate deploy to hang, leading to Helm timeout. Matches the fix already applied in PR-669. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolves SonarQube security finding - omitting --ignore-scripts allows arbitrary shell scripts to run during package execution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…admin_audit_log The migration was renamed to 20260605141023_add_jurisdiction_soft_delete_and_audit_log. If the dev DB has the old name recorded, migrate deploy tries to re-apply the same SQL and fails on already-existing tables/columns, blocking the deploy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Deleting 20260529131401_add_soft_delete_and_admin_audit_log causes migrate deploy to re-apply the same SQL, failing on already-existing tables and columns. Rename it to match the current migration file name so Prisma recognises it as already applied. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add selectattr Nunjucks filter to fix per-field error messages on create/update forms - Show parent jurisdiction in summary on modify and delete pages for Sub-Jurisdiction type - Use type-specific dependency error messages (sub-jurisdictions vs locations) - Fix softDeleteLocation to remove junction table rows, preventing orphaned links blocking sub-jurisdiction deletion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dependency checks hasDependencies was counting locationSubJurisdiction and locationRegion rows without filtering out soft-deleted locations, causing false "linked to locations" errors when trying to delete sub-jurisdictions or regions after their courts had been soft-deleted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion or region The FK constraint on location_sub_jurisdiction.sub_jurisdiction_id is RESTRICT, so deleting a sub-jurisdiction fails if any locationSubJurisdiction rows reference it — even when the linked locations are soft-deleted. Wrapping the deleteMany and delete in a transaction ensures orphaned rows are cleaned up first. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ssage Replace hasDependencies (boolean) with getDependencyType which returns the specific blocker — 'sub-jurisdictions', 'locations', or 'list-types'. The service maps this to a precise error message so users know whether deletion is blocked by linked locations or linked list types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…data upload behaviour - Fix getDependencyType to query from location model side so deletedAt filter works correctly for sub-jurisdiction and region active link counts - Fix hardDeleteJurisdictionRecord to only remove orphaned junction rows (soft-deleted locations), not active location links - Fix upsertLocations to clear deletedAt when re-uploading a soft-deleted location, restoring it as active - Fix download CSV to exclude soft-deleted locations - Add /jurisdiction-data link to missing sub-jurisdiction validation error - Fix reference-data-upload-summary template to render html error content Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The assertion was checking for "locations" but the mock returns "sub-jurisdictions", making it test the wrong dependency type. 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. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…alidation messages - Show linked location links on region-data-delete when region has dependencies, linking to /delete-court - Add findLocationsByRegionId query to fetch locations linked to a region - Add html link to missing region validation error pointing to /region-data-create - Update missing sub-jurisdiction validation error link text to "Click here to manage jurisdiction data" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
PR #680 removed the add-jurisdiction/sub-jurisdiction/region buttons from the reference-data-upload page. Remove the stale test that asserted them and the dead locale keys from the remaining tests' render data; keep the submit button and download link coverage. Full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Jira link
#410
Change description
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation