Feature-805 - Add Financial List (ChD/KB) daily cause list - #929
Conversation
…-chd-list' into Feature-805-financial-List-(ChD/KB)-daily-cause-list
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughAdds the Financial List CHD/KB daily cause-list module. It supports Excel conversion, validation, bilingual web rendering, PDF generation, notifications, publication processing, and application registration. ChangesFinancial List CHD/KB daily cause list
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Converter
participant Database
participant GETHandler
participant Renderer
participant Page
Admin->>Converter: Upload Excel cause list
Converter->>Database: Store validated JSON artefact
GETHandler->>Database: Load hearing-list artefact
GETHandler->>Renderer: Provide hearings, locale, dates, and provenance
Renderer->>Page: Render localised cause list
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
…al-List-(ChD/KB)-daily-cause-list # Conflicts: # apps/web/package.json # libs/list-types/common/src/list-type-data.ts # libs/publication/src/processing/service.ts # yarn.lock
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f89f03a-6479-46ea-af9b-997ea552c7f5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
apps/web/package.jsonapps/web/src/app.tsapps/web/src/pages/(admin)/non-strategic-upload-summary/index.tsapps/web/src/pages/(admin)/non-strategic-upload/index.tsapps/web/src/pages/(list-types)/financial-list-chd-kb-daily-cause-list/financial-list-chd-kb-daily-cause-list.njkapps/web/src/pages/(list-types)/financial-list-chd-kb-daily-cause-list/financial-list-chd-kb-daily-cause-list.njk.test.tsapps/web/src/pages/(list-types)/financial-list-chd-kb-daily-cause-list/index.test.tsapps/web/src/pages/(list-types)/financial-list-chd-kb-daily-cause-list/index.tsdocs/tickets/805/plan.mddocs/tickets/805/review.mddocs/tickets/805/tasks.mddocs/tickets/805/ticket.mdlibs/list-types/common/src/list-type-data.tslibs/list-types/financial-list-chd-kb-daily-cause-list/package.jsonlibs/list-types/financial-list-chd-kb-daily-cause-list/src/config.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/conversion/financial-list-chd-kb-daily-cause-list-config.test.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/conversion/financial-list-chd-kb-daily-cause-list-config.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/index.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/locales/cy.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/locales/en.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/pdf/pdf-generator.test.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/pdf/pdf-generator.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/pdf/pdf-template.njklibs/list-types/financial-list-chd-kb-daily-cause-list/src/rendering/renderer.test.tslibs/list-types/financial-list-chd-kb-daily-cause-list/src/rendering/renderer.tslibs/list-types/financial-list-chd-kb-daily-cause-list/tsconfig.jsonlibs/notifications/package.jsonlibs/notifications/src/notification/notification-service.tslibs/publication/package.jsonlibs/publication/src/processing/service.tstsconfig.json
| "@hmcts/et-daily-list": "workspace:*", | ||
| "@hmcts/et-fortnightly-list": "workspace:*", | ||
| "@hmcts/family-daily-cause-list": "workspace:*", | ||
| "@hmcts/financial-list-chd-kb-daily-cause-list": "workspace:*", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pin the workspace dependency.
"workspace:*" does not identify a dependency version. Set this dependency to the declared package version by using the repository-approved workspace version format.
As per coding guidelines, “pin dependency versions”.
Source: Coding guidelines
| describe("Welsh rendering", () => { | ||
| it("should render Welsh headings, table headers and labels", () => { | ||
| const { $ } = renderPage({ hearings: [buildHearing({ judge: "Barnwr A", venue: "Lleoliad A" })] }, cy); | ||
|
|
||
| expect($("h1#top").text()).toContain(cy.pageTitle); | ||
| expect($(".govuk-grid-column-full").text()).toContain(cy.venueName); | ||
| expect($("h2.govuk-heading-s").text()).toContain(cy.searchCasesTitle); | ||
|
|
||
| // The Welsh pageTitle placeholder contains double quotes, so query the table by class | ||
| // rather than by its aria-label (which would break the CSS attribute selector). | ||
| const headers = $(".hearings-table thead th") | ||
| .map((_, el) => $(el).text().trim()) | ||
| .get(); | ||
| expect(headers).toContain(cy.tableHeaders.judge); | ||
| expect(headers).toContain(cy.tableHeaders.time); | ||
| expect(headers).toContain(cy.tableHeaders.venue); | ||
| expect(headers).toContain(cy.tableHeaders.caseNumber); | ||
|
|
||
| const firstRowCells = $(".hearings-table tbody tr") | ||
| .eq(0) | ||
| .find("td") | ||
| .map((_, el) => $(el).text().trim()) | ||
| .get(); | ||
| expect(firstRowCells[COLUMN.judge]).toBe("Barnwr A"); | ||
| expect($(".back-to-top a").text()).toContain(cy.backToTop); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test Welsh content through the locale query.
This test injects cy into the template. It does not test the i18n middleware or the ?lng=cy route contract. Add a route-level test that requests ?lng=cy and asserts Welsh page content.
As per coding guidelines, “Test Welsh page content using the ?lng=cy query parameter.”
Source: Coding guidelines
| vi.mocked(getArtefactById).mockResolvedValue(mockArtefact as any); | ||
| vi.mocked(getPublicationJson).mockResolvedValue(mockJsonData); | ||
| mockValidate.mockReturnValue({ isValid: true, errors: [] }); | ||
| vi.mocked(renderFinancialListChdKbDailyCauseList).mockReturnValue(mockRenderedData as any); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unjustified any casts.
These casts prevent TypeScript from checking the artefact and rendered-data contracts. Define complete typed fixtures or type-safe test doubles instead.
As per coding guidelines, “Do not use any without justification; TypeScript strict mode is required.”
Also applies to: 187-187, 214-214, 244-244, 306-309, 347-350, 387-390, 425-428
Source: Coding guidelines
| Statement coverage per changed workspace (`vitest run --coverage`): | ||
|
|
||
| | Workspace | Statements | Status | | ||
| |-----------|-----------|--------| | ||
| | `libs/list-types/financial-list-chd-kb-daily-cause-list` | 90.9% | ✅ | | ||
| | `libs/notifications` | 90.53% | ✅ | | ||
| | `libs/publication` | 96.5% | ✅ | | ||
| | `apps/web` (new controller) | 31 tests pass; filtered run shows 44% only because it reports the shared `list-type-handler.ts`, not the new controller | ✅ (advisory) | | ||
|
|
||
| Note on `apps/web`: the filtered coverage number reflects the shared `list-type-handler.ts` helper (partially exercised by a single filtered run), not the new page controller, whose 31 tests all pass. The new controller itself is thin and fully exercised by `index.test.ts`. Not flagged as below-threshold since the new code is well covered. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the overall coverage statement.
The coverage table reports apps/web at 44% for the filtered run, but line 93 states that all workspace coverage is above 80%. The note explains the scope limitation, but it does not make the aggregate statement accurate. Replace the claim with a scoped statement that distinguishes the new controller tests from workspace coverage.
Also applies to: 89-93
🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: Possible missing comma found.
Context: ...y index.test.ts. Not flagged as below-threshold since the new code is well covered. ##...
(AI_HYDRA_LEO_MISSING_COMMA)
| Reference style guide (staging): | ||
| `https://pip-frontend.staging.platform.hmcts.net/financial-list-chd-kb-daily-cause-list?artefactId=bb5307f2-e0fd-4d72-8ae8-b72457413eb8` | ||
|
|
||
| This list follows the same flat single-table pattern used by the RCJ standard daily cause lists (`libs/list-types/rcj-standard-daily-cause-list`), but with a distinct field set. It **cannot** reuse `RCJ_EXCEL_CONFIG` because the columns differ (`type` not `hearingType`, `caseName` not `caseDetails`, and both `caseNumber` and `caseName` are present). It therefore requires its own module, schema, model, Excel config, renderer, and PDF generator. | ||
|
|
||
| **Reference implementation to mirror:** `libs/list-types/rcj-standard-daily-cause-list/` and its page controller at `apps/web/src/pages/(list-types)/rcj-standard-daily-cause-list/`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the ticket with the shared ChD/KB module design.
This section requires a bespoke schema, model, Excel configuration, and validation module. The current plan and review record define a thin wrapper over @hmcts/chd-kb-common with no local schemas/, validation/, or models/ directories. Keeping both designs can cause duplicate validation contracts and schema drift. Update the ticket to describe the shared-module approach.
Also applies to: 209-238
| **A. Excel-to-JSON conversion config** — a new `ExcelConverterConfig` (do **not** reuse `RCJ_EXCEL_CONFIG`), with columns in the required order and `validateNoHtmlTags` on text fields plus `validateTimeFormat` on `Time`: | ||
|
|
||
| | Header | fieldName | required | validators | | ||
| |--------|-----------|----------|------------| | ||
| | Judge | `judge` | true | `validateNoHtmlTags` | | ||
| | Time | `time` | true | `validateTimeFormat` | | ||
| | Venue | `venue` | true | `validateNoHtmlTags` | | ||
| | Type | `type` | true | `validateNoHtmlTags` | | ||
| | Case Number | `caseNumber` | true | `validateNoHtmlTags` | | ||
| | Case Name | `caseName` | true | `validateNoHtmlTags` | | ||
| | Additional Information | `additionalInformation` | false | `validateNoHtmlTags` | | ||
|
|
||
| `minRows: 1`. Register with `registerConverterByName("FINANCIAL_LIST_CHD_KB_DAILY_CAUSE_LIST", converter)`. | ||
|
|
||
| **B. JSON Schema** (`schemas/financial-list-chd-kb-daily-cause-list.json`), draft-07, root `type: array`, required `judge`, `time`, `venue`, `type`, `caseNumber`, `caseName`, `additionalInformation`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document additionalInformation as required.
Line 305 marks additionalInformation as optional, but libs/list-types/chd-kb-common/src/conversion/chd-kb-excel-config.test.ts:30-34 expects an empty value to be rejected. Line 309 also lists the field as required in the JSON schema. Set the Excel configuration entry to true.
Proposed documentation fix
-| Additional Information | `additionalInformation` | false | `validateNoHtmlTags` |
+| Additional Information | `additionalInformation` | true | `validateNoHtmlTags` |📝 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. Excel-to-JSON conversion config** — a new `ExcelConverterConfig` (do **not** reuse `RCJ_EXCEL_CONFIG`), with columns in the required order and `validateNoHtmlTags` on text fields plus `validateTimeFormat` on `Time`: | |
| | Header | fieldName | required | validators | | |
| |--------|-----------|----------|------------| | |
| | Judge | `judge` | true | `validateNoHtmlTags` | | |
| | Time | `time` | true | `validateTimeFormat` | | |
| | Venue | `venue` | true | `validateNoHtmlTags` | | |
| | Type | `type` | true | `validateNoHtmlTags` | | |
| | Case Number | `caseNumber` | true | `validateNoHtmlTags` | | |
| | Case Name | `caseName` | true | `validateNoHtmlTags` | | |
| | Additional Information | `additionalInformation` | false | `validateNoHtmlTags` | | |
| `minRows: 1`. Register with `registerConverterByName("FINANCIAL_LIST_CHD_KB_DAILY_CAUSE_LIST", converter)`. | |
| **B. JSON Schema** (`schemas/financial-list-chd-kb-daily-cause-list.json`), draft-07, root `type: array`, required `judge`, `time`, `venue`, `type`, `caseNumber`, `caseName`, `additionalInformation`. | |
| **A. Excel-to-JSON conversion config** — a new `ExcelConverterConfig` (do **not** reuse `RCJ_EXCEL_CONFIG`), with columns in the required order and `validateNoHtmlTags` on text fields plus `validateTimeFormat` on `Time`: | |
| | Header | fieldName | required | validators | | |
| |--------|-----------|----------|------------| | |
| | Judge | `judge` | true | `validateNoHtmlTags` | | |
| | Time | `time` | true | `validateTimeFormat` | | |
| | Venue | `venue` | true | `validateNoHtmlTags` | | |
| | Type | `type` | true | `validateNoHtmlTags` | | |
| | Case Number | `caseNumber` | true | `validateNoHtmlTags` | | |
| | Case Name | `caseName` | true | `validateNoHtmlTags` | | |
| | Additional Information | `additionalInformation` | true | `validateNoHtmlTags` | | |
| `minRows: 1`. Register with `registerConverterByName("FINANCIAL_LIST_CHD_KB_DAILY_CAUSE_LIST", converter)`. | |
| **B. JSON Schema** (`schemas/financial-list-chd-kb-daily-cause-list.json`), draft-07, root `type: array`, required `judge`, `time`, `venue`, `type`, `caseNumber`, `caseName`, `additionalInformation`. |
| "@hmcts/chd-kb-common": "workspace:*", | ||
| "@hmcts/list-types-common": "workspace:*", | ||
| "@hmcts/pdf-generation": "workspace:*", | ||
| "@hmcts/postgres-prisma": "workspace:*", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
libs/list-types/financial-list-chd-kb-daily-cause-list/package.json \
libs/publication/package.json \
libs/notifications/package.json
do
echo "== $file =="
jq -r '
(.dependencies // {} | to_entries[] | select(.value == "workspace:*") | "\(.key)=\(.value)"),
(.peerDependencies // {} | to_entries[] | select(.value | test("[*^~]")) | "\(.key)=\(.value)")
' "$file"
doneRepository: hmcts/cath-service
Length of output: 4702
Pin dependency declarations in these package manifests.
These package.json files still use unbounded dependency versions. Replace workspace:* entries with exact package versions, and keep Express entries as exact Express 5.x versions.
libs/list-types/financial-list-chd-kb-daily-cause-list/package.json: pin its workspace dependencies andexpresspeer dependency (currently^5.1.0).libs/publication/package.json: pin the@hmcts/financial-list-chd-kb-daily-cause-listdependency.libs/notifications/package.json: pin the@hmcts/financial-list-chd-kb-daily-cause-listdependency.
📍 Affects 3 files
libs/list-types/financial-list-chd-kb-daily-cause-list/package.json#L26-L29(this comment)libs/list-types/financial-list-chd-kb-daily-cause-list/package.json#L40-L41libs/publication/package.json#L40-L40libs/notifications/package.json#L37-L37
Source: Coding guidelines
| () => import("../locales/cy.js") | ||
| ); | ||
|
|
||
| const provenanceLabel = options.provenance ? PROVENANCE_LABELS[options.provenance as keyof typeof PROVENANCE_LABELS] || options.provenance : ""; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the locale-specific provenance label.
This lookup does not use options.locale. A Welsh PDF can therefore show the English data-source label. Resolve the label from the selected locale’s provenanceLabels, then fall back to the raw provenance value. Add Welsh provenance coverage to the PDF test.
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
🎭 Playwright E2E Test Results82 tests 52 ✅ 6m 22s ⏱️ Results for commit 99068ac. ♻️ 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. |
…al-List-(ChD/KB)-daily-cause-list
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…al-List-(ChD/KB)-daily-cause-list
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
2 participants
Jira link
#805
Change description
implement Financial List (ChD/KB) daily cause list style guide + PDF
closes #805
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation