VIBE-339 Add PDF and email summary for RCJ and Care Standards List - #325
Conversation
# Conflicts: # yarn.lock
📝 WalkthroughWalkthroughAdds PDF generation (Puppeteer), per-list PDF and email-summary builders, a publication processing service that generates PDFs and triggers subscription notifications, GOV.UK Notify integration supporting PDF uploads and template selection (PDF+summary vs summary-only), and wiring from uploads/blob-ingestion into the new flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Upload as Admin UI / Blob Ingest
participant Publication as Publication Service
participant PDF as PDF Generator
participant Storage as Temp Storage / Document Service
participant Notify as GOV.UK Notify
Upload->>Publication: processPublication(artefactId, listTypeId, jsonData, provenance, ...)
activate Publication
Publication->>PDF: generate...Pdf(artefactId, jsonData, locale, contentDate, provenance, ...)
activate PDF
PDF->>Storage: savePdfToStorage(artefactId, pdfBuffer)
Storage-->>PDF: pdfPath, sizeBytes, exceedsMaxSize
deactivate PDF
Publication->>Notify: sendPublicationNotificationsForArtefact(locationId, pdfPath?, caseSummary?, ...)
activate Notify
Notify->>Notify: selectTemplate(hasPdf, sizeUnder2MB)
alt attach PDF
Notify->>Storage: prepareUpload(pdfBuffer)
Storage-->>Notify: link_to_file
Notify->>Notify: sendEmail(templateId, personalisation including link_to_file)
else no PDF attachment
Notify->>Notify: sendEmail(templateId, personalisation)
end
Notify-->>Publication: per-subscriber results (sent/failed/skipped)
deactivate Notify
Publication-->>Upload: aggregated results
deactivate Publication
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright E2E Test Results238 tests 238 ✅ 21m 20s ⏱️ Results for commit d823a4b. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/notifications/src/notification/notification-service.ts (1)
127-154:⚠️ Potential issue | 🟠 MajorAudit log left in "Pending" if an exception is thrown after creation.
If
buildEmailTemplateData(line 135) orsendEmail(line 137) throws, thecatchblock on line 151 returns{ status: "failed" }but never callsupdateNotificationStatusfor the notification created at line 127. That record will remain "Pending" indefinitely.Suggested fix
} catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); + if (notification) { + await updateNotificationStatus(notification.notificationId, "Failed", undefined, errorMessage).catch(() => {}); + } return { status: "failed", error: `User ${subscription.userId}: ${errorMessage}` }; }This requires hoisting
notificationabove the try or restructuring slightly so it's in scope. An alternative is to wrap the post-audit-log logic in its own try/catch.
🟡 Minor comments (21)
apps/web/.env.example-44-45 (1)
44-45:⚠️ Potential issue | 🟡 MinorDefault
CATH_SERVICE_URLpoints to production.The
.env.exampleuses the live production URL as the default. Other entries use placeholder values (e.g.template-uuid-here,your-api-key-here). Consider using a local or placeholder value to avoid accidental production-pointing in non-production environments.Suggested change
# Service URL for generating links in notifications -CATH_SERVICE_URL=https://www.court-tribunal-hearings.service.gov.uk +CATH_SERVICE_URL=https://localhost:8080libs/list-types/rcj-standard-daily-cause-list/src/pages/en.ts-131-134 (1)
131-134:⚠️ Potential issue | 🟡 MinorFix typo: "formally" should be "formerly".
"Note this document contains Special Category Data as defined by Data Protection Act 2018, formally known as Sensitive Personal Data…" uses the wrong word. "Formerly" means "in the past"; "formally" means "in a formal manner". This typo appears across 7 locale files. Consider fixing the shared constant in
libs/list-types/common/src/email-summary/case-summary-formatter.tsto resolve it in multiple list types at once.libs/list-types/civil-and-family-daily-cause-list/src/pages/cy.ts-40-43 (1)
40-43:⚠️ Potential issue | 🟡 MinorInconsistent Welsh translations for caution notice fields across list types.
The
cautionNoteandcautionReportingfields use two distinct Welsh variants: this file uses "Noder … Neddf Gwarchod Data 2018 … ei drin yn y ffordd briodol" and "Mae'r ddogfen hon yn cynnwys gwybodaeth a fwriedir …", whilst administrative-court, rcj-standard, court-of-appeal-civil, and london-administrative-court use "Sylwer … Ddeddf Diogelu Data 2018 … ei thrin yn briodol" and "Mae'r ddogfen hon yn cynnwys gwybodaeth sydd â'r bwriad …" respectively. Both translations are valid Welsh, but these notices should use consistent wording across all list types.libs/list-types/civil-and-family-daily-cause-list/src/pdf/pdf-template.njk-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorHardcoded
lang="en"— should reflect the locale.The PDF supports Welsh (
cy) translations but the HTML lang attribute is alwaysen. This affects accessibility metadata in the generated PDF.Proposed fix
-<html lang="en"> +<html lang="{{ locale | default('en') }}">docs/tickets/VIBE-341/plan.md-415-423 (1)
415-423:⚠️ Potential issue | 🟡 MinorTypo: "formally known as" should be "formerly known as".
Line 417 in the
SPECIAL_CATEGORY_DATA_WARNINGtext uses "formally" (meaning "in a formal manner") instead of "formerly" (meaning "in the past"). This is a legal notice that may be copy-pasted into production code.e2e-tests/tests/subscription-notifications.spec.ts-296-302 (1)
296-302:⚠️ Potential issue | 🟡 MinorHardcoded sleep before asserting zero notifications is fragile.
Line 298 uses a raw 2-second
setTimeout, thenwaitForNotificationswith only 3 retries × 500ms. If the system is under load, a notification could arrive after this window, causing a false pass—or the test could be needlessly slow in normal conditions. Consider increasing the wait or documenting the timing rationale.docs/tickets/VIBE-339/plan.md-237-239 (1)
237-239:⚠️ Potential issue | 🟡 MinorTypo: "formally known as" → "formerly known as".
This constant appears in the email summary builder template. Same issue flagged in the VIBE-341 docs—ensure the production
SPECIAL_CATEGORY_DATA_WARNINGconstant uses the correct word.docs/tickets/VIBE-341/ticket.md-52-52 (1)
52-52:⚠️ Potential issue | 🟡 MinorTypo: "formally known as" → "formerly known as".
Same issue as in plan.md. This text is a legal notice that will likely be used verbatim in email templates.
libs/list-types/civil-and-family-daily-cause-list/package.json-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor
build:pdf-templateswill fail ifsrc/pdf/*.njkmatches nothing.The
cp src/pdf/*.njk dist/pdf/command will error if the glob matches no files (shell default behaviour). Thebuild:nunjucksscript usesfindwhich handles empty results gracefully. Consider aligning the approach or adding a glob guard.Proposed fix
- "build:pdf-templates": "mkdir -p dist/pdf && cp src/pdf/*.njk dist/pdf/", + "build:pdf-templates": "mkdir -p dist/pdf && find src/pdf -name '*.njk' -exec cp {} dist/pdf/ \\;",e2e-tests/tests/subscription-notifications.spec.ts-235-236 (1)
235-236:⚠️ Potential issue | 🟡 MinorConsider using unique email addresses for each test user to avoid potential GOV.UK Notify deduplication.
Whilst
testUser1andtestUser2have distinct user IDs and separate subscriptions, they share the same email address. If GOV.UK Notify deduplicates notifications by email, this test may not properly validate multi-subscriber behaviour. Generate unique emails per user (e.g., by appending a timestamp or random identifier to the email address) to ensure each subscriber receives independent notifications.libs/list-types/rcj-standard-daily-cause-list/src/pdf/pdf-template.njk-136-138 (1)
136-138:⚠️ Potential issue | 🟡 MinorHardcoded English string "No hearings scheduled." — should use a translation key.
This string won't be translated for Welsh locale.
Proposed fix
- <p>No hearings scheduled.</p> + <p>{{ t.common.noHearingsScheduled }}</p>libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/en.ts-40-41 (1)
40-41:⚠️ Potential issue | 🟡 MinorFix typo: "formally" should be "formerly" across multiple list-type files.
The phrase "formally known as" is incorrect; the intended phrase is "formerly known as" (previously). This user-facing legal text appears in at least six list-type locale files and one shared common file, suggesting the text was copied with the same typo. All instances require correction:
libs/list-types/court-of-appeal-civil-daily-cause-list/src/pages/en.ts:41libs/list-types/civil-and-family-daily-cause-list/src/pages/en.ts:41libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/en.ts:26libs/list-types/london-administrative-court-daily-cause-list/src/pages/en.ts:35libs/list-types/administrative-court-daily-cause-list/src/pages/en.ts:56libs/list-types/rcj-standard-daily-cause-list/src/pages/en.ts:132libs/list-types/common/src/email-summary/case-summary-formatter.ts:1libs/admin-pages/src/pages/manual-upload-summary/index.ts-111-119 (1)
111-119:⚠️ Potential issue | 🟡 MinorSilent swallow of JSON parse errors may mask corrupted uploads.
When a file has a
.jsonextension (Line 90:isFlatFile = false) but fails to parse, the error is silently ignored andjsonDatastaysundefined. The upload proceeds as if it were a flat file, but the database record still hasisFlatFile: false.This mismatch between the stored metadata and actual processing could lead to confusion during debugging. Consider logging a warning so administrators have visibility.
Suggested improvement
try { jsonData = JSON.parse(uploadData.file.toString("utf8")); } catch { - // Not valid JSON, treat as flat file + console.warn("[Manual Upload] File has .json extension but failed to parse as JSON, treating as flat file:", { + artefactId, + fileName: uploadData.fileName + }); }libs/notifications/src/notification/notification-service.test.ts-360-410 (1)
360-410:⚠️ Potential issue | 🟡 MinorTest name "summary-only" contradicts assertion expecting no summary.
Line 403 asserts
getSubscriptionTemplateIdForListTypeis called with(8, false, false), indicating no summary and no PDF. However, the test is named "should use summary-only template for Civil and Family list without PDF", which suggests a summary should be present.Comparing with related tests: when a PDF is provided (with or without size restrictions), the assertion passes
truefor hasSummary. Only when no PDF is provided does the current test expectfalse. The test name should either be corrected to reflect "standard template without PDF" or the assertion should be updated to(8, true, false)to match the stated intent.libs/notifications/src/govnotify/template-config.test.ts-111-136 (1)
111-136:⚠️ Potential issue | 🟡 MinorPotential timezone-dependent test flakiness.
new Date("2025-01-15")is parsed as UTC midnight. TheformatPublicationDateimplementation usesdate.getDate()(local time), so in timezones behind UTC (e.g. US-based CI runners),getDate()may return14instead of15, causing failures.Use explicit UTC dates with times to avoid ambiguity, or construct dates that are timezone-safe:
Suggested fix
- const date = new Date("2025-01-15"); - expect(formatPublicationDate(date)).toBe("15 January 2025"); + const date = new Date(2025, 0, 15); // local date, no UTC ambiguity + expect(formatPublicationDate(date)).toBe("15 January 2025");Apply the same approach to other date constructions in this block (lines 120, 127–129).
libs/list-types/administrative-court-daily-cause-list/src/pdf/pdf-generator.ts-39-42 (1)
39-42:⚠️ Potential issue | 🟡 MinorMisleading default for unrecognised
listTypeId.Line 42 falls back to
"BIRMINGHAM_ADMINISTRATIVE_COURT_DAILY_CAUSE_LIST"for any unknown ID. This silently produces a PDF labelled as Birmingham, which could be confusing. A generic fallback like"ADMINISTRATIVE_COURT_DAILY_CAUSE_LIST"or an early error would be more appropriate.The same concern applies to the title fallback on line 41, though that one is more generic.
libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts-131-138 (1)
131-138:⚠️ Potential issue | 🟡 MinorSilent JSON parse failure may hide upstream issues.
If a
.jsonfile is uploaded but contains malformed JSON,jsonDatasilently remainsundefinedandprocessPublicationproceeds without it. This could lead to a successful upload response with no PDF or email summary generated, confusing the user.Consider logging a warning so operators can diagnose why a JSON upload produced no summary.
Proposed fix
} else { // Parse JSON data for JSON files try { jsonData = JSON.parse(uploadData.file.toString("utf8")); - } catch { - // Not valid JSON + } catch (parseError) { + console.warn("[Non-Strategic Upload] Failed to parse uploaded file as JSON:", parseError); } }libs/list-types/administrative-court-daily-cause-list/src/pdf/pdf-generator.ts-82-82 (1)
82-82:⚠️ Potential issue | 🟡 MinorNon-null assertion on
sizeBytesis risky.
pdfResult.sizeBytes!assumes the value is always defined whensuccessis true andpdfBufferexists. If the contract isn't enforced upstream, this silently passesundefinedtosavePdfToStorage.Consider defaulting:
pdfResult.sizeBytes ?? pdfResult.pdfBuffer.length.Proposed fix
- return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes!); + return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes ?? pdfResult.pdfBuffer.length);libs/list-types/court-of-appeal-civil-daily-cause-list/src/pdf/pdf-generator.ts-51-60 (1)
51-60:⚠️ Potential issue | 🟡 MinorNon-null assertion on
pdfResult.sizeBytes!is unguarded.Line 53 checks
pdfResult.successandpdfResult.pdfBuffer, butsizeBytescould still beundefined. IfsavePdfToStorageexpects anumber, this silently passesundefined.Proposed fix
- return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes!); + return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes ?? 0);libs/publication/src/processing/service.ts-49-55 (1)
49-55:⚠️ Potential issue | 🟡 MinorNon-null assertions on
displayFrom!anddisplayTo!are unsafe.These fields are optional in
GeneratePdfParams(lines 20-21). IfprocessPublicationis called for a Care Standards list withoutdisplayFrom/displayTo, this silently passesundefineddespite the assertion.Proposed fix — guard before invoking
CARE_STANDARDS_TRIBUNAL_WEEKLY_HEARING_LIST: (p) => + p.displayFrom && p.displayTo + ? generateCareStandardsTribunalWeeklyHearingListPdf({ ...p, jsonData: p.jsonData as CareStandardsTribunalHearingList, - displayFrom: p.displayFrom!, - displayTo: p.displayTo! - }), + displayFrom: p.displayFrom, + displayTo: p.displayTo + }) + : Promise.resolve({ success: false, error: "displayFrom and displayTo are required for Care Standards" }),libs/list-types/rcj-standard-daily-cause-list/src/pdf/pdf-generator.ts-69-69 (1)
69-69:⚠️ Potential issue | 🟡 MinorUnsafe cast on
translationslookup.
listTypeKeymay not exist as a key intranslations, makingcourtTranslationssilentlyundefined. This would surface as a template rendering error at runtime.Proposed fix
- const courtTranslations = translations[listTypeKey as keyof typeof translations] as Record<string, string>; + const courtTranslations = translations[listTypeKey as keyof typeof translations] as Record<string, string> | undefined; + if (!courtTranslations) { + return createPdfErrorResult(new Error(`No translations found for list type key: ${listTypeKey}`)); + }
🧹 Nitpick comments (30)
libs/list-types/rcj-standard-daily-cause-list/src/pages/en.ts (1)
131-134: Consider extracting shared caution strings to@hmcts/list-types-common.The
cautionNoteandcautionReportingtexts are duplicated verbatim across multiple list-type locale files (RCJ, London Admin, etc.). The AI summary mentions a sharedSPECIAL_CATEGORY_DATA_WARNINGconstant already exists in the common module — these strings could live there too, reducing copy-paste drift.libs/list-types/administrative-court-daily-cause-list/package.json (1)
17-17:build:nunjucksscript is becoming unwieldy.This one-liner now performs two separate
find-and-copy passes (pages + pdf). As more template directories are added, this will keep growing. Consider extracting it into a small shell script (e.g.,scripts/copy-templates.sh) that all list-type packages can share.libs/list-types/care-standards-tribunal-weekly-hearing-list/src/pages/en.ts (1)
25-28: Consider centralising shared caution strings.The
cautionNoteandcautionReportingtext is identical across sixen.tslocale files (care-standards, administrative-court, civil-and-family, london-administrative-court, court-of-appeal, and rcj-standard). Extract these to@hmcts/list-types-commonand re-export from each module to maintain a single source of truth for future updates.docs/tickets/VIBE-339/ticket.md (1)
50-54: Add language identifiers to fenced code blocks.The static analysis tool flags these code blocks as missing language specifiers. Adding
textorplaintextwould satisfy the linter.-``` +```text Note this email contains Special Category Data...libs/list-types/london-administrative-court-daily-cause-list/package.json (1)
17-17: Longbuild:nunjucksscript duplicated across packages.This shell one-liner is repeated identically in multiple
package.jsonfiles. Consider extracting it into a shared script (e.g., a root-levelscripts/build-nunjucks.sh) to reduce maintenance burden.e2e-tests/tests/subscription-notifications.spec.ts (1)
153-303: Consider consolidating into a single journey test.The coding guidelines state E2E tests should minimise test count with one test per complete user journey. These three tests (single subscriber, multiple subscribers, no-subscription location) could be combined into one sequential journey, with assertions inline.
This would also improve efficiency since the
afterEachteardown and re-setup overhead is repeated for each test.As per coding guidelines: "E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests."
libs/list-types/civil-and-family-daily-cause-list/src/rendering/renderer.ts (1)
85-104: Pervasiveas anymutation pattern degrades type safety.Functions like
calculateDuration,formatHearingChannel,processParties, andformatReportingRestrictionsall mutate their input objects via(x as any).field = value. This defeats TypeScript's type system and makes the data flow hard to reason about. Consider extending the types or returning new objects instead.This is pre-existing rather than introduced by this PR, so flagging as optional for a future refactor.
libs/list-types/common/src/pdf/pdf-utilities.ts (1)
9-12: Fragile monorepo root resolution via relative path traversal.Five levels of
..fromlibs/list-types/common/src/pdf/assumes a fixed directory depth. If the file is ever moved or the structure changes, this silently breaks. Consider deriving the root from a workspace marker (e.g., searching upward forpackage.jsonwithworkspaces) or using an environment variable.libs/pdf-generation/src/generator.ts (1)
25-28:--no-sandboxdisables Chrome's sandbox — document the justification.This is typically required in containerised environments but is a security trade-off. A brief comment explaining why would help future maintainers.
libs/list-types/care-standards-tribunal-weekly-hearing-list/package.json (1)
17-17: Consider extracting the duplicatedbuild:nunjucksscript into a shared shell script or workspace-level task.This exact command is duplicated across multiple list-type
package.jsonfiles (e.g., RCJ, civil-and-family). A single shared script would reduce maintenance burden and the risk of divergence.libs/list-types/civil-and-family-daily-cause-list/src/pdf/pdf-generator.ts (1)
62-62: Non-null assertion onpdfResult.sizeBytes!relies on an implicit contract.The guard on line 55 checks
successandpdfBuffer, butsizeBytescould theoretically beundefinedeven when both are truthy. Consider includingsizeBytesin the guard or defaulting topdfBuffer.length.Proposed fix
- return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes!); + return await savePdfToStorage(options.artefactId, pdfResult.pdfBuffer, pdfResult.sizeBytes ?? pdfResult.pdfBuffer.length);libs/list-types/rcj-standard-daily-cause-list/src/pdf/pdf-template.njk (1)
84-91: Duplicate<h4>forcourt.bundlesTitlewhen bothbundleFilingTextandbundlesTextexist.If both
court.bundleFilingTextandcourt.bundlesTextare present, the heading renders twice. Consider combining them under a single conditional block.Proposed fix
- {% if court.bundlesTitle and court.bundleFilingText %} - <h4>{{ court.bundlesTitle }}</h4> - <p>{{ court.bundleFilingText }}</p> - {% endif %} - {% if court.bundlesTitle and court.bundlesText %} - <h4>{{ court.bundlesTitle }}</h4> - <p>{{ court.bundlesText }}</p> - {% endif %} + {% if court.bundlesTitle and (court.bundleFilingText or court.bundlesText) %} + <h4>{{ court.bundlesTitle }}</h4> + {% if court.bundleFilingText %}<p>{{ court.bundleFilingText }}</p>{% endif %} + {% if court.bundlesText %}<p>{{ court.bundlesText }}</p>{% endif %} + {% endif %}libs/list-types/london-administrative-court-daily-cause-list/src/pdf/pdf-template.njk (2)
2-2: Hardcodedlang="en"won't reflect Welsh locale.If this template is rendered with Welsh translations, the
langattribute should match. Consider making it dynamic, e.g.{{ language | default("en") }}.Proposed fix
-<html lang="en"> +<html lang="{{ language | default('en') }}">
32-93: Duplicate table markup formainHearingsandplanningCourt.Both sections share identical column definitions and row templates. A Nunjucks macro could reduce duplication, but this is a minor template concern and fine to defer.
libs/notifications/src/notification/notification-service.test.ts (1)
53-82: Verbose mock re-initialisation inbeforeEach.The top-level
vi.mock()blocks (Lines 4–41) already define default implementations. Sincevi.clearAllMocks()resets them, every mock is duplicated insidebeforeEach. This is functionally correct but adds maintenance overhead — if you change a default, you must update two places.Consider extracting shared mock-setup helpers, or removing the inline return values from the top-level
vi.mock()blocks and relying solely onbeforeEach.libs/list-types/administrative-court-daily-cause-list/src/email-summary/summary-builder.test.ts (2)
5-9: Inconsistent assertion depth forSPECIAL_CATEGORY_DATA_WARNINGacross list types.This test only checks for
"Special Category Data"(Line 7), while the equivalent test incourt-of-appeal-civil-daily-cause-listalso asserts the presence of"Data Protection Act 2018". Consider aligning the assertions for consistency.
11-55: Solid coverage of extraction and formatting.Tests correctly validate the label/value mapping for Administrative Court hearings (including the
Hearing typefield, which is specific to this list type) and the email formatting output.Consider adding an edge-case test for an empty hearing list, matching the pattern in the Court of Appeal Civil test suite.
libs/list-types/london-administrative-court-daily-cause-list/src/email-summary/summary-builder.test.ts (1)
11-51: Consider adding edge-case tests for empty data.The sibling test in
court-of-appeal-civil-daily-cause-listcovers empty hearing lists and emptyformatCaseSummaryForEmailinput. Adding similar cases here would ensure consistent coverage across list types.libs/notifications/src/govnotify/govnotify-client.ts (1)
63-64: SpreadingtemplateParametersinto a genericRecord<string, unknown>loses type safety.The
TemplateParameterstype is widened toRecord<string, unknown>, which means any accidental key collision (e.g. a template parameter namedlink_to_file) would be silently overwritten on line 72. This is a minor concern but worth noting.libs/list-types/london-administrative-court-daily-cause-list/src/pdf/pdf-generator.ts (1)
53-60: Non-null assertion onpdfResult.sizeBytes!trusts an implicit contract.The guard checks
!pdfResult.pdfBufferbut notsizeBytes. IfgeneratePdfFromHtmlever returns a buffer withoutsizeBytes, this silently passesundefinedtosavePdfToStorage.That said, this matches the pattern in the other PDF generators (e.g.
civil-and-family-daily-cause-list,care-standards-tribunal), so it's consistent. If you do tighten it, consider doing so across all generators.libs/list-types/administrative-court-daily-cause-list/src/pdf/pdf-generator.test.ts (1)
46-103: Test coverage is limited to the happy path.Only success and render-option verification are tested. Missing coverage for:
- PDF generation failure (
success: false)- Exception thrown by
renderAdminCourtorgeneratePdfFromHtml- Provenance label resolution
Other list-type PDF generator tests in this PR may have the same gap. Consider adding at least an error-path test.
libs/api/src/blob-ingestion/repository/service.test.ts (1)
305-306:setTimeout(50)for async assertions is fragile.This pattern (repeated at lines 306, 333, 358, 383) relies on a timing assumption for fire-and-forget async work. It works because mocks resolve instantly, but it's brittle and slow. Consider a
flushPromisesutility orvi.waitFor()for more deterministic async test control.Example flushPromises helper
const flushPromises = () => new Promise((resolve) => setImmediate(resolve));Then replace:
- await new Promise((resolve) => setTimeout(resolve, 50)); + await flushPromises();libs/notifications/package.json (1)
24-32: Growing list of per-list-type dependencies in the notifications package.Each new list type will require adding another dependency here. Consider whether a registry pattern or dynamic import approach (similar to the converter registry in
@hmcts/list-types-common) could decouple notifications from individual list-type packages, avoiding this linear growth.Not blocking — the current approach works and is explicit.
libs/api/src/blob-ingestion/repository/service.ts (2)
103-103: Unnecessary cast toCauseListData.
processPublicationacceptsjsonData?: unknown. Casting toCauseListDatahere is misleading — the data could be any list type's shape. Pass it as-is.Proposed fix
- jsonData: request.hearing_list as CauseListData, + jsonData: request.hearing_list,
77-77: Duplicated provenance resolution.
PROVENANCE_MAP[request.provenance] || request.provenanceappears on both Line 77 and Line 104. Extract to a local variable once.Proposed fix
+ const resolvedProvenance = PROVENANCE_MAP[request.provenance] || request.provenance; + const artefactId = await createArtefact({ ... - provenance: PROVENANCE_MAP[request.provenance] || request.provenance, + provenance: resolvedProvenance, ... }); ... - provenance: PROVENANCE_MAP[request.provenance] || request.provenance, + provenance: resolvedProvenance,Also applies to: 104-104
libs/notifications/src/govnotify/template-config.ts (2)
48-59: Index signaturestring | unknown | undefinedreduces tounknown.This effectively disables type checking for any dynamic key access on
TemplateParameters. If the intent is to allowlink_to_file(added by govnotify-client), consider typing it explicitly as an optional field instead.Proposed fix
export interface TemplateParameters { locations: string; ListType: string; content_date: string; start_page_link: string; subscription_page_link: string; display_summary?: string; summary_of_cases?: string; - [key: string]: string | unknown | undefined; + link_to_file?: unknown; }
14-14: Boolean parameterpdfUnder2MBshould useisprefix per guidelines.The coding guidelines specify booleans should use
is/has/canprefixes. ConsiderisPdfUnder2MB.Also,
_listTypeIdis currently unused. If it's not needed yet, consider removing it to align with YAGNI. Adding it later is a non-breaking change (callers can pass an extra arg).libs/list-types/civil-and-family-daily-cause-list/src/email-summary/summary-builder.ts (1)
55-84: Six levels of nesting is hard to follow.The deep nesting mirrors the data model, so it's structurally driven. Consider extracting a
flatMapCaseshelper to flatten the traversal and reduce cognitive load.libs/publication/src/processing/service.ts (1)
10-10: RenamemockListTypesto reflect its actual purpose.The name is misleading:
mockListTypesis legitimate production reference data containing list type definitions (e.g.,CIVIL_DAILY_CAUSE_LIST), not mock/test data. It is used correctly in service.ts for list type lookups.Per coding guidelines, constants should use
SCREAMING_SNAKE_CASE. Consider renaming toLIST_TYPESorLIST_TYPE_REGISTRYin the shared library (@hmcts/list-types-common). This would require coordinated updates across packages that import from this library.libs/notifications/src/notification/notification-service.ts (1)
165-176: Skip reasons are collected into theerrorsarray.
skipNotificationreturns{ status: "skipped", error: "User X: No email address" }, andaggregateResults(line 256–258) pushes that intoresult.errors. Downstream consumers may not expect skip reasons mixed in with actual errors. Consider a separatewarningsorskippedReasonsfield, or only push errors for"failed"status.Also applies to: 252-262
| expect(govNotifyEmail.body).toContain("Unsubscribe"); | ||
|
|
||
| // Verify email delivery status | ||
| expect(govNotifyEmail.status).toMatch(/delivered|sending|pending|created|permanent-failure/); |
There was a problem hiding this comment.
permanent-failure should not be an acceptable delivery status.
Including permanent-failure in the expected status regex means the test will pass even when the email permanently failed to deliver. This undermines the purpose of verifying delivery.
Proposed fix
- expect(govNotifyEmail.status).toMatch(/delivered|sending|pending|created|permanent-failure/);
+ expect(govNotifyEmail.status).toMatch(/delivered|sending|pending|created/);📝 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.
| expect(govNotifyEmail.status).toMatch(/delivered|sending|pending|created|permanent-failure/); | |
| expect(govNotifyEmail.status).toMatch(/delivered|sending|pending|created/); |
| </tbody> | ||
| </table> | ||
| {% else %} | ||
| <p>No hearings scheduled.</p> |
There was a problem hiding this comment.
Hardcoded English string bypasses translations.
"No hearings scheduled." should use a translation key (e.g. {{ t.common.noHearingsMessage }}) to support Welsh rendering, consistent with how other text in this template is handled.
Proposed fix
- <p>No hearings scheduled.</p>
+ <p>{{ t.common.noHearingsMessage }}</p>📝 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.
| <p>No hearings scheduled.</p> | |
| <p>{{ t.common.noHearingsMessage }}</p> |
| courtName: "Care Standards Tribunal", | ||
| displayFrom: options.displayFrom, | ||
| displayTo: options.displayTo, | ||
| lastReceivedDate: new Date().toISOString(), |
There was a problem hiding this comment.
lastReceivedDate: new Date().toISOString() captures PDF generation time, not actual data receipt time.
This means the "last updated" timestamp in the PDF will be whenever the PDF happens to be generated, which is misleading. Consider passing the actual last-received date through PdfGenerationOptions instead.
| </tbody> | ||
| </table> | ||
| {% else %} | ||
| <p>No hearings scheduled.</p> |
There was a problem hiding this comment.
Hardcoded English string — use a translation key.
"No hearings scheduled." won't render in Welsh. Use a translation key such as {{ t.noHearingsScheduled }}.
Proposed fix
- <p>No hearings scheduled.</p>
+ <p>{{ t.noHearingsScheduled }}</p>📝 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.
| <p>No hearings scheduled.</p> | |
| <p>{{ t.noHearingsScheduled }}</p> |
| function convertPartyRole(role: string): string { | ||
| const roleMap: Record<string, string> = { | ||
| APPLICANT_PETITIONER: "APPLICANT_PETITIONER", | ||
| APPLICANT_PETITIONER_REPRESENTATIVE: "APPLICANT_PETITIONER_REPRESENTATIVE", | ||
| RESPONDENT: "RESPONDENT", | ||
| RESPONDENT_REPRESENTATIVE: "RESPONDENT_REPRESENTATIVE" | ||
| }; | ||
|
|
||
| return roleMap[role] || role; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
convertPartyRole is an identity function — every key maps to itself.
The roleMap maps each role string to the same string, and the fallback || role returns the input unchanged. This function always returns its argument unmodified.
Remove it and use party.partyRole directly, or implement the actual transformation if one is planned. As per coding guidelines: "Follow YAGNI principle: Don't add speculative functionality."
Proposed fix
-function convertPartyRole(role: string): string {
- const roleMap: Record<string, string> = {
- APPLICANT_PETITIONER: "APPLICANT_PETITIONER",
- APPLICANT_PETITIONER_REPRESENTATIVE: "APPLICANT_PETITIONER_REPRESENTATIVE",
- RESPONDENT: "RESPONDENT",
- RESPONDENT_REPRESENTATIVE: "RESPONDENT_REPRESENTATIVE"
- };
-
- return roleMap[role] || role;
-}
-
function createPartyDetails(party: Party): string {Then on line 41:
- const role = convertPartyRole(party.partyRole);
+ const role = party.partyRole;📝 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.
| function convertPartyRole(role: string): string { | |
| const roleMap: Record<string, string> = { | |
| APPLICANT_PETITIONER: "APPLICANT_PETITIONER", | |
| APPLICANT_PETITIONER_REPRESENTATIVE: "APPLICANT_PETITIONER_REPRESENTATIVE", | |
| RESPONDENT: "RESPONDENT", | |
| RESPONDENT_REPRESENTATIVE: "RESPONDENT_REPRESENTATIVE" | |
| }; | |
| return roleMap[role] || role; | |
| } |
| {% 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 text is hardcoded in English.
Lines 79–92 use hardcoded strings (hours, hour, mins, min). These won't render correctly for Welsh locale PDFs. Use translation keys instead (e.g., {{ t.hours }}, {{ t.mins }}).
| export async function savePdfToStorage(artefactId: string, pdfBuffer: Buffer, sizeBytes: number): Promise<PdfGenerationResult> { | ||
| const exceedsMaxSize = sizeBytes > MAX_PDF_SIZE_BYTES; | ||
|
|
||
| await fs.mkdir(TEMP_STORAGE_BASE, { recursive: true }); | ||
| const pdfPath = path.join(TEMP_STORAGE_BASE, `${artefactId}.pdf`); | ||
| await fs.writeFile(pdfPath, pdfBuffer); | ||
|
|
||
| return { | ||
| success: true, | ||
| pdfPath, | ||
| sizeBytes, | ||
| exceedsMaxSize | ||
| }; | ||
| } |
There was a problem hiding this comment.
artefactId is used unsanitised in a file path — path traversal risk.
If artefactId ever contains path separators (e.g., ../../malicious), path.join(TEMP_STORAGE_BASE, ${artefactId}.pdf) could write outside the intended directory. Even if the value is currently system-generated, defensive validation is warranted.
Proposed fix
export async function savePdfToStorage(artefactId: string, pdfBuffer: Buffer, sizeBytes: number): Promise<PdfGenerationResult> {
const exceedsMaxSize = sizeBytes > MAX_PDF_SIZE_BYTES;
await fs.mkdir(TEMP_STORAGE_BASE, { recursive: true });
- const pdfPath = path.join(TEMP_STORAGE_BASE, `${artefactId}.pdf`);
+ const sanitisedId = path.basename(artefactId);
+ const pdfPath = path.join(TEMP_STORAGE_BASE, `${sanitisedId}.pdf`);
await fs.writeFile(pdfPath, pdfBuffer);📝 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.
| export async function savePdfToStorage(artefactId: string, pdfBuffer: Buffer, sizeBytes: number): Promise<PdfGenerationResult> { | |
| const exceedsMaxSize = sizeBytes > MAX_PDF_SIZE_BYTES; | |
| await fs.mkdir(TEMP_STORAGE_BASE, { recursive: true }); | |
| const pdfPath = path.join(TEMP_STORAGE_BASE, `${artefactId}.pdf`); | |
| await fs.writeFile(pdfPath, pdfBuffer); | |
| return { | |
| success: true, | |
| pdfPath, | |
| sizeBytes, | |
| exceedsMaxSize | |
| }; | |
| } | |
| export async function savePdfToStorage(artefactId: string, pdfBuffer: Buffer, sizeBytes: number): Promise<PdfGenerationResult> { | |
| const exceedsMaxSize = sizeBytes > MAX_PDF_SIZE_BYTES; | |
| await fs.mkdir(TEMP_STORAGE_BASE, { recursive: true }); | |
| const sanitisedId = path.basename(artefactId); | |
| const pdfPath = path.join(TEMP_STORAGE_BASE, `${sanitisedId}.pdf`); | |
| await fs.writeFile(pdfPath, pdfBuffer); | |
| return { | |
| success: true, | |
| pdfPath, | |
| sizeBytes, | |
| exceedsMaxSize | |
| }; | |
| } |
| import { isValidEmail, type PublicationEvent, validatePublicationEvent } from "./validation.js"; | ||
| import { type PublicationEvent, validatePublicationEvent } from "./validation.js"; | ||
|
|
||
| const MAX_PDF_SIZE_BYTES = 2 * 1024 * 1024; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n 'MAX_PDF_SIZE_BYTES' --type=tsRepository: hmcts/cath-service
Length of output: 1202
🏁 Script executed:
# Check the specific file and lines mentioned
cat -n libs/list-types/common/src/index.ts | grep -A 2 -B 2 'MAX_PDF_SIZE_BYTES'Repository: hmcts/cath-service
Length of output: 210
🏁 Script executed:
# Verify the value in notification-service.ts
cat -n libs/notifications/src/notification/notification-service.ts | sed -n '35p'Repository: hmcts/cath-service
Length of output: 112
🏁 Script executed:
# Verify the current imports from `@hmcts/list-types-common` in notification-service.ts
sed -n '18p' libs/notifications/src/notification/notification-service.tsRepository: hmcts/cath-service
Length of output: 158
Remove duplicate constant and import from common package.
MAX_PDF_SIZE_BYTES is already exported from @hmcts/list-types-common. Remove the local definition at line 35 and add it to the import statement at line 18 to avoid maintaining the same value in two locations.
Suggested fix
Remove line 35:
-const MAX_PDF_SIZE_BYTES = 2 * 1024 * 1024;Update line 18:
-import { type CaseSummary, getListTypeName, type ListTypeName } from "@hmcts/list-types-common";
+import { type CaseSummary, getListTypeName, type ListTypeName, MAX_PDF_SIZE_BYTES } from "@hmcts/list-types-common";| interface PdfGenerationResult { | ||
| success: boolean; | ||
| pdfBuffer?: Buffer; | ||
| sizeBytes?: number; | ||
| error?: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
PdfGenerationResult is not exported and is duplicated in pdf-utilities.ts.
This interface is defined here but not exported, so consumers cannot reference the return type. Additionally, a separate PdfGenerationResult interface exists in libs/list-types/common/src/pdf/pdf-utilities.ts with different fields (pdfPath vs pdfBuffer). Consider exporting this one and having pdf-utilities.ts import or extend it to avoid drift.
| const page = await browser.newPage(); | ||
|
|
||
| await page.setContent(html, { | ||
| waitUntil: "networkidle0" | ||
| }); |
There was a problem hiding this comment.
No timeout on setContent with networkidle0 — potential hang.
If the provided HTML references external resources that never resolve, networkidle0 will wait indefinitely. Add a timeout to prevent the process from hanging.
Proposed fix
await page.setContent(html, {
- waitUntil: "networkidle0"
+ waitUntil: "networkidle0",
+ timeout: 30_000
});📝 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 page = await browser.newPage(); | |
| await page.setContent(html, { | |
| waitUntil: "networkidle0" | |
| }); | |
| const page = await browser.newPage(); | |
| await page.setContent(html, { | |
| waitUntil: "networkidle0", | |
| timeout: 30_000 | |
| }); |
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/list-types/court-of-appeal-civil-daily-cause-list/package.json (1)
37-42:⚠️ Potential issue | 🟡 MinorMissing
@types/nunjucksindevDependencies.
nunjucksis used in TypeScript source files (e.g.pdf-generator.ts), but@types/nunjucksis absent fromdevDependencies. Without it,tscwill either error or silently resolve the import asany.🛠️ Proposed fix
"devDependencies": { "@types/luxon": "3.7.1", "@types/node": "24.10.4", + "@types/nunjucks": "3.2.6", "typescript": "5.9.3", "vitest": "4.0.18" },
🧹 Nitpick comments (3)
libs/list-types/civil-and-family-daily-cause-list/package.json (1)
18-18:build:pdf-templatesglob may break on emptysrc/pdf/
cp src/pdf/*.njk dist/pdf/will error if the glob matches nothing (shell passes the literal string tocp, producing "No such file or directory"). The existingbuild:nunjucksscript usesfind … -execprecisely to avoid this. Aligning the new script with that pattern would make it equally resilient.♻️ Proposed fix
-"build:pdf-templates": "mkdir -p dist/pdf && cp src/pdf/*.njk dist/pdf/", +"build:pdf-templates": "mkdir -p dist/pdf && find src/pdf -name '*.njk' -exec sh -c 'cp {} dist/pdf/$(basename {})' \\;",libs/list-types/court-of-appeal-civil-daily-cause-list/package.json (1)
17-17: Fragilecdchaining and unsafe{}substitution inbuild:nunjucks.Two issues:
Fragile
cdchain — the sequencecd src/pages && find ... && cd ../.. && ... && cd src/pdfruns in a single shell process. If thefind -execblock exits non-zero,cd ../..is skipped and the subsequentcd src/pdfresolves against the wrong working directory, silently copying nothing (or crashing).
sh -c '... {} ...'anti-pattern — embedding{}directly inside the shell string rather than passing it as a positional argument breaks on filenames with spaces or special characters, and is flagged as a shell injection risk by most linters.Use subshells to isolate
cdand pass{}as a positional argument:♻️ Proposed fix
-"build:nunjucks": "mkdir -p dist/pages && cd src/pages && find . -name '*.njk' -exec sh -c 'mkdir -p ../../dist/pages/$(dirname {}) && cp {} ../../dist/pages/{}' \\; && cd ../.. && mkdir -p dist/pdf && cd src/pdf && find . -name '*.njk' -exec sh -c 'mkdir -p ../../dist/pdf/$(dirname {}) && cp {} ../../dist/pdf/{}' \\;", +"build:nunjucks": "mkdir -p dist/pages && (cd src/pages && find . -name '*.njk' -exec sh -c 'mkdir -p \"../../dist/pages/$(dirname \"$1\")\" && cp \"$1\" \"../../dist/pages/$1\"' _ {} \\;) && mkdir -p dist/pdf && (cd src/pdf && find . -name '*.njk' -exec sh -c 'mkdir -p \"../../dist/pdf/$(dirname \"$1\")\" && cp \"$1\" \"../../dist/pdf/$1\"' _ {} \\;)",libs/list-types/london-administrative-court-daily-cause-list/package.json (1)
17-17: LGTM — PDF template copy mirrors the existing pages pattern correctly.The
cd ../..navigation back to the package root before enteringsrc/pdfis correct, and../../dist/pdf/from withinsrc/pdfresolves as expected.Optionally, the two
find-based copy blocks could be extracted into a small helper shell script (e.g.,scripts/copy-njk.sh <src> <dest>) to reduce the one-liner length and make future additions trivial — but this is purely a readability improvement.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
libs/list-types/administrative-court-daily-cause-list/package.jsonlibs/list-types/care-standards-tribunal-weekly-hearing-list/package.jsonlibs/list-types/civil-and-family-daily-cause-list/package.jsonlibs/list-types/court-of-appeal-civil-daily-cause-list/package.jsonlibs/list-types/london-administrative-court-daily-cause-list/package.jsonlibs/list-types/rcj-standard-daily-cause-list/package.jsonpackage.json
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/list-types/administrative-court-daily-cause-list/package.json
- package.json
- libs/list-types/care-standards-tribunal-weekly-hearing-list/package.json
- libs/list-types/rcj-standard-daily-cause-list/package.json
| "exceljs": "4.4.0", | ||
| "luxon": "3.7.2" | ||
| "luxon": "3.7.2", | ||
| "nunjucks": "3.2.4" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual package.json file
cat -n libs/list-types/london-administrative-court-daily-cause-list/package.jsonRepository: hmcts/cath-service
Length of output: 1925
🏁 Script executed:
# Check if `@types/nunjucks` is declared anywhere in the workspace
rg "@types/nunjucks" libs/Repository: hmcts/cath-service
Length of output: 215
🏁 Script executed:
# Check if nunjucks is imported directly in source files
fd "\.ts$" libs/list-types/london-administrative-court-daily-cause-list/src --exec grep -l "nunjucks" {} \;Repository: hmcts/cath-service
Length of output: 44
🏁 Script executed:
# Verify `@hmcts/pdf-generation` package dependencies
cat -n libs/pdf-generation/package.json | grep -A 50 "devDependencies\|dependencies"Repository: hmcts/cath-service
Length of output: 254
Add @types/nunjucks to devDependencies for consistency with workspace pattern.
nunjucks is declared as a dependency but @types/nunjucks is missing from devDependencies. Other similar packages in the workspace (e.g., civil-and-family-daily-cause-list, web-core) include it. Although no direct nunjucks imports are currently present in source files, including the types aligns with project standards and protects against any-type fallback if nunjucks is imported.
Proposed addition
"devDependencies": {
+ "@types/nunjucks": "3.2.6",
"@types/luxon": "3.7.1",
4 participants
Jira link
https://tools.hmcts.net/jira/browse/VIBE-339
Change description
Add PDF and email summary for RCJ and Care Standards List
Checklist
Summary by CodeRabbit
New Features
Localization
Tests