Add civil-daily-cause-list and family-daily-cause-list modules (#594) - #663
Conversation
Implements two new list-type modules for civil and family daily cause lists, each with a page controller, renderer, PDF generator, JSON schema validator, and full unit test coverage (56 tests). Also fixes a bug in both renderers where calculateDuration called formatTime after an early return, preventing sittings without an end time from having their time property set. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds two list-type libraries ( ChangesCivil and Family Daily Cause List
Sequence Diagram(s)sequenceDiagram
participant Client as Browser
participant WebApp as Web App
participant Prisma as Postgres (Prisma)
participant Renderer as renderCauseListData
participant PDFGen as generate{Civil|Family}Pdf
participant Storage as PDF Storage
Client->>WebApp: GET /.../daily-cause-list?artefactId=...
WebApp->>Prisma: fetch artefact & listType
WebApp->>WebApp: read artefact JSON from temp upload dir
WebApp->>Renderer: renderCauseListData(jsonData, {locationId, contentDate, locale})
Renderer-->>WebApp: header, openJustice, listData
WebApp->>PDFGen: generate*Pdf(options)
PDFGen->>Storage: savePdfToStorage(artefactId, buffer, sizeBytes)
Storage-->>PDFGen: confirmation
WebApp-->>Client: render page or return PDF
Possibly related issues
Possibly related pull requests
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
🎭 Playwright E2E Test Results84 tests 52 ✅ 4m 7s ⏱️ Results for commit 7970d28. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
libs/list-types/family-daily-cause-list/src/pdf/pdf-generator.ts (1)
55-63: ⚡ Quick win
sizeBytes!guarded by the currentgeneratePdfFromHtmlcontract (optional hardening)
generatePdfFromHtmlsetssizeBytes: pdfBuffer.lengthwhenever it returnssuccess: trueandpdfBuffer, so the existing check (!pdfResult.success || !pdfResult.pdfBuffer) already impliessizeBytesis a number at runtime. The type still markssizeBytesas optional (sizeBytes?: number), so adding an explicitsizeBytesguard is only future-proofing, not a correctness fix.libs/list-types/civil-daily-cause-list/src/rendering/renderer.test.ts (1)
192-209: ⚡ Quick winAdd a Welsh
lastUpdatedassertion in the locale test.This test currently only checks
contentDate. Adding one assertion onheader.lastUpdatedwill lock in full Welsh formatting behaviour and catch mixed-language regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdeddd0b-ebd7-4a04-8efe-cf40bc988bea
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
apps/web/package.jsonapps/web/src/app.tsdocs/tickets/594/review.mddocs/tickets/594/tasks.mdlibs/list-types/civil-daily-cause-list/package.jsonlibs/list-types/civil-daily-cause-list/src/config.tslibs/list-types/civil-daily-cause-list/src/index.tslibs/list-types/civil-daily-cause-list/src/models/types.tslibs/list-types/civil-daily-cause-list/src/pages/civil-daily-cause-list.njklibs/list-types/civil-daily-cause-list/src/pages/cy.tslibs/list-types/civil-daily-cause-list/src/pages/en.tslibs/list-types/civil-daily-cause-list/src/pages/index.test.tslibs/list-types/civil-daily-cause-list/src/pages/index.tslibs/list-types/civil-daily-cause-list/src/pdf/pdf-generator.test.tslibs/list-types/civil-daily-cause-list/src/pdf/pdf-generator.tslibs/list-types/civil-daily-cause-list/src/pdf/pdf-template.njklibs/list-types/civil-daily-cause-list/src/rendering/renderer.test.tslibs/list-types/civil-daily-cause-list/src/rendering/renderer.tslibs/list-types/civil-daily-cause-list/src/schemas/civil-daily-cause-list.jsonlibs/list-types/civil-daily-cause-list/src/validation/json-validator.test.tslibs/list-types/civil-daily-cause-list/src/validation/json-validator.tslibs/list-types/civil-daily-cause-list/tsconfig.jsonlibs/list-types/family-daily-cause-list/package.jsonlibs/list-types/family-daily-cause-list/src/config.tslibs/list-types/family-daily-cause-list/src/index.tslibs/list-types/family-daily-cause-list/src/models/types.tslibs/list-types/family-daily-cause-list/src/pages/cy.tslibs/list-types/family-daily-cause-list/src/pages/en.tslibs/list-types/family-daily-cause-list/src/pages/family-daily-cause-list.njklibs/list-types/family-daily-cause-list/src/pages/index.test.tslibs/list-types/family-daily-cause-list/src/pages/index.tslibs/list-types/family-daily-cause-list/src/pdf/pdf-generator.test.tslibs/list-types/family-daily-cause-list/src/pdf/pdf-generator.tslibs/list-types/family-daily-cause-list/src/pdf/pdf-template.njklibs/list-types/family-daily-cause-list/src/rendering/renderer.test.tslibs/list-types/family-daily-cause-list/src/rendering/renderer.tslibs/list-types/family-daily-cause-list/src/schemas/family-daily-cause-list.jsonlibs/list-types/family-daily-cause-list/src/validation/json-validator.test.tslibs/list-types/family-daily-cause-list/src/validation/json-validator.tslibs/list-types/family-daily-cause-list/tsconfig.jsontsconfig.json
|
|
||
| Two new list-type modules have been added: `@hmcts/civil-daily-cause-list` and `@hmcts/family-daily-cause-list`. Both are correctly structured under `libs/list-types/` and follow the existing module patterns from comparable modules such as `@hmcts/civil-and-family-daily-cause-list`. Both modules are registered in `apps/web/src/app.ts`, the root `tsconfig.json`, and `apps/web/package.json`. All 28 unit tests pass in each module and the linter reports no issues. | ||
|
|
||
| The implementation is broadly solid and consistent with the established codebase conventions. There are no security vulnerabilities or accessibility blockers. However, there are several issues that need attention before deployment: pervasive use of `as any` type casts in the renderer, an unvalidated query parameter used directly in a file-system path, inline `<style>` blocks in templates, and near-total code duplication between the two new modules (and against the existing `civil-and-family` module). There is also a missing `publishedAt` display in both templates despite the translation key being defined, and duration labels are hardcoded English strings in templates that should come from the `t` object. |
There was a problem hiding this comment.
Resolve the contradictory security statement.
Line 7 says there are no security vulnerabilities, but the same section and later critical issue #1 describe a path traversal risk. Please align this wording so the risk posture is unambiguous.
| - [ ] Add path aliases to root `tsconfig.json` | ||
| - [ ] Register modules in `apps/web/src/app.ts` | ||
| - [ ] Add packages as dependencies in `apps/web/package.json` | ||
|
|
There was a problem hiding this comment.
Update checklist status for completed registration tasks.
These items are implemented in this PR but still marked unchecked. Please mark them as completed to keep ticket tracking accurate.
| {% set durationText = '' %} | ||
| {% if sitting.durationAsHours > 0 %} | ||
| {% if sitting.durationAsHours > 1 %} | ||
| {% set durationText = sitting.durationAsHours ~ ' hours' %} | ||
| {% else %} | ||
| {% set durationText = sitting.durationAsHours ~ ' hour' %} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% if sitting.durationAsMinutes > 0 %} | ||
| {% if durationText | length %} | ||
| {% set durationText = durationText ~ ' ' %} | ||
| {% endif %} | ||
| {% if sitting.durationAsMinutes > 1 %} | ||
| {% set durationText = durationText ~ sitting.durationAsMinutes ~ ' mins' %} | ||
| {% else %} | ||
| {% set durationText = durationText ~ sitting.durationAsMinutes ~ ' min' %} | ||
| {% endif %} | ||
| {% endif %} |
There was a problem hiding this comment.
Localise duration units instead of hard-coding English text.
Lines 117-129 use "hour/hours/min/mins" directly, so Welsh pages still show English duration units.
Suggested direction
- {% set durationText = sitting.durationAsHours ~ ' hours' %}
+ {% set durationText = sitting.durationAsHours ~ ' ' ~ t.hoursPlural %}
...
- {% set durationText = durationText ~ sitting.durationAsMinutes ~ ' min' %}
+ {% set durationText = durationText ~ sitting.durationAsMinutes ~ ' ' ~ t.minutesSingular %}Add matching keys in en.ts and cy.ts (hoursSingular, hoursPlural, minutesSingular, minutesPlural).
| const artefactId = req.query.artefactId as string; | ||
|
|
||
| if (!artefactId) { |
There was a problem hiding this comment.
Validate artefactId runtime type before using it.
Line 22 force-casts the query value to string, but Express query values can be arrays/objects. Add a typeof artefactId === "string" guard (and trim/format check) before DB and file operations.
Suggested fix
- const artefactId = req.query.artefactId as string;
-
- if (!artefactId) {
+ const artefactIdParam = req.query.artefactId;
+ if (typeof artefactIdParam !== "string" || artefactIdParam.trim().length === 0) {
return res.status(400).render("errors/common", {
en,
cy,
errorTitle: t.errorTitle,
errorMessage: t.errorMessage
});
}
+ const artefactId = artefactIdParam.trim();📝 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 artefactId = req.query.artefactId as string; | |
| if (!artefactId) { | |
| const artefactIdParam = req.query.artefactId; | |
| if (typeof artefactIdParam !== "string" || artefactIdParam.trim().length === 0) { | |
| return res.status(400).render("errors/common", { | |
| en, | |
| cy, | |
| errorTitle: t.errorTitle, | |
| errorMessage: t.errorMessage | |
| }); | |
| } | |
| const artefactId = artefactIdParam.trim(); |
| @@ -0,0 +1,125 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
Lang attribute should be dynamic based on locale.
The lang attribute is hardcoded to "en", but the template must support Welsh locale. For Welsh PDFs, this should be "cy".
🌍 Suggested fix
Pass the locale parameter to the template (in pdf-generator.ts) and use it:
-<html lang="en">
+<html lang="{{ locale }}">| {% set durationText = '' %} | ||
| {% if sitting.durationAsHours > 0 %} | ||
| {% if sitting.durationAsHours > 1 %} | ||
| {% set durationText = sitting.durationAsHours ~ ' hours' %} | ||
| {% else %} | ||
| {% set durationText = sitting.durationAsHours ~ ' hour' %} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% if sitting.durationAsMinutes > 0 %} | ||
| {% if durationText | length %} | ||
| {% set durationText = durationText ~ ' ' %} | ||
| {% endif %} | ||
| {% if sitting.durationAsMinutes > 1 %} | ||
| {% set durationText = durationText ~ sitting.durationAsMinutes ~ ' mins' %} | ||
| {% else %} | ||
| {% set durationText = durationText ~ sitting.durationAsMinutes ~ ' min' %} | ||
| {% endif %} | ||
| {% endif %} |
There was a problem hiding this comment.
Duration units are hardcoded in English.
The duration calculation uses hardcoded English strings ("hours", "hour", "mins", "min") instead of the translation object t. Welsh PDFs will incorrectly display English units.
🌍 Suggested fix
Add translation keys for duration units to en.ts and cy.ts, then use them:
{% if sitting.durationAsHours > 1 %}
- {% set durationText = sitting.durationAsHours ~ ' hours' %}
+ {% set durationText = sitting.durationAsHours ~ ' ' ~ t.hours %}
{% else %}
- {% set durationText = sitting.durationAsHours ~ ' hour' %}
+ {% set durationText = sitting.durationAsHours ~ ' ' ~ t.hour %}
{% endif %}Apply the same pattern for t.mins and t.min.
| function formatPublicationDateTime(isoDateTime: string, locale: string): string { | ||
| const dt = DateTime.fromISO(isoDateTime).setZone("Europe/London").setLocale(locale); | ||
|
|
||
| const dateStr = dt.toFormat("d MMMM yyyy"); | ||
|
|
||
| const hours = dt.hour; | ||
| const minutes = dt.minute; | ||
| const period = hours >= 12 ? "pm" : "am"; | ||
| const hour12 = hours % 12 || 12; | ||
|
|
||
| const minuteStr = minutes > 0 ? `:${minutes.toString().padStart(2, "0")}` : ""; | ||
| const timeStr = `${hour12}${minuteStr}${period}`; | ||
|
|
||
| return `${dateStr} at ${timeStr}`; | ||
| } |
There was a problem hiding this comment.
lastUpdated is not fully localised for Welsh.
The formatted value always inserts "at", so Welsh output mixes languages.
Suggested fix
function formatPublicationDateTime(isoDateTime: string, locale: string): string {
const dt = DateTime.fromISO(isoDateTime).setZone("Europe/London").setLocale(locale);
@@
- return `${dateStr} at ${timeStr}`;
+ const connector = locale === "cy" ? "am" : "at";
+ return `${dateStr} ${connector} ${timeStr}`;
}| "isPresiding": { | ||
| "description": "A flag to indicate whether a judiciary is presiding", | ||
| "title": "Is Presiding", | ||
| "type": "boolean", | ||
| "examples": [true], | ||
| "default": "" | ||
| } |
There was a problem hiding this comment.
Fix invalid default value for boolean field.
The isPresiding boolean field has a default value of "" (empty string). Boolean fields must have boolean default values (true or false) or omit the default entirely.
🛠️ Proposed fix
"isPresiding": {
"description": "A flag to indicate whether a judiciary is presiding",
"title": "Is Presiding",
"type": "boolean",
"examples": [true],
- "default": ""
+ "default": false
}📝 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.
| "isPresiding": { | |
| "description": "A flag to indicate whether a judiciary is presiding", | |
| "title": "Is Presiding", | |
| "type": "boolean", | |
| "examples": [true], | |
| "default": "" | |
| } | |
| "isPresiding": { | |
| "description": "A flag to indicate whether a judiciary is presiding", | |
| "title": "Is Presiding", | |
| "type": "boolean", | |
| "examples": [true], | |
| "default": false | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/list-types/civil-daily-cause-list/src/pages/civil-daily-cause-list.njk (1)
123-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocalise duration units via translation keys, not hard-coded English literals.
'hours'/'hour'/'mins'/'min'are still embedded in the template, so Welsh pages will render mixed language output for durations.As per coding guidelines and learnings, pages must support both English and Welsh content passed from controllers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1a47396-30a5-4769-a07f-4d236c415441
📒 Files selected for processing (14)
apps/web/src/app.test.tslibs/list-types/civil-daily-cause-list/src/pages/civil-daily-cause-list.njklibs/list-types/civil-daily-cause-list/src/pages/cy.tslibs/list-types/civil-daily-cause-list/src/pages/en.tslibs/list-types/civil-daily-cause-list/src/pages/index.test.tslibs/list-types/civil-daily-cause-list/src/pdf/pdf-template.njklibs/list-types/civil-daily-cause-list/src/rendering/renderer.test.tslibs/list-types/family-daily-cause-list/src/pages/cy.tslibs/list-types/family-daily-cause-list/src/pages/en.tslibs/list-types/family-daily-cause-list/src/pages/family-daily-cause-list.njklibs/list-types/family-daily-cause-list/src/pages/index.test.tslibs/list-types/family-daily-cause-list/src/pdf/pdf-template.njklibs/list-types/family-daily-cause-list/src/rendering/renderer.test.tslibs/publication/src/processing/service.ts
💤 Files with no reviewable changes (2)
- libs/list-types/civil-daily-cause-list/src/pages/en.ts
- libs/list-types/civil-daily-cause-list/src/pages/cy.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/list-types/civil-daily-cause-list/src/pdf/pdf-template.njk
- libs/publication/src/processing/service.ts
- libs/list-types/family-daily-cause-list/src/pages/index.test.ts
- libs/list-types/family-daily-cause-list/src/pages/en.ts
- libs/list-types/family-daily-cause-list/src/pages/family-daily-cause-list.njk
- libs/list-types/family-daily-cause-list/src/pdf/pdf-template.njk
- libs/list-types/civil-daily-cause-list/src/pages/index.test.ts
…ation dependencies The publication service imports from @hmcts/civil-daily-cause-list and @hmcts/family-daily-cause-list but these were missing from package.json, causing the API to crash on startup with ERR_MODULE_NOT_FOUND. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ause-list dependencies 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
…ause list - Replace duplicate renderer.ts in civil-daily-cause-list and family-daily-cause-list with re-exports from the shared civil-and-family-daily-cause-list module, eliminating the 17.7% code duplication that was blocking the quality gate - Replace duplicate models/types.ts in both modules with re-exports from the shared module - Fix calculateDuration in shared renderer to set sitting time before early return, restoring correct behaviour when sittingEnd is absent - Replace unnecessary non-null assertions (sizeBytes!) with nullish coalescing - Add @hmcts/civil-and-family-daily-cause-list as explicit dependency in both packages 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
| @@ -0,0 +1 @@ | |||
| export { renderCauseListData } from "@hmcts/civil-and-family-daily-cause-list"; | |||
There was a problem hiding this comment.
It is a bit odd to call the method in an independent lib this way. If Civil, Family and Mixed List share the same renderer. Is that not better to add all of them to the same lib?
Can we ask AI and see what he can do?
There was a problem hiding this comment.
Move render functionality into one libs and all three lists are now calling common render class.
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…t packages Neither package has a src/pages/ directory so the build:nunjucks step was failing with 'cd: no such file or directory: src/pages'. These are lib-only packages with PDF templates only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
2 participants
Summary
@hmcts/civil-daily-cause-listmodule underlibs/list-types/civil-daily-cause-list/with page controller, renderer, PDF generator, JSON schema validator, and English/Welsh translations@hmcts/family-daily-cause-listmodule underlibs/list-types/family-daily-cause-list/with the same structureapps/web/src/app.tsandapps/web/package.json, and adds path aliases to the roottsconfig.jsoncalculateDurationcalledformatTimeafter an early return, causing sittings without an end time to never have theirtimeproperty setTest plan
yarn workspace @hmcts/civil-daily-cause-list test— 28 tests passyarn workspace @hmcts/family-daily-cause-list test— 28 tests passyarn lint— no errors/civil-daily-cause-list?artefactId=<id>/family-daily-cause-list?artefactId=<id>?lng=cy) and confirm all text is translated🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation