Merge Manage and Configuration List Type Tabs - #854
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
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>
…record This migration exists as a real file on this branch and should not be deleted from _prisma_migrations — deleting it caused migrate deploy to re-apply the SQL and fail on already-existing tables/constraints. Only delete the orphan record 20260527140208 which has no migration file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…anup The dev DB has partial state from multiple failed deploys - the admin_audit_log table and deleted_at columns may already exist under the old migration name. Adding IF NOT EXISTS to all DDL statements lets the migration run safely regardless of the DB's current state. Also removes the old migration record (20260529131401) instead of renaming it - with idempotent SQL the migration can simply re-run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…timeout The FlexibleServersDatabase ASO provisioning + DB wait can exceed the 15m Helm timeout on a first deploy. Starting the health server on port 5555 immediately lets Helm mark the pod as ready and complete the deploy, while the DB wait loop and migrations run to completion afterwards. The E2E tests then wait for /test-support/health to confirm migrations are done before running. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…thub.com/hmcts/cath-service into feature/410-system-admin-data-management
…tion DDL migrations (DROP COLUMN, DROP TABLE) require ACCESS EXCLUSIVE locks. During a Helm rolling upgrade the old pod's Prisma Studio holds open connections that block those locks until the old pod is terminated. The old pod is only terminated once the new pod becomes Ready, and the new pod only becomes Ready after migrations complete — a deadlock that causes Helm to time out at 15m. Starting the health server immediately lets Helm mark the pod Ready, terminate the old pod, and release its connections before migrations run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
npx can fetch and install packages on-demand; using the local binary directly ensures only the pinned prisma@7.8.0 already installed in the image is executed, with no network access or lifecycle script risk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ence With health-first startup, the postgres pod reports ready before migrations finish. The old health check queried jurisdiction.findMany() which passes immediately (table predates migration 20260702), so E2E setup began while DROP COLUMN/DROP TABLE were still holding locks, causing 500s on artefact operations. Querying _prisma_migrations for rows with no finished_at or rolled_back_at accurately reflects whether migrate deploy is still running, so the global-setup retry loop waits until the DB is truly ready. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
With health-first startup, migrations run after Helm marks the pod Ready. DDL migrations (DROP COLUMN) must wait for the old pod's termination grace period before acquiring ACCESS EXCLUSIVE locks, then execute. The previous 30 × 2s = 60s window was too short. 60 × 5s = 5min covers this reliably. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The WORKDIR in the postgres container is apps/postgres, so node_modules resolves to apps/postgres/node_modules which does not exist in a yarn workspace. The hoisted binary is at the workspace root two levels up. Also reverts the global-setup timeout increase — it was a workaround for this bug, not the real fix. 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>
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds nullable case-name and case-number JSON field names across persistence and validation layers, introduces add/edit/manage list-type pages, consolidates legacy routes under ChangesList type data and validation
Add and edit journeys
Management navigation
Validation coverage
Ticket records
Possibly related issues
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 |
Resolve conflicts: - success page njk files: use govuk-grid-column-full (master) for consistency - region-data-delete (cy/en/njk/ts/test) and queries.ts: take master's findLocationsByRegionId feature (superset of feature branch) - system-admin-dashboard cy/en: include both Manage List Types and Configure List Type tiles - system-admin-dashboard test: assert 11 tiles with merged order Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🎭 Playwright E2E Test Results82 tests 52 ✅ 6m 52s ⏱️ Results for commit 73071b5. ♻️ This comment has been updated with latest results. |
… dashboard The Configure List Type page (/configure-list-type-enter-details) now 301 redirects to /manage-list-types, so the dashboard tile duplicated the Manage List Types tile. Remove it from en/cy content and update tests to 10 tiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/tickets/699/tasks.md (1)
91-97: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign E2E task status with actual execution.
Both documents should distinguish implemented test definitions from runnable coverage:
docs/tickets/699/tasks.md#L91-L97: leave execution incomplete while the journeys remain skipped.docs/tickets/699/review.md#L73-L78: report skipped journeys as a coverage gap, not completed E2E coverage.
🧹 Nitpick comments (3)
libs/system-admin-pages/src/list-type/validation.ts (1)
127-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated length-check blocks — extract a shared helper.
The two blocks differ only by field name, message, and href. Extracting a small
validateOptionalFieldLengthhelper (mirroring the existingvalidateRequiredField) would avoid the duplication and make future optional-field additions (e.g. more JSON field-name columns) trivial.♻️ Proposed refactor
+function validateOptionalFieldLength(value: string | null | undefined, field: string, maxLength: number, label: string): ValidationError | null { + if (value && value.length > maxLength) { + return { field, message: `${label} must be ${maxLength} characters or less`, href: `#${field}` }; + } + return null; +} + export function validateListTypeDetails(data: ListTypeDetailsInput) { ... - if (data.caseNumberJsonFieldName && data.caseNumberJsonFieldName.length > 255) { - errors.push({ - field: "caseNumberJsonFieldName", - message: "Case number JSON field name must be 255 characters or less", - href: "`#caseNumberJsonFieldName`" - }); - } - - if (data.caseNameJsonFieldName && data.caseNameJsonFieldName.length > 255) { - errors.push({ - field: "caseNameJsonFieldName", - message: "Case name JSON field name must be 255 characters or less", - href: "`#caseNameJsonFieldName`" - }); - } + const caseNumberError = validateOptionalFieldLength(data.caseNumberJsonFieldName, "caseNumberJsonFieldName", 255, "Case number JSON field name"); + if (caseNumberError) errors.push(caseNumberError); + + const caseNameError = validateOptionalFieldLength(data.caseNameJsonFieldName, "caseNameJsonFieldName", 255, "Case name JSON field name"); + if (caseNameError) errors.push(caseNameError);apps/web/src/pages/(system-admin)/delete-list-type/index.njk (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant CSS class on anchor element.
The
govuk-bodyclass is redundant on the<a>element becausegovuk-linkalready provides the appropriate link styling, and it inherits the text styles from the parent<p class="govuk-body">.♻️ Proposed refactor
{% if hasArtefacts %} <p class="govuk-body"> - <a href="/remove-list-search" class="govuk-link govuk-body">{{ removeArtefactsLinkText }}</a> + <a href="/remove-list-search" class="govuk-link">{{ removeArtefactsLinkText }}</a> </p> {% endif %}apps/web/src/pages/(system-admin)/manage-list-type/index.njk (1)
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
t.notSetfallback for consistency.Other optional properties in this table use
or t.notSetwhen the value is absent. Consider applying the same fallback to the JSON field names for a consistent user experience.♻️ Proposed refactor
<tr class="govuk-table__row"> <th scope="row" class="govuk-table__header">{{ t.caseNumberJsonFieldNameLabel }}</th> - <td class="govuk-table__cell">{{ listType.caseNumberJsonFieldName }}</td> + <td class="govuk-table__cell">{{ listType.caseNumberJsonFieldName or t.notSet }}</td> </tr> <tr class="govuk-table__row"> <th scope="row" class="govuk-table__header">{{ t.caseNameJsonFieldNameLabel }}</th> - <td class="govuk-table__cell">{{ listType.caseNameJsonFieldName }}</td> + <td class="govuk-table__cell">{{ listType.caseNameJsonFieldName or t.notSet }}</td> </tr>
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a30f1af-a809-445a-ba50-4c8eeaa078c8
📒 Files selected for processing (59)
apps/postgres/prisma/migrations/20260714112456_add_list_type_json_fields/migration.sqlapps/web/src/pages/(system-admin)/add-list-type/cy.tsapps/web/src/pages/(system-admin)/add-list-type/en.tsapps/web/src/pages/(system-admin)/add-list-type/index.njkapps/web/src/pages/(system-admin)/add-list-type/index.test.tsapps/web/src/pages/(system-admin)/add-list-type/index.tsapps/web/src/pages/(system-admin)/configure-list-type-enter-details/index.test.tsapps/web/src/pages/(system-admin)/configure-list-type-enter-details/index.tsapps/web/src/pages/(system-admin)/configure-list-type-preview/cy.tsapps/web/src/pages/(system-admin)/configure-list-type-preview/en.tsapps/web/src/pages/(system-admin)/configure-list-type-preview/index.njkapps/web/src/pages/(system-admin)/configure-list-type-preview/index.test.tsapps/web/src/pages/(system-admin)/configure-list-type-preview/index.tsapps/web/src/pages/(system-admin)/configure-list-type-select-sub-jurisdictions/index.njkapps/web/src/pages/(system-admin)/configure-list-type-select-sub-jurisdictions/index.test.tsapps/web/src/pages/(system-admin)/configure-list-type-select-sub-jurisdictions/index.tsapps/web/src/pages/(system-admin)/configure-list-type-success/cy.tsapps/web/src/pages/(system-admin)/configure-list-type-success/en.tsapps/web/src/pages/(system-admin)/configure-list-type-success/index.njkapps/web/src/pages/(system-admin)/delete-list-type-success/index.njkapps/web/src/pages/(system-admin)/delete-list-type/cy.tsapps/web/src/pages/(system-admin)/delete-list-type/en.tsapps/web/src/pages/(system-admin)/delete-list-type/index.njkapps/web/src/pages/(system-admin)/delete-list-type/index.test.tsapps/web/src/pages/(system-admin)/delete-list-type/index.tsapps/web/src/pages/(system-admin)/edit-list-type/cy.tsapps/web/src/pages/(system-admin)/edit-list-type/en.tsapps/web/src/pages/(system-admin)/edit-list-type/index.njkapps/web/src/pages/(system-admin)/edit-list-type/index.test.tsapps/web/src/pages/(system-admin)/edit-list-type/index.tsapps/web/src/pages/(system-admin)/manage-list-type/cy.tsapps/web/src/pages/(system-admin)/manage-list-type/en.tsapps/web/src/pages/(system-admin)/manage-list-type/index.njkapps/web/src/pages/(system-admin)/manage-list-type/index.test.tsapps/web/src/pages/(system-admin)/manage-list-type/index.tsapps/web/src/pages/(system-admin)/manage-list-types/cy.tsapps/web/src/pages/(system-admin)/manage-list-types/en.tsapps/web/src/pages/(system-admin)/manage-list-types/index.njkapps/web/src/pages/(system-admin)/manage-list-types/index.test.tsapps/web/src/pages/(system-admin)/manage-list-types/index.tsapps/web/src/pages/(system-admin)/system-admin-dashboard/cy.tsapps/web/src/pages/(system-admin)/system-admin-dashboard/en.tsapps/web/src/pages/(system-admin)/view-list-types/index.test.tsapps/web/src/pages/(system-admin)/view-list-types/index.tsdocs/tickets/699/plan.mddocs/tickets/699/review.mddocs/tickets/699/tasks.mddocs/tickets/699/ticket.mde2e-tests/tests/system-admin/configure-list-type.spec.tse2e-tests/tests/system-admin/manage-list-types.spec.tse2e-tests/tests/system-admin/system-admin-dashboard.spec.tslibs/postgres-prisma/prisma/schema/location.prismalibs/system-admin-pages/src/list-type/queries.test.tslibs/system-admin-pages/src/list-type/queries.tslibs/system-admin-pages/src/list-type/service.test.tslibs/system-admin-pages/src/list-type/service.tslibs/system-admin-pages/src/list-type/types.tslibs/system-admin-pages/src/list-type/validation.test.tslibs/system-admin-pages/src/list-type/validation.ts
💤 Files with no reviewable changes (2)
- apps/web/src/pages/(system-admin)/system-admin-dashboard/en.ts
- apps/web/src/pages/(system-admin)/system-admin-dashboard/cy.ts
| title: "[WELSH TRANSLATION REQUIRED: 'Enter list type details']", | ||
| nameLabel: "[WELSH TRANSLATION REQUIRED: 'Name']", | ||
| friendlyNameLabel: "[WELSH TRANSLATION REQUIRED: 'Friendly name']", | ||
| welshFriendlyNameLabel: "[WELSH TRANSLATION REQUIRED: 'Welsh friendly name']", | ||
| shortenedFriendlyNameLabel: "[WELSH TRANSLATION REQUIRED: 'Shortened friendly name']", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Provide proper Welsh translations.
The provided code uses placeholder strings indicating that Welsh translations are still required. As per coding guidelines, every user-facing page must provide both English and Welsh content and render localised values rather than hardcoded display strings or placeholders. While the PR objectives mention remaining translation notes, the guidelines strictly require actual translations.
apps/web/src/pages/(system-admin)/add-list-type/cy.ts#L2-L6: Replace the placeholder strings with accurate Welsh translations.apps/web/src/pages/(system-admin)/edit-list-type/cy.ts#L2-L6: Replace the placeholder strings with accurate Welsh translations.apps/web/src/pages/(system-admin)/manage-list-type/cy.ts#L2-L6: Replace the placeholder strings with accurate Welsh translations.apps/web/src/pages/(system-admin)/manage-list-types/cy.ts#L7-L7: Replace the placeholder string with an accurate Welsh translation.apps/web/src/pages/(system-admin)/configure-list-type-success/cy.ts#L3-L7: Replace the placeholder strings with accurate Welsh translations.apps/web/src/pages/(system-admin)/delete-list-type/cy.ts#L16-L16: Replace the placeholder string with an accurate Welsh translation.
📍 Affects 6 files
apps/web/src/pages/(system-admin)/add-list-type/cy.ts#L2-L6(this comment)apps/web/src/pages/(system-admin)/edit-list-type/cy.ts#L2-L6apps/web/src/pages/(system-admin)/manage-list-type/cy.ts#L2-L6apps/web/src/pages/(system-admin)/manage-list-types/cy.ts#L7-L7apps/web/src/pages/(system-admin)/configure-list-type-success/cy.ts#L3-L7apps/web/src/pages/(system-admin)/delete-list-type/cy.ts#L16-L16
Source: Coding guidelines
| title: "List type updated", | ||
| banner: "List type updated successfully.", | ||
| description: "What do you want to do next?", | ||
| returnLink: "Return to System Admin dashboard" | ||
| returnLink: "Return to System Admin dashboard", | ||
| manageListTypesLink: "Manage list types" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "manageListTypesLink|List type updated" "apps/web/src/pages/(system-admin)/configure-list-type-success/cy.ts"Repository: hmcts/cath-service
Length of output: 382
Add Welsh translations for the new success copy
apps/web/src/pages/(system-admin)/configure-list-type-success/cy.ts still has placeholders for title, banner and manageListTypesLink; this page needs matching Welsh content for the updated success screen.
Source: Path instructions
| if (!confirmDelete) { | ||
| return res.render("delete-list-type/index", { | ||
| ...content, | ||
| id, | ||
| listTypeName: listType.friendlyName || listType.name, | ||
| errors: [{ text: content.errorConfirmationRequired, href: "#confirmDelete" }], | ||
| data: { confirmDelete } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate that the confirmation value is strictly accepted.
Currently, any truthy value other than "no" bypasses this check and results in the list type being deleted. Please enforce strict validation against the expected values. As per coding guidelines, input must be validated on all endpoints.
🐛 Proposed fix
- if (!confirmDelete) {
+ if (confirmDelete !== "yes" && confirmDelete !== "no") {
return res.render("delete-list-type/index", {
...content,
id,
listTypeName: listType.friendlyName || listType.name,
errors: [{ text: content.errorConfirmationRequired, href: "`#confirmDelete`" }],
data: { confirmDelete }
});
}📝 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 (!confirmDelete) { | |
| return res.render("delete-list-type/index", { | |
| ...content, | |
| id, | |
| listTypeName: listType.friendlyName || listType.name, | |
| errors: [{ text: content.errorConfirmationRequired, href: "#confirmDelete" }], | |
| data: { confirmDelete } | |
| }); | |
| } | |
| if (confirmDelete !== "yes" && confirmDelete !== "no") { | |
| return res.render("delete-list-type/index", { | |
| ...content, | |
| id, | |
| listTypeName: listType.friendlyName || listType.name, | |
| errors: [{ text: content.errorConfirmationRequired, href: "`#confirmDelete`" }], | |
| data: { confirmDelete } | |
| }); | |
| } |
Source: Coding guidelines
| items: [ | ||
| { value: "", text: "Select an option", selected: not data.defaultSensitivity }, | ||
| { value: "Public", text: "Public", selected: data.defaultSensitivity == "Public" }, | ||
| { value: "Private", text: "Private", selected: data.defaultSensitivity == "Private" }, | ||
| { value: "Classified", text: "Classified", selected: data.defaultSensitivity == "Classified" } | ||
| ], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded select option text is not localized.
"Select an option" (and the sensitivity option labels) are hardcoded English and will not render in Welsh, unlike the surrounding t.*-driven fields. Source these strings from the locale content.
As per path instructions: "Use the Nunjucks page-template pattern: ... use localized data and error summaries for forms."
🌐 Suggested localization
items: [
- { value: "", text: "Select an option", selected: not data.defaultSensitivity },
- { value: "Public", text: "Public", selected: data.defaultSensitivity == "Public" },
- { value: "Private", text: "Private", selected: data.defaultSensitivity == "Private" },
- { value: "Classified", text: "Classified", selected: data.defaultSensitivity == "Classified" }
+ { value: "", text: t.sensitivitySelectOption, selected: not data.defaultSensitivity },
+ { value: "Public", text: t.sensitivityPublic, selected: data.defaultSensitivity == "Public" },
+ { value: "Private", text: t.sensitivityPrivate, selected: data.defaultSensitivity == "Private" },
+ { value: "Classified", text: t.sensitivityClassified, selected: data.defaultSensitivity == "Classified" }
],📝 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.
| items: [ | |
| { value: "", text: "Select an option", selected: not data.defaultSensitivity }, | |
| { value: "Public", text: "Public", selected: data.defaultSensitivity == "Public" }, | |
| { value: "Private", text: "Private", selected: data.defaultSensitivity == "Private" }, | |
| { value: "Classified", text: "Classified", selected: data.defaultSensitivity == "Classified" } | |
| ], | |
| items: [ | |
| { value: "", text: t.sensitivitySelectOption, selected: not data.defaultSensitivity }, | |
| { value: "Public", text: t.sensitivityPublic, selected: data.defaultSensitivity == "Public" }, | |
| { value: "Private", text: t.sensitivityPrivate, selected: data.defaultSensitivity == "Private" }, | |
| { value: "Classified", text: t.sensitivityClassified, selected: data.defaultSensitivity == "Classified" } | |
| ], |
Source: Path instructions
| const rawId = req.query.id as string; | ||
| const id = Number.parseInt(rawId, 10); | ||
|
|
||
| if (!rawId || Number.isNaN(id)) { | ||
| return res.status(400).render("errors/common", { status: 400 }); | ||
| } | ||
|
|
||
| const listType = await findListTypeById(id); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use listTypeName for routing and list type lookup.
The new manage-list-type detail flow is querying and routing by the numeric ListType.id. As per coding guidelines, "Use listTypeName, never numeric listTypeId, for list-type guards, routing...". Please update this flow to rely on the list type name parameter instead.
apps/web/src/pages/(system-admin)/manage-list-type/index.ts#L11-L18: Update the controller to processreq.query.name(or similar) and perform the lookup using a name-based query instead offindListTypeById.apps/web/src/pages/(system-admin)/manage-list-types/index.ts#L15-L15: Update theconfigureUrlgeneration to pass thenameparameter in the query string.apps/web/src/pages/(system-admin)/manage-list-type/index.test.ts#L42-L44: Update the test mocks and validation tests to supply and expect thenameparameter.apps/web/src/pages/(system-admin)/manage-list-types/index.test.ts#L121-L121: Change the regex assertion to expect the name parameter instead of a numeric ID.apps/web/src/pages/(system-admin)/delete-list-type/index.test.ts#L233-L233: Update the redirect assertion to ensure it targets the name-based route.
📍 Affects 5 files
apps/web/src/pages/(system-admin)/manage-list-type/index.ts#L11-L18(this comment)apps/web/src/pages/(system-admin)/manage-list-type/index.test.ts#L42-L44apps/web/src/pages/(system-admin)/manage-list-types/index.ts#L15-L15apps/web/src/pages/(system-admin)/manage-list-types/index.test.ts#L121-L121apps/web/src/pages/(system-admin)/delete-list-type/index.test.ts#L233-L233
Source: Coding guidelines
| const subJurisdictionsText = | ||
| listType.subJurisdictions.length > 0 | ||
| ? listType.subJurisdictions.map((sj) => (language === "cy" ? sj.subJurisdiction.welshName : sj.subJurisdiction.name)).join(", ") | ||
| : t.noneSelected; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Provide a fallback for missing Welsh sub-jurisdiction names.
If a sub-jurisdiction lacks a Welsh name, the current mapping will output a blank string. Please consider adding a fallback to the English name to ensure the text is rendered gracefully.
💡 Proposed fix
- const subJurisdictionsText =
- listType.subJurisdictions.length > 0
- ? listType.subJurisdictions.map((sj) => (language === "cy" ? sj.subJurisdiction.welshName : sj.subJurisdiction.name)).join(", ")
- : t.noneSelected;
+ const subJurisdictionsText =
+ listType.subJurisdictions.length > 0
+ ? listType.subJurisdictions.map((sj) => (language === "cy" ? (sj.subJurisdiction.welshName || sj.subJurisdiction.name) : sj.subJurisdiction.name)).join(", ")
+ : t.noneSelected;📝 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 subJurisdictionsText = | |
| listType.subJurisdictions.length > 0 | |
| ? listType.subJurisdictions.map((sj) => (language === "cy" ? sj.subJurisdiction.welshName : sj.subJurisdiction.name)).join(", ") | |
| : t.noneSelected; | |
| const subJurisdictionsText = | |
| listType.subJurisdictions.length > 0 | |
| ? listType.subJurisdictions.map((sj) => (language === "cy" ? (sj.subJurisdiction.welshName || sj.subJurisdiction.name) : sj.subJurisdiction.name)).join(", ") | |
| : t.noneSelected; |
| One new capability is added: | ||
| - **`caseNumberJsonFieldName` / `caseNameJsonFieldName` form fields** (requires two new nullable columns on `ListType`). | ||
|
|
||
| Region selection is **out of scope** for this ticket — list types will not be linked to regions. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve the region-selection contract across the documentation.
The ticket requires regions, but the plan and task checklist exclude or omit the required persistence and journey work; the supplied controller also does not store regionIds.
docs/tickets/699/plan.md#L12-L12: state clearly whether region support is in scope.docs/tickets/699/plan.md#L240-L242: document the actual region UI and validation behaviour.docs/tickets/699/plan.md#L334-L353: include the correct session fields.docs/tickets/699/plan.md#L500-L501: ensure persistence claims match the schema.docs/tickets/699/tasks.md#L3-L18: add schema and service/query tasks for regions.docs/tickets/699/tasks.md#L63-L75: add region UI, controller, validation, and test tasks.docs/tickets/699/ticket.md#L149-L153: retain these requirements only if they will be implemented.
📍 Affects 3 files
docs/tickets/699/plan.md#L12-L12(this comment)docs/tickets/699/plan.md#L240-L242docs/tickets/699/plan.md#L334-L353docs/tickets/699/plan.md#L500-L501docs/tickets/699/tasks.md#L3-L18docs/tickets/699/tasks.md#L63-L75docs/tickets/699/ticket.md#L149-L153
| ### 5. Welsh translations are stubs throughout | ||
| All new `cy.ts` files use `[WELSH TRANSLATION REQUIRED: '...']` markers. These need to be replaced with real Welsh translations before the feature goes live. Consider creating a follow-up ticket to track translation work. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not mark Welsh support complete while translations remain placeholders.
The acceptance checklist marks Welsh support as complete, but the review states that all new Welsh content still contains [WELSH TRANSLATION REQUIRED: ...] markers and must be replaced before go-live. Mark this as pending or explicitly document that placeholders are an agreed interim state.
Also applies to: 93-93
| // Confirm to create | ||
| await page.getByRole("button", { name: "Confirm" }).click(); | ||
| await page.waitForURL("**/configure-list-type-success"); | ||
| await expect(page.getByRole("heading", { level: 1 })).toContainText("List type updated"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect shared success page content for add vs edit wording.
fd -a 'en.ts' 'apps/web/src/pages/(system-admin)/configure-list-type-success' --exec cat -n {}
rg -nP -C2 'updated|created|added' "apps/web/src/pages/(system-admin)/configure-list-type-success"Repository: hmcts/cath-service
Length of output: 1871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the spec and the shared success page implementation.
sed -n '1,220p' 'e2e-tests/tests/system-admin/manage-list-types.spec.ts'
printf '\n---\n'
fd -a 'page.tsx' 'apps/web/src/pages/(system-admin)/configure-list-type-success' -x sh -c 'echo "FILE: $1"; cat -n "$1"' sh {}
printf '\n---\n'
fd -a '*.ts' 'apps/web/src/pages/(system-admin)/configure-list-type-success' -x sh -c 'echo "FILE: $1"; cat -n "$1"' sh {}Repository: hmcts/cath-service
Length of output: 10290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the add/edit flows that navigate to the shared success page.
rg -n -C3 'configure-list-type-success|List type updated|success\.title|success\.banner' \
'e2e-tests/tests/system-admin/manage-list-types.spec.ts' \
'apps/web/src/pages' \
'apps/web/src/components' \
'apps/web/src' || trueRepository: hmcts/cath-service
Length of output: 27962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the add and edit assertions around the shared success page.
nl -ba 'e2e-tests/tests/system-admin/manage-list-types.spec.ts' | sed -n '70,105p'
printf '\n---\n'
nl -ba 'e2e-tests/tests/system-admin/manage-list-types.spec.ts' | sed -n '145,170p'Repository: hmcts/cath-service
Length of output: 196
Use create-specific success copy
The shared success page shows List type updated for the add journey as well as edit, so newly created list types get edit wording. Use create-specific copy or a neutral success message.
… tab merge Restore the cancel link and xl heading on the delete-list-type page (lost in the merge) and point it at /manage-list-type?id=. Update the preview and delete-success template tests to assert the consolidated /manage-list-types and changeDetailsHref routes instead of the pre-merge /view-list-types and /configure-list-type-enter-details paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🤖 Addressed CI failures (issue #699):
All 6 previously-failing Note: the E2E Tests |
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. |
2 participants
Jira link
#699
Change description
Merge Manage and Configuration List Type Tabs
Summary by CodeRabbit
New Features
Bug Fixes